Re: [PATCH v4 04/11] 9p: darwin: Handle struct dirent differences

[Christian Schoenebeck <qemu_oss@crudebyte.com>]

On Montag, 7. Februar 2022 14:52:58 CET Will Cohen wrote:
> On Mon, Feb 7, 2022 at 4:53 AM Fabian Franz <fabianfranz.oss@gmail.com>
> 
> wrote:
> > Comments inline:
> > 
> > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> > 
> >> index 1a5e3eed73..7137a28109 100644
> >> --- a/hw/9pfs/9p-local.c
> >> +++ b/hw/9pfs/9p-local.c
> >> @@ -559,6 +559,15 @@ static struct dirent *local_readdir(FsContext *ctx,
> >> V9fsFidOpenState *fs)
> >> 
> >>  again:
> >>      entry = readdir(fs->dir.stream);
> >> 
> >> +#ifdef CONFIG_DARWIN
> >> +    int td;
> >> +    td = telldir(fs->dir.stream);
> > 
> > Maybe call this „off“?
> 
> Yes, off is better. Will adjust for v5.
> 
> >> +    /* If telldir fails, fail the entire readdir call */
> >> +    if (td < 0) {
> >> +        return NULL;
> >> +    }
> >> +    entry->d_seekoff = td;
> >> +#endif
> >> 
> >>      if (!entry) {
> >>      
> >>          return NULL;
> >>      
> >>      }
> > 
> > This needs to be before the #ifdef!
> 
> Good catch, will adjust for v5. I moved it around twice and forgot to put
> it in the right place.
> 
> >> diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c
> >> index b1664080d8..8b4b5cf7dc 100644
> >> --- a/hw/9pfs/9p-proxy.c
> >> +++ b/hw/9pfs/9p-proxy.c
> >> @@ -706,7 +706,21 @@ static off_t proxy_telldir(FsContext *ctx,
> >> V9fsFidOpenState *fs)
> >> 
> >>  static struct dirent *proxy_readdir(FsContext *ctx, V9fsFidOpenState
> >>  *fs)
> >>  {
> >> 
> >> -    return readdir(fs->dir.stream);
> >> +    struct dirent *entry;
> >> +    entry = readdir(fs->dir.stream);
> >> +#ifdef CONFIG_DARWIN
> >> +    if (!entry) {
> >> +        return NULL;
> >> +    }
> >> +    int td;
> >> +    td = telldir(fs->dir.stream);
> >> +    /* If telldir fails, fail the entire readdir call */
> >> +    if (td < 0) {
> >> +        return NULL;
> >> +    }
> >> +    entry->d_seekoff = td;
> >> +#endif
> >> +    return entry;
> >> 
> >>  }
> >>  
> >>  static void proxy_seekdir(FsContext *ctx, V9fsFidOpenState *fs, off_t
> >> 
> >> off)
> >> diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
> >> index 4a4a776d06..e264a03eef 100644
> >> --- a/hw/9pfs/9p-synth.c
> >> +++ b/hw/9pfs/9p-synth.c
> >> @@ -222,7 +222,11 @@ static void synth_direntry(V9fsSynthNode *node,
> >> 
> >>  {
> >>  
> >>      strcpy(entry->d_name, node->name);
> >>      entry->d_ino = node->attr->inode;
> >> 
> >> +#ifdef CONFIG_DARWIN
> >> +    entry->d_seekoff = off + 1;
> >> +#else
> >> 
> >>      entry->d_off = off + 1;
> >> 
> >> +#endif

See below ...

> >> 
> >>  }
> >>  
> >>  static struct dirent *synth_get_dentry(V9fsSynthNode *dir,
> >> 
> >> diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h
> >> index 546f46dc7d..accbec9987 100644
> >> --- a/hw/9pfs/9p-util.h
> >> +++ b/hw/9pfs/9p-util.h
> >> @@ -79,3 +79,20 @@ ssize_t fremovexattrat_nofollow(int dirfd, const char
> >> *filename,
> >> 
> >>                                  const char *name);
> >>  
> >>  #endif
> >> 
> >> +
> >> +
> >> +/**
> >> + * Darwin has d_seekoff, which appears to function similarly to d_off.
> >> + * However, it does not appear to be supported on all file systems,
> >> + * so ensure it is manually injected earlier and call here when
> >> + * needed.
> >> + */
> >> +
> >> +inline off_t qemu_dirent_off(struct dirent *dent)
> >> +{
> >> +#ifdef CONFIG_DARWIN
> >> +    return dent->d_seekoff;
> >> +#else
> >> +    return dent->d_off;
> >> +#endif
> >> +}
> > 
> > Are we sure we want a helper for two times the same ifdef? Deferring to
> > maintainers here however.
> 
> Either way works for me too -- my current inclination is to leave it this
> way (as originally suggested by the maintainers), if for no other reason
> than that it allows the one comment to be referenced in the case of both
> uses.

As requested by me in this v4, this will be 3 times in v5. So I assume that 
qualifies the dedicated helper function. :)

As an alternative you could make the helper function a macro instead, then you 
could use it in 9p-synth.c as well, which would make it 4 times. But I don't 
mind about that one.

Best regards,
Christian Schoenebeck

> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > 
> >> index 1563d7b7c6..cf694da354 100644
> >> --- a/hw/9pfs/9p.c
> >> +++ b/hw/9pfs/9p.c
> >> @@ -27,6 +27,7 @@
> >> 
> >>  #include "virtio-9p.h"
> >>  #include "fsdev/qemu-fsdev.h"
> >>  #include "9p-xattr.h"
> >> 
> >> +#include "9p-util.h"
> >> 
> >>  #include "coth.h"
> >>  #include "trace.h"
> >>  #include "migration/blocker.h"
> >> 
> >> @@ -2281,7 +2282,11 @@ static int coroutine_fn
> >> v9fs_do_readdir_with_stat(V9fsPDU *pdu,
> >> 
> >>          count += len;
> >>          v9fs_stat_free(&v9stat);
> >>          v9fs_path_free(&path);
> >> 
> >> -        saved_dir_pos = dent->d_off;
> >> +        saved_dir_pos = qemu_dirent_off(dent);
> >> +        if (saved_dir_pos < 0) {
> >> +            err = saved_dir_pos;
> >> +            break;
> >> +        }
> > 
> > Do we still need this error-handling? I had removed it in my interdiff
> > patch.
> 
> That's correct, it in fact can be removed. d_seekoff yields a __uint64_t (
> https://developer.apple.com/documentation/kernel/direntry/1415494-d_seekoff?
> language=objc). Will adjust for v5.
> 
> >>      }
> >>      
> >>      v9fs_readdir_unlock(&fidp->fs.dir);
> >> 
> >> @@ -2420,6 +2425,7 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU
> >> *pdu, V9fsFidState *fidp,
> >> 
> >>      V9fsString name;
> >>      int len, err = 0;
> >>      int32_t count = 0;
> >> 
> >> +    off_t off;
> >> 
> >>      struct dirent *dent;
> >>      struct stat *st;
> >>      struct V9fsDirEnt *entries = NULL;
> >> 
> >> @@ -2480,12 +2486,17 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU
> >> *pdu, V9fsFidState *fidp,
> >> 
> >>              qid.version = 0;
> >>          
> >>          }
> >> 
> >> +        off = qemu_dirent_off(dent);
> >> +        if (off < 0) {
> >> +            err = off;
> >> +            break;
> >> +        }
> > 
> > Same here - if this can never fail, why add the error handling?
> 
> See above.
> 
> >>          v9fs_string_init(&name);
> >>          v9fs_string_sprintf(&name, "%s", dent->d_name);
> >>          
> >>          /* 11 = 7 + 4 (7 = start offset, 4 = space for storing count) */
> >>          len = pdu_marshal(pdu, 11 + count, "Qqbs",
> >> 
> >> -                          &qid, dent->d_off,
> >> +                          &qid, off,
> >> 
> >>                            dent->d_type, &name);
> >>          
> >>          v9fs_string_free(&name);
> >> 
> >> diff --git a/hw/9pfs/codir.c b/hw/9pfs/codir.c
> >> index 032cce04c4..fac6759a64 100644
> >> --- a/hw/9pfs/codir.c
> >> +++ b/hw/9pfs/codir.c
> >> @@ -167,7 +167,14 @@ static int do_readdir_many(V9fsPDU *pdu,
> >> V9fsFidState *fidp,
> >> 
> >>          }
> >>          
> >>          size += len;
> >> 
> >> +        /* This conditional statement is identical in
> >> +         * function to qemu_dirent_off, described in 9p-util.h,
> >> +         * since that header cannot be included here. */
> >> +#ifdef CONFIG_DARWIN
> >> +        saved_dir_pos = dent->d_seekoff;
> >> +#else
> >> 
> >>          saved_dir_pos = dent->d_off;
> >> 
> >> +#endif
> >> 
> >>      }
> >>      
> >>      /* restore (last) saved position */
> >> 
> >> --
> >> 2.32.0 (Apple Git-132)