| Message ID | 20170720162815.19802-7-ldoktor@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <ldoktor@redhat.com> > --- > 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 QEMUMonitorProtocol: > self.__sock.close() > self.__sockfile.close() > > - timeout = socket.timeout > - > def settimeout(self, timeout): > self.__sock.settimeout(timeout) > > Regards, Phil.
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 <ldoktor@redhat.com> >> --- >> 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.
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 <ldoktor@redhat.com> >>> --- >>> 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)
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 <ldoktor@redhat.com> >>>> --- >>>> 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)
On Tue, Jul 25, 2017 at 3:13 AM, Lukáš Doktor <ldoktor@redhat.com> 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 <ldoktor@redhat.com> >>>>> --- >>>>> 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.
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 + 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: 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 + """ 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. @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)
No actual code changes, just a few pylint/style fixes and docstring clarifications. Signed-off-by: Lukáš Doktor <ldoktor@redhat.com> --- scripts/qmp/qmp.py | 37 ++++++++++++++++++++++++------------- 1 file changed, 24 insertions(+), 13 deletions(-)