qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Lukáš Doktor
Subject: Re: [Qemu-devel] [PATCH v2 2/3] scripts: Remove debug parameter from QEMUMonitorProtocol
Date: Tue, 10 Oct 2017 18:03:14 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0

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" <address@hidden>
>>> Cc: Fam Zheng <address@hidden>
>>> Cc: "Philippe Mathieu-Daudé" <address@hidden>
>>> Signed-off-by: Eduardo Habkost <address@hidden>
>>> ---
>>> Changes v1 -> v2:
>>> * Actually remove debug parameter from method definition
>>>   (Fam Zheng)
>>> * Fix "<<<" vs ">>>" confusion
>>>   (Fam Zheng)
>>> * Remove "import sys" line
>>>   (Lukáš Doktor)
>>> ---
>>>  scripts/qemu.py    |  3 +--
>>>  scripts/qmp/qmp.py | 16 +++++++---------
>>>  2 files changed, 8 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/scripts/qemu.py b/scripts/qemu.py
>>> index c9a106fbce..f6d2e68627 100644
>>> --- a/scripts/qemu.py
>>> +++ b/scripts/qemu.py
>>> @@ -177,8 +177,7 @@ class QEMUMachine(object):
>>>  
>>>      def _pre_launch(self):
>>>          self._qmp = qmp.qmp.QEMUMonitorProtocol(self._monitor_address,
>>> -                                                server=True,
>>> -                                                debug=self._debug)
>>> +                                                server=True)
>>>  
>>>      def _post_launch(self):
>>>          self._qmp.accept()
>>> diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
>>> index ef12e8a1a0..07c9632e9e 100644
>>> --- a/scripts/qmp/qmp.py
>>> +++ b/scripts/qmp/qmp.py
>>> @@ -11,7 +11,7 @@
>>>  import json
>>>  import errno
>>>  import socket
>>> -import sys
>>> +import logging
>>>  
>>>  
>>>  class QMPError(Exception):
>>> @@ -32,12 +32,14 @@ class QMPTimeoutError(QMPError):
>>>  
>>>  class QEMUMonitorProtocol(object):
>>>  
>>> +    #: Logger object for debugging messages
>>> +    logger = logging.getLogger('QMP')
>>>      #: Socket's error class
>>>      error = socket.error
>>>      #: Socket's timeout
>>>      timeout = socket.timeout
>>>  
>>> -    def __init__(self, address, server=False, debug=False):
>>> +    def __init__(self, address, server=False):
>>>          """
>>>          Create a QEMUMonitorProtocol class.
>>>  
>>> @@ -51,7 +53,6 @@ class QEMUMonitorProtocol(object):
>>>          """
>>>          self.__events = []
>>>          self.__address = address
>>> -        self._debug = debug
>>>          self.__sock = self.__get_sock()
>>>          self.__sockfile = None
>>>          if server:
>>> @@ -83,8 +84,7 @@ class QEMUMonitorProtocol(object):
>>>                  return
>>>              resp = json.loads(data)
>>>              if 'event' in resp:
>>> -                if self._debug:
>>> -                    print >>sys.stderr, "QMP:<<< %s" % resp
>>> +                self.logger.debug("<<< %s", resp)
>>>                  self.__events.append(resp)
>>>                  if not only_event:
>>>                      continue
>>> @@ -164,8 +164,7 @@ class QEMUMonitorProtocol(object):
>>>          @return QMP response as a Python dict or None if the connection has
>>>                  been closed
>>>          """
>>> -        if self._debug:
>>> -            print >>sys.stderr, "QMP:>>> %s" % qmp_cmd
>>> +        self.logger.debug(">>> %s", qmp_cmd)
>>>          try:
>>>              self.__sock.sendall(json.dumps(qmp_cmd))
>>>          except socket.error as err:
>>> @@ -173,8 +172,7 @@ class QEMUMonitorProtocol(object):
>>>                  return
>>>              raise socket.error(err)
>>>          resp = self.__json_read()
>>> -        if self._debug:
>>> -            print >>sys.stderr, "QMP:<<< %s" % resp
>>> +        self.logger.debug("<<< %s", resp)
>>>          return resp
>>>  
>>>      def cmd(self, name, args=None, cmd_id=None):
>>>
>>
>> This one looks good, but in order to no break qemu-iotests verbose mode it 
>> requires fix to qtest/iotests:
>>
>> ```diff
>> diff --git a/scripts/qtest.py b/scripts/qtest.py
>> index df0daf2..0e955a8 100644
>> --- a/scripts/qtest.py
>> +++ b/scripts/qtest.py
>> @@ -77,12 +77,12 @@ class QEMUQtestMachine(qemu.QEMUMachine):
>>      '''A QEMU VM'''
>>  
>>      def __init__(self, binary, args=None, name=None, test_dir="/var/tmp",
>> -                 socket_scm_helper=None):
>> +                 socket_scm_helper=None, debug=False):
>>          if name is None:
>>              name = "qemu-%d" % os.getpid()
>>          super(QEMUQtestMachine,
>>                self).__init__(binary, args, name=name, test_dir=test_dir,
>> -                             socket_scm_helper=socket_scm_helper)
>> +                             socket_scm_helper=socket_scm_helper, 
>> debug=debug)
>>          self._qtest = None
>>          self._qtest_path = os.path.join(test_dir, name + "-qtest.sock")
>>  
>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>> index 1af117e..989ebd3 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -193,9 +193,8 @@ class VM(qtest.QEMUQtestMachine):
>>          name = "qemu%s-%d" % (path_suffix, os.getpid())
>>          super(VM, self).__init__(qemu_prog, qemu_opts, name=name,
>>                                   test_dir=test_dir,
>> -                                 socket_scm_helper=socket_scm_helper)
>> -        if debug:
>> -            self._debug = True
>> +                                 socket_scm_helper=socket_scm_helper,
>> +                                 debug=debug)
>>          self._num_drives = 0
>>  
>>      def add_device(self, opts):
>> ```
> 
> I'm confused by why the above patch is necessary.  We are in the
> process of removing the 'debug' parameter from QEMUMachine and
> QEMUMonitorProtocol, why would we add a debug parameter to
> QEMUQtestMachine and iotests.py?
> 
>>
>> (because the `debug` used to be set after `__init__`, but the logging is 
>> initialized during `__init__`.)
>>
>> Therefor conditional ACK when the qtest/iotest fix precedes this commit.
> 
> Do you mean the following?
> 
>   Message-Id: <address@hidden>
>   Subject: [Qemu-devel] [PATCH 2/5] iotests: Set up Python logging
>   https://www.mail-archive.com/address@hidden/msg485036.html
>   
> https://github.com/ehabkost/qemu/commit/afa79b55676dcd1859aa9d1f983c9dfbbcc13197
> 

I'm sorry, my fault. With the afa79b55676dcd1859aa9d1f983c9dfbbcc13197 commit 
it works well and I haven't found any other issues.

Reviewed-by: Lukáš Doktor <address@hidden> 

> It is already queued on python-next.
> 


Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]