On 2025/02/06 0:29, Markus Armbruster wrote:
Akihiko Odaki <akihiko.od...@daynix.com> writes:
Accept bool literals for OnOffAuto properties for consistency with bool
properties. This enables users to set the "on" or "off" value in a
uniform syntax without knowing whether the "auto" value is accepted.
This behavior is especially useful when converting an existing bool
property to OnOffAuto or vice versa.
Signed-off-by: Akihiko Odaki <akihiko.od...@daynix.com>
---
hw/core/qdev-properties.c | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 434a76f5036e..0081d79f9b7b 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -491,6 +491,21 @@ const PropertyInfo qdev_prop_string = {
.set = set_string,
};
+static void set_on_off_auto(Object *obj, Visitor *v, const char *name,
+ void *opaque, Error **errp)
+{
+ Property *prop = opaque;
+ int *ptr = object_field_prop_ptr(obj, prop);
+ bool value;
+
+ if (visit_type_bool(v, name, &value, NULL)) {
+ *ptr = value ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
+ return;
+ }
+
+ qdev_propinfo_set_enum(obj, v, name, opaque, errp);
+}
+
/* --- on/off/auto --- */
const PropertyInfo qdev_prop_on_off_auto = {
@@ -498,7 +513,7 @@ const PropertyInfo qdev_prop_on_off_auto = {
.description = "on/off/auto",
.enum_table = &OnOffAuto_lookup,
.get = qdev_propinfo_get_enum,
- .set = qdev_propinfo_set_enum,
+ .set = set_on_off_auto,
.set_default_value = qdev_propinfo_set_default_value_enum,
};
The qdev properties defined with DEFINE_PROP_ON_OFF_AUTO() now
additionally accept bool.
The commit message tries to explain why this change is useful, but it
leaves me confused.
Does this solve a problem with existing properties? If yes, what
exactly is the problem?
Or does this enable new uses of DEFINE_PROP_ON_OFF_AUTO()?
I'm trying to understand, but my gut feeling is "bad idea".
Having multiple ways to express the same thing is generally undesirable.
In this case, "foo": "on" and "foo": true, as well as "foo": "off" and
"foo": false.
Moreover, OnOffAuto then has two meanings: straightfoward enum as
defined in the QAPI schema, and the funny qdev property. This is
definitely a bad idea. DEFINE_PROP_T(), where T is some QAPI type,
should accept *exactly* the values of T. If these properties need to
accept something else, use another name to not invite confusion.
If I understand the cover letter correctly, you want to make certain
bool properties tri-state for some reason. I haven't looked closely
enough to judge whether that makes sense. But do you really have to
change a whole bunch of unrelated properties to solve your problem?
This is going to be a very hard sell.
I change various virtio properties because they all have a common
problem. The problem is, when the host does not support a virtio
capability, virtio devices automatically set capability properties false
even if the user explicitly sets them true. This problem can be solved
using an existing mechanism, OnOffAuto, which differentiates the "auto"
state and explicit the "on" state.
However, converting bool to OnOffAuto surfaces another problem: they
disagree how "on" and "off" should be written. Please note that the
disagreement already exists and so it is nice to solve anyway.
This patch tries to solve it by tolerating bool values for OnOffAuto. As
you pointed out, this approach has a downside: it makes OnOffAuto more
complicated by having multiple ways to express the same thing.
Another approach is to have one unified way to express "on"/"off" for
bool and OnOffAuto. This will give three options in total:
1. Let OnOffAuto accept JSON bool and "on"/"off" (what this patch does)
2. Let OnOffAuto and bool accept JSON bool and deprecate "on"/"off"
3. Let OnOffAuto and bool accept "on"/"off" and deprecate JSON bool
I'm fine with either of these approaches; they are at least better than
the current situation where users need to care if the value is OnOffAuto
or bool when they just want to express on/off. Please tell me what you
prefer.
