qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: John Snow
Subject: Re: [Qemu-devel] [PATCH 4/5] scripts: qmp-shell: add transaction subshell
Date: Wed, 22 Apr 2015 11:02:46 -0400
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0



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 <address@hidden>
---
  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 <address@hidden>




reply via email to

[Prev in Thread] Current Thread [Next in Thread]