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

[Eric Blake <eblake@redhat.com>]

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