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

[Markus Armbruster <armbru@redhat.com>]

Akihiko Odaki <akihiko.odaki@daynix.com> writes:

> On 2025/02/06 18:48, Markus Armbruster wrote:
>> Akihiko Odaki <akihiko.odaki@daynix.com> writes:

[...]

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

To be clear: this is pretty much a deal-breaker for me.

We established above that you need certain boolean properties to take a
third state.  I'm willing to discuss patches that change exactly these
properties.  I'm going to reject patches that affect properties that do
not need such a change.

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

Nothing stops you from creating qdev properties with a special "absent"
value.  All you need is a special value that cannot be set.

In fact, the humble "str" property already works that way: it's a char *
where null means "absent".

Code can recognize "absent" and do whatever needs doing then.  For
instance, consider device "ide-cd".  It has three such properties:
"ver", "serial", and "model".  "ver" defaults to "2.5+", "serial" to
some unique string, but "model" defaults to NULL.  Since you cannot set
such a value, it effectively means "absent".  The code responsible for
this is in ide_dev_initfn():

    if (!dev->version) {
        dev->version = g_strdup(s->version);
    }
    if (!dev->serial) {
        dev->serial = g_strdup(s->drive_serial_str);
    }

Note it leaves a null dev->model null.

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

Then make the code make "absent" mean what you need it to mean.  Just
like the code from ide_dev_initfn() I quoted above.

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

Unbreaking it will be a good chunk of work and complexity, I believe.

>> My gut feeling: explore 4. first.