[libvirt] [PATCH] Fix several memory leaks
Hi, This patch fixes several memory leaks being scattered over libvirt. Thanks, ozaki-r
From 6fc282eae5192cccda208d8d4fd14c5c0676992b Mon Sep 17 00:00:00 2001 From: Ryota Ozaki <ozaki.ryota@gmail.com> Date: Tue, 1 Sep 2009 01:34:40 +0900 Subject: [PATCH] Fix several memory leaks
--- qemud/qemud.c | 2 +- src/domain_conf.c | 1 + src/network_conf.c | 5 +++-- src/qemu_conf.c | 13 ++++++++----- src/storage_backend_fs.c | 5 +++++ 5 files changed, 18 insertions(+), 8 deletions(-) diff --git a/qemud/qemud.c b/qemud/qemud.c index df275e6..17ba44a 100644 --- a/qemud/qemud.c +++ b/qemud/qemud.c @@ -1733,7 +1733,7 @@ readmore: /* Possibly need to create another receive buffer */ if ((client->nrequests < max_client_requests && - VIR_ALLOC(client->rx) < 0)) { + !client->rx && VIR_ALLOC(client->rx) < 0)) { qemudDispatchClientFailure(client); } else { if (client->rx) diff --git a/src/domain_conf.c b/src/domain_conf.c index 1d2cc7c..4b64219 100644 --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -4361,6 +4361,7 @@ int virDomainSaveXML(virConnectPtr conn, cleanup: if (fd != -1) close(fd); + VIR_FREE(configFile); return ret; } diff --git a/src/network_conf.c b/src/network_conf.c index bb649a4..58a4f32 100644 --- a/src/network_conf.c +++ b/src/network_conf.c @@ -820,6 +820,7 @@ int virNetworkDeleteConfig(virConnectPtr conn, { char *configFile = NULL; char *autostartLink = NULL; + int ret = -1; if ((configFile = virNetworkConfigFile(conn, configDir, net->def->name)) == NULL) goto error; @@ -836,12 +837,12 @@ int virNetworkDeleteConfig(virConnectPtr conn, goto error; } - return 0; + ret = 0; error: VIR_FREE(configFile); VIR_FREE(autostartLink); - return -1; + return ret; } char *virNetworkConfigFile(virConnectPtr conn, diff --git a/src/qemu_conf.c b/src/qemu_conf.c index 22f5edd..32d6a48 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -1034,7 +1034,7 @@ qemudNetworkIfaceConnect(virConnectPtr conn, virDomainNetDefPtr net, int qemuCmdFlags) { - char *brname; + char *brname = NULL; int err; int tapfd = -1; int vnet_hdr = 0; @@ -1053,7 +1053,7 @@ qemudNetworkIfaceConnect(virConnectPtr conn, if (brname == NULL) return -1; } else if (net->type == VIR_DOMAIN_NET_TYPE_BRIDGE) { - brname = net->data.bridge.brname; + brname = strdup(net->data.bridge.brname); } else { qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _("Network type %d is not supported"), net->type); @@ -1063,7 +1063,7 @@ qemudNetworkIfaceConnect(virConnectPtr conn, if (!driver->brctl && (err = brInit(&driver->brctl))) { virReportSystemError(conn, err, "%s", _("cannot initialize bridge support")); - return -1; + goto cleanup; } if (!net->ifname || @@ -1072,7 +1072,7 @@ qemudNetworkIfaceConnect(virConnectPtr conn, VIR_FREE(net->ifname); if (!(net->ifname = strdup("vnet%d"))) { virReportOOMError(conn); - return -1; + goto cleanup; } /* avoid exposing vnet%d in dumpxml or error outputs */ template_ifname = 1; @@ -1100,9 +1100,12 @@ qemudNetworkIfaceConnect(virConnectPtr conn, } if (template_ifname) VIR_FREE(net->ifname); - return -1; + tapfd = -1; } +cleanup: + VIR_FREE(brname); + return tapfd; } diff --git a/src/storage_backend_fs.c b/src/storage_backend_fs.c index ca6d329..222e591 100644 --- a/src/storage_backend_fs.c +++ b/src/storage_backend_fs.c @@ -856,6 +856,8 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn, vol->type = VIR_STORAGE_VOL_FILE; vol->target.format = VIR_STORAGE_VOL_FILE_RAW; /* Real value is filled in during probe */ + if (vol->target.path) + VIR_FREE(vol->target.path); if (virAsprintf(&vol->target.path, "%s/%s", pool->def->target.path, vol->name) == -1) @@ -1022,6 +1024,9 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn, vol->type = VIR_STORAGE_VOL_FILE; + if (vol->target.path) + VIR_FREE(vol->target.path); + if (virAsprintf(&vol->target.path, "%s/%s", pool->def->target.path, vol->name) == -1) { -- 1.6.0.6
Ryota Ozaki wrote:
diff --git a/qemud/qemud.c b/qemud/qemud.c index df275e6..17ba44a 100644 --- a/qemud/qemud.c +++ b/qemud/qemud.c @@ -1733,7 +1733,7 @@ readmore:
/* Possibly need to create another receive buffer */ if ((client->nrequests < max_client_requests && - VIR_ALLOC(client->rx) < 0)) { + !client->rx && VIR_ALLOC(client->rx) < 0)) { qemudDispatchClientFailure(client); } else { if (client->rx)
Hm, I'm not super familiar with this section of code, but this doesn't seem to be right. How can client->rx ever be NULL here? We dereference client->rx->bufferOffset very early on in the function, so we would have crashed before we got here. What am I missing?
diff --git a/src/domain_conf.c b/src/domain_conf.c index 1d2cc7c..4b64219 100644 --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -4361,6 +4361,7 @@ int virDomainSaveXML(virConnectPtr conn, cleanup: if (fd != -1) close(fd); + VIR_FREE(configFile); return ret; }
This one looks good.
diff --git a/src/network_conf.c b/src/network_conf.c index bb649a4..58a4f32 100644 --- a/src/network_conf.c +++ b/src/network_conf.c @@ -820,6 +820,7 @@ int virNetworkDeleteConfig(virConnectPtr conn, { char *configFile = NULL; char *autostartLink = NULL; + int ret = -1;
if ((configFile = virNetworkConfigFile(conn, configDir, net->def->name)) == NULL) goto error; @@ -836,12 +837,12 @@ int virNetworkDeleteConfig(virConnectPtr conn, goto error; }
- return 0; + ret = 0;
error: VIR_FREE(configFile); VIR_FREE(autostartLink); - return -1; + return ret; }
This one looks good.
char *virNetworkConfigFile(virConnectPtr conn, diff --git a/src/qemu_conf.c b/src/qemu_conf.c index 22f5edd..32d6a48 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -1034,7 +1034,7 @@ qemudNetworkIfaceConnect(virConnectPtr conn, virDomainNetDefPtr net, int qemuCmdFlags) { - char *brname; + char *brname = NULL; int err; int tapfd = -1; int vnet_hdr = 0; @@ -1053,7 +1053,7 @@ qemudNetworkIfaceConnect(virConnectPtr conn, if (brname == NULL) return -1; } else if (net->type == VIR_DOMAIN_NET_TYPE_BRIDGE) { - brname = net->data.bridge.brname; + brname = strdup(net->data.bridge.brname); } else { qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _("Network type %d is not supported"), net->type); @@ -1063,7 +1063,7 @@ qemudNetworkIfaceConnect(virConnectPtr conn, if (!driver->brctl && (err = brInit(&driver->brctl))) { virReportSystemError(conn, err, "%s", _("cannot initialize bridge support")); - return -1; + goto cleanup; }
if (!net->ifname || @@ -1072,7 +1072,7 @@ qemudNetworkIfaceConnect(virConnectPtr conn, VIR_FREE(net->ifname); if (!(net->ifname = strdup("vnet%d"))) { virReportOOMError(conn); - return -1; + goto cleanup; } /* avoid exposing vnet%d in dumpxml or error outputs */ template_ifname = 1; @@ -1100,9 +1100,12 @@ qemudNetworkIfaceConnect(virConnectPtr conn, } if (template_ifname) VIR_FREE(net->ifname); - return -1; + tapfd = -1; }
+cleanup: + VIR_FREE(brname); + return tapfd; }
This generally looks OK.
diff --git a/src/storage_backend_fs.c b/src/storage_backend_fs.c index ca6d329..222e591 100644 --- a/src/storage_backend_fs.c +++ b/src/storage_backend_fs.c @@ -856,6 +856,8 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn,
vol->type = VIR_STORAGE_VOL_FILE; vol->target.format = VIR_STORAGE_VOL_FILE_RAW; /* Real value is filled in during probe */ + if (vol->target.path) + VIR_FREE(vol->target.path);
Again, how can this ever be the case? The first time through the loop vol is definitely NULL, so there's no possibility vol->target.path contains any allocated memory. At the end of this loop, we assign the whole vol object to a slot in the pool->volumes array, set vol to NULL, and then subsequent iterations get a new vol object allocated. So how can this ever be true?
if (virAsprintf(&vol->target.path, "%s/%s", pool->def->target.path, vol->name) == -1) @@ -1022,6 +1024,9 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn,
vol->type = VIR_STORAGE_VOL_FILE;
+ if (vol->target.path) + VIR_FREE(vol->target.path); + if (virAsprintf(&vol->target.path, "%s/%s", pool->def->target.path, vol->name) == -1) {
I'm not sure either way on this one, I didn't follow the code path back far enough. What situations can lead to this? -- Chris Lalancette
Hi Chris, On Tue, Sep 1, 2009 at 4:45 AM, Chris Lalancette<clalance@redhat.com> wrote:
Ryota Ozaki wrote:
diff --git a/qemud/qemud.c b/qemud/qemud.c index df275e6..17ba44a 100644 --- a/qemud/qemud.c +++ b/qemud/qemud.c @@ -1733,7 +1733,7 @@ readmore:
/* Possibly need to create another receive buffer */ if ((client->nrequests < max_client_requests && - VIR_ALLOC(client->rx) < 0)) { + !client->rx && VIR_ALLOC(client->rx) < 0)) { qemudDispatchClientFailure(client); } else { if (client->rx)
Hm, I'm not super familiar with this section of code, but this doesn't seem to be right. How can client->rx ever be NULL here? We dereference client->rx->bufferOffset very early on in the function, so we would have crashed before we got here. What am I missing?
No, you are probably right. As Daniel also says, the fix seems wrong. I actually used valgrind to find memory leaks and valgrind pointed out here. However, now the leak cannot be reproduced anymore, so probably I missed something... I'll drop this fix in the next patch.
diff --git a/src/domain_conf.c b/src/domain_conf.c index 1d2cc7c..4b64219 100644 --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -4361,6 +4361,7 @@ int virDomainSaveXML(virConnectPtr conn, cleanup: if (fd != -1) close(fd); + VIR_FREE(configFile); return ret; }
This one looks good.
diff --git a/src/network_conf.c b/src/network_conf.c index bb649a4..58a4f32 100644 --- a/src/network_conf.c +++ b/src/network_conf.c @@ -820,6 +820,7 @@ int virNetworkDeleteConfig(virConnectPtr conn, { char *configFile = NULL; char *autostartLink = NULL; + int ret = -1;
if ((configFile = virNetworkConfigFile(conn, configDir, net->def->name)) == NULL) goto error; @@ -836,12 +837,12 @@ int virNetworkDeleteConfig(virConnectPtr conn, goto error; }
- return 0; + ret = 0;
error: VIR_FREE(configFile); VIR_FREE(autostartLink); - return -1; + return ret; }
This one looks good.
char *virNetworkConfigFile(virConnectPtr conn, diff --git a/src/qemu_conf.c b/src/qemu_conf.c index 22f5edd..32d6a48 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -1034,7 +1034,7 @@ qemudNetworkIfaceConnect(virConnectPtr conn, virDomainNetDefPtr net, int qemuCmdFlags) { - char *brname; + char *brname = NULL; int err; int tapfd = -1; int vnet_hdr = 0; @@ -1053,7 +1053,7 @@ qemudNetworkIfaceConnect(virConnectPtr conn, if (brname == NULL) return -1; } else if (net->type == VIR_DOMAIN_NET_TYPE_BRIDGE) { - brname = net->data.bridge.brname; + brname = strdup(net->data.bridge.brname); } else { qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _("Network type %d is not supported"), net->type); @@ -1063,7 +1063,7 @@ qemudNetworkIfaceConnect(virConnectPtr conn, if (!driver->brctl && (err = brInit(&driver->brctl))) { virReportSystemError(conn, err, "%s", _("cannot initialize bridge support")); - return -1; + goto cleanup; }
if (!net->ifname || @@ -1072,7 +1072,7 @@ qemudNetworkIfaceConnect(virConnectPtr conn, VIR_FREE(net->ifname); if (!(net->ifname = strdup("vnet%d"))) { virReportOOMError(conn); - return -1; + goto cleanup; } /* avoid exposing vnet%d in dumpxml or error outputs */ template_ifname = 1; @@ -1100,9 +1100,12 @@ qemudNetworkIfaceConnect(virConnectPtr conn, } if (template_ifname) VIR_FREE(net->ifname); - return -1; + tapfd = -1; }
+cleanup: + VIR_FREE(brname); + return tapfd; }
This generally looks OK.
diff --git a/src/storage_backend_fs.c b/src/storage_backend_fs.c index ca6d329..222e591 100644 --- a/src/storage_backend_fs.c +++ b/src/storage_backend_fs.c @@ -856,6 +856,8 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn,
vol->type = VIR_STORAGE_VOL_FILE; vol->target.format = VIR_STORAGE_VOL_FILE_RAW; /* Real value is filled in during probe */ + if (vol->target.path) + VIR_FREE(vol->target.path);
Again, how can this ever be the case? The first time through the loop vol is definitely NULL, so there's no possibility vol->target.path contains any allocated memory. At the end of this loop, we assign the whole vol object to a slot in the pool->volumes array, set vol to NULL, and then subsequent iterations get a new vol object allocated. So how can this ever be true?
I'm wrong again... I was too easy to add the fix along with the below fix. I'll drop it as well.
if (virAsprintf(&vol->target.path, "%s/%s", pool->def->target.path, vol->name) == -1) @@ -1022,6 +1024,9 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn,
vol->type = VIR_STORAGE_VOL_FILE;
+ if (vol->target.path) + VIR_FREE(vol->target.path); + if (virAsprintf(&vol->target.path, "%s/%s", pool->def->target.path, vol->name) == -1) {
I'm not sure either way on this one, I didn't follow the code path back far enough. What situations can lead to this?
This one will happen because here comes soon after virStorageVolDefParseString and it (actually virStorageVolDefParseXML) always sets vol->target.path. Thanks for your review! ozaki-r
-- Chris Lalancette
On Tue, Sep 01, 2009 at 01:50:50AM +0900, Ryota Ozaki wrote:
index df275e6..17ba44a 100644 --- a/qemud/qemud.c +++ b/qemud/qemud.c @@ -1733,7 +1733,7 @@ readmore:
/* Possibly need to create another receive buffer */ if ((client->nrequests < max_client_requests && - VIR_ALLOC(client->rx) < 0)) { + !client->rx && VIR_ALLOC(client->rx) < 0)) { qemudDispatchClientFailure(client); } else { if (client->rx)
This is not required. Although client->rx looks like a list, it will only ever have a single entry in it. Since we have just called qemudClientMessageQueueServe() a few lines earlier, we know that client->rx is NULL at this point. So there is no leak by always calling VIR_ALLOC(client->rx);
diff --git a/src/domain_conf.c b/src/domain_conf.c index 1d2cc7c..4b64219 100644 --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -4361,6 +4361,7 @@ int virDomainSaveXML(virConnectPtr conn, cleanup: if (fd != -1) close(fd); + VIR_FREE(configFile); return ret; }
ACK
diff --git a/src/network_conf.c b/src/network_conf.c index bb649a4..58a4f32 100644 --- a/src/network_conf.c +++ b/src/network_conf.c @@ -820,6 +820,7 @@ int virNetworkDeleteConfig(virConnectPtr conn, { char *configFile = NULL; char *autostartLink = NULL; + int ret = -1;
if ((configFile = virNetworkConfigFile(conn, configDir, net->def->name)) == NULL) goto error; @@ -836,12 +837,12 @@ int virNetworkDeleteConfig(virConnectPtr conn, goto error; }
- return 0; + ret = 0;
error: VIR_FREE(configFile); VIR_FREE(autostartLink); - return -1; + return ret; }
ACK
char *virNetworkConfigFile(virConnectPtr conn, diff --git a/src/qemu_conf.c b/src/qemu_conf.c index 22f5edd..32d6a48 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -1034,7 +1034,7 @@ qemudNetworkIfaceConnect(virConnectPtr conn, virDomainNetDefPtr net, int qemuCmdFlags) { - char *brname; + char *brname = NULL; int err; int tapfd = -1; int vnet_hdr = 0; @@ -1053,7 +1053,7 @@ qemudNetworkIfaceConnect(virConnectPtr conn, if (brname == NULL) return -1; } else if (net->type == VIR_DOMAIN_NET_TYPE_BRIDGE) { - brname = net->data.bridge.brname; + brname = strdup(net->data.bridge.brname); } else { qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _("Network type %d is not supported"), net->type); @@ -1063,7 +1063,7 @@ qemudNetworkIfaceConnect(virConnectPtr conn, if (!driver->brctl && (err = brInit(&driver->brctl))) { virReportSystemError(conn, err, "%s", _("cannot initialize bridge support")); - return -1; + goto cleanup; }
if (!net->ifname || @@ -1072,7 +1072,7 @@ qemudNetworkIfaceConnect(virConnectPtr conn, VIR_FREE(net->ifname); if (!(net->ifname = strdup("vnet%d"))) { virReportOOMError(conn); - return -1; + goto cleanup; } /* avoid exposing vnet%d in dumpxml or error outputs */ template_ifname = 1; @@ -1100,9 +1100,12 @@ qemudNetworkIfaceConnect(virConnectPtr conn, } if (template_ifname) VIR_FREE(net->ifname); - return -1; + tapfd = -1; }
+cleanup: + VIR_FREE(brname); + return tapfd; }
ACK
diff --git a/src/storage_backend_fs.c b/src/storage_backend_fs.c index ca6d329..222e591 100644 --- a/src/storage_backend_fs.c +++ b/src/storage_backend_fs.c @@ -856,6 +856,8 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn,
vol->type = VIR_STORAGE_VOL_FILE; vol->target.format = VIR_STORAGE_VOL_FILE_RAW; /* Real value is filled in during probe */ + if (vol->target.path) + VIR_FREE(vol->target.path); if (virAsprintf(&vol->target.path, "%s/%s", pool->def->target.path, vol->name) == -1)
This doesn't make any sense - we just allocated 'vol' a few lines earlier, so it can't possibly be non-NULL.
@@ -1022,6 +1024,9 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn,
vol->type = VIR_STORAGE_VOL_FILE;
+ if (vol->target.path) + VIR_FREE(vol->target.path); + if (virAsprintf(&vol->target.path, "%s/%s", pool->def->target.path, vol->name) == -1) {
THis one is possibly correct, though we would need the same for vol->key too. Also there is no need to have 'if (xx)' before a call to VIR_FREE - it accepts NULLs happily. 'make syntax-check' would complain about this additional 'if'. 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 :|
Hi, On Tue, Sep 1, 2009 at 7:49 PM, Daniel P. Berrange<berrange@redhat.com> wrote:
On Tue, Sep 01, 2009 at 01:50:50AM +0900, Ryota Ozaki wrote:
index df275e6..17ba44a 100644 --- a/qemud/qemud.c +++ b/qemud/qemud.c @@ -1733,7 +1733,7 @@ readmore:
/* Possibly need to create another receive buffer */ if ((client->nrequests < max_client_requests && - VIR_ALLOC(client->rx) < 0)) { + !client->rx && VIR_ALLOC(client->rx) < 0)) { qemudDispatchClientFailure(client); } else { if (client->rx)
This is not required. Although client->rx looks like a list, it will only ever have a single entry in it. Since we have just called qemudClientMessageQueueServe() a few lines earlier, we know that client->rx is NULL at this point. So there is no leak by always calling VIR_ALLOC(client->rx);
As I said in the previous mail, this fix seems wrong because I cannot reproduce the leak. I'll drop it. (snip)
diff --git a/src/storage_backend_fs.c b/src/storage_backend_fs.c index ca6d329..222e591 100644 --- a/src/storage_backend_fs.c +++ b/src/storage_backend_fs.c @@ -856,6 +856,8 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn,
vol->type = VIR_STORAGE_VOL_FILE; vol->target.format = VIR_STORAGE_VOL_FILE_RAW; /* Real value is filled in during probe */ + if (vol->target.path) + VIR_FREE(vol->target.path); if (virAsprintf(&vol->target.path, "%s/%s", pool->def->target.path, vol->name) == -1)
This doesn't make any sense - we just allocated 'vol' a few lines earlier, so it can't possibly be non-NULL.
Yes, I'm wrong and will drop it.
@@ -1022,6 +1024,9 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn,
vol->type = VIR_STORAGE_VOL_FILE;
+ if (vol->target.path) + VIR_FREE(vol->target.path); + if (virAsprintf(&vol->target.path, "%s/%s", pool->def->target.path, vol->name) == -1) {
THis one is possibly correct, though we would need the same for vol->key too.
Hmm, valgrind didn't pointed out about vol->key. vol->key is set only on refreshing a volume and is NULL when coming here. Nonetheless, I think adding VIR_FREE is good idea for preventing the possible leak in the future. So I'll add it.
Also there is no need to have 'if (xx)' before a call to VIR_FREE - it accepts NULLs happily. 'make syntax-check' would complain about this additional 'if'.
Oops, I did it, but did like 'make check; make syntax-check; make -C tests valgrind' and then lost the warning ;< Thanks anyway and will send revised patch soon! ozaki-r
Hi, This is the revised patch. The changes include dropping two fixes and revising one along with Chris' and Daniel's suggestions. Other fixes keep as they were. ozaki-r
From a0ed67d8a5979fb5ab48a30ab840eed212b7cb92 Mon Sep 17 00:00:00 2001 From: Ryota Ozaki <ozaki.ryota@gmail.com> Date: Thu, 3 Sep 2009 08:31:59 +0900 Subject: [PATCH] Fix some memory leaks
--- src/domain_conf.c | 1 + src/network_conf.c | 5 +++-- src/qemu_conf.c | 13 ++++++++----- src/storage_backend_fs.c | 2 ++ 4 files changed, 14 insertions(+), 7 deletions(-) diff --git a/src/domain_conf.c b/src/domain_conf.c index 1d2cc7c..4b64219 100644 --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -4361,6 +4361,7 @@ int virDomainSaveXML(virConnectPtr conn, cleanup: if (fd != -1) close(fd); + VIR_FREE(configFile); return ret; } diff --git a/src/network_conf.c b/src/network_conf.c index bb649a4..58a4f32 100644 --- a/src/network_conf.c +++ b/src/network_conf.c @@ -820,6 +820,7 @@ int virNetworkDeleteConfig(virConnectPtr conn, { char *configFile = NULL; char *autostartLink = NULL; + int ret = -1; if ((configFile = virNetworkConfigFile(conn, configDir, net->def->name)) == NULL) goto error; @@ -836,12 +837,12 @@ int virNetworkDeleteConfig(virConnectPtr conn, goto error; } - return 0; + ret = 0; error: VIR_FREE(configFile); VIR_FREE(autostartLink); - return -1; + return ret; } char *virNetworkConfigFile(virConnectPtr conn, diff --git a/src/qemu_conf.c b/src/qemu_conf.c index 22f5edd..32d6a48 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -1034,7 +1034,7 @@ qemudNetworkIfaceConnect(virConnectPtr conn, virDomainNetDefPtr net, int qemuCmdFlags) { - char *brname; + char *brname = NULL; int err; int tapfd = -1; int vnet_hdr = 0; @@ -1053,7 +1053,7 @@ qemudNetworkIfaceConnect(virConnectPtr conn, if (brname == NULL) return -1; } else if (net->type == VIR_DOMAIN_NET_TYPE_BRIDGE) { - brname = net->data.bridge.brname; + brname = strdup(net->data.bridge.brname); } else { qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _("Network type %d is not supported"), net->type); @@ -1063,7 +1063,7 @@ qemudNetworkIfaceConnect(virConnectPtr conn, if (!driver->brctl && (err = brInit(&driver->brctl))) { virReportSystemError(conn, err, "%s", _("cannot initialize bridge support")); - return -1; + goto cleanup; } if (!net->ifname || @@ -1072,7 +1072,7 @@ qemudNetworkIfaceConnect(virConnectPtr conn, VIR_FREE(net->ifname); if (!(net->ifname = strdup("vnet%d"))) { virReportOOMError(conn); - return -1; + goto cleanup; } /* avoid exposing vnet%d in dumpxml or error outputs */ template_ifname = 1; @@ -1100,9 +1100,12 @@ qemudNetworkIfaceConnect(virConnectPtr conn, } if (template_ifname) VIR_FREE(net->ifname); - return -1; + tapfd = -1; } +cleanup: + VIR_FREE(brname); + return tapfd; } diff --git a/src/storage_backend_fs.c b/src/storage_backend_fs.c index ca6d329..11ac48c 100644 --- a/src/storage_backend_fs.c +++ b/src/storage_backend_fs.c @@ -1022,6 +1022,7 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn, vol->type = VIR_STORAGE_VOL_FILE; + VIR_FREE(vol->target.path); if (virAsprintf(&vol->target.path, "%s/%s", pool->def->target.path, vol->name) == -1) { @@ -1029,6 +1030,7 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn, return -1; } + VIR_FREE(vol->key); vol->key = strdup(vol->target.path); if (vol->key == NULL) { virReportOOMError(conn); -- 1.6.0.6
On Thu, Sep 03, 2009 at 08:51:52AM +0900, Ryota Ozaki wrote:
Hi,
This is the revised patch. The changes include dropping two fixes and revising one along with Chris' and Daniel's suggestions. Other fixes keep as they were. [...] diff --git a/src/network_conf.c b/src/network_conf.c index bb649a4..58a4f32 100644 --- a/src/network_conf.c +++ b/src/network_conf.c @@ -820,6 +820,7 @@ int virNetworkDeleteConfig(virConnectPtr conn, { char *configFile = NULL; char *autostartLink = NULL; + int ret = -1;
if ((configFile = virNetworkConfigFile(conn, configDir, net->def->name)) == NULL)
Beware your mailer is not clean for patches, it somehow wrapped that long line, corrupting the patch, I would suggest to use attachments to avoid this :-) Everything looks fine, applied and commited, 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 Fri, Sep 4, 2009 at 10:58 PM, Daniel Veillard<veillard@redhat.com> wrote:
On Thu, Sep 03, 2009 at 08:51:52AM +0900, Ryota Ozaki wrote:
if ((configFile = virNetworkConfigFile(conn, configDir, net->def->name)) == NULL)
Beware your mailer is not clean for patches, it somehow wrapped that long line, corrupting the patch, I would suggest to use attachments to avoid this :-)
Hmm, Gmail... I had been suggested that my attachments was a bit strange by another Daniel, so I'll attach a patch both as inline and an attachment next time.
Everything looks fine, applied and commited,
thanks !
Thanks! ozaki-r
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 (4)
-
Chris Lalancette -
Daniel P. Berrange -
Daniel Veillard -
Ryota Ozaki