qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: John Snow
Subject: Re: [Qemu-devel] [PATCH v3 0/4] scripts: qmp-shell: add transaction support
Date: Tue, 28 Apr 2015 12:19:41 -0400
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0



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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]