[Qemu-devel] [PATCH] QMP: allow JSON dict arguments in qmp-shell

[Qemu-devel] [PATCH] QMP: allow JSON dict arguments in qmp-shell

[Stefan Hajnoczi]

qmp-shell hides the QMP wire protocol JSON encoding from the user.  Most
of the time this is helpful and makes the command-line human-friendly.

Some QMP commands take a dict as an argument.  In order to express this
we need to revert back to JSON notation.

This patch allows JSON dict arguments in qmp-shell so commands like
blockdev-add and nbd-server-start can be invoked:

  (QEMU) blockdev-add options={"driver":"file","id":"drive1",...}

Note that spaces are not allowed since str.split() is used to break up
the command-line arguments first.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 scripts/qmp/qmp-shell | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell
index d6b420f..d374b35 100755
--- a/scripts/qmp/qmp-shell
+++ b/scripts/qmp/qmp-shell
@@ -31,6 +31,7 @@
 # (QEMU)
 
 import qmp
+import json
 import readline
 import sys
 import pprint
@@ -107,6 +108,8 @@ class QMPShell(qmp.QEMUMonitorProtocol):
                     value = True
                 elif opt[1] == 'false':
                     value = False
+                elif opt[1].startswith('{'):
+                    value = json.loads(opt[1])
                 else:
                     value = opt[1]
             qmpcmd['arguments'][opt[0]] = value
-- 
1.8.5.3

Re: [Qemu-devel] [PATCH] QMP: allow JSON dict arguments in qmp-shell

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 932 bytes --]

On 01/29/2014 04:17 AM, Stefan Hajnoczi wrote:
> qmp-shell hides the QMP wire protocol JSON encoding from the user.  Most
> of the time this is helpful and makes the command-line human-friendly.
> 
> Some QMP commands take a dict as an argument.  In order to express this
> we need to revert back to JSON notation.
> 
> This patch allows JSON dict arguments in qmp-shell so commands like
> blockdev-add and nbd-server-start can be invoked:
> 
>   (QEMU) blockdev-add options={"driver":"file","id":"drive1",...}
> 
> Note that spaces are not allowed since str.split() is used to break up
> the command-line arguments first.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  scripts/qmp/qmp-shell | 3 +++
>  1 file changed, 3 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] QMP: allow JSON dict arguments in qmp-shell

[Benoît Canet]

Le Wednesday 29 Jan 2014 à 12:17:31 (+0100), Stefan Hajnoczi a écrit :
> qmp-shell hides the QMP wire protocol JSON encoding from the user.  Most
> of the time this is helpful and makes the command-line human-friendly.
> 
> Some QMP commands take a dict as an argument.  In order to express this
> we need to revert back to JSON notation.
> 
> This patch allows JSON dict arguments in qmp-shell so commands like
> blockdev-add and nbd-server-start can be invoked:
> 
>   (QEMU) blockdev-add options={"driver":"file","id":"drive1",...}
> 
> Note that spaces are not allowed since str.split() is used to break up
> the command-line arguments first.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  scripts/qmp/qmp-shell | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell
> index d6b420f..d374b35 100755
> --- a/scripts/qmp/qmp-shell
> +++ b/scripts/qmp/qmp-shell
> @@ -31,6 +31,7 @@
>  # (QEMU)
>  
>  import qmp
> +import json
>  import readline
>  import sys
>  import pprint
> @@ -107,6 +108,8 @@ class QMPShell(qmp.QEMUMonitorProtocol):
>                      value = True
>                  elif opt[1] == 'false':
>                      value = False
> +                elif opt[1].startswith('{'):
> +                    value = json.loads(opt[1])
>                  else:
>                      value = opt[1]
>              qmpcmd['arguments'][opt[0]] = value
> -- 
> 1.8.5.3
> 
> 
This patch is very usefull.
Reviewed-by: Benoit Canet <benoit@irqsave.net>

Re: [Qemu-devel] [PATCH] QMP: allow JSON dict arguments in qmp-shell

[Luiz Capitulino]

On Wed, 29 Jan 2014 12:17:31 +0100
Stefan Hajnoczi <stefanha@redhat.com> wrote:

