[Qemu-devel] [PATCH 0/5] scripts: Remove 'debug' parameter from QEMUMachine & QEMUMonitorProtocol

[Qemu-devel] [PATCH 0/5] scripts: Remove 'debug' parameter from QEMUMachine & QEMUMonitorProtocol

[Eduardo Habkost]

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(-)

-- 
2.13.5

[Qemu-devel] [PATCH 1/5] guestperf: Configure logging on all shell frontends

[Eduardo Habkost]

The logging module will eventually replace the 'debug' parameter
in QEMUMachine and QEMUMonitorProtocol.

Cc: Daniel P. Berrange <berrange@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 tests/migration/guestperf/shell.py | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/tests/migration/guestperf/shell.py b/tests/migration/guestperf/shell.py
index 7992459a97..e378cf0cfa 100644
--- a/tests/migration/guestperf/shell.py
+++ b/tests/migration/guestperf/shell.py
@@ -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"
-- 
2.13.5

Re: [Qemu-devel] [PATCH 1/5] guestperf: Configure logging on all shell frontends

[Daniel P. Berrange]

On Wed, Sep 27, 2017 at 10:03:35AM -0300, Eduardo Habkost wrote:
> The logging module will eventually replace the 'debug' parameter
> in QEMUMachine and QEMUMonitorProtocol.
> 
> Cc: Daniel P. Berrange <berrange@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  tests/migration/guestperf/shell.py | 9 +++++++++
>  1 file changed, 9 insertions(+)

Reviewed-by: Daniel P. Berrange <berrange@redhat.com>


Regards,
Daniel
-- 
|: 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

[Lukáš Doktor]

[-- Attachment #1: Type: text/plain, Size: 2244 bytes --]

Dne 27.9.2017 v 15:03 Eduardo Habkost napsal(a):
> The logging module will eventually replace the 'debug' parameter
> in QEMUMachine and QEMUMonitorProtocol.
> 
> Cc: Daniel P. Berrange <berrange@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  tests/migration/guestperf/shell.py | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/tests/migration/guestperf/shell.py b/tests/migration/guestperf/shell.py
> index 7992459a97..e378cf0cfa 100644
> --- a/tests/migration/guestperf/shell.py
> +++ b/tests/migration/guestperf/shell.py
> @@ -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)

This class is not inherited from the `BaseShell`, therefor it does not contain `init_logging` method. Apart from this it looks good.

> +        self.init_logging(args)
>  
>          if len(args.reports) == 0:
>              print >>sys.stderr, "At least one report required"
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 502 bytes --]

Re: [Qemu-devel] [PATCH 1/5] guestperf: Configure logging on all shell frontends

[Eduardo Habkost]

On Thu, Sep 28, 2017 at 11:15:47AM +0200, Lukáš Doktor wrote:
[...]
> > @@ -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.

Oops.  I will fix this in v2.  Thanks!

> 
> > +        self.init_logging(args)
> >  
> >          if len(args.reports) == 0:
> >              print >>sys.stderr, "At least one report required"
> > 

-- 
Eduardo

[Qemu-devel] [PATCH 2/5] iotests: Set up Python logging

[Eduardo Habkost]

Set up Python logging module instead of relying on
QEMUMachine._debug to enable debugging messages.

Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Max Reitz <mreitz@redhat.com>
Cc: qemu-block@nongnu.org
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 tests/qemu-iotests/iotests.py | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 1af117e37d..36a7757aaf 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -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)
-- 
2.13.5

Re: [Qemu-devel] [PATCH 2/5] iotests: Set up Python logging

[Daniel P. Berrange]

On Wed, Sep 27, 2017 at 10:03:36AM -0300, Eduardo Habkost wrote:
> Set up Python logging module instead of relying on
> QEMUMachine._debug to enable debugging messages.
> 
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Max Reitz <mreitz@redhat.com>
> Cc: qemu-block@nongnu.org
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  tests/qemu-iotests/iotests.py | 3 +++
>  1 file changed, 3 insertions(+)

Reviewed-by: Daniel P. Berrange <berrange@redhat.com>


Regards,
Daniel
-- 
|: 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

[Lukáš Doktor]

[-- Attachment #1: Type: text/plain, Size: 1508 bytes --]

Dne 27.9.2017 v 15:03 Eduardo Habkost napsal(a):
> Set up Python logging module instead of relying on
> QEMUMachine._debug to enable debugging messages.
> 
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Max Reitz <mreitz@redhat.com>
> Cc: qemu-block@nongnu.org
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  tests/qemu-iotests/iotests.py | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 1af117e37d..36a7757aaf 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -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))
> +

Strictly speaking the brackets around ternary operator are not necessary, but given this is not a python project it's probably fine.

>      class 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.

Reviewed-by: Lukáš Doktor <ldoktor@redhat.com> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 502 bytes --]

[Qemu-devel] [PATCH 3/5] basevm: Call logging.basicConfig()

[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() calls
logging.basicConfig() implicitly, but it's safer to not rely on
that.

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>
---
 tests/vm/basevm.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py
index 3c863bc237..686d88decf 100755
--- a/tests/vm/basevm.py
+++ b/tests/vm/basevm.py
@@ -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:
-- 
2.13.5

Re: [Qemu-devel] [PATCH 3/5] basevm: Call logging.basicConfig()

[Daniel P. Berrange]

On Wed, Sep 27, 2017 at 10:03:37AM -0300, Eduardo Habkost wrote:
> 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() calls
> logging.basicConfig() implicitly, but it's safer to not rely on
> that.
> 
> 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>
> ---
>  tests/vm/basevm.py | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Daniel P. Berrange <berrange@redhat.com>

Regards,
Daniel
-- 
|: 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()

[Fam Zheng]

On Wed, 09/27 10:03, Eduardo Habkost wrote:
> 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() calls
> logging.basicConfig() implicitly, but it's safer to not rely on
> that.
> 
> 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>
> ---
>  tests/vm/basevm.py | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py
> index 3c863bc237..686d88decf 100755
> --- a/tests/vm/basevm.py
> +++ b/tests/vm/basevm.py
> @@ -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. :)

