[Qemu-devel] [PATCH 0/5] scripts: qmp-shell: add transaction support

[Qemu-devel] [PATCH 0/5] scripts: qmp-shell: add transaction support

[John Snow]

The qmp-shell is a little rudimentary, but it can be hacked
to give us some transactional support without too much difficulty.

(1) Prep.
(2) Add support for serializing json arrays
(3) Allow users to use 'single quotes' instead of "double quotes"
(4) Add a special transaction( ... ) syntax that lets users
    build up transactional commands using the existing qmp shell
    syntax to define each action.
(5) Add a verbose flag to display generated QMP commands.

The parsing is not as robust as one would like, but this suffices
without adding a proper parser.

Design considerations:

(1) Try not to disrupt the existing design of the qmp-shell. The existing
    API is not disturbed.

(2) Pick a "magic token" such that it could not be confused for legitimate
    QMP/JSON syntax. Parentheses are used for this purpose.

John Snow (5):
  scripts: qmp-shell: refactor helpers
  scripts: qmp-shell: add support for [] expressions
  scripts: qmp-shell: allow single-quotes in JSON expressions
  scripts: qmp-shell: add transaction subshell
  scripts: qmp-shell: Add verbose flag

 scripts/qmp/qmp-shell | 90 +++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 73 insertions(+), 17 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH 1/5] scripts: qmp-shell: refactor helpers

[John Snow]

Refactor the qmp-shell command line processing function
into two components. This will be used to allow sub-expressions,
which will assist us in adding transactional support to qmp-shell.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qmp/qmp-shell | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell
index e0e848b..a9632ec 100755
--- a/scripts/qmp/qmp-shell
+++ b/scripts/qmp/qmp-shell
@@ -88,16 +88,8 @@ class QMPShell(qmp.QEMUMonitorProtocol):
         # clearing everything as it doesn't seem to matter
         readline.set_completer_delims('')
 
-    def __build_cmd(self, cmdline):
-        """
-        Build a QMP input object from a user provided command-line in the
-        following format:
-
-            < command-name > [ arg-name1=arg1 ] ... [ arg-nameN=argN ]
-        """
-        cmdargs = cmdline.split()
-        qmpcmd = { 'execute': cmdargs[0], 'arguments': {} }
-        for arg in cmdargs[1:]:
+    def __cli_expr(self, tokens, parent):
+        for arg in tokens:
             opt = arg.split('=')
             try:
                 if(len(opt) > 2):
@@ -113,7 +105,6 @@ class QMPShell(qmp.QEMUMonitorProtocol):
                 else:
                     value = opt[1]
             optpath = opt[0].split('.')
-            parent = qmpcmd['arguments']
             curpath = []
             for p in optpath[:-1]:
                 curpath.append(p)
@@ -128,6 +119,17 @@ class QMPShell(qmp.QEMUMonitorProtocol):
                 else:
                     raise QMPShellError('Cannot set "%s" multiple times' % opt[0])
             parent[optpath[-1]] = value
+
+    def __build_cmd(self, cmdline):
+        """
+        Build a QMP input object from a user provided command-line in the
+        following format:
+
+            < command-name > [ arg-name1=arg1 ] ... [ arg-nameN=argN ]
+        """
+        cmdargs = cmdline.split()
+        qmpcmd = { 'execute': cmdargs[0], 'arguments': {} }
+        self.__cli_expr(cmdargs[1:], qmpcmd['arguments'])
         return qmpcmd
 
     def _execute_cmd(self, cmdline):
-- 
2.1.0

Re: [Qemu-devel] [PATCH 1/5] scripts: qmp-shell: refactor helpers

[Eric Blake]

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

On 04/21/2015 08:02 PM, John Snow wrote:
> Refactor the qmp-shell command line processing function
> into two components. This will be used to allow sub-expressions,
> which will assist us in adding transactional support to qmp-shell.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qmp/qmp-shell | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 

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

[Qemu-devel] [PATCH 2/5] scripts: qmp-shell: add support for [] expressions

[John Snow]

qmp-shell currently allows you to describe values as
JSON expressions:

key={"key":{"key2":"val"}}

But it does not currently support arrays, which are needed
for serializing and deserializing transactions:

