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

[Akihiko Odaki]

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