Re: [Qemu-devel] [PATCH 06/11] qmp.py: Couple of pylint/style fixes

2017-07-25 Thread Philippe Mathieu-Daudé 
On Tue, Jul 25, 2017 at 3:13 AM, Lukáš Doktor  wrote:
> Dne 25.7.2017 v 08:04 Philippe Mathieu-Daudé napsal(a):
>> Hi Lukáš,
>>
>> On 07/24/2017 09:36 AM, Lukáš Doktor wrote:
>>> Dne 22.7.2017 v 03:30 Philippe Mathieu-Daudé napsal(a):
 Hi Lukáš,

 Since comment/indent fixes and code changes are not related I'd rather see 
 this split in at least 2 patches.

>>> Hello Philippe, thank you for the review, I'm wondering what code changes 
>>> you have in mind? This is commit should not bring any code changes, just 
>>> code reorganization (like including the self.* attributes on top of the 
>>> file)
>>>
 On 07/20/2017 01:28 PM, Lukáš Doktor wrote:
> No actual code changes, just a few pylint/style fixes and docstring
> clarifications.
>
> Signed-off-by: Lukáš Doktor 
> ---
>scripts/qmp/qmp.py | 37 -
>1 file changed, 24 insertions(+), 13 deletions(-)
>
> diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
>> [...]
>def __init__(self, address, server=False, debug=False):
>"""
>Create a QEMUMonitorProtocol class.
> @@ -42,6 +53,7 @@ class QEMUMonitorProtocol:
>self.__address = address
>self._debug = debug
>self.__sock = self.__get_sock()
> +self.__sockfile = None
>>
>> I was thinking about this line which is new. Now the declaration and 
>> initialization are done in __init__() while before it was only 
>> declared/initialized in connect() or accept().
>>
>> I'm not expert of python interpreter/jit internals but expect the generated 
>> code to be slightly different, even if achieving the same.
>>
>> not a bit deal, just about wording ;)
>>
> Well the difference is that before `connect` you get `AttributeError` when 
> looking for `self.__sockfile` while with this patch you'll get `None`. In 
> this code nobody queries for `self.__sockfile` before the `connect` so it 
> should not change the behavior of this code so I consider that as a `style` 
> fix as it's ugly to extend attributes later in code (with some exceptions 
> like Namespace-objects..). Anyway if you insist I can split those patches.

I'm not insisting ;) Just add something like "also initialize
__sockfile to avoid AttributeError while introspecting object before
any call to connect/accept" in the commit message and that's fine to
me.

Re: [Qemu-devel] [PATCH 06/11] qmp.py: Couple of pylint/style fixes

2017-07-25 Thread Lukáš Doktor 
Dne 25.7.2017 v 08:04 Philippe Mathieu-Daudé napsal(a):
> Hi Lukáš,
> 
> On 07/24/2017 09:36 AM, Lukáš Doktor wrote:
>> Dne 22.7.2017 v 03:30 Philippe Mathieu-Daudé napsal(a):
>>> Hi Lukáš,
>>>
>>> Since comment/indent fixes and code changes are not related I'd rather see 
>>> this split in at least 2 patches.
>>>
>> Hello Philippe, thank you for the review, I'm wondering what code changes 
>> you have in mind? This is commit should not bring any code changes, just 
>> code reorganization (like including the self.* attributes on top of the file)
>>
>>> On 07/20/2017 01:28 PM, Lukáš Doktor wrote:
 No actual code changes, just a few pylint/style fixes and docstring
 clarifications.

 Signed-off-by: Lukáš Doktor 
 ---
scripts/qmp/qmp.py | 37 -
1 file changed, 24 insertions(+), 13 deletions(-)

 diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
> [...]
def __init__(self, address, server=False, debug=False):
"""
Create a QEMUMonitorProtocol class.
 @@ -42,6 +53,7 @@ class QEMUMonitorProtocol:
self.__address = address
self._debug = debug
self.__sock = self.__get_sock()
 +self.__sockfile = None
> 
> I was thinking about this line which is new. Now the declaration and 
> initialization are done in __init__() while before it was only 
> declared/initialized in connect() or accept().
> 
> I'm not expert of python interpreter/jit internals but expect the generated 
> code to be slightly different, even if achieving the same.
> 
> not a bit deal, just about wording ;)
> 
Well the difference is that before `connect` you get `AttributeError` when 
looking for `self.__sockfile` while with this patch you'll get `None`. In this 
code nobody queries for `self.__sockfile` before the `connect` so it should not 
change the behavior of this code so I consider that as a `style` fix as it's 
ugly to extend attributes later in code (with some exceptions like 
Namespace-objects..). Anyway if you insist I can split those patches.