> qmp-shell hides the QMP wire protocol JSON encoding from the user.  Most
> of the time this is helpful and makes the command-line human-friendly.
> 
> Some QMP commands take a dict as an argument.  In order to express this
> we need to revert back to JSON notation.
> 
> This patch allows JSON dict arguments in qmp-shell so commands like
> blockdev-add and nbd-server-start can be invoked:
> 
>   (QEMU) blockdev-add options={"driver":"file","id":"drive1",...}
> 
> Note that spaces are not allowed since str.split() is used to break up
> the command-line arguments first.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Applied to the qmp branch, thanks.

> ---
>  scripts/qmp/qmp-shell | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell
> index d6b420f..d374b35 100755
> --- a/scripts/qmp/qmp-shell
> +++ b/scripts/qmp/qmp-shell
> @@ -31,6 +31,7 @@
>  # (QEMU)
>  
>  import qmp
> +import json
>  import readline
>  import sys
>  import pprint
> @@ -107,6 +108,8 @@ class QMPShell(qmp.QEMUMonitorProtocol):
>                      value = True
>                  elif opt[1] == 'false':
>                      value = False
> +                elif opt[1].startswith('{'):
> +                    value = json.loads(opt[1])
>                  else:
>                      value = opt[1]
>              qmpcmd['arguments'][opt[0]] = value

Re: [Qemu-devel] [PATCH] QMP: allow JSON dict arguments in qmp-shell

[Fam Zheng]

On Wed, 01/29 12:17, Stefan Hajnoczi wrote:
> qmp-shell hides the QMP wire protocol JSON encoding from the user.  Most
> of the time this is helpful and makes the command-line human-friendly.
> 
> Some QMP commands take a dict as an argument.  In order to express this
> we need to revert back to JSON notation.
> 
> This patch allows JSON dict arguments in qmp-shell so commands like
> blockdev-add and nbd-server-start can be invoked:
> 
>   (QEMU) blockdev-add options={"driver":"file","id":"drive1",...}
> 
> Note that spaces are not allowed since str.split() is used to break up
> the command-line arguments first.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---

While this is definitely useful, just wondering if it is more friendly to allow
passing options as command line options do:

    (QEMU) blockdev-add options.driver=file options.id=drive1

Thanks,
Fam

>  scripts/qmp/qmp-shell | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell
> index d6b420f..d374b35 100755
> --- a/scripts/qmp/qmp-shell
> +++ b/scripts/qmp/qmp-shell
> @@ -31,6 +31,7 @@
>  # (QEMU)
>  
>  import qmp
> +import json
>  import readline
>  import sys
>  import pprint
> @@ -107,6 +108,8 @@ class QMPShell(qmp.QEMUMonitorProtocol):
>                      value = True
>                  elif opt[1] == 'false':
>                      value = False
> +                elif opt[1].startswith('{'):
> +                    value = json.loads(opt[1])
>                  else:
>                      value = opt[1]
>              qmpcmd['arguments'][opt[0]] = value
> -- 
> 1.8.5.3
> 
> 

Re: [Qemu-devel] [PATCH] QMP: allow JSON dict arguments in qmp-shell

[Kevin Wolf]

Am 11.02.2014 um 02:39 hat Fam Zheng geschrieben:
> On Wed, 01/29 12:17, Stefan Hajnoczi wrote:
> > qmp-shell hides the QMP wire protocol JSON encoding from the user.  Most
> > of the time this is helpful and makes the command-line human-friendly.
> > 
> > Some QMP commands take a dict as an argument.  In order to express this
> > we need to revert back to JSON notation.
> > 
> > This patch allows JSON dict arguments in qmp-shell so commands like
> > blockdev-add and nbd-server-start can be invoked:
> > 
> >   (QEMU) blockdev-add options={"driver":"file","id":"drive1",...}
> > 
> > Note that spaces are not allowed since str.split() is used to break up
> > the command-line arguments first.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> 
> While this is definitely useful, just wondering if it is more friendly to allow
> passing options as command line options do:
> 
>     (QEMU) blockdev-add options.driver=file options.id=drive1

