Re: [Qemu-devel] [PATCH V2 04/10] oslib-win32: add lock for time functions

[Wenchao Xia <xiawenc@linux.vnet.ibm.com>]

 > Am 07.01.2013 08:28, schrieb Wenchao Xia:
>>    This patch adding lock for calling gmtime() and localtime()
>> on windows. If no other lib linked into qemu would call those
>> two function itself, then they are thread safe now on windows.
>>
>> Signed-off-by: Wenchao Xia<xiawenc@linux.vnet.ibm.com>
>> ---
>>   oslib-win32.c |   12 ++++++++++--
>>   1 files changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/oslib-win32.c b/oslib-win32.c
>> index e7e283e..344e3dd 100644
>> --- a/oslib-win32.c
>> +++ b/oslib-win32.c
>> @@ -74,27 +74,35 @@ void qemu_vfree(void *ptr)
>>       VirtualFree(ptr, 0, MEM_RELEASE);
>>   }
>>
>> -/* FIXME: add proper locking */
>> +/* WARN: if other lib call gmtime() itself, then it is not thread
>> safe. */
>>   struct tm *gmtime_r(const time_t *timep, struct tm *result)
>>   {
>> +    static GStaticMutex lock = G_STATIC_MUTEX_INIT;
>> +
>> +    g_static_mutex_lock(&lock);
>>       struct tm *p = gmtime(timep);
>>       memset(result, 0, sizeof(*result));
>>       if (p) {
>>           *result = *p;
>>           p = result;
>>       }
>> +    g_static_mutex_unlock(&lock);
>>       return p;
>>   }
>>
>> -/* FIXME: add proper locking */
>> +/* WARN: if other lib call localtime() itself, then it is not thread
>> safe. */
>>   struct tm *localtime_r(const time_t *timep, struct tm *result)
>>   {
>> +    static GStaticMutex lock = G_STATIC_MUTEX_INIT;
>> +
>> +    g_static_mutex_lock(&lock);
>>       struct tm *p = localtime(timep);
>>       memset(result, 0, sizeof(*result));
>>       if (p) {
>>           *result = *p;
>>           p = result;
>>       }
>> +    g_static_mutex_unlock(&lock);
>>       return p;
>>   }
>>
>
> Please remove this patch from the series:
>
> * It is unrelated to the other patches but tries to fix
>    a separate problem. Even if the code now calls localtime_r,
>    it is not worse than the old patch version which called
>    localtime for MinGW.
>
   OK.

> * localtime_r and gmtime_r use the same global storage,
>    so they must use a common mutex instead of one mutex
>    for each function.
>
   Thanks for tipping that, I did not know this before.

> * Even with a common mutex, the comment
>    "FIXME: add proper locking" still applies because there
>    is no common locking mechanism for asctime, ctime, gmtime
>    and localtime. I prefer FIXME or TODO comments for
>    code deficiencies, not WARN.
>
> The correct fix can only be done in MinGW / MinGW-w64.
>
   I think so, will keeps "FIXME" comments and change the code
calling gmtime()/localtime().

> Improving QEMU in a separate series with correct locking
> is ok if that series also removes the remaining
> calls of gmtime and localtime. Then the risk of
> reentrancy problems is reduced to a minimum.
>
> Regards
>
> Stefan W.
>


-- 
Best Regards

Wenchao Xia