From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Simplify qom_set by making it use qmp_qom_set and the JSON parser.
(qemu) qom-get /machine smm
"auto"
(qemu) qom-set /machine smm "auto"
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Message-Id: <20200520151108.160598-3-dgilbert@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
With 's'->'S' type change suggested by Paolo and Markus
---
hmp-commands.hx | 2 +-
qom/qom-hmp-cmds.c | 16 +++++-----------
2 files changed, 6 insertions(+), 12 deletions(-)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 250ddae54d..28256209b5 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1806,7 +1806,7 @@ ERST
{
.name = "qom-set",
- .args_type = "path:s,property:s,value:s",
+ .args_type = "path:s,property:s,value:S",
.params = "path property value",
.help = "set QOM property",
.cmd = hmp_qom_set,
diff --git a/qom/qom-hmp-cmds.c b/qom/qom-hmp-cmds.c
index a8b0a080c7..f704b6949a 100644
--- a/qom/qom-hmp-cmds.c
+++ b/qom/qom-hmp-cmds.c
@@ -48,19 +48,13 @@ void hmp_qom_set(Monitor *mon, const QDict *qdict)
const char *property = qdict_get_str(qdict, "property");
const char *value = qdict_get_str(qdict, "value");
Error *err = NULL;
- bool ambiguous = false;
- Object *obj;
+ QObject *obj;
- obj = object_resolve_path(path, &ambiguous);
- if (obj == NULL) {
- error_set(&err, ERROR_CLASS_DEVICE_NOT_FOUND,
- "Device '%s' not found", path);
- } else {
- if (ambiguous) {
- monitor_printf(mon, "Warning: Path '%s' is ambiguous\n", path);
- }
- object_property_parse(obj, value, property, &err);
+ obj = qobject_from_json(value, &err);
+ if (err == NULL) {
+ qmp_qom_set(path, property, obj, &err);
}
+
hmp_handle_error(mon, err);
}
--
2.26.2
"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes: > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > Simplify qom_set by making it use qmp_qom_set and the JSON parser. > > (qemu) qom-get /machine smm > "auto" > (qemu) qom-set /machine smm "auto" > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > Message-Id: <20200520151108.160598-3-dgilbert@redhat.com> > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > Reviewed-by: Markus Armbruster <armbru@redhat.com> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > With 's'->'S' type change suggested by Paolo and Markus This is actually more than just simplification, it's disarming a bear trap: the string visitor is restricted to a subset of the QAPI types, and when you qom-set a property with a type it can't handle, QEMU aborts. I mentioned this in the discussion of possible ways out of the qom-get impasse, but missed reraising it in patch review. A suitably amended commit would be nice, but respinning the PR just for that may not be worthwhile.
* Markus Armbruster (armbru@redhat.com) wrote: > "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes: > > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > Simplify qom_set by making it use qmp_qom_set and the JSON parser. > > > > (qemu) qom-get /machine smm > > "auto" > > (qemu) qom-set /machine smm "auto" > > > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > Message-Id: <20200520151108.160598-3-dgilbert@redhat.com> > > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > Reviewed-by: Markus Armbruster <armbru@redhat.com> > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > With 's'->'S' type change suggested by Paolo and Markus > > This is actually more than just simplification, it's disarming a bear > trap: the string visitor is restricted to a subset of the QAPI types, > and when you qom-set a property with a type it can't handle, QEMU > aborts. I mentioned this in the discussion of possible ways out of the > qom-get impasse, but missed reraising it in patch review. > > A suitably amended commit would be nice, but respinning the PR just for > that may not be worthwhile. A bit late; still as long as we're removing bear traps not adding them. Dave -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 02.06.20 11:26, Dr. David Alan Gilbert wrote: > * Markus Armbruster (armbru@redhat.com) wrote: >> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes: >> >>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> >>> >>> Simplify qom_set by making it use qmp_qom_set and the JSON parser. >>> >>> (qemu) qom-get /machine smm >>> "auto" >>> (qemu) qom-set /machine smm "auto" >>> >>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> >>> Message-Id: <20200520151108.160598-3-dgilbert@redhat.com> >>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>> Reviewed-by: Markus Armbruster <armbru@redhat.com> >>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> >>> With 's'->'S' type change suggested by Paolo and Markus >> >> This is actually more than just simplification, it's disarming a bear >> trap: the string visitor is restricted to a subset of the QAPI types, >> and when you qom-set a property with a type it can't handle, QEMU >> aborts. I mentioned this in the discussion of possible ways out of the >> qom-get impasse, but missed reraising it in patch review. >> >> A suitably amended commit would be nice, but respinning the PR just for >> that may not be worthwhile. > > A bit late; still as long as we're removing bear traps not adding them. This breaks qom-set for my (virtio-mem) use case: echo "qom-set vm0 requested-size 300M" | sudo nc -U /var/tmp/mon_src QEMU 5.0.50 monitor - type 'help' for more information (qemu) qom-set vm0 requested-size 300M Error: Expecting at most one JSON value -- Thanks, David / dhildenb
* David Hildenbrand (david@redhat.com) wrote: > On 02.06.20 11:26, Dr. David Alan Gilbert wrote: > > * Markus Armbruster (armbru@redhat.com) wrote: > >> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes: > >> > >>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > >>> > >>> Simplify qom_set by making it use qmp_qom_set and the JSON parser. > >>> > >>> (qemu) qom-get /machine smm > >>> "auto" > >>> (qemu) qom-set /machine smm "auto" > >>> > >>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > >>> Message-Id: <20200520151108.160598-3-dgilbert@redhat.com> > >>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > >>> Reviewed-by: Markus Armbruster <armbru@redhat.com> > >>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > >>> With 's'->'S' type change suggested by Paolo and Markus > >> > >> This is actually more than just simplification, it's disarming a bear > >> trap: the string visitor is restricted to a subset of the QAPI types, > >> and when you qom-set a property with a type it can't handle, QEMU > >> aborts. I mentioned this in the discussion of possible ways out of the > >> qom-get impasse, but missed reraising it in patch review. > >> > >> A suitably amended commit would be nice, but respinning the PR just for > >> that may not be worthwhile. > > > > A bit late; still as long as we're removing bear traps not adding them. > > This breaks qom-set for my (virtio-mem) use case: > > echo "qom-set vm0 requested-size 300M" | sudo nc -U /var/tmp/mon_src > QEMU 5.0.50 monitor - type 'help' for more information > (qemu) qom-set vm0 requested-size 300M > Error: Expecting at most one JSON value Does qom-set vm0 requested-size 300e6 do the same thing? Dave > > -- > Thanks, > > David / dhildenb -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 03.06.20 12:43, Dr. David Alan Gilbert wrote: > * David Hildenbrand (david@redhat.com) wrote: >> On 02.06.20 11:26, Dr. David Alan Gilbert wrote: >>> * Markus Armbruster (armbru@redhat.com) wrote: >>>> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes: >>>> >>>>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> >>>>> >>>>> Simplify qom_set by making it use qmp_qom_set and the JSON parser. >>>>> >>>>> (qemu) qom-get /machine smm >>>>> "auto" >>>>> (qemu) qom-set /machine smm "auto" >>>>> >>>>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> >>>>> Message-Id: <20200520151108.160598-3-dgilbert@redhat.com> >>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>>>> Reviewed-by: Markus Armbruster <armbru@redhat.com> >>>>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> >>>>> With 's'->'S' type change suggested by Paolo and Markus >>>> >>>> This is actually more than just simplification, it's disarming a bear >>>> trap: the string visitor is restricted to a subset of the QAPI types, >>>> and when you qom-set a property with a type it can't handle, QEMU >>>> aborts. I mentioned this in the discussion of possible ways out of the >>>> qom-get impasse, but missed reraising it in patch review. >>>> >>>> A suitably amended commit would be nice, but respinning the PR just for >>>> that may not be worthwhile. >>> >>> A bit late; still as long as we're removing bear traps not adding them. >> >> This breaks qom-set for my (virtio-mem) use case: >> >> echo "qom-set vm0 requested-size 300M" | sudo nc -U /var/tmp/mon_src >> QEMU 5.0.50 monitor - type 'help' for more information >> (qemu) qom-set vm0 requested-size 300M >> Error: Expecting at most one JSON value > > Does qom-set vm0 requested-size 300e6 do the same thing? The property is defined to be of type "size". (qemu) qom-set vm0 requested-size 300e6 Error: Parameter 'requested-size' expects uint64 (not sure how "size" and "uint64" are mapped here) -- Thanks, David / dhildenb
On 03.06.20 13:31, David Hildenbrand wrote: > On 03.06.20 12:43, Dr. David Alan Gilbert wrote: >> * David Hildenbrand (david@redhat.com) wrote: >>> On 02.06.20 11:26, Dr. David Alan Gilbert wrote: >>>> * Markus Armbruster (armbru@redhat.com) wrote: >>>>> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes: >>>>> >>>>>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> >>>>>> >>>>>> Simplify qom_set by making it use qmp_qom_set and the JSON parser. >>>>>> >>>>>> (qemu) qom-get /machine smm >>>>>> "auto" >>>>>> (qemu) qom-set /machine smm "auto" >>>>>> >>>>>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> >>>>>> Message-Id: <20200520151108.160598-3-dgilbert@redhat.com> >>>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>>>>> Reviewed-by: Markus Armbruster <armbru@redhat.com> >>>>>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> >>>>>> With 's'->'S' type change suggested by Paolo and Markus >>>>> >>>>> This is actually more than just simplification, it's disarming a bear >>>>> trap: the string visitor is restricted to a subset of the QAPI types, >>>>> and when you qom-set a property with a type it can't handle, QEMU >>>>> aborts. I mentioned this in the discussion of possible ways out of the >>>>> qom-get impasse, but missed reraising it in patch review. >>>>> >>>>> A suitably amended commit would be nice, but respinning the PR just for >>>>> that may not be worthwhile. >>>> >>>> A bit late; still as long as we're removing bear traps not adding them. >>> >>> This breaks qom-set for my (virtio-mem) use case: >>> >>> echo "qom-set vm0 requested-size 300M" | sudo nc -U /var/tmp/mon_src >>> QEMU 5.0.50 monitor - type 'help' for more information >>> (qemu) qom-set vm0 requested-size 300M >>> Error: Expecting at most one JSON value >> >> Does qom-set vm0 requested-size 300e6 do the same thing? > > The property is defined to be of type "size". > > (qemu) qom-set vm0 requested-size 300e6 > Error: Parameter 'requested-size' expects uint64 > > (not sure how "size" and "uint64" are mapped here) > Oh, and before this commit, your example does work as well (well, fails later because the number is not properly aligned, but passes the parsing stage). -- Thanks, David / dhildenb
* David Hildenbrand (david@redhat.com) wrote: > On 03.06.20 12:43, Dr. David Alan Gilbert wrote: > > * David Hildenbrand (david@redhat.com) wrote: > >> On 02.06.20 11:26, Dr. David Alan Gilbert wrote: > >>> * Markus Armbruster (armbru@redhat.com) wrote: > >>>> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes: > >>>> > >>>>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > >>>>> > >>>>> Simplify qom_set by making it use qmp_qom_set and the JSON parser. > >>>>> > >>>>> (qemu) qom-get /machine smm > >>>>> "auto" > >>>>> (qemu) qom-set /machine smm "auto" > >>>>> > >>>>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > >>>>> Message-Id: <20200520151108.160598-3-dgilbert@redhat.com> > >>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > >>>>> Reviewed-by: Markus Armbruster <armbru@redhat.com> > >>>>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > >>>>> With 's'->'S' type change suggested by Paolo and Markus > >>>> > >>>> This is actually more than just simplification, it's disarming a bear > >>>> trap: the string visitor is restricted to a subset of the QAPI types, > >>>> and when you qom-set a property with a type it can't handle, QEMU > >>>> aborts. I mentioned this in the discussion of possible ways out of the > >>>> qom-get impasse, but missed reraising it in patch review. > >>>> > >>>> A suitably amended commit would be nice, but respinning the PR just for > >>>> that may not be worthwhile. > >>> > >>> A bit late; still as long as we're removing bear traps not adding them. > >> > >> This breaks qom-set for my (virtio-mem) use case: > >> > >> echo "qom-set vm0 requested-size 300M" | sudo nc -U /var/tmp/mon_src > >> QEMU 5.0.50 monitor - type 'help' for more information > >> (qemu) qom-set vm0 requested-size 300M > >> Error: Expecting at most one JSON value > > > > Does qom-set vm0 requested-size 300e6 do the same thing? > > The property is defined to be of type "size". > > (qemu) qom-set vm0 requested-size 300e6 > Error: Parameter 'requested-size' expects uint64 > > (not sure how "size" and "uint64" are mapped here) I think the problem here is that the JSON parser is converting anything with an 'e' as a float; JSON itself doesn't have the distinction between int and float. Dave > > -- > Thanks, > > David / dhildenb -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 03.06.20 13:43, Dr. David Alan Gilbert wrote: > * David Hildenbrand (david@redhat.com) wrote: >> On 03.06.20 12:43, Dr. David Alan Gilbert wrote: >>> * David Hildenbrand (david@redhat.com) wrote: >>>> On 02.06.20 11:26, Dr. David Alan Gilbert wrote: >>>>> * Markus Armbruster (armbru@redhat.com) wrote: >>>>>> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes: >>>>>> >>>>>>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> >>>>>>> >>>>>>> Simplify qom_set by making it use qmp_qom_set and the JSON parser. >>>>>>> >>>>>>> (qemu) qom-get /machine smm >>>>>>> "auto" >>>>>>> (qemu) qom-set /machine smm "auto" >>>>>>> >>>>>>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> >>>>>>> Message-Id: <20200520151108.160598-3-dgilbert@redhat.com> >>>>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>>>>>> Reviewed-by: Markus Armbruster <armbru@redhat.com> >>>>>>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> >>>>>>> With 's'->'S' type change suggested by Paolo and Markus >>>>>> >>>>>> This is actually more than just simplification, it's disarming a bear >>>>>> trap: the string visitor is restricted to a subset of the QAPI types, >>>>>> and when you qom-set a property with a type it can't handle, QEMU >>>>>> aborts. I mentioned this in the discussion of possible ways out of the >>>>>> qom-get impasse, but missed reraising it in patch review. >>>>>> >>>>>> A suitably amended commit would be nice, but respinning the PR just for >>>>>> that may not be worthwhile. >>>>> >>>>> A bit late; still as long as we're removing bear traps not adding them. >>>> >>>> This breaks qom-set for my (virtio-mem) use case: >>>> >>>> echo "qom-set vm0 requested-size 300M" | sudo nc -U /var/tmp/mon_src >>>> QEMU 5.0.50 monitor - type 'help' for more information >>>> (qemu) qom-set vm0 requested-size 300M >>>> Error: Expecting at most one JSON value >>> >>> Does qom-set vm0 requested-size 300e6 do the same thing? >> >> The property is defined to be of type "size". >> >> (qemu) qom-set vm0 requested-size 300e6 >> Error: Parameter 'requested-size' expects uint64 >> >> (not sure how "size" and "uint64" are mapped here) > > I think the problem here is that the JSON parser is converting anything > with an 'e' as a float; JSON itself doesn't have the distinction > between int and float. > (and just to clarify - I assume you are aware - 300e6 != 300M. So the interface becomes way harder to use in case one wants to specify properly aligned sizes - 300M vs 314572800) -- Thanks, David / dhildenb
* David Hildenbrand (david@redhat.com) wrote: > On 03.06.20 13:43, Dr. David Alan Gilbert wrote: > > * David Hildenbrand (david@redhat.com) wrote: > >> On 03.06.20 12:43, Dr. David Alan Gilbert wrote: > >>> * David Hildenbrand (david@redhat.com) wrote: > >>>> On 02.06.20 11:26, Dr. David Alan Gilbert wrote: > >>>>> * Markus Armbruster (armbru@redhat.com) wrote: > >>>>>> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes: > >>>>>> > >>>>>>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > >>>>>>> > >>>>>>> Simplify qom_set by making it use qmp_qom_set and the JSON parser. > >>>>>>> > >>>>>>> (qemu) qom-get /machine smm > >>>>>>> "auto" > >>>>>>> (qemu) qom-set /machine smm "auto" > >>>>>>> > >>>>>>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > >>>>>>> Message-Id: <20200520151108.160598-3-dgilbert@redhat.com> > >>>>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > >>>>>>> Reviewed-by: Markus Armbruster <armbru@redhat.com> > >>>>>>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > >>>>>>> With 's'->'S' type change suggested by Paolo and Markus > >>>>>> > >>>>>> This is actually more than just simplification, it's disarming a bear > >>>>>> trap: the string visitor is restricted to a subset of the QAPI types, > >>>>>> and when you qom-set a property with a type it can't handle, QEMU > >>>>>> aborts. I mentioned this in the discussion of possible ways out of the > >>>>>> qom-get impasse, but missed reraising it in patch review. > >>>>>> > >>>>>> A suitably amended commit would be nice, but respinning the PR just for > >>>>>> that may not be worthwhile. > >>>>> > >>>>> A bit late; still as long as we're removing bear traps not adding them. > >>>> > >>>> This breaks qom-set for my (virtio-mem) use case: > >>>> > >>>> echo "qom-set vm0 requested-size 300M" | sudo nc -U /var/tmp/mon_src > >>>> QEMU 5.0.50 monitor - type 'help' for more information > >>>> (qemu) qom-set vm0 requested-size 300M > >>>> Error: Expecting at most one JSON value > >>> > >>> Does qom-set vm0 requested-size 300e6 do the same thing? > >> > >> The property is defined to be of type "size". > >> > >> (qemu) qom-set vm0 requested-size 300e6 > >> Error: Parameter 'requested-size' expects uint64 > >> > >> (not sure how "size" and "uint64" are mapped here) > > > > I think the problem here is that the JSON parser is converting anything > > with an 'e' as a float; JSON itself doesn't have the distinction > > between int and float. > > > > (and just to clarify - I assume you are aware - 300e6 != 300M. So the > interface becomes way harder to use in case one wants to specify > properly aligned sizes - 300M vs 314572800) Oops, yes, good point. I think on balance it's probably best that this keeps supporting JSON; although tbh I'm not convinced there are any complex types that can be set. I'm not seeing a prettier answer. Dave > > -- > Thanks, > > David / dhildenb -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 03.06.20 14:24, Dr. David Alan Gilbert wrote: > * David Hildenbrand (david@redhat.com) wrote: >> On 03.06.20 13:43, Dr. David Alan Gilbert wrote: >>> * David Hildenbrand (david@redhat.com) wrote: >>>> On 03.06.20 12:43, Dr. David Alan Gilbert wrote: >>>>> * David Hildenbrand (david@redhat.com) wrote: >>>>>> On 02.06.20 11:26, Dr. David Alan Gilbert wrote: >>>>>>> * Markus Armbruster (armbru@redhat.com) wrote: >>>>>>>> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes: >>>>>>>> >>>>>>>>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> >>>>>>>>> >>>>>>>>> Simplify qom_set by making it use qmp_qom_set and the JSON parser. >>>>>>>>> >>>>>>>>> (qemu) qom-get /machine smm >>>>>>>>> "auto" >>>>>>>>> (qemu) qom-set /machine smm "auto" >>>>>>>>> >>>>>>>>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> >>>>>>>>> Message-Id: <20200520151108.160598-3-dgilbert@redhat.com> >>>>>>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>>>>>>>> Reviewed-by: Markus Armbruster <armbru@redhat.com> >>>>>>>>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> >>>>>>>>> With 's'->'S' type change suggested by Paolo and Markus >>>>>>>> >>>>>>>> This is actually more than just simplification, it's disarming a bear >>>>>>>> trap: the string visitor is restricted to a subset of the QAPI types, >>>>>>>> and when you qom-set a property with a type it can't handle, QEMU >>>>>>>> aborts. I mentioned this in the discussion of possible ways out of the >>>>>>>> qom-get impasse, but missed reraising it in patch review. >>>>>>>> >>>>>>>> A suitably amended commit would be nice, but respinning the PR just for >>>>>>>> that may not be worthwhile. >>>>>>> >>>>>>> A bit late; still as long as we're removing bear traps not adding them. >>>>>> >>>>>> This breaks qom-set for my (virtio-mem) use case: >>>>>> >>>>>> echo "qom-set vm0 requested-size 300M" | sudo nc -U /var/tmp/mon_src >>>>>> QEMU 5.0.50 monitor - type 'help' for more information >>>>>> (qemu) qom-set vm0 requested-size 300M >>>>>> Error: Expecting at most one JSON value >>>>> >>>>> Does qom-set vm0 requested-size 300e6 do the same thing? >>>> >>>> The property is defined to be of type "size". >>>> >>>> (qemu) qom-set vm0 requested-size 300e6 >>>> Error: Parameter 'requested-size' expects uint64 >>>> >>>> (not sure how "size" and "uint64" are mapped here) >>> >>> I think the problem here is that the JSON parser is converting anything >>> with an 'e' as a float; JSON itself doesn't have the distinction >>> between int and float. >>> >> >> (and just to clarify - I assume you are aware - 300e6 != 300M. So the >> interface becomes way harder to use in case one wants to specify >> properly aligned sizes - 300M vs 314572800) > > Oops, yes, good point. > > I think on balance it's probably best that this keeps supporting JSON; > although tbh I'm not convinced there are any complex types that can be > set. > I'm not seeing a prettier answer. So, I have to use a calculator from now on to set a property that I can set on the QEMU cmdline just fine without it? :( This feels like a step backwards, @Markus any way to keep supporting sizes? -- Thanks, David / dhildenb
On 03.06.20 14:26, David Hildenbrand wrote: > On 03.06.20 14:24, Dr. David Alan Gilbert wrote: >> * David Hildenbrand (david@redhat.com) wrote: >>> On 03.06.20 13:43, Dr. David Alan Gilbert wrote: >>>> * David Hildenbrand (david@redhat.com) wrote: >>>>> On 03.06.20 12:43, Dr. David Alan Gilbert wrote: >>>>>> * David Hildenbrand (david@redhat.com) wrote: >>>>>>> On 02.06.20 11:26, Dr. David Alan Gilbert wrote: >>>>>>>> * Markus Armbruster (armbru@redhat.com) wrote: >>>>>>>>> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes: >>>>>>>>> >>>>>>>>>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> >>>>>>>>>> >>>>>>>>>> Simplify qom_set by making it use qmp_qom_set and the JSON parser. >>>>>>>>>> >>>>>>>>>> (qemu) qom-get /machine smm >>>>>>>>>> "auto" >>>>>>>>>> (qemu) qom-set /machine smm "auto" >>>>>>>>>> >>>>>>>>>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> >>>>>>>>>> Message-Id: <20200520151108.160598-3-dgilbert@redhat.com> >>>>>>>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>>>>>>>>> Reviewed-by: Markus Armbruster <armbru@redhat.com> >>>>>>>>>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> >>>>>>>>>> With 's'->'S' type change suggested by Paolo and Markus >>>>>>>>> >>>>>>>>> This is actually more than just simplification, it's disarming a bear >>>>>>>>> trap: the string visitor is restricted to a subset of the QAPI types, >>>>>>>>> and when you qom-set a property with a type it can't handle, QEMU >>>>>>>>> aborts. I mentioned this in the discussion of possible ways out of the >>>>>>>>> qom-get impasse, but missed reraising it in patch review. >>>>>>>>> >>>>>>>>> A suitably amended commit would be nice, but respinning the PR just for >>>>>>>>> that may not be worthwhile. >>>>>>>> >>>>>>>> A bit late; still as long as we're removing bear traps not adding them. >>>>>>> >>>>>>> This breaks qom-set for my (virtio-mem) use case: >>>>>>> >>>>>>> echo "qom-set vm0 requested-size 300M" | sudo nc -U /var/tmp/mon_src >>>>>>> QEMU 5.0.50 monitor - type 'help' for more information >>>>>>> (qemu) qom-set vm0 requested-size 300M >>>>>>> Error: Expecting at most one JSON value >>>>>> >>>>>> Does qom-set vm0 requested-size 300e6 do the same thing? >>>>> >>>>> The property is defined to be of type "size". >>>>> >>>>> (qemu) qom-set vm0 requested-size 300e6 >>>>> Error: Parameter 'requested-size' expects uint64 >>>>> >>>>> (not sure how "size" and "uint64" are mapped here) >>>> >>>> I think the problem here is that the JSON parser is converting anything >>>> with an 'e' as a float; JSON itself doesn't have the distinction >>>> between int and float. >>>> >>> >>> (and just to clarify - I assume you are aware - 300e6 != 300M. So the >>> interface becomes way harder to use in case one wants to specify >>> properly aligned sizes - 300M vs 314572800) >> >> Oops, yes, good point. >> >> I think on balance it's probably best that this keeps supporting JSON; >> although tbh I'm not convinced there are any complex types that can be >> set. >> I'm not seeing a prettier answer. > > So, I have to use a calculator from now on to set a property that I can > set on the QEMU cmdline just fine without it? :( > > This feels like a step backwards, @Markus any way to keep supporting sizes? Or what about adding qom-set-json instead for complex types instead of changing the behavior if an existing interface? -- Thanks, David / dhildenb
* David Hildenbrand (david@redhat.com) wrote: > On 03.06.20 14:26, David Hildenbrand wrote: > > On 03.06.20 14:24, Dr. David Alan Gilbert wrote: > >> * David Hildenbrand (david@redhat.com) wrote: > >>> On 03.06.20 13:43, Dr. David Alan Gilbert wrote: > >>>> * David Hildenbrand (david@redhat.com) wrote: > >>>>> On 03.06.20 12:43, Dr. David Alan Gilbert wrote: > >>>>>> * David Hildenbrand (david@redhat.com) wrote: > >>>>>>> On 02.06.20 11:26, Dr. David Alan Gilbert wrote: > >>>>>>>> * Markus Armbruster (armbru@redhat.com) wrote: > >>>>>>>>> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes: > >>>>>>>>> > >>>>>>>>>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > >>>>>>>>>> > >>>>>>>>>> Simplify qom_set by making it use qmp_qom_set and the JSON parser. > >>>>>>>>>> > >>>>>>>>>> (qemu) qom-get /machine smm > >>>>>>>>>> "auto" > >>>>>>>>>> (qemu) qom-set /machine smm "auto" > >>>>>>>>>> > >>>>>>>>>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > >>>>>>>>>> Message-Id: <20200520151108.160598-3-dgilbert@redhat.com> > >>>>>>>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > >>>>>>>>>> Reviewed-by: Markus Armbruster <armbru@redhat.com> > >>>>>>>>>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > >>>>>>>>>> With 's'->'S' type change suggested by Paolo and Markus > >>>>>>>>> > >>>>>>>>> This is actually more than just simplification, it's disarming a bear > >>>>>>>>> trap: the string visitor is restricted to a subset of the QAPI types, > >>>>>>>>> and when you qom-set a property with a type it can't handle, QEMU > >>>>>>>>> aborts. I mentioned this in the discussion of possible ways out of the > >>>>>>>>> qom-get impasse, but missed reraising it in patch review. > >>>>>>>>> > >>>>>>>>> A suitably amended commit would be nice, but respinning the PR just for > >>>>>>>>> that may not be worthwhile. > >>>>>>>> > >>>>>>>> A bit late; still as long as we're removing bear traps not adding them. > >>>>>>> > >>>>>>> This breaks qom-set for my (virtio-mem) use case: > >>>>>>> > >>>>>>> echo "qom-set vm0 requested-size 300M" | sudo nc -U /var/tmp/mon_src > >>>>>>> QEMU 5.0.50 monitor - type 'help' for more information > >>>>>>> (qemu) qom-set vm0 requested-size 300M > >>>>>>> Error: Expecting at most one JSON value > >>>>>> > >>>>>> Does qom-set vm0 requested-size 300e6 do the same thing? > >>>>> > >>>>> The property is defined to be of type "size". > >>>>> > >>>>> (qemu) qom-set vm0 requested-size 300e6 > >>>>> Error: Parameter 'requested-size' expects uint64 > >>>>> > >>>>> (not sure how "size" and "uint64" are mapped here) > >>>> > >>>> I think the problem here is that the JSON parser is converting anything > >>>> with an 'e' as a float; JSON itself doesn't have the distinction > >>>> between int and float. > >>>> > >>> > >>> (and just to clarify - I assume you are aware - 300e6 != 300M. So the > >>> interface becomes way harder to use in case one wants to specify > >>> properly aligned sizes - 300M vs 314572800) > >> > >> Oops, yes, good point. > >> > >> I think on balance it's probably best that this keeps supporting JSON; > >> although tbh I'm not convinced there are any complex types that can be > >> set. > >> I'm not seeing a prettier answer. > > > > So, I have to use a calculator from now on to set a property that I can > > set on the QEMU cmdline just fine without it? :( > > > > This feels like a step backwards, @Markus any way to keep supporting sizes? > > Or what about adding qom-set-json instead for complex types instead of > changing the behavior if an existing interface? Yes, this isn't nice - we could add a flag to qom-set to take nice numerics. Dave > -- > Thanks, > > David / dhildenb -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
© 2016 - 2026 Red Hat, Inc.