Re: [Qemu-devel] [PATCH] fixup! scripts: Remove debug parameter from QEMUMonitorProtocol

[Eduardo Habkost]

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 <address@hidden>
> > ---
> >  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:
> > 
> 
> 




-- 
Eduardo