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

[Greg Kurz <groug@kaod.org>]

On Tue, 11 Mar 2025 12:13:06 +0100
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> 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 */
> }
> 

Yuck, I'll stick to the `if` version ;-)

No sure to understand the meaning of `/* POSIX invalid file handle */` though...

> >  
> > @@ -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);
> > 
> 
> 



-- 
Greg