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 <js...@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 <ovosh...@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):
> > """
> >
>
>
