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

[Rohit Kumar]

On 04/01/22 2:17 pm, Peter Krempa wrote:
> On Tue, Jan 04, 2022 at 09:45:30 +0100, Peter Krempa wrote:
>> On Tue, Jan 04, 2022 at 14:10:49 +0530, Rohit Kumar wrote:
>>> On 03/01/22 10:12 pm, Peter Krempa wrote:
>>>> On Wed, Dec 22, 2021 at 22:39:21 -0800, Rohit Kumar wrote:
>>>>> This patch is to determine the VM which had IO or socket hangup error.
>>>>> Accessing directly vm->def->name inside qemuMonitorIO() or qemuMonitorSend()
>>>>> might leads to illegal access as we are out of 'vm' context and vm->def might
>>>>> not exist. Adding a field "domainName" inside mon object to access vm name
>>>>> and initialising it when creating mon object.
>>>>>
>>>>> Signed-off-by: Rohit Kumar <rohit.kumar3 at nutanix.com>
>>>>> ---
>> [...]
>>
>>>>> @@ -609,13 +616,14 @@ qemuMonitorIO(GSocket *socket G_GNUC_UNUSED,
>>>>>                virResetLastError();
>>>>>            } else {
>>>>>                if (virGetLastErrorCode() == VIR_ERR_OK && !mon->goteof)
>>>>> -                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>>>>> -                               _("Error while processing monitor IO"));
>>>>> +                virReportError(VIR_ERR_INTERNAL_ERROR,
>>>>> +                               _("%s: Error while processing monitor IO"), NULLSTR(vmName));
>>>> Same here. Additionally did you ever get to a situation where vmName
>>>> would be NULL?
>>> There might be a situation when g_strdup() fails to allocate vm name, for
>>> e.g. if host ran out of memory ?
>> No, g_strdup on out of memory condition abort()s the program completely.
>>
>> The only time when g_strdup returns NULL is if the argument is NULL.
>>
>>> Let me know your thoughts on this, I would be happy to remove NULLSTR if
>>> it's not the case.
>> That depends if you happened to see whether all callers avoid passing
>> NULL name for the VM which is very likely.
> Alternatively you can do g_strdup(NULLSTR(vm->def->name)). Thus the
> domain name variable will hold a copy of "<null>" as the name. Since
> it's for error messages only this is tolerable and allows you to avoid
> all the other NULLSTR usage.
Sure, this would be much better, I will update this. Thanks for the 
suggestion.
>