* [Qemu-devel] [PATCH 0/5] scripts: qmp-shell: add transaction support
@ 2015-04-22 2:02 John Snow
2015-04-22 2:02 ` [Qemu-devel] [PATCH 1/5] scripts: qmp-shell: refactor helpers John Snow
` (4 more replies)
0 siblings, 5 replies; 17+ messages in thread
From: John Snow @ 2015-04-22 2:02 UTC (permalink / raw
To: qemu-devel; +Cc: lcapitulino, John Snow, kchamart
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.
John Snow (5):
scripts: qmp-shell: refactor helpers
scripts: qmp-shell: add support for [] expressions
scripts: qmp-shell: allow single-quotes in JSON expressions
scripts: qmp-shell: add transaction subshell
scripts: qmp-shell: Add verbose flag
scripts/qmp/qmp-shell | 90 +++++++++++++++++++++++++++++++++++++++++----------
1 file changed, 73 insertions(+), 17 deletions(-)
--
2.1.0
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH 1/5] scripts: qmp-shell: refactor helpers
2015-04-22 2:02 [Qemu-devel] [PATCH 0/5] scripts: qmp-shell: add transaction support John Snow
@ 2015-04-22 2:02 ` John Snow
2015-04-22 14:25 ` Eric Blake
2015-04-22 2:02 ` [Qemu-devel] [PATCH 2/5] scripts: qmp-shell: add support for [] expressions John Snow
` (3 subsequent siblings)
4 siblings, 1 reply; 17+ messages in thread
From: John Snow @ 2015-04-22 2:02 UTC (permalink / raw
To: qemu-devel; +Cc: lcapitulino, John Snow, kchamart
Refactor the qmp-shell command line processing function
into two components. This will be used to allow sub-expressions,
which will assist us in adding transactional support to qmp-shell.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qmp/qmp-shell | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell
index e0e848b..a9632ec 100755
--- a/scripts/qmp/qmp-shell
+++ b/scripts/qmp/qmp-shell
@@ -88,16 +88,8 @@ class QMPShell(qmp.QEMUMonitorProtocol):
# clearing everything as it doesn't seem to matter
readline.set_completer_delims('')
- def __build_cmd(self, cmdline):
- """
- Build a QMP input object from a user provided command-line in the
- following format:
-
- < command-name > [ arg-name1=arg1 ] ... [ arg-nameN=argN ]
- """
- cmdargs = cmdline.split()
- qmpcmd = { 'execute': cmdargs[0], 'arguments': {} }
- for arg in cmdargs[1:]:
+ def __cli_expr(self, tokens, parent):
+ for arg in tokens:
opt = arg.split('=')
try:
if(len(opt) > 2):
@@ -113,7 +105,6 @@ class QMPShell(qmp.QEMUMonitorProtocol):
else:
value = opt[1]
optpath = opt[0].split('.')
- parent = qmpcmd['arguments']
curpath = []
for p in optpath[:-1]:
curpath.append(p)
@@ -128,6 +119,17 @@ class QMPShell(qmp.QEMUMonitorProtocol):
else:
raise QMPShellError('Cannot set "%s" multiple times' % opt[0])
parent[optpath[-1]] = value
+
+ def __build_cmd(self, cmdline):
+ """
+ Build a QMP input object from a user provided command-line in the
+ following format:
+
+ < command-name > [ arg-name1=arg1 ] ... [ arg-nameN=argN ]
+ """
+ cmdargs = cmdline.split()
+ qmpcmd = { 'execute': cmdargs[0], 'arguments': {} }
+ self.__cli_expr(cmdargs[1:], qmpcmd['arguments'])
return qmpcmd
def _execute_cmd(self, cmdline):
--
2.1.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH 2/5] scripts: qmp-shell: add support for [] expressions
2015-04-22 2:02 [Qemu-devel] [PATCH 0/5] scripts: qmp-shell: add transaction support John Snow
2015-04-22 2:02 ` [Qemu-devel] [PATCH 1/5] scripts: qmp-shell: refactor helpers John Snow
@ 2015-04-22 2:02 ` John Snow
2015-04-22 14:28 ` Eric Blake
2015-04-22 2:02 ` [Qemu-devel] [PATCH 3/5] scripts: qmp-shell: allow single-quotes in JSON expressions John Snow
` (2 subsequent siblings)
4 siblings, 1 reply; 17+ messages in thread
From: John Snow @ 2015-04-22 2:02 UTC (permalink / raw
To: qemu-devel; +Cc: lcapitulino, John Snow, kchamart
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":{...}}]
Add support for arrays.
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 | 2 ++
1 file changed, 2 insertions(+)
diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell
index a9632ec..5347f89 100755
--- a/scripts/qmp/qmp-shell
+++ b/scripts/qmp/qmp-shell
@@ -102,6 +102,8 @@ class QMPShell(qmp.QEMUMonitorProtocol):
value = False
elif opt[1].startswith('{'):
value = json.loads(opt[1])
+ elif opt[1].startswith('['):
+ value = json.loads(opt[1])
else:
value = opt[1]
optpath = opt[0].split('.')
--
2.1.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH 3/5] scripts: qmp-shell: allow single-quotes in JSON expressions
2015-04-22 2:02 [Qemu-devel] [PATCH 0/5] scripts: qmp-shell: add transaction support John Snow
2015-04-22 2:02 ` [Qemu-devel] [PATCH 1/5] scripts: qmp-shell: refactor helpers John Snow
2015-04-22 2:02 ` [Qemu-devel] [PATCH 2/5] scripts: qmp-shell: add support for [] expressions John Snow
@ 2015-04-22 2:02 ` John Snow
2015-04-22 14:34 ` Eric Blake
2015-04-22 2:02 ` [Qemu-devel] [PATCH 4/5] scripts: qmp-shell: add transaction subshell John Snow
2015-04-22 2:02 ` [Qemu-devel] [PATCH 5/5] scripts: qmp-shell: Add verbose flag John Snow
4 siblings, 1 reply; 17+ messages in thread
From: John Snow @ 2015-04-22 2:02 UTC (permalink / raw
To: qemu-devel; +Cc: lcapitulino, John Snow, kchamart
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()
qmpcmd = { 'execute': cmdargs[0], 'arguments': {} }
self.__cli_expr(cmdargs[1:], qmpcmd['arguments'])
return qmpcmd
--
2.1.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH 4/5] scripts: qmp-shell: add transaction subshell
2015-04-22 2:02 [Qemu-devel] [PATCH 0/5] scripts: qmp-shell: add transaction support John Snow
` (2 preceding siblings ...)
2015-04-22 2:02 ` [Qemu-devel] [PATCH 3/5] scripts: qmp-shell: allow single-quotes in JSON expressions John Snow
@ 2015-04-22 2:02 ` John Snow
2015-04-22 14:48 ` Eric Blake
2015-04-22 2:02 ` [Qemu-devel] [PATCH 5/5] scripts: qmp-shell: Add verbose flag John Snow
4 siblings, 1 reply; 17+ messages in thread
From: John Snow @ 2015-04-22 2:02 UTC (permalink / raw
To: qemu-devel; +Cc: lcapitulino, John Snow, kchamart
Add a special processing mode to craft transactions.
By entering "transaction(" the shell will enter a special
mode where each subsequent command will be saved as a transaction
instead of executed as an individual command.
The transaction can be submitted by entering ")" on a line by itself.
Examples:
Separate lines:
(QEMU) transaction(
TRANS> block-dirty-bitmap-add node=drive0 name=bitmap1
TRANS> block-dirty-bitmap-clear node=drive0 name=bitmap0
TRANS> )
With a transaction action included on the first line:
(QEMU) transaction( block-dirty-bitmap-add node=drive0 name=bitmap2
TRANS> block-dirty-bitmap-add node=drive0 name=bitmap3
TRANS> )
As a one-liner, with just one transation action:
(QEMU) transaction( block-dirty-bitmap-add node=drive0 name=bitmap0 )
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qmp/qmp-shell | 43 ++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 42 insertions(+), 1 deletion(-)
diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell
index d7cb33d..21d8f71 100755
--- a/scripts/qmp/qmp-shell
+++ b/scripts/qmp/qmp-shell
@@ -59,6 +59,8 @@ class QMPShell(qmp.QEMUMonitorProtocol):
self._greeting = None
self._completer = None
self._pp = pp
+ self._transmode = False
+ self._actions = list()
def __get_address(self, arg):
"""
@@ -130,6 +132,37 @@ class QMPShell(qmp.QEMUMonitorProtocol):
< command-name > [ arg-name1=arg1 ] ... [ arg-nameN=argN ]
"""
cmdargs = cmdline.replace("'", '"').split()
+
+ # Transactional CLI entry/exit:
+ if cmdargs[0] == 'transaction(':
+ self._transmode = True
+ cmdargs.pop(0)
+ elif cmdargs[0] == ')' and self._transmode:
+ self._transmode = False
+ cmdargs.pop(0)
+ if cmdargs:
+ raise QMPShellError("Unexpected input after close of Transaction sub-shell")
+ qmpcmd = { 'execute': 'transaction',
+ 'arguments': { 'actions': self._actions } }
+ self._actions = list()
+ return qmpcmd
+
+ # Nothing to process?
+ if not cmdargs:
+ return None
+
+ # Parse and then cache this Transactional Action
+ if self._transmode:
+ finalize = False
+ action = { 'type': cmdargs[0], 'data': {} }
+ if cmdargs[-1] == ')':
+ cmdargs.pop(-1)
+ finalize = True
+ self.__cli_expr(cmdargs[1:], action['data'])
+ self._actions.append(action)
+ return self.__build_cmd(')') if finalize else None
+
+ # Standard command: parse and return it to be executed.
qmpcmd = { 'execute': cmdargs[0], 'arguments': {} }
self.__cli_expr(cmdargs[1:], qmpcmd['arguments'])
return qmpcmd
@@ -142,6 +175,9 @@ class QMPShell(qmp.QEMUMonitorProtocol):
print 'command format: <command-name> ',
print '[arg-name1=arg1] ... [arg-nameN=argN]'
return True
+ # For transaction mode, we may have just cached the action:
+ if qmpcmd is None:
+ return True
resp = self.cmd_obj(qmpcmd)
if resp is None:
print 'Disconnected'
@@ -162,6 +198,11 @@ class QMPShell(qmp.QEMUMonitorProtocol):
version = self._greeting['QMP']['version']['qemu']
print 'Connected to QEMU %d.%d.%d\n' % (version['major'],version['minor'],version['micro'])
+ def get_prompt(self):
+ if self._transmode:
+ return "TRANS> "
+ return "(QEMU) "
+
def read_exec_command(self, prompt):
"""
Read and execute a command.
@@ -301,7 +342,7 @@ def main():
die('Could not connect to %s' % addr)
qemu.show_banner()
- while qemu.read_exec_command('(QEMU) '):
+ while qemu.read_exec_command(qemu.get_prompt()):
pass
qemu.close()
--
2.1.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH 5/5] scripts: qmp-shell: Add verbose flag
2015-04-22 2:02 [Qemu-devel] [PATCH 0/5] scripts: qmp-shell: add transaction support John Snow
` (3 preceding siblings ...)
2015-04-22 2:02 ` [Qemu-devel] [PATCH 4/5] scripts: qmp-shell: add transaction subshell John Snow
@ 2015-04-22 2:02 ` John Snow
2015-04-22 14:50 ` Eric Blake
4 siblings, 1 reply; 17+ messages in thread
From: John Snow @ 2015-04-22 2:02 UTC (permalink / raw
To: qemu-devel; +Cc: lcapitulino, John Snow, kchamart
Add a verbose flag that shows the QMP command that was
constructed, to allow for later copy/pasting, reference,
debugging, etc.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qmp/qmp-shell | 21 ++++++++++++++++-----
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell
index 21d8f71..72473fc 100755
--- a/scripts/qmp/qmp-shell
+++ b/scripts/qmp/qmp-shell
@@ -167,6 +167,12 @@ class QMPShell(qmp.QEMUMonitorProtocol):
self.__cli_expr(cmdargs[1:], qmpcmd['arguments'])
return qmpcmd
+ def _print(self, qmp):
+ if self._pp is not None:
+ self._pp.pprint(qmp)
+ else:
+ print qmp
+
def _execute_cmd(self, cmdline):
try:
qmpcmd = self.__build_cmd(cmdline)
@@ -178,15 +184,13 @@ class QMPShell(qmp.QEMUMonitorProtocol):
# For transaction mode, we may have just cached the action:
if qmpcmd is None:
return True
+ if self._verbose:
+ self._print(qmpcmd)
resp = self.cmd_obj(qmpcmd)
if resp is None:
print 'Disconnected'
return False
-
- if self._pp is not None:
- self._pp.pprint(resp)
- else:
- print resp
+ self._print(resp)
return True
def connect(self):
@@ -222,6 +226,9 @@ class QMPShell(qmp.QEMUMonitorProtocol):
else:
return self._execute_cmd(cmdline)
+ def set_verbosity(self, verbose):
+ self._verbose = verbose
+
class HMPShell(QMPShell):
def __init__(self, address):
QMPShell.__init__(self, address)
@@ -307,6 +314,7 @@ def main():
qemu = None
hmp = False
pp = None
+ verbose = False
try:
for arg in sys.argv[1:]:
@@ -318,6 +326,8 @@ def main():
if pp is not None:
fail_cmdline(arg)
pp = pprint.PrettyPrinter(indent=4)
+ elif arg == "-v":
+ verbose = True
else:
if qemu is not None:
fail_cmdline(arg)
@@ -342,6 +352,7 @@ def main():
die('Could not connect to %s' % addr)
qemu.show_banner()
+ qemu.set_verbosity(verbose)
while qemu.read_exec_command(qemu.get_prompt()):
pass
qemu.close()
--
2.1.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 1/5] scripts: qmp-shell: refactor helpers
2015-04-22 2:02 ` [Qemu-devel] [PATCH 1/5] scripts: qmp-shell: refactor helpers John Snow
@ 2015-04-22 14:25 ` Eric Blake
0 siblings, 0 replies; 17+ messages in thread
From: Eric Blake @ 2015-04-22 14:25 UTC (permalink / raw
To: John Snow, qemu-devel; +Cc: kchamart, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 569 bytes --]
On 04/21/2015 08:02 PM, John Snow wrote:
> Refactor the qmp-shell command line processing function
> into two components. This will be used to allow sub-expressions,
> which will assist us in adding transactional support to qmp-shell.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> scripts/qmp/qmp-shell | 24 +++++++++++++-----------
> 1 file changed, 13 insertions(+), 11 deletions(-)
>
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 2/5] scripts: qmp-shell: add support for [] expressions
2015-04-22 2:02 ` [Qemu-devel] [PATCH 2/5] scripts: qmp-shell: add support for [] expressions John Snow
@ 2015-04-22 14:28 ` Eric Blake
2015-04-22 14:31 ` John Snow
0 siblings, 1 reply; 17+ messages in thread
From: Eric Blake @ 2015-04-22 14:28 UTC (permalink / raw
To: John Snow, qemu-devel; +Cc: kchamart, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 1489 bytes --]
On 04/21/2015 08:02 PM, John Snow wrote:
> 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":{...}}]
>
> Add support for arrays.
>
> 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 | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell
> index a9632ec..5347f89 100755
> --- a/scripts/qmp/qmp-shell
> +++ b/scripts/qmp/qmp-shell
> @@ -102,6 +102,8 @@ class QMPShell(qmp.QEMUMonitorProtocol):
> value = False
> elif opt[1].startswith('{'):
> value = json.loads(opt[1])
> + elif opt[1].startswith('['):
> + value = json.loads(opt[1])
Why not:
elif opt[1].startswith('{'} or opt[1].startswith('['):
for a one-line change instead of two-line addition? And I'm no python
expert, but I think you can write that with fewer characters. Untested,
but does this work?
elif opt[1][0] in "{[":
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 2/5] scripts: qmp-shell: add support for [] expressions
2015-04-22 14:28 ` Eric Blake
@ 2015-04-22 14:31 ` John Snow
0 siblings, 0 replies; 17+ messages in thread
From: John Snow @ 2015-04-22 14:31 UTC (permalink / raw
To: Eric Blake, qemu-devel; +Cc: kchamart, lcapitulino
On 04/22/2015 10:28 AM, Eric Blake wrote:
> On 04/21/2015 08:02 PM, John Snow wrote:
>> 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":{...}}]
>>
>> Add support for arrays.
>>
>> 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 | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell
>> index a9632ec..5347f89 100755
>> --- a/scripts/qmp/qmp-shell
>> +++ b/scripts/qmp/qmp-shell
>> @@ -102,6 +102,8 @@ class QMPShell(qmp.QEMUMonitorProtocol):
>> value = False
>> elif opt[1].startswith('{'):
>> value = json.loads(opt[1])
>> + elif opt[1].startswith('['):
>> + value = json.loads(opt[1])
>
> Why not:
>
> elif opt[1].startswith('{'} or opt[1].startswith('['):
>
Didn't really consider this patch too long is why :)
If the rest of it looks worth checking in to you, I'll make the tidying
edits.
> for a one-line change instead of two-line addition? And I'm no python
> expert, but I think you can write that with fewer characters. Untested,
> but does this work?
>
> elif opt[1][0] in "{[":
>
It should.
Thanks,
--js
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] scripts: qmp-shell: allow single-quotes in JSON expressions
2015-04-22 2:02 ` [Qemu-devel] [PATCH 3/5] scripts: qmp-shell: allow single-quotes in JSON expressions John Snow
@ 2015-04-22 14:34 ` Eric Blake
2015-04-22 14:39 ` John Snow
2015-04-22 15:04 ` John Snow
0 siblings, 2 replies; 17+ messages in thread
From: Eric Blake @ 2015-04-22 14:34 UTC (permalink / raw
To: John Snow, qemu-devel; +Cc: kchamart, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 1311 bytes --]
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.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] scripts: qmp-shell: allow single-quotes in JSON expressions
2015-04-22 14:34 ` Eric Blake
@ 2015-04-22 14:39 ` John Snow
2015-04-22 15:04 ` John Snow
1 sibling, 0 replies; 17+ messages in thread
From: John Snow @ 2015-04-22 14:39 UTC (permalink / raw
To: Eric Blake, qemu-devel; +Cc: kchamart, lcapitulino
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 4/5] scripts: qmp-shell: add transaction subshell
2015-04-22 2:02 ` [Qemu-devel] [PATCH 4/5] scripts: qmp-shell: add transaction subshell John Snow
@ 2015-04-22 14:48 ` Eric Blake
2015-04-22 15:02 ` John Snow
0 siblings, 1 reply; 17+ messages in thread
From: Eric Blake @ 2015-04-22 14:48 UTC (permalink / raw
To: John Snow, qemu-devel; +Cc: kchamart, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 3178 bytes --]
On 04/21/2015 08:02 PM, John Snow wrote:
> Add a special processing mode to craft transactions.
>
> By entering "transaction(" the shell will enter a special
> mode where each subsequent command will be saved as a transaction
> instead of executed as an individual command.
>
> The transaction can be submitted by entering ")" on a line by itself.
>
> Examples:
>
> Separate lines:
>
> (QEMU) transaction(
> TRANS> block-dirty-bitmap-add node=drive0 name=bitmap1
> TRANS> block-dirty-bitmap-clear node=drive0 name=bitmap0
> TRANS> )
>
> With a transaction action included on the first line:
>
> (QEMU) transaction( block-dirty-bitmap-add node=drive0 name=bitmap2
> TRANS> block-dirty-bitmap-add node=drive0 name=bitmap3
> TRANS> )
>
> As a one-liner, with just one transation action:
s/transation/transaction/
>
> (QEMU) transaction( block-dirty-bitmap-add node=drive0 name=bitmap0 )
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> scripts/qmp/qmp-shell | 43 ++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 42 insertions(+), 1 deletion(-)
Nice. And very similar to the code reuse of how I added transaction
support in libvirt (reusing the guts for constructing each action as a
sibling of 'type', then stringing those together to an arguments array
as a sibling of 'execute').
Minor question:
> +
> + # Nothing to process?
> + if not cmdargs:
> + return None
> +
This returns None if we have a blank line, whether or not we are in
transaction mode...
> + # Parse and then cache this Transactional Action
> + if self._transmode:
> + finalize = False
> + action = { 'type': cmdargs[0], 'data': {} }
> + if cmdargs[-1] == ')':
> + cmdargs.pop(-1)
> + finalize = True
> + self.__cli_expr(cmdargs[1:], action['data'])
> + self._actions.append(action)
> + return self.__build_cmd(')') if finalize else None
and this returns None if we have a non-blank line but remain in
transaction mode...
> @@ -142,6 +175,9 @@ class QMPShell(qmp.QEMUMonitorProtocol):
> print 'command format: <command-name> ',
> print '[arg-name1=arg1] ... [arg-nameN=argN]'
> return True
> + # For transaction mode, we may have just cached the action:
> + if qmpcmd is None:
> + return True
...do we care if the user is entering blank lines even when not in
transaction mode? That is, should this check be tightened to
if qmpcmd is None and self._transmode:
On the other hand, I don't know what the previous behavior was for blank
lines (if an error message was printed or not), and I also think it's
just fine to be silent on a blank line (and save error messages for
non-blank lines that we can't parse). So I don't think it needs
changing, so much as me trying to figure out what's going on.
Therefore, I'm fine with:
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 5/5] scripts: qmp-shell: Add verbose flag
2015-04-22 2:02 ` [Qemu-devel] [PATCH 5/5] scripts: qmp-shell: Add verbose flag John Snow
@ 2015-04-22 14:50 ` Eric Blake
0 siblings, 0 replies; 17+ messages in thread
From: Eric Blake @ 2015-04-22 14:50 UTC (permalink / raw
To: John Snow, qemu-devel; +Cc: kchamart, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 523 bytes --]
On 04/21/2015 08:02 PM, John Snow wrote:
> Add a verbose flag that shows the QMP command that was
> constructed, to allow for later copy/pasting, reference,
> debugging, etc.
Very useful.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> scripts/qmp/qmp-shell | 21 ++++++++++++++++-----
> 1 file changed, 16 insertions(+), 5 deletions(-)
>
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 4/5] scripts: qmp-shell: add transaction subshell
2015-04-22 14:48 ` Eric Blake
@ 2015-04-22 15:02 ` John Snow
2015-04-22 15:28 ` Eric Blake
0 siblings, 1 reply; 17+ messages in thread
From: John Snow @ 2015-04-22 15:02 UTC (permalink / raw
To: Eric Blake, qemu-devel; +Cc: kchamart, lcapitulino
On 04/22/2015 10:48 AM, Eric Blake wrote:
> On 04/21/2015 08:02 PM, John Snow wrote:
>> Add a special processing mode to craft transactions.
>>
>> By entering "transaction(" the shell will enter a special
>> mode where each subsequent command will be saved as a transaction
>> instead of executed as an individual command.
>>
>> The transaction can be submitted by entering ")" on a line by itself.
>>
>> Examples:
>>
>> Separate lines:
>>
>> (QEMU) transaction(
>> TRANS> block-dirty-bitmap-add node=drive0 name=bitmap1
>> TRANS> block-dirty-bitmap-clear node=drive0 name=bitmap0
>> TRANS> )
>>
>> With a transaction action included on the first line:
>>
>> (QEMU) transaction( block-dirty-bitmap-add node=drive0 name=bitmap2
>> TRANS> block-dirty-bitmap-add node=drive0 name=bitmap3
>> TRANS> )
>>
>> As a one-liner, with just one transation action:
>
> s/transation/transaction/
>
>>
>> (QEMU) transaction( block-dirty-bitmap-add node=drive0 name=bitmap0 )
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> scripts/qmp/qmp-shell | 43 ++++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 42 insertions(+), 1 deletion(-)
>
> Nice. And very similar to the code reuse of how I added transaction
> support in libvirt (reusing the guts for constructing each action as a
> sibling of 'type', then stringing those together to an arguments array
> as a sibling of 'execute').
>
> Minor question:
>
>> +
>> + # Nothing to process?
>> + if not cmdargs:
>> + return None
>> +
>
> This returns None if we have a blank line, whether or not we are in
> transaction mode...
>
Yeah, not a big deal. The function that invokes this one actually
explicitly checks for an empty string and avoids __build_cmd already.
If it gets spaces, It actually currently errors out with "list index out
of range" which is not helpful or interesting.
This will at least improve it to do "nothing."
>> + # Parse and then cache this Transactional Action
>> + if self._transmode:
>> + finalize = False
>> + action = { 'type': cmdargs[0], 'data': {} }
>> + if cmdargs[-1] == ')':
>> + cmdargs.pop(-1)
>> + finalize = True
>> + self.__cli_expr(cmdargs[1:], action['data'])
>> + self._actions.append(action)
>> + return self.__build_cmd(')') if finalize else None
>
> and this returns None if we have a non-blank line but remain in
> transaction mode...
>
Yup, so the caller doesn't expect an object it needs to send right away.
>> @@ -142,6 +175,9 @@ class QMPShell(qmp.QEMUMonitorProtocol):
>> print 'command format: <command-name> ',
>> print '[arg-name1=arg1] ... [arg-nameN=argN]'
>> return True
>> + # For transaction mode, we may have just cached the action:
>> + if qmpcmd is None:
>> + return True
>
> ...do we care if the user is entering blank lines even when not in
> transaction mode? That is, should this check be tightened to
>
The behavior of just ignoring empty and blank lines is suitable
regardless of mode, I think, unless you have a counter argument.
For reference, a "False" return here will end the shell. It's seen as
non-recoverable.
> if qmpcmd is None and self._transmode:
>
> On the other hand, I don't know what the previous behavior was for blank
> lines (if an error message was printed or not), and I also think it's
> just fine to be silent on a blank line (and save error messages for
> non-blank lines that we can't parse). So I don't think it needs
> changing, so much as me trying to figure out what's going on.
>
> Therefore, I'm fine with:
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] scripts: qmp-shell: allow single-quotes in JSON expressions
2015-04-22 14:34 ` Eric Blake
2015-04-22 14:39 ` John Snow
@ 2015-04-22 15:04 ` John Snow
2015-04-22 15:18 ` Eric Blake
1 sibling, 1 reply; 17+ messages in thread
From: John Snow @ 2015-04-22 15:04 UTC (permalink / raw
To: Eric Blake, qemu-devel; +Cc: kchamart, lcapitulino
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] scripts: qmp-shell: allow single-quotes in JSON expressions
2015-04-22 15:04 ` John Snow
@ 2015-04-22 15:18 ` Eric Blake
0 siblings, 0 replies; 17+ messages in thread
From: Eric Blake @ 2015-04-22 15:18 UTC (permalink / raw
To: John Snow, qemu-devel; +Cc: kchamart, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 1681 bytes --]
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.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 4/5] scripts: qmp-shell: add transaction subshell
2015-04-22 15:02 ` John Snow
@ 2015-04-22 15:28 ` Eric Blake
0 siblings, 0 replies; 17+ messages in thread
From: Eric Blake @ 2015-04-22 15:28 UTC (permalink / raw
To: John Snow, qemu-devel; +Cc: kchamart, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 786 bytes --]
On 04/22/2015 09:02 AM, John Snow wrote:
> Yeah, not a big deal. The function that invokes this one actually
> explicitly checks for an empty string and avoids __build_cmd already.
>
> If it gets spaces, It actually currently errors out with "list index out
> of range" which is not helpful or interesting.
>
> This will at least improve it to do "nothing."
>
> The behavior of just ignoring empty and blank lines is suitable
> regardless of mode, I think, unless you have a counter argument.
No counter argument here; but it does mean that we should improve the
commit message, to mention that it is an intentionally nice side effect
of the change :)
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2015-04-22 15:28 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-04-22 2:02 [Qemu-devel] [PATCH 0/5] scripts: qmp-shell: add transaction support John Snow
2015-04-22 2:02 ` [Qemu-devel] [PATCH 1/5] scripts: qmp-shell: refactor helpers John Snow
2015-04-22 14:25 ` Eric Blake
2015-04-22 2:02 ` [Qemu-devel] [PATCH 2/5] scripts: qmp-shell: add support for [] expressions John Snow
2015-04-22 14:28 ` Eric Blake
2015-04-22 14:31 ` John Snow
2015-04-22 2:02 ` [Qemu-devel] [PATCH 3/5] scripts: qmp-shell: allow single-quotes in JSON expressions John Snow
2015-04-22 14:34 ` Eric Blake
2015-04-22 14:39 ` John Snow
2015-04-22 15:04 ` John Snow
2015-04-22 15:18 ` Eric Blake
2015-04-22 2:02 ` [Qemu-devel] [PATCH 4/5] scripts: qmp-shell: add transaction subshell John Snow
2015-04-22 14:48 ` Eric Blake
2015-04-22 15:02 ` John Snow
2015-04-22 15:28 ` Eric Blake
2015-04-22 2:02 ` [Qemu-devel] [PATCH 5/5] scripts: qmp-shell: Add verbose flag John Snow
2015-04-22 14:50 ` Eric Blake
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.