Re: [libvirt] [PATCH V4 01/10] Extend virHashTable with function to get hash tables keys
On 10/26/2011 05:36 AM, Stefan Berger wrote:
Add a function to the virHashTable for getting an array of the hash table's
keys and have the keys (optionally) sorted.
Signed-off-by: Stefan Berger stef...@linux.vnet.ibm.com
---
src/libvirt_private.syms |3 +
src/util/hash.c | 98
+++
src/util/hash.h | 14 ++
3 files changed, 115 insertions(+)
Reordering for review purposes:
Index: libvirt-acl/src/util/hash.h
===
--- libvirt-acl.orig/src/util/hash.h
+++ libvirt-acl/src/util/hash.h
@@ -130,6 +130,20 @@ void *virHashLookup(virHashTablePtr tabl
*/
void *virHashSteal(virHashTablePtr table, const void *name);
+/*
+ * Get the set of keys and have them optionally sorted.
+ */
+typedef struct _virHashKeyValuePair virHashKeyValuePair;
+typedef virHashKeyValuePair *virHashKeyValuePairPtr;
+struct _virHashKeyValuePair {
+void *key;
Why isn't this 'const void *key'?
+const void *value;
+};
+typedef int (*virHashKeyComparator)(const virHashKeyValuePairPtr ,
+const virHashKeyValuePairPtr );
+void **virHashGetKeys(virHashTablePtr table, virHashKeyComparator
compar);
So the caller only knows how large the returned array is by calling
virHashSize? I take it passing NULL for compar is what gets the keys in
an unsorted order.
Should this return 'const void **', that is, an array that can be
modified, but whose elements are const void * pointers into the hash
table, where the keys are not modifiable through this array view?
+void virHashFreeKeys(virHashTablePtr table, void **);
I'm not sure we need this. It seems like the array returned by
virHashGetKeys should be able to just pass directly to VIR_FREE, without
reference to which table it was copied from, since all the elements of
the array are just pointers into the hash table. I guess I could see
using this if you are cloning keys in the process of creating the array,
but I'm not sure that cloning keys is worth it.
* Iterators
Index: libvirt-acl/src/libvirt_private.syms
===
--- libvirt-acl.orig/src/libvirt_private.syms
+++ libvirt-acl/src/libvirt_private.syms
@@ -559,6 +559,8 @@ virHashAddEntry;
virHashCreate;
virHashForEach;
virHashFree;
+virHashFreeKeys;
This line might not be needed, per my above questioning.
+virHashGetKeys;
virHashLookup;
virHashRemoveEntry;
virHashRemoveSet;
@@ -566,6 +568,7 @@ virHashSearch;
virHashSize;
virHashSteal;
virHashTableSize;
+virHashUpdateEntry;
Did we really have this one missing?
+++ libvirt-acl/src/util/hash.c
@@ -618,3 +618,101 @@ void *virHashSearch(virHashTablePtr tabl
return NULL;
}
+
+
+struct getKeysIter
+{
+virHashTablePtr table;
+void **array;
+virHashKeyValuePair *sortArray;
+unsigned arrayIdx;
+bool error;
If you go with my idea of VIR_FREE() to clean up the returned array,
then you don't need bool error.
+};
+
+static void virHashGetKeysIterator(void *payload,
+ const void *name, void *data)
+{
+struct getKeysIter *iter = data;
+void *key;
+
+if (iter-error)
+return;
+
+key = iter-table-keyCopy(name);
+
+if (key == NULL) {
+virReportOOMError();
+iter-error = true;
+return;
+}
These statements disappear, replaced by:
key = name;
(const-correcness may require that you use const void * in more places).
+
+if (iter-sortArray) {
+iter-sortArray[iter-arrayIdx].key = key;
+iter-sortArray[iter-arrayIdx].value = payload;
+} else {
+iter-array[iter-arrayIdx] = iter-table-keyCopy(name);
and this becomes iter-array[iter-arrayIdx] = key;
+}
+iter-arrayIdx++;
+}
+
+void virHashFreeKeys(virHashTablePtr table, void **keys)
+{
+unsigned i = 0;
+
+if (keys == NULL)
+return;
+
+while (keys[i])
+table-keyFree(keys[i++]);
+
+VIR_FREE(keys);
+}
then this function disappears.
+
+typedef int (*qsort_comp)(const void *, const void *);
+
+void **virHashGetKeys(virHashTablePtr table,
+ virHashKeyComparator compar)
This function needs better documentation; compare to virHashForEach to
see an example. Mention that the returned array must be passed to VIR_FREE
+{
+int i, numElems = virHashSize(table);
+struct getKeysIter iter = {
+.table = table,
+.arrayIdx = 0,
+.error = false,
You need to explicitly initialize .sortArray to NULL, otherwise,
virHashGetKeysIterator will see it as uninitialized garbage, and may
attempt to store into .sortArray instead of the intended .array.
+};
+
+if (numElems 0) {
+return NULL;
+}
+
+if (VIR_ALLOC_N(iter.array, numElems + 1)) {
+
Re: [libvirt] [PATCH V4 01/10] Extend virHashTable with function to get hash tables keys
On 11/16/2011 07:32 PM, Eric Blake wrote:
On 10/26/2011 05:36 AM, Stefan Berger wrote:
Add a function to the virHashTable for getting an array of the hash table's
keys and have the keys (optionally) sorted.
Signed-off-by: Stefan Bergerstef...@linux.vnet.ibm.com
---
src/libvirt_private.syms |3 +
src/util/hash.c | 98
+++
src/util/hash.h | 14 ++
3 files changed, 115 insertions(+)
Reordering for review purposes:
Index: libvirt-acl/src/util/hash.h
===
--- libvirt-acl.orig/src/util/hash.h
+++ libvirt-acl/src/util/hash.h
@@ -130,6 +130,20 @@ void *virHashLookup(virHashTablePtr tabl
*/
void *virHashSteal(virHashTablePtr table, const void *name);
+/*
+ * Get the set of keys and have them optionally sorted.
+ */
+typedef struct _virHashKeyValuePair virHashKeyValuePair;
+typedef virHashKeyValuePair *virHashKeyValuePairPtr;
+struct _virHashKeyValuePair {
+void *key;
Why isn't this 'const void *key'?
My bad.
I converted everything. It now looks like this.
*
* Get the set of keys and have them optionally sorted.
*/
typedef struct _virHashKeyValuePair virHashKeyValuePair;
typedef virHashKeyValuePair *virHashKeyValuePairPtr;
struct _virHashKeyValuePair {
const void *key;
const void *value;
};
typedef int (*virHashKeyComparator)(const virHashKeyValuePairPtr ,
const virHashKeyValuePairPtr );
virHashKeyValuePairPtr virHashGetItems(virHashTablePtr table,
virHashKeyComparator compar);
I hope this addresses your concerns. I do need the sorting and left it
in there without
wanting to either wrap it or offload it into the caller - if that's ok.
Stefan
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH V4 01/10] Extend virHashTable with function to get hash tables keys
Add a function to the virHashTable for getting an array of the hash table's
keys and have the keys (optionally) sorted.
Signed-off-by: Stefan Berger stef...@linux.vnet.ibm.com
---
src/libvirt_private.syms |3 +
src/util/hash.c | 98 +++
src/util/hash.h | 14 ++
3 files changed, 115 insertions(+)
Index: libvirt-acl/src/util/hash.c
===
--- libvirt-acl.orig/src/util/hash.c
+++ libvirt-acl/src/util/hash.c
@@ -618,3 +618,101 @@ void *virHashSearch(virHashTablePtr tabl
return NULL;
}
+
+
+struct getKeysIter
+{
+virHashTablePtr table;
+void **array;
+virHashKeyValuePair *sortArray;
+unsigned arrayIdx;
+bool error;
+};
+
+static void virHashGetKeysIterator(void *payload,
+ const void *name, void *data)
+{
+struct getKeysIter *iter = data;
+void *key;
+
+if (iter-error)
+return;
+
+key = iter-table-keyCopy(name);
+
+if (key == NULL) {
+virReportOOMError();
+iter-error = true;
+return;
+}
+
+if (iter-sortArray) {
+iter-sortArray[iter-arrayIdx].key = key;
+iter-sortArray[iter-arrayIdx].value = payload;
+} else {
+iter-array[iter-arrayIdx] = iter-table-keyCopy(name);
+}
+iter-arrayIdx++;
+}
+
+void virHashFreeKeys(virHashTablePtr table, void **keys)
+{
+unsigned i = 0;
+
+if (keys == NULL)
+return;
+
+while (keys[i])
+table-keyFree(keys[i++]);
+
+VIR_FREE(keys);
+}
+
+typedef int (*qsort_comp)(const void *, const void *);
+
+void **virHashGetKeys(virHashTablePtr table,
+ virHashKeyComparator compar)
+{
+int i, numElems = virHashSize(table);
+struct getKeysIter iter = {
+.table = table,
+.arrayIdx = 0,
+.error = false,
+};
+
+if (numElems 0) {
+return NULL;
+}
+
+if (VIR_ALLOC_N(iter.array, numElems + 1)) {
+virReportOOMError();
+return NULL;
+}
+
+/* null-terminate array */
+iter.array[numElems] = NULL;
+
+if (compar
+VIR_ALLOC_N(iter.sortArray, numElems)) {
+VIR_FREE(iter.array);
+virReportOOMError();
+return NULL;
+}
+
+virHashForEach(table, virHashGetKeysIterator, iter);
+if (iter.error) {
+VIR_FREE(iter.sortArray);
+virHashFreeKeys(table, iter.array);
+return NULL;
+}
+
+if (compar) {
+qsort(iter.sortArray[0], numElems, sizeof(iter.sortArray[0]),
+ (qsort_comp)compar);
+for (i = 0; i numElems; i++)
+iter.array[i] = iter.sortArray[i].key;
+VIR_FREE(iter.sortArray);
+}
+
+return iter.array;
+}
Index: libvirt-acl/src/util/hash.h
===
--- libvirt-acl.orig/src/util/hash.h
+++ libvirt-acl/src/util/hash.h
@@ -130,6 +130,20 @@ void *virHashLookup(virHashTablePtr tabl
*/
void *virHashSteal(virHashTablePtr table, const void *name);
+/*
+ * Get the set of keys and have them optionally sorted.
+ */
+typedef struct _virHashKeyValuePair virHashKeyValuePair;
+typedef virHashKeyValuePair *virHashKeyValuePairPtr;
+struct _virHashKeyValuePair {
+void *key;
+const void *value;
+};
+typedef int (*virHashKeyComparator)(const virHashKeyValuePairPtr ,
+const virHashKeyValuePairPtr );
+void **virHashGetKeys(virHashTablePtr table, virHashKeyComparator compar);
+void virHashFreeKeys(virHashTablePtr table, void **);
+
/*
* Iterators
Index: libvirt-acl/src/libvirt_private.syms
===
--- libvirt-acl.orig/src/libvirt_private.syms
+++ libvirt-acl/src/libvirt_private.syms
@@ -559,6 +559,8 @@ virHashAddEntry;
virHashCreate;
virHashForEach;
virHashFree;
+virHashFreeKeys;
+virHashGetKeys;
virHashLookup;
virHashRemoveEntry;
virHashRemoveSet;
@@ -566,6 +568,7 @@ virHashSearch;
virHashSize;
virHashSteal;
virHashTableSize;
+virHashUpdateEntry;
# hooks.h
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
