scripts: Remove 'debug' parameter from QEMUMachine & QEMUMonitorProtocol
cover scripts: Remove 'debug' parameter from QEMUMachine & QEMUMonitorProtocol
This series removes the 'debug' parameter from QEMUMachine and QEMUMonitorProtocol and lets scripts use the logging module to enable debugging messages.
Eduardo Habkost (5):
guestperf: Configure logging on all shell frontends
iotests: Set up Python logging
basevm: Call logging.basicConfig()
scripts: Remove debug parameter from QEMUMonitorProtocol
scripts: Remove debug parameter from QEMUMachine
scripts/qemu.py | 9 +++------ scripts/qmp/qmp.py | 13 ++++++------- tests/migration/guestperf/engine.py | 6 ++---- tests/migration/guestperf/shell.py | 9 +++++++++ tests/qemu-iotests/iotests.py | 5 +++-- tests/vm/basevm.py | 4 ++-- 6 files changed, 25 insertions(+), 21 deletions(-)
Signature · 1 line
2.13.5
PATCH 1/5 guestperf: Configure logging on all shell frontends
The logging module will eventually replace the 'debug' parameter in QEMUMachine and QEMUMonitorProtocol.
--- tests/migration/guestperf/shell.py | 9 +++++++++ 1 file changed, 9 insertions(+)
tests/migration/guestperf/shell.py +9 -0
@@ -26,6 +26,7 @@ sys.path.append(os.path.join(os.path.dirname(__file__), import argparse import fnmatch import platform +import logging from guestperf.hardware import Hardware from guestperf.engine import Engine
@@ -64,6 +65,11 @@ class BaseShell(object): self._parser = parser + def init_logging(self, args): + logging.basicConfig(level=(logging.DEBUG if args.debug else + logging.INFO if args.verbose else + logging.WARN)) + def get_engine(self, args): return Engine(binary=args.binary, dst_host=args.dst_host,
@@ -147,6 +153,7 @@ class Shell(BaseShell): def run(self, argv): args = self._parser.parse_args(argv) + self.init_logging(args) engine = self.get_engine(args) hardware = self.get_hardware(args)
@@ -179,6 +186,7 @@ class BatchShell(BaseShell): def run(self, argv): args = self._parser.parse_args(argv) + self.init_logging(args) engine = self.get_engine(args) hardware = self.get_hardware(args)
@@ -231,6 +239,7 @@ class PlotShell(object): def run(self, argv): args = self._parser.parse_args(argv) + self.init_logging(args) if len(args.reports) == 0: print >>sys.stderr, "At least one report required"
Signature · 1 line
2.13.5
Re: [Qemu-devel] [PATCH 1/5] guestperf: Configure logging on all shell frontends
Quoted 8 lines · Eduardo Habkost
The logging module will eventually replace the 'debug' parameter in QEMUMachine and QEMUMonitorProtocol.
Cc Daniel P. Berrange <[email protected]>Signed-off-by Eduardo Habkost <[email protected]>--- tests/migration/guestperf/shell.py | 9 +++++++++ 1 file changed, 9 insertions(+)
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH 1/5] guestperf: Configure logging on all shell frontends
In reply to Eduardo HabkostDne 27.9.2017 v 15:03 Eduardo Habkost napsal(a):
Quoted 50 lines
The logging module will eventually replace the 'debug' parameter in QEMUMachine and QEMUMonitorProtocol.
Cc Daniel P. Berrange <[email protected]>Signed-off-by Eduardo Habkost <[email protected]>--- tests/migration/guestperf/shell.py | 9 +++++++++ 1 file changed, 9 insertions(+)tests/migration/guestperf/shell.py +8
-0@@ -26,6 +26,7 @@ sys.path.append(os.path.join(os.path.dirname(__file__), import argparse import fnmatch import platform +import logging from guestperf.hardware import Hardware from guestperf.engine import Engine@@ -64,6 +65,11 @@ class BaseShell(object): self._parser = parser + def init_logging(self, args): + logging.basicConfig(level=(logging.DEBUG if args.debug else + logging.INFO if args.verbose else + logging.WARN)) + def get_engine(self, args): return Engine(binary=args.binary, dst_host=args.dst_host,@@ -147,6 +153,7 @@ class Shell(BaseShell): def run(self, argv): args = self._parser.parse_args(argv) + self.init_logging(args) engine = self.get_engine(args) hardware = self.get_hardware(args)@@ -179,6 +186,7 @@ class BatchShell(BaseShell): def run(self, argv): args = self._parser.parse_args(argv) + self.init_logging(args) engine = self.get_engine(args) hardware = self.get_hardware(args)
tests/migration/guestperf/shell.py class PlotShell(object): · line 241
Quoted 3 lines
tests/migration/guestperf/shell.py +0
-0@@ -231,6 +239,7 @@ class PlotShell(object): def run(self, argv): args = self._parser.parse_args(argv)
This class is not inherited from the BaseShell, therefor it does not contain init_logging method. Apart from this it looks good.
Quoted 5 lines
tests/migration/guestperf/shell.py +1
-0+ self.init_logging(args) if len(args.reports) == 0: print >>sys.stderr, "At least one report required"
Re: [Qemu-devel] [PATCH 1/5] guestperf: Configure logging on all shell frontends
On Thu, Sep 28, 2017 at 11:15:47AM +0200, Lukáš Doktor wrote:
[...]
Quoted 8 lines
tests/migration/guestperf/shell.py class PlotShell(object): · line 241
Quoted 4 lines
tests/migration/guestperf/shell.py +0
-0@@ -231,6 +239,7 @@ class PlotShell(object): def run(self, argv): args = self._parser.parse_args(argv)This class is not inherited from the
BaseShell, therefor it does not containinit_loggingmethod. Apart from this it looks good.
Oops. I will fix this in v2. Thanks!
Quoted 6 lines
Quoted 6 lines
tests/migration/guestperf/shell.py +1
-0+ self.init_logging(args) if len(args.reports) == 0: print >>sys.stderr, "At least one report required"
Signature · 1 line
Eduardo
PATCH 2/5 iotests: Set up Python logging
In reply to Eduardo HabkostSet up Python logging module instead of relying on QEMUMachine._debug to enable debugging messages.
--- tests/qemu-iotests/iotests.py | 3 +++ 1 file changed, 3 insertions(+)
tests/qemu-iotests/iotests.py +3 -0
@@ -28,6 +28,7 @@ import qtest import struct import json import signal +import logging # This will not work if arguments contain spaces but is necessary if we
@@ -467,6 +468,8 @@ def main(supported_fmts=[], supported_oses=['linux']): else: output = StringIO.StringIO() + logging.basicConfig(level=(logging.DEBUG if debug else logging.WARN)) + class MyTestRunner(unittest.TextTestRunner): def __init__(self, stream=output, descriptions=True, verbosity=verbosity): unittest.TextTestRunner.__init__(self, stream, descriptions, verbosity)
Signature · 1 line
2.13.5
Re: [Qemu-devel] [PATCH 2/5] iotests: Set up Python logging
Quoted 10 lines · Eduardo Habkost
Set up Python logging module instead of relying on QEMUMachine._debug to enable debugging messages.
Cc Kevin Wolf <[email protected]>Cc Max Reitz <[email protected]>Signed-off-by Eduardo Habkost <[email protected]>--- tests/qemu-iotests/iotests.py | 3 +++ 1 file changed, 3 insertions(+)
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH 2/5] iotests: Set up Python logging
In reply to Eduardo HabkostDne 27.9.2017 v 15:03 Eduardo Habkost napsal(a):
Quoted 24 lines
Set up Python logging module instead of relying on QEMUMachine._debug to enable debugging messages.
Cc Kevin Wolf <[email protected]>Cc Max Reitz <[email protected]>Signed-off-by Eduardo Habkost <[email protected]>--- tests/qemu-iotests/iotests.py | 3 +++ 1 file changed, 3 insertions(+)tests/qemu-iotests/iotests.py +1
-0@@ -28,6 +28,7 @@ import qtest import struct import json import signal +import logging # This will not work if arguments contain spaces but is necessary if we
tests/qemu-iotests/iotests.py def main(supported_fmts=[], supported_oses=['linux']): · line 472
Quoted 5 lines
tests/qemu-iotests/iotests.py +2
-0@@ -467,6 +468,8 @@ def main(supported_fmts=[], supported_oses=['linux']): else: output = StringIO.StringIO() + logging.basicConfig(level=(logging.DEBUG if debug else logging.WARN)) +
Strictly speaking the brackets around ternary operator are not necessary, but given this is not a python project it's probably fine.
tests/qemu-iotests/iotests.py def main(supported_fmts=[], supported_oses=['linux']): · line 475
Quoted 4 lines
tests/qemu-iotests/iotests.py +0
-0class MyTestRunner(unittest.TextTestRunner): def __init__(self, stream=output, descriptions=True, verbosity=verbosity): unittest.TextTestRunner.__init__(self, stream, descriptions, verbosity)
Yes, this is a better solution than the previous logging hotfix.
PATCH 3/5 basevm: Call logging.basicConfig()
In reply to Eduardo HabkostJust setting level=DEBUG when debug is enabled is not enough: we need to set up a log handler if we want debug messages generated using logging.getLogger(...).debug() to be printed.
This was not a problem before because logging.debug() calls logging.basicConfig() implicitly, but it's safer to not rely on that.
--- tests/vm/basevm.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
tests/vm/basevm.py +2 -2
@@ -227,8 +227,8 @@ def main(vmcls): if not argv and not args.build_qemu and not args.build_image: print "Nothing to do?" return 1 - if args.debug: - logging.getLogger().setLevel(logging.DEBUG) + logging.basicConfig(level=(logging.DEBUG if args.debug + else logging.WARN)) vm = vmcls(debug=args.debug, vcpus=args.jobs) if args.build_image: if os.path.exists(args.image) and not args.force:
Signature · 1 line
2.13.5
Re: [Qemu-devel] [PATCH 3/5] basevm: Call logging.basicConfig()
Quoted 8 lines · Eduardo Habkost
Just setting level=DEBUG when debug is enabled is not enough: we need to set up a log handler if we want debug messages generated using logging.getLogger(...).debug() to be printed.
This was not a problem before because
logging.debug()callslogging.basicConfig()implicitly, but it's safer to not rely on that.
Quoted 7 lines
Cc "Alex Bennée" <[email protected]>Cc Fam Zheng <[email protected]>Cc "Philippe Mathieu-Daudé" <[email protected]>Signed-off-by Eduardo Habkost <[email protected]>--- tests/vm/basevm.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH 3/5] basevm: Call logging.basicConfig()
In reply to Eduardo HabkostQuoted 21 lines · Eduardo Habkost
Just setting level=DEBUG when debug is enabled is not enough: we need to set up a log handler if we want debug messages generated using logging.getLogger(...).debug() to be printed.
This was not a problem before because
logging.debug()callslogging.basicConfig()implicitly, but it's safer to not rely on that.Cc "Alex Bennée" <[email protected]>Cc Fam Zheng <[email protected]>Cc "Philippe Mathieu-Daudé" <[email protected]>Signed-off-by Eduardo Habkost <[email protected]>--- tests/vm/basevm.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)tests/vm/basevm.py +0
-0
tests/vm/basevm.py def main(vmcls): · line 231
Quoted 7 lines
tests/vm/basevm.py +2
-2@@ -227,8 +227,8 @@ def main(vmcls): if not argv and not args.build_qemu and not args.build_image: print "Nothing to do?" return 1 - if args.debug: - logging.getLogger().setLevel(logging.DEBUG) + logging.basicConfig(level=(logging.DEBUG if args.debug + else logging.WARN))
I find the " ? : " expression in C more readable than this in Python. :)
Quoted 6 lines
tests/vm/basevm.py +0
-0vm = vmcls(debug=args.debug, vcpus=args.jobs) if args.build_image: if os.path.exists(args.image) and not args.force:Signature · 1 line
2.13.5
Re: [Qemu-devel] [PATCH 3/5] basevm: Call logging.basicConfig()
Quoted 17 lines · Fam Zheng
Quoted 16 lines · Eduardo Habkost
Just setting level=DEBUG when debug is enabled is not enough: we need to set up a log handler if we want debug messages generated using logging.getLogger(...).debug() to be printed.
This was not a problem before because
logging.debug()callslogging.basicConfig()implicitly, but it's safer to not rely on that.Cc "Alex Bennée" <[email protected]>Cc Fam Zheng <[email protected]>Cc "Philippe Mathieu-Daudé" <[email protected]>Signed-off-by Eduardo Habkost <[email protected]>--- tests/vm/basevm.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Quoted 14 lines
tests/vm/basevm.py def main(vmcls): · line 231
Quoted 12 lines
tests/vm/basevm.py +2
-2@@ -227,8 +227,8 @@ def main(vmcls): if not argv and not args.build_qemu and not args.build_image: print "Nothing to do?" return 1 - if args.debug: - logging.getLogger().setLevel(logging.DEBUG) + logging.basicConfig(level=(logging.DEBUG if args.debug + else logging.WARN))I find the " ? : " expression in C more readable than this in Python. :)
I think everybody except Guido does. :)
Signature · 1 line
Eduardo
Re: [Qemu-devel] [PATCH 3/5] basevm: Call logging.basicConfig()
In reply to Eduardo HabkostDne 27.9.2017 v 15:03 Eduardo Habkost napsal(a):
Quoted 22 lines
Just setting level=DEBUG when debug is enabled is not enough: we need to set up a log handler if we want debug messages generated using logging.getLogger(...).debug() to be printed.
This was not a problem before because
logging.debug()callslogging.basicConfig()implicitly, but it's safer to not rely on that.Cc "Alex Bennée" <[email protected]>Cc Fam Zheng <[email protected]>Cc "Philippe Mathieu-Daudé" <[email protected]>Signed-off-by Eduardo Habkost <[email protected]>--- tests/vm/basevm.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)tests/vm/basevm.py +0
-0
tests/vm/basevm.py def main(vmcls): · line 234
Quoted 10 lines
tests/vm/basevm.py +2
-2@@ -227,8 +227,8 @@ def main(vmcls): if not argv and not args.build_qemu and not args.build_image: print "Nothing to do?" return 1 - if args.debug: - logging.getLogger().setLevel(logging.DEBUG) + logging.basicConfig(level=(logging.DEBUG if args.debug + else logging.WARN)) vm = vmcls(debug=args.debug, vcpus=args.jobs) if args.build_image: if os.path.exists(args.image) and not args.force:
PATCH 4/5 scripts: Remove debug parameter from QEMUMonitorProtocol
In reply to Eduardo HabkostUse 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.
--- scripts/qemu.py | 3 +-- scripts/qmp/qmp.py | 13 ++++++------- 2 files changed, 7 insertions(+), 9 deletions(-)
scripts/qemu.py +1 -2
@@ -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()
scripts/qmp/qmp.py +6 -7
@@ -12,6 +12,7 @@ import json import errno import socket import sys +import logging class QMPError(Exception):
@@ -32,6 +33,8 @@ 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
@@ -51,7 +54,6 @@ class QEMUMonitorProtocol(object): """ self.__events = [] self.__address = address - self._debug = debug 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) try: self.__sock.sendall(json.dumps(qmp_cmd)) except socket.error as err:
@@ -173,8 +173,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):
Signature · 1 line
2.13.5
Re: [Qemu-devel] [PATCH 4/5] scripts: Remove debug parameter from QEMUMonitorProtocol
Quoted 13 lines · Eduardo Habkost
Use logging module for the QMP debug messages. The only scripts that set debug=True are
iotests.pyandguestperf/engine.py, and they already calllogging.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.
Quoted 8 lines
Cc "Alex Bennée" <[email protected]>Cc Fam Zheng <[email protected]>Cc "Philippe Mathieu-Daudé" <[email protected]>Signed-off-by Eduardo Habkost <[email protected]>--- scripts/qemu.py | 3 +-- scripts/qmp/qmp.py | 13 ++++++------- 2 files changed, 7 insertions(+), 9 deletions(-)
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH 4/5] scripts: Remove debug parameter from QEMUMonitorProtocol
In reply to Eduardo Habkostscripts/qmp/qmp.py class QEMUMonitorProtocol(object): · line 54
Quoted 5 lines · Eduardo Habkost
scripts/qmp/qmp.py +0
-1@@ -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?
Quoted 14 lines
scripts/qmp/qmp.py +1
-2self.__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
scripts/qmp/qmp.py class QEMUMonitorProtocol(object): · line 168
Quoted 6 lines
scripts/qmp/qmp.py +1
-2@@ -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".
Re: [Qemu-devel] [PATCH 4/5] scripts: Remove debug parameter from QEMUMonitorProtocol
Quoted 8 lines · Fam Zheng
scripts/qmp/qmp.py class QEMUMonitorProtocol(object): · line 54
Quoted 5 lines · Eduardo Habkost
scripts/qmp/qmp.py +0
-1@@ -51,7 +54,6 @@ class QEMUMonitorProtocol(object): """ self.__events = [] self.__address = address - self._debug = debugShould you also drop the debug parameter from the method?
I will do. Thanks for noticing!
Quoted 15 lines
Quoted 15 lines
scripts/qmp/qmp.py +1
-2self.__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):
Quoted 8 lines
scripts/qmp/qmp.py class QEMUMonitorProtocol(object): · line 168
Quoted 6 lines
scripts/qmp/qmp.py +1
-2@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".
Good catch. Thanks!
I will send a fixup in a moment.
Signature · 1 line
Eduardo
PATCH 1/1 fixup! scripts: Remove debug parameter from QEMUMonitorProtocol
In reply to Fam ZhengQuoted 23 lines · Fam Zheng
Review class QEMUMonitorProtocol(object): · line 54
Quoted 5 lines · Eduardo Habkost
(quoted diff) +0
-1@@ -51,7 +54,6 @@ class QEMUMonitorProtocol(object): """ self.__events = [] self.__address = address - self._debug = debugShould you also drop the debug parameter from the method?
Quoted 14 lines
(quoted diff) +1
-2self.__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):
Quoted 9 lines
Review class QEMUMonitorProtocol(object): · line 168
Quoted 6 lines
(quoted diff) +1
-2@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.
--- scripts/qmp/qmp.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
scripts/qmp/qmp.py +2 -2
@@ -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:
Signature · 1 line
2.13.5
Re: [Qemu-devel] [PATCH] fixup! scripts: Remove debug parameter from QEMUMonitorProtocol
Dne 27.9.2017 v 15:44 Eduardo Habkost napsal(a):
Quoted 10 lines · Fam Zheng
Quoted 9 lines · Eduardo Habkost
Review class QEMUMonitorProtocol(object): · line 54
Quoted 5 lines
(quoted diff) +0
-1@@ -51,7 +54,6 @@ class QEMUMonitorProtocol(object): """ self.__events = [] self.__address = address - self._debug = debugShould you also drop the debug parameter from the method?
Quoted 9 lines
Quoted 9 lines
Review class QEMUMonitorProtocol(object): · line 87
Quoted 9 lines
(quoted diff) +0
-2self.__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áš
Quoted 45 lines
Quoted 5 lines
Quoted 5 lines
(quoted diff) +1
-0+ self.logger.debug("<<< %s", resp) self.__events.append(resp) if not only_event: continue@@ -164,8 +165,7 @@ class QEMUMonitorProtocol(object):Quoted 9 lines
scripts/qmp/qmp.py class QEMUMonitorProtocol(object):
Quoted 6 lines
scripts/qmp/qmp.py +1
-2@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 <[email protected]>--- scripts/qmp/qmp.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)scripts/qmp/qmp.py +2
-2@@ -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:
Re: [Qemu-devel] [PATCH] fixup! scripts: Remove debug parameter from QEMUMonitorProtocol
Quoted 11 lines · Lukáš Doktor
Dne 27.9.2017 v 15:44 Eduardo Habkost napsal(a):
Quoted 10 lines · Fam Zheng
Quoted 9 lines · Eduardo Habkost
Review class QEMUMonitorProtocol(object): · line 54
Quoted 5 lines
(quoted diff) +0
-1@@ -51,7 +54,6 @@ class QEMUMonitorProtocol(object): """ self.__events = [] self.__address = address - self._debug = debugShould you also drop the debug parameter from the method?
Quoted 15 lines
Quoted 9 lines
Quoted 9 lines
Review class QEMUMonitorProtocol(object): · line 87
Quoted 9 lines
(quoted diff) +0
-2self.__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" % respThis is the only user of
sysimport, please remove it as well. Apart from this it looks good, although you might consider using__name__instead of hardcodedQMPinlogger = 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!
Quoted 50 lines
Lukáš
Quoted 45 lines
Quoted 5 lines
Quoted 5 lines
(quoted diff) +1
-0+ self.logger.debug("<<< %s", resp) self.__events.append(resp) if not only_event: continue@@ -164,8 +165,7 @@ class QEMUMonitorProtocol(object):Quoted 9 lines
scripts/qmp/qmp.py class QEMUMonitorProtocol(object):
Quoted 6 lines
scripts/qmp/qmp.py +1
-2@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 <[email protected]>--- scripts/qmp/qmp.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)scripts/qmp/qmp.py +2
-2@@ -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:
Signature · 1 line
Eduardo
PATCH 5/5 scripts: Remove debug parameter from QEMUMachine
In reply to Eduardo HabkostAll scripts that use the QEMUMachine and QEMUQtestMachine classes (device-crash-test, tests/migration/*, iotests.py, basevm.py) already configure logging.
The basicConfig() call inside QEMUMachine.__init__() is being kept just to make sure a script would still work if it didn't configure logging.
--- scripts/qemu.py | 6 ++---- tests/migration/guestperf/engine.py | 6 ++---- tests/qemu-iotests/iotests.py | 2 -- 3 files changed, 4 insertions(+), 10 deletions(-)
scripts/qemu.py +2 -4
@@ -54,7 +54,7 @@ class QEMUMachine(object): def __init__(self, binary, args=None, wrapper=None, name=None, test_dir="/var/tmp", monitor_address=None, - socket_scm_helper=None, debug=False): + socket_scm_helper=None): ''' Initialize a QEMUMachine
@@ -65,7 +65,6 @@ class QEMUMachine(object): @param test_dir: where to create socket and log file @param monitor_address: address for QMP monitor @param socket_scm_helper: helper program, required for send_fd_scm()" - @param debug: enable debug mode @note: Qemu process is not started until launch() is used. ''' if args is None:
@@ -85,12 +84,11 @@ class QEMUMachine(object): self._events = [] self._iolog = None self._socket_scm_helper = socket_scm_helper - self._debug = debug self._qmp = None self._qemu_full_args = None # just in case logging wasn't configured by the main script: - logging.basicConfig(level=(logging.DEBUG if debug else logging.WARN)) + logging.basicConfig() def __enter__(self): return self
tests/migration/guestperf/engine.py +2 -4
@@ -388,15 +388,13 @@ class Engine(object): args=self._get_src_args(hardware), wrapper=self._get_src_wrapper(hardware), name="qemu-src-%d" % os.getpid(), - monitor_address=srcmonaddr, - debug=self._debug) + monitor_address=srcmonaddr) dst = qemu.QEMUMachine(self._binary, args=self._get_dst_args(hardware, uri), wrapper=self._get_dst_wrapper(hardware), name="qemu-dst-%d" % os.getpid(), - monitor_address=dstmonaddr, - debug=self._debug) + monitor_address=dstmonaddr) try: src.launch()
tests/qemu-iotests/iotests.py +0 -2
@@ -195,8 +195,6 @@ class VM(qtest.QEMUQtestMachine): 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 self._num_drives = 0 def add_device(self, opts):
Signature · 1 line
2.13.5
Re: [Qemu-devel] [PATCH 5/5] scripts: Remove debug parameter from QEMUMachine
Quoted 7 lines · Eduardo Habkost
All scripts that use the QEMUMachine and QEMUQtestMachine classes (device-crash-test, tests/migration/*,
iotests.py,basevm.py) already configure logging.The
basicConfig()call insideQEMUMachine.__init__()is being kept just to make sure a script would still work if it didn't configure logging.
I don't find that compelling. IIUC, if we remove this basicConfig they'll see a message that logging is not configured, which is a suitable hint to fix the script to configure logging.
scripts/qemu.py class QEMUMachine(object): · line 91
Quoted 4 lines
# just in case logging wasn't configured by the main script:scripts/qemu.py +1
-1- logging.basicConfig(level=(logging.DEBUG if debug else logging.WARN)) + logging.basicConfig()
So I'd just remove this line entirely
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH 5/5] scripts: Remove debug parameter from QEMUMachine
Quoted 5 lines · Daniel P. Berrange
Quoted 4 lines · Eduardo Habkost
All scripts that use the QEMUMachine and QEMUQtestMachine classes (device-crash-test, tests/migration/*,
iotests.py,basevm.py) already configure logging.
Quoted 7 lines
Quoted 3 lines
The
basicConfig()call insideQEMUMachine.__init__()is being kept just to make sure a script would still work if it didn't configure logging.I don't find that compelling. IIUC, if we remove this basicConfig they'll see a message that logging is not configured, which is a suitable hint to fix the script to configure logging.
I don't see the benefit of requiring the caller to configure logging even if they just want the default behavior (WARN loglevel, logged to stderr).
Quoted 6 lines
scripts/qemu.py class QEMUMachine(object): · line 91
Quoted 4 lines
# just in case logging wasn't configured by the main script:scripts/qemu.py +1
-1- logging.basicConfig(level=(logging.DEBUG if debug else logging.WARN)) + logging.basicConfig()So I'd just remove this line entirely
I think it does no harm, and can save people from wasting time googling for "No handlers could be found for logger" just to find out they need to add a logging.basicConfig() call to their script.
Signature · 1 line
Eduardo