[PATCH v4 00/11] 9p: Add support for darwin

[PATCH v4 00/11] 9p: Add support for darwin

[Will Cohen]

This is a followup to https://lists.nongnu.org/archive/html/qemu-devel/2022-01/msg05993.html,
adding 9p server support for Darwin.

Since v3, the following changes have been made:

- Move XATTR_SIZE_MAX to P9_XATTR_SIZE MAX in 9p.h, and provide explanatory context as preliminary solution
- Add explanatory note surrounding virtio-9p-test with output of pre-patch failing test
- Remove superfluous header guards from file-opt-9p
- Add note about virtfs-proxy-helper being disabled on non-linux for this patch series
- Note radar filed with Apple for missing mknodat syscall
- Replace direct syscall to pthread_fchdir with pthread_fchdir_np, and add check for this function’s presence in meson
- Ensure that d_seekoff is filled using telldir on darwin, and create qemu_dirent_off helper to decide which to access.
- Ensure that [amc]tim.tv_sec are all initialized alongside [amc]tim.tv_nsec in 9p-proxy
- Ensure that all patch email addresses are valid
- Add telldir error handling for dirent on darwin

Keno Fischer (10):
  9p: linux: Fix a couple Linux assumptions
  9p: Rename 9p-util -> 9p-util-linux
  9p: darwin: Handle struct stat(fs) differences
  9p: darwin: Handle struct dirent differences
  9p: darwin: Ignore O_{NOATIME, DIRECT}
  9p: darwin: Move XATTR_SIZE_MAX->P9_XATTR_SIZE_MAX
  9p: darwin: *xattr_nofollow implementations
  9p: darwin: Compatibility for f/l*xattr
  9p: darwin: Implement compatibility for mknodat
  9p: darwin: meson: Allow VirtFS on Darwin

Will Cohen (1):
  9p: darwin: Adjust assumption on virtio-9p-test

 fsdev/file-op-9p.h                     |  9 ++-
 fsdev/meson.build                      |  1 +
 hw/9pfs/9p-local.c                     | 28 ++++++--
 hw/9pfs/9p-proxy.c                     | 38 ++++++++++-
 hw/9pfs/9p-synth.c                     |  6 ++
 hw/9pfs/9p-util-darwin.c               | 91 ++++++++++++++++++++++++++
 hw/9pfs/{9p-util.c => 9p-util-linux.c} |  7 +-
 hw/9pfs/9p-util.h                      | 38 +++++++++++
 hw/9pfs/9p.c                           | 50 ++++++++++++--
 hw/9pfs/9p.h                           | 11 ++++
 hw/9pfs/codir.c                        |  7 ++
 hw/9pfs/meson.build                    |  3 +-
 include/qemu/xattr.h                   |  4 +-
 meson.build                            | 14 ++--
 tests/qtest/virtio-9p-test.c           |  2 +-
 15 files changed, 285 insertions(+), 24 deletions(-)
 create mode 100644 hw/9pfs/9p-util-darwin.c
 rename hw/9pfs/{9p-util.c => 9p-util-linux.c} (90%)

-- 
2.32.0 (Apple Git-132)

[PATCH v4 01/11] 9p: linux: Fix a couple Linux assumptions

[Will Cohen]

From: Keno Fischer <keno@juliacomputing.com>

 - Guard Linux only headers.
 - Add qemu/statfs.h header to abstract over the which
   headers are needed for struct statfs
 - Define `ENOATTR` only if not only defined
   (it's defined in system headers on Darwin).

Signed-off-by: Keno Fischer <keno@juliacomputing.com>
[Michael Roitzsch: - Rebase for NixOS]
Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>

While it might at first appear that fsdev/virtfs-proxy-header.c would
need similar adjustment for darwin as file-op-9p here, a later patch in
this series disables virtfs-proxy-helper for non-Linux. Allowing
virtfs-proxy-helper on darwin could potentially be an additional
optimization later.

[Will Cohen: - Fix headers for Alpine
             - Integrate statfs.h back into file-op-9p.h
             - Remove superfluous header guards from file-opt-9p
             - Add note about virtfs-proxy-helper being disabled
               on non-Linux for this patch series]
Signed-off-by: Will Cohen <wwcohen@gmail.com>
---
 fsdev/file-op-9p.h   | 9 ++++++++-
 hw/9pfs/9p-local.c   | 2 ++
 hw/9pfs/9p.c         | 4 ++++
 include/qemu/xattr.h | 4 +++-
 4 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
index 8fd89f0447..4997677460 100644
--- a/fsdev/file-op-9p.h
+++ b/fsdev/file-op-9p.h
@@ -16,10 +16,17 @@
 
 #include <dirent.h>
 #include <utime.h>
-#include <sys/vfs.h>
 #include "qemu-fsdev-throttle.h"
 #include "p9array.h"
 
+#ifdef CONFIG_LINUX
+# include <sys/vfs.h>
+#endif
+#ifdef CONFIG_DARWIN
+# include <sys/param.h>
+# include <sys/mount.h>
+#endif
+
 #define SM_LOCAL_MODE_BITS    0600
 #define SM_LOCAL_DIR_MODE_BITS    0700
 
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 210d9e7705..1a5e3eed73 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -32,10 +32,12 @@
 #include "qemu/error-report.h"
 #include "qemu/option.h"
 #include <libgen.h>
+#ifdef CONFIG_LINUX
 #include <linux/fs.h>
 #ifdef CONFIG_LINUX_MAGIC_H
 #include <linux/magic.h>
 #endif
+#endif
 #include <sys/ioctl.h>
 
 #ifndef XFS_SUPER_MAGIC
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 15b3f4d385..9c63e14b28 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -32,7 +32,11 @@
 #include "migration/blocker.h"
 #include "qemu/xxhash.h"
 #include <math.h>
+#ifdef CONFIG_LINUX
 #include <linux/limits.h>
+#else
+#include <limits.h>
+#endif
 
 int open_fd_hw;
 int total_open_fd;
diff --git a/include/qemu/xattr.h b/include/qemu/xattr.h
index a83fe8e749..f1d0f7be74 100644
--- a/include/qemu/xattr.h
+++ b/include/qemu/xattr.h
@@ -22,7 +22,9 @@
 #ifdef CONFIG_LIBATTR
 #  include <attr/xattr.h>
 #else
-#  define ENOATTR ENODATA
+#  if !defined(ENOATTR)
+#    define ENOATTR ENODATA
+#  endif
 #  include <sys/xattr.h>
 #endif
 
-- 
2.32.0 (Apple Git-132)

[PATCH v4 02/11] 9p: Rename 9p-util -> 9p-util-linux

[Will Cohen]

From: Keno Fischer <keno@juliacomputing.com>

The current file only has the Linux versions of these functions.
Rename the file accordingly and update the Makefile to only build
it on Linux. A Darwin version of these will follow later in the
series.

Signed-off-by: Keno Fischer <keno@juliacomputing.com>
[Michael Roitzsch: - Rebase for NixOS]
Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
Signed-off-by: Will Cohen <wwcohen@gmail.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/{9p-util.c => 9p-util-linux.c} | 2 +-
 hw/9pfs/meson.build                    | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
 rename hw/9pfs/{9p-util.c => 9p-util-linux.c} (97%)

diff --git a/hw/9pfs/9p-util.c b/hw/9pfs/9p-util-linux.c
similarity index 97%
rename from hw/9pfs/9p-util.c
rename to hw/9pfs/9p-util-linux.c
index 3221d9b498..398614a5d0 100644
--- a/hw/9pfs/9p-util.c
+++ b/hw/9pfs/9p-util-linux.c
@@ -1,5 +1,5 @@
 /*
- * 9p utilities
+ * 9p utilities (Linux Implementation)
  *
  * Copyright IBM, Corp. 2017
  *
diff --git a/hw/9pfs/meson.build b/hw/9pfs/meson.build
index 99be5d9119..1b28e70040 100644
--- a/hw/9pfs/meson.build
+++ b/hw/9pfs/meson.build
@@ -4,7 +4,6 @@ fs_ss.add(files(
   '9p-posix-acl.c',
   '9p-proxy.c',
   '9p-synth.c',
-  '9p-util.c',
   '9p-xattr-user.c',
   '9p-xattr.c',
   '9p.c',
@@ -14,6 +13,7 @@ fs_ss.add(files(
   'coth.c',
   'coxattr.c',
 ))
+fs_ss.add(when: 'CONFIG_LINUX', if_true: files('9p-util-linux.c'))
 fs_ss.add(when: 'CONFIG_XEN', if_true: files('xen-9p-backend.c'))
 softmmu_ss.add_all(when: 'CONFIG_FSDEV_9P', if_true: fs_ss)
 
-- 
2.32.0 (Apple Git-132)

[PATCH v4 03/11] 9p: darwin: Handle struct stat(fs) differences

[Will Cohen]

From: Keno Fischer <keno@juliacomputing.com>

Signed-off-by: Keno Fischer <keno@juliacomputing.com>
Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
[Will Cohen: - Note lack of f_namelen and f_frsize on Darwin
             - Ensure that tv_sec and tv_nsec are both
               initialized for Darwin and non-Darwin]
Signed-off-by: Will Cohen <wwcohen@gmail.com>
---
 hw/9pfs/9p-proxy.c | 22 ++++++++++++++++++++--
 hw/9pfs/9p-synth.c |  2 ++
 hw/9pfs/9p.c       | 16 ++++++++++++++--
 3 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c
index 09bd9f1464..b1664080d8 100644
--- a/hw/9pfs/9p-proxy.c
+++ b/hw/9pfs/9p-proxy.c
@@ -123,10 +123,16 @@ static void prstatfs_to_statfs(struct statfs *stfs, ProxyStatFS *prstfs)
     stfs->f_bavail = prstfs->f_bavail;
     stfs->f_files = prstfs->f_files;
     stfs->f_ffree = prstfs->f_ffree;
+#ifdef CONFIG_DARWIN
+    /* f_namelen and f_frsize do not exist on Darwin */
+    stfs->f_fsid.val[0] = prstfs->f_fsid[0] & 0xFFFFFFFFU;
+    stfs->f_fsid.val[1] = prstfs->f_fsid[1] >> 32 & 0xFFFFFFFFU;
+#else
     stfs->f_fsid.__val[0] = prstfs->f_fsid[0] & 0xFFFFFFFFU;
     stfs->f_fsid.__val[1] = prstfs->f_fsid[1] >> 32 & 0xFFFFFFFFU;
     stfs->f_namelen = prstfs->f_namelen;
     stfs->f_frsize = prstfs->f_frsize;
+#endif
 }
 
 /* Converts proxy_stat structure to VFS stat structure */
@@ -143,12 +149,24 @@ static void prstat_to_stat(struct stat *stbuf, ProxyStat *prstat)
    stbuf->st_size = prstat->st_size;
    stbuf->st_blksize = prstat->st_blksize;
    stbuf->st_blocks = prstat->st_blocks;
+   stbuf->st_atime = prstat->st_atim_sec;
+   stbuf->st_mtime = prstat->st_mtim_sec;
+   stbuf->st_ctime = prstat->st_ctim_sec;
+#ifdef CONFIG_DARWIN
+   stbuf->st_atimespec.tv_sec = prstat->st_atim_sec;
+   stbuf->st_mtimespec.tv_sec = prstat->st_mtim_sec;
+   stbuf->st_ctimespec.tv_sec = prstat->st_ctim_sec;
+   stbuf->st_atimespec.tv_nsec = prstat->st_atim_nsec;
+   stbuf->st_mtimespec.tv_nsec = prstat->st_mtim_nsec;
+   stbuf->st_ctimespec.tv_nsec = prstat->st_ctim_nsec;
+#else
    stbuf->st_atim.tv_sec = prstat->st_atim_sec;
+   stbuf->st_mtim.tv_sec = prstat->st_mtim_sec;
+   stbuf->st_ctim.tv_sec = prstat->st_ctim_sec;
    stbuf->st_atim.tv_nsec = prstat->st_atim_nsec;
-   stbuf->st_mtime = prstat->st_mtim_sec;
    stbuf->st_mtim.tv_nsec = prstat->st_mtim_nsec;
-   stbuf->st_ctime = prstat->st_ctim_sec;
    stbuf->st_ctim.tv_nsec = prstat->st_ctim_nsec;
+#endif
 }
 
 /*
diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
index b38088e066..4a4a776d06 100644
--- a/hw/9pfs/9p-synth.c
+++ b/hw/9pfs/9p-synth.c
@@ -427,7 +427,9 @@ static int synth_statfs(FsContext *s, V9fsPath *fs_path,
     stbuf->f_bsize = 512;
     stbuf->f_blocks = 0;
     stbuf->f_files = synth_node_count;
+#ifndef CONFIG_DARWIN
     stbuf->f_namelen = NAME_MAX;
+#endif
     return 0;
 }
 
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 9c63e14b28..1563d7b7c6 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1313,11 +1313,17 @@ static int stat_to_v9stat_dotl(V9fsPDU *pdu, const struct stat *stbuf,
     v9lstat->st_blksize = stat_to_iounit(pdu, stbuf);
     v9lstat->st_blocks = stbuf->st_blocks;
     v9lstat->st_atime_sec = stbuf->st_atime;
-    v9lstat->st_atime_nsec = stbuf->st_atim.tv_nsec;
     v9lstat->st_mtime_sec = stbuf->st_mtime;
-    v9lstat->st_mtime_nsec = stbuf->st_mtim.tv_nsec;
     v9lstat->st_ctime_sec = stbuf->st_ctime;
+#ifdef CONFIG_DARWIN
+    v9lstat->st_atime_nsec = stbuf->st_atimespec.tv_nsec;
+    v9lstat->st_mtime_nsec = stbuf->st_mtimespec.tv_nsec;
+    v9lstat->st_ctime_nsec = stbuf->st_ctimespec.tv_nsec;
+#else
+    v9lstat->st_atime_nsec = stbuf->st_atim.tv_nsec;
+    v9lstat->st_mtime_nsec = stbuf->st_mtim.tv_nsec;
     v9lstat->st_ctime_nsec = stbuf->st_ctim.tv_nsec;
+#endif
     /* Currently we only support BASIC fields in stat */
     v9lstat->st_result_mask = P9_STATS_BASIC;
 
@@ -3519,9 +3525,15 @@ static int v9fs_fill_statfs(V9fsState *s, V9fsPDU *pdu, struct statfs *stbuf)
     f_bavail = stbuf->f_bavail / bsize_factor;
     f_files  = stbuf->f_files;
     f_ffree  = stbuf->f_ffree;
+#ifdef CONFIG_DARWIN
+    fsid_val = (unsigned int)stbuf->f_fsid.val[0] |
+               (unsigned long long)stbuf->f_fsid.val[1] << 32;
+    f_namelen = NAME_MAX;
+#else
     fsid_val = (unsigned int) stbuf->f_fsid.__val[0] |
                (unsigned long long)stbuf->f_fsid.__val[1] << 32;
     f_namelen = stbuf->f_namelen;
+#endif
 
     return pdu_marshal(pdu, offset, "ddqqqqqqd",
                        f_type, f_bsize, f_blocks, f_bfree,
-- 
2.32.0 (Apple Git-132)

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

[Will Cohen]

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]
[Will Cohen: - Ensure that telldir error handling uses
               signed int]
