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

[Greg Kurz <groug@kaod.org>]

On Tue, 11 Mar 2025 15:03:13 +0100
Greg Kurz <groug@kaod.org> wrote:

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

And this change is no longer needed if I go...

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

... that way :-)

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