Akihiko Odaki <akihiko.od...@daynix.com> writes:
> On 2025/02/06 18:48, Markus Armbruster wrote:
>> Akihiko Odaki <akihiko.od...@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.
