libvirt users - Apr 2018 - Re: error : virHashForEach:597 : Hash operation not allowed during iteration

❦  4 avril 2018 15:19 +0200, Michal Privoznik <mprivozn@redhat.com> :
> Both threads call virHashForEach(table=0x7f92fc69a480). Thread 6 was
> first so it starts iterating and sets table->iterating so later when
> thread 10 enters the function an error is reported.
>
> I guess we can go with what Dan suggested and after some rework we can
> just drop ->iterating completely.
I may have missed this suggestion. Maybe Dan only sent it to you? In the
meantime, could I change the locks around virHashForEach() and similar
as read/write locks?
-- 
Nothing so needs reforming as other people's habits.
		-- Mark Twain, "Pudd'nhead Wilson's Calendar"

On 04/04/2018 03:48 PM, Vincent Bernat wrote:
>  ❦  4 avril 2018 15:19 +0200, Michal Privoznik
<mprivozn@redhat.com> :
> 
>> Both threads call virHashForEach(table=0x7f92fc69a480). Thread 6 was
>> first so it starts iterating and sets table->iterating so later when
>> thread 10 enters the function an error is reported.
>>
>> I guess we can go with what Dan suggested and after some rework we can
>> just drop ->iterating completely.
> 
> I may have missed this suggestion. Maybe Dan only sent it to you?
No, there is another thread where this issue is discussed:

https://www.redhat.com/archives/libvir-list/2018-April/msg00190.html

 In the
> meantime, could I change the locks around virHashForEach() and similar
> as read/write locks?
> 
You can do that locally, but as a patch it's very unlikely to be
accepted upstream because we've introduced RW locks to be able to access
domain list from multiple threads.

Michal

❦  4 avril 2018 15:59 +0200, Michal Privoznik <mprivozn@redhat.com> :
>> I may have missed this suggestion. Maybe Dan only sent it to you?
>
> No, there is another thread where this issue is discussed:
>
> https://www.redhat.com/archives/libvir-list/2018-April/msg00190.html
>
>  In the
>> meantime, could I change the locks around virHashForEach() and similar
>> as read/write locks?
>> 
>
> You can do that locally, but as a patch it's very unlikely to be
> accepted upstream because we've introduced RW locks to be able to
access
> domain list from multiple threads.
Looking a bit more, the whole "iterating" bit is currently
unsafe. First, it is racy since it's "check then take". Two
threads may
check the value is false and start iterating both. Moreover, at some
places, it is not set, for example in virHashAddOrUpdateEntry(). So, it
would be possible to call this function and during the update to iterate
over the hash. This seems to be prevented only by callers using RW
lock. So, it seems we can just remove this "iterating" bool and just
check all users of these functions are using the appropriate lock.
-- 
Make sure all variables are initialised before use.
            - The Elements of Programming Style (Kernighan & Plauger)