In fact, I have been considering the brace syntax even for the command
line, because the dot syntax gets a bit too verbose when you have a lot
of nested dicts. The reason why I didn't do it is that it would
introduce more special characters.

>From a usability POV, it might be best to have both.

Kevin

Re: [Qemu-devel] [PATCH] QMP: allow JSON dict arguments in qmp-shell

[Markus Armbruster]

Fam Zheng <famz@redhat.com> writes:

> On Wed, 01/29 12:17, Stefan Hajnoczi wrote:
>> qmp-shell hides the QMP wire protocol JSON encoding from the user.  Most
>> of the time this is helpful and makes the command-line human-friendly.
>> 
>> Some QMP commands take a dict as an argument.  In order to express this
>> we need to revert back to JSON notation.
>> 
>> This patch allows JSON dict arguments in qmp-shell so commands like
>> blockdev-add and nbd-server-start can be invoked:
>> 
>>   (QEMU) blockdev-add options={"driver":"file","id":"drive1",...}
>> 
>> Note that spaces are not allowed since str.split() is used to break up
>> the command-line arguments first.
>> 
>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>
> While this is definitely useful, just wondering if it is more friendly to allow
> passing options as command line options do:
>
>     (QEMU) blockdev-add options.driver=file options.id=drive1

Yeah.  The "spaces are not allowed" syntax is pretty horrid :)

Re: [Qemu-devel] [PATCH] QMP: allow JSON dict arguments in qmp-shell

[Fam Zheng]

On Tue, 02/11 09:41, Kevin Wolf wrote:
> Am 11.02.2014 um 02:39 hat Fam Zheng geschrieben:
> > On Wed, 01/29 12:17, Stefan Hajnoczi wrote:
> > > qmp-shell hides the QMP wire protocol JSON encoding from the user.  Most
> > > of the time this is helpful and makes the command-line human-friendly.
> > > 
> > > Some QMP commands take a dict as an argument.  In order to express this
> > > we need to revert back to JSON notation.
> > > 
> > > This patch allows JSON dict arguments in qmp-shell so commands like
> > > blockdev-add and nbd-server-start can be invoked:
> > > 
> > >   (QEMU) blockdev-add options={"driver":"file","id":"drive1",...}
> > > 
> > > Note that spaces are not allowed since str.split() is used to break up
> > > the command-line arguments first.
> > > 
> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > ---
> > 
> > While this is definitely useful, just wondering if it is more friendly to allow
> > passing options as command line options do:
> > 
> >     (QEMU) blockdev-add options.driver=file options.id=drive1
> 
> In fact, I have been considering the brace syntax even for the command
> line, because the dot syntax gets a bit too verbose when you have a lot
> of nested dicts. The reason why I didn't do it is that it would
> introduce more special characters.

Typing json syntax is harder than repeating the dict paths, so personally I
still prefer dot syntax over braces, colons and double quotes.

Fam

Re: [Qemu-devel] [PATCH] QMP: allow JSON dict arguments in qmp-shell

[Fam Zheng]

On Tue, 02/11 09:43, Markus Armbruster wrote:
> Fam Zheng <famz@redhat.com> writes:
> 
> > On Wed, 01/29 12:17, Stefan Hajnoczi wrote:
> >> qmp-shell hides the QMP wire protocol JSON encoding from the user.  Most
> >> of the time this is helpful and makes the command-line human-friendly.
> >> 
> >> Some QMP commands take a dict as an argument.  In order to express this
> >> we need to revert back to JSON notation.
> >> 
> >> This patch allows JSON dict arguments in qmp-shell so commands like
> >> blockdev-add and nbd-server-start can be invoked:
> >> 
> >>   (QEMU) blockdev-add options={"driver":"file","id":"drive1",...}
> >> 
> >> Note that spaces are not allowed since str.split() is used to break up
> >> the command-line arguments first.
> >> 
> >> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> >> ---
> >
> > While this is definitely useful, just wondering if it is more friendly to allow
> > passing options as command line options do:
> >
> >     (QEMU) blockdev-add options.driver=file options.id=drive1
> 
> Yeah.  The "spaces are not allowed" syntax is pretty horrid :)