Signed-off-by: Fabian Franz <fabianfranz.oss@gmail.com>
Signed-off-by: Will Cohen <wwcohen@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;
     }
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.
+ */
+
+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;
+        }
     }
 
     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;
+        }
         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)

[PATCH v4 05/11] 9p: darwin: Ignore O_{NOATIME, DIRECT}

[Will Cohen]

From: Keno Fischer <keno@juliacomputing.com>

Darwin doesn't have either of these flags. Darwin does have
F_NOCACHE, which is similar to O_DIRECT, but has different
enough semantics that other projects don't generally map
them automatically. In any case, we don't support O_DIRECT
on Linux at the moment either.

Signed-off-by: Keno Fischer <keno@juliacomputing.com>
[Michael Roitzsch: - Rebase for NixOS]
Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
[Will Cohen: - Adjust coding style]
Signed-off-by: Will Cohen <wwcohen@gmail.com>
---
 hw/9pfs/9p-util.h |  2 ++
 hw/9pfs/9p.c      | 13 ++++++++++++-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h
index accbec9987..3b9c485414 100644
--- a/hw/9pfs/9p-util.h
+++ b/hw/9pfs/9p-util.h
@@ -41,6 +41,7 @@ again:
     fd = openat(dirfd, name, flags | O_NOFOLLOW | O_NOCTTY | O_NONBLOCK,
                 mode);
     if (fd == -1) {
+#ifndef CONFIG_DARWIN
         if (errno == EPERM && (flags & O_NOATIME)) {
             /*
              * The client passed O_NOATIME but we lack permissions to honor it.
@@ -53,6 +54,7 @@ again:
             flags &= ~O_NOATIME;
             goto again;
         }
+#endif
         return -1;
     }
 
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index cf694da354..f3b00d20a2 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -138,11 +138,20 @@ static int dotl_to_open_flags(int flags)
         { P9_DOTL_NONBLOCK, O_NONBLOCK } ,
         { P9_DOTL_DSYNC, O_DSYNC },
         { P9_DOTL_FASYNC, FASYNC },
+#ifndef CONFIG_DARWIN
+        { P9_DOTL_NOATIME, O_NOATIME },
+        /*
+         *  On Darwin, we could map to F_NOCACHE, which is
+         *  similar, but doesn't quite have the same
+         *  semantics. However, we don't support O_DIRECT
+         *  even on linux at the moment, so we just ignore
+         *  it here.
+         */
         { P9_DOTL_DIRECT, O_DIRECT },
+#endif
         { P9_DOTL_LARGEFILE, O_LARGEFILE },
         { P9_DOTL_DIRECTORY, O_DIRECTORY },
         { P9_DOTL_NOFOLLOW, O_NOFOLLOW },
-        { P9_DOTL_NOATIME, O_NOATIME },
         { P9_DOTL_SYNC, O_SYNC },
     };
 
@@ -171,10 +180,12 @@ static int get_dotl_openflags(V9fsState *s, int oflags)
      */
     flags = dotl_to_open_flags(oflags);
     flags &= ~(O_NOCTTY | O_ASYNC | O_CREAT);
+#ifndef CONFIG_DARWIN
     /*
      * Ignore direct disk access hint until the server supports it.
      */
     flags &= ~O_DIRECT;
+#endif
     return flags;
 }
 
-- 
2.32.0 (Apple Git-132)

[PATCH v4 06/11] 9p: darwin: Move XATTR_SIZE_MAX->P9_XATTR_SIZE_MAX

[Will Cohen]

From: Keno Fischer <keno@juliacomputing.com>

Signed-off-by: Keno Fischer <keno@juliacomputing.com>
Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>

Because XATTR_SIZE_MAX is not defined on Darwin,
create a cross-platform P9_XATTR_SIZE_MAX instead.

[Will Cohen: - Adjust coding style
             - Lower XATTR_SIZE_MAX to 64k
             - Add explanatory context related to XATTR_SIZE_MAX]
[Fabian Franz: - Move XATTR_SIZE_MAX reference from 9p.c to
                 P9_XATTR_SIZE_MAX in 9p.h]
Signed-off-by: Will Cohen <wwcohen@gmail.com>
Signed-off-by: Fabian Franz <fabianfranz.oss@gmail.com>
---
 hw/9pfs/9p.c |  2 +-
 hw/9pfs/9p.h | 11 +++++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index f3b00d20a2..440bf5c9e5 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -3957,7 +3957,7 @@ static void coroutine_fn v9fs_xattrcreate(void *opaque)
         rflags |= XATTR_REPLACE;
     }
 
-    if (size > XATTR_SIZE_MAX) {
+    if (size > P9_XATTR_SIZE_MAX) {
         err = -E2BIG;
         goto out_nofid;
     }
diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index 1567b67841..6a1856b4dc 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -479,4 +479,15 @@ struct V9fsTransport {
     void        (*push_and_notify)(V9fsPDU *pdu);
 };
 
+/*
+ * Darwin doesn't seem to define a maximum xattr size in its user
+ * space header, so manually configure it across platforms as 64k.
+ *
+ * Having no limit at all can lead to QEMU crashing during large g_malloc()
+ * calls. Because QEMU does not currently support macOS guests, the below
+ * preliminary solution only works due to its being a reflection of the limit of
+ * Linux guests.
+ */
+#define P9_XATTR_SIZE_MAX 65536
+
 #endif
-- 
2.32.0 (Apple Git-132)

[PATCH v4 07/11] 9p: darwin: *xattr_nofollow implementations

[Will Cohen]

From: Keno Fischer <keno@juliacomputing.com>

This implements the darwin equivalent of the functions that were
moved to 9p-util(-linux) earlier in this series in the new
9p-util-darwin file.

Signed-off-by: Keno Fischer <keno@juliacomputing.com>
[Michael Roitzsch: - Rebase for NixOS]
Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
Signed-off-by: Will Cohen <wwcohen@gmail.com>
---
 hw/9pfs/9p-util-darwin.c | 64 ++++++++++++++++++++++++++++++++++++++++
 hw/9pfs/meson.build      |  1 +
 2 files changed, 65 insertions(+)
 create mode 100644 hw/9pfs/9p-util-darwin.c

diff --git a/hw/9pfs/9p-util-darwin.c b/hw/9pfs/9p-util-darwin.c
new file mode 100644
index 0000000000..cdb4c9e24c
--- /dev/null
+++ b/hw/9pfs/9p-util-darwin.c
@@ -0,0 +1,64 @@
+/*
+ * 9p utilities (Darwin Implementation)
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/xattr.h"
+#include "9p-util.h"
+
+ssize_t fgetxattrat_nofollow(int dirfd, const char *filename, const char *name,
+                             void *value, size_t size)
+{
+    int ret;
+    int fd = openat_file(dirfd, filename,
+                         O_RDONLY | O_PATH_9P_UTIL | O_NOFOLLOW, 0);
+    if (fd == -1) {
+        return -1;
+    }
+    ret = fgetxattr(fd, name, value, size, 0, 0);
+    close_preserve_errno(fd);
+    return ret;
+}
+
+ssize_t flistxattrat_nofollow(int dirfd, const char *filename,
+                              char *list, size_t size)
+{
+    int ret;
+    int fd = openat_file(dirfd, filename,
+                         O_RDONLY | O_PATH_9P_UTIL | O_NOFOLLOW, 0);
+    if (fd == -1) {
+        return -1;
+    }
+    ret = flistxattr(fd, list, size, 0);
+    close_preserve_errno(fd);
+    return ret;
+}
+
+ssize_t fremovexattrat_nofollow(int dirfd, const char *filename,
+                                const char *name)
+{
+    int ret;
+    int fd = openat_file(dirfd, filename, O_PATH_9P_UTIL | O_NOFOLLOW, 0);
+    if (fd == -1) {
+        return -1;
+    }
+    ret = fremovexattr(fd, name, 0);
+    close_preserve_errno(fd);
+    return ret;
+}
+
+int fsetxattrat_nofollow(int dirfd, const char *filename, const char *name,
+                         void *value, size_t size, int flags)
+{
+    int ret;
+    int fd = openat_file(dirfd, filename, O_PATH_9P_UTIL | O_NOFOLLOW, 0);
+    if (fd == -1) {
+        return -1;
+    }
+    ret = fsetxattr(fd, name, value, size, 0, flags);
+    close_preserve_errno(fd);
+    return ret;
+}
diff --git a/hw/9pfs/meson.build b/hw/9pfs/meson.build
index 1b28e70040..12443b6ad5 100644
--- a/hw/9pfs/meson.build
+++ b/hw/9pfs/meson.build
@@ -14,6 +14,7 @@ fs_ss.add(files(
   'coxattr.c',
 ))
 fs_ss.add(when: 'CONFIG_LINUX', if_true: files('9p-util-linux.c'))
+fs_ss.add(when: 'CONFIG_DARWIN', if_true: files('9p-util-darwin.c'))
 fs_ss.add(when: 'CONFIG_XEN', if_true: files('xen-9p-backend.c'))
 softmmu_ss.add_all(when: 'CONFIG_FSDEV_9P', if_true: fs_ss)
 
-- 
2.32.0 (Apple Git-132)

[PATCH v4 08/11] 9p: darwin: Compatibility for f/l*xattr

[Will Cohen]

From: Keno Fischer <keno@juliacomputing.com>

On darwin `fgetxattr` takes two extra optional arguments,
and the l* variants are not defined (in favor of an extra
flag to the regular variants.

Signed-off-by: Keno Fischer <keno@juliacomputing.com>
[Michael Roitzsch: - Rebase for NixOS]
Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
Signed-off-by: Will Cohen <wwcohen@gmail.com>
---
 hw/9pfs/9p-local.c | 12 ++++++++----
 hw/9pfs/9p-util.h  | 17 +++++++++++++++++
 2 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 7137a28109..ea3ded5fca 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -790,16 +790,20 @@ static int local_fstat(FsContext *fs_ctx, int fid_type,
         mode_t tmp_mode;
         dev_t tmp_dev;
 
-        if (fgetxattr(fd, "user.virtfs.uid", &tmp_uid, sizeof(uid_t)) > 0) {
+        if (qemu_fgetxattr(fd, "user.virtfs.uid",
+                           &tmp_uid, sizeof(uid_t)) > 0) {
             stbuf->st_uid = le32_to_cpu(tmp_uid);
         }
-        if (fgetxattr(fd, "user.virtfs.gid", &tmp_gid, sizeof(gid_t)) > 0) {
+        if (qemu_fgetxattr(fd, "user.virtfs.gid",
+                           &tmp_gid, sizeof(gid_t)) > 0) {
             stbuf->st_gid = le32_to_cpu(tmp_gid);
         }
-        if (fgetxattr(fd, "user.virtfs.mode", &tmp_mode, sizeof(mode_t)) > 0) {
+        if (qemu_fgetxattr(fd, "user.virtfs.mode",
+                           &tmp_mode, sizeof(mode_t)) > 0) {
             stbuf->st_mode = le32_to_cpu(tmp_mode);
         }
-        if (fgetxattr(fd, "user.virtfs.rdev", &tmp_dev, sizeof(dev_t)) > 0) {
+        if (qemu_fgetxattr(fd, "user.virtfs.rdev",
+                           &tmp_dev, sizeof(dev_t)) > 0) {
             stbuf->st_rdev = le64_to_cpu(tmp_dev);
         }
     } else if (fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h
index 3b9c485414..8e610ad224 100644
--- a/hw/9pfs/9p-util.h
+++ b/hw/9pfs/9p-util.h
@@ -19,6 +19,23 @@
 #define O_PATH_9P_UTIL 0
 #endif
 
+#ifdef CONFIG_DARWIN
+#define qemu_fgetxattr(...) fgetxattr(__VA_ARGS__, 0, 0)
+#define qemu_lgetxattr(...) getxattr(__VA_ARGS__, 0, XATTR_NOFOLLOW)
+#define qemu_llistxattr(...) listxattr(__VA_ARGS__, XATTR_NOFOLLOW)
+#define qemu_lremovexattr(...) removexattr(__VA_ARGS__, XATTR_NOFOLLOW)
+static inline int qemu_lsetxattr(const char *path, const char *name,
+                                 const void *value, size_t size, int flags) {
+    return setxattr(path, name, value, size, 0, flags | XATTR_NOFOLLOW);
+}
+#else
+#define qemu_fgetxattr fgetxattr
+#define qemu_lgetxattr lgetxattr
+#define qemu_llistxattr llistxattr
+#define qemu_lremovexattr lremovexattr
+#define qemu_lsetxattr lsetxattr
+#endif
+
 static inline void close_preserve_errno(int fd)
 {
     int serrno = errno;
-- 
2.32.0 (Apple Git-132)

[PATCH v4 09/11] 9p: darwin: Implement compatibility for mknodat

[Will Cohen]

From: Keno Fischer <keno@juliacomputing.com>

Darwin does not support mknodat. However, to avoid race conditions
with later setting the permissions, we must avoid using mknod on
the full path instead. We could try to fchdir, but that would cause
problems if multiple threads try to call mknodat at the same time.
However, luckily there is a solution: Darwin includes a function
that sets the cwd for the current thread only.
This should suffice to use mknod safely.

This function (pthread_fchdir_np) is protected by a check in
meson in a patch later in tihs series.

Signed-off-by: Keno Fischer <keno@juliacomputing.com>
Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
[Will Cohen: - Adjust coding style
             - Replace clang references with gcc
             - Note radar filed with Apple for missing syscall
             - Replace direct syscall with pthread_fchdir_np and
               adjust patch notes accordingly]
Signed-off-by: Will Cohen <wwcohen@gmail.com>
---
 hw/9pfs/9p-local.c       |  5 +++--
 hw/9pfs/9p-util-darwin.c | 27 +++++++++++++++++++++++++++
 hw/9pfs/9p-util-linux.c  |  5 +++++
 hw/9pfs/9p-util.h        |  2 ++
 4 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index ea3ded5fca..0569a8b4de 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -682,7 +682,7 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath *dir_path,
 
     if (fs_ctx->export_flags & V9FS_SM_MAPPED ||
         fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
-        err = mknodat(dirfd, name, fs_ctx->fmode | S_IFREG, 0);
+        err = qemu_mknodat(dirfd, name, fs_ctx->fmode | S_IFREG, 0);
         if (err == -1) {
             goto out;
         }
@@ -697,7 +697,7 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath *dir_path,
         }
     } else if (fs_ctx->export_flags & V9FS_SM_PASSTHROUGH ||
                fs_ctx->export_flags & V9FS_SM_NONE) {
-        err = mknodat(dirfd, name, credp->fc_mode, credp->fc_rdev);
+        err = qemu_mknodat(dirfd, name, credp->fc_mode, credp->fc_rdev);
         if (err == -1) {
             goto out;
         }
@@ -710,6 +710,7 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath *dir_path,
 
 err_end:
     unlinkat_preserve_errno(dirfd, name, 0);
+
 out:
     close_preserve_errno(dirfd);
     return err;
diff --git a/hw/9pfs/9p-util-darwin.c b/hw/9pfs/9p-util-darwin.c
index cdb4c9e24c..128b6d87e8 100644
--- a/hw/9pfs/9p-util-darwin.c
+++ b/hw/9pfs/9p-util-darwin.c
@@ -5,6 +5,7 @@
  * See the COPYING file in the top-level directory.
  */
 
+#include <os/availability.h>
 #include "qemu/osdep.h"
 #include "qemu/xattr.h"
 #include "9p-util.h"
@@ -62,3 +63,29 @@ int fsetxattrat_nofollow(int dirfd, const char *filename, const char *name,
     close_preserve_errno(fd);
     return ret;
 }
+
+/*
+ * As long as mknodat is not available on macOS, this workaround
+ * using pthread_fchdir_np is needed.
+ *
+ * Radar filed with Apple for implementing mknodat:
+ * rdar://FB9862426 (https://openradar.appspot.com/FB9862426)
+ */
+
+int pthread_fchdir_np(int fd) API_AVAILABLE(macosx(10.12));
+
+int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev)
+{
+    int preserved_errno, err;
+    if (pthread_fchdir_np(dirfd) < 0) {
+        return -1;
+    }
+    err = mknod(filename, mode, dev);
+    preserved_errno = errno;
+    /* Stop using the thread-local cwd */
+    pthread_fchdir_np(-1);
+    if (err < 0) {
+        errno = preserved_errno;
+    }
+    return err;
+}
diff --git a/hw/9pfs/9p-util-linux.c b/hw/9pfs/9p-util-linux.c
index 398614a5d0..66e0ab1865 100644
--- a/hw/9pfs/9p-util-linux.c
+++ b/hw/9pfs/9p-util-linux.c
@@ -62,3 +62,8 @@ int fsetxattrat_nofollow(int dirfd, const char *filename, const char *name,
     g_free(proc_path);
     return ret;
 }
