[libvirt] [PATCH] qemu: avoid memory leaks

2011-08-02 Thread Eric Blake 
Quite a few leaks detected by coverity.  For chr, the leaks were
close enough to the allocations to plug in place; for disk, the
leaks were separated from the allocation by enough other lines with
intermediate failure cases that I refactored the cleanup instead.

* src/qemu/qemu_command.c (qemuParseCommandLine): Plug leaks.
---
 src/qemu/qemu_command.c |   23 ---
 1 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 6a2e2ae..c638420 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -5938,6 +5938,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
 int video = VIR_DOMAIN_VIDEO_TYPE_CIRRUS;
 int nvirtiodisk = 0;
 qemuDomainCmdlineDefPtr cmd = NULL;
+virDomainDiskDefPtr disk = NULL;

 if (pidfile)
 *pidfile = NULL;
@@ -6124,7 +6125,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
STRPREFIX(arg, "-fd") ||
STREQ(arg, "-cdrom")) {
 WANT_VALUE();
-virDomainDiskDefPtr disk;
 if (VIR_ALLOC(disk) < 0)
 goto no_memory;

@@ -6239,6 +6239,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
 goto no_memory;
 }
 def->disks[def->ndisks++] = disk;
+disk = NULL;
 } else if (STREQ(arg, "-no-acpi")) {
 def->features &= ~(1 << VIR_DOMAIN_FEATURE_ACPI);
 } else if (STREQ(arg, "-no-reboot")) {
@@ -6307,8 +6308,10 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
 if (!(chr = virDomainChrDefNew()))
 goto error;

-if (qemuParseCommandLineChr(&chr->source, val) < 0)
+if (qemuParseCommandLineChr(&chr->source, val) < 0) {
+virDomainChrDefFree(chr);
 goto error;
+}
 if (VIR_REALLOC_N(def->serials, def->nserials+1) < 0) {
 virDomainChrDefFree(chr);
 goto no_memory;
@@ -6325,8 +6328,10 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
 if (!(chr = virDomainChrDefNew()))
 goto error;

-if (qemuParseCommandLineChr(&chr->source, val) < 0)
+if (qemuParseCommandLineChr(&chr->source, val) < 0) {
+virDomainChrDefFree(chr);
 goto error;
+}
 if (VIR_REALLOC_N(def->parallels, def->nparallels+1) < 0) {
 virDomainChrDefFree(chr);
 goto no_memory;
@@ -6353,7 +6358,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
 }
 def->inputs[def->ninputs++] = input;
 } else if (STRPREFIX(val, "disk:")) {
-virDomainDiskDefPtr disk;
 if (VIR_ALLOC(disk) < 0)
 goto no_memory;
 disk->src = strdup(val + strlen("disk:"));
@@ -6376,6 +6380,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
 goto no_memory;
 }
 def->disks[def->ndisks++] = disk;
+disk = NULL;
 } else {
 virDomainHostdevDefPtr hostdev;
 if (!(hostdev = qemuParseCommandLineUSB(val)))
@@ -6399,7 +6404,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
 def->nets[def->nnets++] = net;
 }
 } else if (STREQ(arg, "-drive")) {
-virDomainDiskDefPtr disk;
 WANT_VALUE();
 if (!(disk = qemuParseCommandLineDisk(caps, val, nvirtiodisk)))
 goto error;
@@ -6408,6 +6412,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
 goto no_memory;
 }
 def->disks[def->ndisks++] = disk;
+disk = NULL;

 if (disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO)
 nvirtiodisk++;
@@ -6516,8 +6521,10 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
 if (VIR_ALLOC(chr) < 0)
 goto no_memory;

-if (qemuParseCommandLineChr(chr, val) < 0)
+if (qemuParseCommandLineChr(chr, val) < 0) {
+VIR_FREE(chr);
 goto error;
+}

 *monConfig = chr;
 }
