Re: [Qemu-devel] [PATCH] Python3 Support for qmp.py

[Stefan Hajnoczi]

On Sat, Jul 01, 2017 at 12:39:41AM +0530, Ishani Chugh wrote:
> This patch intends to make qmp.py compatible with both python2 and python3.

Please identify the Python 2/3 compatibility issues addressed in the
patch like:

 * Python 3 does not have dict.has_key(key), use key in dict instead
 * Avoid line-based I/O since Python 2/3 have different character
   encoding behavior.  Explicitly encode/decode JSON UTF-8.

This explains why code changes are being made.

> 
> Signed-off-by: Ishani Chugh <address@hidden>
> ---
>  scripts/qmp/qmp.py | 66 
> +++++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 43 insertions(+), 23 deletions(-)
> 
> diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
> index 62d3651..9926c36 100644
> --- a/scripts/qmp/qmp.py
> +++ b/scripts/qmp/qmp.py
> @@ -13,18 +13,23 @@ import errno
>  import socket
>  import sys
>  
> +
>  class QMPError(Exception):
>      pass

Whitespace changes are generally discouraged because they perturb the
code (creating merge conflicts, adding noise to diffs, and obscuring
line change history).

I think you're making them for a good reason here - probably to conform
to Python PEP8 coding style?  But I'm not sure because there is
explanation in the commit description.

If you want to reformat this file to meet the PEP8 standard, please do
it as a separate patch.

> @@ -42,6 +47,7 @@ class QEMUMonitorProtocol:
>          self.__address = address
>          self._debug = debug
>          self.__sock = self.__get_sock()
> +        self.data = b""

Please follow the variable naming convention: two underscores for
private member fields (i.e. self.__data).

>          if server:
>              self.__sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
>              self.__sock.bind(self.__address)
> @@ -56,23 +62,35 @@ class QEMUMonitorProtocol:
>  
>      def __negotiate_capabilities(self):
>          greeting = self.__json_read()
> -        if greeting is None or not greeting.has_key('QMP'):
> +        if greeting is None or 'QMP' not in greeting:
>              raise QMPConnectError
> -        # Greeting seems ok, negotiate capabilities

Why remove this comment?

> @@ -87,18 +105,18 @@ class QEMUMonitorProtocol:
>          @param wait (bool): block until an event is available.
>          @param wait (float): If wait is a float, treat it as a timeout value.
>  
> -        @raise QMPTimeoutError: If a timeout float is provided and the 
> timeout
> -                                period elapses.
> -        @raise QMPConnectError: If wait is True but no events could be 
> retrieved
> -                                or if some other error occurred.
> +        @raise QMPTimeoutError: If a timeout float is provided and the
> +                                timeout period elapses.
> +        @raise QMPConnectError: If wait is True but no events could be
> +                                retrieved or if some other error occurred.
>          """
>  
>          # Check for new events regardless and pull them into the cache:
>          self.__sock.setblocking(0)
>          try:
> -            self.__json_read()
> +            test = self.__json_read()

Is 'test' a debug variable that should be removed from the final patch?

>          except socket.error as err:
> -            if err[0] == errno.EAGAIN:
> +            if err.errno == errno.EAGAIN:
>                  # No data available
>                  pass

Pre-existing bug: if the exceptin is not EAGAIN we need to raise the
exception again.

>          self.__sock.setblocking(1)
> @@ -128,7 +146,7 @@ class QEMUMonitorProtocol:
>          @raise QMPCapabilitiesError if fails to negotiate capabilities
>          """
>          self.__sock.connect(self.__address)
> -        self.__sockfile = self.__sock.makefile()
> +        self.__sockfile = self.__sock.makefile('rb')

self.__sockfile is no longer used, please delete it.

>          if negotiate:
>              return self.__negotiate_capabilities()
>  
> @@ -143,7 +161,7 @@ class QEMUMonitorProtocol:
>          """
>          self.__sock.settimeout(15)
>          self.__sock, _ = self.__sock.accept()
> -        self.__sockfile = self.__sock.makefile()
> +        self.__sockfile = self.__sock.makefile('rb')

Same here.

> @@ -245,3 +264,4 @@ class QEMUMonitorProtocol:
>  
>      def is_scm_available(self):
>          return self.__sock.family == socket.AF_UNIX
> +

signature.asc
Description: PGP signature