From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:54466) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TsOvA-0003kX-6s for qemu-devel@nongnu.org; Mon, 07 Jan 2013 21:28:10 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TsOv9-0008VI-4Q for qemu-devel@nongnu.org; Mon, 07 Jan 2013 21:28:08 -0500 Received: from e23smtp04.au.ibm.com ([202.81.31.146]:36188) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TsOv8-0008V0-Iw for qemu-devel@nongnu.org; Mon, 07 Jan 2013 21:28:07 -0500 Received: from /spool/local by e23smtp04.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 8 Jan 2013 12:20:43 +1000 Received: from d23relay03.au.ibm.com (d23relay03.au.ibm.com [9.190.235.21]) by d23dlp01.au.ibm.com (Postfix) with ESMTP id 51F5C2CE804F for ; Tue, 8 Jan 2013 13:28:00 +1100 (EST) Received: from d23av04.au.ibm.com (d23av04.au.ibm.com [9.190.235.139]) by d23relay03.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r082RxMC49479812 for ; Tue, 8 Jan 2013 13:27:59 +1100 Received: from d23av04.au.ibm.com (loopback [127.0.0.1]) by d23av04.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r082Rxln028796 for ; Tue, 8 Jan 2013 13:28:00 +1100 Message-ID: <50EB842F.1060606@linux.vnet.ibm.com> Date: Tue, 08 Jan 2013 10:27:59 +0800 From: Wenchao Xia 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> <50EB0219.8040605@weilnetz.de> In-Reply-To: <50EB0219.8040605@weilnetz.de> 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: Stefan Weil 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. > 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