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

[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 <address@hidden>
>> ---
>>   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.
>> +        @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 QEMUMonitorProtocol:
>>           self.__sock.close()
>>           self.__sockfile.close()
>>   -    timeout = socket.timeout
>> -
>>       def settimeout(self, timeout):
>>           self.__sock.settimeout(timeout)
>>  
> 
> Regards,
> 
> Phil.

signature.asc
Description: OpenPGP digital signature