Re: [libvirt] [PATCH] util: don't check for parallel iteration in hash-related functions

2018-04-10 Thread Vincent Bernat 
 ❦  6 avril 2018 12:01 +0200, Michal Privoznik  :

> So I went through all callbacks (even transitive ones) and I've found
> two problems:
>
> umlProcessAutoDestroyRun -> umlProcessAutoDestroyDom -> virHashRemoveEntry
>
> qemuDomainSnapshotDiscardAllMetadata -> qemuDomainSnapshotDiscardAll ->
> qemuDomainSnapshotDiscard -> virDomainSnapshotObjListRemove ->
> virHashRemoveEntry
>
> While me (and probably Peter :-)) don't care about the first one, the
> second one is a real issue. I guess we need to fix that one before this
> can be merged.
>
> On a positive side, I haven't spotted any other problem. So once qemu
> (and possibly uml) are fixed this can be merged as is.

I updated the patch with the other small issue you noticed, but not this
one (didn't spot an immediate lock to use and got not time to dig
further).

FI, we didn't run into any problem so far and we are running a patched
libvirt on all our hypervisors (with QEMU).
-- 
Zounds!  I was never so bethumped with words
since I first called my brother's father dad.
-- William Shakespeare, "Kind John"

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] util: don't check for parallel iteration in hash-related functions

2018-04-06 Thread Michal Privoznik 
On 04/06/2018 09:56 AM, Vincent Bernat wrote:
> This is the responsability of the caller to apply the correct lock
> before using these functions. Moreover, the use of a simple boolean
> was still racy: two threads may check the boolean and "lock" it
> simultaneously.
> 
> Users of functions from src/util/virhash.c have to be checked for
> correctness. Lookups and iteration should hold a RO
> lock. Modifications should hold a RW lock.
> 
> Most important uses seem to be covered. Callers have now a greater
> responsability, notably the ability to execute some operations while
> iterating were reliably forbidden before are now accepted.
> ---
>  src/util/virhash.c  | 37 --
>  tests/virhashtest.c | 75 -
>  2 files changed, 112 deletions(-)

So I went through all callbacks (even transitive ones) and I've found
two problems:

umlProcessAutoDestroyRun -> umlProcessAutoDestroyDom -> virHashRemoveEntry

qemuDomainSnapshotDiscardAllMetadata -> qemuDomainSnapshotDiscardAll ->
qemuDomainSnapshotDiscard -> virDomainSnapshotObjListRemove ->
virHashRemoveEntry

While me (and probably Peter :-)) don't care about the first one, the
second one is a real issue. I guess we need to fix that one before this
can be merged.

On a positive side, I haven't spotted any other problem. So once qemu
(and possibly uml) are fixed this can be merged as is.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] util: don't check for parallel iteration in hash-related functions

2018-04-06 Thread Vincent Bernat 
This is the responsability of the caller to apply the correct lock
before using these functions. Moreover, the use of a simple boolean
was still racy: two threads may check the boolean and "lock" it
simultaneously.

Users of functions from src/util/virhash.c have to be checked for
correctness. Lookups and iteration should hold a RO
lock. Modifications should hold a RW lock.

Most important uses seem to be covered. Callers have now a greater
responsability, notably the ability to execute some operations while
iterating were reliably forbidden before are now accepted.
---
 src/util/virhash.c  | 37 --
 tests/virhashtest.c | 75 -
 2 files changed, 112 deletions(-)

diff --git a/src/util/virhash.c b/src/util/virhash.c
index 0ffbfcce2c64..475c2b0281b2 100644
--- a/src/util/virhash.c
+++ b/src/util/virhash.c
@@ -41,12 +41,6 @@ VIR_LOG_INIT("util.hash");
 
 /* #define DEBUG_GROW */
 
-#define virHashIterationError(ret) \
-do { \
-VIR_ERROR(_("Hash operation not allowed during iteration")); \
-return ret; \
-} while (0)
-
 /*
  * A single entry in the hash table
  */