key=[{"type":"drive-backup","data":{...}}]

Add support for arrays.

CAVEAT: The parser is still extremely rudimentary and does not
expect to find spaces in {} nor [] expressions. This patch does
not improve this functionality.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qmp/qmp-shell | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell
index a9632ec..5347f89 100755
--- a/scripts/qmp/qmp-shell
+++ b/scripts/qmp/qmp-shell
@@ -102,6 +102,8 @@ class QMPShell(qmp.QEMUMonitorProtocol):
                     value = False
                 elif opt[1].startswith('{'):
                     value = json.loads(opt[1])
+                elif opt[1].startswith('['):
+                    value = json.loads(opt[1])
                 else:
                     value = opt[1]
             optpath = opt[0].split('.')
-- 
2.1.0

Re: [Qemu-devel] [PATCH 2/5] scripts: qmp-shell: add support for [] expressions

[Eric Blake]

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

On 04/21/2015 08:02 PM, John Snow wrote:
> qmp-shell currently allows you to describe values as
> JSON expressions:
> 
> key={"key":{"key2":"val"}}
> 
> But it does not currently support arrays, which are needed
> for serializing and deserializing transactions:
> 
> key=[{"type":"drive-backup","data":{...}}]
> 
> Add support for arrays.
> 
> CAVEAT: The parser is still extremely rudimentary and does not
> expect to find spaces in {} nor [] expressions. This patch does
> not improve this functionality.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qmp/qmp-shell | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell
> index a9632ec..5347f89 100755
> --- a/scripts/qmp/qmp-shell
> +++ b/scripts/qmp/qmp-shell
> @@ -102,6 +102,8 @@ class QMPShell(qmp.QEMUMonitorProtocol):
>                      value = False
>                  elif opt[1].startswith('{'):
>                      value = json.loads(opt[1])
> +                elif opt[1].startswith('['):
> +                    value = json.loads(opt[1])

Why not:

elif opt[1].startswith('{'} or opt[1].startswith('['):

for a one-line change instead of two-line addition?  And I'm no python
expert, but I think you can write that with fewer characters. Untested,
but does this work?

elif opt[1][0] in "{[":

-- 
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 2/5] scripts: qmp-shell: add support for [] expressions

[John Snow]

On 04/22/2015 10:28 AM, Eric Blake wrote:
> On 04/21/2015 08:02 PM, John Snow wrote:
>> qmp-shell currently allows you to describe values as
>> JSON expressions:
>>
>> key={"key":{"key2":"val"}}
>>
>> But it does not currently support arrays, which are needed
>> for serializing and deserializing transactions:
>>
>> key=[{"type":"drive-backup","data":{...}}]
>>
>> Add support for arrays.
>>
>> CAVEAT: The parser is still extremely rudimentary and does not
>> expect to find spaces in {} nor [] expressions. This patch does
>> not improve this functionality.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   scripts/qmp/qmp-shell | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell
>> index a9632ec..5347f89 100755
>> --- a/scripts/qmp/qmp-shell
>> +++ b/scripts/qmp/qmp-shell
>> @@ -102,6 +102,8 @@ class QMPShell(qmp.QEMUMonitorProtocol):
>>                       value = False
>>                   elif opt[1].startswith('{'):
>>                       value = json.loads(opt[1])
>> +                elif opt[1].startswith('['):
>> +                    value = json.loads(opt[1])
>
> Why not:
>
> elif opt[1].startswith('{'} or opt[1].startswith('['):
>

Didn't really consider this patch too long is why :)
If the rest of it looks worth checking in to you, I'll make the tidying 
edits.

> for a one-line change instead of two-line addition?  And I'm no python
> expert, but I think you can write that with fewer characters. Untested,
> but does this work?
>
> elif opt[1][0] in "{[":
>

It should.

Thanks,
--js

[Qemu-devel] [PATCH 3/5] scripts: qmp-shell: allow single-quotes in JSON expressions

[John Snow]

