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

[John Snow <jsnow@redhat.com>]

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