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

[John Snow <jsnow@redhat.com>]

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.

>              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>