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

[John Snow <jsnow@redhat.com>]

On 04/28/2015 12:17 PM, Kashyap Chamarthy wrote:
> 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.
>

Yeah, I NACKed the version sent to list because I need to review it more 
after some hasty changes last week. I apparently made some cleanup edits 
that I thought didn't change anything, but very clearly they do :)

Might as well take the time to try to improve it a little bit more while 
I'm at it.

--js