Re: [PATCH 2/4] 9pfs: Don't use file descriptors in core code

[Christian Schoenebeck <qemu_oss@crudebyte.com>]

On Monday, March 10, 2025 6:10:59 PM CET Greg Kurz wrote:
> v9fs_getattr() currently peeks into V9fsFidOpenState to know if a fid
> has a valid file descriptor or directory stream. Even though the fields
> are accessible, this is an implementation detail of the local backend
> that should not be manipulated directly by the server code.
> 
> Abstract that with a new has_valid_handle() backend operation.
> 
> Make the existing local_fid_fd() helper more robust so that it
> can cope with P9_FID_NONE or a null directory stream and reuse
> it.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  fsdev/file-op-9p.h |  1 +
>  hw/9pfs/9p-local.c | 12 +++++++++---
>  hw/9pfs/9p-synth.c |  6 ++++++
>  hw/9pfs/9p.c       |  9 ++++++---
>  4 files changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
> index 4997677460e8..39fee185f4ce 100644
> --- a/fsdev/file-op-9p.h
> +++ b/fsdev/file-op-9p.h
> @@ -164,6 +164,7 @@ struct FileOperations {
>      int (*renameat)(FsContext *ctx, V9fsPath *olddir, const char *old_name,
>                      V9fsPath *newdir, const char *new_name);
>      int (*unlinkat)(FsContext *ctx, V9fsPath *dir, const char *name, int flags);
> +    bool (*has_valid_handle)(int fid_type, V9fsFidOpenState *fs);
>  };
>  
>  #endif
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index c4366c867988..03e5304ef888 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -768,11 +768,11 @@ out:
>  
>  static int local_fid_fd(int fid_type, V9fsFidOpenState *fs)
>  {
> -    int fd;
> +    int fd = -1;
>  
> -    if (fid_type == P9_FID_DIR) {
> +    if (fid_type == P9_FID_DIR && fs->dir.stream != NULL) {
>          fd = dirfd(fs->dir.stream);
> -    } else {
> +    } else if (fid_type == P9_FID_FILE) {
>          fd = fs->fd;
>      }

Follow-up on previous patch, this could be reduced to:

static int local_fid_fd(int fid_type, V9fsFidOpenState *fs)
{
    if (fid_type == P9_FID_DIR && fs->dir.stream != NULL) {
        return dirfd(fs->dir.stream);
    } else if (fid_type == P9_FID_FILE) {
        return fs->fd;
    }
    return -1; /* POSIX invalid file handle */
}

or even:

static int local_fid_fd(int fid_type, V9fsFidOpenState *fs)
{
    return (fid_type == P9_FID_DIR && fs->dir.stream != NULL) ?
               dirfd(fs->dir.stream) :
                   (fid_type == P9_FID_FILE) ? fs->fd :
                       -1; /* POSIX invalid file handle */
}

>  
> @@ -1576,6 +1576,11 @@ static int local_parse_opts(QemuOpts *opts, FsDriverEntry *fse, Error **errp)
>      return 0;
>  }
>  
> +static bool local_has_valid_handle(int fid_type, V9fsFidOpenState *fs)
> +{
> +    return local_fid_fd(fid_type, fs) != -1;
> +}

I would avoid the implied dirfd() call here. It's usually just a libc function
on user side, so usually no context switch, but who knows if that applies to
all systems. So adapted to existing code this would be:

static bool local_has_valid_handle(int fid_type, V9fsFidOpenState *fs)
{
    return (fidp->fid_type == P9_FID_FILE && fidp->fs.fd != -1) ||
           (fidp->fid_type == P9_FID_DIR && fidp->fs.dir.stream != NULL);
}

>  FileOperations local_ops = {
>      .parse_opts = local_parse_opts,
>      .init  = local_init,
> @@ -1613,4 +1618,5 @@ FileOperations local_ops = {
>      .name_to_path = local_name_to_path,
>      .renameat  = local_renameat,
>      .unlinkat = local_unlinkat,
> +    .has_valid_handle = local_has_valid_handle,
>  };
> diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
> index 2abaf3a2918a..fa0b187a1b80 100644
> --- a/hw/9pfs/9p-synth.c
> +++ b/hw/9pfs/9p-synth.c
> @@ -615,6 +615,11 @@ static int synth_init(FsContext *ctx, Error **errp)
>      return 0;
>  }
>  
> +static bool synth_has_valid_handle(int fid_type, V9fsFidOpenState *fs)
> +{
> +    return false;
> +}
> +

I was worried that this would cause the synth tests to fail, but apparently it
does not break them. So fine.

>  FileOperations synth_ops = {
>      .init         = synth_init,
>      .lstat        = synth_lstat,
> @@ -650,4 +655,5 @@ FileOperations synth_ops = {
>      .name_to_path = synth_name_to_path,
>      .renameat     = synth_renameat,
>      .unlinkat     = synth_unlinkat,
> +    .has_valid_handle = synth_has_valid_handle,

Mabye rather has_valid_file_handle?

>  };
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 7cad2bce6209..969fb2f8c494 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -1574,6 +1574,11 @@ out_nofid:
>      pdu_complete(pdu, err);
>  }
>  
> +static bool fid_has_valid_handle(V9fsState *s, V9fsFidState *fidp)
> +{
> +    return s->ops->has_valid_handle(fidp->fid_type, &fidp->fs);
> +}
> +
>  static void coroutine_fn v9fs_getattr(void *opaque)
>  {
>      int32_t fid;
> @@ -1596,9 +1601,7 @@ static void coroutine_fn v9fs_getattr(void *opaque)
>          retval = -ENOENT;
>          goto out_nofid;
>      }
> -    if ((fidp->fid_type == P9_FID_FILE && fidp->fs.fd != -1) ||
> -        (fidp->fid_type == P9_FID_DIR && fidp->fs.dir.stream))
> -    {
> +    if (fid_has_valid_handle(pdu->s, fidp)) {
>          retval = v9fs_co_fstat(pdu, fidp, &stbuf);
>      } else {
>          retval = v9fs_co_lstat(pdu, &fidp->path, &stbuf);
>