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