qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] hmp: Make json format optional for qom-set
@ 2020-06-10  7:51 David Hildenbrand
  2020-06-10 10:37 ` David Hildenbrand
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: David Hildenbrand @ 2020-06-10  7:51 UTC (permalink / raw)
  To: qemu-devel
  Cc: Daniel P. Berrangé,
	Eduardo Habkost, David Hildenbrand, Markus Armbruster,
	Dr . David Alan Gilbert, Paolo Bonzini,
	Philippe Mathieu-Daudé

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é <philmd@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Daniel P. Berrangé" <berrange@redhat.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
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



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] hmp: Make json format optional for qom-set
  2020-06-10  7:51 [PATCH v2] hmp: Make json format optional for qom-set David Hildenbrand
@ 2020-06-10 10:37 ` David Hildenbrand
  2020-06-10 10:39   ` Dr. David Alan Gilbert
  2020-06-10 10:38 ` Dr. David Alan Gilbert
  2020-06-15  6:17 ` Markus Armbruster
  2 siblings, 1 reply; 10+ messages in thread
From: David Hildenbrand @ 2020-06-10 10:37 UTC (permalink / raw)
  To: qemu-devel
  Cc: Daniel P. Berrangé,
	Eduardo Habkost, Markus Armbruster, Dr . David Alan Gilbert,
	Paolo Bonzini, Philippe Mathieu-Daudé

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é <philmd@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: "Daniel P. Berrangé" <berrange@redhat.com>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> 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



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] hmp: Make json format optional for qom-set
  2020-06-10  7:51 [PATCH v2] hmp: Make json format optional for qom-set David Hildenbrand
  2020-06-10 10:37 ` David Hildenbrand
@ 2020-06-10 10:38 ` Dr. David Alan Gilbert
  2020-06-15  6:17 ` Markus Armbruster
  2 siblings, 0 replies; 10+ messages in thread
From: Dr. David Alan Gilbert @ 2020-06-10 10:38 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: Daniel P. Berrangé,
	Eduardo Habkost, qemu-devel, Markus Armbruster, Paolo Bonzini,
	Philippe Mathieu-Daudé

* David Hildenbrand (david@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é <philmd@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: "Daniel P. Berrangé" <berrange@redhat.com>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>

Yep OK.  Shame it's got back to even more complex but it makes sense.


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
> 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 / dgilbert@redhat.com / Manchester, UK



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] hmp: Make json format optional for qom-set
  2020-06-10 10:37 ` David Hildenbrand
@ 2020-06-10 10:39   ` Dr. David Alan Gilbert
  2020-06-10 10:40     ` David Hildenbrand
  0 siblings, 1 reply; 10+ messages in thread
From: Dr. David Alan Gilbert @ 2020-06-10 10:39 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: Daniel P. Berrangé,
	Eduardo Habkost, qemu-devel, Markus Armbruster, Paolo Bonzini,
	Philippe Mathieu-Daudé

* David Hildenbrand (david@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é <philmd@redhat.com>
> > Cc: Markus Armbruster <armbru@redhat.com>
> > Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: "Daniel P. Berrangé" <berrange@redhat.com>
> > Cc: Eduardo Habkost <ehabkost@redhat.com>
> > Signed-off-by: David Hildenbrand <david@redhat.com>
> > ---
> > 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 / dgilbert@redhat.com / Manchester, UK



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] hmp: Make json format optional for qom-set
  2020-06-10 10:39   ` Dr. David Alan Gilbert
@ 2020-06-10 10:40     ` David Hildenbrand
  2020-06-17  9:53       ` Dr. David Alan Gilbert
  0 siblings, 1 reply; 10+ messages in thread
From: David Hildenbrand @ 2020-06-10 10:40 UTC (permalink / raw)
  To: Dr. David Alan Gilbert
  Cc: Daniel P. Berrangé,
	Eduardo Habkost, qemu-devel, Markus Armbruster, Paolo Bonzini,
	Philippe Mathieu-Daudé

On 10.06.20 12:39, Dr. David Alan Gilbert wrote:
> * David Hildenbrand (david@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é <philmd@redhat.com>
>>> Cc: Markus Armbruster <armbru@redhat.com>
>>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>> Cc: "Daniel P. Berrangé" <berrange@redhat.com>
>>> Cc: Eduardo Habkost <ehabkost@redhat.com>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>> 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



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] hmp: Make json format optional for qom-set
  2020-06-10  7:51 [PATCH v2] hmp: Make json format optional for qom-set David Hildenbrand
  2020-06-10 10:37 ` David Hildenbrand
  2020-06-10 10:38 ` Dr. David Alan Gilbert