As a convenience for the user, replace any single quotes given
with double quotes so that the data will deserialize correctly
via json.loads().

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qmp/qmp-shell | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell
index 5347f89..d7cb33d 100755
--- a/scripts/qmp/qmp-shell
+++ b/scripts/qmp/qmp-shell
@@ -129,7 +129,7 @@ class QMPShell(qmp.QEMUMonitorProtocol):
 
             < command-name > [ arg-name1=arg1 ] ... [ arg-nameN=argN ]
         """
-        cmdargs = cmdline.split()
+        cmdargs = cmdline.replace("'", '"').split()
         qmpcmd = { 'execute': cmdargs[0], 'arguments': {} }
         self.__cli_expr(cmdargs[1:], qmpcmd['arguments'])
         return qmpcmd
-- 
2.1.0

Re: [Qemu-devel] [PATCH 3/5] scripts: qmp-shell: allow single-quotes in JSON expressions

[Eric Blake]

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

On 04/21/2015 08:02 PM, John Snow wrote:
> As a convenience for the user, replace any single quotes given
> with double quotes so that the data will deserialize correctly
> via json.loads().
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qmp/qmp-shell | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell
> index 5347f89..d7cb33d 100755
> --- a/scripts/qmp/qmp-shell
> +++ b/scripts/qmp/qmp-shell
> @@ -129,7 +129,7 @@ class QMPShell(qmp.QEMUMonitorProtocol):
>  
>              < command-name > [ arg-name1=arg1 ] ... [ arg-nameN=argN ]
>          """
> -        cmdargs = cmdline.split()
> +        cmdargs = cmdline.replace("'", '"').split()

This replaces ALL single quotes, even if they would otherwise be
escaped.  That is, if I pass foo="a\'\"b", it will probably be corrupted.

qmp-shell exists mainly as a convenience for testing, and I doubt
testers are likely to want to use unbalanced quotes as contents of
strings, so I can give a weak:
Reviewed-by: Eric Blake <eblake@redhat.com>

But it's still worth thinking about whether there is a more robust
solution to be used.

-- 
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 3/5] scripts: qmp-shell: allow single-quotes in JSON expressions

[John Snow]

On 04/22/2015 10:34 AM, Eric Blake wrote:
> On 04/21/2015 08:02 PM, John Snow wrote:
>> As a convenience for the user, replace any single quotes given
>> with double quotes so that the data will deserialize correctly
>> via json.loads().
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   scripts/qmp/qmp-shell | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell
>> index 5347f89..d7cb33d 100755
>> --- a/scripts/qmp/qmp-shell
>> +++ b/scripts/qmp/qmp-shell
>> @@ -129,7 +129,7 @@ class QMPShell(qmp.QEMUMonitorProtocol):
>>
>>               < command-name > [ arg-name1=arg1 ] ... [ arg-nameN=argN ]
>>           """
>> -        cmdargs = cmdline.split()
>> +        cmdargs = cmdline.replace("'", '"').split()
>
> This replaces ALL single quotes, even if they would otherwise be
> escaped.  That is, if I pass foo="a\'\"b", it will probably be corrupted.
>
> qmp-shell exists mainly as a convenience for testing, and I doubt
> testers are likely to want to use unbalanced quotes as contents of
> strings, so I can give a weak:
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> But it's still worth thinking about whether there is a more robust
> solution to be used.
>

Oh, hmm. I naively assumed that such quotes weren't permissable within 
the namespace for any valid QMP commands, but...

There's a lot of simplifying assumptions within qmp-shell already and I 
was really hoping to avoid re-architecting the parser where I just 
wanted to add a few "hit and run" improvements.

This patch could be dropped if it poses a serious problem; mostly I 
wanted to ensure that QMP commands could be entered as they are 
displayed (with single quotes!)

I'll look briefly to see if there's a quick win-button for converting 
the quotes in a "safe" way.

--js

Re: [Qemu-devel] [PATCH 3/5] scripts: qmp-shell: allow single-quotes in JSON expressions

[John Snow]

On 04/22/2015 10:34 AM, Eric Blake wrote:
> On 04/21/2015 08:02 PM, John Snow wrote:
>> As a convenience for the user, replace any single quotes given
>> with double quotes so that the data will deserialize correctly
>> via json.loads().
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   scripts/qmp/qmp-shell | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell
>> index 5347f89..d7cb33d 100755
>> --- a/scripts/qmp/qmp-shell
>> +++ b/scripts/qmp/qmp-shell
>> @@ -129,7 +129,7 @@ class QMPShell(qmp.QEMUMonitorProtocol):
>>
>>               < command-name > [ arg-name1=arg1 ] ... [ arg-nameN=argN ]
>>           """
>> -        cmdargs = cmdline.split()
>> +        cmdargs = cmdline.replace("'", '"').split()
>
> This replaces ALL single quotes, even if they would otherwise be
> escaped.  That is, if I pass foo="a\'\"b", it will probably be corrupted.
>
> qmp-shell exists mainly as a convenience for testing, and I doubt
> testers are likely to want to use unbalanced quotes as contents of
> strings, so I can give a weak:
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> But it's still worth thinking about whether there is a more robust
> solution to be used.
>

