Re: [Qemu-devel] [PATCH v2] qmp-shell: fix pretty printing of JSON responses

[Markus Armbruster <armbru@redhat.com>]

John Snow <jsnow@redhat.com> writes:

> On 02/23/2016 05:51 AM, Daniel P. Berrange wrote:
>> Pretty printing of JSON responses is important to be able to understand
>> large responses from query commands in particular. Unfortunately this
>> was broken during the addition of the verbose flag in
>> 
>>   commit 1ceca07e48ead0dd2e41576c81d40e6a91cafefd
>>   Author: John Snow <jsnow@redhat.com>
>>   Date:   Wed Apr 29 15:14:04 2015 -0400
>> 
>>     scripts: qmp-shell: Add verbose flag
>> 
>> This is because that change turned the python data structure into a
>> formatted JSON string before the pretty print was given it. So we're
>> just pretty printing a string, which is a no-op.
>> 
>
> Mea Culpa.
>
>> The original pretty printer would output python objects.
>> 
>> (QEMU) query-chardev
>> {   u'return': [   {   u'filename': u'vc',
>>                        u'frontend-open': False,
>>                        u'label': u'parallel0'},
>>                    {   u'filename': u'vc',
>>                        u'frontend-open': True,
>>                        u'label': u'serial0'},
>>                    {   u'filename': u'unix:/tmp/qemp,server',
>>                        u'frontend-open': True,
>>                        u'label': u'compat_monitor0'}]}
>> 
>> This fixes the problem by switching to outputting pretty formatted JSON
>> text instead. This has the added benefit that the pretty printed output
>> is now valid JSON text. Due to the way the verbose flag was handled, the
>> pretty printing now applies to the command sent, as well as its response:
>> 
>> (QEMU) query-chardev
>> {
>>     "execute": "query-chardev",
>>     "arguments": {}
>> }
>> {
>>     "return": [
>>         {
>>             "frontend-open": false,
>>             "label": "parallel0",
>>             "filename": "vc"
>>         },
>>         {
>>             "frontend-open": true,
>>             "label": "serial0",
>>             "filename": "vc"
>>         },
>>         {
>>             "frontend-open": true,
>>             "label": "compat_monitor0",
>>             "filename": "unix:/tmp/qmp,server"
>>         }
>>     ]
>> }
>> 
>
> That's good news as far as I am concerned.
>
>> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
>> ---
>>  scripts/qmp/qmp-shell | 23 ++++++++++-------------
>>  1 file changed, 10 insertions(+), 13 deletions(-)
>> 
>> diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell
>> index 7a402ed..0373b24 100755
>> --- a/scripts/qmp/qmp-shell
>> +++ b/scripts/qmp/qmp-shell
>> @@ -70,7 +70,6 @@ import json
>>  import ast
>>  import readline
>>  import sys
>> -import pprint
>>  
>>  class QMPCompleter(list):
>>      def complete(self, text, state):
>> @@ -103,11 +102,11 @@ class FuzzyJSON(ast.NodeTransformer):
>>  # TODO: QMPShell's interface is a bit ugly (eg. _fill_completion() and
>>  #       _execute_cmd()). Let's design a better one.
>
> ^ Heh ^
>
>>  class QMPShell(qmp.QEMUMonitorProtocol):
>> -    def __init__(self, address, pp=None):
>> +    def __init__(self, address, pretty=False):
>>          qmp.QEMUMonitorProtocol.__init__(self, self.__get_address(address))
>>          self._greeting = None
>>          self._completer = None
>> -        self._pp = pp
>> +        self._pretty = pretty
>>          self._transmode = False
>>          self._actions = list()
>>  
>> @@ -231,11 +230,11 @@ class QMPShell(qmp.QEMUMonitorProtocol):
>>          return qmpcmd
>>  
>>      def _print(self, qmp):
>> -        jsobj = json.dumps(qmp)
>> -        if self._pp is not None:
>> -            self._pp.pprint(jsobj)
>> -        else:
>> -            print str(jsobj)
>> +        indent = None
>> +        if self._pretty:
>> +            indent = 4
>> +        jsobj = json.dumps(qmp, indent=indent)
>> +        print str(jsobj)
>>  
>>      def _execute_cmd(self, cmdline):
>>          try:
>> @@ -377,7 +376,7 @@ def main():
>>      addr = ''
>>      qemu = None
>>      hmp = False
>> -    pp = None
>> +    pretty = False
>>      verbose = False
>>  
>>      try:
>> @@ -387,9 +386,7 @@ def main():
>>                      fail_cmdline(arg)
>>                  hmp = True
>>              elif arg == "-p":
>> -                if pp is not None:
>> -                    fail_cmdline(arg)
>> -                pp = pprint.PrettyPrinter(indent=4)
>> +                pretty = True
>
> We now accept any number of -p arguments. That's probably fine.

I'd call that a bug fix.

>>              elif arg == "-v":
>>                  verbose = True
>>              else:
>> @@ -398,7 +395,7 @@ def main():
>>                  if hmp:
>>                      qemu = HMPShell(arg)
>>                  else:
>> -                    qemu = QMPShell(arg, pp)
>> +                    qemu = QMPShell(arg, pretty)
>>                  addr = arg
>>  
>>          if qemu is None:
>> 
>
> Reviewed-by: John Snow <jsnow@redhat.com>

Applied to my qapi-next branch, thanks!