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

[Rohit Kumar]

On 07/12/21 8:44 pm, Peter Krempa wrote:
> 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.
I wanted to add vm Name in with "End of File from qemu Monitor error" 
atleast, just to find which vm had a socket hangup.
How should i go further on this ?  To add vm name, we can do this steps :

1.) Pass virDomainObjPtr in this function after locking this. Just like 
we do in qemuDomainObjEnterMonitorInternal.
2.) Add a vm_name field in mon object and populate vm_name in 
qemuDomainObjEnterMonitorInternal, since that is still vm context.

These will be complex to do. Or should i just add 'mon=%p vm=%p' to 
every monitor debug logs ?
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20211208/432b5dd3/attachment-0001.htm>