I done looked it up:

I can probably squash patches 2&3 with a single fix to use the 
ast.literal_eval() function, which does accept single or double quotes, 
since it parses python syntax instead of JSON.

I could probably also just have it 'try' each method until it finds 
something it likes.

--js

Re: [Qemu-devel] [PATCH 3/5] scripts: qmp-shell: allow single-quotes in JSON expressions

[Eric Blake]

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

On 04/22/2015 09:04 AM, John Snow wrote:

>> But it's still worth thinking about whether there is a more robust
>> solution to be used.
>>
> 
> I done looked it up:
> 
> I can probably squash patches 2&3 with a single fix to use the
> ast.literal_eval() function, which does accept single or double quotes,
> since it parses python syntax instead of JSON.

Our QMP parser accepts 'string' as an extension to JSON (which is
"string" only).  That's about the only JSON extension our parser has at
the moment.

Another thing to consider - JSON uses "bool-key":false while Python uses
"bool-key":False (we don't yet support any QMP commands that would
validly require a null argument, but JSON would use "key":null while
Python uses "key":None).  Once you get past that difference in spelling
for True, False, and None (which could be done by string replace of bare
'true', 'false', and 'null'), then JSON is pretty much a strict subset
of valid Python.  Having qmp-shell be friendly and accept both JSON and
Python spellings of the three keywords might actually be a nice hack.
Maybe we should even extend our QMP parser to also accept True in place
of true (output would still always be strict JSON, but there's no harm
in making the input engine parse extensions, as we've already proven
with the 'string' extension).

> 
> I could probably also just have it 'try' each method until it finds
> something it likes.

I haven't used qmp-shell much myself, so at this point, I'll leave it up
to you how much effort you want to put into it.

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

[Qemu-devel] [PATCH 4/5] scripts: qmp-shell: add transaction subshell

[John Snow]

Add a special processing mode to craft transactions.

By entering "transaction(" the shell will enter a special
mode where each subsequent command will be saved as a transaction
instead of executed as an individual command.

The transaction can be submitted by entering ")" on a line by itself.

Examples:

Separate lines:

(QEMU) transaction(
TRANS> block-dirty-bitmap-add node=drive0 name=bitmap1
TRANS> block-dirty-bitmap-clear node=drive0 name=bitmap0
TRANS> )

With a transaction action included on the first line:

(QEMU) transaction( block-dirty-bitmap-add node=drive0 name=bitmap2
TRANS> block-dirty-bitmap-add node=drive0 name=bitmap3
TRANS> )

As a one-liner, with just one transation action:

(QEMU) transaction( block-dirty-bitmap-add node=drive0 name=bitmap0 )

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qmp/qmp-shell | 43 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell
index d7cb33d..21d8f71 100755
--- a/scripts/qmp/qmp-shell
+++ b/scripts/qmp/qmp-shell
@@ -59,6 +59,8 @@ class QMPShell(qmp.QEMUMonitorProtocol):
         self._greeting = None
         self._completer = None
         self._pp = pp
+        self._transmode = False
+        self._actions = list()
 
     def __get_address(self, arg):
         """
@@ -130,6 +132,37 @@ class QMPShell(qmp.QEMUMonitorProtocol):
             < command-name > [ arg-name1=arg1 ] ... [ arg-nameN=argN ]
         """
         cmdargs = cmdline.replace("'", '"').split()
