Patchwork [1/2] oslib-win32: add lock for gmtime_r()

login
register
mail settings
Submitter Wayne Xia
Date Jan. 9, 2013, 6:18 a.m.
Message ID <1357712290-16496-1-git-send-email-xiawenc@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/210628/
State New
Headers show

Comments

Wayne Xia - Jan. 9, 2013, 6:18 a.m.
Also changed the caller of gmtime() to gmtime_r().

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 oslib-win32.c |    6 +++++-
 vl.c          |    4 ++--
 2 files changed, 7 insertions(+), 3 deletions(-)
Paolo Bonzini - Jan. 9, 2013, 9:47 a.m.
Il 09/01/2013 07:18, Wenchao Xia ha scritto:
>   Also changed the caller of gmtime() to gmtime_r().

Should be a separate patch.

Also, what is the reason to add this lock?  They are already protected
by the BQL.

Paolo

> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  oslib-win32.c |    6 +++++-
>  vl.c          |    4 ++--
>  2 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/oslib-win32.c b/oslib-win32.c
> index e7e283e..74e0c52 100644
> --- a/oslib-win32.c
> +++ b/oslib-win32.c
> @@ -32,6 +32,8 @@
>  #include "trace.h"
>  #include "qemu/sockets.h"
>  
> +static GStaticMutex time_lock = G_STATIC_MUTEX_INIT;
> +
>  void *qemu_oom_check(void *ptr)
>  {
>      if (ptr == NULL) {
> @@ -74,15 +76,17 @@ void qemu_vfree(void *ptr)
>      VirtualFree(ptr, 0, MEM_RELEASE);
>  }
>  
> -/* FIXME: add proper locking */
> +/* FIXME: add proper locking in MinGW */
>  struct tm *gmtime_r(const time_t *timep, struct tm *result)
>  {
> +    g_static_mutex_lock(&time_lock);
>      struct tm *p = gmtime(timep);
>      memset(result, 0, sizeof(*result));
>      if (p) {
>          *result = *p;
>          p = result;
>      }
> +    g_static_mutex_unlock(&time_lock);
>      return p;
>  }
>  
> diff --git a/vl.c b/vl.c
> index 79e5122..8d7864c 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -454,12 +454,12 @@ void qemu_get_timedate(struct tm *tm, int offset)
>      ti += offset;
>      if (rtc_date_offset == -1) {
>          if (rtc_utc)
> -            ret = gmtime(&ti);
> +            ret = gmtime_r(&ti, tm);
>          else
>              ret = localtime(&ti);
>      } else {
>          ti -= rtc_date_offset;
> -        ret = gmtime(&ti);
> +        ret = gmtime_r(&ti, tm);
>      }
>  
>      memcpy(tm, ret, sizeof(struct tm));
>
Stefan Weil - Jan. 9, 2013, 5:06 p.m.
> Il 09/01/2013 07:18, Wenchao Xia ha scritto:
>>   Also changed the caller of gmtime() to gmtime_r().
>
> Should be a separate patch.

There is already a pending patch which replaces gmtime, localtime:

http://patchwork.ozlabs.org/patch/210250/

Stefan
Wayne Xia - Jan. 10, 2013, 3:01 a.m.
于 2013-1-9 17:47, Paolo Bonzini 写道:
> Il 09/01/2013 07:18, Wenchao Xia ha scritto:
>>    Also changed the caller of gmtime() to gmtime_r().
>
> Should be a separate patch.
>
> Also, what is the reason to add this lock?  They are already protected
> by the BQL.
>
> Paolo
>
  localtime_r() will be used in internal snapshot. I am not sure if it is
already protected by BQL, if yes I think this can be dropped but
add comments on API "need external lock".
   But with this lock API calling them can be tagged as "thread safe",
which would be much nicer.

>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   oslib-win32.c |    6 +++++-
>>   vl.c          |    4 ++--
>>   2 files changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/oslib-win32.c b/oslib-win32.c
>> index e7e283e..74e0c52 100644
>> --- a/oslib-win32.c
>> +++ b/oslib-win32.c
>> @@ -32,6 +32,8 @@
>>   #include "trace.h"
>>   #include "qemu/sockets.h"
>>
>> +static GStaticMutex time_lock = G_STATIC_MUTEX_INIT;
>> +
>>   void *qemu_oom_check(void *ptr)
>>   {
>>       if (ptr == NULL) {
>> @@ -74,15 +76,17 @@ void qemu_vfree(void *ptr)
>>       VirtualFree(ptr, 0, MEM_RELEASE);
>>   }
>>
>> -/* FIXME: add proper locking */
>> +/* FIXME: add proper locking in MinGW */
>>   struct tm *gmtime_r(const time_t *timep, struct tm *result)
>>   {
>> +    g_static_mutex_lock(&time_lock);
>>       struct tm *p = gmtime(timep);
>>       memset(result, 0, sizeof(*result));
>>       if (p) {
>>           *result = *p;
>>           p = result;
>>       }
>> +    g_static_mutex_unlock(&time_lock);
>>       return p;
>>   }
>>
>> diff --git a/vl.c b/vl.c
>> index 79e5122..8d7864c 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -454,12 +454,12 @@ void qemu_get_timedate(struct tm *tm, int offset)
>>       ti += offset;
>>       if (rtc_date_offset == -1) {
>>           if (rtc_utc)
>> -            ret = gmtime(&ti);
>> +            ret = gmtime_r(&ti, tm);
>>           else
>>               ret = localtime(&ti);
>>       } else {
>>           ti -= rtc_date_offset;
>> -        ret = gmtime(&ti);
>> +        ret = gmtime_r(&ti, tm);
>>       }
>>
>>       memcpy(tm, ret, sizeof(struct tm));
>>
>
Wayne Xia - Jan. 10, 2013, 3:03 a.m.
于 2013-1-10 1:06, Stefan Weil 写道:
>> Il 09/01/2013 07:18, Wenchao Xia ha scritto:
>>>    Also changed the caller of gmtime() to gmtime_r().
>>
>> Should be a separate patch.
>
> There is already a pending patch which replaces gmtime, localtime:
>
> http://patchwork.ozlabs.org/patch/210250/
>
> Stefan
>
>
   Should other calling to localtime be changed to localtime_r() also?
Stefan Weil - Jan. 10, 2013, 6:22 a.m.
>>> Il 09/01/2013 07:18, Wenchao Xia ha scritto:
>>>>    Also changed the caller of gmtime() to gmtime_r().
>>>
>>> Should be a separate patch.
>>
>> There is already a pending patch which replaces gmtime, localtime:
>>
>> http://patchwork.ozlabs.org/patch/210250/
>>
>> Stefan
>>
>>
>    Should other calling to localtime be changed to localtime_r() also?
>
> --
> Best Regards
>
> Wenchao Xia



With http://patchwork.ozlabs.org/patch/210240/ and
http://patchwork.ozlabs.org/patch/210250/ applied,
there remain no calls of localtime or gmtime which
need to be replaced.

Stefan W.

Patch

diff --git a/oslib-win32.c b/oslib-win32.c
index e7e283e..74e0c52 100644
--- a/oslib-win32.c
+++ b/oslib-win32.c
@@ -32,6 +32,8 @@ 
 #include "trace.h"
 #include "qemu/sockets.h"
 
+static GStaticMutex time_lock = G_STATIC_MUTEX_INIT;
+
 void *qemu_oom_check(void *ptr)
 {
     if (ptr == NULL) {
@@ -74,15 +76,17 @@  void qemu_vfree(void *ptr)
     VirtualFree(ptr, 0, MEM_RELEASE);
 }
 
-/* FIXME: add proper locking */
+/* FIXME: add proper locking in MinGW */
 struct tm *gmtime_r(const time_t *timep, struct tm *result)
 {
+    g_static_mutex_lock(&time_lock);
     struct tm *p = gmtime(timep);
     memset(result, 0, sizeof(*result));
     if (p) {
         *result = *p;
         p = result;
     }
+    g_static_mutex_unlock(&time_lock);
     return p;
 }
 
diff --git a/vl.c b/vl.c
index 79e5122..8d7864c 100644
--- a/vl.c
+++ b/vl.c
@@ -454,12 +454,12 @@  void qemu_get_timedate(struct tm *tm, int offset)
     ti += offset;
     if (rtc_date_offset == -1) {
         if (rtc_utc)
-            ret = gmtime(&ti);
+            ret = gmtime_r(&ti, tm);
         else
             ret = localtime(&ti);
     } else {
         ti -= rtc_date_offset;
-        ret = gmtime(&ti);
+        ret = gmtime_r(&ti, tm);
     }
 
     memcpy(tm, ret, sizeof(struct tm));