+
+int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev)
+{
+    return mknodat(dirfd, filename, mode, dev);
+}
diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h
index 8e610ad224..f6fed963bf 100644
--- a/hw/9pfs/9p-util.h
+++ b/hw/9pfs/9p-util.h
@@ -97,6 +97,8 @@ ssize_t flistxattrat_nofollow(int dirfd, const char *filename,
 ssize_t fremovexattrat_nofollow(int dirfd, const char *filename,
                                 const char *name);
 
+int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev);
+
 #endif
 
 
-- 
2.32.0 (Apple Git-132)

[PATCH v4 10/11] 9p: darwin: meson: Allow VirtFS on Darwin

[Will Cohen]

From: Keno Fischer <keno@juliacomputing.com>

Signed-off-by: Keno Fischer <keno@juliacomputing.com>
[Michael Roitzsch: - Rebase for NixOS]
Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
[Will Cohen: - Rebase to master]
Signed-off-by: Will Cohen <wwcohen@gmail.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
[Will Cohen: - Add check for pthread_fchdir_np to virtfs]
Signed-off-by: Will Cohen <wwcohen@gmail.com>
---
 fsdev/meson.build |  1 +
 meson.build       | 14 ++++++++++----
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/fsdev/meson.build b/fsdev/meson.build
index adf57cc43e..b632b66348 100644
--- a/fsdev/meson.build
+++ b/fsdev/meson.build
@@ -7,6 +7,7 @@ fsdev_ss.add(when: ['CONFIG_FSDEV_9P'], if_true: files(
   'qemu-fsdev.c',
 ), if_false: files('qemu-fsdev-dummy.c'))
 softmmu_ss.add_all(when: 'CONFIG_LINUX', if_true: fsdev_ss)
