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

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(a)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.