[libvirt] hash.c patches
Hi, I recently started looking at libvirt code, and while reading hash.c, I noticed a few buglets and some cleanup that could be done. I was planning to test them carefully before sending them (ie make sure there's a unit test for this code), but since I don't know when I have time for that, and since make check and make syntax-check are passing, I'm just sending them for an initial review :) If you prefer the patches inline/in separate mails, just let me know. Thanks! Christophe
On 02/11/2011 03:39 AM, Christophe Fergeau wrote:
Hi,
I recently started looking at libvirt code, and while reading hash.c, I noticed a few buglets and some cleanup that could be done. I was planning to test them carefully before sending them (ie make sure there's a unit test for this code), but since I don't know when I have time for that, and since make check and make syntax-check are passing, I'm just sending them for an initial review :)
If you prefer the patches inline/in separate mails, just let me know.
Thanks for your efforts so far; I've run out of time to review them much today, other than to point out that the patches state n/8 but there are only 6 of them. Yes, we prefer one patch per email all threaded together (git send-email -8 can do the right thing), as it's easier to comment on one patch at a time in that manner (if some of your patches are ready to be applied while others need rework, for example). Also, a question on 5/8, where you added calls to the OOM reporter - did you check that all hash table clients elsewhere in the code base are not duplicating the call when they get a NULL return? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Fri, Feb 11, 2011 at 05:07:43PM -0700, Eric Blake wrote:
Thanks for your efforts so far; I've run out of time to review them much today, other than to point out that the patches state n/8 but there are only 6 of them.
Oops, indeed, sorry for the confusion, the other 2 were unrelated cleanups I didn't want to send with the others, and I didn't pay attention the patch count was in the name :-/
Also, a question on 5/8, where you added calls to the OOM reporter - did you check that all hash table clients elsewhere in the code base are not duplicating the call when they get a NULL return?
Nope, I hadn't thought of checking that. I did it now, and indeed there were some places where virReportOOMError was called upon virHash call errors. Most of the time it was not called though, so having virHash functions directly call it help to make things more consistent.
Yes, we prefer one patch per email all threaded together (git send-email -8 can do the right thing)
Ok, I just tried doing this with this patch series with 2 additional patches related to your comment above, I hope this will work ok :) Christophe
--- src/util/hash.c | 7 ++++--- 1 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/util/hash.c b/src/util/hash.c index 5c56dae..e102d7d 100644 --- a/src/util/hash.c +++ b/src/util/hash.c @@ -184,9 +184,10 @@ virHashGrow(virHashTablePtr table, int size) VIR_FREE(oldtable); #ifdef DEBUG_GROW - xmlGenericError(xmlGenericErrorContext, - "virHashGrow : from %d to %d, %d elems\n", oldsize, - size, nbElem); + virReportErrorHelper(NULL, VIR_FROM_NONE, VIR_ERR_OK, __FILE__, + __FUNCTION__, __LINE__, + "virHashGrow : from %d to %d, %d elems", + oldsize, size, nbElem); #endif return (0); -- 1.7.4
missing NULL check on strdup --- src/util/hash.c | 18 +++++++++++++++--- 1 files changed, 15 insertions(+), 3 deletions(-) diff --git a/src/util/hash.c b/src/util/hash.c index e102d7d..893fe96 100644 --- a/src/util/hash.c +++ b/src/util/hash.c @@ -254,6 +254,7 @@ virHashAddEntry(virHashTablePtr table, const char *name, void *userdata) unsigned long key, len = 0; virHashEntryPtr entry; virHashEntryPtr insert; + char *new_name; if ((table == NULL) || (name == NULL)) return (-1); @@ -282,12 +283,17 @@ virHashAddEntry(virHashTablePtr table, const char *name, void *userdata) return (-1); } - entry->name = strdup(name); + new_name = strdup(name); + if (new_name == NULL) { + if (insert != NULL) + VIR_FREE(entry); + return (-1); + } + entry->name = new_name; entry->payload = userdata; entry->next = NULL; entry->valid = 1; - if (insert != NULL) insert->next = entry; @@ -354,7 +360,13 @@ virHashUpdateEntry(virHashTablePtr table, const char *name, return (-1); } - entry->name = strdup(name); + new_name= strdup(name); + if (new_name == NULL) { + if (insert != NULL) + VIR_FREE(entry); + return (-1); + } + entry->name = new_name; entry->payload = userdata; entry->next = NULL; entry->valid = 1; -- 1.7.4
On Sun, Feb 13, 2011 at 10:45:18PM +0100, Christophe Fergeau wrote:
missing NULL check on strdup --- src/util/hash.c | 18 +++++++++++++++--- 1 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/src/util/hash.c b/src/util/hash.c index e102d7d..893fe96 100644 --- a/src/util/hash.c +++ b/src/util/hash.c @@ -254,6 +254,7 @@ virHashAddEntry(virHashTablePtr table, const char *name, void *userdata) unsigned long key, len = 0; virHashEntryPtr entry; virHashEntryPtr insert; + char *new_name;
if ((table == NULL) || (name == NULL)) return (-1); @@ -282,12 +283,17 @@ virHashAddEntry(virHashTablePtr table, const char *name, void *userdata) return (-1); }
- entry->name = strdup(name); + new_name = strdup(name); + if (new_name == NULL) { + if (insert != NULL) + VIR_FREE(entry); + return (-1); + } + entry->name = new_name; entry->payload = userdata; entry->next = NULL; entry->valid = 1;
- if (insert != NULL) insert->next = entry;
@@ -354,7 +360,13 @@ virHashUpdateEntry(virHashTablePtr table, const char *name, return (-1); }
- entry->name = strdup(name); + new_name= strdup(name); + if (new_name == NULL) { + if (insert != NULL) + VIR_FREE(entry); + return (-1); + } + entry->name = new_name; entry->payload = userdata; entry->next = NULL; entry->valid = 1;
Okay, I commited that one, there was just a lack of declaration for new_name in virHashUpdateEntry, but the 2 routines had to be fixed, 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/
The code in virHashUpdateEntry and virHashAddEntry is really similar. However, the latter rebalances the hash table when one of its buckets contains too many elements while the former does not. Fix this discrepancy. --- src/util/hash.c | 7 ++++++- 1 files changed, 6 insertions(+), 1 deletions(-) diff --git a/src/util/hash.c b/src/util/hash.c index 893fe96..8e337eb 100644 --- a/src/util/hash.c +++ b/src/util/hash.c @@ -322,7 +322,7 @@ int virHashUpdateEntry(virHashTablePtr table, const char *name, void *userdata, virHashDeallocator f) { - unsigned long key; + unsigned long key, len = 0; virHashEntryPtr entry; virHashEntryPtr insert; @@ -344,6 +344,7 @@ virHashUpdateEntry(virHashTablePtr table, const char *name, insert->payload = userdata; return (0); } + len++; } if (STREQ(insert->name, name)) { if (f) @@ -376,6 +377,10 @@ virHashUpdateEntry(virHashTablePtr table, const char *name, if (insert != NULL) { insert->next = entry; } + + if (len > MAX_HASH_LEN) + virHashGrow(table, MAX_HASH_LEN * table->size); + return (0); } -- 1.7.4
The only difference between these 2 functions is that one errors out when the entry is already present while the other modifies the existing entry. Add an helper function with a boolean argument indicating whether existing entries should be updated or not, and use this helper in both functions. --- src/util/hash.c | 115 +++++++++++++++++++------------------------------------ 1 files changed, 40 insertions(+), 75 deletions(-) diff --git a/src/util/hash.c b/src/util/hash.c index 8e337eb..031e5af 100644 --- a/src/util/hash.c +++ b/src/util/hash.c @@ -21,6 +21,7 @@ #include <config.h> #include <string.h> +#include <stdbool.h> #include <stdlib.h> #include "virterror_internal.h" @@ -237,24 +238,16 @@ virHashFree(virHashTablePtr table, virHashDeallocator f) VIR_FREE(table); } -/** - * virHashAddEntry: - * @table: the hash table - * @name: the name of the userdata - * @userdata: a pointer to the userdata - * - * Add the @userdata to the hash @table. This can later be retrieved - * by using @name. Duplicate entries generate errors. - * - * Returns 0 the addition succeeded and -1 in case of error. - */ -int -virHashAddEntry(virHashTablePtr table, const char *name, void *userdata) +static int +virHashAddOrUpdateEntry(virHashTablePtr table, const char *name, + void *userdata, virHashDeallocator f, + bool is_update) { unsigned long key, len = 0; virHashEntryPtr entry; virHashEntryPtr insert; char *new_name; + bool found; if ((table == NULL) || (name == NULL)) return (-1); @@ -262,18 +255,32 @@ virHashAddEntry(virHashTablePtr table, const char *name, void *userdata) /* * Check for duplicate and insertion location. */ + found = false; key = virHashComputeKey(table, name); if (table->table[key].valid == 0) { insert = NULL; } else { for (insert = &(table->table[key]); insert->next != NULL; insert = insert->next) { - if (STREQ(insert->name, name)) - return (-1); + if (STREQ(insert->name, name)) { + found = true; + break; + } len++; } if (STREQ(insert->name, name)) + found = true; + } + + if (found) { + if (is_update) { + if (f) + f(insert->payload, insert->name); + insert->payload = userdata; + return (0); + } else { return (-1); + } } if (insert == NULL) { @@ -306,6 +313,23 @@ virHashAddEntry(virHashTablePtr table, const char *name, void *userdata) } /** + * virHashAddEntry: + * @table: the hash table + * @name: the name of the userdata + * @userdata: a pointer to the userdata + * + * Add the @userdata to the hash @table. This can later be retrieved + * by using @name. Duplicate entries generate errors. + * + * Returns 0 the addition succeeded and -1 in case of error. + */ +int +virHashAddEntry(virHashTablePtr table, const char *name, void *userdata) +{ + return virHashAddOrUpdateEntry(table, name, userdata, NULL, false); +} + +/** * virHashUpdateEntry: * @table: the hash table * @name: the name of the userdata @@ -322,66 +346,7 @@ int virHashUpdateEntry(virHashTablePtr table, const char *name, void *userdata, virHashDeallocator f) { - unsigned long key, len = 0; - virHashEntryPtr entry; - virHashEntryPtr insert; - - if ((table == NULL) || name == NULL) - return (-1); - - /* - * Check for duplicate and insertion location. - */ - key = virHashComputeKey(table, name); - if (table->table[key].valid == 0) { - insert = NULL; - } else { - for (insert = &(table->table[key]); insert->next != NULL; - insert = insert->next) { - if (STREQ(insert->name, name)) { - if (f) - f(insert->payload, insert->name); - insert->payload = userdata; - return (0); - } - len++; - } - if (STREQ(insert->name, name)) { - if (f) - f(insert->payload, insert->name); - insert->payload = userdata; - return (0); - } - } - - if (insert == NULL) { - entry = &(table->table[key]); - } else { - if (VIR_ALLOC(entry) < 0) - return (-1); - } - - new_name= strdup(name); - if (new_name == NULL) { - if (insert != NULL) - VIR_FREE(entry); - return (-1); - } - entry->name = new_name; - entry->payload = userdata; - entry->next = NULL; - entry->valid = 1; - table->nbElems++; - - - if (insert != NULL) { - insert->next = entry; - } - - if (len > MAX_HASH_LEN) - virHashGrow(table, MAX_HASH_LEN * table->size); - - return (0); + return virHashAddOrUpdateEntry(table, name, userdata, f, true); } /** -- 1.7.4
--- src/util/hash.c | 13 +++++++++++-- 1 files changed, 11 insertions(+), 2 deletions(-) diff --git a/src/util/hash.c b/src/util/hash.c index 031e5af..f19d249 100644 --- a/src/util/hash.c +++ b/src/util/hash.c @@ -28,6 +28,8 @@ #include "hash.h" #include "memory.h" +#define VIR_FROM_THIS VIR_FROM_NONE + #define MAX_HASH_LEN 8 /* #define DEBUG_GROW */ @@ -89,12 +91,15 @@ virHashCreate(int size) if (size <= 0) size = 256; - if (VIR_ALLOC(table) < 0) + if (VIR_ALLOC(table) < 0) { + virReportOOMError(); return NULL; + } table->size = size; table->nbElems = 0; if (VIR_ALLOC_N(table->table, size) < 0) { + virReportOOMError(); VIR_FREE(table); return NULL; } @@ -136,6 +141,7 @@ virHashGrow(virHashTablePtr table, int size) return (-1); if (VIR_ALLOC_N(table->table, size) < 0) { + virReportOOMError(); table->table = oldtable; return (-1); } @@ -286,12 +292,15 @@ virHashAddOrUpdateEntry(virHashTablePtr table, const char *name, if (insert == NULL) { entry = &(table->table[key]); } else { - if (VIR_ALLOC(entry) < 0) + if (VIR_ALLOC(entry) < 0) { + virReportOOMError(); return (-1); + } } new_name = strdup(name); if (new_name == NULL) { + virReportOOMError(); if (insert != NULL) VIR_FREE(entry); return (-1); -- 1.7.4
On Sun, Feb 13, 2011 at 10:45:21PM +0100, Christophe Fergeau wrote:
--- src/util/hash.c | 13 +++++++++++-- 1 files changed, 11 insertions(+), 2 deletions(-)
ACK, and pushed as it's something I would like to get for 0.8.8, 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/
Now that the virHash handling functions call virReportOOMError by themselves when needed, users of the virHash API no longer need to do it by themselves. Since users of the virHash API were not consitently calling virReportOOMError after memory failures from the virHash code, this has the added benefit of making OOM reporting from this code more consistent and reliable. --- src/conf/domain_conf.c | 18 ++++-------------- src/conf/nwfilter_conf.c | 4 +--- src/conf/nwfilter_params.c | 1 - src/nwfilter/nwfilter_gentech_driver.c | 2 -- src/nwfilter/nwfilter_learnipaddr.c | 3 --- src/openvz/openvz_conf.c | 2 +- src/qemu/qemu_command.c | 2 +- 7 files changed, 7 insertions(+), 25 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 59adf36..b780c74 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -389,10 +389,8 @@ VIR_ENUM_IMPL(virDomainTimerMode, VIR_DOMAIN_TIMER_MODE_LAST, int virDomainObjListInit(virDomainObjListPtr doms) { doms->objs = virHashCreate(50); - if (!doms->objs) { - virReportOOMError(); + if (!doms->objs) return -1; - } return 0; } @@ -1056,7 +1054,6 @@ virDomainObjPtr virDomainAssignDef(virCapsPtr caps, virUUIDFormat(def->uuid, uuidstr); if (virHashAddEntry(doms->objs, uuidstr, domain) < 0) { VIR_FREE(domain); - virReportOOMError(); return NULL; } @@ -8157,10 +8154,8 @@ static virDomainObjPtr virDomainLoadStatus(virCapsPtr caps, goto error; } - if (virHashAddEntry(doms->objs, uuidstr, obj) < 0) { - virReportOOMError(); + if (virHashAddEntry(doms->objs, uuidstr, obj) < 0) goto error; - } if (notify) (*notify)(obj, 1, opaque); @@ -8718,7 +8713,6 @@ virDomainSnapshotObjPtr virDomainSnapshotAssignDef(virDomainSnapshotObjListPtr s if (virHashAddEntry(snapshots->objs, snap->def->name, snap) < 0) { VIR_FREE(snap); - virReportOOMError(); return NULL; } @@ -8729,10 +8723,8 @@ virDomainSnapshotObjPtr virDomainSnapshotAssignDef(virDomainSnapshotObjListPtr s int virDomainSnapshotObjListInit(virDomainSnapshotObjListPtr snapshots) { snapshots->objs = virHashCreate(50); - if (!snapshots->objs) { - virReportOOMError(); + if (!snapshots->objs) return -1; - } return 0; } @@ -9021,10 +9013,8 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, _("could not close file %s"), path); - if (virHashAddEntry(paths, path, (void*)0x1) < 0) { - virReportOOMError(); + if (virHashAddEntry(paths, path, (void*)0x1) < 0) goto cleanup; - } depth++; nextpath = meta.backingStore; diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index a5703cb..c6a4d6f 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -2300,10 +2300,8 @@ virNWFilterTriggerVMFilterRebuild(virConnectPtr conn) .skipInterfaces = virHashCreate(0), }; - if (!cb.skipInterfaces) { - virReportOOMError(); + if (!cb.skipInterfaces) return 1; - } for (i = 0; i < nCallbackDriver; i++) { callbackDrvArray[i]->vmFilterRebuild(conn, diff --git a/src/conf/nwfilter_params.c b/src/conf/nwfilter_params.c index 7a59387..23423fa 100644 --- a/src/conf/nwfilter_params.c +++ b/src/conf/nwfilter_params.c @@ -121,7 +121,6 @@ virNWFilterHashTableCreate(int n) { } ret->hashTable = virHashCreate(n); if (!ret->hashTable) { - virReportOOMError(); VIR_FREE(ret); return NULL; } diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 9ef3692..d81aac8 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -1037,8 +1037,6 @@ virNWFilterDomainFWUpdateCB(void *payload, cb->err = virHashAddEntry(cb->skipInterfaces, net->ifname, (void *)~0); - if (cb->err) - virReportOOMError(); } break; diff --git a/src/nwfilter/nwfilter_learnipaddr.c b/src/nwfilter/nwfilter_learnipaddr.c index c593481..02af918 100644 --- a/src/nwfilter/nwfilter_learnipaddr.c +++ b/src/nwfilter/nwfilter_learnipaddr.c @@ -165,7 +165,6 @@ virNWFilterLockIface(const char *ifname) { } while (virHashAddEntry(ifaceLockMap, ifname, ifaceLock)) { - virReportOOMError(); VIR_FREE(ifaceLock); goto err_exit; } @@ -825,7 +824,6 @@ virNWFilterLearnInit(void) { pendingLearnReq = virHashCreate(0); if (!pendingLearnReq) { - virReportOOMError(); return 1; } @@ -848,7 +846,6 @@ virNWFilterLearnInit(void) { ifaceLockMap = virHashCreate(0); if (!ifaceLockMap) { - virReportOOMError(); virNWFilterLearnShutdown(); return 1; } diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index dae66a5..0eb5ab3 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -526,7 +526,7 @@ int openvzLoadDomains(struct openvz_driver *driver) { virUUIDFormat(dom->def->uuid, uuidstr); if (virHashAddEntry(driver->domains.objs, uuidstr, dom) < 0) - goto no_memory; + goto cleanup; virDomainObjUnlock(dom); dom = NULL; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 3ba0950..7826132 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -723,7 +723,7 @@ qemuDomainPCIAddressSetPtr qemuDomainPCIAddressSetCreate(virDomainDefPtr def) goto no_memory; if (!(addrs->used = virHashCreate(10))) - goto no_memory; + goto error; if (virDomainDeviceInfoIterate(def, qemuCollectPCIAddress, addrs) < 0) goto error; -- 1.7.4
virHashFree follows the convention described in HACKING that XXXFree() functions can be called with a NULL argument. --- src/conf/domain_conf.c | 6 +--- src/datatypes.c | 51 ++++++++++++++++-------------------------------- src/qemu/qemu_driver.c | 4 +-- 3 files changed, 20 insertions(+), 41 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b780c74..8129829 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -405,8 +405,7 @@ static void virDomainObjListDeallocator(void *payload, const char *name ATTRIBUT void virDomainObjListDeinit(virDomainObjListPtr doms) { - if (doms->objs) - virHashFree(doms->objs, virDomainObjListDeallocator); + virHashFree(doms->objs, virDomainObjListDeallocator); } @@ -8738,8 +8737,7 @@ static void virDomainSnapshotObjListDeallocator(void *payload, static void virDomainSnapshotObjListDeinit(virDomainSnapshotObjListPtr snapshots) { - if (snapshots->objs) - virHashFree(snapshots->objs, virDomainSnapshotObjListDeallocator); + virHashFree(snapshots->objs, virDomainSnapshotObjListDeallocator); } struct virDomainSnapshotNameData { diff --git a/src/datatypes.c b/src/datatypes.c index e26e332..4f7373c 100644 --- a/src/datatypes.c +++ b/src/datatypes.c @@ -209,22 +209,14 @@ virGetConnect(void) { failed: if (ret != NULL) { - if (ret->domains != NULL) - virHashFree(ret->domains, (virHashDeallocator) virDomainFreeName); - if (ret->networks != NULL) - virHashFree(ret->networks, (virHashDeallocator) virNetworkFreeName); - if (ret->interfaces != NULL) - virHashFree(ret->interfaces, (virHashDeallocator) virInterfaceFreeName); - if (ret->storagePools != NULL) - virHashFree(ret->storagePools, (virHashDeallocator) virStoragePoolFreeName); - if (ret->storageVols != NULL) - virHashFree(ret->storageVols, (virHashDeallocator) virStorageVolFreeName); - if (ret->nodeDevices != NULL) - virHashFree(ret->nodeDevices, (virHashDeallocator) virNodeDeviceFree); - if (ret->secrets != NULL) - virHashFree(ret->secrets, (virHashDeallocator) virSecretFreeName); - if (ret->nwfilters != NULL) - virHashFree(ret->nwfilters, (virHashDeallocator) virNWFilterFreeName); + virHashFree(ret->domains, (virHashDeallocator) virDomainFreeName); + virHashFree(ret->networks, (virHashDeallocator) virNetworkFreeName); + virHashFree(ret->interfaces, (virHashDeallocator) virInterfaceFreeName); + virHashFree(ret->storagePools, (virHashDeallocator) virStoragePoolFreeName); + virHashFree(ret->storageVols, (virHashDeallocator) virStorageVolFreeName); + virHashFree(ret->nodeDevices, (virHashDeallocator) virNodeDeviceFree); + virHashFree(ret->secrets, (virHashDeallocator) virSecretFreeName); + virHashFree(ret->nwfilters, (virHashDeallocator) virNWFilterFreeName); virMutexDestroy(&ret->lock); VIR_FREE(ret); @@ -267,22 +259,14 @@ virReleaseConnect(virConnectPtr conn) { virMutexLock(&conn->lock); - if (conn->domains != NULL) - virHashFree(conn->domains, (virHashDeallocator) virDomainFreeName); - if (conn->networks != NULL) - virHashFree(conn->networks, (virHashDeallocator) virNetworkFreeName); - if (conn->interfaces != NULL) - virHashFree(conn->interfaces, (virHashDeallocator) virInterfaceFreeName); - if (conn->storagePools != NULL) - virHashFree(conn->storagePools, (virHashDeallocator) virStoragePoolFreeName); - if (conn->storageVols != NULL) - virHashFree(conn->storageVols, (virHashDeallocator) virStorageVolFreeName); - if (conn->nodeDevices != NULL) - virHashFree(conn->nodeDevices, (virHashDeallocator) virNodeDeviceFree); - if (conn->secrets != NULL) - virHashFree(conn->secrets, (virHashDeallocator) virSecretFreeName); - if (conn->nwfilters != NULL) - virHashFree(conn->nwfilters, (virHashDeallocator) virNWFilterFreeName); + virHashFree(conn->domains, (virHashDeallocator) virDomainFreeName); + virHashFree(conn->networks, (virHashDeallocator) virNetworkFreeName); + virHashFree(conn->interfaces, (virHashDeallocator) virInterfaceFreeName); + virHashFree(conn->storagePools, (virHashDeallocator) virStoragePoolFreeName); + virHashFree(conn->storageVols, (virHashDeallocator) virStorageVolFreeName); + virHashFree(conn->nodeDevices, (virHashDeallocator) virNodeDeviceFree); + virHashFree(conn->secrets, (virHashDeallocator) virSecretFreeName); + virHashFree(conn->nwfilters, (virHashDeallocator) virNWFilterFreeName); virResetError(&conn->err); @@ -429,8 +413,7 @@ virReleaseDomain(virDomainPtr domain) { domain->magic = -1; domain->id = -1; VIR_FREE(domain->name); - if (domain->snapshots != NULL) - virHashFree(domain->snapshots, (virHashDeallocator) virDomainSnapshotFreeName); + virHashFree(domain->snapshots, (virHashDeallocator) virDomainSnapshotFreeName); VIR_FREE(domain); if (conn) { diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index fa462f9..495ad05 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1870,9 +1870,7 @@ qemudWaitForMonitor(struct qemud_driver* driver, } cleanup: - if (paths) { - virHashFree(paths, qemudFreePtyPath); - } + virHashFree(paths, qemudFreePtyPath); if (kill(vm->pid, 0) == -1 && errno == ESRCH) { /* VM is dead, any other error raised in the interim is probably -- 1.7.4
When creating the virDomain::snapshots hash table, virGetDomain wasn't checking if the creation was successful. This would then lead to failures in the vir*DomainSnapshot functions. Better to report this error early and make virGetDomain fail if the snapshots hash couldn't be created. --- src/datatypes.c | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-) diff --git a/src/datatypes.c b/src/datatypes.c index 4f7373c..7cc37c1 100644 --- a/src/datatypes.c +++ b/src/datatypes.c @@ -359,6 +359,12 @@ virGetDomain(virConnectPtr conn, const char *name, const unsigned char *uuid) { ret->id = -1; memcpy(&(ret->uuid[0]), uuid, VIR_UUID_BUFLEN); ret->snapshots = virHashCreate(20); + if (ret->snapshots == NULL) { + virMutexUnlock(&conn->lock); + virLibConnError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to create domain snapshots hash")); + goto error; + } if (virHashAddEntry(conn->domains, uuidstr, ret) < 0) { virMutexUnlock(&conn->lock); -- 1.7.4
On Sun, Feb 13, 2011 at 10:45:24PM +0100, Christophe Fergeau wrote:
When creating the virDomain::snapshots hash table, virGetDomain wasn't checking if the creation was successful. This would then lead to failures in the vir*DomainSnapshot functions. Better to report this error early and make virGetDomain fail if the snapshots hash couldn't be created. --- src/datatypes.c | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/src/datatypes.c b/src/datatypes.c index 4f7373c..7cc37c1 100644 --- a/src/datatypes.c +++ b/src/datatypes.c @@ -359,6 +359,12 @@ virGetDomain(virConnectPtr conn, const char *name, const unsigned char *uuid) { ret->id = -1; memcpy(&(ret->uuid[0]), uuid, VIR_UUID_BUFLEN); ret->snapshots = virHashCreate(20); + if (ret->snapshots == NULL) { + virMutexUnlock(&conn->lock); + virLibConnError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to create domain snapshots hash")); + goto error; + }
if (virHashAddEntry(conn->domains, uuidstr, ret) < 0) { virMutexUnlock(&conn->lock);
ACK, and pushed, taht one should go in for 0.8.8 too. The others which are more about refactoring and cleanups should wait a bit until after the release, 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/
Hi, This is a resend of my virHash patches. I rebased my git branch on top of origin/master after the release, since 2 of them needed rediffing, here they are again. Christophe
--- src/util/hash.c | 7 ++++--- 1 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/util/hash.c b/src/util/hash.c index c7a52c9..ba156b1 100644 --- a/src/util/hash.c +++ b/src/util/hash.c @@ -190,9 +190,10 @@ virHashGrow(virHashTablePtr table, int size) VIR_FREE(oldtable); #ifdef DEBUG_GROW - xmlGenericError(xmlGenericErrorContext, - "virHashGrow : from %d to %d, %d elems\n", oldsize, - size, nbElem); + virReportErrorHelper(NULL, VIR_FROM_NONE, VIR_ERR_OK, __FILE__, + __FUNCTION__, __LINE__, + "virHashGrow : from %d to %d, %d elems", + oldsize, size, nbElem); #endif return (0); -- 1.7.4
On 02/17/2011 02:14 PM, Christophe Fergeau wrote:
--- src/util/hash.c | 7 ++++--- 1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/util/hash.c b/src/util/hash.c index c7a52c9..ba156b1 100644 --- a/src/util/hash.c +++ b/src/util/hash.c @@ -190,9 +190,10 @@ virHashGrow(virHashTablePtr table, int size) VIR_FREE(oldtable);
#ifdef DEBUG_GROW - xmlGenericError(xmlGenericErrorContext, - "virHashGrow : from %d to %d, %d elems\n", oldsize, - size, nbElem); + virReportErrorHelper(NULL, VIR_FROM_NONE, VIR_ERR_OK, __FILE__, + __FUNCTION__, __LINE__, + "virHashGrow : from %d to %d, %d elems", + oldsize, size, nbElem);
I'd rather use VIR_DEBUG than virReportErrorHelper here. I pushed this instead: diff --git i/src/util/hash.c w/src/util/hash.c index c7a52c9..41df7ae 100644 --- i/src/util/hash.c +++ w/src/util/hash.c @@ -3,6 +3,7 @@ * * Reference: Your favorite introductory book on algorithms * + * Copyright (C) 2011 Red Hat, Inc. * Copyright (C) 2000 Bjorn Reese and Daniel Veillard. * * Permission to use, copy, modify, and distribute this software for any @@ -26,6 +27,7 @@ #include "virterror_internal.h" #include "hash.h" #include "memory.h" +#include "logging.h" #define VIR_FROM_THIS VIR_FROM_NONE @@ -190,9 +192,8 @@ virHashGrow(virHashTablePtr table, int size) VIR_FREE(oldtable); #ifdef DEBUG_GROW - xmlGenericError(xmlGenericErrorContext, - "virHashGrow : from %d to %d, %d elems\n", oldsize, - size, nbElem); + VIR_DEBUG("virHashGrow : from %d to %d, %ld elems\n", oldsize, + size, nbElem); #endif return (0); -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
The code in virHashUpdateEntry and virHashAddEntry is really similar. However, the latter rebalances the hash table when one of its buckets contains too many elements while the former does not. Fix this discrepancy. --- src/util/hash.c | 7 ++++++- 1 files changed, 6 insertions(+), 1 deletions(-) diff --git a/src/util/hash.c b/src/util/hash.c index ba156b1..595f447 100644 --- a/src/util/hash.c +++ b/src/util/hash.c @@ -331,7 +331,7 @@ int virHashUpdateEntry(virHashTablePtr table, const char *name, void *userdata, virHashDeallocator f) { - unsigned long key; + unsigned long key, len = 0; virHashEntryPtr entry; virHashEntryPtr insert; char *new_name; @@ -354,6 +354,7 @@ virHashUpdateEntry(virHashTablePtr table, const char *name, insert->payload = userdata; return (0); } + len++; } if (STREQ(insert->name, name)) { if (f) @@ -386,6 +387,10 @@ virHashUpdateEntry(virHashTablePtr table, const char *name, if (insert != NULL) { insert->next = entry; } + + if (len > MAX_HASH_LEN) + virHashGrow(table, MAX_HASH_LEN * table->size); + return (0); } -- 1.7.4
On 02/17/2011 02:14 PM, Christophe Fergeau wrote:
The code in virHashUpdateEntry and virHashAddEntry is really similar. However, the latter rebalances the hash table when one of its buckets contains too many elements while the former does not. Fix this discrepancy. --- src/util/hash.c | 7 ++++++- 1 files changed, 6 insertions(+), 1 deletions(-)
ACK and pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
The only difference between these 2 functions is that one errors out when the entry is already present while the other modifies the existing entry. Add an helper function with a boolean argument indicating whether existing entries should be updated or not, and use this helper in both functions. --- src/util/hash.c | 116 +++++++++++++++++++------------------------------------ 1 files changed, 40 insertions(+), 76 deletions(-) diff --git a/src/util/hash.c b/src/util/hash.c index 595f447..f19d249 100644 --- a/src/util/hash.c +++ b/src/util/hash.c @@ -21,6 +21,7 @@ #include <config.h> #include <string.h> +#include <stdbool.h> #include <stdlib.h> #include "virterror_internal.h" @@ -243,24 +244,16 @@ virHashFree(virHashTablePtr table, virHashDeallocator f) VIR_FREE(table); } -/** - * virHashAddEntry: - * @table: the hash table - * @name: the name of the userdata - * @userdata: a pointer to the userdata - * - * Add the @userdata to the hash @table. This can later be retrieved - * by using @name. Duplicate entries generate errors. - * - * Returns 0 the addition succeeded and -1 in case of error. - */ -int -virHashAddEntry(virHashTablePtr table, const char *name, void *userdata) +static int +virHashAddOrUpdateEntry(virHashTablePtr table, const char *name, + void *userdata, virHashDeallocator f, + bool is_update) { unsigned long key, len = 0; virHashEntryPtr entry; virHashEntryPtr insert; char *new_name; + bool found; if ((table == NULL) || (name == NULL)) return (-1); @@ -268,18 +261,32 @@ virHashAddEntry(virHashTablePtr table, const char *name, void *userdata) /* * Check for duplicate and insertion location. */ + found = false; key = virHashComputeKey(table, name); if (table->table[key].valid == 0) { insert = NULL; } else { for (insert = &(table->table[key]); insert->next != NULL; insert = insert->next) { - if (STREQ(insert->name, name)) - return (-1); + if (STREQ(insert->name, name)) { + found = true; + break; + } len++; } if (STREQ(insert->name, name)) + found = true; + } + + if (found) { + if (is_update) { + if (f) + f(insert->payload, insert->name); + insert->payload = userdata; + return (0); + } else { return (-1); + } } if (insert == NULL) { @@ -315,6 +322,23 @@ virHashAddEntry(virHashTablePtr table, const char *name, void *userdata) } /** + * virHashAddEntry: + * @table: the hash table + * @name: the name of the userdata + * @userdata: a pointer to the userdata + * + * Add the @userdata to the hash @table. This can later be retrieved + * by using @name. Duplicate entries generate errors. + * + * Returns 0 the addition succeeded and -1 in case of error. + */ +int +virHashAddEntry(virHashTablePtr table, const char *name, void *userdata) +{ + return virHashAddOrUpdateEntry(table, name, userdata, NULL, false); +} + +/** * virHashUpdateEntry: * @table: the hash table * @name: the name of the userdata @@ -331,67 +355,7 @@ int virHashUpdateEntry(virHashTablePtr table, const char *name, void *userdata, virHashDeallocator f) { - unsigned long key, len = 0; - virHashEntryPtr entry; - virHashEntryPtr insert; - char *new_name; - - if ((table == NULL) || name == NULL) - return (-1); - - /* - * Check for duplicate and insertion location. - */ - key = virHashComputeKey(table, name); - if (table->table[key].valid == 0) { - insert = NULL; - } else { - for (insert = &(table->table[key]); insert->next != NULL; - insert = insert->next) { - if (STREQ(insert->name, name)) { - if (f) - f(insert->payload, insert->name); - insert->payload = userdata; - return (0); - } - len++; - } - if (STREQ(insert->name, name)) { - if (f) - f(insert->payload, insert->name); - insert->payload = userdata; - return (0); - } - } - - if (insert == NULL) { - entry = &(table->table[key]); - } else { - if (VIR_ALLOC(entry) < 0) - return (-1); - } - - new_name= strdup(name); - if (new_name == NULL) { - if (insert != NULL) - VIR_FREE(entry); - return (-1); - } - entry->name = new_name; - entry->payload = userdata; - entry->next = NULL; - entry->valid = 1; - table->nbElems++; - - - if (insert != NULL) { - insert->next = entry; - } - - if (len > MAX_HASH_LEN) - virHashGrow(table, MAX_HASH_LEN * table->size); - - return (0); + return virHashAddOrUpdateEntry(table, name, userdata, f, true); } /** -- 1.7.4
On 02/17/2011 02:14 PM, Christophe Fergeau wrote:
The only difference between these 2 functions is that one errors out when the entry is already present while the other modifies the existing entry. Add an helper function with a boolean argument indicating whether existing entries should be updated or not, and use this helper in both functions. --- src/util/hash.c | 116 +++++++++++++++++++------------------------------------ 1 files changed, 40 insertions(+), 76 deletions(-)
ACK, and the diffstat shows how nice it is. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Now that the virHash handling functions call virReportOOMError by themselves when needed, users of the virHash API no longer need to do it by themselves. Since users of the virHash API were not consitently calling virReportOOMError after memory failures from the virHash code, this has the added benefit of making OOM reporting from this code more consistent and reliable. --- src/conf/domain_conf.c | 18 ++++-------------- src/conf/nwfilter_conf.c | 4 +--- src/conf/nwfilter_params.c | 1 - src/nwfilter/nwfilter_gentech_driver.c | 2 -- src/nwfilter/nwfilter_learnipaddr.c | 3 --- src/openvz/openvz_conf.c | 2 +- src/qemu/qemu_command.c | 2 +- 7 files changed, 7 insertions(+), 25 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 01a5b2c..351daf7 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -394,10 +394,8 @@ VIR_ENUM_IMPL(virDomainTimerMode, VIR_DOMAIN_TIMER_MODE_LAST, int virDomainObjListInit(virDomainObjListPtr doms) { doms->objs = virHashCreate(50); - if (!doms->objs) { - virReportOOMError(); + if (!doms->objs) return -1; - } return 0; } @@ -1061,7 +1059,6 @@ virDomainObjPtr virDomainAssignDef(virCapsPtr caps, virUUIDFormat(def->uuid, uuidstr); if (virHashAddEntry(doms->objs, uuidstr, domain) < 0) { VIR_FREE(domain); - virReportOOMError(); return NULL; } @@ -8189,10 +8186,8 @@ static virDomainObjPtr virDomainLoadStatus(virCapsPtr caps, goto error; } - if (virHashAddEntry(doms->objs, uuidstr, obj) < 0) { - virReportOOMError(); + if (virHashAddEntry(doms->objs, uuidstr, obj) < 0) goto error; - } if (notify) (*notify)(obj, 1, opaque); @@ -8750,7 +8745,6 @@ virDomainSnapshotObjPtr virDomainSnapshotAssignDef(virDomainSnapshotObjListPtr s if (virHashAddEntry(snapshots->objs, snap->def->name, snap) < 0) { VIR_FREE(snap); - virReportOOMError(); return NULL; } @@ -8761,10 +8755,8 @@ virDomainSnapshotObjPtr virDomainSnapshotAssignDef(virDomainSnapshotObjListPtr s int virDomainSnapshotObjListInit(virDomainSnapshotObjListPtr snapshots) { snapshots->objs = virHashCreate(50); - if (!snapshots->objs) { - virReportOOMError(); + if (!snapshots->objs) return -1; - } return 0; } @@ -9053,10 +9045,8 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, _("could not close file %s"), path); - if (virHashAddEntry(paths, path, (void*)0x1) < 0) { - virReportOOMError(); + if (virHashAddEntry(paths, path, (void*)0x1) < 0) goto cleanup; - } depth++; nextpath = meta.backingStore; diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index a5703cb..c6a4d6f 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -2300,10 +2300,8 @@ virNWFilterTriggerVMFilterRebuild(virConnectPtr conn) .skipInterfaces = virHashCreate(0), }; - if (!cb.skipInterfaces) { - virReportOOMError(); + if (!cb.skipInterfaces) return 1; - } for (i = 0; i < nCallbackDriver; i++) { callbackDrvArray[i]->vmFilterRebuild(conn, diff --git a/src/conf/nwfilter_params.c b/src/conf/nwfilter_params.c index 7a59387..23423fa 100644 --- a/src/conf/nwfilter_params.c +++ b/src/conf/nwfilter_params.c @@ -121,7 +121,6 @@ virNWFilterHashTableCreate(int n) { } ret->hashTable = virHashCreate(n); if (!ret->hashTable) { - virReportOOMError(); VIR_FREE(ret); return NULL; } diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 9ef3692..d81aac8 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -1037,8 +1037,6 @@ virNWFilterDomainFWUpdateCB(void *payload, cb->err = virHashAddEntry(cb->skipInterfaces, net->ifname, (void *)~0); - if (cb->err) - virReportOOMError(); } break; diff --git a/src/nwfilter/nwfilter_learnipaddr.c b/src/nwfilter/nwfilter_learnipaddr.c index c593481..02af918 100644 --- a/src/nwfilter/nwfilter_learnipaddr.c +++ b/src/nwfilter/nwfilter_learnipaddr.c @@ -165,7 +165,6 @@ virNWFilterLockIface(const char *ifname) { } while (virHashAddEntry(ifaceLockMap, ifname, ifaceLock)) { - virReportOOMError(); VIR_FREE(ifaceLock); goto err_exit; } @@ -825,7 +824,6 @@ virNWFilterLearnInit(void) { pendingLearnReq = virHashCreate(0); if (!pendingLearnReq) { - virReportOOMError(); return 1; } @@ -848,7 +846,6 @@ virNWFilterLearnInit(void) { ifaceLockMap = virHashCreate(0); if (!ifaceLockMap) { - virReportOOMError(); virNWFilterLearnShutdown(); return 1; } diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index dae66a5..0eb5ab3 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -526,7 +526,7 @@ int openvzLoadDomains(struct openvz_driver *driver) { virUUIDFormat(dom->def->uuid, uuidstr); if (virHashAddEntry(driver->domains.objs, uuidstr, dom) < 0) - goto no_memory; + goto cleanup; virDomainObjUnlock(dom); dom = NULL; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 618d3a9..c9feb9b 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -753,7 +753,7 @@ qemuDomainPCIAddressSetPtr qemuDomainPCIAddressSetCreate(virDomainDefPtr def) goto no_memory; if (!(addrs->used = virHashCreate(10))) - goto no_memory; + goto error; if (virDomainDeviceInfoIterate(def, qemuCollectPCIAddress, addrs) < 0) goto error; -- 1.7.4
On 02/17/2011 02:14 PM, Christophe Fergeau wrote:
Now that the virHash handling functions call virReportOOMError by themselves when needed, users of the virHash API no longer need to do it by themselves. Since users of the virHash API were not consitently calling virReportOOMError after memory failures from
s/consitently/consistently/
diff --git a/src/nwfilter/nwfilter_learnipaddr.c b/src/nwfilter/nwfilter_learnipaddr.c index c593481..02af918 100644 --- a/src/nwfilter/nwfilter_learnipaddr.c +++ b/src/nwfilter/nwfilter_learnipaddr.c @@ -165,7 +165,6 @@ virNWFilterLockIface(const char *ifname) { }
while (virHashAddEntry(ifaceLockMap, ifname, ifaceLock)) { - virReportOOMError(); VIR_FREE(ifaceLock); goto err_exit; }
What an odd-looking control flow, when an 'if' would have been more idiomatic. But independent of this patch. You missed one in qemu_process: diff --git i/src/qemu/qemu_process.c w/src/qemu/qemu_process.c index 46da600..5237e2b 100644 --- i/src/qemu/qemu_process.c +++ w/src/qemu/qemu_process.c @@ -995,10 +995,8 @@ qemuProcessWaitForMonitor(struct qemud_driver* driver, * Note that the monitor itself can be on a pty, so we still need to try the * log output method. */ paths = virHashCreate(0); - if (paths == NULL) { - virReportOOMError(); + if (paths == NULL) goto cleanup; - } qemuDomainObjEnterMonitorWithDriver(driver, vm); qemuDomainObjPrivatePtr priv = vm->privateData; ACK with those corrections, so pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
virHashFree follows the convention described in HACKING that XXXFree() functions can be called with a NULL argument. --- src/conf/domain_conf.c | 6 +--- src/datatypes.c | 51 +++++++++++++++------------------------------- src/qemu/qemu_process.c | 4 +-- 3 files changed, 20 insertions(+), 41 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 351daf7..e7c3409 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -410,8 +410,7 @@ static void virDomainObjListDeallocator(void *payload, const char *name ATTRIBUT void virDomainObjListDeinit(virDomainObjListPtr doms) { - if (doms->objs) - virHashFree(doms->objs, virDomainObjListDeallocator); + virHashFree(doms->objs, virDomainObjListDeallocator); } @@ -8770,8 +8769,7 @@ static void virDomainSnapshotObjListDeallocator(void *payload, static void virDomainSnapshotObjListDeinit(virDomainSnapshotObjListPtr snapshots) { - if (snapshots->objs) - virHashFree(snapshots->objs, virDomainSnapshotObjListDeallocator); + virHashFree(snapshots->objs, virDomainSnapshotObjListDeallocator); } struct virDomainSnapshotNameData { diff --git a/src/datatypes.c b/src/datatypes.c index d870e5d..7cc37c1 100644 --- a/src/datatypes.c +++ b/src/datatypes.c @@ -209,22 +209,14 @@ virGetConnect(void) { failed: if (ret != NULL) { - if (ret->domains != NULL) - virHashFree(ret->domains, (virHashDeallocator) virDomainFreeName); - if (ret->networks != NULL) - virHashFree(ret->networks, (virHashDeallocator) virNetworkFreeName); - if (ret->interfaces != NULL) - virHashFree(ret->interfaces, (virHashDeallocator) virInterfaceFreeName); - if (ret->storagePools != NULL) - virHashFree(ret->storagePools, (virHashDeallocator) virStoragePoolFreeName); - if (ret->storageVols != NULL) - virHashFree(ret->storageVols, (virHashDeallocator) virStorageVolFreeName); - if (ret->nodeDevices != NULL) - virHashFree(ret->nodeDevices, (virHashDeallocator) virNodeDeviceFree); - if (ret->secrets != NULL) - virHashFree(ret->secrets, (virHashDeallocator) virSecretFreeName); - if (ret->nwfilters != NULL) - virHashFree(ret->nwfilters, (virHashDeallocator) virNWFilterFreeName); + virHashFree(ret->domains, (virHashDeallocator) virDomainFreeName); + virHashFree(ret->networks, (virHashDeallocator) virNetworkFreeName); + virHashFree(ret->interfaces, (virHashDeallocator) virInterfaceFreeName); + virHashFree(ret->storagePools, (virHashDeallocator) virStoragePoolFreeName); + virHashFree(ret->storageVols, (virHashDeallocator) virStorageVolFreeName); + virHashFree(ret->nodeDevices, (virHashDeallocator) virNodeDeviceFree); + virHashFree(ret->secrets, (virHashDeallocator) virSecretFreeName); + virHashFree(ret->nwfilters, (virHashDeallocator) virNWFilterFreeName); virMutexDestroy(&ret->lock); VIR_FREE(ret); @@ -267,22 +259,14 @@ virReleaseConnect(virConnectPtr conn) { virMutexLock(&conn->lock); - if (conn->domains != NULL) - virHashFree(conn->domains, (virHashDeallocator) virDomainFreeName); - if (conn->networks != NULL) - virHashFree(conn->networks, (virHashDeallocator) virNetworkFreeName); - if (conn->interfaces != NULL) - virHashFree(conn->interfaces, (virHashDeallocator) virInterfaceFreeName); - if (conn->storagePools != NULL) - virHashFree(conn->storagePools, (virHashDeallocator) virStoragePoolFreeName); - if (conn->storageVols != NULL) - virHashFree(conn->storageVols, (virHashDeallocator) virStorageVolFreeName); - if (conn->nodeDevices != NULL) - virHashFree(conn->nodeDevices, (virHashDeallocator) virNodeDeviceFree); - if (conn->secrets != NULL) - virHashFree(conn->secrets, (virHashDeallocator) virSecretFreeName); - if (conn->nwfilters != NULL) - virHashFree(conn->nwfilters, (virHashDeallocator) virNWFilterFreeName); + virHashFree(conn->domains, (virHashDeallocator) virDomainFreeName); + virHashFree(conn->networks, (virHashDeallocator) virNetworkFreeName); + virHashFree(conn->interfaces, (virHashDeallocator) virInterfaceFreeName); + virHashFree(conn->storagePools, (virHashDeallocator) virStoragePoolFreeName); + virHashFree(conn->storageVols, (virHashDeallocator) virStorageVolFreeName); + virHashFree(conn->nodeDevices, (virHashDeallocator) virNodeDeviceFree); + virHashFree(conn->secrets, (virHashDeallocator) virSecretFreeName); + virHashFree(conn->nwfilters, (virHashDeallocator) virNWFilterFreeName); virResetError(&conn->err); @@ -435,8 +419,7 @@ virReleaseDomain(virDomainPtr domain) { domain->magic = -1; domain->id = -1; VIR_FREE(domain->name); - if (domain->snapshots != NULL) - virHashFree(domain->snapshots, (virHashDeallocator) virDomainSnapshotFreeName); + virHashFree(domain->snapshots, (virHashDeallocator) virDomainSnapshotFreeName); VIR_FREE(domain); if (conn) { diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 46da600..f0c1e9f 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1010,9 +1010,7 @@ qemuProcessWaitForMonitor(struct qemud_driver* driver, ret = qemuProcessFindCharDevicePTYsMonitor(vm, paths); cleanup: - if (paths) { - virHashFree(paths, qemuProcessFreePtyPath); - } + virHashFree(paths, qemuProcessFreePtyPath); if (kill(vm->pid, 0) == -1 && errno == ESRCH) { /* VM is dead, any other error raised in the interim is probably -- 1.7.4
[adding Jim to cc, as author of useless-if-before-free] On 02/17/2011 02:14 PM, Christophe Fergeau wrote:
virHashFree follows the convention described in HACKING that XXXFree() functions can be called with a NULL argument. --- src/conf/domain_conf.c | 6 +--- src/datatypes.c | 51 +++++++++++++++------------------------------- src/qemu/qemu_process.c | 4 +-- 3 files changed, 20 insertions(+), 41 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 351daf7..e7c3409 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -410,8 +410,7 @@ static void virDomainObjListDeallocator(void *payload, const char *name ATTRIBUT
void virDomainObjListDeinit(virDomainObjListPtr doms) { - if (doms->objs) - virHashFree(doms->objs, virDomainObjListDeallocator); + virHashFree(doms->objs, virDomainObjListDeallocator);
I tried adding --name=virHashFree to the useless_free_options variable in cfg.mk, to see if that would prevent regressions. However, it appears that this two-argument free-like function is not picked up by the heuristics in the useless-if-before-free script (it only works on one-argument functions). ACK to your patch as-is, so I pushed it. But I can't help but wonder if we could make this easier to enforce at 'make syntax-check' time by tweaking the perl script somehow. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Eric Blake wrote:
[adding Jim to cc, as author of useless-if-before-free]
On 02/17/2011 02:14 PM, Christophe Fergeau wrote:
virHashFree follows the convention described in HACKING that XXXFree() functions can be called with a NULL argument. --- src/conf/domain_conf.c | 6 +--- src/datatypes.c | 51 +++++++++++++++------------------------------- src/qemu/qemu_process.c | 4 +-- 3 files changed, 20 insertions(+), 41 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 351daf7..e7c3409 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -410,8 +410,7 @@ static void virDomainObjListDeallocator(void *payload, const char *name ATTRIBUT
void virDomainObjListDeinit(virDomainObjListPtr doms) { - if (doms->objs) - virHashFree(doms->objs, virDomainObjListDeallocator); + virHashFree(doms->objs, virDomainObjListDeallocator);
I tried adding --name=virHashFree to the useless_free_options variable in cfg.mk, to see if that would prevent regressions. However, it appears that this two-argument free-like function is not picked up by the heuristics in the useless-if-before-free script (it only works on one-argument functions).
ACK to your patch as-is, so I pushed it. But I can't help but wonder if we could make this easier to enforce at 'make syntax-check' time by tweaking the perl script somehow.
Right. useless-if-before-free deliberately detects only one-argument free-like functions. I'm reluctant to add such a change to that script because - there are comparatively few free-like functions with more than one argument, so there's not much anticipated gain - we don't know in general whether the free'd pointer arg is the first or second, so... - having to match against either the first or 2nd argument would greatly complicate the already complicated regular expressions If you're interested in pursuing it, I suggest switching tools altogether and using spatch. That would make for a much cleaner (albeit probably less efficient) script. It would also depend on a less-ubiquitous tool than perl.
On 02/18/2011 12:30 PM, Jim Meyering wrote:
- if (doms->objs) - virHashFree(doms->objs, virDomainObjListDeallocator); + virHashFree(doms->objs, virDomainObjListDeallocator);
I tried adding --name=virHashFree to the useless_free_options variable in cfg.mk, to see if that would prevent regressions. However, it appears that this two-argument free-like function is not picked up by the heuristics in the useless-if-before-free script (it only works on one-argument functions).
Right. useless-if-before-free deliberately detects only one-argument free-like functions.
An even better idea - as long as we're improving hash.h, why not fix things to pass the destructor in the Create call, rather than the Free call. Then freeing is a one-argument operation, using the destructor callback registered inside the virHash object, rather than a two-argument operation requiring the caller to pass in the destructor. At which point, we've defined away the problem of useless-if-before-free. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Fri, Feb 18, 2011 at 01:22:41PM -0700, Eric Blake wrote:
On 02/18/2011 12:30 PM, Jim Meyering wrote:
- if (doms->objs) - virHashFree(doms->objs, virDomainObjListDeallocator); + virHashFree(doms->objs, virDomainObjListDeallocator);
I tried adding --name=virHashFree to the useless_free_options variable in cfg.mk, to see if that would prevent regressions. However, it appears that this two-argument free-like function is not picked up by the heuristics in the useless-if-before-free script (it only works on one-argument functions).
Right. useless-if-before-free deliberately detects only one-argument free-like functions.
An even better idea - as long as we're improving hash.h, why not fix things to pass the destructor in the Create call, rather than the Free call. Then freeing is a one-argument operation, using the destructor callback registered inside the virHash object, rather than a two-argument operation requiring the caller to pass in the destructor. At which point, we've defined away the problem of useless-if-before-free.
Yes, that makes alot more sense as an API design Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (6)
-
Christophe Fergeau -
Christophe Fergeau -
Daniel P. Berrange -
Daniel Veillard -
Eric Blake -
Jim Meyering