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

[Markus Armbruster]

Akihiko Odaki <akihiko.odaki@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.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,
>  };

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.