| Message ID | 20171005172013.3098-3-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
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.bennee@linaro.org> > Cc: Fam Zheng <famz@redhat.com> > Cc: "Philippe Mathieu-Daudé" <f4bug@amsat.org> > Signed-off-by: Eduardo Habkost <ehabkost@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): ``` (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. Lukáš
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.bennee@linaro.org> > > Cc: Fam Zheng <famz@redhat.com> > > Cc: "Philippe Mathieu-Daudé" <f4bug@amsat.org> > > Signed-off-by: Eduardo Habkost <ehabkost@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-ehabkost@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.
Dne 10.10.2017 v 04:49 Eduardo Habkost napsal(a): > 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.bennee@linaro.org> >>> Cc: Fam Zheng <famz@redhat.com> >>> Cc: "Philippe Mathieu-Daudé" <f4bug@amsat.org> >>> Signed-off-by: Eduardo Habkost <ehabkost@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-ehabkost@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 > I'm sorry, my fault. With the afa79b55676dcd1859aa9d1f983c9dfbbcc13197 commit it works well and I haven't found any other issues. Reviewed-by: Lukáš Doktor <ldoktor@redhat.com> > It is already queued on python-next. >
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):
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.bennee@linaro.org> Cc: Fam Zheng <famz@redhat.com> Cc: "Philippe Mathieu-Daudé" <f4bug@amsat.org> Signed-off-by: Eduardo Habkost <ehabkost@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(-)