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

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