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

[Stefan Hajnoczi <stefanha@redhat.com>]

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

On Fri, Jul 07, 2017 at 12:38:47AM +0530, Ishani Chugh wrote:

Commit messages usually begin with a prefix indicating the affected
component.  This makes it easier for code reviewers and maintainers to
browse email subjects and the git log.  I recommend the following commit
message:

  scripts/qmp: Python3 Support for qmp.py

>      def __json_read(self, only_event=False):
>          while True:
> -            data = self.__sockfile.readline()
> +            data = self.__sock_readline()
>              if not data:
>                  return
>              resp = json.loads(data)
>              if 'event' in resp:
>                  if self._debug:
> -                    print >>sys.stderr, "QMP:<<< %s" % resp
> +                    print("QMP:<<< %s" % resp)

Previously debug output was printed to stderr.  Please do not change
this.

Here is how to keep stderr in Python 2.6+/3 compatible fashion:

from __future__ import print_function
...
print("QMP:<<< %s" % resp, file=sys.stderr)

There are more instances of this below.

> @@ -98,9 +112,11 @@ class QEMUMonitorProtocol:
>          try:
>              self.__json_read()
>          except socket.error as err:
> -            if err[0] == errno.EAGAIN:
> +            if err.errno == errno.EAGAIN:
>                  # No data available
>                  pass
> +            else:
> +                raise
>          self.__sock.setblocking(1)
>  
>          # Wait for new events, if needed.

The "else: raise" silently fixes a bug.  Please mention that socket
errors were silently ignored in the commit description.  That will make
the intention of this change clear for anyone who looks at this commit
in the future.

> @@ -175,7 +190,7 @@ class QEMUMonitorProtocol:
>          @param args: command arguments (dict)
>          @param id: command id (dict, list, string or int)
>          """
> -        qmp_cmd = { 'execute': name }
> +        qmp_cmd = {'execute': name}
>          if args:
>              qmp_cmd['arguments'] = args
>          if id:

Please make PEP8 fixes in a separate commit and only Python 2/3
compatibility changes in this commit.  Commits that do only one thing
are easier to understand, backport, and bisect.

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

What is the purpose of this extra newline?

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