Re: [Qemu-devel] [PATCH v2 2/3] scripts: Remove debug parameter from QEMUMonitorProtocol

[Eduardo Habkost]

On Sat, Oct 07, 2017 at 10:26:14AM +0200, Lukáš Doktor wrote:
> Dne 5.10.2017 v 19:20 Eduardo Habkost napsal(a):
> > Use logging module for the QMP debug messages.  The only scripts
> > that set debug=True are iotests.py and guestperf/engine.py, and
> > they already call logging.basicConfig() to set up logging.
> > 
> > Scripts that don't configure logging are safe as long as they
> > don't need debugging output, because debug messages don't trigger
> > the "No handlers could be found for logger" message from the
> > Python logging module.
> > 
> > Scripts that already configure logging but don't use debug=True
> > (e.g. scripts/vm/basevm.py) will get QMP debugging enabled for
> > free.
> > 
> > Cc: "Alex Bennée" <address@hidden>
> > Cc: Fam Zheng <address@hidden>
> > Cc: "Philippe Mathieu-Daudé" <address@hidden>
> > Signed-off-by: Eduardo Habkost <address@hidden>
> > ---
> > Changes v1 -> v2:
> > * Actually remove debug parameter from method definition
> >   (Fam Zheng)
> > * Fix "<<<" vs ">>>" confusion
> >   (Fam Zheng)
> > * Remove "import sys" line
> >   (Lukáš Doktor)
> > ---
> >  scripts/qemu.py    |  3 +--
> >  scripts/qmp/qmp.py | 16 +++++++---------
> >  2 files changed, 8 insertions(+), 11 deletions(-)
> > 
> > diff --git a/scripts/qemu.py b/scripts/qemu.py
> > index c9a106fbce..f6d2e68627 100644
> > --- a/scripts/qemu.py
> > +++ b/scripts/qemu.py
> > @@ -177,8 +177,7 @@ class QEMUMachine(object):
> >  
> >      def _pre_launch(self):
> >          self._qmp = qmp.qmp.QEMUMonitorProtocol(self._monitor_address,
> > -                                                server=True,
> > -                                                debug=self._debug)
> > +                                                server=True)
> >  
> >      def _post_launch(self):
> >          self._qmp.accept()
> > diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
> > index ef12e8a1a0..07c9632e9e 100644
> > --- a/scripts/qmp/qmp.py
> > +++ b/scripts/qmp/qmp.py
> > @@ -11,7 +11,7 @@
> >  import json
> >  import errno
> >  import socket
> > -import sys
> > +import logging
> >  
> >  
> >  class QMPError(Exception):
> > @@ -32,12 +32,14 @@ class QMPTimeoutError(QMPError):
> >  
> >  class QEMUMonitorProtocol(object):
> >  
> > +    #: Logger object for debugging messages
> > +    logger = logging.getLogger('QMP')
> >      #: Socket's error class
> >      error = socket.error
> >      #: Socket's timeout
> >      timeout = socket.timeout
> >  
> > -    def __init__(self, address, server=False, debug=False):
> > +    def __init__(self, address, server=False):
> >          """
> >          Create a QEMUMonitorProtocol class.
> >  
> > @@ -51,7 +53,6 @@ class QEMUMonitorProtocol(object):
> >          """
> >          self.__events = []
> >          self.__address = address
> > -        self._debug = debug
> >          self.__sock = self.__get_sock()
> >          self.__sockfile = None
> >          if server:
> > @@ -83,8 +84,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 +164,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)
> >          try:
> >              self.__sock.sendall(json.dumps(qmp_cmd))
> >          except socket.error as err:
> > @@ -173,8 +172,7 @@ class QEMUMonitorProtocol(object):
> >                  return
> >              raise socket.error(err)
> >          resp = self.__json_read()
> > -        if self._debug:
> > -            print >>sys.stderr, "QMP:<<< %s" % resp
> > +        self.logger.debug("<<< %s", resp)
> >          return resp
> >  
> >      def cmd(self, name, args=None, cmd_id=None):
> > 
> 
> This one looks good, but in order to no break qemu-iotests verbose mode it 
> requires fix to qtest/iotests:
> 
> ```diff
> diff --git a/scripts/qtest.py b/scripts/qtest.py
> index df0daf2..0e955a8 100644
> --- a/scripts/qtest.py
> +++ b/scripts/qtest.py
> @@ -77,12 +77,12 @@ class QEMUQtestMachine(qemu.QEMUMachine):
>      '''A QEMU VM'''
>  
>      def __init__(self, binary, args=None, name=None, test_dir="/var/tmp",
> -                 socket_scm_helper=None):
> +                 socket_scm_helper=None, debug=False):
>          if name is None:
>              name = "qemu-%d" % os.getpid()
>          super(QEMUQtestMachine,
>                self).__init__(binary, args, name=name, test_dir=test_dir,
> -                             socket_scm_helper=socket_scm_helper)
> +                             socket_scm_helper=socket_scm_helper, 
> debug=debug)
>          self._qtest = None
>          self._qtest_path = os.path.join(test_dir, name + "-qtest.sock")
>  
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 1af117e..989ebd3 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -193,9 +193,8 @@ class VM(qtest.QEMUQtestMachine):
>          name = "qemu%s-%d" % (path_suffix, os.getpid())
>          super(VM, self).__init__(qemu_prog, qemu_opts, name=name,
>                                   test_dir=test_dir,
> -                                 socket_scm_helper=socket_scm_helper)
> -        if debug:
> -            self._debug = True
> +                                 socket_scm_helper=socket_scm_helper,
> +                                 debug=debug)
>          self._num_drives = 0
>  
>      def add_device(self, opts):
> ```

I'm confused by why the above patch is necessary.  We are in the
process of removing the 'debug' parameter from QEMUMachine and
QEMUMonitorProtocol, why would we add a debug parameter to
QEMUQtestMachine and iotests.py?

> 
> (because the `debug` used to be set after `__init__`, but the logging is 
> initialized during `__init__`.)
> 
> Therefor conditional ACK when the qtest/iotest fix precedes this commit.

Do you mean the following?

  Message-Id: <address@hidden>
  Subject: [Qemu-devel] [PATCH 2/5] iotests: Set up Python logging
  https://www.mail-archive.com/address@hidden/msg485036.html
  
https://github.com/ehabkost/qemu/commit/afa79b55676dcd1859aa9d1f983c9dfbbcc13197

It is already queued on python-next.

-- 
Eduardo