Re: [Qemu-devel] [PATCH] hw/9pfs: Add CephFS support in VirtFS

[Greg Kurz]

On Wed, 9 Mar 2016 13:09:58 -0700
Eric Blake <address@hidden> wrote:

> On 03/09/2016 12:02 PM, Greg Kurz wrote:
> > On Wed,  2 Mar 2016 23:41:43 +0800
> > Jevon Qiao <address@hidden> wrote:
> >   
> 
> >> +}
> >> +
> >> +static int cephfs_readdir_r(FsContext *ctx, V9fsFidOpenState *fs,
> >> +                            struct dirent *entry,
> >> +                            struct dirent **result)
> >> +{
> >> +    int ret;  
> 
> >> +    
> >> +    return ret;  
> > 
> > This function should behave like the original readdir_r() function from the
> > C library, but it doesn't.
> >   
> 
> readdir_r() is hopelessly broken.  POSIX is withdrawing it as such.
> http://austingroupbugs.net/view.php?id=696
> 
> readdir() should be all the more any sane program needs, because it
> should already be thread-safe.
> 

I wasn't aware that readdir_r() was so badly broken. It is currently
used here:

hw/9pfs/9p-handle.c:    return readdir_r(fs->dir, entry, result);
hw/9pfs/9p-local.c:    ret = readdir_r(fs->dir, entry, result);
hw/9pfs/9p-proxy.c:    return readdir_r(fs->dir, entry, result);

I'll see how we can move to readdir().

> > According to the the libcephfs.h header:
> > 
> >  * @returns 1 if the next entry was filled in, 0 if the end of the 
> > directory stream was reached,
> >  *          and a negative error code on failure.
> >  */
> > int ceph_readdir_r(struct ceph_mount_info *cmount, struct ceph_dir_result 
> > *dirp, struct dirent *de);
> > 
> > and the readdir_r() manual page says:
> > 
> >    The  readdir_r() function returns 0 on success.  On error, it returns a
> >    positive error number (listed under ERRORS).  If the end of the  direc‐
> >    tory  stream  is  reached,  readdir_r()  returns 0, and returns NULL in
> >    *result.  
> 
> readdir_r() can silently overflow buffers, with no recourse.  Its use
> should not be encouraged.
> 

Sure... this being said, fsdev currently exposes a readdir_r operation, not
a readdir one. Until we decide to change that, backends must follow the
readdir_r() API.

Cheers.

--
Greg