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

On Tue, 4 Jan 2022, Peter Krempa wrote: > On Tue, Jan 04, 2022 at 15:30:00 +0530, Ani Sinha wrote: >> On Tue, 4 Jan 2022, Rohit Kumar wrote: >>> On 03/01/22 7:12 pm, Ani Sinha wrote: >>>> On Wed, 22 Dec 2021, Rohit Kumar wrote: > [...] > >>>>> @@ -694,6 +702,7 @@ qemuMonitorOpenInternal(virDomainObj *vm, >>>>> mon->fd = fd; >>>>> mon->context = g_main_context_ref(context); >>>>> mon->vm = virObjectRef(vm); >>>>> + mon->domainName = g_strdup(vm->def->name); >>>> do not forget to g_free() this during cleanup in the same function. >>> So, in cleanup: qemuMonitorDispose is called. And there I have added g_free() >>> to clean mon->domainName. >> No, in cleanup, I see qemuMonitorClose() is called where do you do not add >> any additional code to free the allocation. >> >> This is what I see in cleanup code: >> >> ``` >> cleanup: >> /* We don't want the 'destroy' callback invoked during >> * cleanup from construction failure, because that can >> * give a double-unref on virDomainObj *in the caller, >> * so kill the callbacks now. >> */ >> mon->cb = NULL; >> /* The caller owns 'fd' on failure */ >> mon->fd = -1; >> qemuMonitorClose(mon); > qemuMonitorClose() eventually calls virObjectUnref(mon). Once the last > reference on the monitor object is removed the object is freed via > qemuMonitorDispose(). > > This patch has: > > @@ -243,6 +244,7 @@ qemuMonitorDispose(void *obj) > virCondDestroy(&mon->notify); > g_free(mon->buffer); > g_free(mon->balloonpath); > + g_free(mon->domainName); > } Oh ok. I assumed that there was two cleanup paths : one is the natural tear down where qemuMonitorDispose() would be called. Other was failure during contruction itself which I thought needed additional handling. Its ok then, no need to add additional g_free in monitorOpen()