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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
