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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 04/01/22 3:56 pm, Ani Sinha wrote:

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

Ack. Thanks for the review Ani.




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux