| Message ID | 20241022-virtio-v2-1-b2394236e053@daynix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | virtio: Convert feature properties to OnOffAuto | expand |
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. > + > /* --- 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.0 > With regards, Daniel
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. We can add a alternate type of OnOffAuto and bool to the QAPI schema, but this type is not used in QAPI and is unnecessary. Regards, Akihiko Odaki > >> + >> /* --- 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.0 >> > > With regards, > Daniel
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
On 2024/11/01 20:41, Daniel P. Berrangé wrote: > 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. My understanding of the present situation is different. OnOffAuto always requires to specify string "on" or "off", not a JSON bool type. I see the core problem is that QOM bool and OnOffAuto disagrees how "on" and "off" should be specified, especially in the JSON syntax. In JSON syntax, QOM bool requires JSON bool, which is inconsistent with OnOffAuto. If the added redundancy is not OK, I think we need to decide how "on" and "off" should be decided, and use the syntax for both of QOM bool and OnOffAuto while deprecating the old syntax. I don't have a strong opinion regarding the JSON syntax that should be used for "on" or "off". While I feel the JSON type system fits poor for QEMU, QEMU already have code that handles JSON numbers and null so using JSON true/false should be OK. Regards, Akihiko Odaki > > 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
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); +} + /* --- 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, };
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(-)