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

[Ishani]

- On Jul 7, 2017, at 4:31 PM, Daniel P. Berrange berra...@redhat.com wrote:

> On Fri, Jul 07, 2017 at 12:38:47AM +0530, Ishani Chugh wrote:
>> This patch intends to make qmp.py compatible with both python2 and python3.
>> 
>>  * 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.
>>  * Replace print by print function.
> 
> If you're going todo this, then you need to actually import the python3
> compatible print function - not just call the python2 print statement
> with brackets, as the semantics are different:
> 
>  $ python2
>  >>> print("foo", "bar")
>  ('foo', 'bar')
>  >>> from __future__ import print_function
>  >>> print("foo", "bar")
>  foo bar
> 
> Only the latter matches python3 semantics

Thanks. Will fix in v3.

>> 
>> Signed-off-by: Ishani Chugh 
>> ---
>>  scripts/qmp/qmp.py | 58 
>> ++
>>  1 file changed, 37 insertions(+), 21 deletions(-)
>> 
>> diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
>> index 62d3651..58fb7d1 100644
>> --- a/scripts/qmp/qmp.py
>> +++ b/scripts/qmp/qmp.py
>> @@ -42,6 +42,7 @@ class QEMUMonitorProtocol:
>>  self.__address = address
>>  self._debug = debug
>>  self.__sock = self.__get_sock()
>> +self.__data = b""
>>  if server:
>>  self.__sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 
>> 1)
>>  self.__sock.bind(self.__address)
>> @@ -56,7 +57,7 @@ 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
>>  resp = self.cmd('qmp_capabilities')
>> @@ -64,15 +65,28 @@ class QEMUMonitorProtocol:
>>  return greeting
>>  raise QMPCapabilitiesError
>>  
>> +def __sock_readline(self):
>> +while True:
>> +ch = self.__sock.recv(1)
>> +if ch is None:
>> +if self.__data:
>> +raise ValueError('socket closed mid-line')
>> +return None
>> +self.__data += ch
>> +if ch == b'\n':
>> +line = self.__data.decode('utf-8')
>> +self.__data = b""
>> +return line
>> +
>>  def __json_read(self, only_event=False):
>>  while True:
>> -data = self.__sockfile.readline()
>> +data = self.__sock_readline()
> 
> So originally we could get back a "str" on python2, but now
> we get a "str" (which is unicode) on python2, but "unicode"
> on python2.
> 
>>  if not data:
>>  return
>>  resp = json.loads(data)
> 
> This means the 'resp' now contains "unicode" data too on
> python2 instead of 'str'.
> 
> This hopefully isn't a problem, but we certainly need to
> make sure the iotests still pass on py2, as it could well
> have a ripple effect in this respect.  Have you run the
> iotests with this change applied ?

No. I haven't run iotests with this change. Thanks for good 
suggestion. I will run iotests before v3.

> 
>>  if 'event' in resp:
>>  if self._debug:
>> -print >>sys.stderr, "QMP:<<< %s" % resp
>> +print("QMP:<<< %s" % resp)
> 
> This is changing from printing to stderr, to printing to stdout
> which is not desirable. Likewise all the similar changes below.
> 
>>  self.__events.append(resp)
>>  if not only_event:
>>  continue
>> @@ -87,10 +101,10 @@ 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.
>>  """
> 
> Don't mix whitespace changes in with other functional changes.
> 
> 
>>  
>>  # Check for new events regardless and pull them into the cache:
>> @@ -98,9 +112,11 @@ class QEMUMonitorProtocol:
>>  try:
>>  self.__json_read()
>>  except socket.error as err:
>> -if err[0] == 

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

[Markus Armbruster]

Ishani Chugh  writes:

> This patch intends to make qmp.py compatible with both python2 and python3.
>
>  * 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.
>  * Replace print by print function.
>
> Signed-off-by: Ishani Chugh 

$ scripts/get_maintainer.pl -f $your_patch
Markus Armbruster  (supporter:QMP)
qemu-devel@nongnu.org (open list:All patches CC here)

Just sayin' :)

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

[Daniel P. Berrange]

On Fri, Jul 07, 2017 at 12:38:47AM +0530, Ishani Chugh wrote:
> This patch intends to make qmp.py compatible with both python2 and python3.
> 
>  * 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.
>  * Replace print by print function.

If you're going todo this, then you need to actually import the python3
compatible print function - not just call the python2 print statement
with brackets, as the semantics are different:

  $ python2
  >>> print("foo", "bar")
  ('foo', 'bar')
  >>> from __future__ import print_function
  >>> print("foo", "bar")
  foo bar

Only the latter matches python3 semantics

> 
> Signed-off-by: Ishani Chugh 
> ---
>  scripts/qmp/qmp.py | 58 
> ++
>  1 file changed, 37 insertions(+), 21 deletions(-)
> 
> diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
> index 62d3651..58fb7d1 100644
> --- a/scripts/qmp/qmp.py
> +++ b/scripts/qmp/qmp.py
> @@ -42,6 +42,7 @@ class QEMUMonitorProtocol:
>  self.__address = address
>  self._debug = debug
>  self.__sock = self.__get_sock()
> +self.__data = b""
>  if server:
>  self.__sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
>  self.__sock.bind(self.__address)
> @@ -56,7 +57,7 @@ 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
>  resp = self.cmd('qmp_capabilities')
> @@ -64,15 +65,28 @@ class QEMUMonitorProtocol:
>  return greeting
>  raise QMPCapabilitiesError
>  
> +def __sock_readline(self):
> +while True:
> +ch = self.__sock.recv(1)
> +if ch is None:
> +if self.__data:
> +raise ValueError('socket closed mid-line')
> +return None
> +self.__data += ch
> +if ch == b'\n':
> +line = self.__data.decode('utf-8')
> +self.__data = b""
> +return line
> +
>  def __json_read(self, only_event=False):
>  while True:
> -data = self.__sockfile.readline()
> +data = self.__sock_readline()

So originally we could get back a "str" on python2, but now
we get a "str" (which is unicode) on python2, but "unicode"
on python2.

>  if not data:
>  return
>  resp = json.loads(data)

This means the 'resp' now contains "unicode" data too on
python2 instead of 'str'.

This hopefully isn't a problem, but we certainly need to
make sure the iotests still pass on py2, as it could well
have a ripple effect in this respect.  Have you run the
iotests with this change applied ?

>  if 'event' in resp:
>  if self._debug:
> -print >>sys.stderr, "QMP:<<< %s" % resp
> +print("QMP:<<< %s" % resp)

This is changing from printing to stderr, to printing to stdout
which is not desirable. Likewise all the similar changes below.

>  self.__events.append(resp)
>  if not only_event:
>  continue
> @@ -87,10 +101,10 @@ 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.
>  """

