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

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

[Wenchao Xia]

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

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

[Qemu-devel] [PATCH 2/2] oslib-win32: add lock for localtime_r()

[Wenchao Xia]

  Also switch calling of localtime() to localtime_r() in qemu code.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 block.c       |   10 ----------
 block/vvfat.c |   14 +++++---------
 hw/omap1.c    |    2 +-
 oslib-win32.c |    4 +++-
 savevm.c      |    4 ++--
 vl.c          |    9 +++------
 6 files changed, 14 insertions(+), 29 deletions(-)

diff --git a/block.c b/block.c
index 4e28c55..60873ea 100644
--- a/block.c
+++ b/block.c
@@ -3338,11 +3338,7 @@ char *get_human_readable_size(char *buf, int buf_size, int64_t size)
 char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn)
 {
     char buf1[128], date_buf[128], clock_buf[128];
-#ifdef _WIN32
-    struct tm *ptm;
-#else
     struct tm tm;
-#endif
     time_t ti;
     int64_t secs;
 
@@ -3352,15 +3348,9 @@ char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn)
                  "ID", "TAG", "VM SIZE", "DATE", "VM CLOCK");
     } else {
         ti = sn->date_sec;
-#ifdef _WIN32
-        ptm = localtime(&ti);
-        strftime(date_buf, sizeof(date_buf),
-                 "%Y-%m-%d %H:%M:%S", ptm);
-#else
         localtime_r(&ti, &tm);
         strftime(date_buf, sizeof(date_buf),
                  "%Y-%m-%d %H:%M:%S", &tm);
-#endif
         secs = sn->vm_clock_nsec / 1000000000;
         snprintf(clock_buf, sizeof(clock_buf),
                  "%02d:%02d:%02d.%03d",
diff --git a/block/vvfat.c b/block/vvfat.c
index 83706ce..4a56fc6 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -527,15 +527,11 @@ static inline uint8_t fat_chksum(const direntry_t* entry)
 }
 
 /* if return_time==0, this returns the fat_date, else the fat_time */
-static uint16_t fat_datetime(time_t time,int return_time) {
-    struct tm* t;
-#ifdef _WIN32
-    t=localtime(&time); /* this is not thread safe */
-#else
-    struct tm t1;
-    t = &t1;
-    localtime_r(&time,t);
-#endif
+static uint16_t fat_datetime(time_t time, int return_time)
+{
+    struct tm *t, t1;
+
+    t = localtime_r(&time, &t1);
     if(return_time)
 	return cpu_to_le16((t->tm_sec/2)|(t->tm_min<<5)|(t->tm_hour<<11));
     return cpu_to_le16((t->tm_mday)|((t->tm_mon+1)<<5)|((t->tm_year-80)<<9));
diff --git a/hw/omap1.c b/hw/omap1.c
index 8536e96..e85f2e2 100644
--- a/hw/omap1.c
+++ b/hw/omap1.c
@@ -2830,7 +2830,7 @@ static void omap_rtc_tick(void *opaque)
         s->round = 0;
     }
 
-    memcpy(&s->current_tm, localtime(&s->ti), sizeof(s->current_tm));
+    localtime_r(&s->ti, &s->current_tm);
 
     if ((s->interrupts & 0x08) && s->ti == s->alarm_ti) {
         s->status |= 0x40;
diff --git a/oslib-win32.c b/oslib-win32.c
index 74e0c52..d96d053 100644
--- a/oslib-win32.c
+++ b/oslib-win32.c
@@ -90,15 +90,17 @@ struct tm *gmtime_r(const time_t *timep, struct tm *result)
     return p;
 }
 
-/* FIXME: add proper locking */
+/* FIXME: add proper locking in MinGW */
 struct tm *localtime_r(const time_t *timep, struct tm *result)
 {
+    g_static_mutex_lock(&time_lock);
     struct tm *p = localtime(timep);
     memset(result, 0, sizeof(*result));
     if (p) {
         *result = *p;
         p = result;
     }
+    g_static_mutex_unlock(&time_lock);
     return p;
 }
 
diff --git a/savevm.c b/savevm.c
index 529d60e..ce18762 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2095,7 +2095,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
     uint64_t vm_state_size;
 #ifdef _WIN32
     struct _timeb tb;
-    struct tm *ptm;
+    struct tm *ptm, tm;
 #else
     struct timeval tv;
     struct tm tm;
@@ -2151,7 +2151,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
     } else {
 #ifdef _WIN32
         time_t t = tb.time;
-        ptm = localtime(&t);
+        ptm = localtime_r(&t, &tm);
         strftime(sn->name, sizeof(sn->name), "vm-%Y%m%d%H%M%S", ptm);
 #else
         /* cast below needed for OpenBSD where tv_sec is still 'long' */
diff --git a/vl.c b/vl.c
index 8d7864c..e5da31c 100644
--- a/vl.c
+++ b/vl.c
@@ -448,21 +448,18 @@ StatusInfo *qmp_query_status(Error **errp)
 void qemu_get_timedate(struct tm *tm, int offset)
 {
     time_t ti;
-    struct tm *ret;
 
     time(&ti);
     ti += offset;
     if (rtc_date_offset == -1) {
         if (rtc_utc)
-            ret = gmtime_r(&ti, tm);
+            gmtime_r(&ti, tm);
         else
-            ret = localtime(&ti);
+            localtime_r(&ti, tm);
     } else {
         ti -= rtc_date_offset;
-        ret = gmtime_r(&ti, tm);
+        gmtime_r(&ti, tm);
     }
-
-    memcpy(tm, ret, sizeof(struct tm));
 }
 
 int qemu_timedate_diff(struct tm *tm)
-- 
1.7.1

Re: [Qemu-devel] [PATCH 1/2] oslib-win32: add lock for gmtime_r()

[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

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

Re: [Qemu-devel] [PATCH 1/2] oslib-win32: add lock for gmtime_r()

[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

Re: [Qemu-devel] [PATCH 1/2] oslib-win32: add lock for gmtime_r()

[Wenchao Xia]

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


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH 1/2] oslib-win32: add lock for gmtime_r()

[Wenchao Xia]

于 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?

-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH 1/2] oslib-win32: add lock for gmtime_r()

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