@@ -66,10 +60,6 @@ struct _virHashTable {
 uint32_t seed;
 size_t size;
 size_t nbElems;
-/* True iff we are iterating over hash entries. */
-bool iterating;
-/* Pointer to the current entry during iteration. */
-virHashEntryPtr current;
 virHashDataFree dataFree;
 virHashKeyCode keyCode;
 virHashKeyEqual keyEqual;
@@ -339,9 +329,6 @@ virHashAddOrUpdateEntry(virHashTablePtr table, const void 
*name,
 if ((table == NULL) || (name == NULL))
 return -1;
 
-if (table->iterating)
-virHashIterationError(-1);
-
 key = virHashComputeKey(table, name);
 
 /* Check for duplicate entry */
@@ -551,9 +538,6 @@ virHashRemoveEntry(virHashTablePtr table, const void *name)
 nextptr = table->table + virHashComputeKey(table, name);
 for (entry = *nextptr; entry; entry = entry->next) {
 if (table->keyEqual(entry->name, name)) {
-if (table->iterating && table->current != entry)
-virHashIterationError(-1);
-
 if (table->dataFree)
 table->dataFree(entry->payload, entry->name);
 if (table->keyFree)
@@ -593,18 +577,11 @@ virHashForEach(virHashTablePtr table, virHashIterator 
iter, void *data)
 if (table == NULL || iter == NULL)
 return -1;
 
-if (table->iterating)
-virHashIterationError(-1);
-
-table->iterating = true;
-table->current = NULL;
 for (i = 0; i < table->size; i++) {
 virHashEntryPtr entry = table->table[i];
 while (entry) {
 virHashEntryPtr next = entry->next;
-table->current = entry;
 ret = iter(entry->payload, entry->name, data);
-table->current = NULL;
 
 if (ret < 0)
 goto cleanup;
@@ -615,7 +592,6 @@ virHashForEach(virHashTablePtr table, virHashIterator iter, 
void *data)
 
 ret = 0;
  cleanup:
-table->iterating = false;
 return ret;
 }
 
@@ -643,11 +619,6 @@ virHashRemoveSet(virHashTablePtr table,
 if (table == NULL || iter == NULL)
 return -1;
 
-if (table->iterating)
-virHashIterationError(-1);
-
-table->iterating = true;
-table->current = NULL;
 for (i = 0; i < table->size; i++) {
 virHashEntryPtr *nextptr = table->table + i;
 
@@ -667,7 +638,6 @@ virHashRemoveSet(virHashTablePtr table,
 }
 }
 }
-table->iterating = false;
 
 return count;
 }
@@ -723,23 +693,16 @@ void *virHashSearch(const virHashTable *ctable,
 if (table == NULL || iter == NULL)
 return NULL;
 
-if (table->iterating)
-virHashIterationError(NULL);
-
-table->iterating = true;
-table->current = NULL;
 for (i = 0; i < table->size; i++) {
 virHashEntryPtr entry;
 for (entry = table->table[i]; entry; entry = entry->next) {
 if (iter(entry->payload, entry->name, data)) {
-table->iterating = false;
 if (name)
 *name = table->keyCopy(entry->name);
 return entry->payload;
 }
 }
 }
-table->iterating = false;
 
 return NULL;
 }
diff --git a/tests/virhashtest.c b/tests/virhashtest.c
index 3b85b62c301d..a013bc716943 100644
--- a/tests/virhashtest.c
+++ b/tests/virhashtest.c
@@ -221,32 +221,6 @@ testHashRemoveForEachAll(void *payload ATTRIBUTE_UNUSED,
 }
 
 
-const int testHashCountRemoveForEachForbidden = ARRAY_CARDINALITY(uuids);
-
-static int
-testHashRemoveForEachForbidden(void *payload ATTRIBUTE_UNUSED,
-   const void *name,
-   void *data)
-{
-virHashTablePtr hash = data;
-size_t i;
-
-for (i = 0; i < ARRAY_CARDINALITY(uuids_subset); i++) {
-if (STREQ(uuids_subset[i], name)) {
-int next = (i + 1) %