Re: [PATCH v4 2/4] qdev-properties: Accept bool for OnOffAuto

[Akihiko Odaki <akihiko.odaki@daynix.com>]

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.
>