+
+        # Transactional CLI entry/exit:
+        if cmdargs[0] == 'transaction(':
+            self._transmode = True
+            cmdargs.pop(0)
+        elif cmdargs[0] == ')' and self._transmode:
+            self._transmode = False
+            cmdargs.pop(0)
+            if cmdargs:
+                raise QMPShellError("Unexpected input after close of Transaction sub-shell")
+            qmpcmd = { 'execute': 'transaction',
+                       'arguments': { 'actions': self._actions } }
+            self._actions = list()
+            return qmpcmd
+
+        # Nothing to process?
+        if not cmdargs:
+            return None
+
+        # Parse and then cache this Transactional Action
+        if self._transmode:
+            finalize = False
+            action = { 'type': cmdargs[0], 'data': {} }
+            if cmdargs[-1] == ')':
+                cmdargs.pop(-1)
+                finalize = True
+            self.__cli_expr(cmdargs[1:], action['data'])
+            self._actions.append(action)
+            return self.__build_cmd(')') if finalize else None
+
+        # Standard command: parse and return it to be executed.
         qmpcmd = { 'execute': cmdargs[0], 'arguments': {} }
         self.__cli_expr(cmdargs[1:], qmpcmd['arguments'])
         return qmpcmd
@@ -142,6 +175,9 @@ class QMPShell(qmp.QEMUMonitorProtocol):
             print 'command format: <command-name> ',
             print '[arg-name1=arg1] ... [arg-nameN=argN]'
             return True
+        # For transaction mode, we may have just cached the action:
+        if qmpcmd is None:
+            return True
         resp = self.cmd_obj(qmpcmd)
         if resp is None:
             print 'Disconnected'
@@ -162,6 +198,11 @@ class QMPShell(qmp.QEMUMonitorProtocol):
         version = self._greeting['QMP']['version']['qemu']
         print 'Connected to QEMU %d.%d.%d\n' % (version['major'],version['minor'],version['micro'])
 
