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" <alex.ben...@linaro.org>
> > Cc: Fam Zheng <f...@redhat.com>
> > Cc: "Philippe Mathieu-Daudé" <f4...@amsat.org>
> > Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
> > ---
> > 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: <20170927130339.21444-3-ehabk...@redhat.com>
Subject: [Qemu-devel] [PATCH 2/5] iotests: Set up Python logging
https://www.mail-archive.com/qemu-devel@nongnu.org/msg485036.html
https://github.com/ehabkost/qemu/commit/afa79b55676dcd1859aa9d1f983c9dfbbcc13197
It is already queued on python-next.
--
Eduardo
