[libvirt] [PATCH 0/4] Fixes to some bugs encountered testing virInterface*
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.
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
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@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
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 :|
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
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 :|
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@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
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?
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 :|
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.
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 :|
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@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
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
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 :|
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@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
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).
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
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 :|
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@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Laine Stump