[V2,04/10] oslib-win32: add lock for time functions

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(-)

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.

* localtime_r and gmtime_r use the same global storage,
   so they must use a common mutex instead of one mutex
   for each function.

* 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.

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.

> 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.
>