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.odaki@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,
};
--
2.47.1Akihiko Odaki <akihiko.odaki@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.odaki@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.
On 2025/02/06 0:29, Markus Armbruster wrote:
> Akihiko Odaki <akihiko.odaki@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.odaki@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.
Akihiko Odaki <akihiko.odaki@daynix.com> writes:
> On 2025/02/06 0:29, Markus Armbruster wrote:
>> Akihiko Odaki <akihiko.odaki@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.odaki@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.
I understand we have something like this:
* true: on if possible, else off
* false: off (always possible)
Which one is the default?
There is no way to reliably configure "on", i.e. fail if it's not
possible. I agree that's a problem.
> This problem can be solved
> using an existing mechanism, OnOffAuto, which differentiates the "auto"
> state and explicit the "on" state.
I guess you're proposing something like this:
* auto: on if possible, else off
* on: on if possible, else error
* off: off (always possible)
Which one is the default?
> 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.
Yes, converting bool to OnOffAuto is an incompatible change.
> 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.
It also affects existing uses of OnOffAuto, where such a change is
unnecessary and undesirable.
> 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)
The parenthesis is inaccurate. This patch only affects qdev properties.
It does not affect use of OnOffAuto elsewhere, e.g. QOM object
"sev-guest" property "legacy-vm-type", or QMP command blockdev-add
argument "locking" with driver "file".
> 2. Let OnOffAuto and bool accept JSON bool and deprecate "on"/"off"
> 3. Let OnOffAuto and bool accept "on"/"off" and deprecate JSON bool
For each of these options:
(a) Change exactly the uses of OnOffAuto that need to become tri-state
(b) Change all qdev properties (currently a superset of (a); what this
patch does)
(c) Change all uses of OnOffAuto
I dislike (c) and especially (b).
> 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.
We managed to maneuver ourselves into a bit of a corner in just a few
simple steps:
* The obvious type for a flag is bool.
* The obvious type for a small set of values is enum.
* Thus, the obvious type for a tri-state is enum.
* But this prevents growing a flag into a tri-state compatibly. Which
is what you want to do.
However, we actually have a second way to do a tri-state: optional bool,
i.e. present and true, present and false, absent.
Permit me a digression... I'm not a fan of assigning "absent" a meaning
different from any present value. But it's a design choice QAPI made.
Using optional that way can occasionally lead to trouble. Consider
migrate-set-parameters. Its arguments are all optional. For each
argument present, the respective migration parameter is set to the
argument value. You cannot use this to reset a migration parameter from
present to absent. Matters for parameters where "absent" has a meaning
different from any "present" value.
End of digression.
Start of next digression :)
Note that qdev properties are generally optional. The only way to make
them mandatory is to reject their default value in .realize(). When
users set this default value explicitly, the error message will almost
certainly be confusing.
End of digression.
Optional bool may enable a fourth solution:
4. Make "absent" mean on if possible, else off, "present and true" mean
on if possible, else error, and "present and false" mean off (always
possible).
This changes the meaning of "present and true", but it's precisely
the change you want, isn't it?
Yet another solutions:
5. Alternate of bool and an enum with a single value "auto".
Falls apart with the keyval visitor used for the command line.
Fixable, I believe, but a good chunk of work and complexity.
My gut feeling: explore 4. first.
On 2025/02/06 18:48, Markus Armbruster wrote:
> Akihiko Odaki <akihiko.odaki@daynix.com> writes:
>
>> On 2025/02/06 0:29, Markus Armbruster wrote:
>>> Akihiko Odaki <akihiko.odaki@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.odaki@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.
First, I'd like to thank you for your detailed reply.
>
> I understand we have something like this:
>
> * true: on if possible, else off
>
> * false: off (always possible)
>
> Which one is the default?
It depends. Some properties have true by default. The others have false.
>
> There is no way to reliably configure "on", i.e. fail if it's not
> possible. I agree that's a problem.
>
>> This problem can be solved
>> using an existing mechanism, OnOffAuto, which differentiates the "auto"
>> state and explicit the "on" state.
>
> I guess you're proposing something like this:
>
> * auto: on if possible, else off
>
> * on: on if possible, else error
>
> * off: off (always possible)
>
> Which one is the default?
I converted on to auto and off to false in a following patch.
>
>> 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.
>
> Yes, converting bool to OnOffAuto is an incompatible change.
Not just about conversion, but this inconsistency require users to know
whether a property is bool or OnOffAuto and change how the values are
written in JSON accordingly. This somewhat hurts usability.
>
>> 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.
>
> It also affects existing uses of OnOffAuto, where such a change is
> unnecessary and undesirable.
>
>> 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)
>
> The parenthesis is inaccurate. This patch only affects qdev properties.
> It does not affect use of OnOffAuto elsewhere, e.g. QOM object
> "sev-guest" property "legacy-vm-type", or QMP command blockdev-add
> argument "locking" with driver "file".
>
>> 2. Let OnOffAuto and bool accept JSON bool and deprecate "on"/"off"
>> 3. Let OnOffAuto and bool accept "on"/"off" and deprecate JSON bool
>
> For each of these options:
>
> (a) Change exactly the uses of OnOffAuto that need to become tri-state
>
> (b) Change all qdev properties (currently a superset of (a); what this
> patch does)
>
> (c) Change all uses of OnOffAuto
>
> I dislike (c) and especially (b).
>
>> 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.
>
> We managed to maneuver ourselves into a bit of a corner in just a few
> simple steps:
>
> * The obvious type for a flag is bool.
>
> * The obvious type for a small set of values is enum.
>
> * Thus, the obvious type for a tri-state is enum.
>
> * But this prevents growing a flag into a tri-state compatibly. Which
> is what you want to do.
>
> However, we actually have a second way to do a tri-state: optional bool,
> i.e. present and true, present and false, absent.
>
> Permit me a digression... I'm not a fan of assigning "absent" a meaning
> different from any present value. But it's a design choice QAPI made.
It's a new insight I didn't know. Properties in qdev have a default
value instead of special "absent". But if QAPI does have special
"absent", perhaps qdev may be modified to align with.
>
> Using optional that way can occasionally lead to trouble. Consider
> migrate-set-parameters. Its arguments are all optional. For each
> argument present, the respective migration parameter is set to the
> argument value. You cannot use this to reset a migration parameter from
> present to absent. Matters for parameters where "absent" has a meaning
> different from any "present" value.
>
> End of digression.
>
> Start of next digression :)
>
> Note that qdev properties are generally optional. The only way to make
> them mandatory is to reject their default value in .realize(). When
> users set this default value explicitly, the error message will almost
> certainly be confusing.
>
> End of digression.
>
> Optional bool may enable a fourth solution:
>
> 4. Make "absent" mean on if possible, else off, "present and true" mean
> on if possible, else error, and "present and false" mean off (always
> possible).
>
> This changes the meaning of "present and true", but it's precisely
> the change you want, isn't it?
We have "false by default" properties so it unfortunately does not work.
>
> Yet another solutions:
>
> 5. Alternate of bool and an enum with a single value "auto".
>
> Falls apart with the keyval visitor used for the command line.
> Fixable, I believe, but a good chunk of work and complexity.
I may have missed something, but I think that will break JSON string
literals "on" and "off".
Regards,
Akihiko Odaki
>
> My gut feeling: explore 4. first.
>
On Thu, 6 Feb 2025, Akihiko Odaki wrote: > On 2025/02/06 18:48, Markus Armbruster wrote: >>> This problem can be solved >>> using an existing mechanism, OnOffAuto, which differentiates the "auto" >>> state and explicit the "on" state. >> >> I guess you're proposing something like this: >> >> * auto: on if possible, else off >> >> * on: on if possible, else error >> >> * off: off (always possible) >> >> Which one is the default? > > I converted on to auto and off to false in a following patch. > >> >>> 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. >> >> Yes, converting bool to OnOffAuto is an incompatible change. > > Not just about conversion, but this inconsistency require users to know > whether a property is bool or OnOffAuto and change how the values are written > in JSON accordingly. This somewhat hurts usability. Worse than that, the help text is also confusing. Excerpt from -device virtio-gpu,help blob=<bool> - on/off (default: false) busnr=<busnr> disable-legacy=<OnOffAuto> - on/off/auto (default: "auto") disable-modern=<bool> - (default: false) edid=<bool> - on/off (default: true) event_idx=<bool> - on/off (default: true) For bools it says on/off yet expects true/false. Wasn't originally true/on/1 and false/off/0 accepted for bools? Where that got lost? I think getting back that behaviour would be easiest for users. Users don't care if OnOffAuto is an enum internally and can't (or don't want to) remember if bools are written on/off or true/false so accepting these as synonyms would help users. Regads, BALATON Zoltan
BALATON Zoltan <balaton@eik.bme.hu> writes: > On Thu, 6 Feb 2025, Akihiko Odaki wrote: >> On 2025/02/06 18:48, Markus Armbruster wrote: >>>> This problem can be solved >>>> using an existing mechanism, OnOffAuto, which differentiates the "auto" >>>> state and explicit the "on" state. >>> >>> I guess you're proposing something like this: >>> >>> * auto: on if possible, else off >>> >>> * on: on if possible, else error >>> >>> * off: off (always possible) >>> >>> Which one is the default? >> >> I converted on to auto and off to false in a following patch. >> >>>> 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. >>> >>> Yes, converting bool to OnOffAuto is an incompatible change. >> >> Not just about conversion, but this inconsistency require users to know whether a property is bool or OnOffAuto and change how the values are written in JSON accordingly. This somewhat hurts usability. > > Worse than that, the help text is also confusing. > Excerpt from -device virtio-gpu,help > > blob=<bool> - on/off (default: false) > busnr=<busnr> > disable-legacy=<OnOffAuto> - on/off/auto (default: "auto") > disable-modern=<bool> - (default: false) > edid=<bool> - on/off (default: true) > event_idx=<bool> - on/off (default: true) > > For bools it says on/off yet expects true/false. Wasn't originally true/on/1 and false/off/0 accepted for bools? Where that got lost? I think getting back that behaviour would be easiest for users. Users don't care if OnOffAuto is an enum internally and can't (or don't want to) remember if bools are written on/off or true/false so accepting these as synonyms would help users. The help text printed by -device is about usage of -device, not about QMP. There, the preferred syntax for bool values is on/off, but yes/true/y/no/false/n are also accepted. I think that's a disgusting mess, but it's here to stay, so let's not argue this any further. Instead of "(default: true)" we should have "(default: on)". All of this is a digression from the topic at hand, which is QMP.
On 2025/02/06 22:23, BALATON Zoltan wrote: > On Thu, 6 Feb 2025, Akihiko Odaki wrote: >> On 2025/02/06 18:48, Markus Armbruster wrote: >>>> This problem can be solved >>>> using an existing mechanism, OnOffAuto, which differentiates the "auto" >>>> state and explicit the "on" state. >>> >>> I guess you're proposing something like this: >>> >>> * auto: on if possible, else off >>> >>> * on: on if possible, else error >>> >>> * off: off (always possible) >>> >>> Which one is the default? >> >> I converted on to auto and off to false in a following patch. >> >>> >>>> 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. >>> >>> Yes, converting bool to OnOffAuto is an incompatible change. >> >> Not just about conversion, but this inconsistency require users to >> know whether a property is bool or OnOffAuto and change how the values >> are written in JSON accordingly. This somewhat hurts usability. > > Worse than that, the help text is also confusing. > Excerpt from -device virtio-gpu,help > > blob=<bool> - on/off (default: false) > busnr=<busnr> > disable-legacy=<OnOffAuto> - on/off/auto (default: "auto") > disable-modern=<bool> - (default: false) > edid=<bool> - on/off (default: true) > event_idx=<bool> - on/off (default: true) > > For bools it says on/off yet expects true/false. Wasn't originally true/ > on/1 and false/off/0 accepted for bools? Where that got lost? I think > getting back that behaviour would be easiest for users. Users don't care > if OnOffAuto is an enum internally and can't (or don't want to) remember > if bools are written on/off or true/false so accepting these as synonyms > would help users. The help shows another problem: it mixes two different syntaxes. I sent a patch to fix the inconsistency in the help. Please review it for more detailed explanation and actual fix: https://lore.kernel.org/qemu-devel/20250207-bool-v1-1-5749d5d6df24@daynix.com Let me go back to the discussion of the bool/OnOffAuto problem below: The values the command line syntax accepts are on/yes/true/y and off/no/false/n. For the command line syntax, you can always use on/off whether the type is bool or OnOffAuto. In my opinion, it is still not good to reject yes/true/y and no/false/n for OnOffAuto; why do we suddenly reject them when the property gets the "auto" value? As you pointed out, the usage of enum is our internal concern and should not bother users. The situation is worse for JSON as there is no common literals that are compatible with both of bool and OnOffAuto, which forces users to remember the type. So I think this patch makes sense in terms of usability. Accepting multiple representations for one value is ugly, but it is better than exposing the ugliness to users. We should deprecate the representations except one if we really hate the ugliness. Regards, Akihiko Odaki
Akihiko Odaki <akihiko.odaki@daynix.com> writes: [...] > Let me go back to the discussion of the bool/OnOffAuto problem below: > > The values the command line syntax accepts are on/yes/true/y and off/no/false/n. > > For the command line syntax, you can always use on/off whether the type is bool or OnOffAuto. In my opinion, it is still not good to reject yes/true/y and no/false/n for OnOffAuto; why do we suddenly reject them when the property gets the "auto" value? As you pointed out, the usage of enum is our internal concern and should not bother users. The command line is a different mess. For better or worse (worse if you ask me), we added code to accept additional syntax for bool values. Doing the same for enums that happen to have some values that look boolean at a glance is in my opinion a terrible idea. We have at least two: OnOffAuto and OnOffSplit. But let's get back to QMP. > The situation is worse for JSON as there is no common literals that are compatible with both of bool and OnOffAuto, which forces users to remember the type. JSON is primarily for machines, and machines are very good at remembering the type. An argument can be made that OnOffAuto is problematic interface design. In fact, I made it; see "managed to maneuver ourselves into a bit of a corner" upthread. > So I think this patch makes sense in terms of usability. Accepting multiple representations for one value is ugly, but it is better than exposing the ugliness to users. We should deprecate the representations except one if we really hate the ugliness. I believe churn & complexity outweigh the benefits.
On Fri, Feb 07, 2025 at 01:31:47PM +0100, Markus Armbruster wrote: > Akihiko Odaki <akihiko.odaki@daynix.com> writes: > > [...] > > > Let me go back to the discussion of the bool/OnOffAuto problem below: > > > > The values the command line syntax accepts are on/yes/true/y and off/no/false/n. > > > > For the command line syntax, you can always use on/off whether the type is bool or OnOffAuto. In my opinion, it is still not good to reject yes/true/y and no/false/n for OnOffAuto; why do we suddenly reject them when the property gets the "auto" value? As you pointed out, the usage of enum is our internal concern and should not bother users. > > The command line is a different mess. > > For better or worse (worse if you ask me), we added code to accept > additional syntax for bool values. > > Doing the same for enums that happen to have some values that look > boolean at a glance is in my opinion a terrible idea. We have at least > two: OnOffAuto and OnOffSplit. > > But let's get back to QMP. Before we get back to QMP I should point out that our current HMP bool / OnOffAuto properties are a significant developer foot-gun in terms of back compat. Though I'm struggling to find the examples, I'm pretty sure I've seen patches where we converted a property from bool to OnOffAuto, with the developer (likely) thinking it was back-compatible. It does have the illusion of being compatible given that the HMP bool syntax is accepting 'on/off' (especially when our usage examples often on/off rather than yes/no/true/false) values ... very much not the case for QMP though. This does make me really want the idea of an "alternate" in QMP that can be made to work for scalars. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Wed, Jan 08, 2025 at 03:17:51PM +0900, Akihiko Odaki wrote:
> 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.
Again, to repeat my previous feedback, OnOffAuto is a well defined
QAPI type - making it secretly accept other values/types behind
the scenes which are not visible in QAPI scheme is not acceptable.
Effectively this is a backdoor impl of a QAPI alternate
{ 'alternate': 'OnOffAutoOrBool',
'data': {
'o': 'OnOffAuto',
'b': 'bool'
}
}
except this isn't permitted as the QAPI generator explicitly blocks
use of alternate when the two branches are 'bool' and 'enum'.
I'm assuming this is because in the QemuOpts scenario, it cannot
guess upfront whether the input is a bool or enum. This is unfortunate
though, because at the JSON visitor level it is unambiguous.
I wonder if the QAPI generator could be relaxed in any viable way ?
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@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,
> };
>
>
> --
> 2.47.1
>
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Daniel P. Berrangé <berrange@redhat.com> writes:
> On Wed, Jan 08, 2025 at 03:17:51PM +0900, Akihiko Odaki wrote:
>> 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.
>
> Again, to repeat my previous feedback, OnOffAuto is a well defined
> QAPI type - making it secretly accept other values/types behind
> the scenes which are not visible in QAPI scheme is not acceptable.
>
> Effectively this is a backdoor impl of a QAPI alternate
>
> { 'alternate': 'OnOffAutoOrBool',
> 'data': {
> 'o': 'OnOffAuto',
> 'b': 'bool'
> }
> }
>
> except this isn't permitted as the QAPI generator explicitly blocks
> use of alternate when the two branches are 'bool' and 'enum'.
>
> I'm assuming this is because in the QemuOpts scenario, it cannot
> guess upfront whether the input is a bool or enum. This is unfortunate
> though, because at the JSON visitor level it is unambiguous.
>
> I wonder if the QAPI generator could be relaxed in any viable way ?
Discussed in review of a related prior patch:
https://lore.kernel.org/qemu-devel/87h6c4fqz6.fsf@pond.sub.org/
Here's the relevant part for your convenience:
>> parse the value as enum, and if that fails, as uint32_t. QAPI already
>> provides a way to express "either this type or that type": alternate.
>> Like this:
>>
>> { 'alternate': 'OnOffAutoUint32',
>> 'data': { 'sym': 'OnOffAuto',
>> 'uint': 'uint32' } }
>>
>> Unfortunately, such alternates don't work on the command line due to
>> keyval visitor restrictions. These cannot be lifted entirely, but we
>> might be able to lift them sufficiently to make this alternate work.
>
> The keyval visitor cannot implement alternates because the command line
> input does not have type information. For example, you cannot
> distinguish string "0" and integer 0.
Correct.
For alternate types, an input visitor picks the branch based on the
QType.
With JSON, we have scalar types null, number, string, and bool.
With keyval, we only have string. Alternates with more than one scalar
branch don't work.
They could be made to work to some degree, though. Observe:
* Any value can be a string.
* "frob" can be nothing else.
* "on" and "off" can also be bool.
* "123" and "1e3" can also be number or enum.
Instead of picking the branch based on the QType, we could pick based on
QType and value, where the value part is delegated to a visitor method.
On 2025/01/10 20:09, Daniel P. Berrangé wrote:
> On Wed, Jan 08, 2025 at 03:17:51PM +0900, Akihiko Odaki wrote:
>> 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.
>
> Again, to repeat my previous feedback, OnOffAuto is a well defined
> QAPI type - making it secretly accept other values/types behind
> the scenes which are not visible in QAPI scheme is not acceptable.
>
> Effectively this is a backdoor impl of a QAPI alternate
>
> { 'alternate': 'OnOffAutoOrBool',
> 'data': {
> 'o': 'OnOffAuto',
> 'b': 'bool'
> }
> }
>
> except this isn't permitted as the QAPI generator explicitly blocks
> use of alternate when the two branches are 'bool' and 'enum'.
The QAPI generator specifically blocks the case where the enum contains
'on' or 'off'.
>
> I'm assuming this is because in the QemuOpts scenario, it cannot
> guess upfront whether the input is a bool or enum. This is unfortunate
> though, because at the JSON visitor level it is unambiguous.
It's probably for the command line and possibly HMP.
>
> I wonder if the QAPI generator could be relaxed in any viable way ?
It will make the interpretation of 'on' and 'off' on the command line
ambigious; it can be either of OnOffAuto or bool.
Making some sort of backdoor is necessary to support the proposed
semantics. We will need a new built-in type to represent the tristate
value; I haven't added one though as it's not necessary for virtio
properties I'm concerned of.
>
>>
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@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,
>> };
>>
>>
>> --
>> 2.47.1
>>
>
> With regards,
> Daniel
On Fri, Jan 10, 2025 at 08:31:57PM +0900, Akihiko Odaki wrote:
> On 2025/01/10 20:09, Daniel P. Berrangé wrote:
> > On Wed, Jan 08, 2025 at 03:17:51PM +0900, Akihiko Odaki wrote:
> > > 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.
> >
> > Again, to repeat my previous feedback, OnOffAuto is a well defined
> > QAPI type - making it secretly accept other values/types behind
> > the scenes which are not visible in QAPI scheme is not acceptable.
> >
> > Effectively this is a backdoor impl of a QAPI alternate
> >
> > { 'alternate': 'OnOffAutoOrBool',
> > 'data': {
> > 'o': 'OnOffAuto',
> > 'b': 'bool'
> > }
> > }
> >
> > except this isn't permitted as the QAPI generator explicitly blocks
> > use of alternate when the two branches are 'bool' and 'enum'.
>
> The QAPI generator specifically blocks the case where the enum contains 'on'
> or 'off'.
>
> >
> > I'm assuming this is because in the QemuOpts scenario, it cannot
> > guess upfront whether the input is a bool or enum. This is unfortunate
> > though, because at the JSON visitor level it is unambiguous.
>
> It's probably for the command line and possibly HMP.
>
> >
> > I wonder if the QAPI generator could be relaxed in any viable way ?
> It will make the interpretation of 'on' and 'off' on the command line
> ambigious; it can be either of OnOffAuto or bool.
The ambiguity would not neccessarily matter from a functional POV
though.
The consumer of an "OnOffAutoOrBool" field, would likely want to apply
logic to collapse it into just "OnOffAuto". As such, whether "on" is
parsed as a enum value or a bool value would have no functional
difference in the end. The OnOffAutoOrBool is essentially there to
just make sure we clearly express our input schema.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 2025/01/10 21:16, Daniel P. Berrangé wrote:
> On Fri, Jan 10, 2025 at 08:31:57PM +0900, Akihiko Odaki wrote:
>> On 2025/01/10 20:09, Daniel P. Berrangé wrote:
>>> On Wed, Jan 08, 2025 at 03:17:51PM +0900, Akihiko Odaki wrote:
>>>> 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.
>>>
>>> Again, to repeat my previous feedback, OnOffAuto is a well defined
>>> QAPI type - making it secretly accept other values/types behind
>>> the scenes which are not visible in QAPI scheme is not acceptable.
>>>
>>> Effectively this is a backdoor impl of a QAPI alternate
>>>
>>> { 'alternate': 'OnOffAutoOrBool',
>>> 'data': {
>>> 'o': 'OnOffAuto',
>>> 'b': 'bool'
>>> }
>>> }
>>>
>>> except this isn't permitted as the QAPI generator explicitly blocks
>>> use of alternate when the two branches are 'bool' and 'enum'.
>>
>> The QAPI generator specifically blocks the case where the enum contains 'on'
>> or 'off'.
>>
>>>
>>> I'm assuming this is because in the QemuOpts scenario, it cannot
>>> guess upfront whether the input is a bool or enum. This is unfortunate
>>> though, because at the JSON visitor level it is unambiguous.
>>
>> It's probably for the command line and possibly HMP.
>>
>>>
>>> I wonder if the QAPI generator could be relaxed in any viable way ?
>> It will make the interpretation of 'on' and 'off' on the command line
>> ambigious; it can be either of OnOffAuto or bool.
>
> The ambiguity would not neccessarily matter from a functional POV
> though.
>
> The consumer of an "OnOffAutoOrBool" field, would likely want to apply
> logic to collapse it into just "OnOffAuto". As such, whether "on" is
> parsed as a enum value or a bool value would have no functional
> difference in the end. The OnOffAutoOrBool is essentially there to
> just make sure we clearly express our input schema.
I think it is worth creating a special type that requires the logic to
collapse into OnOffAuto.
If we don't have a special type, the QAPI generator will write C struct
like as follows:
struct OnOffAutoOrBool {
QType type;
union { /* union tag is @type */
OnOffAuto o;
bool b;
} u;
};
It's easier if we have a special type that directly maps into a
tristate. The story shouldn't be different for other possible consumers;
a special type will make it easy to map QAPI types into
consumer-specific type systems.
Regards,
Akihiko Odaki
© 2016 - 2025 Red Hat, Inc.