Lukáš

if server:
self.__sock.setsockopt(socket.SOL_SOCKET, 
 socket.SO_REUSEADDR, 1)
self.__sock.bind(self.__address)




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 06/11] qmp.py: Couple of pylint/style fixes

2017-07-25 Thread Philippe Mathieu-Daudé 

Hi Lukáš,

On 07/24/2017 09:36 AM, Lukáš Doktor wrote:

Dne 22.7.2017 v 03:30 Philippe Mathieu-Daudé napsal(a):

Hi Lukáš,

Since comment/indent fixes and code changes are not related I'd rather see this 
split in at least 2 patches.


Hello Philippe, thank you for the review, I'm wondering what code changes you 
have in mind? This is commit should not bring any code changes, just code 
reorganization (like including the self.* attributes on top of the file)


On 07/20/2017 01:28 PM, Lukáš Doktor wrote:

No actual code changes, just a few pylint/style fixes and docstring
clarifications.

Signed-off-by: Lukáš Doktor 
---
   scripts/qmp/qmp.py | 37 -
   1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py

[...]

   def __init__(self, address, server=False, debug=False):
   """
   Create a QEMUMonitorProtocol class.
@@ -42,6 +53,7 @@ class QEMUMonitorProtocol:
   self.__address = address
   self._debug = debug
   self.__sock = self.__get_sock()
+self.__sockfile = None


I was thinking about this line which is new. Now the declaration and 
initialization are done in __init__() while before it was only 
declared/initialized in connect() or accept().


I'm not expert of python interpreter/jit internals but expect the 
generated code to be slightly different, even if achieving the same.


not a bit deal, just about wording ;)


   if server:
   self.__sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
   self.__sock.bind(self.__address)

Re: [Qemu-devel] [PATCH 06/11] qmp.py: Couple of pylint/style fixes

2017-07-24 Thread Lukáš Doktor 
Dne 22.7.2017 v 03:30 Philippe Mathieu-Daudé napsal(a):
> Hi Lukáš,
> 
> Since comment/indent fixes and code changes are not related I'd rather see 
> this split in at least 2 patches.
> 
Hello Philippe, thank you for the review, I'm wondering what code changes you 
have in mind? This is commit should not bring any code changes, just code 
reorganization (like including the self.* attributes on top of the file)

> On 07/20/2017 01:28 PM, Lukáš Doktor wrote:
>> No actual code changes, just a few pylint/style fixes and docstring
>> clarifications.
>>
>> Signed-off-by: Lukáš Doktor 
>> ---
>>   scripts/qmp/qmp.py | 37 -
>>   1 file changed, 24 insertions(+), 13 deletions(-)
>>
>> diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
>> index 62d3651..bb4d614 100644
>> --- a/scripts/qmp/qmp.py
>> +++ b/scripts/qmp/qmp.py
>> @@ -13,19 +13,30 @@ import errno
>>   import socket
>>   import sys
>>   +
>>   class QMPError(Exception):
>>   pass
>>   +
>>   class QMPConnectError(QMPError):
>>   pass
>>   +
>>   class QMPCapabilitiesError(QMPError):
>>   pass
>>   +
>>   class QMPTimeoutError(QMPError):
>>   pass
>>   +
>>   class QEMUMonitorProtocol:
>> +
>> +#: Socket's error class
>> +error = socket.error
>> +#: Socket's timeout
>> +timeout = socket.timeout
>> +
> 
> ok
> 
>>   def __init__(self, address, server=False, debug=False):
>>   """
>>   Create a QEMUMonitorProtocol class.
>> @@ -42,6 +53,7 @@ class QEMUMonitorProtocol:
>>   self.__address = address
>>   self._debug = debug
>>   self.__sock = self.__get_sock()
>> +self.__sockfile = None
>>   if server:
>>   self.__sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 
>> 1)
>>   self.__sock.bind(self.__address)
>> @@ -56,7 +68,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:
> 
> ok
> 
>>   raise QMPConnectError
>>   # Greeting seems ok, negotiate capabilities
>>   resp = self.cmd('qmp_capabilities')
>> @@ -78,8 +90,6 @@ class QEMUMonitorProtocol:
>>   continue
>>   return resp
>>   -error = socket.error
>> -
>>   def __get_events(self, wait=False):
>>   """
>>   Check for new events in the stream and cache them in __events.
>> @@ -89,8 +99,8 @@ class QEMUMonitorProtocol:
>> @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 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:
>> @@ -175,7 +185,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:
>> @@ -183,6 +193,9 @@ class QEMUMonitorProtocol:
>>   return self.cmd_obj(qmp_cmd)
>> def command(self, cmd, **kwds):
>> +"""
>> +Build and send a QMP command to the monitor, report errors when any
> 
> I'm not native english speaker but I think "if any" sounds better.
> 
Yep, you are right.

