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.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 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
