[v1] 9pfs: Fix ftruncate-after-unlink

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

Posted by Greg Kurz 3 weeks, 2 days ago

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;
     }
 
@@ -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;
+}
+
 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;
+}
+
 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,
 };
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);
-- 
2.48.1

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

Posted by Christian Schoenebeck 3 weeks, 2 days ago

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

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

Posted by Greg Kurz 3 weeks, 1 day ago

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

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

Posted by Christian Schoenebeck 3 weeks, 1 day ago

On Tuesday, March 11, 2025 3:03:13 PM CET Greg Kurz 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:
[...]
> > 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...

"invalid file descriptor" rather than "invalid file handle". What I mean is
that outside of the Unix world -1 is not necessarily the predefined and
reserved value for "invalid file descriptor".

But no big deal.

/Christian

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

Posted by Greg Kurz 3 weeks, 1 day ago

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

[PATCH 1/4] 9pfs: local : Introduce local_fid_fd() helper
[PATCH 2/4] 9pfs: Don't use file descriptors in core code
[PATCH 3/4] 9pfs: Introduce ftruncate file op
[PATCH 4/4] 9pfs: Introduce futimens file op