Re: [Qemu-devel] [PATCH v3 0/4] scripts: qmp-shell: add transaction support

[Kashyap Chamarthy]

On Thu, Apr 23, 2015 at 06:23:31PM +0200, Kashyap Chamarthy wrote:
> On Thu, Apr 23, 2015 at 10:34:57AM -0400, John Snow wrote:
> > The qmp-shell is a little rudimentary, but it can be hacked
> > to give us some transactional support without too much difficulty.
> > 
> > (1) Prep.
> > (2) Add support for serializing json arrays
> > (3) Allow users to use 'single quotes' instead of "double quotes"
> > (4) Add a special transaction( ... ) syntax that lets users
> >     build up transactional commands using the existing qmp shell
> >     syntax to define each action.
> > (5) Add a verbose flag to display generated QMP commands.
> > 
> > The parsing is not as robust as one would like, but this suffices
> > without adding a proper parser.
> > 
> > Design considerations:
> > 
> > (1) Try not to disrupt the existing design of the qmp-shell. The existing
> >     API is not disturbed.
> > 
> > (2) Pick a "magic token" such that it could not be confused for legitimate
> >     QMP/JSON syntax. Parentheses are used for this purpose.
> > 
> > ===
> > v3:
> > ===
> > 
> >  - Folding in hotfix from list (import ast)
> 
> Seems like a regression from your v2.
> 
> It fails here, even for a non-transaction command, with your patch series
> applied:
> 
>   (QEMU) blockdev-snapshot-internal-sync device=drive-ide0-0-0 name=snapshot0
>   Error while parsing command line: global name '_QMPShell__parse_value' is 
> not defined
>   command format: <command-name>  [arg-name1=arg1] ... [arg-nameN=argN]

I now tested again with your qmp-shell-plus branch:

  $ git describe
  v2.3.0-rc4-4-g994af97

  $ git log --oneline | head -4
  994af97 scripts: qmp-shell: Add verbose flag
  1009369 scripts: qmp-shell: add transaction subshell
  0ae65ff scripts: qmp-shell: Expand support for QMP expressions
  5f367d9 scripts: qmp-shell: refactor helpers

Result:

  - The non-transaction commands work just fine; so that regression is
    fixed in your qmp-shell-plus branch.
  - The transaction command (same commands as tested previously,
    retained it below) still fails.

Just wanted to let you know here.

-- 
/kashyap

> Earlier the day, with v2, I was able to test it correctly:
> 
>  (QEMU) transaction(
>  TRANS> blockdev-snapshot-sync device=drive-ide0-0-0 
> snapshot-file=./ext-snap1.qcow2 format=qcow2
>  TRANS> blockdev-snapshot-internal-sync device=drive-ide0-0-0 name=snapshot0
>  TRANS> )
>  {u'return': {}}
>  (QEMU)
>