| Message ID | 1429719709-880-3-git-send-email-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <jsnow@redhat.com> > --- > 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 <eblake@redhat.com> > > 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.
On 04/22/2015 11:18 AM, 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. >> >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> 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 <eblake@redhat.com> When combined with your one-line touchup, of course.
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 <jsnow@redhat.com> >> --- >> 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 <eblake@redhat.com> > >> >> 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
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 + if val.startswith(('{', '[')): + try: + return json.loads(val) + except ValueError: + pass + try: + return ast.literal_eval(val) + except SyntaxError: + pass + return val + def __cli_expr(self, tokens, parent): for arg in tokens: - opt = arg.split('=') - try: - if(len(opt) > 2): - opt[1] = '='.join(opt[1:]) - value = int(opt[1]) - except ValueError: - if opt[1] == 'true': - value = True - elif opt[1] == 'false': - value = False - elif opt[1].startswith('{'): - value = json.loads(opt[1]) - else: - value = opt[1] - optpath = opt[0].split('.') + (key, _, val) = arg.partition('=') + if not val: + raise QMPShellError("Expected a key=value pair, got '%s'" % arg) + + value = __parse_value(val) + optpath = key.split('.') curpath = [] for p in optpath[:-1]: curpath.append(p) @@ -117,7 +129,7 @@ class QMPShell(qmp.QEMUMonitorProtocol): if type(parent[optpath[-1]]) is dict: raise QMPShellError('Cannot use "%s" as both leaf and non-leaf key' % '.'.join(curpath)) else: - raise QMPShellError('Cannot set "%s" multiple times' % opt[0]) + raise QMPShellError('Cannot set "%s" multiple times' % key) parent[optpath[-1]] = value def __build_cmd(self, cmdline):
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. 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. Signed-off-by: John Snow <jsnow@redhat.com> --- scripts/qmp/qmp-shell | 44 ++++++++++++++++++++++++++++---------------- 1 file changed, 28 insertions(+), 16 deletions(-)