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

[Greg Kurz <groug@kaod.org>]

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