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

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(a)nutanix.com&gt; > > > --- > > > 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.