[libvirt] [PATCH] Fix memory leak in qemuBuildDriveStr()
This patch fixes memory leaks reported by valgrind on running qemuxml2argvtest Most of them are of the form: ==24777== 15 bytes in 1 blocks are definitely lost in loss record 39 of 129 ==24777== at 0x4A0887C: malloc (vg_replace_malloc.c:270) ==24777== by 0x341F485E21: strdup (strdup.c:42) ==24777== by 0x4CADE5F: virStrdup (virstring.c:554) ==24777== by 0x4362B6: qemuBuildDriveStr (qemu_command.c:3848) ==24777== by 0x43EF73: qemuBuildCommandLine (qemu_command.c:8500) ==24777== by 0x426670: testCompareXMLToArgvHelper (qemuxml2argvtest.c:350) ==24777== by 0x427C01: virtTestRun (testutils.c:138) ==24777== by 0x41DDB5: mymain (qemuxml2argvtest.c:658) ==24777== by 0x4282A2: virtTestMain (testutils.c:593) ==24777== by 0x341F421A04: (below main) (libc-start.c:225) ==24777== --- src/qemu/qemu_command.c | 74 +++++++++++++++++++++++++------------------------ 1 file changed, 38 insertions(+), 36 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 768fdc4..6a30a29 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4016,52 +4016,54 @@ qemuBuildDriveStr(virConnectPtr conn, if (qemuDomainDiskGetSourceString(conn, disk, &source) < 0) goto error; - if (source && - !((disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY || - disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) && - disk->tray_status == VIR_DOMAIN_DISK_TRAY_OPEN)) { + if (source) { + if(!((disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY || + disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) && + disk->tray_status == VIR_DOMAIN_DISK_TRAY_OPEN)) { - virBufferAddLit(&opt, "file="); + virBufferAddLit(&opt, "file="); - switch (actualType) { - case VIR_DOMAIN_DISK_TYPE_DIR: - /* QEMU only supports magic FAT format for now */ - if (disk->format > 0 && disk->format != VIR_STORAGE_FILE_FAT) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unsupported disk driver type for '%s'"), - virStorageFileFormatTypeToString(disk->format)); - goto error; - } + switch (actualType) { + case VIR_DOMAIN_DISK_TYPE_DIR: + /* QEMU only supports magic FAT format for now */ + if (disk->format > 0 && disk->format != VIR_STORAGE_FILE_FAT) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unsupported disk driver type for '%s'"), + virStorageFileFormatTypeToString(disk->format)); + goto error; + } - if (!disk->readonly) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cannot create virtual FAT disks in read-write mode")); - goto error; - } + if (!disk->readonly) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot create virtual FAT disks in read-write mode")); + goto error; + } - virBufferAddLit(&opt, "fat:"); + virBufferAddLit(&opt, "fat:"); - if (disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) - virBufferAddLit(&opt, "floppy:"); + if (disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) + virBufferAddLit(&opt, "floppy:"); - break; + break; - case VIR_DOMAIN_DISK_TYPE_BLOCK: - if (disk->tray_status == VIR_DOMAIN_DISK_TRAY_OPEN) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - disk->type == VIR_DOMAIN_DISK_TYPE_VOLUME ? - _("tray status 'open' is invalid for block type volume") : - _("tray status 'open' is invalid for block type disk")); - goto error; - } + case VIR_DOMAIN_DISK_TYPE_BLOCK: + if (disk->tray_status == VIR_DOMAIN_DISK_TRAY_OPEN) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + disk->type == VIR_DOMAIN_DISK_TYPE_VOLUME ? + _("tray status 'open' is invalid for block type volume") : + _("tray status 'open' is invalid for block type disk")); + goto error; + } - break; + break; - default: - break; - } + default: + break; + } - virBufferEscape(&opt, ',', ",", "%s,", source); + virBufferEscape(&opt, ',', ",", "%s,", source); + } + VIR_FREE(source); } if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) -- 1.8.1.4
On 12/04/2013 01:23 PM, Nehal J Wani wrote:
This patch fixes memory leaks reported by valgrind on running qemuxml2argvtest
Most of them are of the form:
==24777== 15 bytes in 1 blocks are definitely lost in loss record 39 of 129 ==24777== at 0x4A0887C: malloc (vg_replace_malloc.c:270) ==24777== by 0x341F485E21: strdup (strdup.c:42) ==24777== by 0x4CADE5F: virStrdup (virstring.c:554) ==24777== by 0x4362B6: qemuBuildDriveStr (qemu_command.c:3848) ==24777== by 0x43EF73: qemuBuildCommandLine (qemu_command.c:8500) ==24777== by 0x426670: testCompareXMLToArgvHelper (qemuxml2argvtest.c:350) ==24777== by 0x427C01: virtTestRun (testutils.c:138) ==24777== by 0x41DDB5: mymain (qemuxml2argvtest.c:658) ==24777== by 0x4282A2: virtTestMain (testutils.c:593) ==24777== by 0x341F421A04: (below main) (libc-start.c:225) ==24777==
Correct that it is a leak, but it seems like this patch is overkill in the number of lines touched, and still has leaks on error cases. I think we want this instead: diff --git i/src/qemu/qemu_command.c w/src/qemu/qemu_command.c index 768fdc4..9539be7 100644 --- i/src/qemu/qemu_command.c +++ w/src/qemu/qemu_command.c @@ -4063,6 +4063,7 @@ qemuBuildDriveStr(virConnectPtr conn, virBufferEscape(&opt, ',', ",", "%s,", source); } + VIR_FREE(source); if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) virBufferAddLit(&opt, "if=none"); @@ -4274,6 +4275,7 @@ qemuBuildDriveStr(virConnectPtr conn, return virBufferContentAndReset(&opt); error: + VIR_FREE(source); virBufferFreeAndReset(&opt); return NULL; } -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Thu, Dec 5, 2013 at 2:17 AM, Eric Blake <eblake@redhat.com> wrote:
On 12/04/2013 01:23 PM, Nehal J Wani wrote:
This patch fixes memory leaks reported by valgrind on running qemuxml2argvtest
Most of them are of the form:
==24777== 15 bytes in 1 blocks are definitely lost in loss record 39 of 129 ==24777== at 0x4A0887C: malloc (vg_replace_malloc.c:270) ==24777== by 0x341F485E21: strdup (strdup.c:42) ==24777== by 0x4CADE5F: virStrdup (virstring.c:554) ==24777== by 0x4362B6: qemuBuildDriveStr (qemu_command.c:3848) ==24777== by 0x43EF73: qemuBuildCommandLine (qemu_command.c:8500) ==24777== by 0x426670: testCompareXMLToArgvHelper (qemuxml2argvtest.c:350) ==24777== by 0x427C01: virtTestRun (testutils.c:138) ==24777== by 0x41DDB5: mymain (qemuxml2argvtest.c:658) ==24777== by 0x4282A2: virtTestMain (testutils.c:593) ==24777== by 0x341F421A04: (below main) (libc-start.c:225) ==24777==
Correct that it is a leak, but it seems like this patch is overkill in the number of lines touched, and still has leaks on error cases.
I think we want this instead:
diff --git i/src/qemu/qemu_command.c w/src/qemu/qemu_command.c index 768fdc4..9539be7 100644 --- i/src/qemu/qemu_command.c +++ w/src/qemu/qemu_command.c @@ -4063,6 +4063,7 @@ qemuBuildDriveStr(virConnectPtr conn,
virBufferEscape(&opt, ',', ",", "%s,", source); } + VIR_FREE(source);
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) virBufferAddLit(&opt, "if=none"); @@ -4274,6 +4275,7 @@ qemuBuildDriveStr(virConnectPtr conn, return virBufferContentAndReset(&opt);
error: + VIR_FREE(source); virBufferFreeAndReset(&opt); return NULL; }
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Sounds reasonable. Thanks. -- Nehal J Wani
participants (2)
-
Eric Blake -
Nehal J Wani