+softmmu_ss.add_all(when: 'CONFIG_DARWIN', if_true: fsdev_ss)
 
 if have_virtfs_proxy_helper
   executable('virtfs-proxy-helper',
diff --git a/meson.build b/meson.build
index 5f43355071..6b4adf7e15 100644
--- a/meson.build
+++ b/meson.build
@@ -1421,17 +1421,23 @@ if not get_option('dbus_display').disabled()
   endif
 endif
 
-have_virtfs = (targetos == 'linux' and
+if targetos == 'darwin' and cc.has_function('pthread_fchdir_np')
+  have_virtfs = have_system
+else
+  have_virtfs = (targetos == 'linux' and
     have_system and
     libattr.found() and
     libcap_ng.found())
+endif
 
-have_virtfs_proxy_helper = have_virtfs and have_tools
+have_virtfs_proxy_helper = targetos == 'linux' and have_virtfs and have_tools
 
 if get_option('virtfs').enabled()
   if not have_virtfs
-    if targetos != 'linux'
-      error('virtio-9p (virtfs) requires Linux')
+    if targetos != 'linux' and targetos != 'darwin'
+      error('virtio-9p (virtfs) requires Linux or Darwin')
+    elif targetos == 'darwin' and not cc.has_function('pthread_fchdir_np')
+      error('virtio-9p (virtfs) on Darwin requires the presence of pthread_fchdir_np')
     elif not libcap_ng.found() or not libattr.found()
       error('virtio-9p (virtfs) requires libcap-ng-devel and libattr-devel')
     elif not have_system
-- 
2.32.0 (Apple Git-132)

[PATCH v4 11/11] 9p: darwin: Adjust assumption on virtio-9p-test

[Will Cohen]

The previous test depended on the assumption that P9_DOTL_AT_REMOVEDIR
and AT_REMOVEDIR have the same value.

While this is true on Linux, it is not true everywhere, and leads to an
incorrect test failure on unlink_at, noticed when adding 9p to darwin:

Received response 7 (RLERROR) instead of 77 (RUNLINKAT)
Rlerror has errno 22 (Invalid argument)
**

ERROR:../tests/qtest/virtio-9p-test.c:305:v9fs_req_recv: assertion
failed (hdr.id == id): (7 == 77) Bail out!

ERROR:../tests/qtest/virtio-9p-test.c:305:v9fs_req_recv: assertion
failed (hdr.id == id): (7 == 77)

Signed-off-by: Fabian Franz <fabianfranz.oss@gmail.com>
[Will Cohen: - Add explanation of patch and description
               of pre-patch test failure]
Signed-off-by: Will Cohen <wwcohen@gmail.com>
---
 tests/qtest/virtio-9p-test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index 41fed41de1..6bcf89f0f8 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -1270,7 +1270,7 @@ static void fs_unlinkat_dir(void *obj, void *data, QGuestAllocator *t_alloc)
     /* ... and is actually a directory */
     g_assert((st.st_mode & S_IFMT) == S_IFDIR);
 
-    do_unlinkat(v9p, "/", "02", AT_REMOVEDIR);
+    do_unlinkat(v9p, "/", "02", P9_DOTL_AT_REMOVEDIR);
     /* directory should be gone now */
     g_assert(stat(new_dir, &st) != 0);
 
-- 
2.32.0 (Apple Git-132)

Re: [PATCH v4 01/11] 9p: linux: Fix a couple Linux assumptions

[Philippe Mathieu-Daudé via]

On 6/2/22 21:07, Will Cohen wrote:
> From: Keno Fischer <keno@juliacomputing.com>
> 
>   - Guard Linux only headers.
>   - Add qemu/statfs.h header to abstract over the which
>     headers are needed for struct statfs
>   - Define `ENOATTR` only if not only defined
>     (it's defined in system headers on Darwin).
> 
> Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> [Michael Roitzsch: - Rebase for NixOS]
> Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
> 
> While it might at first appear that fsdev/virtfs-proxy-header.c would
> need similar adjustment for darwin as file-op-9p here, a later patch in
> this series disables virtfs-proxy-helper for non-Linux. Allowing
> virtfs-proxy-helper on darwin could potentially be an additional
> optimization later.
> 
> [Will Cohen: - Fix headers for Alpine
>               - Integrate statfs.h back into file-op-9p.h
>               - Remove superfluous header guards from file-opt-9p
>               - Add note about virtfs-proxy-helper being disabled
>                 on non-Linux for this patch series]
> Signed-off-by: Will Cohen <wwcohen@gmail.com>
> ---
>   fsdev/file-op-9p.h   | 9 ++++++++-
>   hw/9pfs/9p-local.c   | 2 ++
>   hw/9pfs/9p.c         | 4 ++++
>   include/qemu/xattr.h | 4 +++-
>   4 files changed, 17 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v4 02/11] 9p: Rename 9p-util -> 9p-util-linux

[Philippe Mathieu-Daudé via]

On 6/2/22 21:07, Will Cohen wrote:
> From: Keno Fischer <keno@juliacomputing.com>
> 
> The current file only has the Linux versions of these functions.
> Rename the file accordingly and update the Makefile to only build
> it on Linux. A Darwin version of these will follow later in the
> series.
> 
> Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> [Michael Roitzsch: - Rebase for NixOS]
> Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
> Signed-off-by: Will Cohen <wwcohen@gmail.com>
> Reviewed-by: Greg Kurz <groug@kaod.org>
> ---
>   hw/9pfs/{9p-util.c => 9p-util-linux.c} | 2 +-
>   hw/9pfs/meson.build                    | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
>   rename hw/9pfs/{9p-util.c => 9p-util-linux.c} (97%)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v4 09/11] 9p: darwin: Implement compatibility for mknodat

[Philippe Mathieu-Daudé via]

On 6/2/22 21:07, Will Cohen wrote:
> From: Keno Fischer <keno@juliacomputing.com>
> 
> Darwin does not support mknodat. However, to avoid race conditions
> with later setting the permissions, we must avoid using mknod on
> the full path instead. We could try to fchdir, but that would cause
> problems if multiple threads try to call mknodat at the same time.
> However, luckily there is a solution: Darwin includes a function
> that sets the cwd for the current thread only.
> This should suffice to use mknod safely.
> 
> This function (pthread_fchdir_np) is protected by a check in
> meson in a patch later in tihs series.
> 
> Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
> [Will Cohen: - Adjust coding style
>               - Replace clang references with gcc
>               - Note radar filed with Apple for missing syscall
>               - Replace direct syscall with pthread_fchdir_np and
>                 adjust patch notes accordingly]
> Signed-off-by: Will Cohen <wwcohen@gmail.com>
> ---
>   hw/9pfs/9p-local.c       |  5 +++--
>   hw/9pfs/9p-util-darwin.c | 27 +++++++++++++++++++++++++++
>   hw/9pfs/9p-util-linux.c  |  5 +++++
>   hw/9pfs/9p-util.h        |  2 ++
>   4 files changed, 37 insertions(+), 2 deletions(-)

> diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h
> index 8e610ad224..f6fed963bf 100644
> --- a/hw/9pfs/9p-util.h
> +++ b/hw/9pfs/9p-util.h
> @@ -97,6 +97,8 @@ ssize_t flistxattrat_nofollow(int dirfd, const char *filename,
>   ssize_t fremovexattrat_nofollow(int dirfd, const char *filename,
>                                   const char *name);
>   
> +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev);

I think this belong to "osdep.h" & os-posix.c.

Re: [PATCH v4 10/11] 9p: darwin: meson: Allow VirtFS on Darwin

[Philippe Mathieu-Daudé via]

On 6/2/22 21:07, Will Cohen wrote:
> From: Keno Fischer <keno@juliacomputing.com>
> 
> Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> [Michael Roitzsch: - Rebase for NixOS]
> Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
> [Will Cohen: - Rebase to master]
> Signed-off-by: Will Cohen <wwcohen@gmail.com>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> [Will Cohen: - Add check for pthread_fchdir_np to virtfs]
> Signed-off-by: Will Cohen <wwcohen@gmail.com>
> ---
>   fsdev/meson.build |  1 +
>   meson.build       | 14 ++++++++++----
>   2 files changed, 11 insertions(+), 4 deletions(-)

> -have_virtfs_proxy_helper = have_virtfs and have_tools
> +have_virtfs_proxy_helper = targetos == 'linux' and have_virtfs and have_tools

Why do you restrict the proxy-helper to Linux?

Re: [PATCH v4 10/11] 9p: darwin: meson: Allow VirtFS on Darwin

[Will Cohen]

[-- Attachment #1: Type: text/plain, Size: 1385 bytes --]

Only because porting the proxy-helper to macOS is outside the scope of this
particular patch. While some initial concepts around it have been
considered by some of the contributors to this patch, those implementations
weren't tested enough and the security implications weren't considered in
full. We assume that this could be an additional implementation later on,
if the functionality is considered important down the road.

On Sun, Feb 6, 2022 at 4:22 PM Philippe Mathieu-Daudé <f4bug@amsat.org>
wrote:

> On 6/2/22 21:07, Will Cohen wrote:
> > From: Keno Fischer <keno@juliacomputing.com>
> >
> > Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> > [Michael Roitzsch: - Rebase for NixOS]
> > Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
> > [Will Cohen: - Rebase to master]
> > Signed-off-by: Will Cohen <wwcohen@gmail.com>
> > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> > [Will Cohen: - Add check for pthread_fchdir_np to virtfs]
> > Signed-off-by: Will Cohen <wwcohen@gmail.com>
> > ---
> >   fsdev/meson.build |  1 +
> >   meson.build       | 14 ++++++++++----
> >   2 files changed, 11 insertions(+), 4 deletions(-)
>
> > -have_virtfs_proxy_helper = have_virtfs and have_tools
> > +have_virtfs_proxy_helper = targetos == 'linux' and have_virtfs and
> have_tools
>
> Why do you restrict the proxy-helper to Linux?
>

[-- Attachment #2: Type: text/html, Size: 2154 bytes --]

Re: [PATCH v4 09/11] 9p: darwin: Implement compatibility for mknodat

[Will Cohen]

[-- Attachment #1: Type: text/plain, Size: 2227 bytes --]

This patch set currently places it in 9p-util only because 9p is the only
place where this issue seems to have come up so far and we were wary of
editing files too far afield, but I have no attachment to its specific
location!

On Sun, Feb 6, 2022 at 4:21 PM Philippe Mathieu-Daudé <f4bug@amsat.org>
wrote:

> On 6/2/22 21:07, Will Cohen wrote:
> > From: Keno Fischer <keno@juliacomputing.com>
> >
> > Darwin does not support mknodat. However, to avoid race conditions
> > with later setting the permissions, we must avoid using mknod on
> > the full path instead. We could try to fchdir, but that would cause
> > problems if multiple threads try to call mknodat at the same time.
> > However, luckily there is a solution: Darwin includes a function
> > that sets the cwd for the current thread only.
> > This should suffice to use mknod safely.
> >
> > This function (pthread_fchdir_np) is protected by a check in
> > meson in a patch later in tihs series.
> >
> > Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> > Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
> > [Will Cohen: - Adjust coding style
> >               - Replace clang references with gcc
> >               - Note radar filed with Apple for missing syscall
> >               - Replace direct syscall with pthread_fchdir_np and
> >                 adjust patch notes accordingly]
> > Signed-off-by: Will Cohen <wwcohen@gmail.com>
> > ---
> >   hw/9pfs/9p-local.c       |  5 +++--
> >   hw/9pfs/9p-util-darwin.c | 27 +++++++++++++++++++++++++++
> >   hw/9pfs/9p-util-linux.c  |  5 +++++
> >   hw/9pfs/9p-util.h        |  2 ++
> >   4 files changed, 37 insertions(+), 2 deletions(-)
>
> > diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h
> > index 8e610ad224..f6fed963bf 100644
> > --- a/hw/9pfs/9p-util.h
> > +++ b/hw/9pfs/9p-util.h
> > @@ -97,6 +97,8 @@ ssize_t flistxattrat_nofollow(int dirfd, const char
> *filename,
> >   ssize_t fremovexattrat_nofollow(int dirfd, const char *filename,
> >                                   const char *name);
> >
> > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t
> dev);
>
> I think this belong to "osdep.h" & os-posix.c.
>

[-- Attachment #2: Type: text/html, Size: 3040 bytes --]

Re: [PATCH v4 11/11] 9p: darwin: Adjust assumption on virtio-9p-test

[Thomas Huth]

On 06/02/2022 21.07, Will Cohen wrote:
> The previous test depended on the assumption that P9_DOTL_AT_REMOVEDIR
> and AT_REMOVEDIR have the same value.
> 
> While this is true on Linux, it is not true everywhere, and leads to an
> incorrect test failure on unlink_at, noticed when adding 9p to darwin:
> 
> Received response 7 (RLERROR) instead of 77 (RUNLINKAT)
> Rlerror has errno 22 (Invalid argument)
> **
> 
> ERROR:../tests/qtest/virtio-9p-test.c:305:v9fs_req_recv: assertion
> failed (hdr.id == id): (7 == 77) Bail out!
> 
> ERROR:../tests/qtest/virtio-9p-test.c:305:v9fs_req_recv: assertion
> failed (hdr.id == id): (7 == 77)
> 
> Signed-off-by: Fabian Franz <fabianfranz.oss@gmail.com>
> [Will Cohen: - Add explanation of patch and description
>                 of pre-patch test failure]
> Signed-off-by: Will Cohen <wwcohen@gmail.com>
> ---
>   tests/qtest/virtio-9p-test.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
> index 41fed41de1..6bcf89f0f8 100644
> --- a/tests/qtest/virtio-9p-test.c
> +++ b/tests/qtest/virtio-9p-test.c
> @@ -1270,7 +1270,7 @@ static void fs_unlinkat_dir(void *obj, void *data, QGuestAllocator *t_alloc)
>       /* ... and is actually a directory */
>       g_assert((st.st_mode & S_IFMT) == S_IFDIR);
>   
> -    do_unlinkat(v9p, "/", "02", AT_REMOVEDIR);
> +    do_unlinkat(v9p, "/", "02", P9_DOTL_AT_REMOVEDIR);
>       /* directory should be gone now */
>       g_assert(stat(new_dir, &st) != 0);
>   

Acked-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v4 01/11] 9p: linux: Fix a couple Linux assumptions

[Greg Kurz]

On Sun,  6 Feb 2022 15:07:09 -0500
Will Cohen <wwcohen@gmail.com> wrote:

> From: Keno Fischer <keno@juliacomputing.com>
> 
>  - Guard Linux only headers.
>  - Add qemu/statfs.h header to abstract over the which
>    headers are needed for struct statfs
>  - Define `ENOATTR` only if not only defined
>    (it's defined in system headers on Darwin).
> 
> Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> [Michael Roitzsch: - Rebase for NixOS]
> Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
> 
> While it might at first appear that fsdev/virtfs-proxy-header.c would
> need similar adjustment for darwin as file-op-9p here, a later patch in
> this series disables virtfs-proxy-helper for non-Linux. Allowing
> virtfs-proxy-helper on darwin could potentially be an additional
> optimization later.
> 
> [Will Cohen: - Fix headers for Alpine
>              - Integrate statfs.h back into file-op-9p.h
>              - Remove superfluous header guards from file-opt-9p
>              - Add note about virtfs-proxy-helper being disabled
>                on non-Linux for this patch series]
> Signed-off-by: Will Cohen <wwcohen@gmail.com>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  fsdev/file-op-9p.h   | 9 ++++++++-
>  hw/9pfs/9p-local.c   | 2 ++
>  hw/9pfs/9p.c         | 4 ++++
>  include/qemu/xattr.h | 4 +++-
>  4 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
> index 8fd89f0447..4997677460 100644
> --- a/fsdev/file-op-9p.h
> +++ b/fsdev/file-op-9p.h
> @@ -16,10 +16,17 @@
>  
>  #include <dirent.h>
>  #include <utime.h>
> -#include <sys/vfs.h>
>  #include "qemu-fsdev-throttle.h"
>  #include "p9array.h"
>  
> +#ifdef CONFIG_LINUX
> +# include <sys/vfs.h>
> +#endif
> +#ifdef CONFIG_DARWIN
> +# include <sys/param.h>
> +# include <sys/mount.h>
> +#endif
> +
>  #define SM_LOCAL_MODE_BITS    0600
>  #define SM_LOCAL_DIR_MODE_BITS    0700
>  
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index 210d9e7705..1a5e3eed73 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -32,10 +32,12 @@
>  #include "qemu/error-report.h"
>  #include "qemu/option.h"
>  #include <libgen.h>
> +#ifdef CONFIG_LINUX
>  #include <linux/fs.h>
>  #ifdef CONFIG_LINUX_MAGIC_H
>  #include <linux/magic.h>
>  #endif
> +#endif
>  #include <sys/ioctl.h>
>  
>  #ifndef XFS_SUPER_MAGIC
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 15b3f4d385..9c63e14b28 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -32,7 +32,11 @@
>  #include "migration/blocker.h"
>  #include "qemu/xxhash.h"
>  #include <math.h>
> +#ifdef CONFIG_LINUX
>  #include <linux/limits.h>
> +#else
> +#include <limits.h>
> +#endif
>  
>  int open_fd_hw;
>  int total_open_fd;
> diff --git a/include/qemu/xattr.h b/include/qemu/xattr.h
> index a83fe8e749..f1d0f7be74 100644
> --- a/include/qemu/xattr.h
> +++ b/include/qemu/xattr.h
> @@ -22,7 +22,9 @@
>  #ifdef CONFIG_LIBATTR
>  #  include <attr/xattr.h>
>  #else
> -#  define ENOATTR ENODATA
> +#  if !defined(ENOATTR)
> +#    define ENOATTR ENODATA
> +#  endif
>  #  include <sys/xattr.h>
>  #endif
>  

Re: [PATCH v4 09/11] 9p: darwin: Implement compatibility for mknodat

[Greg Kurz]

On Sun, 6 Feb 2022 20:10:23 -0500
Will Cohen <wwcohen@gmail.com> wrote:

> This patch set currently places it in 9p-util only because 9p is the only
> place where this issue seems to have come up so far and we were wary of
> editing files too far afield, but I have no attachment to its specific
> location!
> 

Inline comments are preferred on qemu-devel. Please don't top post !
This complicates the review a lot.

This is indeed a good candidate for osdep. This being said, unless there's
some other user in the QEMU code base, it is acceptable to leave it under
9pfs.

> On Sun, Feb 6, 2022 at 4:21 PM Philippe Mathieu-Daudé <f4bug@amsat.org>
> wrote:
> 
> > On 6/2/22 21:07, Will Cohen wrote:
> > > From: Keno Fischer <keno@juliacomputing.com>
> > >
> > > Darwin does not support mknodat. However, to avoid race conditions
> > > with later setting the permissions, we must avoid using mknod on
> > > the full path instead. We could try to fchdir, but that would cause
> > > problems if multiple threads try to call mknodat at the same time.
> > > However, luckily there is a solution: Darwin includes a function
> > > that sets the cwd for the current thread only.
> > > This should suffice to use mknod safely.
> > >
> > > This function (pthread_fchdir_np) is protected by a check in
> > > meson in a patch later in tihs series.
> > >
> > > Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> > > Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
> > > [Will Cohen: - Adjust coding style
> > >               - Replace clang references with gcc
> > >               - Note radar filed with Apple for missing syscall
> > >               - Replace direct syscall with pthread_fchdir_np and
> > >                 adjust patch notes accordingly]
> > > Signed-off-by: Will Cohen <wwcohen@gmail.com>
> > > ---
> > >   hw/9pfs/9p-local.c       |  5 +++--
> > >   hw/9pfs/9p-util-darwin.c | 27 +++++++++++++++++++++++++++
> > >   hw/9pfs/9p-util-linux.c  |  5 +++++
> > >   hw/9pfs/9p-util.h        |  2 ++
> > >   4 files changed, 37 insertions(+), 2 deletions(-)
> >
> > > diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h
> > > index 8e610ad224..f6fed963bf 100644
> > > --- a/hw/9pfs/9p-util.h
> > > +++ b/hw/9pfs/9p-util.h
> > > @@ -97,6 +97,8 @@ ssize_t flistxattrat_nofollow(int dirfd, const char
> > *filename,
> > >   ssize_t fremovexattrat_nofollow(int dirfd, const char *filename,
> > >                                   const char *name);
> > >
> > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t
> > dev);
> >
> > I think this belong to "osdep.h" & os-posix.c.
> >

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

[Fabian Franz]

[-- Attachment #1: Type: text/plain, Size: 5525 bytes --]

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“?

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


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

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.

>
>      }
>
>      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?


>          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)
>
>

[-- Attachment #2: Type: text/html, Size: 7453 bytes --]

Re: [PATCH v4 09/11] 9p: darwin: Implement compatibility for mknodat

[Philippe Mathieu-Daudé via]

On 7/2/22 09:47, Greg Kurz wrote:
> On Sun, 6 Feb 2022 20:10:23 -0500
> Will Cohen <wwcohen@gmail.com> wrote:
> 
>> This patch set currently places it in 9p-util only because 9p is the only
>> place where this issue seems to have come up so far and we were wary of
>> editing files too far afield, but I have no attachment to its specific
>> location!
>>
> 
> Inline comments are preferred on qemu-devel. Please don't top post !
> This complicates the review a lot.
> 
> This is indeed a good candidate for osdep. This being said, unless there's
> some other user in the QEMU code base, it is acceptable to leave it under
> 9pfs.

virtiofsd could eventually use it.

Re: [PATCH v4 09/11] 9p: darwin: Implement compatibility for mknodat

[Greg Kurz]

On Mon, 7 Feb 2022 11:30:18 +0100
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> On 7/2/22 09:47, Greg Kurz wrote:
> > On Sun, 6 Feb 2022 20:10:23 -0500
> > Will Cohen <wwcohen@gmail.com> wrote:
> > 
> >> This patch set currently places it in 9p-util only because 9p is the only
> >> place where this issue seems to have come up so far and we were wary of
> >> editing files too far afield, but I have no attachment to its specific
> >> location!
> >>
> > 
> > Inline comments are preferred on qemu-devel. Please don't top post !
> > This complicates the review a lot.
> > 
> > This is indeed a good candidate for osdep. This being said, unless there's
> > some other user in the QEMU code base, it is acceptable to leave it under
> > 9pfs.
> 
> virtiofsd could eventually use it.


Indeed but virtiofsd is for linux hosts only AFAICT and I'm not aware of any
work to support any other host OS.

Cc'ing virtio-fs people for inputs on this topic.

Re: [PATCH v4 09/11] 9p: darwin: Implement compatibility for mknodat

[Dr. David Alan Gilbert]

* Greg Kurz (groug@kaod.org) wrote:
> On Mon, 7 Feb 2022 11:30:18 +0100
> Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> 
> > On 7/2/22 09:47, Greg Kurz wrote:
> > > On Sun, 6 Feb 2022 20:10:23 -0500
> > > Will Cohen <wwcohen@gmail.com> wrote:
> > > 
> > >> This patch set currently places it in 9p-util only because 9p is the only
> > >> place where this issue seems to have come up so far and we were wary of
> > >> editing files too far afield, but I have no attachment to its specific
> > >> location!
> > >>
> > > 
> > > Inline comments are preferred on qemu-devel. Please don't top post !
> > > This complicates the review a lot.
> > > 
> > > This is indeed a good candidate for osdep. This being said, unless there's
> > > some other user in the QEMU code base, it is acceptable to leave it under
> > > 9pfs.
> > 
> > virtiofsd could eventually use it.
> 
> 
> Indeed but virtiofsd is for linux hosts only AFAICT and I'm not aware of any
> work to support any other host OS.
> 
> Cc'ing virtio-fs people for inputs on this topic.

Indeeed, there's a lot of Linux specific code in the virtiofsd - I know
people are interested in other platforms, but I'm not sure that's the
right starting point.

Dave

-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Will Cohen]

[-- Attachment #1: Type: text/plain, Size: 6429 bytes --]

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
>>  }
>>
>>  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.


>
> 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)
>>
>>

[-- Attachment #2: Type: text/html, Size: 9706 bytes --]

Re: [PATCH v4 10/11] 9p: darwin: meson: Allow VirtFS on Darwin

[Christian Schoenebeck]

On Montag, 7. Februar 2022 02:05:32 CET Will Cohen wrote:
> On Sun, Feb 6, 2022 at 4:22 PM Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
> wrote:
> > On 6/2/22 21:07, Will Cohen wrote:
> > > From: Keno Fischer <keno@juliacomputing.com>
> > > 
> > > Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> > > [Michael Roitzsch: - Rebase for NixOS]
> > > Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
> > > [Will Cohen: - Rebase to master]
> > > Signed-off-by: Will Cohen <wwcohen@gmail.com>
> > > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> > > [Will Cohen: - Add check for pthread_fchdir_np to virtfs]
> > > Signed-off-by: Will Cohen <wwcohen@gmail.com>
> > > ---
> > > 
> > >   fsdev/meson.build |  1 +
> > >   meson.build       | 14 ++++++++++----
> > >   2 files changed, 11 insertions(+), 4 deletions(-)
> > > 
> > > -have_virtfs_proxy_helper = have_virtfs and have_tools
> > > +have_virtfs_proxy_helper = targetos == 'linux' and have_virtfs and
> > 
> > have_tools
> > 
> > Why do you restrict the proxy-helper to Linux?
>
> Only because porting the proxy-helper to macOS is outside the scope of this
> particular patch. While some initial concepts around it have been
> considered by some of the contributors to this patch, those implementations
> weren't tested enough and the security implications weren't considered in
> full. We assume that this could be an additional implementation later on,
> if the functionality is considered important down the road.

In general that's fine with me. I would have probably made that
"targetos != 'darwin'" instead of "targetos == 'linux'", but I leave that up 
to you.

On the long term we will probably deprecate the 9p 'proxy' fs driver anyway. 
While it had some good ideas, being realistic though: nobody has worked on the 
9p proxy driver/backend for many years and it is not in good shape.

I can imagine that due to the ground being laid by these series, that we will 
also open 9p for BSD, but that should be done a bit later and hence does not 
belong into these series.

But once again: it would not have hurt to make your intentions clear either in 
the commit log or by in-source comment. :)

Best regards,
Christian Schoenebeck

Re: [PATCH v4 10/11] 9p: darwin: meson: Allow VirtFS on Darwin

[Will Cohen]

[-- Attachment #1: Type: text/plain, Size: 2558 bytes --]

On Mon, Feb 7, 2022 at 9:15 AM Christian Schoenebeck <qemu_oss@crudebyte.com>
wrote:

> On Montag, 7. Februar 2022 02:05:32 CET Will Cohen wrote:
> > On Sun, Feb 6, 2022 at 4:22 PM Philippe Mathieu-Daudé <f4bug@amsat.org>
> >
> > wrote:
> > > On 6/2/22 21:07, Will Cohen wrote:
> > > > From: Keno Fischer <keno@juliacomputing.com>
> > > >
> > > > Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> > > > [Michael Roitzsch: - Rebase for NixOS]
> > > > Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
> > > > [Will Cohen: - Rebase to master]
> > > > Signed-off-by: Will Cohen <wwcohen@gmail.com>
> > > > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> > > > [Will Cohen: - Add check for pthread_fchdir_np to virtfs]
> > > > Signed-off-by: Will Cohen <wwcohen@gmail.com>
> > > > ---
> > > >
> > > >   fsdev/meson.build |  1 +
> > > >   meson.build       | 14 ++++++++++----
> > > >   2 files changed, 11 insertions(+), 4 deletions(-)
> > > >
> > > > -have_virtfs_proxy_helper = have_virtfs and have_tools
> > > > +have_virtfs_proxy_helper = targetos == 'linux' and have_virtfs and
> > >
> > > have_tools
> > >
> > > Why do you restrict the proxy-helper to Linux?
> >
> > Only because porting the proxy-helper to macOS is outside the scope of
> this
> > particular patch. While some initial concepts around it have been
> > considered by some of the contributors to this patch, those
> implementations
> > weren't tested enough and the security implications weren't considered in
> > full. We assume that this could be an additional implementation later on,
> > if the functionality is considered important down the road.
>
> In general that's fine with me. I would have probably made that
> "targetos != 'darwin'" instead of "targetos == 'linux'", but I leave that
> up
> to you.
>
> On the long term we will probably deprecate the 9p 'proxy' fs driver
> anyway.
> While it had some good ideas, being realistic though: nobody has worked on
> the
> 9p proxy driver/backend for many years and it is not in good shape.
>
> I can imagine that due to the ground being laid by these series, that we
> will
> also open 9p for BSD, but that should be done a bit later and hence does
> not
> belong into these series.
>
> But once again: it would not have hurt to make your intentions clear
> either in
> the commit log or by in-source comment. :)
>
> Best regards,
> Christian Schoenebeck
>
>
>
Acknowledged! For v5 will change it to != 'darwin' and note it as well, for
clarity.

[-- Attachment #2: Type: text/html, Size: 3742 bytes --]

Re: [PATCH v4 09/11] 9p: darwin: Implement compatibility for mknodat

[Christian Schoenebeck]

On Montag, 7. Februar 2022 11:57:25 CET Dr. David Alan Gilbert wrote:
> * Greg Kurz (groug@kaod.org) wrote:
> > On Mon, 7 Feb 2022 11:30:18 +0100
> > 
> > Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> > > On 7/2/22 09:47, Greg Kurz wrote:
> > > > On Sun, 6 Feb 2022 20:10:23 -0500
> > > > 
> > > > Will Cohen <wwcohen@gmail.com> wrote:
> > > >> This patch set currently places it in 9p-util only because 9p is the
> > > >> only
> > > >> place where this issue seems to have come up so far and we were wary
> > > >> of
> > > >> editing files too far afield, but I have no attachment to its
> > > >> specific
> > > >> location!
> > > > 
> > > > Inline comments are preferred on qemu-devel. Please don't top post !
> > > > This complicates the review a lot.
> > > > 
> > > > This is indeed a good candidate for osdep. This being said, unless
> > > > there's
> > > > some other user in the QEMU code base, it is acceptable to leave it
> > > > under
> > > > 9pfs.
> > > 
> > > virtiofsd could eventually use it.
> > 
> > Indeed but virtiofsd is for linux hosts only AFAICT and I'm not aware of
> > any work to support any other host OS.
> > 
> > Cc'ing virtio-fs people for inputs on this topic.
> 
> Indeeed, there's a lot of Linux specific code in the virtiofsd - I know
> people are interested in other platforms, but I'm not sure that's the
> right starting point.
> 
> Dave

Agreeing with Greg here: i.e. I would have placed this into osdep, but I would 
not insist on it either.

Best regards,
Christian Schoenebeck

Re: [PATCH v4 10/11] 9p: darwin: meson: Allow VirtFS on Darwin

[Christian Schoenebeck]

On Sonntag, 6. Februar 2022 21:07:18 CET Will Cohen wrote:
> From: Keno Fischer <keno@juliacomputing.com>
> 
> Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> [Michael Roitzsch: - Rebase for NixOS]
> Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
> [Will Cohen: - Rebase to master]
> Signed-off-by: Will Cohen <wwcohen@gmail.com>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> [Will Cohen: - Add check for pthread_fchdir_np to virtfs]
> Signed-off-by: Will Cohen <wwcohen@gmail.com>
> ---
>  fsdev/meson.build |  1 +
>  meson.build       | 14 ++++++++++----
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/fsdev/meson.build b/fsdev/meson.build
> index adf57cc43e..b632b66348 100644
> --- a/fsdev/meson.build
> +++ b/fsdev/meson.build
> @@ -7,6 +7,7 @@ fsdev_ss.add(when: ['CONFIG_FSDEV_9P'], if_true: files(
>    'qemu-fsdev.c',
>  ), if_false: files('qemu-fsdev-dummy.c'))
>  softmmu_ss.add_all(when: 'CONFIG_LINUX', if_true: fsdev_ss)
> +softmmu_ss.add_all(when: 'CONFIG_DARWIN', if_true: fsdev_ss)
> 
>  if have_virtfs_proxy_helper
>    executable('virtfs-proxy-helper',
> diff --git a/meson.build b/meson.build
> index 5f43355071..6b4adf7e15 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1421,17 +1421,23 @@ if not get_option('dbus_display').disabled()
>    endif
>  endif
> 
> -have_virtfs = (targetos == 'linux' and
> +if targetos == 'darwin' and cc.has_function('pthread_fchdir_np')
> +  have_virtfs = have_system

As you are going for a v5 anyway: I would add an error message here if 
pthread_fchdir_np() is not available. Because it is a bit frustrating for 
users if their options silently got ignored without any indication why.

> +else
> +  have_virtfs = (targetos == 'linux' and
>      have_system and
>      libattr.found() and
>      libcap_ng.found())
> +endif
> 
> -have_virtfs_proxy_helper = have_virtfs and have_tools
> +have_virtfs_proxy_helper = targetos == 'linux' and have_virtfs and
> have_tools
> 
>  if get_option('virtfs').enabled()
>    if not have_virtfs
> -    if targetos != 'linux'
> -      error('virtio-9p (virtfs) requires Linux')
> +    if targetos != 'linux' and targetos != 'darwin'
> +      error('virtio-9p (virtfs) requires Linux or Darwin')
> +    elif targetos == 'darwin' and not cc.has_function('pthread_fchdir_np')
> +      error('virtio-9p (virtfs) on Darwin requires the presence of
> pthread_fchdir_np') elif not libcap_ng.found() or not libattr.found()
>        error('virtio-9p (virtfs) requires libcap-ng-devel and
> libattr-devel') elif not have_system

Re: [PATCH v4 10/11] 9p: darwin: meson: Allow VirtFS on Darwin

[Will Cohen]

[-- Attachment #1: Type: text/plain, Size: 3200 bytes --]

On Mon, Feb 7, 2022 at 9:27 AM Christian Schoenebeck <qemu_oss@crudebyte.com>
wrote:

> On Sonntag, 6. Februar 2022 21:07:18 CET Will Cohen wrote:
> > From: Keno Fischer <keno@juliacomputing.com>
> >
> > Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> > [Michael Roitzsch: - Rebase for NixOS]
> > Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
> > [Will Cohen: - Rebase to master]
> > Signed-off-by: Will Cohen <wwcohen@gmail.com>
> > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> > [Will Cohen: - Add check for pthread_fchdir_np to virtfs]
> > Signed-off-by: Will Cohen <wwcohen@gmail.com>
> > ---
> >  fsdev/meson.build |  1 +
> >  meson.build       | 14 ++++++++++----
> >  2 files changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/fsdev/meson.build b/fsdev/meson.build
> > index adf57cc43e..b632b66348 100644
> > --- a/fsdev/meson.build
> > +++ b/fsdev/meson.build
> > @@ -7,6 +7,7 @@ fsdev_ss.add(when: ['CONFIG_FSDEV_9P'], if_true: files(
> >    'qemu-fsdev.c',
> >  ), if_false: files('qemu-fsdev-dummy.c'))
> >  softmmu_ss.add_all(when: 'CONFIG_LINUX', if_true: fsdev_ss)
> > +softmmu_ss.add_all(when: 'CONFIG_DARWIN', if_true: fsdev_ss)
> >
> >  if have_virtfs_proxy_helper
> >    executable('virtfs-proxy-helper',
> > diff --git a/meson.build b/meson.build
> > index 5f43355071..6b4adf7e15 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -1421,17 +1421,23 @@ if not get_option('dbus_display').disabled()
> >    endif
> >  endif
> >
> > -have_virtfs = (targetos == 'linux' and
> > +if targetos == 'darwin' and cc.has_function('pthread_fchdir_np')
> > +  have_virtfs = have_system
>
> As you are going for a v5 anyway: I would add an error message here if
> pthread_fchdir_np() is not available. Because it is a bit frustrating for
> users if their options silently got ignored without any indication why.
>
> > +else
> > +  have_virtfs = (targetos == 'linux' and
> >      have_system and
> >      libattr.found() and
> >      libcap_ng.found())
> > +endif
> >
> > -have_virtfs_proxy_helper = have_virtfs and have_tools
> > +have_virtfs_proxy_helper = targetos == 'linux' and have_virtfs and
> > have_tools
> >
> >  if get_option('virtfs').enabled()
> >    if not have_virtfs
> > -    if targetos != 'linux'
> > -      error('virtio-9p (virtfs) requires Linux')
> > +    if targetos != 'linux' and targetos != 'darwin'
> > +      error('virtio-9p (virtfs) requires Linux or Darwin')
> > +    elif targetos == 'darwin' and not
> cc.has_function('pthread_fchdir_np')
> > +      error('virtio-9p (virtfs) on Darwin requires the presence of
> > pthread_fchdir_np')


Does the error message here suffice for that need? Right now if they're
running a system without pthread_fchdir_np and don't specify the option, I
think it'll just quietly disable, but if they --enable-virtfs and the
function isn't there, they should get a note. I assume this is better, so
that the ability to compile isn't contingent on having the latest OS, even
if full support for older OSes isn't provided.


> elif not libcap_ng.found() or not libattr.found()
> >        error('virtio-9p (virtfs) requires libcap-ng-devel and
> > libattr-devel') elif not have_system
>
>
>

[-- Attachment #2: Type: text/html, Size: 4710 bytes --]

Re: [PATCH v4 10/11] 9p: darwin: meson: Allow VirtFS on Darwin

[Greg Kurz]

On Mon, 07 Feb 2022 15:15:46 +0100
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Montag, 7. Februar 2022 02:05:32 CET Will Cohen wrote:
> > On Sun, Feb 6, 2022 at 4:22 PM Philippe Mathieu-Daudé <f4bug@amsat.org>
> > 
> > wrote:
> > > On 6/2/22 21:07, Will Cohen wrote:
> > > > From: Keno Fischer <keno@juliacomputing.com>
> > > > 
> > > > Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> > > > [Michael Roitzsch: - Rebase for NixOS]
> > > > Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
> > > > [Will Cohen: - Rebase to master]
> > > > Signed-off-by: Will Cohen <wwcohen@gmail.com>
> > > > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> > > > [Will Cohen: - Add check for pthread_fchdir_np to virtfs]
> > > > Signed-off-by: Will Cohen <wwcohen@gmail.com>
> > > > ---
> > > > 
> > > >   fsdev/meson.build |  1 +
> > > >   meson.build       | 14 ++++++++++----
> > > >   2 files changed, 11 insertions(+), 4 deletions(-)
> > > > 
> > > > -have_virtfs_proxy_helper = have_virtfs and have_tools
> > > > +have_virtfs_proxy_helper = targetos == 'linux' and have_virtfs and
> > > 
> > > have_tools
> > > 
> > > Why do you restrict the proxy-helper to Linux?
> >
> > Only because porting the proxy-helper to macOS is outside the scope of this
> > particular patch. While some initial concepts around it have been
> > considered by some of the contributors to this patch, those implementations
> > weren't tested enough and the security implications weren't considered in
> > full. We assume that this could be an additional implementation later on,
> > if the functionality is considered important down the road.
> 
> In general that's fine with me. I would have probably made that
> "targetos != 'darwin'" instead of "targetos == 'linux'", but I leave that up 
> to you.
> 
> On the long term we will probably deprecate the 9p 'proxy' fs driver anyway. 
> While it had some good ideas, being realistic though: nobody has worked on the 
> 9p proxy driver/backend for many years and it is not in good shape.
> 

It definitely isn't indeed. Also it is super slow by design
since the round trip of a 9p request involves QEMU on both entry
and exit:

   [guest] --> [QEMU]--> [virtfs-proxy-helper]-->[QEMU]-->[guest]

A more modern and efficient approach would be to have a vhost-user-9p
implementation : requests would be directly handled by the external
process, without QEMU hops. But this a fair amount of work.

> I can imagine that due to the ground being laid by these series, that we will 
> also open 9p for BSD, but that should be done a bit later and hence does not 
> belong into these series.
> 
> But once again: it would not have hurt to make your intentions clear either in 
> the commit log or by in-source comment. :)
> 
> Best regards,
> Christian Schoenebeck
> 
> 

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

[Christian Schoenebeck]

On Sonntag, 6. Februar 2022 21:07:12 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]
> [Will Cohen: - Ensure that telldir error handling uses
>                signed int]
> Signed-off-by: Fabian Franz <fabianfranz.oss@gmail.com>
> Signed-off-by: Will Cohen <wwcohen@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.

> 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.

> +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?

> 
>      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?

>          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.

>      }
> 
>      /* restore (last) saved position */

Re: [PATCH v4 00/11] 9p: Add support for darwin

[Christian Schoenebeck]

On Sonntag, 6. Februar 2022 21:07:08 CET Will Cohen wrote:
> This is a followup to
> https://lists.nongnu.org/archive/html/qemu-devel/2022-01/msg05993.html,
> adding 9p server support for Darwin.
> 
> Since v3, the following changes have been made:
> 
> - Move XATTR_SIZE_MAX to P9_XATTR_SIZE MAX in 9p.h, and provide explanatory
> context as preliminary solution - Add explanatory note surrounding
> virtio-9p-test with output of pre-patch failing test - Remove superfluous
> header guards from file-opt-9p
> - Add note about virtfs-proxy-helper being disabled on non-linux for this
> patch series - Note radar filed with Apple for missing mknodat syscall
> - Replace direct syscall to pthread_fchdir with pthread_fchdir_np, and add
> check for this function’s presence in meson - Ensure that d_seekoff is
> filled using telldir on darwin, and create qemu_dirent_off helper to decide
> which to access. - Ensure that [amc]tim.tv_sec are all initialized
> alongside [amc]tim.tv_nsec in 9p-proxy - Ensure that all patch email
> addresses are valid
> - Add telldir error handling for dirent on darwin

As this series already has seen some revisions and is on a good way to become 
queued soon: it is helpful to immediately see here which patches exactly got 
changed, because some of the patches already look fine.

For already reviewed patches that you won't change: you can take over people's 
reviewed-by tags in the next revision.

Best regards,
Christian Schoenebeck

> Keno Fischer (10):
>   9p: linux: Fix a couple Linux assumptions
>   9p: Rename 9p-util -> 9p-util-linux
>   9p: darwin: Handle struct stat(fs) differences
>   9p: darwin: Handle struct dirent differences
>   9p: darwin: Ignore O_{NOATIME, DIRECT}
>   9p: darwin: Move XATTR_SIZE_MAX->P9_XATTR_SIZE_MAX
>   9p: darwin: *xattr_nofollow implementations
>   9p: darwin: Compatibility for f/l*xattr
>   9p: darwin: Implement compatibility for mknodat
>   9p: darwin: meson: Allow VirtFS on Darwin
> 
> Will Cohen (1):
>   9p: darwin: Adjust assumption on virtio-9p-test
> 
>  fsdev/file-op-9p.h                     |  9 ++-
>  fsdev/meson.build                      |  1 +
>  hw/9pfs/9p-local.c                     | 28 ++++++--
>  hw/9pfs/9p-proxy.c                     | 38 ++++++++++-
>  hw/9pfs/9p-synth.c                     |  6 ++
>  hw/9pfs/9p-util-darwin.c               | 91 ++++++++++++++++++++++++++
>  hw/9pfs/{9p-util.c => 9p-util-linux.c} |  7 +-
>  hw/9pfs/9p-util.h                      | 38 +++++++++++
>  hw/9pfs/9p.c                           | 50 ++++++++++++--
>  hw/9pfs/9p.h                           | 11 ++++
>  hw/9pfs/codir.c                        |  7 ++
>  hw/9pfs/meson.build                    |  3 +-
>  include/qemu/xattr.h                   |  4 +-
>  meson.build                            | 14 ++--
>  tests/qtest/virtio-9p-test.c           |  2 +-
>  15 files changed, 285 insertions(+), 24 deletions(-)
>  create mode 100644 hw/9pfs/9p-util-darwin.c
>  rename hw/9pfs/{9p-util.c => 9p-util-linux.c} (90%)

Re: [PATCH v4 00/11] 9p: Add support for darwin

[Will Cohen]

[-- Attachment #1: Type: text/plain, Size: 3980 bytes --]

On Mon, Feb 7, 2022 at 9:47 AM Christian Schoenebeck <qemu_oss@crudebyte.com>
wrote:

> On Sonntag, 6. Februar 2022 21:07:08 CET Will Cohen wrote:
> > This is a followup to
> > https://lists.nongnu.org/archive/html/qemu-devel/2022-01/msg05993.html,
> > adding 9p server support for Darwin.
> >
> > Since v3, the following changes have been made:
> >
> > - Move XATTR_SIZE_MAX to P9_XATTR_SIZE MAX in 9p.h, and provide
> explanatory
> > context as preliminary solution - Add explanatory note surrounding
> > virtio-9p-test with output of pre-patch failing test - Remove superfluous
> > header guards from file-opt-9p
> > - Add note about virtfs-proxy-helper being disabled on non-linux for this
> > patch series - Note radar filed with Apple for missing mknodat syscall
> > - Replace direct syscall to pthread_fchdir with pthread_fchdir_np, and
> add
> > check for this function’s presence in meson - Ensure that d_seekoff is
> > filled using telldir on darwin, and create qemu_dirent_off helper to
> decide
> > which to access. - Ensure that [amc]tim.tv_sec are all initialized
> > alongside [amc]tim.tv_nsec in 9p-proxy - Ensure that all patch email
> > addresses are valid
> > - Add telldir error handling for dirent on darwin
>
> As this series already has seen some revisions and is on a good way to
> become
> queued soon: it is helpful to immediately see here which patches exactly
> got
> changed, because some of the patches already look fine.
>
> For already reviewed patches that you won't change: you can take over
> people's
> reviewed-by tags in the next revision.
>
> Best regards,
> Christian Schoenebeck
>
> > Keno Fischer (10):
> >   9p: linux: Fix a couple Linux assumptions
> >   9p: Rename 9p-util -> 9p-util-linux
> >   9p: darwin: Handle struct stat(fs) differences
> >   9p: darwin: Handle struct dirent differences
> >   9p: darwin: Ignore O_{NOATIME, DIRECT}
> >   9p: darwin: Move XATTR_SIZE_MAX->P9_XATTR_SIZE_MAX
> >   9p: darwin: *xattr_nofollow implementations
> >   9p: darwin: Compatibility for f/l*xattr
> >   9p: darwin: Implement compatibility for mknodat
> >   9p: darwin: meson: Allow VirtFS on Darwin
> >
> > Will Cohen (1):
> >   9p: darwin: Adjust assumption on virtio-9p-test
> >
> >  fsdev/file-op-9p.h                     |  9 ++-
> >  fsdev/meson.build                      |  1 +
> >  hw/9pfs/9p-local.c                     | 28 ++++++--
> >  hw/9pfs/9p-proxy.c                     | 38 ++++++++++-
> >  hw/9pfs/9p-synth.c                     |  6 ++
> >  hw/9pfs/9p-util-darwin.c               | 91 ++++++++++++++++++++++++++
> >  hw/9pfs/{9p-util.c => 9p-util-linux.c} |  7 +-
> >  hw/9pfs/9p-util.h                      | 38 +++++++++++
> >  hw/9pfs/9p.c                           | 50 ++++++++++++--
> >  hw/9pfs/9p.h                           | 11 ++++
> >  hw/9pfs/codir.c                        |  7 ++
> >  hw/9pfs/meson.build                    |  3 +-
> >  include/qemu/xattr.h                   |  4 +-
> >  meson.build                            | 14 ++--
> >  tests/qtest/virtio-9p-test.c           |  2 +-
> >  15 files changed, 285 insertions(+), 24 deletions(-)
> >  create mode 100644 hw/9pfs/9p-util-darwin.c
> >  rename hw/9pfs/{9p-util.c => 9p-util-linux.c} (90%)
>
>
Understood! Previous reviewed-by tags are already in v4, and these new ones
will go into v5!

For reference in terms of changes, patches that were touched in v4:

Keno Fischer (10):
9p: linux: Fix a couple Linux assumptions (1/11)
9p: darwin: Handle struct stat(fs) differences (3/11)
9p: darwin: Handle struct dirent differences (4/11)
9p: darwin: Move XATTR_SIZE_MAX->P9_XATTR_SIZE_MAX (5/11)
9p: darwin: Implement compatibility for mknodat (9/11)
9p: darwin: meson: Allow VirtFS on Darwin (10/11)

Will Cohen (1):
9p: darwin: Adjust assumption on virtio-9p-test (11/11)

Additional changes to patches will be highlighted in the opening note for
v5.

[-- Attachment #2: Type: text/html, Size: 5052 bytes --]

Re: [PATCH v4 10/11] 9p: darwin: meson: Allow VirtFS on Darwin

[Christian Schoenebeck]

On Montag, 7. Februar 2022 15:37:00 CET Will Cohen wrote:
> On Mon, Feb 7, 2022 at 9:27 AM Christian Schoenebeck
> <qemu_oss@crudebyte.com>
> wrote:
> > On Sonntag, 6. Februar 2022 21:07:18 CET Will Cohen wrote:
> > > From: Keno Fischer <keno@juliacomputing.com>
> > > 
> > > Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> > > [Michael Roitzsch: - Rebase for NixOS]
> > > Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
> > > [Will Cohen: - Rebase to master]
> > > Signed-off-by: Will Cohen <wwcohen@gmail.com>
> > > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> > > [Will Cohen: - Add check for pthread_fchdir_np to virtfs]
> > > Signed-off-by: Will Cohen <wwcohen@gmail.com>
> > > ---
> > > 
> > >  fsdev/meson.build |  1 +
> > >  meson.build       | 14 ++++++++++----
> > >  2 files changed, 11 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/fsdev/meson.build b/fsdev/meson.build
> > > index adf57cc43e..b632b66348 100644
> > > --- a/fsdev/meson.build
> > > +++ b/fsdev/meson.build
> > > @@ -7,6 +7,7 @@ fsdev_ss.add(when: ['CONFIG_FSDEV_9P'], if_true: files(
> > > 
> > >    'qemu-fsdev.c',
> > >  
> > >  ), if_false: files('qemu-fsdev-dummy.c'))
> > >  softmmu_ss.add_all(when: 'CONFIG_LINUX', if_true: fsdev_ss)
> > > 
> > > +softmmu_ss.add_all(when: 'CONFIG_DARWIN', if_true: fsdev_ss)
> > > 
> > >  if have_virtfs_proxy_helper
> > >  
> > >    executable('virtfs-proxy-helper',
> > > 
> > > diff --git a/meson.build b/meson.build
> > > index 5f43355071..6b4adf7e15 100644
> > > --- a/meson.build
> > > +++ b/meson.build
> > > @@ -1421,17 +1421,23 @@ if not get_option('dbus_display').disabled()
> > > 
> > >    endif
> > >  
> > >  endif
> > > 
> > > -have_virtfs = (targetos == 'linux' and
> > > +if targetos == 'darwin' and cc.has_function('pthread_fchdir_np')
> > > +  have_virtfs = have_system
> > 
> > As you are going for a v5 anyway: I would add an error message here if
> > pthread_fchdir_np() is not available. Because it is a bit frustrating for
> > users if their options silently got ignored without any indication why.
> > 
> > > +else
> > > +  have_virtfs = (targetos == 'linux' and
> > > 
> > >      have_system and
> > >      libattr.found() and
> > >      libcap_ng.found())
> > > 
> > > +endif
> > > 
> > > -have_virtfs_proxy_helper = have_virtfs and have_tools
> > > +have_virtfs_proxy_helper = targetos == 'linux' and have_virtfs and
> > > have_tools
> > > 
> > >  if get_option('virtfs').enabled()
> > >  
> > >    if not have_virtfs
> > > 
> > > -    if targetos != 'linux'
> > > -      error('virtio-9p (virtfs) requires Linux')
> > > +    if targetos != 'linux' and targetos != 'darwin'
> > > +      error('virtio-9p (virtfs) requires Linux or Darwin')
> > > +    elif targetos == 'darwin' and not
> > 
> > cc.has_function('pthread_fchdir_np')
> > 
> > > +      error('virtio-9p (virtfs) on Darwin requires the presence of
> > > pthread_fchdir_np')
> 
> Does the error message here suffice for that need? Right now if they're
> running a system without pthread_fchdir_np and don't specify the option, I
> think it'll just quietly disable, but if they --enable-virtfs and the
> function isn't there, they should get a note. I assume this is better, so
> that the ability to compile isn't contingent on having the latest OS, even
> if full support for older OSes isn't provided.

Ah, got it. Yes, makes sense.

But what I would definitely change is the precise error message text here: 
"Darwin" is a bit awkward for a regular user, because most macOS users never 
heard of "Darwin" in the context of Apple systems before. Using the term 
"darwin" in code is fine as it can be assumed that developers know the 
background, but as for regular users I would make it more clear that this is 
actually about macOS, e.g:

	error('virtio-9p (virtfs) requires either Linux or Darwin (macOS)')

I don't mind how to write that exactly; braces, slash, replacing Darwin by 
macOS or whatever, but it should mention 'macOS' here in some form.

> > elif not libcap_ng.found() or not libattr.found()
> > 
> > >        error('virtio-9p (virtfs) requires libcap-ng-devel and
> > > 
> > > libattr-devel') elif not have_system

Re: [PATCH v4 09/11] 9p: darwin: Implement compatibility for mknodat

[Vivek Goyal]

On Mon, Feb 07, 2022 at 11:49:12AM +0100, Greg Kurz wrote:
> On Mon, 7 Feb 2022 11:30:18 +0100
> Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> 
> > On 7/2/22 09:47, Greg Kurz wrote:
> > > On Sun, 6 Feb 2022 20:10:23 -0500
> > > Will Cohen <wwcohen@gmail.com> wrote:
> > > 
> > >> This patch set currently places it in 9p-util only because 9p is the only
> > >> place where this issue seems to have come up so far and we were wary of
> > >> editing files too far afield, but I have no attachment to its specific
> > >> location!
> > >>
> > > 
> > > Inline comments are preferred on qemu-devel. Please don't top post !
> > > This complicates the review a lot.
> > > 
> > > This is indeed a good candidate for osdep. This being said, unless there's
> > > some other user in the QEMU code base, it is acceptable to leave it under
> > > 9pfs.
> > 
> > virtiofsd could eventually use it.
> 
> 
> Indeed but virtiofsd is for linux hosts only AFAICT and I'm not aware of any
> work to support any other host OS.

[ CC Sergio ]

Will like to support virtiofs on other host OS. Getting rid of Linux
specific parts should be doable. I think bigger challenge is how to
make vhost-user stuff work on other OS, like macOS.

If virtiofsd was somehow running as part of qemu (and not as a separate
process), then making rest of the filesystem code to work on other
OS should not be too hard, I guess.

So question is, can one somehow run same virtiofsd code both as part
of qemu as well as separate daemon based on need (and one does not have
to maintain two separate code bases).

Thanks
Vivek

> 
> Cc'ing virtio-fs people for inputs on this topic.
> 

Re: [PATCH v4 10/11] 9p: darwin: meson: Allow VirtFS on Darwin

[Christian Schoenebeck]

On Montag, 7. Februar 2022 15:39:30 CET Greg Kurz wrote:
> On Mon, 07 Feb 2022 15:15:46 +0100
> 
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > On Montag, 7. Februar 2022 02:05:32 CET Will Cohen wrote:
> > > On Sun, Feb 6, 2022 at 4:22 PM Philippe Mathieu-Daudé <f4bug@amsat.org>
> > > 
> > > wrote:
> > > > On 6/2/22 21:07, Will Cohen wrote:
> > > > > From: Keno Fischer <keno@juliacomputing.com>
> > > > > 
> > > > > Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> > > > > [Michael Roitzsch: - Rebase for NixOS]
> > > > > Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
> > > > > [Will Cohen: - Rebase to master]
> > > > > Signed-off-by: Will Cohen <wwcohen@gmail.com>
> > > > > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> > > > > [Will Cohen: - Add check for pthread_fchdir_np to virtfs]
> > > > > Signed-off-by: Will Cohen <wwcohen@gmail.com>
> > > > > ---
> > > > > 
> > > > >   fsdev/meson.build |  1 +
> > > > >   meson.build       | 14 ++++++++++----
> > > > >   2 files changed, 11 insertions(+), 4 deletions(-)
> > > > > 
> > > > > -have_virtfs_proxy_helper = have_virtfs and have_tools
> > > > > +have_virtfs_proxy_helper = targetos == 'linux' and have_virtfs and
> > > > 
> > > > have_tools
> > > > 
> > > > Why do you restrict the proxy-helper to Linux?
> > > 
> > > Only because porting the proxy-helper to macOS is outside the scope of
> > > this
> > > particular patch. While some initial concepts around it have been
> > > considered by some of the contributors to this patch, those
> > > implementations
> > > weren't tested enough and the security implications weren't considered
> > > in
> > > full. We assume that this could be an additional implementation later
> > > on,
> > > if the functionality is considered important down the road.
> > 
> > In general that's fine with me. I would have probably made that
> > "targetos != 'darwin'" instead of "targetos == 'linux'", but I leave that
> > up to you.
> > 
> > On the long term we will probably deprecate the 9p 'proxy' fs driver
> > anyway. While it had some good ideas, being realistic though: nobody has
> > worked on the 9p proxy driver/backend for many years and it is not in
> > good shape.
> It definitely isn't indeed. Also it is super slow by design
> since the round trip of a 9p request involves QEMU on both entry
> and exit:
> 
>    [guest] --> [QEMU]--> [virtfs-proxy-helper]-->[QEMU]-->[guest]
> 
> A more modern and efficient approach would be to have a vhost-user-9p
> implementation : requests would be directly handled by the external
> process, without QEMU hops. But this a fair amount of work.

That's already a bit offtopic, but how would you imagine that to work? You 
mean a system dependent solution that e.g. plugs in into KVM or something?

> > I can imagine that due to the ground being laid by these series, that we
> > will also open 9p for BSD, but that should be done a bit later and hence
> > does not belong into these series.
> > 
> > But once again: it would not have hurt to make your intentions clear
> > either in the commit log or by in-source comment. :)
> > 
> > Best regards,
> > Christian Schoenebeck

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

[Will Cohen]

[-- Attachment #1: Type: text/plain, Size: 7328 bytes --]

On Mon, Feb 7, 2022 at 9:41 AM Christian Schoenebeck <qemu_oss@crudebyte.com>
wrote:

> On Sonntag, 6. Februar 2022 21:07:12 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]
> > [Will Cohen: - Ensure that telldir error handling uses
> >                signed int]
> > Signed-off-by: Fabian Franz <fabianfranz.oss@gmail.com>
> > Signed-off-by: Will Cohen <wwcohen@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 */
>
>
>

[-- Attachment #2: Type: text/html, Size: 10549 bytes --]

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

[Christian Schoenebeck]

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)

Re: [PATCH v4 09/11] 9p: darwin: Implement compatibility for mknodat

[Will Cohen]

[-- Attachment #1: Type: text/plain, Size: 6294 bytes --]

On Mon, Feb 7, 2022 at 9:21 AM Christian Schoenebeck <qemu_oss@crudebyte.com>
wrote:

> On Montag, 7. Februar 2022 11:57:25 CET Dr. David Alan Gilbert wrote:
> > * Greg Kurz (groug@kaod.org) wrote:
> > > On Mon, 7 Feb 2022 11:30:18 +0100
> > >
> > > Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> > > > On 7/2/22 09:47, Greg Kurz wrote:
> > > > > On Sun, 6 Feb 2022 20:10:23 -0500
> > > > >
> > > > > Will Cohen <wwcohen@gmail.com> wrote:
> > > > >> This patch set currently places it in 9p-util only because 9p is
> the
> > > > >> only
> > > > >> place where this issue seems to have come up so far and we were
> wary
> > > > >> of
> > > > >> editing files too far afield, but I have no attachment to its
> > > > >> specific
> > > > >> location!
> > > > >
> > > > > Inline comments are preferred on qemu-devel. Please don't top post
> !
> > > > > This complicates the review a lot.
> > > > >
> > > > > This is indeed a good candidate for osdep. This being said, unless
> > > > > there's
> > > > > some other user in the QEMU code base, it is acceptable to leave it
> > > > > under
> > > > > 9pfs.
> > > >
> > > > virtiofsd could eventually use it.
> > >
> > > Indeed but virtiofsd is for linux hosts only AFAICT and I'm not aware
> of
> > > any work to support any other host OS.
> > >
> > > Cc'ing virtio-fs people for inputs on this topic.
> >
> > Indeeed, there's a lot of Linux specific code in the virtiofsd - I know
> > people are interested in other platforms, but I'm not sure that's the
> > right starting point.
> >
> > Dave
>
> Agreeing with Greg here: i.e. I would have placed this into osdep, but I
> would
> not insist on it either.
>
> Best regards,
> Christian Schoenebeck
>
>
This makes sense. A revised version of this patch, moving qemu_mknodat from
9p-util to osdep and os-posix, is attached below. I'd appreciate any
feedback from those looped in here, so that the context isn't lost before
resubmitting as a v5 patch, especially since this is starting to touch
files outside of 9p.

From c9713c87163da7c96b5357d0d85ac318ae3d3051 Mon Sep 17 00:00:00 2001
From: Keno Fischer <keno@juliacomputing.com>
Date: Sat, 16 Jun 2018 20:56:55 -0400
Subject: [PATCH] 9p: darwin: Implement compatibility for mknodat

Darwin does not support mknodat. However, to avoid race conditions
with later setting the permissions, we must avoid using mknod on
the full path instead. We could try to fchdir, but that would cause
problems if multiple threads try to call mknodat at the same time.
However, luckily there is a solution: Darwin includes a function
that sets the cwd for the current thread only.
This should suffice to use mknod safely.

This function (pthread_fchdir_np) is protected by a check in
meson in a patch later in tihs series.

Signed-off-by: Keno Fischer <keno@juliacomputing.com>
Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
[Will Cohen: - Adjust coding style
             - Replace clang references with gcc
             - Note radar filed with Apple for missing syscall
             - Replace direct syscall with pthread_fchdir_np and
               adjust patch notes accordingly
             - Move qemu_mknodat from 9p-util to osdep and os-posix]
Signed-off-by: Will Cohen <wwcohen@gmail.com>
---
 hw/9pfs/9p-local.c   |  4 ++--
 include/qemu/osdep.h | 10 ++++++++++
 os-posix.c           | 34 ++++++++++++++++++++++++++++++++++
 3 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index a0d08e5216..d42ce6d8b8 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -682,7 +682,7 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath
*dir_path,

     if (fs_ctx->export_flags & V9FS_SM_MAPPED ||
         fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
-        err = mknodat(dirfd, name, fs_ctx->fmode | S_IFREG, 0);
+        err = qemu_mknodat(dirfd, name, fs_ctx->fmode | S_IFREG, 0);
         if (err == -1) {
             goto out;
         }
@@ -697,7 +697,7 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath
*dir_path,
         }
     } else if (fs_ctx->export_flags & V9FS_SM_PASSTHROUGH ||
                fs_ctx->export_flags & V9FS_SM_NONE) {
-        err = mknodat(dirfd, name, credp->fc_mode, credp->fc_rdev);
+        err = qemu_mknodat(dirfd, name, credp->fc_mode, credp->fc_rdev);
         if (err == -1) {
             goto out;
         }
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index d1660d67fa..f3a8367ece 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -810,3 +810,13 @@ static inline int
platform_does_not_support_system(const char *command)
 #endif

 #endif
+
+/*
+ * As long as mknodat is not available on macOS, this workaround
+ * using pthread_fchdir_np is needed. qemu_mknodat is defined in
+ * os-posix.c
+ */
+#ifdef CONFIG_DARWIN
+int pthread_fchdir_np(int fd);
+#endif
+int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev);
diff --git a/os-posix.c b/os-posix.c
index ae6c9f2a5e..95c1607065 100644
--- a/os-posix.c
+++ b/os-posix.c
@@ -24,6 +24,7 @@
  */

 #include "qemu/osdep.h"
+#include <os/availability.h>
 #include <sys/wait.h>
 #include <pwd.h>
 #include <grp.h>
@@ -332,3 +333,36 @@ int os_mlock(void)
     return -ENOSYS;
 #endif
 }
+
+/*
+ * As long as mknodat is not available on macOS, this workaround
+ * using pthread_fchdir_np is needed.
+ *
+ * Radar filed with Apple for implementing mknodat:
+ * rdar://FB9862426 (https://openradar.appspot.com/FB9862426)
+ */
+#ifdef CONFIG_DARWIN
+
+int pthread_fchdir_np(int fd) API_AVAILABLE(macosx(10.12));
+
+int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev)
+{
+    int preserved_errno, err;
+    if (pthread_fchdir_np(dirfd) < 0) {
+        return -1;
+    }
+    err = mknod(filename, mode, dev);
+    preserved_errno = errno;
+    /* Stop using the thread-local cwd */
+    pthread_fchdir_np(-1);
+    if (err < 0) {
+        errno = preserved_errno;
+    }
+    return err;
+}
+#else
+int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev)
+{
+    return mknodat(dirfd, filename, mode, dev);
+}
+#endif
-- 
2.34.1

[-- Attachment #2: Type: text/html, Size: 7987 bytes --]

Re: [PATCH v4 09/11] 9p: darwin: Implement compatibility for mknodat

[Will Cohen]

[-- Attachment #1: Type: text/plain, Size: 6952 bytes --]

On Mon, Feb 7, 2022 at 4:07 PM Will Cohen <wwcohen@gmail.com> wrote:

> On Mon, Feb 7, 2022 at 9:21 AM Christian Schoenebeck <
> qemu_oss@crudebyte.com> wrote:
>
>> On Montag, 7. Februar 2022 11:57:25 CET Dr. David Alan Gilbert wrote:
>> > * Greg Kurz (groug@kaod.org) wrote:
>> > > On Mon, 7 Feb 2022 11:30:18 +0100
>> > >
>> > > Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> > > > On 7/2/22 09:47, Greg Kurz wrote:
>> > > > > On Sun, 6 Feb 2022 20:10:23 -0500
>> > > > >
>> > > > > Will Cohen <wwcohen@gmail.com> wrote:
>> > > > >> This patch set currently places it in 9p-util only because 9p is
>> the
>> > > > >> only
>> > > > >> place where this issue seems to have come up so far and we were
>> wary
>> > > > >> of
>> > > > >> editing files too far afield, but I have no attachment to its
>> > > > >> specific
>> > > > >> location!
>> > > > >
>> > > > > Inline comments are preferred on qemu-devel. Please don't top
>> post !
>> > > > > This complicates the review a lot.
>> > > > >
>> > > > > This is indeed a good candidate for osdep. This being said, unless
>> > > > > there's
>> > > > > some other user in the QEMU code base, it is acceptable to leave
>> it
>> > > > > under
>> > > > > 9pfs.
>> > > >
>> > > > virtiofsd could eventually use it.
>> > >
>> > > Indeed but virtiofsd is for linux hosts only AFAICT and I'm not aware
>> of
>> > > any work to support any other host OS.
>> > >
>> > > Cc'ing virtio-fs people for inputs on this topic.
>> >
>> > Indeeed, there's a lot of Linux specific code in the virtiofsd - I know
>> > people are interested in other platforms, but I'm not sure that's the
>> > right starting point.
>> >
>> > Dave
>>
>> Agreeing with Greg here: i.e. I would have placed this into osdep, but I
>> would
>> not insist on it either.
>>
>> Best regards,
>> Christian Schoenebeck
>>
>>
> This makes sense. A revised version of this patch, moving qemu_mknodat
> from 9p-util to osdep and os-posix, is attached below. I'd appreciate any
> feedback from those looped in here, so that the context isn't lost before
> resubmitting as a v5 patch, especially since this is starting to touch
> files outside of 9p.
>
> From c9713c87163da7c96b5357d0d85ac318ae3d3051 Mon Sep 17 00:00:00 2001
> From: Keno Fischer <keno@juliacomputing.com>
> Date: Sat, 16 Jun 2018 20:56:55 -0400
> Subject: [PATCH] 9p: darwin: Implement compatibility for mknodat
>
> Darwin does not support mknodat. However, to avoid race conditions
> with later setting the permissions, we must avoid using mknod on
> the full path instead. We could try to fchdir, but that would cause
> problems if multiple threads try to call mknodat at the same time.
> However, luckily there is a solution: Darwin includes a function
> that sets the cwd for the current thread only.
> This should suffice to use mknod safely.
>
> This function (pthread_fchdir_np) is protected by a check in
> meson in a patch later in tihs series.
>
> Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
> [Will Cohen: - Adjust coding style
>              - Replace clang references with gcc
>              - Note radar filed with Apple for missing syscall
>              - Replace direct syscall with pthread_fchdir_np and
>                adjust patch notes accordingly
>              - Move qemu_mknodat from 9p-util to osdep and os-posix]
> Signed-off-by: Will Cohen <wwcohen@gmail.com>
> ---
>  hw/9pfs/9p-local.c   |  4 ++--
>  include/qemu/osdep.h | 10 ++++++++++
>  os-posix.c           | 34 ++++++++++++++++++++++++++++++++++
>  3 files changed, 46 insertions(+), 2 deletions(-)
>
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index a0d08e5216..d42ce6d8b8 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -682,7 +682,7 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath
> *dir_path,
>
>      if (fs_ctx->export_flags & V9FS_SM_MAPPED ||
>          fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
> -        err = mknodat(dirfd, name, fs_ctx->fmode | S_IFREG, 0);
> +        err = qemu_mknodat(dirfd, name, fs_ctx->fmode | S_IFREG, 0);
>          if (err == -1) {
>              goto out;
>          }
> @@ -697,7 +697,7 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath
> *dir_path,
>          }
>      } else if (fs_ctx->export_flags & V9FS_SM_PASSTHROUGH ||
>                 fs_ctx->export_flags & V9FS_SM_NONE) {
> -        err = mknodat(dirfd, name, credp->fc_mode, credp->fc_rdev);
> +        err = qemu_mknodat(dirfd, name, credp->fc_mode, credp->fc_rdev);
>          if (err == -1) {
>              goto out;
>          }
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index d1660d67fa..f3a8367ece 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -810,3 +810,13 @@ static inline int
> platform_does_not_support_system(const char *command)
>  #endif
>
>  #endif
> +
> +/*
> + * As long as mknodat is not available on macOS, this workaround
> + * using pthread_fchdir_np is needed. qemu_mknodat is defined in
> + * os-posix.c
> + */
> +#ifdef CONFIG_DARWIN
> +int pthread_fchdir_np(int fd);
> +#endif
> +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev);
> diff --git a/os-posix.c b/os-posix.c
> index ae6c9f2a5e..95c1607065 100644
> --- a/os-posix.c
> +++ b/os-posix.c
> @@ -24,6 +24,7 @@
>   */
>
>  #include "qemu/osdep.h"
> +#include <os/availability.h>
>  #include <sys/wait.h>
>  #include <pwd.h>
>  #include <grp.h>
> @@ -332,3 +333,36 @@ int os_mlock(void)
>      return -ENOSYS;
>  #endif
>  }
> +
> +/*
> + * As long as mknodat is not available on macOS, this workaround
> + * using pthread_fchdir_np is needed.
> + *
> + * Radar filed with Apple for implementing mknodat:
> + * rdar://FB9862426 (https://openradar.appspot.com/FB9862426)
> + */
> +#ifdef CONFIG_DARWIN
> +
> +int pthread_fchdir_np(int fd) API_AVAILABLE(macosx(10.12));
> +
> +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev)
> +{
> +    int preserved_errno, err;
> +    if (pthread_fchdir_np(dirfd) < 0) {
> +        return -1;
> +    }
> +    err = mknod(filename, mode, dev);
> +    preserved_errno = errno;
> +    /* Stop using the thread-local cwd */
> +    pthread_fchdir_np(-1);
> +    if (err < 0) {
> +        errno = preserved_errno;
> +    }
> +    return err;
> +}
> +#else
> +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev)
> +{
> +    return mknodat(dirfd, filename, mode, dev);
> +}
> +#endif
> --
> 2.34.1
>

Noting on
https://www.qemu.org/docs/master/devel/submitting-a-patch.html#when-resending-patches-add-a-version-tag
that resending a single patch outside of a new entire patch series is not
helpful, so I'm resubmitting with this patch substituted in as v5 shortly.

[-- Attachment #2: Type: text/html, Size: 8818 bytes --]

Re: [PATCH v4 09/11] 9p: darwin: Implement compatibility for mknodat

[Christian Schoenebeck]

On Montag, 7. Februar 2022 22:07:34 CET Will Cohen wrote:
> On Mon, Feb 7, 2022 at 9:21 AM Christian Schoenebeck
> <qemu_oss@crudebyte.com>
> wrote:
> > On Montag, 7. Februar 2022 11:57:25 CET Dr. David Alan Gilbert wrote:
> > > * Greg Kurz (groug@kaod.org) wrote:
> > > > On Mon, 7 Feb 2022 11:30:18 +0100
> > > > 
> > > > Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> > > > > On 7/2/22 09:47, Greg Kurz wrote:
> > > > > > On Sun, 6 Feb 2022 20:10:23 -0500
> > > > > > 
> > > > > > Will Cohen <wwcohen@gmail.com> wrote:
> > > > > >> This patch set currently places it in 9p-util only because 9p is
> > 
> > the
> > 
> > > > > >> only
> > > > > >> place where this issue seems to have come up so far and we were
> > 
> > wary
> > 
> > > > > >> of
> > > > > >> editing files too far afield, but I have no attachment to its
> > > > > >> specific
> > > > > >> location!
> > > > > > 
> > > > > > Inline comments are preferred on qemu-devel. Please don't top post
> > 
> > !
> > 
> > > > > > This complicates the review a lot.
> > > > > > 
> > > > > > This is indeed a good candidate for osdep. This being said, unless
> > > > > > there's
> > > > > > some other user in the QEMU code base, it is acceptable to leave
> > > > > > it
> > > > > > under
> > > > > > 9pfs.
> > > > > 
> > > > > virtiofsd could eventually use it.
> > > > 
> > > > Indeed but virtiofsd is for linux hosts only AFAICT and I'm not aware
> > 
> > of
> > 
> > > > any work to support any other host OS.
> > > > 
> > > > Cc'ing virtio-fs people for inputs on this topic.
> > > 
> > > Indeeed, there's a lot of Linux specific code in the virtiofsd - I know
> > > people are interested in other platforms, but I'm not sure that's the
> > > right starting point.
> > > 
> > > Dave
> > 
> > Agreeing with Greg here: i.e. I would have placed this into osdep, but I
> > would
> > not insist on it either.
> > 
> > Best regards,
> > Christian Schoenebeck
> 
> This makes sense. A revised version of this patch, moving qemu_mknodat from
> 9p-util to osdep and os-posix, is attached below. I'd appreciate any
> feedback from those looped in here, so that the context isn't lost before
> resubmitting as a v5 patch, especially since this is starting to touch
> files outside of 9p.
> 
> From c9713c87163da7c96b5357d0d85ac318ae3d3051 Mon Sep 17 00:00:00 2001
> From: Keno Fischer <keno@juliacomputing.com>
> Date: Sat, 16 Jun 2018 20:56:55 -0400
> Subject: [PATCH] 9p: darwin: Implement compatibility for mknodat
> 
> Darwin does not support mknodat. However, to avoid race conditions
> with later setting the permissions, we must avoid using mknod on
> the full path instead. We could try to fchdir, but that would cause
> problems if multiple threads try to call mknodat at the same time.
> However, luckily there is a solution: Darwin includes a function
> that sets the cwd for the current thread only.
> This should suffice to use mknod safely.
> 
> This function (pthread_fchdir_np) is protected by a check in
> meson in a patch later in tihs series.
> 
> Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
> [Will Cohen: - Adjust coding style
>              - Replace clang references with gcc
>              - Note radar filed with Apple for missing syscall
>              - Replace direct syscall with pthread_fchdir_np and
>                adjust patch notes accordingly
>              - Move qemu_mknodat from 9p-util to osdep and os-posix]
> Signed-off-by: Will Cohen <wwcohen@gmail.com>
> ---
>  hw/9pfs/9p-local.c   |  4 ++--
>  include/qemu/osdep.h | 10 ++++++++++
>  os-posix.c           | 34 ++++++++++++++++++++++++++++++++++
>  3 files changed, 46 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index a0d08e5216..d42ce6d8b8 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -682,7 +682,7 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath
> *dir_path,
> 
>      if (fs_ctx->export_flags & V9FS_SM_MAPPED ||
>          fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
> -        err = mknodat(dirfd, name, fs_ctx->fmode | S_IFREG, 0);
> +        err = qemu_mknodat(dirfd, name, fs_ctx->fmode | S_IFREG, 0);
>          if (err == -1) {
>              goto out;
>          }
> @@ -697,7 +697,7 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath
> *dir_path,
>          }
>      } else if (fs_ctx->export_flags & V9FS_SM_PASSTHROUGH ||
>                 fs_ctx->export_flags & V9FS_SM_NONE) {
> -        err = mknodat(dirfd, name, credp->fc_mode, credp->fc_rdev);
> +        err = qemu_mknodat(dirfd, name, credp->fc_mode, credp->fc_rdev);
>          if (err == -1) {
>              goto out;
>          }
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index d1660d67fa..f3a8367ece 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -810,3 +810,13 @@ static inline int
> platform_does_not_support_system(const char *command)
>  #endif
> 
>  #endif
> +
> +/*
> + * As long as mknodat is not available on macOS, this workaround
> + * using pthread_fchdir_np is needed. qemu_mknodat is defined in
> + * os-posix.c
> + */
> +#ifdef CONFIG_DARWIN
> +int pthread_fchdir_np(int fd);
> +#endif

I would make that:

#ifdef CONFIG_DARWIN
int pthread_fchdir_np(int fd) API_AVAILABLE(macosx(10.12));
#endif

here and ...

> +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev);
> diff --git a/os-posix.c b/os-posix.c
> index ae6c9f2a5e..95c1607065 100644
> --- a/os-posix.c
> +++ b/os-posix.c
> @@ -24,6 +24,7 @@
>   */
> 
>  #include "qemu/osdep.h"
> +#include <os/availability.h>
>  #include <sys/wait.h>
>  #include <pwd.h>
>  #include <grp.h>
> @@ -332,3 +333,36 @@ int os_mlock(void)
>      return -ENOSYS;
>  #endif
>  }
> +
> +/*
> + * As long as mknodat is not available on macOS, this workaround
> + * using pthread_fchdir_np is needed.
> + *
> + * Radar filed with Apple for implementing mknodat:
> + * rdar://FB9862426 (https://openradar.appspot.com/FB9862426)
> + */
> +#ifdef CONFIG_DARWIN
> +
> +int pthread_fchdir_np(int fd) API_AVAILABLE(macosx(10.12));

... drop the duplicate declaration of pthread_fchdir_np() here.

> +
> +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev)
> +{
> +    int preserved_errno, err;
> +    if (pthread_fchdir_np(dirfd) < 0) {
> +        return -1;
> +    }
> +    err = mknod(filename, mode, dev);
> +    preserved_errno = errno;
> +    /* Stop using the thread-local cwd */
> +    pthread_fchdir_np(-1);
> +    if (err < 0) {
> +        errno = preserved_errno;
> +    }
> +    return err;
> +}
> +#else
> +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev)
> +{
> +    return mknodat(dirfd, filename, mode, dev);
> +}
> +#endif

Re: [PATCH v4 09/11] 9p: darwin: Implement compatibility for mknodat

[Will Cohen]

[-- Attachment #1: Type: text/plain, Size: 7837 bytes --]

On Mon, Feb 7, 2022 at 5:48 PM Christian Schoenebeck <qemu_oss@crudebyte.com>
wrote:

> On Montag, 7. Februar 2022 22:07:34 CET Will Cohen wrote:
> > On Mon, Feb 7, 2022 at 9:21 AM Christian Schoenebeck
> > <qemu_oss@crudebyte.com>
> > wrote:
> > > On Montag, 7. Februar 2022 11:57:25 CET Dr. David Alan Gilbert wrote:
> > > > * Greg Kurz (groug@kaod.org) wrote:
> > > > > On Mon, 7 Feb 2022 11:30:18 +0100
> > > > >
> > > > > Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> > > > > > On 7/2/22 09:47, Greg Kurz wrote:
> > > > > > > On Sun, 6 Feb 2022 20:10:23 -0500
> > > > > > >
> > > > > > > Will Cohen <wwcohen@gmail.com> wrote:
> > > > > > >> This patch set currently places it in 9p-util only because 9p
> is
> > >
> > > the
> > >
> > > > > > >> only
> > > > > > >> place where this issue seems to have come up so far and we
> were
> > >
> > > wary
> > >
> > > > > > >> of
> > > > > > >> editing files too far afield, but I have no attachment to its
> > > > > > >> specific
> > > > > > >> location!
> > > > > > >
> > > > > > > Inline comments are preferred on qemu-devel. Please don't top
> post
> > >
> > > !
> > >
> > > > > > > This complicates the review a lot.
> > > > > > >
> > > > > > > This is indeed a good candidate for osdep. This being said,
> unless
> > > > > > > there's
> > > > > > > some other user in the QEMU code base, it is acceptable to
> leave
> > > > > > > it
> > > > > > > under
> > > > > > > 9pfs.
> > > > > >
> > > > > > virtiofsd could eventually use it.
> > > > >
> > > > > Indeed but virtiofsd is for linux hosts only AFAICT and I'm not
> aware
> > >
> > > of
> > >
> > > > > any work to support any other host OS.
> > > > >
> > > > > Cc'ing virtio-fs people for inputs on this topic.
> > > >
> > > > Indeeed, there's a lot of Linux specific code in the virtiofsd - I
> know
> > > > people are interested in other platforms, but I'm not sure that's the
> > > > right starting point.
> > > >
> > > > Dave
> > >
> > > Agreeing with Greg here: i.e. I would have placed this into osdep, but
> I
> > > would
> > > not insist on it either.
> > >
> > > Best regards,
> > > Christian Schoenebeck
> >
> > This makes sense. A revised version of this patch, moving qemu_mknodat
> from
> > 9p-util to osdep and os-posix, is attached below. I'd appreciate any
> > feedback from those looped in here, so that the context isn't lost before
> > resubmitting as a v5 patch, especially since this is starting to touch
> > files outside of 9p.
> >
> > From c9713c87163da7c96b5357d0d85ac318ae3d3051 Mon Sep 17 00:00:00 2001
> > From: Keno Fischer <keno@juliacomputing.com>
> > Date: Sat, 16 Jun 2018 20:56:55 -0400
> > Subject: [PATCH] 9p: darwin: Implement compatibility for mknodat
> >
> > Darwin does not support mknodat. However, to avoid race conditions
> > with later setting the permissions, we must avoid using mknod on
> > the full path instead. We could try to fchdir, but that would cause
> > problems if multiple threads try to call mknodat at the same time.
> > However, luckily there is a solution: Darwin includes a function
> > that sets the cwd for the current thread only.
> > This should suffice to use mknod safely.
> >
> > This function (pthread_fchdir_np) is protected by a check in
> > meson in a patch later in tihs series.
> >
> > Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> > Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
> > [Will Cohen: - Adjust coding style
> >              - Replace clang references with gcc
> >              - Note radar filed with Apple for missing syscall
> >              - Replace direct syscall with pthread_fchdir_np and
> >                adjust patch notes accordingly
> >              - Move qemu_mknodat from 9p-util to osdep and os-posix]
> > Signed-off-by: Will Cohen <wwcohen@gmail.com>
> > ---
> >  hw/9pfs/9p-local.c   |  4 ++--
> >  include/qemu/osdep.h | 10 ++++++++++
> >  os-posix.c           | 34 ++++++++++++++++++++++++++++++++++
> >  3 files changed, 46 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> > index a0d08e5216..d42ce6d8b8 100644
> > --- a/hw/9pfs/9p-local.c
> > +++ b/hw/9pfs/9p-local.c
> > @@ -682,7 +682,7 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath
> > *dir_path,
> >
> >      if (fs_ctx->export_flags & V9FS_SM_MAPPED ||
> >          fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
> > -        err = mknodat(dirfd, name, fs_ctx->fmode | S_IFREG, 0);
> > +        err = qemu_mknodat(dirfd, name, fs_ctx->fmode | S_IFREG, 0);
> >          if (err == -1) {
> >              goto out;
> >          }
> > @@ -697,7 +697,7 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath
> > *dir_path,
> >          }
> >      } else if (fs_ctx->export_flags & V9FS_SM_PASSTHROUGH ||
> >                 fs_ctx->export_flags & V9FS_SM_NONE) {
> > -        err = mknodat(dirfd, name, credp->fc_mode, credp->fc_rdev);
> > +        err = qemu_mknodat(dirfd, name, credp->fc_mode, credp->fc_rdev);
> >          if (err == -1) {
> >              goto out;
> >          }
> > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > index d1660d67fa..f3a8367ece 100644
> > --- a/include/qemu/osdep.h
> > +++ b/include/qemu/osdep.h
> > @@ -810,3 +810,13 @@ static inline int
> > platform_does_not_support_system(const char *command)
> >  #endif
> >
> >  #endif
> > +
> > +/*
> > + * As long as mknodat is not available on macOS, this workaround
> > + * using pthread_fchdir_np is needed. qemu_mknodat is defined in
> > + * os-posix.c
> > + */
> > +#ifdef CONFIG_DARWIN
> > +int pthread_fchdir_np(int fd);
> > +#endif
>
> I would make that:
>
> #ifdef CONFIG_DARWIN
> int pthread_fchdir_np(int fd) API_AVAILABLE(macosx(10.12));
> #endif
>
> here and ...
>
> > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t
> dev);
> > diff --git a/os-posix.c b/os-posix.c
> > index ae6c9f2a5e..95c1607065 100644
> > --- a/os-posix.c
> > +++ b/os-posix.c
> > @@ -24,6 +24,7 @@
> >   */
> >
> >  #include "qemu/osdep.h"
> > +#include <os/availability.h>
> >  #include <sys/wait.h>
> >  #include <pwd.h>
> >  #include <grp.h>
> > @@ -332,3 +333,36 @@ int os_mlock(void)
> >      return -ENOSYS;
> >  #endif
> >  }
> > +
> > +/*
> > + * As long as mknodat is not available on macOS, this workaround
> > + * using pthread_fchdir_np is needed.
> > + *
> > + * Radar filed with Apple for implementing mknodat:
> > + * rdar://FB9862426 (https://openradar.appspot.com/FB9862426)
> > + */
> > +#ifdef CONFIG_DARWIN
> > +
> > +int pthread_fchdir_np(int fd) API_AVAILABLE(macosx(10.12));
>
> ... drop the duplicate declaration of pthread_fchdir_np() here.
>
> > +
> > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t
> dev)
> > +{
> > +    int preserved_errno, err;
> > +    if (pthread_fchdir_np(dirfd) < 0) {
> > +        return -1;
> > +    }
> > +    err = mknod(filename, mode, dev);
> > +    preserved_errno = errno;
> > +    /* Stop using the thread-local cwd */
> > +    pthread_fchdir_np(-1);
> > +    if (err < 0) {
> > +        errno = preserved_errno;
> > +    }
> > +    return err;
> > +}
> > +#else
> > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t
> dev)
> > +{
> > +    return mknodat(dirfd, filename, mode, dev);
> > +}
> > +#endif
>

Noted and all of that makes sense to me. Sorry about trying to lump this in
v4. If more things come up with v5 I'll stick these changes in for v6 as
well. If there's no other major comments with v5, I can either still
resubmit as v6 or defer to whatever process is easiest for the maintainers
to integrate, given that I accept all of these modifications.

[-- Attachment #2: Type: text/html, Size: 10515 bytes --]