[PATCH] Add VM info to improve error log message for qemu monitor

[Peter Krempa]

On Tue, Dec 07, 2021 at 15:08:02 +0000, Daniel P. Berrangé wrote:
> On Tue, Dec 07, 2021 at 04:02:11PM +0100, Peter Krempa wrote:
> > On Tue, Dec 07, 2021 at 14:53:20 +0000, Daniel P. Berrangé wrote:
> > > On Tue, Dec 07, 2021 at 05:34:00AM -0800, Rohit Kumar wrote:
> > > > This patch is to determine the VM which had IO or socket hangup error.
> > > > 
> > > > Signed-off-by: Rohit Kumar <rohit.kumar3 at nutanix.com>
> > > > ---
> > > >  src/qemu/qemu_monitor.c | 46 +++++++++++++++++++++++++----------------
> > > >  1 file changed, 28 insertions(+), 18 deletions(-)
> > > > 
> > > > diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> > > > index 75e0e4ed92..d36db280d9 100644
> > > > --- a/src/qemu/qemu_monitor.c
> > > > +++ b/src/qemu/qemu_monitor.c
> > > > @@ -530,13 +530,19 @@ qemuMonitorIO(GSocket *socket G_GNUC_UNUSED,
> > > >      qemuMonitor *mon = opaque;
> > > >      bool error = false;
> > > >      bool hangup = false;
> > > > +    virDomainObj *vm = mon->vm;
> > > > +    g_autofree char *vmName = NULL;
> > > > +
> > > > +    if (vm != NULL && vm->def != NULL) {
> > > > +        vmName = g_strdup(vm->def->name);
> > > > +    }
> > > 
> > > This looks a little questionable.
> > > 
> > > Although we hold a reference on 'vm', this code doesn't do anything
> > > to protect its access of 'vm->def'. If we were protected when accesing
> > > vm->def, then we wouldn't need to strdup it anyway.
> > 
> > Additionally we also regularly log the monitor pointer and VM name when
> > entering the monitor context:
> > 
> > See qemuDomainObjEnterMonitorInternal:
> > 
> > VIR_DEBUG("Entering monitor (mon=%p vm=%p name=%s)",
> >           priv->mon, obj, obj->def->name);
> > 
> > In that place we are correctly still in the 'vm' context so we can
> > reference the name.
> > 
> > This call is usually very close to any other monitor calls or can be
> > easily looked up, so I don't think it's worth to increase the complexity
> > of the monitor code just to put the VM name in every single debug
> > message.
> 
> I'd be in favour of /consistently/ having  'mon=%p vm=%p' for all
> monitor debug logs though.

Use %p for 'vm' is fine as it doesn't break the locking boundary, but
the patch is doing %s for vm->def->name, which, if done properly would
make the code way more complex with questionable benefits.