@@ -6545,10 +6552,11 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
 char *hosts, *port, *saveptr = NULL, *token;
 virDomainDiskDefPtr first_rbd_disk = NULL;
 for (i = 0 ; i < def->ndisks ; i++) {
-virDomainDiskDefPtr disk = def->disks[i];
+disk = def->disks[i];
 if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK &&
 disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) {
 first_rbd_disk = disk;
+disk = NULL;

Re: [libvirt] [PATCH] qemu: avoid memory leaks

2011-08-02 Thread Laine Stump 

On 08/02/2011 04:10 PM, Eric Blake wrote:

Quite a few leaks detected by coverity.  For chr, the leaks were
close enough to the allocations to plug in place; for disk, the
leaks were separated from the allocation by enough other lines with
intermediate failure cases that I refactored the cleanup instead.

* src/qemu/qemu_command.c (qemuParseCommandLine): Plug leaks.
---
  src/qemu/qemu_command.c |   23 ---
  1 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 6a2e2ae..c638420 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -5938,6 +5938,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
  int video = VIR_DOMAIN_VIDEO_TYPE_CIRRUS;
  int nvirtiodisk = 0;
  qemuDomainCmdlineDefPtr cmd = NULL;
+virDomainDiskDefPtr disk = NULL;

  if (pidfile)
  *pidfile = NULL;
@@ -6124,7 +6125,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
 STRPREFIX(arg, "-fd") ||
 STREQ(arg, "-cdrom")) {
  WANT_VALUE();
-virDomainDiskDefPtr disk;
  if (VIR_ALLOC(disk)<  0)
  goto no_memory;

@@ -6239,6 +6239,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
  goto no_memory;
  }
  def->disks[def->ndisks++] = disk;
+disk = NULL;
  } else if (STREQ(arg, "-no-acpi")) {
  def->features&= ~(1<<  VIR_DOMAIN_FEATURE_ACPI);
  } else if (STREQ(arg, "-no-reboot")) {
@@ -6307,8 +6308,10 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
  if (!(chr = virDomainChrDefNew()))
  goto error;

-if (qemuParseCommandLineChr(&chr->source, val)<  0)
+if (qemuParseCommandLineChr(&chr->source, val)<  0) {
+virDomainChrDefFree(chr);
  goto error;
+}
  if (VIR_REALLOC_N(def->serials, def->nserials+1)<  0) {
  virDomainChrDefFree(chr);
  goto no_memory;
@@ -6325,8 +6328,10 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
  if (!(chr = virDomainChrDefNew()))
  goto error;

-if (qemuParseCommandLineChr(&chr->source, val)<  0)
+if (qemuParseCommandLineChr(&chr->source, val)<  0) {
+virDomainChrDefFree(chr);
  goto error;
+}
  if (VIR_REALLOC_N(def->parallels, def->nparallels+1)<  0) {
  virDomainChrDefFree(chr);
  goto no_memory;
@@ -6353,7 +6358,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
  }
  def->inputs[def->ninputs++] = input;
  } else if (STRPREFIX(val, "disk:")) {
-virDomainDiskDefPtr disk;
  if (VIR_ALLOC(disk)<  0)
  goto no_memory;
  disk->src = strdup(val + strlen("disk:"));
@@ -6376,6 +6380,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
  goto no_memory;
  }
  def->disks[def->ndisks++] = disk;
+disk = NULL;
  } else {
  virDomainHostdevDefPtr hostdev;
  if (!(hostdev = qemuParseCommandLineUSB(val)))
@@ -6399,7 +6404,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
  def->nets[def->nnets++] = net;
  }
  } else if (STREQ(arg, "-drive")) {
-virDomainDiskDefPtr disk;
  WANT_VALUE();
  if (!(disk = qemuParseCommandLineDisk(caps, val, nvirtiodisk)))
  goto error;
@@ -6408,6 +6412,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
  goto no_memory;
  }
  def->disks[def->ndisks++] = disk;
+disk = NULL;

  if (disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO)
  nvirtiodisk++;
@@ -6516,8 +6521,10 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
  if (VIR_ALLOC(chr)<  0)
  goto no_memory;

-if (qemuParseCommandLineChr(chr, val)<  0)
+if (qemuParseCommandLineChr(chr, val)<  0) {
+VIR_FREE(chr);



Shouldn't this be virDomainChrDefFree(chr)?



  goto error;
+}

  *monConfig = chr;
  }
