From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48823) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YkyWf-000539-PH for qemu-devel@nongnu.org; Wed, 22 Apr 2015 13:33:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YkyWc-0007RD-Te for qemu-devel@nongnu.org; Wed, 22 Apr 2015 13:33:29 -0400 Received: from mx1.redhat.com ([209.132.183.28]:37794) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YkyWc-0007QY-MY for qemu-devel@nongnu.org; Wed, 22 Apr 2015 13:33:26 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t3MHXQLn022997 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Wed, 22 Apr 2015 13:33:26 -0400 Message-ID: <5537DB64.6010201@redhat.com> Date: Wed, 22 Apr 2015 13:33:24 -0400 From: John Snow MIME-Version: 1.0 References: <1429719709-880-1-git-send-email-jsnow@redhat.com> <1429719709-880-3-git-send-email-jsnow@redhat.com> <5537D7F1.1090700@redhat.com> In-Reply-To: <5537D7F1.1090700@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 2/4] scripts: qmp-shell: Expand support for QMP expressions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-devel@nongnu.org Cc: kchamart@redhat.com, lcapitulino@redhat.com On 04/22/2015 01:18 PM, Eric Blake wrote: > On 04/22/2015 10:21 AM, John Snow wrote: >> This includes support for [] expressions, single-quotes in >> QMP expressions (which is not strictly a part of JSON), and >> the ability to use "True" or "False" literals instead of >> JSON's lowercased 'true' and 'false' literals. >> >> qmp-shell currently allows you to describe values as >> JSON expressions: >> key={"key":{"key2":"val"}} >> >> But it does not currently support arrays, which are needed >> for serializing and deserializing transactions: >> key=[{"type":"drive-backup","data":{...}}] >> >> qmp-shell also only currently accepts doubly quoted strings >> as-per JSON spec, but QMP allows single quotes. >> >> Lastly, python allows you to utilize "True" or "False" as >> boolean literals, but JSON expects "true" or "false". Expand >> qmp-shell to allow the user to type either, converting to the >> correct type. > > Well, when easy to do. See below for more discussion... > >> >> As a consequence of the above, the key=val parsing is also improved >> to give better error messages if a key=val token is not provided. >> >> CAVEAT: The parser is still extremely rudimentary and does not >> expect to find spaces in {} nor [] expressions. This patch does >> not improve this functionality. > > If someone cares about this, they can fix it later. As I said in the v1 > review, qmp-shell is mostly for debugging and testsuites, and we can > live with the caveats for now. > >> >> Signed-off-by: John Snow >> --- >> scripts/qmp/qmp-shell | 44 ++++++++++++++++++++++++++++---------------- >> 1 file changed, 28 insertions(+), 16 deletions(-) > > As far as I can tell, this makes qmp-shell strictly more useful, so I > have no qualms giving: > > Reviewed-by: Eric Blake > >> >> diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell >> index a9632ec..4cdcb6c 100755 >> --- a/scripts/qmp/qmp-shell >> +++ b/scripts/qmp/qmp-shell >> @@ -88,23 +88,35 @@ class QMPShell(qmp.QEMUMonitorProtocol): >> # clearing everything as it doesn't seem to matter >> readline.set_completer_delims('') >> >> + def __parse_value(self, val): >> + try: >> + return int(val) >> + except ValueError: >> + pass >> + >> + if val.lower() == 'true': >> + return True >> + if val.lower() == 'false': >> + return False > > ...as promised earlier, > > This allows param=tRuE (neither JSON nor Python), but that's a minor > price to pay to get both param=true (param=JSON style) and param=True > (param=Python style) to work. Meanwhile... > >> + if val.startswith(('{', '[')): >> + try: >> + return json.loads(val) > > This statement accepts param=JSON style, as in: > param={ "foo":true } > but it rejects (as invalid JSON): > param={ 'foo':true } > param={ "foo":True } > param={ 'foo':True } > >> + except ValueError: >> + pass >> + try: >> + return ast.literal_eval(val) > > And this statement accepts param=Python style, as in: > param={ "foo":True } > param={ 'foo':True } > but it rejects (as invalid Python): > param={ "foo":true } > param={ 'foo':true } > > Of those four combinations, QMP accepts: > param={ "foo":true } > param={ 'foo':true } > while rejecting: > param={ "foo":True } > param={ 'foo':True } > > So among the four combinations of single/double quoting vs upper/lower > casing of boolean literals, qmp-shell now accepts three variants, but we > have a case that QMP accepts but qmp-shell rejects (single quotes mixed > with lower-case true). > > Not the end of the world (again, if someone wants all four cases to work > in qmp-shell and/or QMP proper, they can provide a patch later to > further enhance things). But maybe worth expanding that CAVEAT in the > commit message to acknowledge the issues. > > At any rate, doesn't affect my R-b given above. > Yes, unfortunately the object literals still must comply with some sort of standard, neither of which is truly QMP: They must either be Python or JSON. key=val pairs can/will accept true/TRUE/tRuE etc as well, which I think is probably acceptable since that falls within the realm of "qmp-shell syntax" which doesn't necessarily have to maintain similarities to JSON/QMP/etc. I could probably go back and adjust it to just lowercase the first letter, but this is probably only ever going to be a typo (which we can unambiguously resolve) and the generated QMP (if -v is present) will of course appear correctly formatted. GIQO: Garbage in, QMP out? :) I will author an additional patch as an add-on a bit later that documents all the quirks to help improve the usability of this little tool. Probably I'll also update the QMP banner to display a quick help message. Thank you, --John