Does the version proposed in v3 address the V9fsFidState issues? In 9p.c
for v2 to v3, we propose
- return telldir(fidp->fs.dir.stream);
+ return v9fs_co_telldir(pdu, fidp);
and in codir.c from v2 to v3 we propose
- saved_dir_pos = telldir(fidp->fs.dir.stream);
+ saved_dir_pos = s->ops->telldir(&s->ctx, &fidp->fs);
This removes the direct access to fidp->, and we hope this should be
sufficient to avoid the concurrency
and undefined behaviors you noted in the v2 review.
On Thu, Jan 27, 2022 at 7:56 PM Will Cohen <wwco...@gmail.com> 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.
>
> 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]
> Signed-off-by: Will Cohen <wwco...@gmail.com>
> Signed-off-by: Fabian Franz <git...@fabian-franz.de>
> ---
> hw/9pfs/9p-synth.c | 2 ++
> hw/9pfs/9p.c | 33 +++++++++++++++++++++++++++++++--
> hw/9pfs/codir.c | 4 ++++
> 3 files changed, 37 insertions(+), 2 deletions(-)
>
> diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
> index 4a4a776d06..09b9c25288 100644
> --- a/hw/9pfs/9p-synth.c
> +++ b/hw/9pfs/9p-synth.c
> @@ -222,7 +222,9 @@ static void synth_direntry(V9fsSynthNode *node,
> {
> strcpy(entry->d_name, node->name);
> entry->d_ino = node->attr->inode;
> +#ifndef CONFIG_DARWIN
> entry->d_off = off + 1;
> +#endif
> }
>
> static struct dirent *synth_get_dentry(V9fsSynthNode *dir,
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 1563d7b7c6..7851f85f8f 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -2218,6 +2218,25 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU
> *pdu, V9fsFidState *fidp,
> return offset;
> }
>
> +/**
> + * Get the seek offset of a dirent. If not available from the structure
> itself,
> + * obtain it by calling telldir.
> + */
> +static int v9fs_dent_telldir(V9fsPDU *pdu, V9fsFidState *fidp,
> + struct dirent *dent)
> +{
> +#ifdef CONFIG_DARWIN
> + /*
> + * 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 use telldir for correctness.
> + */
> + return v9fs_co_telldir(pdu, fidp);
> +#else
> + return dent->d_off;
> +#endif
> +}
> +
> static int coroutine_fn v9fs_do_readdir_with_stat(V9fsPDU *pdu,
> V9fsFidState *fidp,
> uint32_t max_count)
> @@ -2281,7 +2300,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 = v9fs_dent_telldir(pdu, fidp, dent);
> + if (saved_dir_pos < 0) {
> + err = saved_dir_pos;
> + break;
> + }
> }
>
> v9fs_readdir_unlock(&fidp->fs.dir);
> @@ -2420,6 +2443,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 +2504,17 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU
> *pdu, V9fsFidState *fidp,
> qid.version = 0;
> }
>
> + off = v9fs_dent_telldir(pdu, fidp, dent);
> + if (off < 0) {
> + err = off;
> + break;
> + }
> 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..c1b5694f3f 100644
> --- a/hw/9pfs/codir.c
> +++ b/hw/9pfs/codir.c
> @@ -167,7 +167,11 @@ static int do_readdir_many(V9fsPDU *pdu, V9fsFidState
> *fidp,
> }
>
> size += len;
> +#ifdef CONFIG_DARWIN
> + saved_dir_pos = s->ops->telldir(&s->ctx, &fidp->fs);
> +#else
> saved_dir_pos = dent->d_off;
> +#endif
> }
>
> /* restore (last) saved position */
> --
> 2.34.1
>
>
