| Message ID | 1429668155-1606-5-git-send-email-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
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>
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> >
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 :)
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()
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(-)