@@ -6545,10 +6552,11 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
  char *hosts, *port, *saveptr = NULL, *token;
  virDomainDiskDefPtr first_rbd_disk = NULL;
  for (i = 0 ; i<  def->ndisks ; i++) {
-virDomainDiskDefPtr disk = def->disks[i];
+disk = def->disks[i];
  if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK&&

Re: [libvirt] [PATCH] qemu: avoid memory leaks

2011-08-02 Thread Eric Blake 

On 08/02/2011 02:48 PM, Laine Stump wrote:

On 08/02/2011 04:10 PM, Eric Blake wrote:

Quite a few leaks detected by coverity. For chr, the leaks were
close enough to the allocations to plug in place; for disk, the
leaks were separated from the allocation by enough other lines with
intermediate failure cases that I refactored the cleanup instead.


- if (qemuParseCommandLineChr(chr, val)< 0)
+ if (qemuParseCommandLineChr(chr, val)< 0) {
+ VIR_FREE(chr);



Shouldn't this be virDomainChrDefFree(chr)?





virDomainDiskDefPtr first_rbd_disk = NULL;
for (i = 0 ; i< def->ndisks ; i++) {
- virDomainDiskDefPtr disk = def->disks[i];
+ disk = def->disks[i];
if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK&&
disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) {
first_rbd_disk = disk;
+ disk = NULL;
break;
}



If you never hit the if condition, disk will be left pointing to one of
the disks in def, and will be freed during cleanup. I think you want to
set disk = NULL after this look is finished as well, don't you? (Either
that, or just use a different variable).


Both good points.  I'm squashing this in to create v2.

diff --git i/src/qemu/qemu_command.c w/src/qemu/qemu_command.c
index c638420..194f3ff 100644
--- i/src/qemu/qemu_command.c
+++ w/src/qemu/qemu_command.c
@@ -6522,7 +6522,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
 goto no_memory;

 if (qemuParseCommandLineChr(chr, val) < 0) {
-VIR_FREE(chr);
+virDomainChrSourceDefFree(chr);
 goto error;
 }

@@ -6552,11 +6552,9 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
 char *hosts, *port, *saveptr = NULL, *token;
 virDomainDiskDefPtr first_rbd_disk = NULL;
 for (i = 0 ; i < def->ndisks ; i++) {
-disk = def->disks[i];
-if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK &&
-disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) {
-first_rbd_disk = disk;
-disk = NULL;
+if (def->disks[i]->type == VIR_DOMAIN_DISK_TYPE_NETWORK &&
+def->disks[i]->protocol == 
VIR_DOMAIN_DISK_PROTOCOL_RBD) {

+first_rbd_disk = def->disks[i];
 break;
 }
 }


--
Eric Blake   ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: avoid memory leaks

2011-08-02 Thread Laine Stump 

On 08/02/2011 05:46 PM, Eric Blake wrote:

On 08/02/2011 02:48 PM, Laine Stump wrote:

On 08/02/2011 04:10 PM, Eric Blake wrote:

Quite a few leaks detected by coverity. For chr, the leaks were
close enough to the allocations to plug in place; for disk, the
leaks were separated from the allocation by enough other lines with
intermediate failure cases that I refactored the cleanup instead.


- if (qemuParseCommandLineChr(chr, val)< 0)
+ if (qemuParseCommandLineChr(chr, val)< 0) {
+ VIR_FREE(chr);



Shouldn't this be virDomainChrDefFree(chr)?





virDomainDiskDefPtr first_rbd_disk = NULL;
for (i = 0 ; i< def->ndisks ; i++) {
- virDomainDiskDefPtr disk = def->disks[i];
+ disk = def->disks[i];
if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK&&
disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) {
first_rbd_disk = disk;
+ disk = NULL;
break;
}



If you never hit the if condition, disk will be left pointing to one of
the disks in def, and will be freed during cleanup. I think you want to
set disk = NULL after this look is finished as well, don't you? (Either
that, or just use a different variable).


Both good points.  I'm squashing this in to create v2.

diff --git i/src/qemu/qemu_command.c w/src/qemu/qemu_command.c
index c638420..194f3ff 100644
--- i/src/qemu/qemu_command.c
+++ w/src/qemu/qemu_command.c
@@ -6522,7 +6522,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr 
caps,

 goto no_memory;

 if (qemuParseCommandLineChr(chr, val) < 0) {
-VIR_FREE(chr);
+virDomainChrSourceDefFree(chr);
 goto error;
 }

@@ -6552,11 +6552,9 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr 
caps,

 char *hosts, *port, *saveptr = NULL, *token;
 virDomainDiskDefPtr first_rbd_disk = NULL;
 for (i = 0 ; i < def->ndisks ; i++) {
-disk = def->disks[i];
-if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK &&
-disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) {
-first_rbd_disk = disk;
-disk = NULL;
+if (def->disks[i]->type == 
VIR_DOMAIN_DISK_TYPE_NETWORK &&
+def->disks[i]->protocol == 
VIR_DOMAIN_DISK_PROTOCOL_RBD) {

+first_rbd_disk = def->disks[i];
 break;
 }
 }




ACK with those changes.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] qemu: Avoid memory leaks on qemuParseRBDString

2012-01-05 Thread ajia 
From: Alex Jia 

Detected by valgrind. Leak introduced in commit 5745dc1.

* src/qemu/qemu_command.c: fix memory leak on failure and successful path.

* How to reproduce?
% valgrind -v --leak-check=full ./qemuargv2xmltest

* Actual result:

==2196== 80 bytes in 1 blocks are definitely lost in loss record 3 of 4
==2196==at 0x4A05FDE: malloc (vg_replace_malloc.c:236)
==2196==by 0x39CF07F6E1: strdup (in /lib64/libc-2.12.so)
==2196==by 0x419823: qemuParseRBDString (qemu_command.c:1657)
==2196==by 0x4221ED: qemuParseCommandLine (qemu_command.c:5934)
==2196==by 0x422AFB: qemuParseCommandLineString (qemu_command.c:7561)
==2196==by 0x416864: testCompareXMLToArgvHelper (qemuargv2xmltest.c:48)
==2196==by 0x417DB1: virtTestRun (testutils.c:141)
==2196==by 0x415CAF: mymain (qemuargv2xmltest.c:175)
==2196==by 0x4174A7: virtTestMain (testutils.c:696)
==2196==by 0x39CF01ECDC: (below main) (in /lib64/libc-2.12.so)
==2196==
==2196== LEAK SUMMARY:
==2196==definitely lost: 80 bytes in 1 blocks

Signed-off-by: Alex Jia 
---
 src/qemu/qemu_command.c |2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index ea1b763..69bf868 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1709,9 +1709,11 @@ static int qemuParseRBDString(virDomainDiskDefPtr disk)
 
 p = next;
 }
+VIR_FREE(options);
 return 0;
 
 no_memory:
+VIR_FREE(options);
 virReportOOMError();
 return -1;
 }
-- 
1.7.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: Avoid memory leaks on qemuParseRBDString

2012-01-05 Thread Osier Yang 

On 2012年01月06日 14:36, a...@redhat.com wrote:

From: Alex Jia

Detected by valgrind. Leak introduced in commit 5745dc1.

* src/qemu/qemu_command.c: fix memory leak on failure and successful path.

* How to reproduce?
% valgrind -v --leak-check=full ./qemuargv2xmltest

* Actual result:

==2196== 80 bytes in 1 blocks are definitely lost in loss record 3 of 4
==2196==at 0x4A05FDE: malloc (vg_replace_malloc.c:236)
==2196==by 0x39CF07F6E1: strdup (in /lib64/libc-2.12.so)
==2196==by 0x419823: qemuParseRBDString (qemu_command.c:1657)
==2196==by 0x4221ED: qemuParseCommandLine (qemu_command.c:5934)
==2196==by 0x422AFB: qemuParseCommandLineString (qemu_command.c:7561)
==2196==by 0x416864: testCompareXMLToArgvHelper (qemuargv2xmltest.c:48)
==2196==by 0x417DB1: virtTestRun (testutils.c:141)
==2196==by 0x415CAF: mymain (qemuargv2xmltest.c:175)
==2196==by 0x4174A7: virtTestMain (testutils.c:696)
==2196==by 0x39CF01ECDC: (below main) (in /lib64/libc-2.12.so)
==2196==
==2196== LEAK SUMMARY:
==2196==definitely lost: 80 bytes in 1 blocks

Signed-off-by: Alex Jia
---
  src/qemu/qemu_command.c |2 ++
  1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index ea1b763..69bf868 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1709,9 +1709,11 @@ static int qemuParseRBDString(virDomainDiskDefPtr disk)

  p = next;
  }
+VIR_FREE(options);
  return 0;

  no_memory:
+VIR_FREE(options);
  virReportOOMError();
  return -1;
  }


Trivial, ACK and Pushed.

Osier

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list