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

[Oksana Voshchana <ovoshcha@redhat.com>]

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

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 <jsnow@redhat.com> 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 <ovoshcha@redhat.com>
> > ---
> >  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):
> >          """
> >
>
>

[-- Attachment #2: Type: text/html, Size: 7009 bytes --]