From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:43263) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TsGbc-00049c-56 for qemu-devel@nongnu.org; Mon, 07 Jan 2013 12:35:25 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TsGbY-0004FQ-Du for qemu-devel@nongnu.org; Mon, 07 Jan 2013 12:35:24 -0500 Received: from v220110690675601.yourvserver.net ([78.47.199.172]:44719) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TsGFw-0007M9-NT for qemu-devel@nongnu.org; Mon, 07 Jan 2013 12:13:00 -0500 Message-ID: <50EB0219.8040605@weilnetz.de> Date: Mon, 07 Jan 2013 18:12:57 +0100 From: Stefan Weil MIME-Version: 1.0 References: <1357543689-11415-1-git-send-email-xiawenc@linux.vnet.ibm.com> <1357543689-11415-5-git-send-email-xiawenc@linux.vnet.ibm.com> In-Reply-To: <1357543689-11415-5-git-send-email-xiawenc@linux.vnet.ibm.com> Content-Type: text/plain; charset=ISO-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH V2 04/10] oslib-win32: add lock for time functions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Wenchao Xia Cc: kwolf@redhat.com, aliguori@us.ibm.com, quintela@redhat.com, stefanha@gmail.com, qemu-devel@nongnu.org, lcapitulino@redhat.com, pbonzini@redhat.com, dietmar@proxmox.com 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 > --- > 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.