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
> > +                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:
-- 
2.13.5

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:
> > 
> 
> 




-- 
Eduardo