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

[PATCH 1/4] 9pfs: local : Introduce local_fid_fd() helper

Posted by Greg Kurz 3 weeks, 2 days ago

Factor out duplicated code to a single helper. More users to come.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 928523afcc6c..c4366c867988 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -766,10 +766,9 @@ out:
     return err;
 }
 
-static int local_fstat(FsContext *fs_ctx, int fid_type,
-                       V9fsFidOpenState *fs, struct stat *stbuf)
+static int local_fid_fd(int fid_type, V9fsFidOpenState *fs)
 {
-    int err, fd;
+    int fd;
 
     if (fid_type == P9_FID_DIR) {
         fd = dirfd(fs->dir.stream);
@@ -777,6 +776,14 @@ static int local_fstat(FsContext *fs_ctx, int fid_type,
         fd = fs->fd;
     }
 
+    return fd;
+}
+
+static int local_fstat(FsContext *fs_ctx, int fid_type,
+                       V9fsFidOpenState *fs, struct stat *stbuf)
+{
+    int err, fd = local_fid_fd(fid_type, fs);
+
     err = fstat(fd, stbuf);
     if (err) {
         return err;
@@ -1167,13 +1174,7 @@ out:
 static int local_fsync(FsContext *ctx, int fid_type,
                        V9fsFidOpenState *fs, int datasync)
 {
-    int fd;
-
-    if (fid_type == P9_FID_DIR) {
-        fd = dirfd(fs->dir.stream);
-    } else {
-        fd = fs->fd;
-    }
+    int fd = local_fid_fd(fid_type, fs);
 
     if (datasync) {
         return qemu_fdatasync(fd);
-- 
2.48.1

Re: [PATCH 1/4] 9pfs: local : Introduce local_fid_fd() helper

Posted by Christian Schoenebeck 3 weeks, 2 days ago

On Monday, March 10, 2025 6:10:58 PM CET Greg Kurz wrote:
> Factor out duplicated code to a single helper. More users to come.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/9pfs/9p-local.c | 21 +++++++++++----------
>  1 file changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index 928523afcc6c..c4366c867988 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -766,10 +766,9 @@ out:
>      return err;
>  }
>  
> -static int local_fstat(FsContext *fs_ctx, int fid_type,
> -                       V9fsFidOpenState *fs, struct stat *stbuf)
> +static int local_fid_fd(int fid_type, V9fsFidOpenState *fs)
>  {
> -    int err, fd;
> +    int fd;
>  
>      if (fid_type == P9_FID_DIR) {
>          fd = dirfd(fs->dir.stream);
> @@ -777,6 +776,14 @@ static int local_fstat(FsContext *fs_ctx, int fid_type,
>          fd = fs->fd;
>      }
>  
> +    return fd;
> +}

Maybe simplifying this like:

static int local_fid_fd(int fid_type, V9fsFidOpenState *fs)
{
    if (fid_type == P9_FID_DIR) {
        return dirfd(fs->dir.stream);
    } else {
        return fs->fd;
    }
}

or even just:

static int local_fid_fd(int fid_type, V9fsFidOpenState *fs)
{
    return (fid_type == P9_FID_DIR) ? dirfd(fs->dir.stream) : return fs->fd;
}

/Christian

> +
> +static int local_fstat(FsContext *fs_ctx, int fid_type,
> +                       V9fsFidOpenState *fs, struct stat *stbuf)
> +{
> +    int err, fd = local_fid_fd(fid_type, fs);
> +
>      err = fstat(fd, stbuf);
>      if (err) {
>          return err;
> @@ -1167,13 +1174,7 @@ out:
>  static int local_fsync(FsContext *ctx, int fid_type,
>                         V9fsFidOpenState *fs, int datasync)
>  {
> -    int fd;
> -
> -    if (fid_type == P9_FID_DIR) {
> -        fd = dirfd(fs->dir.stream);
> -    } else {
> -        fd = fs->fd;
> -    }
> +    int fd = local_fid_fd(fid_type, fs);
>  
>      if (datasync) {
>          return qemu_fdatasync(fd);
>

Re: [PATCH 1/4] 9pfs: local : Introduce local_fid_fd() helper

Posted by Greg Kurz 3 weeks, 1 day ago

On Tue, 11 Mar 2025 11:58:28 +0100
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Monday, March 10, 2025 6:10:58 PM CET Greg Kurz wrote:
> > Factor out duplicated code to a single helper. More users to come.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  hw/9pfs/9p-local.c | 21 +++++++++++----------
> >  1 file changed, 11 insertions(+), 10 deletions(-)
> > 
> > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> > index 928523afcc6c..c4366c867988 100644
> > --- a/hw/9pfs/9p-local.c
> > +++ b/hw/9pfs/9p-local.c
> > @@ -766,10 +766,9 @@ out:
> >      return err;
> >  }
> >  
> > -static int local_fstat(FsContext *fs_ctx, int fid_type,
> > -                       V9fsFidOpenState *fs, struct stat *stbuf)
> > +static int local_fid_fd(int fid_type, V9fsFidOpenState *fs)
> >  {
> > -    int err, fd;
> > +    int fd;
> >  
> >      if (fid_type == P9_FID_DIR) {
> >          fd = dirfd(fs->dir.stream);
> > @@ -777,6 +776,14 @@ static int local_fstat(FsContext *fs_ctx, int fid_type,
> >          fd = fs->fd;
> >      }
> >  
> > +    return fd;
> > +}
> 
> Maybe simplifying this like:
> 
> static int local_fid_fd(int fid_type, V9fsFidOpenState *fs)
> {
>     if (fid_type == P9_FID_DIR) {
>         return dirfd(fs->dir.stream);
>     } else {
>         return fs->fd;
>     }
> }
> 
> or even just:
> 
> static int local_fid_fd(int fid_type, V9fsFidOpenState *fs)
> {
>     return (fid_type == P9_FID_DIR) ? dirfd(fs->dir.stream) : return fs->fd;
> }
> 

I'll go for you suggestion with the `if`. It is clearer than the ternary
expression and it is easier to put a breakpoint. I'm pretty sure all three
result in the same assembly code anyway.

> /Christian
> 
> > +
> > +static int local_fstat(FsContext *fs_ctx, int fid_type,
> > +                       V9fsFidOpenState *fs, struct stat *stbuf)
> > +{
> > +    int err, fd = local_fid_fd(fid_type, fs);
> > +
> >      err = fstat(fd, stbuf);
> >      if (err) {
> >          return err;
> > @@ -1167,13 +1174,7 @@ out:
> >  static int local_fsync(FsContext *ctx, int fid_type,
> >                         V9fsFidOpenState *fs, int datasync)
> >  {
> > -    int fd;
> > -
> > -    if (fid_type == P9_FID_DIR) {
> > -        fd = dirfd(fs->dir.stream);
> > -    } else {
> > -        fd = fs->fd;
> > -    }
> > +    int fd = local_fid_fd(fid_type, fs);
> >  
> >      if (datasync) {
> >          return qemu_fdatasync(fd);
> > 
> 
> 



-- 
Greg