Message ID | 20170927134431.GN21016@localhost.localdomain |
---|---|
State | New |
Headers | show |
Series | fixup! scripts: Remove debug parameter from QEMUMonitorProtocol | expand |
Dne 27.9.2017 v 15:44 Eduardo Habkost napsal(a): > On Wed, Sep 27, 2017 at 09:33:21PM +0800, Fam Zheng wrote: >> On Wed, 09/27 10:03, Eduardo Habkost wrote: >>> @@ -51,7 +54,6 @@ class QEMUMonitorProtocol(object): >>> """ >>> self.__events = [] >>> self.__address = address >>> - self._debug = debug >> >> Should you also drop the debug parameter from the method? >> >>> self.__sock = self.__get_sock() >>> self.__sockfile = None >>> if server: >>> @@ -83,8 +85,7 @@ class QEMUMonitorProtocol(object): >>> return >>> resp = json.loads(data) >>> if 'event' in resp: >>> - if self._debug: >>> - print >>sys.stderr, "QMP:<<< %s" % resp This is the only user of `sys` import, please remove it as well. Apart from this it looks good, although you might consider using `__name__` instead of hardcoded `QMP` in `logger = logging.getLogger(__name__)` for the sake of consistency (people might expect it to correlate with the module name). Lukáš >>> + self.logger.debug("<<< %s", resp) >>> self.__events.append(resp) >>> if not only_event: >>> continue >>> @@ -164,8 +165,7 @@ class QEMUMonitorProtocol(object): >>> @return QMP response as a Python dict or None if the connection has >>> been closed >>> """ >>> - if self._debug: >>> - print >>sys.stderr, "QMP:>>> %s" % qmp_cmd >>> + self.logger.debug("<<< %s", qmp_cmd) >> >> This should be ">>> %s". >> > > Fixed. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > scripts/qmp/qmp.py | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py > index be79d7aa80..369d9fef39 100644 > --- a/scripts/qmp/qmp.py > +++ b/scripts/qmp/qmp.py > @@ -40,7 +40,7 @@ class QEMUMonitorProtocol(object): > #: Socket's timeout > timeout = socket.timeout > > - def __init__(self, address, server=False, debug=False): > + def __init__(self, address, server=False): > """ > Create a QEMUMonitorProtocol class. > > @@ -165,7 +165,7 @@ class QEMUMonitorProtocol(object): > @return QMP response as a Python dict or None if the connection has > been closed > """ > - self.logger.debug("<<< %s", qmp_cmd) > + self.logger.debug(">>> %s", qmp_cmd) > try: > self.__sock.sendall(json.dumps(qmp_cmd)) > except socket.error as err: >
On Thu, Sep 28, 2017 at 11:33:57AM +0200, Lukáš Doktor wrote: > Dne 27.9.2017 v 15:44 Eduardo Habkost napsal(a): > > On Wed, Sep 27, 2017 at 09:33:21PM +0800, Fam Zheng wrote: > >> On Wed, 09/27 10:03, Eduardo Habkost wrote: > >>> @@ -51,7 +54,6 @@ class QEMUMonitorProtocol(object): > >>> """ > >>> self.__events = [] > >>> self.__address = address > >>> - self._debug = debug > >> > >> Should you also drop the debug parameter from the method? > >> > >>> self.__sock = self.__get_sock() > >>> self.__sockfile = None > >>> if server: > >>> @@ -83,8 +85,7 @@ class QEMUMonitorProtocol(object): > >>> return > >>> resp = json.loads(data) > >>> if 'event' in resp: > >>> - if self._debug: > >>> - print >>sys.stderr, "QMP:<<< %s" % resp > > This is the only user of `sys` import, please remove it as > well. Apart from this it looks good, although you might > consider using `__name__` instead of hardcoded `QMP` in `logger > = logging.getLogger(__name__)` for the sake of consistency > (people might expect it to correlate with the module name). I will remove 'import sys' in v2, but I disagree about using the module name: prefixing them with "QMP" makes them more readable and familiar than using "qmp.qmp". Thanks! > > Lukáš > > >>> + self.logger.debug("<<< %s", resp) > >>> self.__events.append(resp) > >>> if not only_event: > >>> continue > >>> @@ -164,8 +165,7 @@ class QEMUMonitorProtocol(object): > >>> @return QMP response as a Python dict or None if the connection has > >>> been closed > >>> """ > >>> - if self._debug: > >>> - print >>sys.stderr, "QMP:>>> %s" % qmp_cmd > >>> + self.logger.debug("<<< %s", qmp_cmd) > >> > >> This should be ">>> %s". > >> > > > > Fixed. > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > --- > > scripts/qmp/qmp.py | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py > > index be79d7aa80..369d9fef39 100644 > > --- a/scripts/qmp/qmp.py > > +++ b/scripts/qmp/qmp.py > > @@ -40,7 +40,7 @@ class QEMUMonitorProtocol(object): > > #: Socket's timeout > > timeout = socket.timeout > > > > - def __init__(self, address, server=False, debug=False): > > + def __init__(self, address, server=False): > > """ > > Create a QEMUMonitorProtocol class. > > > > @@ -165,7 +165,7 @@ class QEMUMonitorProtocol(object): > > @return QMP response as a Python dict or None if the connection has > > been closed > > """ > > - self.logger.debug("<<< %s", qmp_cmd) > > + self.logger.debug(">>> %s", qmp_cmd) > > try: > > self.__sock.sendall(json.dumps(qmp_cmd)) > > except socket.error as err: > > > >
diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py index be79d7aa80..369d9fef39 100644 --- a/scripts/qmp/qmp.py +++ b/scripts/qmp/qmp.py @@ -40,7 +40,7 @@ class QEMUMonitorProtocol(object): #: Socket's timeout timeout = socket.timeout - def __init__(self, address, server=False, debug=False): + def __init__(self, address, server=False): """ Create a QEMUMonitorProtocol class. @@ -165,7 +165,7 @@ class QEMUMonitorProtocol(object): @return QMP response as a Python dict or None if the connection has been closed """ - self.logger.debug("<<< %s", qmp_cmd) + self.logger.debug(">>> %s", qmp_cmd) try: self.__sock.sendall(json.dumps(qmp_cmd)) except socket.error as err: