12:20 a.m.
This series of patches fixes some problems I found while testing the
virInterface implementation.
Note that the 3rd patch in the series only fixes virInterface-related
instances of the problem (releasing the conn lock before reporting
errors). If that patch is approved, a similar thing must be done for
several other functions in datatypes.c.
12:20 a.m.
New subject: [libvirt] [PATCH 1/4] rename variable called 'interface' to 'iface'
This avoids a problem when building under MINGW32
---
src/virsh.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/virsh.c b/src/virsh.c
index 91f5b5a..bae0c66 100644
--- a/src/virsh.c
+++ b/src/virsh.c
@@ -3307,7 +3307,7 @@ static const vshCmdOptDef opts_interface_define[] = {
static int
cmdInterfaceDefine(vshControl *ctl, const vshCmd *cmd)
{
- virInterfacePtr interface;
+ virInterfacePtr iface;
char *from;
int found;
int ret = TRUE;
@@ -3323,12 +3323,12 @@ cmdInterfaceDefine(vshControl *ctl, const vshCmd *cmd)
if (virFileReadAll(from, VIRSH_MAX_XML_FILE, &buffer) < 0)
return FALSE;
- interface = virInterfaceDefineXML(ctl->conn, buffer, 0);
+ iface = virInterfaceDefineXML(ctl->conn, buffer, 0);
free (buffer);
- if (interface != NULL) {
+ if (iface != NULL) {
vshPrint(ctl, _("Interface %s defined from %s\n"),
- virInterfaceGetName(interface), from);
+ virInterfaceGetName(iface), from);
} else {
vshError(ctl, FALSE, _("Failed to define interface from %s"), from);
ret = FALSE;
--
1.6.0.6
3:33 a.m.
New subject: [libvirt] [PATCH 1/4] rename variable called 'interface' to 'iface'
On Wed, Jul 22, 2009 at 01:20:56AM -0400, Laine Stump wrote:
This avoids a problem when building under MINGW32
Known issue, uncontroversial, applied, thanks !
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
4:40 a.m.
New subject: [libvirt] [PATCH 1/4] rename variable called 'interface' to 'iface'
On Wed, Jul 22, 2009 at 10:33:13AM +0200, Daniel Veillard wrote:
On Wed, Jul 22, 2009 at 01:20:56AM -0400, Laine Stump wrote:
> This avoids a problem when building under MINGW32
Known issue, uncontroversial, applied, thanks !
This problem is repeated throughout the interface_conf.c/.h files too....
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
12:20 a.m.
New subject: [libvirt] [PATCH 2/4] Update modified mac address in place in virGetInterface
If the mac address of an interface is changed, we must either create a
new virInterface object in the cache that has the new name/mac, or
modify the existing object in the cache. Because the cache is in a
hash that's indexed by name only, we can't create a new entry having
the same name without destroying the old one first (which isn't a
possibility as someone else is already referencing the old one), so
we're stuck modifying the existing entry. We have to do that without
changing the pointer to the mac though, so we really can only do it if
the length of the new mac is equal to or less than the old
mac. Otherwise, we have to just bail.
Fixing this problem to properly handle this corner case would require
a hash table that used both name and mac as keys, but that would be
inefficient (using the existing hash table implementation, which only
allows a single key), and probably unnecessary, as the length of mac
for any given interface will never change in practice.
---
src/datatypes.c | 33 ++++++++++++++++++++++++++++++++-
1 files changed, 32 insertions(+), 1 deletions(-)
diff --git a/src/datatypes.c b/src/datatypes.c
index 5f90aad..000fa66 100644
--- a/src/datatypes.c
+++ b/src/datatypes.c
@@ -535,7 +535,38 @@ virGetInterface(virConnectPtr conn, const char *name, const char
*mac) {
ret = (virInterfacePtr) virHashLookup(conn->interfaces, name);
- if ((ret == NULL) || STRCASENEQ(ret->mac, mac)) {
+ if (ret != NULL) {
+ if (STRCASENEQ(ret->mac, mac)) {
+ /*
+ * If the mac address has changed, try to modify it in
+ * place, which will only work if the new mac is the
+ * same length as, or shorter than, the old mac.
+ */
+ size_t newmaclen = strlen(mac);
+ size_t oldmaclen = strlen(ret->mac);
+ if (newmaclen <= oldmaclen) {
+ strcpy(ret->mac, mac);
+ } else {
+ /*
+ * If it's longer, we're kind of screwed, because we
+ * can't add a new hashtable entry (it would clash
+ * with the existing entry of same name), and we can't
+ * free/re-alloc the existing entry's mac, as some
+ * other thread may already be using the existing mac
+ * pointer. Fortunately, this should never happen,
+ * since the length of the mac address for any
+ * interface is determined by the type of the
+ * interface, and that is unlikely to change.
+ */
+ virMutexUnlock(&conn->lock);
+ virLibConnError(conn, VIR_ERR_INTERNAL_ERROR,
+ "Failed to change interface mac address from %s to
%s due to differing lengths.",
+ ret->mac, mac);
+ ret = NULL;
+ goto error;
+ }
+ }
+ } else {
if (VIR_ALLOC(ret) < 0) {
virReportOOMError(conn);
goto error;
--
1.6.0.6
6:22 a.m.
New subject: [libvirt] [PATCH 2/4] Update modified mac address in place in virGetInterface
On Wed, Jul 22, 2009 at 01:20:57AM -0400, Laine Stump wrote:
If the mac address of an interface is changed, we must either create
a
new virInterface object in the cache that has the new name/mac, or
modify the existing object in the cache. Because the cache is in a
hash that's indexed by name only, we can't create a new entry having
the same name without destroying the old one first (which isn't a
possibility as someone else is already referencing the old one), so
we're stuck modifying the existing entry. We have to do that without
changing the pointer to the mac though, so we really can only do it if
the length of the new mac is equal to or less than the old
mac. Otherwise, we have to just bail.
Fixing this problem to properly handle this corner case would require
a hash table that used both name and mac as keys, but that would be
inefficient (using the existing hash table implementation, which only
allows a single key), and probably unnecessary, as the length of mac
for any given interface will never change in practice.
Yep, the odds of this error case happening are sooooo small, that
giving a fatal error here is more than good enough.
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
9:20 a.m.
New subject: [libvirt] [PATCH 2/4] Update modified mac address in place in virGetInterface
On Wed, Jul 22, 2009 at 12:22:08PM +0100, Daniel P. Berrange wrote:
On Wed, Jul 22, 2009 at 01:20:57AM -0400, Laine Stump wrote:
> If the mac address of an interface is changed, we must either create a
> new virInterface object in the cache that has the new name/mac, or
> modify the existing object in the cache. Because the cache is in a
> hash that's indexed by name only, we can't create a new entry having
> the same name without destroying the old one first (which isn't a
> possibility as someone else is already referencing the old one), so
> we're stuck modifying the existing entry. We have to do that without
> changing the pointer to the mac though, so we really can only do it if
> the length of the new mac is equal to or less than the old
> mac. Otherwise, we have to just bail.
>
> Fixing this problem to properly handle this corner case would require
> a hash table that used both name and mac as keys, but that would be
> inefficient (using the existing hash table implementation, which only
> allows a single key), and probably unnecessary, as the length of mac
> for any given interface will never change in practice.
Yep, the odds of this error case happening are sooooo small, that
giving a fatal error here is more than good enough.
Agreed, patch applied, I only had to add _() to get the message
localized,
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
10:25 a.m.
New subject: [libvirt] [PATCH 2/4] Update modified mac address in place in virGetInterface
On 07/22/2009 10:20 AM, Daniel Veillard wrote:
Agreed, patch applied, I only had to add _() to get the message
localized,
I've idly wondered about that macro, as it causes scores of compile
warnings, like this:
datatypes.c:291: warning: format not a string literal and no format
arguments
What's it for? And what's the best way to change things to eliminate the
warnings?
10:36 a.m.
New subject: [libvirt] [PATCH 2/4] Update modified mac address in place in virGetInterface
On Wed, Jul 22, 2009 at 11:25:38AM -0400, Laine Stump wrote:
On 07/22/2009 10:20 AM, Daniel Veillard wrote:
>Agreed, patch applied, I only had to add _() to get the message localized,
I've idly wondered about that macro, as it causes scores of compile
warnings, like this:
datatypes.c:291: warning: format not a string literal and no format
arguments
What's it for? And what's the best way to change things to eliminate the
warnings?
It is a potential security hole, if the user finds a way to send a string
with an embeded format specifier. Thus if you're not doing any subsitutions,
and the string isn't constant, then you should always at least do
char *therealstring = ...from somewhere untrusted...
printf("%s", therealstring)
NB, anyone sending patches should always set
--enable-compile-warnings=error
when running 'autogen.sh' and make sure all warnings & errors are fixed
before submitting a patch.
Regards,
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
1:30 p.m.
New subject: [libvirt] [PATCH 2/4] Update modified mac address in place in virGetInterface
On 07/22/2009 11:36 AM, Daniel P. Berrange wrote:
On Wed, Jul 22, 2009 at 11:25:38AM -0400, Laine Stump wrote:
> On 07/22/2009 10:20 AM, Daniel Veillard wrote:
>
>> Agreed, patch applied, I only had to add _() to get the message localized,
>>
> I've idly wondered about that macro, as it causes scores of compile
> warnings, like this:
>
> datatypes.c:291: warning: format not a string literal and no format
> arguments
>
> What's it for? And what's the best way to change things to eliminate the
> warnings?
>
It is a potential security hole, if the user finds a way to send a string
with an embeded format specifier. Thus if you're not doing any subsitutions,
and the string isn't constant, then you should always at least do
char *therealstring = ...from somewhere untrusted...
printf("%s", therealstring)
Yeah, I understand the perils of using a non-literal string as the
format to *printf(). I'm wondering what the purpose of _() is, and why
it causes the compiler to believe the string isn't a literal. (I
received this warning when others don't because I use "-Wformat
-Wformat-security" in my CFLAGS, at Jim's suggestion).
NB, anyone sending patches should always set
--enable-compile-warnings=error
when running 'autogen.sh' and make sure all warnings & errors are fixed
before submitting a patch.
It's actually because I like doing this that I'd like to know the
preferred method of eliminating the warnings I mentioned. There are a
bunch of them pre-existing in the code that I want to get rid of so I
can turn on warnings=error (without turning off these warnings in
CFLAGS), and I want to do it the "accepted" way. For example, from
domain_conf.c:2137:
virDomainReportError(conn, VIR_ERR_XML_ERROR,
_("invalid security type"));
spits out the warning. We all know that it really *is* literal, but the
macro is changing the class so the compile thinks it isn't. It would be
simple to just change it to:
virDomainReportError(conn, VIR_ERR_XML_ERROR,
"%s", _("invalid security type"));
(and there are plenty of those too), but that's inefficient, and doesn't
do the _() around the "%s" (is that correct or not?).
If someone wants to tell me the preferred way of doing these, I'll
handle the grunt work of making the changes.
1:36 p.m.
New subject: [libvirt] [PATCH 2/4] Update modified mac address in place in virGetInterface
On Wed, Jul 22, 2009 at 02:30:55PM -0400, Laine Stump wrote:
On 07/22/2009 11:36 AM, Daniel P. Berrange wrote:
>On Wed, Jul 22, 2009 at 11:25:38AM -0400, Laine Stump wrote:
>
>>On 07/22/2009 10:20 AM, Daniel Veillard wrote:
>>
>>>Agreed, patch applied, I only had to add _() to get the message
>>>localized,
>>>
>>I've idly wondered about that macro, as it causes scores of compile
>>warnings, like this:
>>
>>datatypes.c:291: warning: format not a string literal and no format
>>arguments
>>
>>What's it for? And what's the best way to change things to eliminate the
>>warnings?
>>
>
>It is a potential security hole, if the user finds a way to send a string
>with an embeded format specifier. Thus if you're not doing any
>subsitutions,
>and the string isn't constant, then you should always at least do
>
> char *therealstring = ...from somewhere untrusted...
> printf("%s", therealstring)
>
Yeah, I understand the perils of using a non-literal string as the
format to *printf(). I'm wondering what the purpose of _() is, and why
it causes the compiler to believe the string isn't a literal. (I
received this warning when others don't because I use "-Wformat
-Wformat-security" in my CFLAGS, at Jim's suggestion).
Hmm, everyone gets -Wformat -Wformat-security added by default if
they run autogen.sh without the 'compile-warnings' flag.
I'd say its more likely that you are building without gettext
support, hence _() turns into a no-op, and thus lets the compiler
identify these format problems more reliably.
>
>NB, anyone sending patches should always set
>
> --enable-compile-warnings=error
>
>when running 'autogen.sh' and make sure all warnings & errors are fixed
>before submitting a patch.
>
It's actually because I like doing this that I'd like to know the
preferred method of eliminating the warnings I mentioned. There are a
bunch of them pre-existing in the code that I want to get rid of so I
can turn on warnings=error (without turning off these warnings in
CFLAGS), and I want to do it the "accepted" way. For example, from
domain_conf.c:2137:
virDomainReportError(conn, VIR_ERR_XML_ERROR,
_("invalid security type"));
spits out the warning. We all know that it really *is* literal, but the
macro is changing the class so the compile thinks it isn't. It would be
simple to just change it to:
virDomainReportError(conn, VIR_ERR_XML_ERROR,
"%s", _("invalid security type"));
(and there are plenty of those too), but that's inefficient, and doesn't
do the _() around the "%s" (is that correct or not?).
This is actually good, because it protects against a translator
introduceing a '%' sequence in the non-english text either delibrately
or accidentally.
Regards,
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
1:39 p.m.
New subject: [libvirt] [PATCH 2/4] Update modified mac address in place in virGetInterface
On Wed, Jul 22, 2009 at 02:30:55PM -0400, Laine Stump wrote:
On 07/22/2009 11:36 AM, Daniel P. Berrange wrote:
It's actually because I like doing this that I'd like to know the
preferred method of eliminating the warnings I mentioned. There are a
bunch of them pre-existing in the code that I want to get rid of so I
can turn on warnings=error (without turning off these warnings in
CFLAGS), and I want to do it the "accepted" way. For example, from
domain_conf.c:2137:
virDomainReportError(conn, VIR_ERR_XML_ERROR,
_("invalid security type"));
suppose one of the translators made a mistake and updated with a
wrong string from somewhere else replacing it with
"Erreur de securite %s"
for example due to a cut and paste mistake, it's better to see the
%s out than have the application crash, right ;-) ?
Of couse this can happen with formats embedding '%s' but that's just
another argument to use "%s" in my book.
spits out the warning. We all know that it really *is* literal, but
the
macro is changing the class so the compile thinks it isn't. It would be
simple to just change it to:
virDomainReportError(conn, VIR_ERR_XML_ERROR,
"%s", _("invalid security type"));
Feels safer to me, really.
(and there are plenty of those too), but that's inefficient, and
doesn't
do the _() around the "%s" (is that correct or not?).
If someone wants to tell me the preferred way of doing these, I'll
handle the grunt work of making the changes.
Thanks :-)
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
12:20 a.m.
New subject: [libvirt] [PATCH 3/4] Release conn lock before reporting errors in vir(Get|Release)Interface
Since virRaiseErrorFull needs to lock the conn, we must take care to
call it with the lock *not* held.
If this patch is approved, similar patches need to be done for other
types in datatypes.c
---
src/datatypes.c | 29 +++++++++++++++++++----------
1 files changed, 19 insertions(+), 10 deletions(-)
diff --git a/src/datatypes.c b/src/datatypes.c
index 000fa66..ba5401d 100644
--- a/src/datatypes.c
+++ b/src/datatypes.c
@@ -568,16 +568,19 @@ virGetInterface(virConnectPtr conn, const char *name, const char
*mac) {
}
} else {
if (VIR_ALLOC(ret) < 0) {
+ virMutexUnlock(&conn->lock);
virReportOOMError(conn);
goto error;
}
ret->name = strdup(name);
if (ret->name == NULL) {
+ virMutexUnlock(&conn->lock);
virReportOOMError(conn);
goto error;
}
ret->mac = strdup(mac);
if (ret->mac == NULL) {
+ virMutexUnlock(&conn->lock);
virReportOOMError(conn);
goto error;
}
@@ -586,6 +589,7 @@ virGetInterface(virConnectPtr conn, const char *name, const char *mac)
{
ret->conn = conn;
if (virHashAddEntry(conn->interfaces, name, ret) < 0) {
+ virMutexUnlock(&conn->lock);
virLibConnError(conn, VIR_ERR_INTERNAL_ERROR,
_("failed to add interface to connection hash
table"));
goto error;
@@ -597,7 +601,6 @@ virGetInterface(virConnectPtr conn, const char *name, const char *mac)
{
return(ret);
error:
- virMutexUnlock(&conn->lock);
if (ret != NULL) {
VIR_FREE(ret->name);
VIR_FREE(ret->mac);
@@ -623,24 +626,30 @@ virReleaseInterface(virInterfacePtr iface) {
virConnectPtr conn = iface->conn;
DEBUG("release interface %p %s", iface, iface->name);
- if (virHashRemoveEntry(conn->interfaces, iface->name, NULL) < 0)
+ if (virHashRemoveEntry(conn->interfaces, iface->name, NULL) < 0) {
+ /* unlock before reporting error because error report grabs lock */
+ virMutexUnlock(&conn->lock);
virLibConnError(conn, VIR_ERR_INTERNAL_ERROR,
_("interface missing from connection hash table"));
+ /* don't decr the conn refct if we weren't connected to it */
+ conn = NULL;
+ }
iface->magic = -1;
VIR_FREE(iface->name);
VIR_FREE(iface->mac);
VIR_FREE(iface);
- DEBUG("unref connection %p %d", conn, conn->refs);
- conn->refs--;
- if (conn->refs == 0) {
- virReleaseConnect(conn);
- /* Already unlocked mutex */
- return;
+ if (conn) {
+ DEBUG("unref connection %p %d", conn, conn->refs);
+ conn->refs--;
+ if (conn->refs == 0) {
+ virReleaseConnect(conn);
+ /* Already unlocked mutex */
+ return;
+ }
+ virMutexUnlock(&conn->lock);
}
-
- virMutexUnlock(&conn->lock);
}
--
1.6.0.6
6:23 a.m.
New subject: [libvirt] [PATCH 3/4] Release conn lock before reporting errors in vir(Get|Release)Interface
On Wed, Jul 22, 2009 at 01:20:58AM -0400, Laine Stump wrote:
Since virRaiseErrorFull needs to lock the conn, we must take care to
call it with the lock *not* held.
If this patch is approved, similar patches need to be done for other
types in datatypes.c
Doh, that's a annoying edge case ! ACK
If you have time to do the same change to other methods in this
file that'd be very helpful
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
9:21 a.m.
New subject: [libvirt] [PATCH 3/4] Release conn lock before reporting errors in vir(Get|Release)Interface
On Wed, Jul 22, 2009 at 12:23:31PM +0100, Daniel P. Berrange wrote:
On Wed, Jul 22, 2009 at 01:20:58AM -0400, Laine Stump wrote:
> Since virRaiseErrorFull needs to lock the conn, we must take care to
> call it with the lock *not* held.
>
> If this patch is approved, similar patches need to be done for other
> types in datatypes.c
Doh, that's a annoying edge case ! ACK
I wonder why we didn't see deadlocks so far ...
If you have time to do the same change to other methods in this
file that'd be very helpful
I pushed the patch for the Interface, Laine do you think you will
fix the others or should I do it ?
thanks,
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
10:29 a.m.
New subject: [libvirt] [PATCH 3/4] Release conn lock before reporting errors in vir(Get|Release)Interface
On 07/22/2009 10:21 AM, Daniel Veillard wrote:
On Wed, Jul 22, 2009 at 12:23:31PM +0100, Daniel P. Berrange wrote:
> On Wed, Jul 22, 2009 at 01:20:58AM -0400, Laine Stump wrote:
>
>> Since virRaiseErrorFull needs to lock the conn, we must take care to
>> call it with the lock *not* held.
>>
>> If this patch is approved, similar patches need to be done for other
>> types in datatypes.c
>>
> Doh, that's a annoying edge case ! ACK
>
I wonder why we didn't see deadlocks so far ...
Because you didn't write any bugridden code to inadvertantly exercise it
like I did! ;-)
> If you have time to do the same change to other methods in this
> file that'd be very helpful
>
I pushed the patch for the Interface, Laine do you think you will
fix the others or should I do it ?
I just submitted a patch for the rest (in datatypes.c anyway).
12:20 a.m.
New subject: [libvirt] [PATCH 4/4] Fix multiple vir*Ptr memory leaks in virsh.c
I noticed that one of my virInterface commandline functions in virsh.c
was forgetting to call virInterfaceFree() before it was done, and
since I had written it with copy-paste, I decided to check other
functions in virsh.c for the same problem. Unless I misunderstand the
APIs, I found a whole bunch of similar leaks of virDomain,
virStoragePool, virNodeDevice, etc.
---
src/virsh.c | 32 +++++++++++++++++++++++++++-----
1 files changed, 27 insertions(+), 5 deletions(-)
diff --git a/src/virsh.c b/src/virsh.c
index bae0c66..fff73a1 100644
--- a/src/virsh.c
+++ b/src/virsh.c
@@ -784,8 +784,10 @@ cmdDomblkstat (vshControl *ctl, const vshCmd *cmd)
if (!(dom = vshCommandOptDomain (ctl, cmd, &name)))
return FALSE;
- if (!(device = vshCommandOptString (cmd, "device", NULL)))
+ if (!(device = vshCommandOptString (cmd, "device", NULL))) {
+ virDomainFree(dom);
return FALSE;
+ }
if (virDomainBlockStats (dom, device, &stats, sizeof stats) == -1) {
vshError (ctl, FALSE, _("Failed to get block stats %s %s"),
@@ -840,8 +842,10 @@ cmdDomIfstat (vshControl *ctl, const vshCmd *cmd)
if (!(dom = vshCommandOptDomain (ctl, cmd, &name)))
return FALSE;
- if (!(device = vshCommandOptString (cmd, "interface", NULL)))
+ if (!(device = vshCommandOptString (cmd, "interface", NULL))) {
+ virDomainFree(dom);
return FALSE;
+ }
if (virDomainInterfaceStats (dom, device, &stats, sizeof stats) == -1) {
vshError (ctl, FALSE, _("Failed to get interface stats %s %s"),
@@ -2528,6 +2532,7 @@ cmdNetworkAutostart(vshControl *ctl, const vshCmd *cmd)
else
vshPrint(ctl, _("Network %s unmarked as autostarted\n"), name);
+ virNetworkFree(network);
return TRUE;
}
@@ -2570,6 +2575,7 @@ cmdNetworkCreate(vshControl *ctl, const vshCmd *cmd)
if (network != NULL) {
vshPrint(ctl, _("Network %s created from %s\n"),
virNetworkGetName(network), from);
+ virNetworkFree(network);
} else {
vshError(ctl, FALSE, _("Failed to create network from %s"), from);
ret = FALSE;
@@ -2617,6 +2623,7 @@ cmdNetworkDefine(vshControl *ctl, const vshCmd *cmd)
if (network != NULL) {
vshPrint(ctl, _("Network %s defined from %s\n"),
virNetworkGetName(network), from);
+ virNetworkFree(network);
} else {
vshError(ctl, FALSE, _("Failed to define network from %s"), from);
ret = FALSE;
@@ -2997,6 +3004,7 @@ cmdNetworkStart(vshControl *ctl, const vshCmd *cmd)
virNetworkGetName(network));
ret = FALSE;
}
+ virNetworkFree(network);
return ret;
}
@@ -3035,6 +3043,7 @@ cmdNetworkUndefine(vshControl *ctl, const vshCmd *cmd)
ret = FALSE;
}
+ virNetworkFree(network);
return ret;
}
@@ -3071,6 +3080,7 @@ cmdNetworkUuid(vshControl *ctl, const vshCmd *cmd)
else
vshError(ctl, FALSE, "%s", _("failed to get network UUID"));
+ virNetworkFree(network);
return TRUE;
}
@@ -3329,6 +3339,7 @@ cmdInterfaceDefine(vshControl *ctl, const vshCmd *cmd)
if (iface != NULL) {
vshPrint(ctl, _("Interface %s defined from %s\n"),
virInterfaceGetName(iface), from);
+ virInterfaceFree (iface);
} else {
vshError(ctl, FALSE, _("Failed to define interface from %s"), from);
ret = FALSE;
@@ -3498,6 +3509,7 @@ cmdPoolAutostart(vshControl *ctl, const vshCmd *cmd)
else
vshPrint(ctl, _("Pool %s unmarked as autostarted\n"), name);
+ virStoragePoolFree(pool);
return TRUE;
}
@@ -3541,6 +3553,7 @@ cmdPoolCreate(vshControl *ctl, const vshCmd *cmd)
if (pool != NULL) {
vshPrint(ctl, _("Pool %s created from %s\n"),
virStoragePoolGetName(pool), from);
+ virStoragePoolFree(pool);
} else {
vshError(ctl, FALSE, _("Failed to create pool from %s"), from);
ret = FALSE;
@@ -3594,6 +3607,7 @@ cmdNodeDeviceCreate(vshControl *ctl, const vshCmd *cmd)
if (dev != NULL) {
vshPrint(ctl, _("Node device %s created from %s\n"),
virNodeDeviceGetName(dev), from);
+ virNodeDeviceFree(dev);
} else {
vshError(ctl, FALSE, _("Failed to create node device from %s"), from);
ret = FALSE;
@@ -3801,6 +3815,7 @@ cmdPoolDefine(vshControl *ctl, const vshCmd *cmd)
if (pool != NULL) {
vshPrint(ctl, _("Pool %s defined from %s\n"),
virStoragePoolGetName(pool), from);
+ virStoragePoolFree(pool);
} else {
vshError(ctl, FALSE, _("Failed to define pool from %s"), from);
ret = FALSE;
@@ -3960,9 +3975,9 @@ cmdPoolDelete(vshControl *ctl, const vshCmd *cmd)
} else {
vshError(ctl, FALSE, _("Failed to delete pool %s"), name);
ret = FALSE;
- virStoragePoolFree(pool);
}
+ virStoragePoolFree(pool);
return ret;
}
@@ -4460,6 +4475,8 @@ cmdPoolStart(vshControl *ctl, const vshCmd *cmd)
virStoragePoolGetName(pool));
ret = FALSE;
}
+
+ virStoragePoolFree(pool);
return ret;
}
@@ -4619,6 +4636,7 @@ cmdPoolUndefine(vshControl *ctl, const vshCmd *cmd)
ret = FALSE;
}
+ virStoragePoolFree(pool);
return ret;
}
@@ -4655,6 +4673,7 @@ cmdPoolUuid(vshControl *ctl, const vshCmd *cmd)
else
vshError(ctl, FALSE, "%s", _("failed to get pool UUID"));
+ virStoragePoolFree(pool);
return TRUE;
}
@@ -4779,6 +4798,8 @@ cleanup:
virStoragePoolFree(pool);
if (inputvol)
virStorageVolFree(inputvol);
+ if (newvol)
+ virStorageVolFree(newvol);
return ret;
}
@@ -4871,7 +4892,6 @@ cmdVolClone(vshControl *ctl, const vshCmd *cmd)
if (newvol != NULL) {
vshPrint(ctl, _("Vol %s cloned from %s\n"),
virStorageVolGetName(newvol), virStorageVolGetName(origvol));
- virStorageVolFree(newvol);
} else {
vshError(ctl, FALSE, _("Failed to clone vol from %s"),
virStorageVolGetName(origvol));
@@ -4885,6 +4905,8 @@ cleanup:
xmlFree(newxml);
if (origvol)
virStorageVolFree(origvol);
+ if (newvol)
+ virStorageVolFree(newvol);
if (origpool)
virStoragePoolFree(origpool);
return ret;
@@ -4924,9 +4946,9 @@ cmdVolDelete(vshControl *ctl, const vshCmd *cmd)
} else {
vshError(ctl, FALSE, _("Failed to delete vol %s"), name);
ret = FALSE;
- virStorageVolFree(vol);
}
+ virStorageVolFree(vol);
return ret;
}
--
1.6.0.6
6:24 a.m.
New subject: [libvirt] [PATCH 4/4] Fix multiple vir*Ptr memory leaks in virsh.c
On Wed, Jul 22, 2009 at 01:20:59AM -0400, Laine Stump wrote:
I noticed that one of my virInterface commandline functions in
virsh.c
was forgetting to call virInterfaceFree() before it was done, and
since I had written it with copy-paste, I decided to check other
functions in virsh.c for the same problem. Unless I misunderstand the
APIs, I found a whole bunch of similar leaks of virDomain,
virStoragePool, virNodeDevice, etc.
---
src/virsh.c | 32 +++++++++++++++++++++++++++-----
1 files changed, 27 insertions(+), 5 deletions(-)
ACk
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
8:53 a.m.
New subject: [libvirt] [PATCH 4/4] Fix multiple vir*Ptr memory leaks in virsh.c
On Wed, Jul 22, 2009 at 01:20:59AM -0400, Laine Stump wrote:
I noticed that one of my virInterface commandline functions in
virsh.c
was forgetting to call virInterfaceFree() before it was done, and
since I had written it with copy-paste, I decided to check other
functions in virsh.c for the same problem. Unless I misunderstand the
APIs, I found a whole bunch of similar leaks of virDomain,
virStoragePool, virNodeDevice, etc.
Great, pushed, thanks !
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
5602
days inactive
5602
days old
18 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Laine Stump