Re: [PATCH v2 1/3] qdev-properties: Accept bool for OnOffAuto

[Daniel P . Berrangé]

On Thu, Oct 31, 2024 at 04:21:08PM +0900, Akihiko Odaki wrote:
> On 2024/10/29 1:49, Daniel P. Berrangé wrote:
> > The parent msg was sent off-list orignially, so below is a copy
> > of my feedback to that off-list posting.
> > 
> > On Tue, Oct 22, 2024 at 01:50:38PM +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.
> > > 
> > > 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 86a583574dd0..f0a270bb4f61 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);
> > > +}
> > 
> > My feedback is the same as last time this was posted.
> > 
> > This is adding redundant new input-only & secret syntax for every
> > usage of OnOffAuto across QEMU.
> > 
> > "consistency with bool" isn't a expressing a compelling advantage.
> > 
> > The new permitted values are invisible to applications, beacuse
> > introspecting QAPI schema for the "OnOffAuto" type will never
> > report them, and querying the value of a property will also never
> > report them.
> > 
> > I'm not seeing an advantage, or clear problem solved, by adding
> > this.
> 
> The intent of this patch is to ease migration from bool to OnOffAuto; a user
> should be able to set the "on" or "off" value without knowing the "auto"
> value is accepted.
> 
> The redundancy of syntax is already present with bool. If it is problematic,
> the redundant syntax should be deprecated altogether, whether the type is
> OnOffAuto or bool.

The redundant syntax for bool is only present in the legacy QemuOpts
based CLI syntax. If using modern JSON syntax, or QMP, it is required
to use the navtive JSON bool type.

This proposed change to OnOffAuto is affecting both legacy and modern
syntax, adding redundancy to both, here none currently exists for the
latter. So this is qualatively different from the status quo, and
taking us in a direction we don't want to go.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|