From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41467) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yn8Ed-0000AZ-5c for qemu-devel@nongnu.org; Tue, 28 Apr 2015 12:19:48 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Yn8EZ-0007sw-5J for qemu-devel@nongnu.org; Tue, 28 Apr 2015 12:19:47 -0400 Received: from mx1.redhat.com ([209.132.183.28]:47870) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yn8EY-0007sq-Sa for qemu-devel@nongnu.org; Tue, 28 Apr 2015 12:19:43 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t3SGJgij011215 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Tue, 28 Apr 2015 12:19:42 -0400 Message-ID: <553FB31D.5020704@redhat.com> Date: Tue, 28 Apr 2015 12:19:41 -0400 From: John Snow MIME-Version: 1.0 References: <1429799701-27089-1-git-send-email-jsnow@redhat.com> <20150423162331.GE10067@tesla.redhat.com> <20150428161732.GD11726@tesla.redhat.com> In-Reply-To: <20150428161732.GD11726@tesla.redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 0/4] scripts: qmp-shell: add transaction support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kashyap Chamarthy Cc: qemu-devel@nongnu.org, lcapitulino@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: [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