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.
