Re: [Qemu-devel] [PATCH 4/5] virtiofsd: drop lo_dirp->fd field

[Dr. David Alan Gilbert]

* Stefan Hajnoczi (address@hidden) wrote:
> fdopendir(3) takes ownership of the file descriptor.  The presence of
> the lo_dirp->fd field could lead to someone incorrectly adding a
> close(d->fd) cleanup call in the future.
> 
> Do not store the file descriptor in struct lo_dirp since it is unused.
> 
> Signed-off-by: Stefan Hajnoczi <address@hidden>

Reviewed-by: Dr. David Alan Gilbert <address@hidden>


Thanks, applied; note:
  a) This looks like it can go into upstream libfuse
  b) I think we're probably leaking DIR *'s if we do an lo_shutdown;
I've added that to my todo

> ---
>  contrib/virtiofsd/passthrough_ll.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/contrib/virtiofsd/passthrough_ll.c 
> b/contrib/virtiofsd/passthrough_ll.c
> index c1500e092d..ad3abdd532 100644
> --- a/contrib/virtiofsd/passthrough_ll.c
> +++ b/contrib/virtiofsd/passthrough_ll.c
> @@ -1293,7 +1293,6 @@ static void lo_readlink(fuse_req_t req, fuse_ino_t ino)
>  }
>  
>  struct lo_dirp {
> -     int fd;
>       DIR *dp;
>       struct dirent *entry;
>       off_t offset;
> @@ -1319,16 +1318,17 @@ static void lo_opendir(fuse_req_t req, fuse_ino_t 
> ino, struct fuse_file_info *fi
>       struct lo_data *lo = lo_data(req);
>       struct lo_dirp *d;
>       ssize_t fh;
> +     int fd = -1;
>  
>       d = calloc(1, sizeof(struct lo_dirp));
>       if (d == NULL)
>               goto out_err;
>  
> -     d->fd = openat(lo_fd(req, ino), ".", O_RDONLY);
> -     if (d->fd == -1)
> +     fd = openat(lo_fd(req, ino), ".", O_RDONLY);
> +     if (fd == -1)
>               goto out_errno;
>  
> -     d->dp = fdopendir(d->fd);
> +     d->dp = fdopendir(fd);
>       if (d->dp == NULL)
>               goto out_errno;
>  
> @@ -1348,11 +1348,12 @@ static void lo_opendir(fuse_req_t req, fuse_ino_t 
> ino, struct fuse_file_info *fi
>  out_errno:
>       error = errno;
>  out_err:
> +     if (fd != -1) {
> +             close(fd);
> +     }
>       if (d) {
>               if (d->dp)
>                       closedir(d->dp);
> -             if (d->fd != -1)
> -                     close(d->fd);
>               free(d);
>       }
>       fuse_reply_err(req, error);
> -- 
> 2.21.0
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK