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

[Wenchao Xia]

> 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<address@hidden>
> > ---
> >   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