On 04/06/2018 12:01 PM, Michal Privoznik wrote:
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.
Oh, I forgot to add: this function needs to be removed too testHashIter() because you're removing its only caller. diff --git i/tests/virhashtest.c w/tests/virhashtest.c index a013bc7169..e9c03c1afb 100644 --- i/tests/virhashtest.c +++ w/tests/virhashtest.c @@ -277,14 +277,6 @@ testHashSteal(const void *data ATTRIBUTE_UNUSED) } -static int -testHashIter(void *payload ATTRIBUTE_UNUSED, - const void *name ATTRIBUTE_UNUSED, - void *data ATTRIBUTE_UNUSED) -{ - return 0; -} - Michal