>> +"""
>>   ret = self.cmd(cmd, kwds)
>>   if ret.has_key('error'):
>>   raise Exception(ret['error']['desc'])
>> @@ -190,15 +203,15 @@ class QEMUMonitorProtocol:
>> def pull_event(self, wait=False):
>>   """
>> -Get and delete the first available QMP event.
>> +Get and pop the first available QMP event.
> 
> Both sentences seems unclear to me, regarding the function name... I wonder 
> if removing this comment makes this function clearer.
> 
I was trying to avoid confusion with the delete. How about `Pulls/waits for a 
single event`? I'd like to keep the rest of the docstring as the details might 
be useful.

Lukáš

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

Re: [Qemu-devel] [PATCH 06/11] qmp.py: Couple of pylint/style fixes

2017-07-21 Thread Philippe Mathieu-Daudé 

Hi Lukáš,

Since comment/indent fixes and code changes are not related I'd rather 
see this split in at least 2 patches.


On 07/20/2017 01:28 PM, Lukáš Doktor wrote:

No actual code changes, just a few pylint/style fixes and docstring
clarifications.

Signed-off-by: Lukáš Doktor 
---
  scripts/qmp/qmp.py | 37 -
  1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
index 62d3651..bb4d614 100644
--- a/scripts/qmp/qmp.py
+++ b/scripts/qmp/qmp.py
@@ -13,19 +13,30 @@ import errno
  import socket
  import sys
  
+

  class QMPError(Exception):
  pass
  
+

  class QMPConnectError(QMPError):
  pass
  
+

  class QMPCapabilitiesError(QMPError):
  pass
  
+

  class QMPTimeoutError(QMPError):
  pass
  
+

  class QEMUMonitorProtocol:
+
+#: Socket's error class
+error = socket.error
+#: Socket's timeout
+timeout = socket.timeout
+


ok


  def __init__(self, address, server=False, debug=False):
  """
  Create a QEMUMonitorProtocol class.
@@ -42,6 +53,7 @@ class QEMUMonitorProtocol:
  self.__address = address
  self._debug = debug
  self.__sock = self.__get_sock()
+self.__sockfile = None
  if server:
  self.__sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
  self.__sock.bind(self.__address)
@@ -56,7 +68,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:


ok


  raise QMPConnectError
  # Greeting seems ok, negotiate capabilities
  resp = self.cmd('qmp_capabilities')
@@ -78,8 +90,6 @@ class QEMUMonitorProtocol:
  continue
  return resp
  
-error = socket.error

-
  def __get_events(self, wait=False):
  """
  Check for new events in the stream and cache them in __events.
@@ -89,8 +99,8 @@ class QEMUMonitorProtocol:
  
  @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 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:

@@ -175,7 +185,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:
@@ -183,6 +193,9 @@ class QEMUMonitorProtocol:
  return self.cmd_obj(qmp_cmd)
  
  def command(self, cmd, **kwds):

+"""
+Build and send a QMP command to the monitor, report errors when any


I'm not native english speaker but I think "if any" sounds better.


+"""
  ret = self.cmd(cmd, kwds)
  if ret.has_key('error'):
  raise Exception(ret['error']['desc'])
@@ -190,15 +203,15 @@ class QEMUMonitorProtocol:
  
  def pull_event(self, wait=False):

  """
-Get and delete the first available QMP event.
+Get and pop the first available QMP event.


Both sentences seems unclear to me, regarding the function name... I 
wonder if removing this comment makes this function clearer.


  
  @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 QMPConnectError: If wait is True but no events could be
+retrieved or if some other error occurred.
  
  @return The first available QMP event, or None.

  """
@@ -217,8 +230,8 @@ class QEMUMonitorProtocol:
  
  @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 QMPConnectError: If wait is True but no events could be
+retrieved or if some other error occurred.
  
  @return The list of available QMP events.

  """
@@ -235,8 +248,6 @@ class