4:39 a.m.
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
6:07 p.m.
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
3:45 p.m.
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
3:45 p.m.
New subject: [libvirt] [PATCH 1/8] use virReportErrorHelper instead of xmlGenericError
---
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
3:45 p.m.
New subject: [libvirt] [PATCH 2/8] fix OOM handling in hash.c
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
11:38 p.m.
New subject: [libvirt] [PATCH 2/8] fix OOM handling in hash.c
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
3:45 p.m.
New subject: [libvirt] [PATCH 3/8] add hash table rebalancing in virHashUpdateEntry
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
3:45 p.m.
New subject: [libvirt] [PATCH 4/8] factor common code in virHashAddEntry and virHashUpdateEntry
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
3:45 p.m.
New subject: [libvirt] [PATCH 5/8] call virReportOOMError when appropriate in hash.c
---
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
9 p.m.
New subject: [libvirt] [PATCH 5/8] call virReportOOMError when appropriate in hash.c
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
3:45 p.m.
New subject: [libvirt] [PATCH 6/8] remove no longer needed calls to virReportOOMError
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
3:45 p.m.
New subject: [libvirt] [PATCH 7/8] don't check for NULL before calling virHashFree
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
3:45 p.m.
New subject: [libvirt] [PATCH 8/8] add missing error handling to virGetDomain
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
9:07 p.m.
New subject: [libvirt] [PATCH 8/8] add missing error handling to virGetDomain
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
3:14 p.m.
New subject: [libvirt] virHash patches (resend)
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
3:14 p.m.
New subject: [libvirt] [PATCH 1/5] use virReportErrorHelper instead of xmlGenericError
---
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
5:44 p.m.
New subject: [libvirt] [PATCH 1/5] use virReportErrorHelper instead of xmlGenericError
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
3:14 p.m.
New subject: [libvirt] [PATCH 2/5] add hash table rebalancing in virHashUpdateEntry
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
5:50 p.m.
New subject: [libvirt] [PATCH 2/5] add hash table rebalancing in virHashUpdateEntry
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
3:14 p.m.
New subject: [libvirt] [PATCH 3/5] factor common code in virHashAddEntry and virHashUpdateEntry
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
5:50 p.m.
New subject: [libvirt] [PATCH 3/5] factor common code in virHashAddEntry and virHashUpdateEntry
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
3:14 p.m.
New subject: [libvirt] [PATCH 4/5] remove no longer needed calls to virReportOOMError
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
6:01 p.m.
New subject: [libvirt] [PATCH 4/5] remove no longer needed calls to virReportOOMError
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
3:14 p.m.
New subject: [libvirt] [PATCH 5/5] don't check for NULL before calling virHashFree
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
6:09 p.m.
New subject: [libvirt] [PATCH 5/5] don't check for NULL before calling virHashFree
[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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
1:30 p.m.
New subject: [libvirt] [PATCH 5/5] don't check for NULL before calling virHashFree
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.
2:22 p.m.
New subject: [libvirt] [PATCH 5/5] don't check for NULL before calling virHashFree
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
5:14 a.m.
New subject: [libvirt] [PATCH 5/5] don't check for NULL before calling virHashFree
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 :|
5023
days inactive
5033
days old
28 comments
6 participants
participants (6)
-
Christophe Fergeau -
Christophe Fergeau -
Daniel P. Berrange -
Daniel Veillard -
Eric Blake -
Jim Meyering