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
From bf1ee26dbca93caa15db70085ce51615c8e7986d Mon Sep 17 00:00:00 2001
From: Christophe Fergeau <teuf@xxxxxxxxx>
Date: Fri, 4 Feb 2011 19:28:33 +0100
Subject: [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
From c6b9d2f085a08c30047d17b41d20eafa86cc2ba5 Mon Sep 17 00:00:00 2001
From: Christophe Fergeau <teuf@xxxxxxxxx>
Date: Fri, 4 Feb 2011 19:29:27 +0100
Subject: [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
From 0b6a536fd0a5a9c3832e5afdefef2d0b7de8b4cc Mon Sep 17 00:00:00 2001
From: Christophe Fergeau <teuf@xxxxxxxxx>
Date: Fri, 4 Feb 2011 19:30:15 +0100
Subject: [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
From 819fb2a40c01ea6ec599f9a30adc0c90f9cbe79c Mon Sep 17 00:00:00 2001
From: Christophe Fergeau <teuf@xxxxxxxxx>
Date: Fri, 4 Feb 2011 21:20:08 +0100
Subject: [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
From 31a00849ea4d2a5a1f600c53181d9eb191ab6e03 Mon Sep 17 00:00:00 2001
From: Christophe Fergeau <teuf@xxxxxxxxx>
Date: Sat, 5 Feb 2011 13:25:36 +0100
Subject: [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
From e3e6b5827891c6282371c2d308bccf2d41d50fa6 Mon Sep 17 00:00:00 2001
From: Christophe Fergeau <teuf@xxxxxxxxx>
Date: Sun, 6 Feb 2011 22:48:50 +0100
Subject: [PATCH 6/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 59adf36..c3d2fbe 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -407,8 +407,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);
}
@@ -8746,8 +8745,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 21d7779..0c7c026 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list