Re: [Qemu-devel] [PATCH v1] Provided python3 support for qmp.py

[Stefan Hajnoczi]

On Mon, Mar 06, 2017 at 02:43:50PM +0300, Joannah Nanjekye wrote:
> From: Joannah Najekye <address@hidden>
> 
> Signed-off-by: Joannah Nanjekye <address@hidden>
> ---

Text below '---' is not included as part of the commit description.  I
think the following lines should be included, they describe the purpose
of the patch.  Please the text above '---'.

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

PyPI packages can be used if necessary but dependencies should be
minimized.  Comments on the new dependencies below:

>  scripts/qmp/qmp.py | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
> index 62d3651..6dcb870 100644
> --- a/scripts/qmp/qmp.py
> +++ b/scripts/qmp/qmp.py
> @@ -7,7 +7,9 @@
>  #
>  # 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
> +from future.utils import raise_from

Why is raise_from() needed?  My understanding is that it provides
exception chaining, which this code doesn't use.  Therefore plain
'raise' is enough.  The dependency can be dropped.

> +from builtins import str 
>  import json
>  import errno
>  import socket
> @@ -57,12 +59,12 @@ class QEMUMonitorProtocol:
>      def __negotiate_capabilities(self):
>          greeting = self.__json_read()
>          if greeting is None or not greeting.has_key('QMP'):
> -            raise QMPConnectError
> +            raise_from(QMPConnectError())
>          # Greeting seems ok, negotiate capabilities
>          resp = self.cmd('qmp_capabilities')
>          if "return" in resp:
>              return greeting
> -        raise QMPCapabilitiesError
> +        raise_from(QMPCapabilitiesError())
>  
>      def __json_read(self, only_event=False):
>          while True:
> @@ -72,7 +74,7 @@ class QEMUMonitorProtocol:
>              resp = json.loads(data)
>              if 'event' in resp:
>                  if self._debug:
> -                    print >>sys.stderr, "QMP:<<< %s" % resp
> +                    print(u"QMP:<<< %s" % str(resp), file=sys.stderr) 

Why is str() necessary?  resp is a dict and the %s format specifier
already produces a string representation.

The same applies to the str(qmp_cmd) and str(resp) changes below.

>                  self.__events.append(resp)
>                  if not only_event:
>                      continue
> @@ -111,11 +113,11 @@ class QEMUMonitorProtocol:
>              try:
>                  ret = self.__json_read(only_event=True)
>              except socket.timeout:
> -                raise QMPTimeoutError("Timeout waiting for event")
> +                raise_from(QMPTimeoutError("Timeout waiting for event"))
>              except:
> -                raise QMPConnectError("Error while reading from socket")
> +                raise_from(QMPConnectError("Error while reading from 
> socket"))
>              if ret is None:
> -                raise QMPConnectError("Error while reading from socket")
> +                raise_from(QMPConnectError("Error while reading from 
> socket"))
>              self.__sock.settimeout(None)
>  
>      def connect(self, negotiate=True):
> @@ -155,16 +157,16 @@ class QEMUMonitorProtocol:
>                  been closed
>          """
>          if self._debug:
> -            print >>sys.stderr, "QMP:>>> %s" % qmp_cmd
> +            print(u"QMP:>>> %s" % str(qmp_cmd), file=sys.stderr) 
>          try:
>              self.__sock.sendall(json.dumps(qmp_cmd))
>          except socket.error as err:
>              if err[0] == errno.EPIPE:
>                  return
> -            raise socket.error(err)
> +            raise_from(socket.error(), err)
>          resp = self.__json_read()
>          if self._debug:
> -            print >>sys.stderr, "QMP:<<< %s" % resp
> +            print(u"QMP:<<< %s" % str(resp), file=sys.stderr)  
>          return resp
>  
>      def cmd(self, name, args=None, id=None):
> -- 
> 2.7.4
>

signature.asc
Description: PGP signature