>          vm = vmcls(debug=args.debug, vcpus=args.jobs)
>          if args.build_image:
>              if os.path.exists(args.image) and not args.force:
> -- 
> 2.13.5
> 

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH 3/5] basevm: Call logging.basicConfig()

[Eduardo Habkost]

On Wed, Sep 27, 2017 at 09:35:44PM +0800, Fam Zheng wrote:
> On Wed, 09/27 10:03, Eduardo Habkost wrote:
> > 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() calls
> > logging.basicConfig() implicitly, but it's safer to not rely on
> > that.
> > 
> > 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>
> > ---
> >  tests/vm/basevm.py | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py
> > index 3c863bc237..686d88decf 100755
> > --- a/tests/vm/basevm.py
> > +++ b/tests/vm/basevm.py
> > @@ -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.  :)

-- 
Eduardo

Re: [Qemu-devel] [PATCH 3/5] basevm: Call logging.basicConfig()

[Lukáš Doktor]

[-- Attachment #1: Type: text/plain, Size: 1429 bytes --]

Dne 27.9.2017 v 15:03 Eduardo Habkost napsal(a):
> 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() calls
> logging.basicConfig() implicitly, but it's safer to not rely on
> that.
> 
> 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>
> ---
>  tests/vm/basevm.py | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py
> index 3c863bc237..686d88decf 100755
> --- a/tests/vm/basevm.py
> +++ b/tests/vm/basevm.py
> @@ -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:
> 

Reviewed-by: Lukáš Doktor <ldoktor@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 502 bytes --]

[Qemu-devel] [PATCH 4/5] scripts: Remove debug parameter from QEMUMonitorProtocol

[Eduardo Habkost]

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>
---
 scripts/qemu.py    |  3 +--
 scripts/qmp/qmp.py | 13 ++++++-------
 2 files changed, 7 insertions(+), 9 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..be79d7aa80 100644
--- a/scripts/qmp/qmp.py
+++ b/scripts/qmp/qmp.py
@@ -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):
-- 
2.13.5

Re: [Qemu-devel] [PATCH 4/5] scripts: Remove debug parameter from QEMUMonitorProtocol

[Daniel P. Berrange]

On Wed, Sep 27, 2017 at 10:03:38AM -0300, Eduardo Habkost wrote:
> 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>
> ---
>  scripts/qemu.py    |  3 +--
>  scripts/qmp/qmp.py | 13 ++++++-------
>  2 files changed, 7 insertions(+), 9 deletions(-)

Reviewed-by: Daniel P. Berrange <berrange@redhat.com>

Regards,
Daniel
-- 
|: 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

[Fam Zheng]

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".

Re: [Qemu-devel] [PATCH 4/5] scripts: Remove debug parameter from QEMUMonitorProtocol

[Eduardo Habkost]

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?

I will do.  Thanks for noticing!

> 
> >          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".

Good catch.  Thanks!

I will send a fixup in a moment.

-- 
Eduardo

[Qemu-devel] [PATCH] fixup! scripts: Remove debug parameter from QEMUMonitorProtocol

[Eduardo Habkost]

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

Re: [Qemu-devel] [PATCH] fixup! scripts: Remove debug parameter from QEMUMonitorProtocol

[Lukáš Doktor]

[-- Attachment #1: Type: text/plain, Size: 2691 bytes --]

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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 502 bytes --]

Re: [Qemu-devel] [PATCH] fixup! scripts: Remove debug parameter from QEMUMonitorProtocol

[Eduardo Habkost]

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

[Qemu-devel] [PATCH 5/5] scripts: Remove debug parameter from QEMUMachine

[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 inside QEMUMachine.__init__() is being
kept just to make sure a script would still work if it didn't
configure logging.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 scripts/qemu.py                     | 6 ++----
 tests/migration/guestperf/engine.py | 6 ++----
 tests/qemu-iotests/iotests.py       | 2 --
 3 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/scripts/qemu.py b/scripts/qemu.py
index f6d2e68627..9bfdf6d37d 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -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
diff --git a/tests/migration/guestperf/engine.py b/tests/migration/guestperf/engine.py
index 0a13050bc6..e14d4320b2 100644
--- a/tests/migration/guestperf/engine.py
+++ b/tests/migration/guestperf/engine.py
@@ -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()
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 36a7757aaf..6f057904a9 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -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):
-- 
2.13.5

Re: [Qemu-devel] [PATCH 5/5] scripts: Remove debug parameter from QEMUMachine

[Daniel P. Berrange]

On Wed, Sep 27, 2017 at 10:03:39AM -0300, Eduardo Habkost wrote:
> All 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.

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.

>  
>          # just in case logging wasn't configured by the main script:
> -        logging.basicConfig(level=(logging.DEBUG if debug else logging.WARN))
> +        logging.basicConfig()

So I'd just remove this line entirely


Regards,
Daniel
-- 
|: 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

[Eduardo Habkost]

On Wed, Sep 27, 2017 at 02:22:24PM +0100, Daniel P. Berrange wrote:
> On Wed, Sep 27, 2017 at 10:03:39AM -0300, Eduardo Habkost wrote:
> > All 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.
> 
> 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).

> >  
> >          # just in case logging wasn't configured by the main script:
> > -        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.

-- 
Eduardo