On Mon, Feb 7, 2022 at 9:41 AM Christian Schoenebeck <qemu_...@crudebyte.com>
wrote:
> On Sonntag, 6. Februar 2022 21:07:12 CET Will Cohen wrote:
> > From: Keno Fischer <k...@juliacomputing.com>
> >
> > On darwin d_seekoff exists, but is optional and does not seem to
> > be commonly used by file systems. Use `telldir` instead to obtain
> > the seek offset and inject it into d_seekoff, and create a
> > qemu_dirent_off helper to call it appropriately when appropriate.
> >
> > Signed-off-by: Keno Fischer <k...@juliacomputing.com>
> > [Michael Roitzsch: - Rebase for NixOS]
> > Signed-off-by: Michael Roitzsch <reactorcont...@icloud.com>
> > [Will Cohen: - Adjust to pass testing
> > - Ensure that d_seekoff is filled using telldir
> > on darwin, and create qemu_dirent_off helper
> > to decide which to access]
> > [Fabian Franz: - Add telldir error handling for darwin]
> > [Will Cohen: - Ensure that telldir error handling uses
> > signed int]
> > Signed-off-by: Fabian Franz <fabianfranz....@gmail.com>
> > Signed-off-by: Will Cohen <wwco...@gmail.com>
> > ---
> > hw/9pfs/9p-local.c | 9 +++++++++
> > hw/9pfs/9p-proxy.c | 16 +++++++++++++++-
> > hw/9pfs/9p-synth.c | 4 ++++
> > hw/9pfs/9p-util.h | 17 +++++++++++++++++
> > hw/9pfs/9p.c | 15 +++++++++++++--
> > hw/9pfs/codir.c | 7 +++++++
> > 6 files changed, 65 insertions(+), 3 deletions(-)
> >
> > 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);
> > + /* If telldir fails, fail the entire readdir call */
> > + if (td < 0) {
> > + return NULL;
> > + }
> > + entry->d_seekoff = td;
> > +#endif
> > if (!entry) {
> > return NULL;
> > }
>
> 'entry' may be NULL, so the 'if (!entry) {' check should be before the
> Darwin
> specific code to avoid a crash on macOS.
>
Adjusting for v5.
>
> > 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
> > }
> >
> > 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.
> > + */
> > +
>
> Nitpicking: no blank line here please.
>
Adjusting for v5.
>
> > +inline off_t qemu_dirent_off(struct dirent *dent)
> > +{
> > +#ifdef CONFIG_DARWIN
> > + return dent->d_seekoff;
> > +#else
> > + return dent->d_off;
> > +#endif
> > +}
> > 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;
> > + }
> > }
>
> That check is no longer needed here, is it?
>
Correct! Adjusting 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;
> > + }
>
> Likewise: is this check still needed?
>
As above, removing for v5.
>
> > 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
>
> Why can't the header not be included here? Obvious preference would be to
> use
> qemu_dirent_off() here as well, to have control at one central code
> location.
>
Only my own imprecision in organizing the includes correctly. After
including "9p-xattr.h" as well, this works. Will adjust for v5.
>
> > }
> >
> > /* restore (last) saved position */
>
>
>
