Re: [PATCH] python/qemu/qmp.py: QMP debug with VM label

[Oksana Voshchana]

Hi John

Your approach to using logger hierarchy it's actually what I need.

Thanks for the review.
The second version of the patch I will send in a few mins

On Tue, Mar 10, 2020 at 3:46 AM John Snow <address@hidden> wrote:

On 3/4/20 5:05 AM, Oksana Vohchana wrote:
> QEMUMachine writes some messages to the default logger.
> But it sometimes to hard the read the output if we have requested to
> more than one VM.
> This patch adds name in QMP command if it needs and labels with it in
> debug mode.
>

Hiya!

I like the end result, but I don't like the methodology of pushing
higher-level details into a lower-level library.

> Signed-off-by: Oksana Vohchana <address@hidden>
> ---
>  python/qemu/machine.py | 8 ++++----
>  python/qemu/qmp.py     | 9 ++++++---
>  2 files changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
> index 183d8f3d38..060e68f06b 100644
> --- a/python/qemu/machine.py
> +++ b/python/qemu/machine.py
> @@ -391,7 +391,7 @@ class QEMUMachine(object):
>              self._qmp_set = False
>              self._qmp = None
> 
> -    def qmp(self, cmd, conv_keys=True, **args):
> +    def qmp(self, cmd, conv_keys=True, vm_name=None, **args):

in machine.py, we should already have access to self._name -- the name
of the machine. Let's use that.

>          """
>          Invoke a QMP command and return the response dict
>          """
> @@ -402,15 +402,15 @@ class QEMUMachine(object):
>              else:
>                  qmp_args[key] = value
> 
> -        return self._qmp.cmd(cmd, args=qmp_args)
> +        return self._qmp.cmd(cmd, args=qmp_args, vm_name=vm_name)
> 

Adding a name per-each call to QMP is a bit much. Let's consolidate it
and set it *exactly once*.

A fine place to do that would be QMP's __init__ method:

(in machine.py:)

self._qmp = qmp.QEMUMonitorProtocol(self._vm_monitor, server=True,
remote_name=self._name)


Then, in QMP's init, you can do something like:

def __init__(self, address, server=False, nickname=None):
    self.nickname = nickname

... and then on subsequent logging calls, you can use the nickname of
the connection to print better logging messages.


Some other notes:

1. QEMUMonitorProtocol uses a class variable `logger` instead of an
instance variable logger. If this was made per-instance, you could
change the logger of any given QMP object as-desired from the caller.

2. I'd rename the default QMP logger to be 'qemu.QMP' instead of 'QMP'
to respect the hierarchical logging namespace.

3. If a caller set qmp.logger = logging.getLogger('qemu.QMP.mynamehere')
then all messages printed by this QMP instance would use the
`mynamehere` prefix by default for all messages it printed. This might
be enough to get the behavior you want.

(Also, it would be very powerful for many other reasons, well beyond
what you're asking for here, to allow callers to change how QMP logs,
where, and with what messages.)


There's probably a lot of ways to do it; but I'd pick one where we don't
have to pass names around for every call.

--js


> -    def command(self, cmd, conv_keys=True, **args):
> +    def command(self, cmd, conv_keys=True, vm_name=None, **args):
>          """
>          Invoke a QMP command.
>          On success return the response dict.
>          On failure raise an exception.
>          """
> -        reply = self.qmp(cmd, conv_keys, **args)
> +        reply = self.qmp(cmd, conv_keys, vm_name, **args)
>          if reply is None:
>              raise qmp.QMPError("Monitor is closed")
>          if "error" in reply:
> diff --git a/python/qemu/qmp.py b/python/qemu/qmp.py
> index f40586eedd..96b455b53f 100644
> --- a/python/qemu/qmp.py
> +++ b/python/qemu/qmp.py
> @@ -180,11 +180,12 @@ class QEMUMonitorProtocol:
>          self.__sockfile = self.__sock.makefile()
>          return self.__negotiate_capabilities()
> 
> -    def cmd_obj(self, qmp_cmd):
> +    def cmd_obj(self, qmp_cmd, vm_name=None):
>          """
>          Send a QMP command to the QMP Monitor.
> 
>          @param qmp_cmd: QMP command to be sent as a Python dict
> +        @param vm_name: name for the virtual machine (string)
>          @return QMP response as a Python dict or None if the connection has
>                  been closed
>          """
> @@ -196,10 +197,12 @@ class QEMUMonitorProtocol:
>                  return None
>              raise err
>          resp = self.__json_read()
> +        if vm_name:
> +            self.logger.debug("<<< {'vm_name' : %s }",  vm_name)
>          self.logger.debug("<<< %s", resp)
>          return resp
> 
> -    def cmd(self, name, args=None, cmd_id=None):
> +    def cmd(self, name, args=None, cmd_id=None, vm_name=None):
>          """
>          Build a QMP command and send it to the QMP Monitor.
> 
> @@ -212,7 +215,7 @@ class QEMUMonitorProtocol:
>              qmp_cmd['arguments'] = args
>          if cmd_id:
>              qmp_cmd['id'] = cmd_id
> -        return self.cmd_obj(qmp_cmd)
> +        return self.cmd_obj(qmp_cmd, vm_name)
> 
>      def command(self, cmd, **kwds):
>          """
>