On Thu, 2 Jun 2016 15:59:29 +0200 Michael Fritscher <michael.fritsc...@telematik-zentrum.de> wrote:
> Am 02.06.2016 um 11:42 schrieb Greg Kurz: > > On Thu, 2 Jun 2016 10:33:06 +0100 > > Peter Maydell <peter.mayd...@linaro.org> wrote: > > > >> On 2 June 2016 at 09:51, Greg Kurz <gk...@linux.vnet.ibm.com> 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; > } >