Re: [PATCH v2] hmp: Make json format optional for qom-set
David Hildenbrand writes:
> On 15.06.20 08:17, Markus Armbruster wrote:
>> David Hildenbrand writes:
>>
>>> Commit 7d2ef6dcc1cf ("hmp: Simplify qom-set") switched to the json
>>> parser, making it possible to specify complex types. However, with this
>>> change it is no longer possible to specify proper sizes (e.g., 2G, 128M),
>>> turning the interface harder to use for properties that consume sizes.
>>>
>>> Let's switch back to the previous handling and allow to specify passing
>>> json via the "-j" parameter.
>>
>> Two issues:
>>
>> 1. Makes qom-get and qom-set inconsistent
>>
>>qom-get formats as JSON, always.
>>
>>qom-set parses the string visitor's undocumented ad hoc language by
>>default. You can make it parse JSON by passing -j.
>
> This is the same language the QEMU cmdline uses, no?
The CLI uses many, many languages. The string visitor's language may
well be among them; can't tell offhand.
>
>>
>>Not a show stopper, but sure ugly. I feel documentation should point
>>it out.
>
> Sure, we can fine-tune the documentation. For now we didn't have any
> qom-get users, in contrast to qom-set. Not sure if it makes sense to
> implement the same functionality for qom-get.
>
> For now I can e.g.,
>
> "echo "qom-set vm1 requested-size 256M" | sudo nc -U /var/tmp/mon_src"
>
> then I can
>
> echo "qom-get vm1 requested-size " | sudo nc -U /var/tmp/mon_src
> -> 268435456
>
> which is a value I can punch back into qom-set. At least for sizes this
> works. Not perfect, not bad. Opinions?
It happens to work in this case, because the JSON number returned by
qom-get happens to get parsed the right way by qom-set.
Is this the case for all properties where qom-set isn't deadly due to
issue 2.? Nobody knows.
>> 2. Rearms the string visitor death trap
>>
>>If you try to qom-set a property whose ->set() uses something the
>>string input visitor doesn't support, QEMU crashes. I'm not aware of
>>such a ->set(), but this is a death trap all the same. Mind, I
>>didn't actually *look* for such a ->set(). Details:
>
> Thanks. Maybe I am missing something important, but this sounds like we
> are missing a bunch of checks+errors.
The string visitor feels like a quick hack to get something that is
human-friendly. It provides just enough functionality for its initial
uses. The trouble is new uses that violate its restrictions are hard to
spot.
In my opinion, what we're really missing a replacement of the
ill-conceived string visitor. The less it's used, the better.
Since a replacement isn't being worked on, we may have to make it less
dangerous to use. Patches welcome.
> (wouldn't we be able to crash
> using the QEMU cmdline as well when setting such properties?).
If the string visitor is used there. Nobody knows.
>> Subject: Re: [RFC PATCH] qom: Implement qom-get HMP command
>> Date: Sat, 02 May 2020 08:02:43 +0200 (6 weeks, 2 days, 4 minutes ago)
>> Message-ID: <87a72q6fi4@dusky.pond.sub.org>
>> https://lists.nongnu.org/archive/html/qemu-devel/2020-05/msg00178.html
>>
>>Since we've had this death trap in the code for a number of years, I
>>can't call its restoration a show stopper. It does feel like an
>>unadvisable risk, though.
>>
>
> As long as there are no better alternatives to punch in data in the same
> format the QEMU cmdline consumes, I think this is perfectly reasonable.
> No good reason to make a HMP interface harder to use by humans IMHO.
Yes, HMP should be human-friendly. Not at any cost, though; I reiterate
my conviction that this is an unadvisable risk.
A crash is the most unfriendly response of all.
Re: [PATCH v2] hmp: Make json format optional for qom-set
* David Hildenbrand (da...@redhat.com) wrote:
> On 10.06.20 12:39, Dr. David Alan Gilbert wrote:
> > * David Hildenbrand (da...@redhat.com) wrote:
> >> On 10.06.20 09:51, David Hildenbrand wrote:
> >>> Commit 7d2ef6dcc1cf ("hmp: Simplify qom-set") switched to the json
> >>> parser, making it possible to specify complex types. However, with this
> >>> change it is no longer possible to specify proper sizes (e.g., 2G, 128M),
> >>> turning the interface harder to use for properties that consume sizes.
> >>>
> >>> Let's switch back to the previous handling and allow to specify passing
> >>> json via the "-j" parameter.
> >>>
> >>> Cc: Philippe Mathieu-Daudé
> >>> Cc: Markus Armbruster
> >>> Cc: Dr. David Alan Gilbert
> >>> Cc: Paolo Bonzini
> >>> Cc: "Daniel P. Berrangé"
> >>> Cc: Eduardo Habkost
> >>> Signed-off-by: David Hildenbrand
> >>> ---
> >>> v1 - v2:
> >>> - keep the "value:S" as correctly noted by Paolo :)
> >>> ---
> >>> hmp-commands.hx| 7 ---
> >>> qom/qom-hmp-cmds.c | 20
> >>> 2 files changed, 20 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/hmp-commands.hx b/hmp-commands.hx
> >>> index 28256209b5..5d12fbeebe 100644
> >>> --- a/hmp-commands.hx
> >>> +++ b/hmp-commands.hx
> >>> @@ -1806,9 +1806,10 @@ ERST
> >>>
> >>> {
> >>> .name = "qom-set",
> >>> -.args_type = "path:s,property:s,value:S",
> >>> -.params = "path property value",
> >>> -.help = "set QOM property",
> >>> +.args_type = "json:-j,path:s,property:s,value:S",
> >>> +.params = "[-j] path property value",
> >>> +.help = "set QOM property.\n\t\t\t"
> >>> + "-j: the property is specified in json format.",
> >>
> >> Stupid mistake:
> >>
> >> "-j: the value is specified in json format
> >
> > oops; can fix that in commit
>
> Perfect, let me know in case you need a respin. Thanks!
I've queued this; if we can come up with something nicer to meet
Markus's suggestions that would be great, but for now we get the
functionality back.
>
> --
> Thanks,
>
> David / dhildenb
>
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [PATCH v2] hmp: Make json format optional for qom-set
* Markus Armbruster (arm...@redhat.com) wrote:
> David Hildenbrand writes:
>
> > Commit 7d2ef6dcc1cf ("hmp: Simplify qom-set") switched to the json
> > parser, making it possible to specify complex types. However, with this
> > change it is no longer possible to specify proper sizes (e.g., 2G, 128M),
> > turning the interface harder to use for properties that consume sizes.
> >
> > Let's switch back to the previous handling and allow to specify passing
> > json via the "-j" parameter.
>
> Two issues:
>
> 1. Makes qom-get and qom-set inconsistent
>
>qom-get formats as JSON, always.
>
>qom-set parses the string visitor's undocumented ad hoc language by
>default. You can make it parse JSON by passing -j.
>
>Not a show stopper, but sure ugly. I feel documentation should point
>it out.
I can imagine one way around this owuld be to remove the flag and make
it happen in the failure case; i.e.:
obj = qobject_from_json(value, &err);
if (err == NULL) {
qmp_qom_set(path, property, obj, &err);
} else {
somehow check if it parses with the integer parser and if it
does use object_property_parse
}
unfortunately that else path is a bit messy, because you need to pick a
parser in this case and then if that fails probably present the json
error message not it's error.
> 2. Rearms the string visitor death trap
>
>If you try to qom-set a property whose ->set() uses something the
>string input visitor doesn't support, QEMU crashes. I'm not aware of
>such a ->set(), but this is a death trap all the same. Mind, I
>didn't actually *look* for such a ->set(). Details:
>
> Subject: Re: [RFC PATCH] qom: Implement qom-get HMP command
> Date: Sat, 02 May 2020 08:02:43 +0200 (6 weeks, 2 days, 4 minutes ago)
> Message-ID: <87a72q6fi4@dusky.pond.sub.org>
> https://lists.nongnu.org/archive/html/qemu-devel/2020-05/msg00178.html
>
>Since we've had this death trap in the code for a number of years, I
>can't call its restoration a show stopper. It does feel like an
>unadvisable risk, though.
That just needs fixing in qom somewhere; it shouldn't assert - people
are too free with asserts.
Dave
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [PATCH v2] hmp: Make json format optional for qom-set
On 15.06.20 08:17, Markus Armbruster wrote:
> David Hildenbrand writes:
>
>> Commit 7d2ef6dcc1cf ("hmp: Simplify qom-set") switched to the json
>> parser, making it possible to specify complex types. However, with this
>> change it is no longer possible to specify proper sizes (e.g., 2G, 128M),
>> turning the interface harder to use for properties that consume sizes.
>>
>> Let's switch back to the previous handling and allow to specify passing
>> json via the "-j" parameter.
>
> Two issues:
>
> 1. Makes qom-get and qom-set inconsistent
>
>qom-get formats as JSON, always.
>
>qom-set parses the string visitor's undocumented ad hoc language by
>default. You can make it parse JSON by passing -j.
This is the same language the QEMU cmdline uses, no?
>
>Not a show stopper, but sure ugly. I feel documentation should point
>it out.
Sure, we can fine-tune the documentation. For now we didn't have any
qom-get users, in contrast to qom-set. Not sure if it makes sense to
implement the same functionality for qom-get.
For now I can e.g.,
"echo "qom-set vm1 requested-size 256M" | sudo nc -U /var/tmp/mon_src"
then I can
echo "qom-get vm1 requested-size " | sudo nc -U /var/tmp/mon_src
-> 268435456
which is a value I can punch back into qom-set. At least for sizes this
works. Not perfect, not bad. Opinions?
>
> 2. Rearms the string visitor death trap
>
>If you try to qom-set a property whose ->set() uses something the
>string input visitor doesn't support, QEMU crashes. I'm not aware of
>such a ->set(), but this is a death trap all the same. Mind, I
>didn't actually *look* for such a ->set(). Details:
Thanks. Maybe I am missing something important, but this sounds like we
are missing a bunch of checks+errors. (wouldn't we be able to crash
using the QEMU cmdline as well when setting such properties?).
>
> Subject: Re: [RFC PATCH] qom: Implement qom-get HMP command
> Date: Sat, 02 May 2020 08:02:43 +0200 (6 weeks, 2 days, 4 minutes ago)
> Message-ID: <87a72q6fi4@dusky.pond.sub.org>
> https://lists.nongnu.org/archive/html/qemu-devel/2020-05/msg00178.html
>
>Since we've had this death trap in the code for a number of years, I
>can't call its restoration a show stopper. It does feel like an
>unadvisable risk, though.
>
As long as there are no better alternatives to punch in data in the same
format the QEMU cmdline consumes, I think this is perfectly reasonable.
No good reason to make a HMP interface harder to use by humans IMHO.
Thanks!
--
Thanks,
David / dhildenb
Re: [PATCH v2] hmp: Make json format optional for qom-set
David Hildenbrand writes:
> Commit 7d2ef6dcc1cf ("hmp: Simplify qom-set") switched to the json
> parser, making it possible to specify complex types. However, with this
> change it is no longer possible to specify proper sizes (e.g., 2G, 128M),
> turning the interface harder to use for properties that consume sizes.
>
> Let's switch back to the previous handling and allow to specify passing
> json via the "-j" parameter.
Two issues:
1. Makes qom-get and qom-set inconsistent
qom-get formats as JSON, always.
qom-set parses the string visitor's undocumented ad hoc language by
default. You can make it parse JSON by passing -j.
Not a show stopper, but sure ugly. I feel documentation should point
it out.
2. Rearms the string visitor death trap
If you try to qom-set a property whose ->set() uses something the
string input visitor doesn't support, QEMU crashes. I'm not aware of
such a ->set(), but this is a death trap all the same. Mind, I
didn't actually *look* for such a ->set(). Details:
Subject: Re: [RFC PATCH] qom: Implement qom-get HMP command
Date: Sat, 02 May 2020 08:02:43 +0200 (6 weeks, 2 days, 4 minutes ago)
Message-ID: <87a72q6fi4@dusky.pond.sub.org>
https://lists.nongnu.org/archive/html/qemu-devel/2020-05/msg00178.html
Since we've had this death trap in the code for a number of years, I
can't call its restoration a show stopper. It does feel like an
unadvisable risk, though.
Re: [PATCH v2] hmp: Make json format optional for qom-set
On 10.06.20 12:39, Dr. David Alan Gilbert wrote:
> * David Hildenbrand (da...@redhat.com) wrote:
>> On 10.06.20 09:51, David Hildenbrand wrote:
>>> Commit 7d2ef6dcc1cf ("hmp: Simplify qom-set") switched to the json
>>> parser, making it possible to specify complex types. However, with this
>>> change it is no longer possible to specify proper sizes (e.g., 2G, 128M),
>>> turning the interface harder to use for properties that consume sizes.
>>>
>>> Let's switch back to the previous handling and allow to specify passing
>>> json via the "-j" parameter.
>>>
>>> Cc: Philippe Mathieu-Daudé
>>> Cc: Markus Armbruster
>>> Cc: Dr. David Alan Gilbert
>>> Cc: Paolo Bonzini
>>> Cc: "Daniel P. Berrangé"
>>> Cc: Eduardo Habkost
>>> Signed-off-by: David Hildenbrand
>>> ---
>>> v1 - v2:
>>> - keep the "value:S" as correctly noted by Paolo :)
>>> ---
>>> hmp-commands.hx| 7 ---
>>> qom/qom-hmp-cmds.c | 20
>>> 2 files changed, 20 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>>> index 28256209b5..5d12fbeebe 100644
>>> --- a/hmp-commands.hx
>>> +++ b/hmp-commands.hx
>>> @@ -1806,9 +1806,10 @@ ERST
>>>
>>> {
>>> .name = "qom-set",
>>> -.args_type = "path:s,property:s,value:S",
>>> -.params = "path property value",
>>> -.help = "set QOM property",
>>> +.args_type = "json:-j,path:s,property:s,value:S",
>>> +.params = "[-j] path property value",
>>> +.help = "set QOM property.\n\t\t\t"
>>> + "-j: the property is specified in json format.",
>>
>> Stupid mistake:
>>
>> "-j: the value is specified in json format
>
> oops; can fix that in commit
Perfect, let me know in case you need a respin. Thanks!
--
Thanks,
David / dhildenb
Re: [PATCH v2] hmp: Make json format optional for qom-set
* David Hildenbrand (da...@redhat.com) wrote:
> On 10.06.20 09:51, David Hildenbrand wrote:
> > Commit 7d2ef6dcc1cf ("hmp: Simplify qom-set") switched to the json
> > parser, making it possible to specify complex types. However, with this
> > change it is no longer possible to specify proper sizes (e.g., 2G, 128M),
> > turning the interface harder to use for properties that consume sizes.
> >
> > Let's switch back to the previous handling and allow to specify passing
> > json via the "-j" parameter.
> >
> > Cc: Philippe Mathieu-Daudé
> > Cc: Markus Armbruster
> > Cc: Dr. David Alan Gilbert
> > Cc: Paolo Bonzini
> > Cc: "Daniel P. Berrangé"
> > Cc: Eduardo Habkost
> > Signed-off-by: David Hildenbrand
> > ---
> > v1 - v2:
> > - keep the "value:S" as correctly noted by Paolo :)
> > ---
> > hmp-commands.hx| 7 ---
> > qom/qom-hmp-cmds.c | 20
> > 2 files changed, 20 insertions(+), 7 deletions(-)
> >
> > diff --git a/hmp-commands.hx b/hmp-commands.hx
> > index 28256209b5..5d12fbeebe 100644
> > --- a/hmp-commands.hx
> > +++ b/hmp-commands.hx
> > @@ -1806,9 +1806,10 @@ ERST
> >
> > {
> > .name = "qom-set",
> > -.args_type = "path:s,property:s,value:S",
> > -.params = "path property value",
> > -.help = "set QOM property",
> > +.args_type = "json:-j,path:s,property:s,value:S",
> > +.params = "[-j] path property value",
> > +.help = "set QOM property.\n\t\t\t"
> > + "-j: the property is specified in json format.",
>
> Stupid mistake:
>
> "-j: the value is specified in json format
oops; can fix that in commit
>
> --
> Thanks,
>
> David / dhildenb
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [PATCH v2] hmp: Make json format optional for qom-set
* David Hildenbrand (da...@redhat.com) wrote:
> Commit 7d2ef6dcc1cf ("hmp: Simplify qom-set") switched to the json
> parser, making it possible to specify complex types. However, with this
> change it is no longer possible to specify proper sizes (e.g., 2G, 128M),
> turning the interface harder to use for properties that consume sizes.
>
> Let's switch back to the previous handling and allow to specify passing
> json via the "-j" parameter.
>
> Cc: Philippe Mathieu-Daudé
> Cc: Markus Armbruster
> Cc: Dr. David Alan Gilbert
> Cc: Paolo Bonzini
> Cc: "Daniel P. Berrangé"
> Cc: Eduardo Habkost
> Signed-off-by: David Hildenbrand
Yep OK. Shame it's got back to even more complex but it makes sense.
Reviewed-by: Dr. David Alan Gilbert
> ---
> v1 - v2:
> - keep the "value:S" as correctly noted by Paolo :)
> ---
> hmp-commands.hx| 7 ---
> qom/qom-hmp-cmds.c | 20
> 2 files changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 28256209b5..5d12fbeebe 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1806,9 +1806,10 @@ ERST
>
> {
> .name = "qom-set",
> -.args_type = "path:s,property:s,value:S",
> -.params = "path property value",
> -.help = "set QOM property",
> +.args_type = "json:-j,path:s,property:s,value:S",
> +.params = "[-j] path property value",
> +.help = "set QOM property.\n\t\t\t"
> + "-j: the property is specified in json format.",
> .cmd= hmp_qom_set,
> .flags = "p",
> },
> diff --git a/qom/qom-hmp-cmds.c b/qom/qom-hmp-cmds.c
> index f704b6949a..a794e62f0b 100644
> --- a/qom/qom-hmp-cmds.c
> +++ b/qom/qom-hmp-cmds.c
> @@ -44,15 +44,27 @@ void hmp_qom_list(Monitor *mon, const QDict *qdict)
>
> void hmp_qom_set(Monitor *mon, const QDict *qdict)
> {
> +const bool json = qdict_get_try_bool(qdict, "json", false);
> const char *path = qdict_get_str(qdict, "path");
> const char *property = qdict_get_str(qdict, "property");
> const char *value = qdict_get_str(qdict, "value");
> Error *err = NULL;
> -QObject *obj;
>
> -obj = qobject_from_json(value, &err);
> -if (err == NULL) {
> -qmp_qom_set(path, property, obj, &err);
> +if (!json) {
> +Object *obj = object_resolve_path(path, NULL);
> +
> +if (!obj) {
> +error_set(&err, ERROR_CLASS_DEVICE_NOT_FOUND,
> + "Device '%s' not found", path);
> +} else {
> +object_property_parse(obj, value, property, &err);
> +}
> +} else {
> +QObject *obj = qobject_from_json(value, &err);
> +
> +if (!err) {
> +qmp_qom_set(path, property, obj, &err);
> +}
> }
>
> hmp_handle_error(mon, err);
> --
> 2.26.2
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [PATCH v2] hmp: Make json format optional for qom-set
On 10.06.20 09:51, David Hildenbrand wrote:
> Commit 7d2ef6dcc1cf ("hmp: Simplify qom-set") switched to the json
> parser, making it possible to specify complex types. However, with this
> change it is no longer possible to specify proper sizes (e.g., 2G, 128M),
> turning the interface harder to use for properties that consume sizes.
>
> Let's switch back to the previous handling and allow to specify passing
> json via the "-j" parameter.
>
> Cc: Philippe Mathieu-Daudé
> Cc: Markus Armbruster
> Cc: Dr. David Alan Gilbert
> Cc: Paolo Bonzini
> Cc: "Daniel P. Berrangé"
> Cc: Eduardo Habkost
> Signed-off-by: David Hildenbrand
> ---
> v1 - v2:
> - keep the "value:S" as correctly noted by Paolo :)
> ---
> hmp-commands.hx| 7 ---
> qom/qom-hmp-cmds.c | 20
> 2 files changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 28256209b5..5d12fbeebe 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1806,9 +1806,10 @@ ERST
>
> {
> .name = "qom-set",
> -.args_type = "path:s,property:s,value:S",
> -.params = "path property value",
> -.help = "set QOM property",
> +.args_type = "json:-j,path:s,property:s,value:S",
> +.params = "[-j] path property value",
> +.help = "set QOM property.\n\t\t\t"
> + "-j: the property is specified in json format.",
Stupid mistake:
"-j: the value is specified in json format
--
Thanks,
David / dhildenb
[PATCH v2] hmp: Make json format optional for qom-set
Commit 7d2ef6dcc1cf ("hmp: Simplify qom-set") switched to the json
parser, making it possible to specify complex types. However, with this
change it is no longer possible to specify proper sizes (e.g., 2G, 128M),
turning the interface harder to use for properties that consume sizes.
Let's switch back to the previous handling and allow to specify passing
json via the "-j" parameter.
Cc: Philippe Mathieu-Daudé
Cc: Markus Armbruster
Cc: Dr. David Alan Gilbert
Cc: Paolo Bonzini
Cc: "Daniel P. Berrangé"
Cc: Eduardo Habkost
Signed-off-by: David Hildenbrand
---
v1 - v2:
- keep the "value:S" as correctly noted by Paolo :)
---
hmp-commands.hx| 7 ---
qom/qom-hmp-cmds.c | 20
2 files changed, 20 insertions(+), 7 deletions(-)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 28256209b5..5d12fbeebe 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1806,9 +1806,10 @@ ERST
{
.name = "qom-set",
-.args_type = "path:s,property:s,value:S",
-.params = "path property value",
-.help = "set QOM property",
+.args_type = "json:-j,path:s,property:s,value:S",
+.params = "[-j] path property value",
+.help = "set QOM property.\n\t\t\t"
+ "-j: the property is specified in json format.",
.cmd= hmp_qom_set,
.flags = "p",
},
diff --git a/qom/qom-hmp-cmds.c b/qom/qom-hmp-cmds.c
index f704b6949a..a794e62f0b 100644
--- a/qom/qom-hmp-cmds.c
+++ b/qom/qom-hmp-cmds.c
@@ -44,15 +44,27 @@ void hmp_qom_list(Monitor *mon, const QDict *qdict)
void hmp_qom_set(Monitor *mon, const QDict *qdict)
{
+const bool json = qdict_get_try_bool(qdict, "json", false);
const char *path = qdict_get_str(qdict, "path");
const char *property = qdict_get_str(qdict, "property");
const char *value = qdict_get_str(qdict, "value");
Error *err = NULL;
-QObject *obj;
-obj = qobject_from_json(value, &err);
-if (err == NULL) {
-qmp_qom_set(path, property, obj, &err);
+if (!json) {
+Object *obj = object_resolve_path(path, NULL);
+
+if (!obj) {
+error_set(&err, ERROR_CLASS_DEVICE_NOT_FOUND,
+ "Device '%s' not found", path);
+} else {
+object_property_parse(obj, value, property, &err);
+}
+} else {
+QObject *obj = qobject_from_json(value, &err);
+
+if (!err) {
+qmp_qom_set(path, property, obj, &err);
+}
}
hmp_handle_error(mon, err);
--
2.26.2
