From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42440) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b8UqN-000231-Qj for qemu-devel@nongnu.org; Thu, 02 Jun 2016 11:47:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b8UqI-0000F9-PZ for qemu-devel@nongnu.org; Thu, 02 Jun 2016 11:47:35 -0400 Received: from e06smtp16.uk.ibm.com ([195.75.94.112]:60499) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b8UqI-0000Dg-FM for qemu-devel@nongnu.org; Thu, 02 Jun 2016 11:47:30 -0400 Received: from localhost by e06smtp16.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 2 Jun 2016 16:47:28 +0100 Received: from b06cxnps4075.portsmouth.uk.ibm.com (d06relay12.portsmouth.uk.ibm.com [9.149.109.197]) by d06dlp01.portsmouth.uk.ibm.com (Postfix) with ESMTP id 9BB6117D8042 for ; Thu, 2 Jun 2016 16:48:34 +0100 (BST) Received: from d06av11.portsmouth.uk.ibm.com (d06av11.portsmouth.uk.ibm.com [9.149.37.252]) by b06cxnps4075.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id u52FlRo360096580 for ; Thu, 2 Jun 2016 15:47:27 GMT Received: from d06av11.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av11.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id u52FlQ3D023274 for ; Thu, 2 Jun 2016 09:47:26 -0600 Date: Thu, 2 Jun 2016 17:47:23 +0200 From: Greg Kurz Message-ID: <20160602174723.37cc68c1@bahia.huguette.org> In-Reply-To: <9e0ab20f-0b5a-c393-2383-0de063c12290@telematik-zentrum.de> References: <146485751812.31047.11608773464731615044.stgit@bahia.huguette.org> <20160602114229.41c06a80@bahia.huguette.org> <9e0ab20f-0b5a-c393-2383-0de063c12290@telematik-zentrum.de> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 0/4] 9p: get rid of readdir_r() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Michael Fritscher Cc: Peter Maydell , "Aneesh Kumar K.V" , QEMU Developers On Thu, 2 Jun 2016 15:59:29 +0200 Michael Fritscher wrote: > Am 02.06.2016 um 11:42 schrieb Greg Kurz: > > On Thu, 2 Jun 2016 10:33:06 +0100 > > Peter Maydell wrote: > > > >> On 2 June 2016 at 09:51, Greg Kurz wrote: > >>> The readdir_r() function has a broken design and should not be used anymore. > >>> It is expected to be obsoleted in a future version of POSIX.1: > >>> > >>> http://austingroupbugs.net/view.php?id=696#c2857 > >>> > >>> Glibc has already announced that 2.24 (scheduled for August 2016) will > >>> deprecates readdir_r() and encourages people to use readdir() with > >>> external synchronization instead. > >> > >>> Since POSIX.1 will require readdir() to be thread-safe when employed on > >>> different directory streams, and glibc already does that, the choice > >>> was made to have per-directory locking. > >> > >> AIUI the argument is that all sensible implementations of readdir() > >> already provide the thread-safety guarantees POSIX is going to > >> specify, but have you tested this on one of the BSDs or OSX? > >> (and/or checked their current readdir implementation...) > >> > > > > No I haven't because "VirtFS is supported only on Linux" at the moment. > > > > But thanks for raising the flag: it reminds me that there's ongoing > > work to support VirtFS on win32 hosts and I should also Cc Michael > > Fritscher. > > Hi, > thanks for CCing me! > Hi Michael ! > I've digged a bit about the readdir<89 problem. > Following question: So we should relay on the thread safeness on > readdir() in the case of different directory streams? > > I looked into the MingW implementation of it (see below). If I see it > correctly it seems to be threadsafe on the case of different directory > streams as well. Cool. > But I didn't found any info about whether findfirst is > - but no warning regarding it on the MSDN > (https://msdn.microsoft.com/en-us/library/6tkkkc1y.aspx) > Heh the windows API is a bit similar to readdir_r(): the caller has a handle to pass to all find* calls. So it is likely to be reentrant, but we would need to see the source to be sure ;) > It is clearly NOT threadsafe using it on the same dir stream. > > I think we need to lock based on the pointer saved in V9fsFidOpenState > fs->dir . Do we have a way to archive something like this: > > lock(fs->dir); //if there is already a lock named the address stored in > fs->dir then wait > readdir(fs_dir,...); > unlock(fs->dir); > > ? Else we would have to track all the fs->dir, which is a bit of work > I'm afraid (some kind of hashset or similiar would be usefull then)... > This series gets rid of readdir_r() completely and already addresses all the locking around critical sections. So you don't have to implement your own version of readdir_r() anymore. Maybe you can rebase your patchset against: https://github.com/gkurz/qemu.git 9p-next Cheers. -- Greg > Best regards, > Michael Fritscher > > /* > * readdir > * > * Return a pointer to a dirent structure filled with the information > on the > * next entry in the directory. > */ > struct _tdirent * > _treaddir (_TDIR * dirp) > { > errno = 0; > > /* Check for valid DIR struct. */ > if (!dirp) > { > errno = EFAULT; > return (struct _tdirent *) 0; > } > > if (dirp->dd_stat < 0) > { > /* We have already returned all files in the directory > * (or the structure has an invalid dd_stat). */ > return (struct _tdirent *) 0; > } > else if (dirp->dd_stat == 0) > { > /* We haven't started the search yet. */ > /* Start the search */ > dirp->dd_handle = _tfindfirst (dirp->dd_name, &(dirp->dd_dta)); > > if (dirp->dd_handle == -1) > { > /* Whoops! Seems there are no files in that > * directory. */ > dirp->dd_stat = -1; > } > else > { > dirp->dd_stat = 1; > } > } > else > { > /* Get the next search entry. */ > if (_tfindnext (dirp->dd_handle, &(dirp->dd_dta))) > { > /* We are off the end or otherwise error. > _findnext sets errno to ENOENT if no more file > Undo this. */ > DWORD winerr = GetLastError (); > if (winerr == ERROR_NO_MORE_FILES) > errno = 0; > _findclose (dirp->dd_handle); > dirp->dd_handle = -1; > dirp->dd_stat = -1; > } > else > { > /* Update the status to indicate the correct > * number. */ > dirp->dd_stat++; > } > } > > if (dirp->dd_stat > 0) > { > /* Successfully got an entry. Everything about the file is > * already appropriately filled in except the length of the > * file name. */ > dirp->dd_dir.d_namlen = _tcslen (dirp->dd_dta.name); > _tcscpy (dirp->dd_dir.d_name, dirp->dd_dta.name); > return &dirp->dd_dir; > } > > return (struct _tdirent *) 0; > } >