On Montag, 7. Februar 2022 14:52:58 CET Will Cohen wrote:
> On Mon, Feb 7, 2022 at 4:53 AM Fabian Franz <fabianfranz....@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)
