scripts: qmp-shell: add transaction support

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 <[email protected]>

---
 scripts/qmp/qmp-shell | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

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 <[email protected]>

---
 scripts/qmp/qmp-shell | 2 ++
 1 file changed, 2 insertions(+)

scripts/qmp/qmp-shell +0 -0

index a9632ec..5347f89 100755

Quoted 26 lines · 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 <[email protected]>

---
  scripts/qmp/qmp-shell | 2 ++
  1 file changed, 2 insertions(+)

scripts/qmp/qmp-shell +0 -0

index a9632ec..5347f89 100755

scripts/qmp/qmp-shell class QMPShell(qmp.QEMUMonitorProtocol): · line 106

Quoted 5 lines

scripts/qmp/qmp-shell +2 -0

@@ -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 "{[":

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 <[email protected]>

---
 scripts/qmp/qmp-shell | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

scripts/qmp/qmp-shell +0 -0

index 5347f89..d7cb33d 100755

Quoted 14 lines · 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 <[email protected]>

---
  scripts/qmp/qmp-shell | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

scripts/qmp/qmp-shell +0 -0

index 5347f89..d7cb33d 100755

scripts/qmp/qmp-shell class QMPShell(qmp.QEMUMonitorProtocol): · line 132

Quoted 5 lines

scripts/qmp/qmp-shell +1 -1

@@ -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 <[email protected]>

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

Quoted 14 lines · 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 <[email protected]>

---
  scripts/qmp/qmp-shell | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

scripts/qmp/qmp-shell +0 -0

index 5347f89..d7cb33d 100755

scripts/qmp/qmp-shell class QMPShell(qmp.QEMUMonitorProtocol): · line 132

Quoted 5 lines

scripts/qmp/qmp-shell +1 -1

@@ -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 <[email protected]>

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

Quoted 3 lines

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.

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 <[email protected]>

---
 scripts/qmp/qmp-shell | 43 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

Quoted 17 lines · 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> )

Quoted 7 lines

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/

Quoted 7 lines

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

Signed-off-by John Snow <[email protected]>

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

scripts/qmp/qmp-shell class QMPShell(qmp.QEMUMonitorProtocol): · line 153

Quoted 5 lines

scripts/qmp/qmp-shell +5 -0

+
+        # 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...

scripts/qmp/qmp-shell class QMPShell(qmp.QEMUMonitorProtocol): · line 163

Quoted 10 lines

scripts/qmp/qmp-shell +10 -0

+        # 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...

scripts/qmp/qmp-shell class QMPShell(qmp.QEMUMonitorProtocol): · line 180

Quoted 7 lines

scripts/qmp/qmp-shell +3 -0

@@ -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 <[email protected]>

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.

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 <[email protected]>

---
 scripts/qmp/qmp-shell | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)