Akihiko Odaki <akihiko.od...@daynix.com> writes:
> 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.
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.