It's not hard to loose the space restriction by making qmp-shell smarter to
match "}" with "{", but the quote marks and colons are still troublesome.

Maybe I'll think differently when many levels of nesting becomes common. :)

Fam

[Qemu-devel] [PATCH] QMP: allow dot separated dict path arguments in qmp-shell

[Fam Zheng]

As another convinience to allow using commands that expect a dict as
argument, this patch adds support for foo.bar=value syntax, similar to
command line argument style:

  (QEMU) blockdev-add options.driver=file options.id=drive1 options.filename=...

Signed-off-by: Fam Zheng <famz@redhat.com>

--
Applies on top of Stefan's patch

    [PATCH] QMP: allow JSON dict arguments in qmp-shell

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 scripts/qmp/qmp-shell | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell
index d374b35..9c84551 100755
--- a/scripts/qmp/qmp-shell
+++ b/scripts/qmp/qmp-shell
@@ -112,7 +112,14 @@ class QMPShell(qmp.QEMUMonitorProtocol):
                     value = json.loads(opt[1])
                 else:
                     value = opt[1]
-            qmpcmd['arguments'][opt[0]] = value
+            optpath = opt[0].split('.')
+            parent = qmpcmd['arguments']
+            for p in optpath[:-1]:
+                if not p in parent:
+                    d = dict()
+                    parent[p] = d
+                parent = d
+            parent[optpath[-1]] = value
         return qmpcmd
 
     def _execute_cmd(self, cmdline):
-- 
1.8.5.4

Re: [Qemu-devel] [PATCH] QMP: allow dot separated dict path arguments in qmp-shell

[Stefan Hajnoczi]

On Tue, Feb 11, 2014 at 06:45:05PM +0800, Fam Zheng wrote:
> diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell
> index d374b35..9c84551 100755
> --- a/scripts/qmp/qmp-shell
> +++ b/scripts/qmp/qmp-shell
> @@ -112,7 +112,14 @@ class QMPShell(qmp.QEMUMonitorProtocol):
>                      value = json.loads(opt[1])
>                  else:
>                      value = opt[1]
> -            qmpcmd['arguments'][opt[0]] = value
> +            optpath = opt[0].split('.')
> +            parent = qmpcmd['arguments']
> +            for p in optpath[:-1]:
> +                if not p in parent:
> +                    d = dict()
> +                    parent[p] = d
> +                parent = d

d is a stale reference when the path component already exists (e.g.
a.b.c=1 a.d=2).  Since 'a' already exists when processing 'a.d' we'll
the value of d will actually be the a.b dict!

I think you need the following instead:

for p in optpath[:-1]:
    d = parent.get(p, {})
    parent[p] = d
    parent = d

Re: [Qemu-devel] [PATCH] QMP: allow dot separated dict path arguments in qmp-shell

[Fam Zheng]

On Tue, 02/11 14:28, Stefan Hajnoczi wrote:
> On Tue, Feb 11, 2014 at 06:45:05PM +0800, Fam Zheng wrote:
> > diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell
> > index d374b35..9c84551 100755
> > --- a/scripts/qmp/qmp-shell
> > +++ b/scripts/qmp/qmp-shell
> > @@ -112,7 +112,14 @@ class QMPShell(qmp.QEMUMonitorProtocol):
> >                      value = json.loads(opt[1])
> >                  else:
> >                      value = opt[1]
> > -            qmpcmd['arguments'][opt[0]] = value
> > +            optpath = opt[0].split('.')
> > +            parent = qmpcmd['arguments']
> > +            for p in optpath[:-1]:
> > +                if not p in parent:
> > +                    d = dict()
> > +                    parent[p] = d
> > +                parent = d
> 
> d is a stale reference when the path component already exists (e.g.
> a.b.c=1 a.d=2).  Since 'a' already exists when processing 'a.d' we'll
> the value of d will actually be the a.b dict!
> 
> I think you need the following instead:
> 
> for p in optpath[:-1]:
>     d = parent.get(p, {})
>     parent[p] = d
>     parent = d

Yes, thanks. And we should check the contradictive case "foo=a foo.bar=b". Will
send v2.

Fam