Re: [libvirt] [PATCH v2] util: don't check for parallel iteration in hash-related functions
On 04/10/2018 08:27 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 | 83 ---------------------------------------------
2 files changed, 120 deletions(-)
This looks good. And further analysis of two call traces I raised in
review to v1 showed that we don't need to fix them. I mean, for the uml
case - the while uml driver is locked, so nobody else can access the
hash table while autodestroy is iterating over the hash table.
Similarly, in case of qemu the code relies on VM lock so again, it's
safe to remove entries from hash table.
In both cases current entries are removed from hash tables. In general,
it is not safe to remove 'random' entries while iterating.
I still think we need to rework those call traces I raised, but since
the code is safe the rework falls into 'nice to have' category.
However, in order to push you need to add SoB line into the commit
message to declare that you are Developer Certificate of Origin
compliant ( https://libvirt.org/hacking.html#patches point 6).
Michal