Re: [Qemu-devel] [PATCH v3] scripts/qmp: python3 support for qmp.py

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4362 bytes --]

On Sat, Mar 25, 2017 at 04:56:41PM +0300, Joannah Nanjekye wrote:
> From: nanjekyejoannah <nanjekyejoannah@gmail.com>
> 
> The patch provides python 3 support for one of the scripts  in scripts/qmp that is to say qmp.py. This is not a port to python 3 but rather the patch ensures that the script runs fine for both python 2 and 3.
> 
> Minimum Python Versions supported:
> 
>   Python 2 : python 2.6 +
>   Python 3 : python 3.3 +
> 
> The two new imports future and builtins introduced refer to the future
> pip-installable package on PyPI.
> 
> Signed-off-by: nanjekyejoannah <nanjekyejoannah@gmail.com>
> ---
>  scripts/qmp/qmp.py | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
> index 62d3651..57ac86b 100644
> --- a/scripts/qmp/qmp.py
> +++ b/scripts/qmp/qmp.py
> @@ -7,7 +7,7 @@
>  #
>  # This work is licensed under the terms of the GNU GPL, version 2.  See
>  # the COPYING file in the top-level directory.
> -
> +from __future__ import absolute_import, print_function, unicode_literals
>  import json
>  import errno
>  import socket
> @@ -56,13 +56,13 @@ class QEMUMonitorProtocol:
>  
>      def __negotiate_capabilities(self):
>          greeting = self.__json_read()
> -        if greeting is None or not greeting.has_key('QMP'):
> -            raise QMPConnectError
> +        if greeting is None or not 'QMP' in list(greeting):

list() is not necessary.  The "key in dict" expression already operates
on just the dict's keys.

> +            raise QMPConnectError()
>          # Greeting seems ok, negotiate capabilities
>          resp = self.cmd('qmp_capabilities')
>          if "return" in resp:
>              return greeting
> -        raise QMPCapabilitiesError
> +        raise QMPCapabilitiesError()
>  
>      def __json_read(self, only_event=False):
>          while True:
> @@ -72,7 +72,7 @@ class QEMUMonitorProtocol:
>              resp = json.loads(data)
>              if 'event' in resp:
>                  if self._debug:
> -                    print >>sys.stderr, "QMP:<<< %s" % resp
> +                    print(u"QMP:<<< %s" % resp, file=sys.stderr) 

unicode_literals already makes all string literals unicode.  There is no
need to explicitly add u"".

>                  self.__events.append(resp)
>                  if not only_event:
>                      continue
> @@ -113,7 +113,7 @@ class QEMUMonitorProtocol:
>              except socket.timeout:
>                  raise QMPTimeoutError("Timeout waiting for event")
>              except:
> -                raise QMPConnectError("Error while reading from socket")
> +                raise(QMPConnectError("Error while reading from socket"))

Is there a reason for this change?

>              if ret is None:
>                  raise QMPConnectError("Error while reading from socket")
>              self.__sock.settimeout(None)
> @@ -155,16 +155,16 @@ class QEMUMonitorProtocol:
>                  been closed
>          """
>          if self._debug:
> -            print >>sys.stderr, "QMP:>>> %s" % qmp_cmd
> +            print(u"QMP:>>> %s" % qmp_cmd, file=sys.stderr) 
>          try:
> -            self.__sock.sendall(json.dumps(qmp_cmd))
> +            self.__sock.sendall((json.dumps(qmp_cmd)).encode('latin-1'))

The socket's encoding should be 'utf-8'.

You didn't specify an encoding in __json_read() where
self.__sockfile.readline() is called.  It will use the default encoding
and could therefore hit decoding errors.  We need to specify 'utf-8' for
reading too.

I think that is a little tricky to do in a Python 2/3 compatible way
because Python 2.6 socket and makefile do not take an encoding argument.
The simplest solution might be to read 1 byte at a time in __json_read()
until '\n' is encountered instead of calling readline().  That way you
can call line.decode('utf-8') on the raw bytes.

>          except socket.error as err:
>              if err[0] == errno.EPIPE:
>                  return
> -            raise socket.error(err)
> +            raise (socket.error(), err)

We're re-raising the exception.  There is shorter syntax for this:

  except socket.error as err:
      if err[0] == errno.EPIPE:
          return
      raise

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]