@ 2020-06-15  6:17 ` Markus Armbruster
  2020-06-15  7:45   ` David Hildenbrand
  2020-06-15  9:40   ` Dr. David Alan Gilbert
  2 siblings, 2 replies; 10+ messages in thread
From: Markus Armbruster @ 2020-06-15  6:17 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: Daniel P. Berrangé,
	Eduardo Habkost, qemu-devel, Dr . David Alan Gilbert,
	Paolo Bonzini, Philippe Mathieu-Daudé

David Hildenbrand <david@redhat.com> 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.fsf@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.



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] hmp: Make json format optional for qom-set
  2020-06-15  6:17 ` Markus Armbruster
@ 2020-06-15  7:45   ` David Hildenbrand
  2020-06-22  8:05     ` Markus Armbruster
  2020-06-15  9:40   ` Dr. David Alan Gilbert
  1 sibling, 1 reply; 10+ messages in thread
From: David Hildenbrand @ 2020-06-15  7:45 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: Daniel P. Berrangé,
	Eduardo Habkost, qemu-devel, Dr . David Alan Gilbert,
	Paolo Bonzini, Philippe Mathieu-Daudé

On 15.06.20 08:17, Markus Armbruster wrote:
> David Hildenbrand <david@redhat.com> 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.fsf@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



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] hmp: Make json format optional for qom-set
  2020-06-15  6:17 ` Markus Armbruster
  2020-06-15  7:45   ` David Hildenbrand
@ 2020-06-15  9:40   ` Dr. David Alan Gilbert
  1 sibling, 0 replies; 10+ messages in thread
From: Dr. David Alan Gilbert @ 2020-06-15  9:40 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: Daniel P. Berrangé,
	Eduardo Habkost, David Hildenbrand, qemu-devel, Paolo Bonzini,
	Philippe Mathieu-Daudé

* Markus Armbruster (armbru@redhat.com) wrote:
> David Hildenbrand <david@redhat.com> 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.fsf@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 / dgilbert@redhat.com / Manchester, UK



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] hmp: Make json format optional for qom-set
  2020-06-10 10:40     ` David Hildenbrand
@ 2020-06-17  9:53       ` Dr. David Alan Gilbert
  0 siblings, 0 replies; 10+ messages in thread
From: Dr. David Alan Gilbert @ 2020-06-17  9:53 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: Daniel P. Berrangé,
	Eduardo Habkost, qemu-devel, Markus Armbruster, Paolo Bonzini,
	Philippe Mathieu-Daudé

* David Hildenbrand (david@redhat.com) wrote:
> On 10.06.20 12:39, Dr. David Alan Gilbert wrote:
> > * David Hildenbrand (david@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é <philmd@redhat.com>
> >>> Cc: Markus Armbruster <armbru@redhat.com>
> >>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >>> Cc: Paolo Bonzini <pbonzini@redhat.com>
> >>> Cc: "Daniel P. Berrangé" <berrange@redhat.com>
> >>> Cc: Eduardo Habkost <ehabkost@redhat.com>
> >>> Signed-off-by: David Hildenbrand <david@redhat.com>
> >>> ---
> >>> 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 / dgilbert@redhat.com / Manchester, UK



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] hmp: Make json format optional for qom-set
  2020-06-15  7:45   ` David Hildenbrand
@ 2020-06-22  8:05     ` Markus Armbruster
  0 siblings, 0 replies; 10+ messages in thread
From: Markus Armbruster @ 2020-06-22  8:05 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: Daniel P. Berrangé,
	Eduardo Habkost, Dr . David Alan Gilbert, qemu-devel,
	Paolo Bonzini, Philippe Mathieu-Daudé

David Hildenbrand <david@redhat.com> writes:

> On 15.06.20 08:17, Markus Armbruster wrote:
>> David Hildenbrand <david@redhat.com> 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.fsf@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.



^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2020-06-22  8:06 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-10  7:51 [PATCH v2] hmp: Make json format optional for qom-set David Hildenbrand
2020-06-10 10:37 ` David Hildenbrand
2020-06-10 10:39   ` Dr. David Alan Gilbert
2020-06-10 10:40     ` David Hildenbrand
2020-06-17  9:53       ` Dr. David Alan Gilbert
2020-06-10 10:38 ` Dr. David Alan Gilbert
2020-06-15  6:17 ` Markus Armbruster
2020-06-15  7:45   ` David Hildenbrand
2020-06-22  8:05     ` Markus Armbruster
2020-06-15  9:40   ` Dr. David Alan Gilbert

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).