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

[Stefan Weil <sw@weilnetz.de>]

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.