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

Submitted by Wayne Xia on Jan. 9, 2013, 6:18 a.m.

Details

Message ID 1357712290-16496-1-git-send-email-xiawenc@linux.vnet.ibm.com
State New
Headers show

Commit Message

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

Comments

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 hide | download patch | download mbox

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