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

[Michael Fritscher]

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!

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. 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)

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)...

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;
}

--
ZfT - Zentrum für Telematik e.V.
Michael Fritscher
Magdalene-Schoch-Straße 5
97074 Würzburg
Tel: +49 (931) 615 633 - 57
Fax: +49 (931) 615 633 - 11
Email: address@hidden
Web: http://www.telematik-zentrum.de

Vorstand:
Prof. Dr. Klaus Schilling, Hans-Joachim Leistner
Sitz: Gerbrunn
USt.-ID Nr.: DE 257 244 580, Steuer-Nr.:  257/111/70203
Amtsgericht Würzburg, Vereinsregister-Nr.: VR 200 167

michael_fritscher.vcf
Description: Vcard

smime.p7s
Description: S/MIME Cryptographic Signature