Don't mix whitespace changes in with other functional changes.


>  
>  # Check for new events regardless and pull them into the cache:
> @@ -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.
> @@ -128,7 +144,6 @@ class QEMUMonitorProtocol:
>  @raise QMPCapabilitiesError if fails to negotiate 

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

[Stefan Hajnoczi]

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?


signature.asc
Description: PGP signature

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

[Ishani Chugh]

This patch intends to make qmp.py compatible with both python2 and python3.

 * 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.
 * Replace print by print function.

Signed-off-by: Ishani Chugh 
---
 scripts/qmp/qmp.py | 58 ++
 1 file changed, 37 insertions(+), 21 deletions(-)

diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
index 62d3651..58fb7d1 100644
--- a/scripts/qmp/qmp.py
+++ b/scripts/qmp/qmp.py
@@ -42,6 +42,7 @@ class QEMUMonitorProtocol:
 self.__address = address
 self._debug = debug
 self.__sock = self.__get_sock()
+self.__data = b""
 if server:
 self.__sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
 self.__sock.bind(self.__address)
@@ -56,7 +57,7 @@ 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
 resp = self.cmd('qmp_capabilities')
@@ -64,15 +65,28 @@ class QEMUMonitorProtocol:
 return greeting
 raise QMPCapabilitiesError
 
+def __sock_readline(self):
+while True:
+ch = self.__sock.recv(1)
+if ch is None:
+if self.__data:
+raise ValueError('socket closed mid-line')
+return None
+self.__data += ch
+if ch == b'\n':
+line = self.__data.decode('utf-8')
+self.__data = b""
+return line
+
 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)
 self.__events.append(resp)
 if not only_event:
 continue
@@ -87,10 +101,10 @@ 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:
@@ -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.
@@ -128,7 +144,6 @@ class QEMUMonitorProtocol:
 @raise QMPCapabilitiesError if fails to negotiate capabilities
 """
 self.__sock.connect(self.__address)
-self.__sockfile = self.__sock.makefile()
 if negotiate:
 return self.__negotiate_capabilities()
 
@@ -143,7 +158,6 @@ class QEMUMonitorProtocol:
 """
 self.__sock.settimeout(15)
 self.__sock, _ = self.__sock.accept()
-self.__sockfile = self.__sock.makefile()
 return self.__negotiate_capabilities()
 
 def cmd_obj(self, qmp_cmd):
@@ -155,16 +169,17 @@ class QEMUMonitorProtocol:
 been closed
 """
 if self._debug:
-print >>sys.stderr, "QMP:>>> %s" % qmp_cmd
+print("QMP:>>> %s" % qmp_cmd)
 try:
-self.__sock.sendall(json.dumps(qmp_cmd))
+command = json.dumps(qmp_cmd)
+self.__sock.sendall(command.encode('UTF-8'))
 except socket.error as err:
-if err[0] == errno.EPIPE:
+if err.errno == errno.EPIPE:
 return
-raise socket.error(err)
+raise
 resp = self.__json_read()
 if self._debug:
-print >>sys.stderr, "QMP:<<< %s" % resp
+print("QMP:<<< %s" % resp)
 return resp
 
 def cmd(self, name,