Re: [Qemu-devel] [RFC PATCH v2] qmp.py: Fix exception parsing partial JSON

[Eduardo Habkost]

On Wed, Jun 06, 2018 at 04:27:31PM -0300, Philippe Mathieu-Daudé wrote:
> The readline() call returns partial data.

How can this be reproduced?  Despite not being forbidden by the
QMP specification, QEMU normally doesn't break QMP replies in
multiple lines, and readline() is not supposed to return a
partial line unless it encounters EOF.


> Keep appending until the JSON buffer is complete.
> 
> This fixes:
> 
>     $ scripts/qmp/qmp-shell -v -p /tmp/qmp.sock
>     Traceback (most recent call last):
>       File "scripts/qmp/qmp-shell", line 456, in <module>
>         main()
>       File "scripts/qmp/qmp-shell", line 441, in main
>         qemu.connect(negotiate)
>       File "scripts/qmp/qmp-shell", line 284, in connect
>         self._greeting = super(QMPShell, self).connect(negotiate)
>       File "scripts/qmp/qmp.py", line 143, in connect
>         return self.__negotiate_capabilities()
>       File "scripts/qmp/qmp.py", line 71, in __negotiate_capabilities
>         greeting = self.__json_read()
>       File "scripts/qmp/qmp.py", line 85, in __json_read
>         resp = json.loads(data)
>       File "/usr/lib/python2.7/json/__init__.py", line 339, in loads
>         return _default_decoder.decode(s)
>       File "/usr/lib/python2.7/json/decoder.py", line 364, in decode
>         obj, end = self.raw_decode(s, idx=_w(s, 0).end())
>       File "/usr/lib/python2.7/json/decoder.py", line 380, in raw_decode
>         obj, end = self.scan_once(s, idx)
>     ValueError: Expecting object: line 1 column 3 (char 2)
> 
> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> ---
> Since v1:
> - addressed Daniel review: clean data after json.loads() succeeds
> - add a XXX comment
> 
> Daniel suggested this is due to blocking i/o.
> 
> I'm sure there is a nicer/more pythonic way to do this, but this works for me,
> sorry :)

It looks like there's no elegant solution for this:
https://stackoverflow.com/a/21709058

> 
>  scripts/qmp/qmp.py | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
> index 5c8cf6a056..14f0b48936 100644
> --- a/scripts/qmp/qmp.py
> +++ b/scripts/qmp/qmp.py
> @@ -78,11 +78,18 @@ class QEMUMonitorProtocol(object):
>          raise QMPCapabilitiesError
>  
>      def __json_read(self, only_event=False):
> +        data = ""
>          while True:
> -            data = self.__sockfile.readline()
> -            if not data:
> +            tmp = self.__sockfile.readline()
> +            if not tmp:
>                  return
> -            resp = json.loads(data)
> +            data += tmp
> +            try:
> +                resp = json.loads(data)
> +            except ValueError:
> +                # XXX: blindly loop, even if QEMU ever sends malformed data
> +                continue

I was going to suggest using json.JSONDecoder.raw_decode() and
saving the remaining data in case we already read partial data
for a second JSON message.

But the QMP specification says all messages are terminated with
CRLF, so we we should never see the data for two different
messages in a single readline() call.  Noting this in a comment
wouldn't hurt, though.

The patch seems reasonable, but first I would like to understand
how this bug can be triggered.

> +            data = ""
>              if 'event' in resp:
>                  self.logger.debug("<<< %s", resp)
>                  self.__events.append(resp)
> -- 
> 2.17.1
> 

-- 
Eduardo