+    def get_prompt(self):
+        if self._transmode:
+            return "TRANS> "
+        return "(QEMU) "
+
     def read_exec_command(self, prompt):
         """
         Read and execute a command.
@@ -301,7 +342,7 @@ def main():
         die('Could not connect to %s' % addr)
 
     qemu.show_banner()
-    while qemu.read_exec_command('(QEMU) '):
+    while qemu.read_exec_command(qemu.get_prompt()):
         pass
     qemu.close()
 
-- 
2.1.0

Re: [Qemu-devel] [PATCH 4/5] scripts: qmp-shell: add transaction subshell

[Eric Blake]

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

On 04/21/2015 08:02 PM, John Snow wrote:
> Add a special processing mode to craft transactions.
> 
> By entering "transaction(" the shell will enter a special
> mode where each subsequent command will be saved as a transaction
> instead of executed as an individual command.
> 
> The transaction can be submitted by entering ")" on a line by itself.
> 
> Examples:
> 
> Separate lines:
> 
> (QEMU) transaction(
> TRANS> block-dirty-bitmap-add node=drive0 name=bitmap1
> TRANS> block-dirty-bitmap-clear node=drive0 name=bitmap0
> TRANS> )
> 
> With a transaction action included on the first line:
> 
> (QEMU) transaction( block-dirty-bitmap-add node=drive0 name=bitmap2
> TRANS> block-dirty-bitmap-add node=drive0 name=bitmap3
> TRANS> )
> 
> As a one-liner, with just one transation action:

s/transation/transaction/

> 
> (QEMU) transaction( block-dirty-bitmap-add node=drive0 name=bitmap0 )
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qmp/qmp-shell | 43 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 42 insertions(+), 1 deletion(-)

Nice.  And very similar to the code reuse of how I added transaction
support in libvirt (reusing the guts for constructing each action as a
sibling of 'type', then stringing those together to an arguments array
as a sibling of 'execute').

Minor question:

> +
> +        # Nothing to process?
> +        if not cmdargs:
> +            return None
> +

This returns None if we have a blank line, whether or not we are in
transaction mode...

> +        # Parse and then cache this Transactional Action
> +        if self._transmode:
> +            finalize = False
> +            action = { 'type': cmdargs[0], 'data': {} }
> +            if cmdargs[-1] == ')':
> +                cmdargs.pop(-1)
> +                finalize = True
> +            self.__cli_expr(cmdargs[1:], action['data'])
> +            self._actions.append(action)
> +            return self.__build_cmd(')') if finalize else None

and this returns None if we have a non-blank line but remain in
transaction mode...

> @@ -142,6 +175,9 @@ class QMPShell(qmp.QEMUMonitorProtocol):
>              print 'command format: <command-name> ',
>              print '[arg-name1=arg1] ... [arg-nameN=argN]'
>              return True
> +        # For transaction mode, we may have just cached the action:
> +        if qmpcmd is None:
> +            return True

...do we care if the user is entering blank lines even when not in
transaction mode? That is, should this check be tightened to

if qmpcmd is None and self._transmode:

On the other hand, I don't know what the previous behavior was for blank
lines (if an error message was printed or not), and I also think it's
just fine to be silent on a blank line (and save error messages for
non-blank lines that we can't parse).  So I don't think it needs
changing, so much as me trying to figure out what's going on.

Therefore, I'm fine with:

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 4/5] scripts: qmp-shell: add transaction subshell

[John Snow]

On 04/22/2015 10:48 AM, Eric Blake wrote:
> On 04/21/2015 08:02 PM, John Snow wrote:
>> Add a special processing mode to craft transactions.
>>
>> By entering "transaction(" the shell will enter a special
>> mode where each subsequent command will be saved as a transaction
>> instead of executed as an individual command.
>>
>> The transaction can be submitted by entering ")" on a line by itself.
>>
>> Examples:
>>
>> Separate lines:
>>
>> (QEMU) transaction(
>> TRANS> block-dirty-bitmap-add node=drive0 name=bitmap1
>> TRANS> block-dirty-bitmap-clear node=drive0 name=bitmap0
>> TRANS> )
>>
>> With a transaction action included on the first line:
>>
>> (QEMU) transaction( block-dirty-bitmap-add node=drive0 name=bitmap2
>> TRANS> block-dirty-bitmap-add node=drive0 name=bitmap3
>> TRANS> )
>>
>> As a one-liner, with just one transation action:
>
> s/transation/transaction/
>
>>
>> (QEMU) transaction( block-dirty-bitmap-add node=drive0 name=bitmap0 )
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   scripts/qmp/qmp-shell | 43 ++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 42 insertions(+), 1 deletion(-)
>
> Nice.  And very similar to the code reuse of how I added transaction
> support in libvirt (reusing the guts for constructing each action as a
> sibling of 'type', then stringing those together to an arguments array
> as a sibling of 'execute').
>
> Minor question:
>
>> +
>> +        # Nothing to process?
>> +        if not cmdargs:
>> +            return None
>> +
>
> This returns None if we have a blank line, whether or not we are in
> transaction mode...
>

Yeah, not a big deal. The function that invokes this one actually 
explicitly checks for an empty string and avoids __build_cmd already.

If it gets spaces, It actually currently errors out with "list index out 
of range" which is not helpful or interesting.

This will at least improve it to do "nothing."

>> +        # Parse and then cache this Transactional Action
>> +        if self._transmode:
>> +            finalize = False
>> +            action = { 'type': cmdargs[0], 'data': {} }
>> +            if cmdargs[-1] == ')':
>> +                cmdargs.pop(-1)
>> +                finalize = True
>> +            self.__cli_expr(cmdargs[1:], action['data'])
>> +            self._actions.append(action)
>> +            return self.__build_cmd(')') if finalize else None
>
> and this returns None if we have a non-blank line but remain in
> transaction mode...
>

Yup, so the caller doesn't expect an object it needs to send right away.

>> @@ -142,6 +175,9 @@ class QMPShell(qmp.QEMUMonitorProtocol):
>>               print 'command format: <command-name> ',
>>               print '[arg-name1=arg1] ... [arg-nameN=argN]'
>>               return True
>> +        # For transaction mode, we may have just cached the action:
>> +        if qmpcmd is None:
>> +            return True
>
> ...do we care if the user is entering blank lines even when not in
> transaction mode? That is, should this check be tightened to
>

The behavior of just ignoring empty and blank lines is suitable 
regardless of mode, I think, unless you have a counter argument.

For reference, a "False" return here will end the shell. It's seen as 
non-recoverable.

> if qmpcmd is None and self._transmode:
>
> On the other hand, I don't know what the previous behavior was for blank
> lines (if an error message was printed or not), and I also think it's
> just fine to be silent on a blank line (and save error messages for
> non-blank lines that we can't parse).  So I don't think it needs
> changing, so much as me trying to figure out what's going on.
>
> Therefore, I'm fine with:
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>

Re: [Qemu-devel] [PATCH 4/5] scripts: qmp-shell: add transaction subshell

[Eric Blake]

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

On 04/22/2015 09:02 AM, John Snow wrote:

> Yeah, not a big deal. The function that invokes this one actually
> explicitly checks for an empty string and avoids __build_cmd already.
> 
> If it gets spaces, It actually currently errors out with "list index out
> of range" which is not helpful or interesting.
> 
> This will at least improve it to do "nothing."

> 
> The behavior of just ignoring empty and blank lines is suitable
> regardless of mode, I think, unless you have a counter argument.

No counter argument here; but it does mean that we should improve the
commit message, to mention that it is an intentionally nice side effect
of the change :)

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

[Qemu-devel] [PATCH 5/5] scripts: qmp-shell: Add verbose flag

[John Snow]

Add a verbose flag that shows the QMP command that was
constructed, to allow for later copy/pasting, reference,
debugging, etc.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qmp/qmp-shell | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell
index 21d8f71..72473fc 100755
--- a/scripts/qmp/qmp-shell
+++ b/scripts/qmp/qmp-shell
@@ -167,6 +167,12 @@ class QMPShell(qmp.QEMUMonitorProtocol):
         self.__cli_expr(cmdargs[1:], qmpcmd['arguments'])
         return qmpcmd
 
+    def _print(self, qmp):
+        if self._pp is not None:
+            self._pp.pprint(qmp)
+        else:
+            print qmp
+
     def _execute_cmd(self, cmdline):
         try:
             qmpcmd = self.__build_cmd(cmdline)
@@ -178,15 +184,13 @@ class QMPShell(qmp.QEMUMonitorProtocol):
         # For transaction mode, we may have just cached the action:
         if qmpcmd is None:
             return True
+        if self._verbose:
+            self._print(qmpcmd)
         resp = self.cmd_obj(qmpcmd)
         if resp is None:
             print 'Disconnected'
             return False
-
-        if self._pp is not None:
-            self._pp.pprint(resp)
-        else:
-            print resp
+        self._print(resp)
         return True
 
     def connect(self):
@@ -222,6 +226,9 @@ class QMPShell(qmp.QEMUMonitorProtocol):
         else:
             return self._execute_cmd(cmdline)
 
+    def set_verbosity(self, verbose):
+        self._verbose = verbose
+
 class HMPShell(QMPShell):
     def __init__(self, address):
         QMPShell.__init__(self, address)
@@ -307,6 +314,7 @@ def main():
     qemu = None
     hmp = False
     pp = None
+    verbose = False
 
     try:
         for arg in sys.argv[1:]:
@@ -318,6 +326,8 @@ def main():
                 if pp is not None:
                     fail_cmdline(arg)
                 pp = pprint.PrettyPrinter(indent=4)
+            elif arg == "-v":
+                verbose = True
             else:
                 if qemu is not None:
                     fail_cmdline(arg)
@@ -342,6 +352,7 @@ def main():
         die('Could not connect to %s' % addr)
 
     qemu.show_banner()
+    qemu.set_verbosity(verbose)
     while qemu.read_exec_command(qemu.get_prompt()):
         pass
     qemu.close()
-- 
2.1.0

Re: [Qemu-devel] [PATCH 5/5] scripts: qmp-shell: Add verbose flag

[Eric Blake]

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

On 04/21/2015 08:02 PM, John Snow wrote:
> Add a verbose flag that shows the QMP command that was
> constructed, to allow for later copy/pasting, reference,
> debugging, etc.

Very useful.

> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qmp/qmp-shell | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
> 

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