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

[Christian Schoenebeck]

On Sonntag, 20. Februar 2022 17:50:49 CET Will Cohen wrote:
> From: Keno Fischer <keno@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 <keno@juliacomputing.com>
> [Michael Roitzsch: - Rebase for NixOS]
> Signed-off-by: Michael Roitzsch <reactorcontrol@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]
> Signed-off-by: Fabian Franz <fabianfranz.oss@gmail.com>
> [Will Cohen: - Ensure that telldir error handling uses
>                signed int
>              - Cleanup of telldir error handling
>              - Remove superfluous error handling for
>                qemu_dirent_off
>              - Adjust formatting
>              - Use qemu_dirent_off in codir.c]
> Signed-off-by: Will Cohen <wwcohen@gmail.com>
> ---

This patch does not compile on Linux ...

>  hw/9pfs/9p-local.c |  9 +++++++++
>  hw/9pfs/9p-proxy.c | 16 +++++++++++++++-
>  hw/9pfs/9p-synth.c |  4 ++++
>  hw/9pfs/9p-util.h  | 16 ++++++++++++++++
>  hw/9pfs/9p.c       |  7 +++++--
>  hw/9pfs/codir.c    |  4 +++-
>  6 files changed, 52 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index 1a5e3eed73..f3272f0b43 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -562,6 +562,15 @@ again:
>      if (!entry) {
>          return NULL;
>      }
> +#ifdef CONFIG_DARWIN
> +    int off;
> +    off = telldir(fs->dir.stream);
> +    /* If telldir fails, fail the entire readdir call */
> +    if (off < 0) {
> +        return NULL;
> +    }
> +    entry->d_seekoff = off;
> +#endif
> 
>      if (ctx->export_flags & V9FS_SM_MAPPED) {
>          entry->d_type = DT_UNKNOWN;
> 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 bf9b0c5ddd..b3080e415b 100644
> --- a/hw/9pfs/9p-synth.c
> +++ b/hw/9pfs/9p-synth.c
> @@ -234,7 +234,11 @@ static void synth_direntry(V9fsSynthNode *node,
>               offsetof(struct dirent, d_name) + sz);
>      memcpy(entry->d_name, node->name, sz);
>      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..d41f37f085 100644
> --- a/hw/9pfs/9p-util.h
> +++ b/hw/9pfs/9p-util.h
> @@ -79,3 +79,19 @@ ssize_t fremovexattrat_nofollow(int dirfd, const char
> *filename, const char *name);
> 
>  #endif

... ^- this is the end of file #endif, so qemu_dirent_off() should be above 
that #endif, and ...

> +
> +
> +/**
> + * 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)

... this function declaration misses the 'static' keyword, which is mandatory 
to prevent a linker error.

Best regards,
Christian Schoenebeck

> +{
> +#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..caf3b240fe 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,7 @@ 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);
>      }
> 
>      v9fs_readdir_unlock(&fidp->fs.dir);
> @@ -2420,6 +2421,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 +2482,13 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU
> *pdu, V9fsFidState *fidp, qid.version = 0;
>          }
> 
> +        off = qemu_dirent_off(dent);
>          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 c0873bde16..f96d8ac4e6 100644
> --- a/hw/9pfs/codir.c
> +++ b/hw/9pfs/codir.c
> @@ -22,6 +22,8 @@
>  #include "qemu/coroutine.h"
>  #include "qemu/main-loop.h"
>  #include "coth.h"
> +#include "9p-xattr.h"
> +#include "9p-util.h"
> 
>  /*
>   * Intended to be called from bottom-half (e.g. background I/O thread)
> @@ -166,7 +168,7 @@ static int do_readdir_many(V9fsPDU *pdu, V9fsFidState
> *fidp, }
> 
>          size += len;
> -        saved_dir_pos = dent->d_off;
> +        saved_dir_pos = qemu_dirent_off(dent);
>      }
> 
>      /* restore (last) saved position */