| Message ID | 1429668155-1606-4-git-send-email-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 04/21/2015 08:02 PM, John Snow wrote: > As a convenience for the user, replace any single quotes given > with double quotes so that the data will deserialize correctly > via json.loads(). > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > scripts/qmp/qmp-shell | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell > index 5347f89..d7cb33d 100755 > --- a/scripts/qmp/qmp-shell > +++ b/scripts/qmp/qmp-shell > @@ -129,7 +129,7 @@ class QMPShell(qmp.QEMUMonitorProtocol): > > < command-name > [ arg-name1=arg1 ] ... [ arg-nameN=argN ] > """ > - cmdargs = cmdline.split() > + cmdargs = cmdline.replace("'", '"').split() This replaces ALL single quotes, even if they would otherwise be escaped. That is, if I pass foo="a\'\"b", it will probably be corrupted. qmp-shell exists mainly as a convenience for testing, and I doubt testers are likely to want to use unbalanced quotes as contents of strings, so I can give a weak: Reviewed-by: Eric Blake <eblake@redhat.com> But it's still worth thinking about whether there is a more robust solution to be used.
On 04/22/2015 10:34 AM, Eric Blake wrote: > On 04/21/2015 08:02 PM, John Snow wrote: >> As a convenience for the user, replace any single quotes given >> with double quotes so that the data will deserialize correctly >> via json.loads(). >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> scripts/qmp/qmp-shell | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell >> index 5347f89..d7cb33d 100755 >> --- a/scripts/qmp/qmp-shell >> +++ b/scripts/qmp/qmp-shell >> @@ -129,7 +129,7 @@ class QMPShell(qmp.QEMUMonitorProtocol): >> >> < command-name > [ arg-name1=arg1 ] ... [ arg-nameN=argN ] >> """ >> - cmdargs = cmdline.split() >> + cmdargs = cmdline.replace("'", '"').split() > > This replaces ALL single quotes, even if they would otherwise be > escaped. That is, if I pass foo="a\'\"b", it will probably be corrupted. > > qmp-shell exists mainly as a convenience for testing, and I doubt > testers are likely to want to use unbalanced quotes as contents of > strings, so I can give a weak: > Reviewed-by: Eric Blake <eblake@redhat.com> > > But it's still worth thinking about whether there is a more robust > solution to be used. > Oh, hmm. I naively assumed that such quotes weren't permissable within the namespace for any valid QMP commands, but... There's a lot of simplifying assumptions within qmp-shell already and I was really hoping to avoid re-architecting the parser where I just wanted to add a few "hit and run" improvements. This patch could be dropped if it poses a serious problem; mostly I wanted to ensure that QMP commands could be entered as they are displayed (with single quotes!) I'll look briefly to see if there's a quick win-button for converting the quotes in a "safe" way. --js
On 04/22/2015 10:34 AM, Eric Blake wrote: > On 04/21/2015 08:02 PM, John Snow wrote: >> As a convenience for the user, replace any single quotes given >> with double quotes so that the data will deserialize correctly >> via json.loads(). >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> scripts/qmp/qmp-shell | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell >> index 5347f89..d7cb33d 100755 >> --- a/scripts/qmp/qmp-shell >> +++ b/scripts/qmp/qmp-shell >> @@ -129,7 +129,7 @@ class QMPShell(qmp.QEMUMonitorProtocol): >> >> < command-name > [ arg-name1=arg1 ] ... [ arg-nameN=argN ] >> """ >> - cmdargs = cmdline.split() >> + cmdargs = cmdline.replace("'", '"').split() > > This replaces ALL single quotes, even if they would otherwise be > escaped. That is, if I pass foo="a\'\"b", it will probably be corrupted. > > qmp-shell exists mainly as a convenience for testing, and I doubt > testers are likely to want to use unbalanced quotes as contents of > strings, so I can give a weak: > Reviewed-by: Eric Blake <eblake@redhat.com> > > But it's still worth thinking about whether there is a more robust > solution to be used. > I done looked it up: I can probably squash patches 2&3 with a single fix to use the ast.literal_eval() function, which does accept single or double quotes, since it parses python syntax instead of JSON. I could probably also just have it 'try' each method until it finds something it likes. --js
On 04/22/2015 09:04 AM, John Snow wrote: >> But it's still worth thinking about whether there is a more robust >> solution to be used. >> > > I done looked it up: > > I can probably squash patches 2&3 with a single fix to use the > ast.literal_eval() function, which does accept single or double quotes, > since it parses python syntax instead of JSON. Our QMP parser accepts 'string' as an extension to JSON (which is "string" only). That's about the only JSON extension our parser has at the moment. Another thing to consider - JSON uses "bool-key":false while Python uses "bool-key":False (we don't yet support any QMP commands that would validly require a null argument, but JSON would use "key":null while Python uses "key":None). Once you get past that difference in spelling for True, False, and None (which could be done by string replace of bare 'true', 'false', and 'null'), then JSON is pretty much a strict subset of valid Python. Having qmp-shell be friendly and accept both JSON and Python spellings of the three keywords might actually be a nice hack. Maybe we should even extend our QMP parser to also accept True in place of true (output would still always be strict JSON, but there's no harm in making the input engine parse extensions, as we've already proven with the 'string' extension). > > I could probably also just have it 'try' each method until it finds > something it likes. I haven't used qmp-shell much myself, so at this point, I'll leave it up to you how much effort you want to put into it.
diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell index 5347f89..d7cb33d 100755 --- a/scripts/qmp/qmp-shell +++ b/scripts/qmp/qmp-shell @@ -129,7 +129,7 @@ class QMPShell(qmp.QEMUMonitorProtocol): < command-name > [ arg-name1=arg1 ] ... [ arg-nameN=argN ] """ - cmdargs = cmdline.split() + cmdargs = cmdline.replace("'", '"').split() qmpcmd = { 'execute': cmdargs[0], 'arguments': {} } self.__cli_expr(cmdargs[1:], qmpcmd['arguments']) return qmpcmd
As a convenience for the user, replace any single quotes given with double quotes so that the data will deserialize correctly via json.loads(). Signed-off-by: John Snow <jsnow@redhat.com> --- scripts/qmp/qmp-shell | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)