Re: [Qemu-devel] [PATCH 0/4] 9p: get rid of readdir_r()

[Greg Kurz]

On Thu, 2 Jun 2016 15:59:29 +0200
Michael Fritscher <address@hidden> wrote:

> Am 02.06.2016 um 11:42 schrieb Greg Kurz:
> > On Thu, 2 Jun 2016 10:33:06 +0100
> > Peter Maydell <address@hidden> wrote:
> >  
> >> On 2 June 2016 at 09:51, Greg Kurz <address@hidden> 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;
> }
>