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

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

[Will Cohen]

This is a followup to https://lists.gnu.org/archive/html/qemu-devel/2021-10/msg02902.html,
adding 9p server support for Darwin.

Since v1, the following changes have been made:

Submission and formatting
- Submission via git-publish
- Signed-off-by headers now reflect modifications since original submission in 2018
- Previous reviews have been removed, retaining only the newest https://lists.gnu.org/archive/html/qemu-devel/2021-10/msg03426.html

Testing
- Rebased to apply to latest master
- Updated to pass Gitlab CI pipeline

Keno Fischer (11):
  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: Compatibility defn for XATTR_SIZE_MAX
  9p: darwin: *xattr_nofollow implementations
  9p: darwin: Compatibility for f/l*xattr
  9p: darwin: Provide fallback impl for utimensat
  9p: darwin: Implement compatibility for mknodat
  9p: darwin: meson: Allow VirtFS on Darwin

 fsdev/file-op-9p.h                     |   2 +-
 fsdev/meson.build                      |   1 +
 hw/9pfs/9p-local.c                     |  21 ++-
 hw/9pfs/9p-proxy.c                     |  17 ++-
 hw/9pfs/9p-synth.c                     |   4 +
 hw/9pfs/9p-util-darwin.c               | 193 +++++++++++++++++++++++++
 hw/9pfs/{9p-util.c => 9p-util-linux.c} |  13 +-
 hw/9pfs/9p-util.h                      |  29 ++++
 hw/9pfs/9p.c                           |  75 +++++++++-
 hw/9pfs/codir.c                        |   4 +
 hw/9pfs/meson.build                    |   3 +-
 include/qemu/statfs.h                  |  19 +++
 include/qemu/xattr.h                   |   4 +-
 meson.build                            |  12 +-
 14 files changed, 374 insertions(+), 23 deletions(-)
 create mode 100644 hw/9pfs/9p-util-darwin.c
 rename hw/9pfs/{9p-util.c => 9p-util-linux.c} (82%)
 create mode 100644 include/qemu/statfs.h

-- 
2.34.0

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

[Will Cohen]

From: Keno Fischer <keno@alumni.harvard.edu>

 - 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>
[Will Cohen: - Fix headers for Alpine]
Signed-off-by: Will Cohen <wwcohen@gmail.com>
---
 fsdev/file-op-9p.h    |  2 +-
 hw/9pfs/9p-local.c    |  2 ++
 hw/9pfs/9p.c          |  4 ++++
 include/qemu/statfs.h | 19 +++++++++++++++++++
 include/qemu/xattr.h  |  4 +++-
 5 files changed, 29 insertions(+), 2 deletions(-)
 create mode 100644 include/qemu/statfs.h

diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
index 8fd89f0447..16c1a9d9fe 100644
--- a/fsdev/file-op-9p.h
+++ b/fsdev/file-op-9p.h
@@ -16,7 +16,7 @@
 
 #include <dirent.h>
 #include <utime.h>
-#include <sys/vfs.h>
+#include "qemu/statfs.h"
 #include "qemu-fsdev-throttle.h"
 #include "p9array.h"
 
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/statfs.h b/include/qemu/statfs.h
new file mode 100644
index 0000000000..dde289f9bb
--- /dev/null
+++ b/include/qemu/statfs.h
@@ -0,0 +1,19 @@
+/*
+ * Host statfs header abstraction
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2, or any
+ * later version.  See the COPYING file in the top-level directory.
+ *
+ */
+#ifndef QEMU_STATFS_H
+#define QEMU_STATFS_H
+
+#ifdef CONFIG_LINUX
+# include <sys/vfs.h>
+#endif
+#ifdef CONFIG_DARWIN
+# include <sys/param.h>
+# include <sys/mount.h>
+#endif
+
+#endif
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.34.0

[PATCH v2 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>
---
 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.34.0

[PATCH v2 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>
Signed-off-by: Will Cohen <wwcohen@gmail.com>
---
 hw/9pfs/9p-proxy.c | 17 ++++++++++++++---
 hw/9pfs/9p-synth.c |  2 ++
 hw/9pfs/9p.c       | 16 ++++++++++++++--
 3 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c
index 09bd9f1464..be1546c1be 100644
--- a/hw/9pfs/9p-proxy.c
+++ b/hw/9pfs/9p-proxy.c
@@ -123,10 +123,15 @@ 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
+    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 +148,18 @@ 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_atim.tv_sec = prstat->st_atim_sec;
-   stbuf->st_atim.tv_nsec = prstat->st_atim_nsec;
+   stbuf->st_atime = prstat->st_atim_sec;
    stbuf->st_mtime = prstat->st_mtim_sec;
-   stbuf->st_mtim.tv_nsec = prstat->st_mtim_nsec;
    stbuf->st_ctime = prstat->st_ctim_sec;
+#ifdef CONFIG_DARWIN
+   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_nsec = prstat->st_atim_nsec;
+   stbuf->st_mtim.tv_nsec = prstat->st_mtim_nsec;
    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..f4f0c200c7 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 = MAXNAMLEN;
+#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.34.0

[PATCH v2 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.

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-synth.c |  2 ++
 hw/9pfs/9p.c       | 33 +++++++++++++++++++++++++++++++--
 hw/9pfs/codir.c    |  4 ++++
 3 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
index 4a4a776d06..09b9c25288 100644
--- a/hw/9pfs/9p-synth.c
+++ b/hw/9pfs/9p-synth.c
@@ -222,7 +222,9 @@ static void synth_direntry(V9fsSynthNode *node,
 {
     strcpy(entry->d_name, node->name);
     entry->d_ino = node->attr->inode;
+#ifndef CONFIG_DARWIN
     entry->d_off = off + 1;
+#endif
 }
 
 static struct dirent *synth_get_dentry(V9fsSynthNode *dir,
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index f4f0c200c7..c06e8a85a0 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -2218,6 +2218,25 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp,
     return offset;
 }
 
+/**
+ * Get the seek offset of a dirent. If not available from the structure itself,
+ * obtain it by calling telldir.
+ */
+static int v9fs_dent_telldir(V9fsPDU *pdu, V9fsFidState *fidp,
+                             struct dirent *dent)
+{
+#ifdef CONFIG_DARWIN
+    /*
+     * Darwin has d_seekoff, which appears to function similarly to d_off.
+     * However, it does not appear to be supported on all file systems,
+     * so use telldir for correctness.
+     */
+    return telldir(fidp->fs.dir.stream);
+#else
+    return dent->d_off;
+#endif
+}
+
 static int coroutine_fn v9fs_do_readdir_with_stat(V9fsPDU *pdu,
                                                   V9fsFidState *fidp,
                                                   uint32_t max_count)
@@ -2281,7 +2300,11 @@ static int coroutine_fn v9fs_do_readdir_with_stat(V9fsPDU *pdu,
         count += len;
         v9fs_stat_free(&v9stat);
         v9fs_path_free(&path);
-        saved_dir_pos = dent->d_off;
+        saved_dir_pos = v9fs_dent_telldir(pdu, fidp, dent);
+        if (saved_dir_pos < 0) {
+            err = saved_dir_pos;
+            break;
+        }
     }
 
     v9fs_readdir_unlock(&fidp->fs.dir);
@@ -2420,6 +2443,7 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu, V9fsFidState *fidp,
     V9fsString name;
     int len, err = 0;
     int32_t count = 0;
+    off_t off;
     struct dirent *dent;
     struct stat *st;
     struct V9fsDirEnt *entries = NULL;
@@ -2480,12 +2504,17 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu, V9fsFidState *fidp,
             qid.version = 0;
         }
 
+        off = v9fs_dent_telldir(pdu, fidp, dent);
+        if (off < 0) {
+            err = off;
+            break;
+        }
         v9fs_string_init(&name);
         v9fs_string_sprintf(&name, "%s", dent->d_name);
 
         /* 11 = 7 + 4 (7 = start offset, 4 = space for storing count) */
         len = pdu_marshal(pdu, 11 + count, "Qqbs",
-                          &qid, dent->d_off,
+                          &qid, off,
                           dent->d_type, &name);
 
         v9fs_string_free(&name);
diff --git a/hw/9pfs/codir.c b/hw/9pfs/codir.c
index 032cce04c4..78aca1d98b 100644
--- a/hw/9pfs/codir.c
+++ b/hw/9pfs/codir.c
@@ -167,7 +167,11 @@ static int do_readdir_many(V9fsPDU *pdu, V9fsFidState *fidp,
         }
 
         size += len;
+#ifdef CONFIG_DARWIN
+        saved_dir_pos = telldir(fidp->fs.dir.stream);
+#else
         saved_dir_pos = dent->d_off;
+#endif
     }
 
     /* restore (last) saved position */
-- 
2.34.0

[PATCH v2 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 546f46dc7d..627baebaba 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 c06e8a85a0..c941b46f60 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -137,11 +137,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 },
     };
 
@@ -170,10 +179,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.34.0

[PATCH v2 06/11] 9p: darwin: Compatibility defn for 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>
[Will Cohen: - Adjust coding style]
Signed-off-by: Will Cohen <wwcohen@gmail.com>
---
 hw/9pfs/9p.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index c941b46f60..d671995aa4 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -3943,6 +3943,14 @@ out_nofid:
     v9fs_string_free(&name);
 }
 
+#if defined(CONFIG_DARWIN) && !defined(XATTR_SIZE_MAX)
+/*
+ * Darwin doesn't seem to define a maximum xattr size in its user
+ * space header, but looking at the kernel source, HFS supports
+ *  up to INT32_MAX, so use that as the maximum.
+ */
+#define XATTR_SIZE_MAX INT32_MAX
+#endif
 static void coroutine_fn v9fs_xattrcreate(void *opaque)
 {
     int flags, rflags = 0;
-- 
2.34.0

[PATCH v2 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.34.0

[PATCH v2 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 1a5e3eed73..2bfff79b12 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -781,16 +781,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 627baebaba..38ef8b289d 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.34.0

[PATCH v2 09/11] 9p: darwin: Provide fallback impl for utimensat

[Will Cohen]

From: Keno Fischer <keno@juliacomputing.com>

This function is new in Mac OS 10.13. Provide a fallback implementation
when building against older SDKs. The complication in the definition comes
having to separately handle the used SDK version and the target OS version.

- If the SDK version is too low (__MAC_10_13 not defined), utimensat is not
  defined in the header, so we must not try to use it (doing so would error).
- Otherwise, if the targetted OS version is at least 10.13, we know this
  function is available, so we can unconditionally call it.
- Lastly, we check for the availability of the __builtin_available macro to
  potentially insert a dynamic check for this OS version. However, __builtin_available
  is only available with sufficiently recent versions of clang and while all
  Apple clang versions that ship with Xcode versions that support the 10.13
  SDK support with builtin, we want to allow building with compilers other
  than Apple clang that may not support this builtin.

Signed-off-by: Keno Fischer <keno@juliacomputing.com>
Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
Signed-off-by: Will Cohen <wwcohen@gmail.com>
---
 hw/9pfs/9p-local.c       |  2 +-
 hw/9pfs/9p-util-darwin.c | 96 ++++++++++++++++++++++++++++++++++++++++
 hw/9pfs/9p-util-linux.c  |  6 +++
 hw/9pfs/9p-util.h        |  8 ++++
 hw/9pfs/9p.c             |  1 +
 5 files changed, 112 insertions(+), 1 deletion(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 2bfff79b12..4268703d05 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1076,7 +1076,7 @@ static int local_utimensat(FsContext *s, V9fsPath *fs_path,
         goto out;
     }
 
-    ret = utimensat(dirfd, name, buf, AT_SYMLINK_NOFOLLOW);
+    ret = utimensat_nofollow(dirfd, name, buf);
     close_preserve_errno(dirfd);
 out:
     g_free(dirpath);
diff --git a/hw/9pfs/9p-util-darwin.c b/hw/9pfs/9p-util-darwin.c
index cdb4c9e24c..ac414bcbfd 100644
--- a/hw/9pfs/9p-util-darwin.c
+++ b/hw/9pfs/9p-util-darwin.c
@@ -62,3 +62,99 @@ int fsetxattrat_nofollow(int dirfd, const char *filename, const char *name,
     close_preserve_errno(fd);
     return ret;
 }
+
+#ifndef __has_builtin
+#define __has_builtin(x) 0
+#endif
+
+static int update_times_from_stat(int fd, struct timespec times[2],
+                                  int update0, int update1)
+{
+    struct stat buf;
+    int ret = fstat(fd, &buf);
+    if (ret == -1) {
+        return ret;
+    }
+    if (update0) {
+        times[0] = buf.st_atimespec;
+    }
+    if (update1) {
+        times[1] = buf.st_mtimespec;
+    }
+    return 0;
+}
+
+int utimensat_nofollow(int dirfd, const char *filename,
+                       const struct timespec times_in[2])
+{
+    int ret, fd;
+    int special0, special1;
+    struct timeval futimes_buf[2];
+    struct timespec times[2];
+    memcpy(times, times_in, 2 * sizeof(struct timespec));
+
+/* Check whether we have an SDK version that defines utimensat */
+#if defined(__MAC_10_13)
+# if __MAC_OS_X_VERSION_MIN_REQUIRED >= __MAC_10_13
+#  define UTIMENSAT_AVAILABLE 1
+# elif __has_builtin(__builtin_available)
+#  define UTIMENSAT_AVAILABLE __builtin_available(macos 10.13, *)
+# else
+#  define UTIMENSAT_AVAILABLE 0
+# endif
+    if (UTIMENSAT_AVAILABLE) {
+        return utimensat(dirfd, filename, times, AT_SYMLINK_NOFOLLOW);
+    }
+#endif
+
+    /* utimensat not available. Use futimes. */
+    fd = openat_file(dirfd, filename, O_PATH_9P_UTIL | O_NOFOLLOW, 0);
+    if (fd == -1) {
+        return -1;
+    }
+
+    special0 = times[0].tv_nsec == UTIME_OMIT;
+    special1 = times[1].tv_nsec == UTIME_OMIT;
+    if (special0 || special1) {
+        /* If both are set, nothing to do */
+        if (special0 && special1) {
+            ret = 0;
+            goto done;
+        }
+
+        ret = update_times_from_stat(fd, times, special0, special1);
+        if (ret < 0) {
+            goto done;
+        }
+    }
+
+    special0 = times[0].tv_nsec == UTIME_NOW;
+    special1 = times[1].tv_nsec == UTIME_NOW;
+    if (special0 || special1) {
+        ret = futimes(fd, NULL);
+        if (ret < 0) {
+            goto done;
+        }
+
+        /* If both are set, we are done */
+        if (special0 && special1) {
+            ret = 0;
+            goto done;
+        }
+
+        ret = update_times_from_stat(fd, times, special0, special1);
+        if (ret < 0) {
+            goto done;
+        }
+    }
+
+    futimes_buf[0].tv_sec = times[0].tv_sec;
+    futimes_buf[0].tv_usec = times[0].tv_nsec / 1000;
+    futimes_buf[1].tv_sec = times[1].tv_sec;
+    futimes_buf[1].tv_usec = times[1].tv_nsec / 1000;
+    ret = futimes(fd, futimes_buf);
+
+done:
+    close_preserve_errno(fd);
+    return ret;
+}
diff --git a/hw/9pfs/9p-util-linux.c b/hw/9pfs/9p-util-linux.c
index 398614a5d0..d54bf57a59 100644
--- a/hw/9pfs/9p-util-linux.c
+++ b/hw/9pfs/9p-util-linux.c
@@ -62,3 +62,9 @@ int fsetxattrat_nofollow(int dirfd, const char *filename, const char *name,
     g_free(proc_path);
     return ret;
 }
+
+int utimensat_nofollow(int dirfd, const char *filename,
+                       const struct timespec times[2])
+{
+    return utimensat(dirfd, filename, times, AT_SYMLINK_NOFOLLOW);
+}
diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h
index 38ef8b289d..1c477a0e66 100644
--- a/hw/9pfs/9p-util.h
+++ b/hw/9pfs/9p-util.h
@@ -36,6 +36,12 @@ static inline int qemu_lsetxattr(const char *path, const char *name,
 #define qemu_lsetxattr lsetxattr
 #endif
 
+/* Compatibility with old SDK Versions for Darwin */
+#if defined(CONFIG_DARWIN) && !defined(UTIME_NOW)
+#define UTIME_NOW -1
+#define UTIME_OMIT -2
+#endif
+
 static inline void close_preserve_errno(int fd)
 {
     int serrno = errno;
@@ -96,5 +102,7 @@ ssize_t flistxattrat_nofollow(int dirfd, const char *filename,
                               char *list, size_t size);
 ssize_t fremovexattrat_nofollow(int dirfd, const char *filename,
                                 const char *name);
+int utimensat_nofollow(int dirfd, const char *filename,
+                       const struct timespec times[2]);
 
 #endif
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index d671995aa4..3d676405c7 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"
-- 
2.34.0

[PATCH v2 10/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 as an (unexposed in the
C library) system call that sets the cwd for the current thread only.
This should suffice to use mknod safely.

Signed-off-by: Keno Fischer <keno@juliacomputing.com>
Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
[Will Cohen: - Adjust coding style]
Signed-off-by: Will Cohen <wwcohen@gmail.com>
---
 hw/9pfs/9p-local.c       |  5 +++--
 hw/9pfs/9p-util-darwin.c | 33 +++++++++++++++++++++++++++++++++
 hw/9pfs/9p-util-linux.c  |  5 +++++
 hw/9pfs/9p-util.h        |  2 ++
 4 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 4268703d05..42b65e143b 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -673,7 +673,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;
         }
@@ -688,7 +688,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;
         }
@@ -701,6 +701,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 ac414bcbfd..25e67d5067 100644
--- a/hw/9pfs/9p-util-darwin.c
+++ b/hw/9pfs/9p-util-darwin.c
@@ -158,3 +158,36 @@ done:
     close_preserve_errno(fd);
     return ret;
 }
+
+#ifndef SYS___pthread_fchdir
+# define SYS___pthread_fchdir 349
+#endif
+
+/*
+ * This is an undocumented OS X syscall. It would be best to avoid it,
+ * but there doesn't seem to be another safe way to implement mknodat.
+ * Dear Apple, please implement mknodat before you remove this syscall.
+ */
+static int fchdir_thread_local(int fd)
+{
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wdeprecated-declarations"
+    return syscall(SYS___pthread_fchdir, fd);
+#pragma clang diagnostic pop
+}
+
+int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev)
+{
+    int preserved_errno, err;
+    if (fchdir_thread_local(dirfd) < 0) {
+        return -1;
+    }
+    err = mknod(filename, mode, dev);
+    preserved_errno = errno;
+    /* Stop using the thread-local cwd */
+    fchdir_thread_local(-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 d54bf57a59..4f57d8c047 100644
--- a/hw/9pfs/9p-util-linux.c
+++ b/hw/9pfs/9p-util-linux.c
@@ -68,3 +68,8 @@ int utimensat_nofollow(int dirfd, const char *filename,
 {
     return utimensat(dirfd, filename, times, AT_SYMLINK_NOFOLLOW);
 }
+
+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 1c477a0e66..cac682d335 100644
--- a/hw/9pfs/9p-util.h
+++ b/hw/9pfs/9p-util.h
@@ -105,4 +105,6 @@ ssize_t fremovexattrat_nofollow(int dirfd, const char *filename,
 int utimensat_nofollow(int dirfd, const char *filename,
                        const struct timespec times[2]);
 
+int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev);
+
 #endif
-- 
2.34.0

[PATCH v2 11/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>
---
 fsdev/meson.build |  1 +
 meson.build       | 12 ++++++++----
 2 files changed, 9 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 96de1a6ef9..0f92adea52 100644
--- a/meson.build
+++ b/meson.build
@@ -1383,17 +1383,21 @@ endif
 have_host_block_device = (targetos != 'darwin' or
     cc.has_header('IOKit/storage/IOMedia.h'))
 
-have_virtfs = (targetos == 'linux' and
+if targetos == 'darwin'
+  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 not libcap_ng.found() or not libattr.found()
       error('virtio-9p (virtfs) requires libcap-ng-devel and libattr-devel')
     elif not have_system
-- 
2.34.0

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

[Christian Schoenebeck]

On Montag, 22. November 2021 01:49:03 CET Will Cohen wrote:
> From: Keno Fischer <keno@alumni.harvard.edu>
> 
>  - 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>
> [Will Cohen: - Fix headers for Alpine]
> Signed-off-by: Will Cohen <wwcohen@gmail.com>
> ---
>  fsdev/file-op-9p.h    |  2 +-
>  hw/9pfs/9p-local.c    |  2 ++
>  hw/9pfs/9p.c          |  4 ++++
>  include/qemu/statfs.h | 19 +++++++++++++++++++
>  include/qemu/xattr.h  |  4 +++-
>  5 files changed, 29 insertions(+), 2 deletions(-)
>  create mode 100644 include/qemu/statfs.h
> 
> diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
> index 8fd89f0447..16c1a9d9fe 100644
> --- a/fsdev/file-op-9p.h
> +++ b/fsdev/file-op-9p.h
> @@ -16,7 +16,7 @@
> 
>  #include <dirent.h>
>  #include <utime.h>
> -#include <sys/vfs.h>
> +#include "qemu/statfs.h"
>  #include "qemu-fsdev-throttle.h"
>  #include "p9array.h"
> 
> 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/statfs.h b/include/qemu/statfs.h
> new file mode 100644
> index 0000000000..dde289f9bb
> --- /dev/null
> +++ b/include/qemu/statfs.h
> @@ -0,0 +1,19 @@
> +/*
> + * Host statfs header abstraction
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2, or any
> + * later version.  See the COPYING file in the top-level directory.
> + *
> + */
> +#ifndef QEMU_STATFS_H
> +#define QEMU_STATFS_H
> +
> +#ifdef CONFIG_LINUX
> +# include <sys/vfs.h>
> +#endif
> +#ifdef CONFIG_DARWIN
> +# include <sys/param.h>
> +# include <sys/mount.h>
> +#endif
> +
> +#endif

I wonder whether adding a separate header file statfs.h just for this isn't 
overkill, as this is only included once. OTOH there is already xattr.h in 
QEMU:

> 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 v2 03/11] 9p: darwin: Handle struct stat(fs) differences

[Christian Schoenebeck]

On Montag, 22. November 2021 01:49:05 CET Will Cohen wrote:
> From: Keno Fischer <keno@juliacomputing.com>
> 
> Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
> Signed-off-by: Will Cohen <wwcohen@gmail.com>
> ---
>  hw/9pfs/9p-proxy.c | 17 ++++++++++++++---
>  hw/9pfs/9p-synth.c |  2 ++
>  hw/9pfs/9p.c       | 16 ++++++++++++++--
>  3 files changed, 30 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c
> index 09bd9f1464..be1546c1be 100644
> --- a/hw/9pfs/9p-proxy.c
> +++ b/hw/9pfs/9p-proxy.c
> @@ -123,10 +123,15 @@ 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
> +    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
>  }

Please assign some value to f_namelen. You could either use the BSD version 
MAXNAMLEN from dirent.h (which you actually use for 9p.c below) or NAME_MAX 
from sys/syslimits.h on macOS.

>  /* Converts proxy_stat structure to VFS stat structure */
> @@ -143,12 +148,18 @@ 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_atim.tv_sec = prstat->st_atim_sec;

Where did that go to? ^-

> -   stbuf->st_atim.tv_nsec = prstat->st_atim_nsec;
> +   stbuf->st_atime = prstat->st_atim_sec;
>     stbuf->st_mtime = prstat->st_mtim_sec;
> -   stbuf->st_mtim.tv_nsec = prstat->st_mtim_nsec;
>     stbuf->st_ctime = prstat->st_ctim_sec;
> +#ifdef CONFIG_DARWIN
> +   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_nsec = prstat->st_atim_nsec;
> +   stbuf->st_mtim.tv_nsec = prstat->st_mtim_nsec;
>     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;
>  }

As mentioned above.

> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 9c63e14b28..f4f0c200c7 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 = MAXNAMLEN;
> +#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,

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

[Christian Schoenebeck]

On Montag, 22. November 2021 01:49:06 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.

Are you sure d_seekoff doesn't work on macOS? Because using telldir() instead
is not the same thing. Accessing d_*off is just POD access, whereas telldir()
is a syscall. What you are trying in this patch with telldir() easily gets
hairy.

AFAIK there was d_off in previous versions of macOS, which was then replaced
by d_seekof in macOS 11.1, no?

> 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-synth.c |  2 ++
>  hw/9pfs/9p.c       | 33 +++++++++++++++++++++++++++++++--
>  hw/9pfs/codir.c    |  4 ++++
>  3 files changed, 37 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
> index 4a4a776d06..09b9c25288 100644
> --- a/hw/9pfs/9p-synth.c
> +++ b/hw/9pfs/9p-synth.c
> @@ -222,7 +222,9 @@ static void synth_direntry(V9fsSynthNode *node,
>  {
>      strcpy(entry->d_name, node->name);
>      entry->d_ino = node->attr->inode;
> +#ifndef CONFIG_DARWIN
>      entry->d_off = off + 1;
> +#endif
>  }

^ That doesn't look like it would work. Compiling sure.

Have you tried running the test cases?
https://wiki.qemu.org/Documentation/9p#Test_Cases

>  static struct dirent *synth_get_dentry(V9fsSynthNode *dir,
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index f4f0c200c7..c06e8a85a0 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -2218,6 +2218,25 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU
> *pdu, V9fsFidState *fidp, return offset;
>  }
> 
> +/**
> + * Get the seek offset of a dirent. If not available from the structure
> itself, + * obtain it by calling telldir.
> + */
> +static int v9fs_dent_telldir(V9fsPDU *pdu, V9fsFidState *fidp,
> +                             struct dirent *dent)
> +{
> +#ifdef CONFIG_DARWIN
> +    /*
> +     * Darwin has d_seekoff, which appears to function similarly to d_off.
> +     * However, it does not appear to be supported on all file systems,
> +     * so use telldir for correctness.
> +     */
> +    return telldir(fidp->fs.dir.stream);
> +#else
> +    return dent->d_off;
> +#endif

The thing here is, we usually run fs syscalls as coroutines on a worker thread
as they might block for a long time, and in the meantime 9p server's main
thread could handle other tasks. Plus if a fs syscall gets stuck, we can abort
the request, which is not possible if its called directly from main thread.

https://wiki.qemu.org/Documentation/9p#Threads_and_Coroutines

dent->d_off is just POD access, so it is instantanious. But that does not mean
you should wrap that telldir() call now to be a background task, because that
will add other implications. I would rather prefer to clarify first whether
d_*off is really not working on macOS to avoid all the foreseeable trouble.

> +}
> +
>  static int coroutine_fn v9fs_do_readdir_with_stat(V9fsPDU *pdu,
>                                                    V9fsFidState *fidp,
>                                                    uint32_t max_count)
> @@ -2281,7 +2300,11 @@ static int coroutine_fn
> v9fs_do_readdir_with_stat(V9fsPDU *pdu, count += len;
>          v9fs_stat_free(&v9stat);
>          v9fs_path_free(&path);
> -        saved_dir_pos = dent->d_off;
> +        saved_dir_pos = v9fs_dent_telldir(pdu, fidp, dent);
> +        if (saved_dir_pos < 0) {
> +            err = saved_dir_pos;
> +            break;
> +        }
>      }
> 
>      v9fs_readdir_unlock(&fidp->fs.dir);
> @@ -2420,6 +2443,7 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu,
> V9fsFidState *fidp, V9fsString name;
>      int len, err = 0;
>      int32_t count = 0;
> +    off_t off;
>      struct dirent *dent;
>      struct stat *st;
>      struct V9fsDirEnt *entries = NULL;
> @@ -2480,12 +2504,17 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU
> *pdu, V9fsFidState *fidp, qid.version = 0;
>          }
> 
> +        off = v9fs_dent_telldir(pdu, fidp, dent);
> +        if (off < 0) {
> +            err = off;
> +            break;
> +        }
>          v9fs_string_init(&name);
>          v9fs_string_sprintf(&name, "%s", dent->d_name);
> 
>          /* 11 = 7 + 4 (7 = start offset, 4 = space for storing count) */
>          len = pdu_marshal(pdu, 11 + count, "Qqbs",
> -                          &qid, dent->d_off,
> +                          &qid, off,
>                            dent->d_type, &name);
> 
>          v9fs_string_free(&name);
> diff --git a/hw/9pfs/codir.c b/hw/9pfs/codir.c
> index 032cce04c4..78aca1d98b 100644
> --- a/hw/9pfs/codir.c
> +++ b/hw/9pfs/codir.c
> @@ -167,7 +167,11 @@ static int do_readdir_many(V9fsPDU *pdu, V9fsFidState
> *fidp, }
> 
>          size += len;
> +#ifdef CONFIG_DARWIN
> +        saved_dir_pos = telldir(fidp->fs.dir.stream);
> +#else
>          saved_dir_pos = dent->d_off;
> +#endif
>      }
> 
>      /* restore (last) saved position */

Re: [PATCH v2 06/11] 9p: darwin: Compatibility defn for XATTR_SIZE_MAX

[Christian Schoenebeck]

On Montag, 22. November 2021 01:49:08 CET Will Cohen wrote:
> From: Keno Fischer <keno@juliacomputing.com>
> 
> Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
> [Will Cohen: - Adjust coding style]
> Signed-off-by: Will Cohen <wwcohen@gmail.com>
> ---
>  hw/9pfs/9p.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index c941b46f60..d671995aa4 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -3943,6 +3943,14 @@ out_nofid:
>      v9fs_string_free(&name);
>  }
> 
> +#if defined(CONFIG_DARWIN) && !defined(XATTR_SIZE_MAX)
> +/*
> + * Darwin doesn't seem to define a maximum xattr size in its user
> + * space header, but looking at the kernel source, HFS supports
> + *  up to INT32_MAX, so use that as the maximum.
> + */
> +#define XATTR_SIZE_MAX INT32_MAX
> +#endif
>  static void coroutine_fn v9fs_xattrcreate(void *opaque)
>  {
>      int flags, rflags = 0;

That would be 2 GB.

v9fs_xattrcreate() [9p.c] calls g_malloc0(size), if that fails it would
terminate QEMU.

I think it would make sense to simply set this to a reasonable small value for
macOS for now as well. The intended supported scenario for 9p currently is
macOS being host and Linux being guest. So the limitations of Linux would
apply anyway ATM.

Once we have macOS guest support, we can still work on a refinement to support
larger attributes IMO.

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

[Michael Roitzsch]

Hi,

> Are you sure d_seekoff doesn't work on macOS?

I just tried on an APFS volume on macOS Monterey and d_seekoff is always 0, while telldir() outputs useful values.

> Because using telldir() instead
> is not the same thing. Accessing d_*off is just POD access, whereas telldir()
> is a syscall.

I am not sure this is the case. I have tried a quick test program:

#include <dirent.h>

int main(void)
{
	int result = 0;
	DIR *dir = opendir(".");
	while (readdir(dir)) {
		result += telldir(dir);
	}
	closedir(dir);
	return result;
}

I ran it with 'sudo dtruss ./test', which should give me a trace of the system calls. The relevant portion is:

open_nocancel(".\0", 0x1100004, 0x0)		 = 3 0
sysctlbyname(kern.secure_kernel, 0x12, 0x7FF7BE49810C, 0x7FF7BE498110, 0x0)		 = 0 0
fstatfs64(0x3, 0x7FF7BE498110, 0x0)		 = 0 0
getdirentries64(0x3, 0x7FF4A5808A00, 0x2000)		 = 1472 0
close_nocancel(0x3)		 = 0 0

The directory has more than 30 entries, but the loop does not appear to cause individual system calls. Instead, readdir() and telldir() appear to be library functions powered by this getdirentries64 syscall.

This is on Monterey. Unfortunately I cannot test if older versions of macOS are different.

Michael

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

[Christian Schoenebeck]

On Montag, 22. November 2021 01:49:10 CET Will Cohen wrote:
> 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 1a5e3eed73..2bfff79b12 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -781,16 +781,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 627baebaba..38ef8b289d 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)

Why does this not have XATTR_NOFOLLOW and the others do? -^

> +#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;

Hmm, so we would have two different behaviours for Linux vs. macOS here.

If there is a symbolic link on host, Linux currently applies the permission
map as xattrs to the destination of the symlink, whereas macOS would map the
permissions as xattrs to the symbolic link itself.

Who is right?

Re: [PATCH v2 09/11] 9p: darwin: Provide fallback impl for utimensat

[Christian Schoenebeck]

On Montag, 22. November 2021 01:49:11 CET Will Cohen wrote:
> From: Keno Fischer <keno@juliacomputing.com>
> 
> This function is new in Mac OS 10.13. Provide a fallback implementation
> when building against older SDKs. The complication in the definition comes
> having to separately handle the used SDK version and the target OS version.
> 
> - If the SDK version is too low (__MAC_10_13 not defined), utimensat is not
>   defined in the header, so we must not try to use it (doing so would
> error). - Otherwise, if the targetted OS version is at least 10.13, we know
> this function is available, so we can unconditionally call it.

AFAIK QEMU officially only supports "the last two released versions" of macOS,
and Peter periodically wipes backward compatibility code for older macOS
versions with new release cycles:

commit 483644c25b932360018d15818d8bcd8c85ba70b8
Author: Peter Maydell <peter.maydell@linaro.org>
Date:   Sat Feb 1 17:05:34 2020 +0000

    ui/cocoa: Drop workarounds for pre-10.12 OSX
    
    Our official OSX support policy covers the last two released versions.
    Currently that is 10.14 and 10.15.  We also may work on older versions, but
    don't guarantee it.
    
    In commit 50290c002c045280f8d in mid-2019 we introduced some uses of
    CLOCK_MONOTONIC which incidentally broke compilation for pre-10.12 OSX
    versions (see LP:1861551). We don't intend to fix that, so we might
    as well drop the code in ui/cocoa.m which caters for pre-10.12
    versions as well. (For reference, 10.11 fell out of Apple extended
    security support in September 2018.)
    
    Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
    Message-Id: <20200201170534.22123-1-peter.maydell@linaro.org>
    Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

And this series would not go into a QEMU release prior early next year,
so probably dropping this macOS backward compatibility code right from
the start?

> - Lastly, we check for the availability of the __builtin_available macro to
>   potentially insert a dynamic check for this OS version. However,
> __builtin_available is only available with sufficiently recent versions of
> clang and while all Apple clang versions that ship with Xcode versions that
> support the 10.13 SDK support with builtin, we want to allow building with
> compilers other than Apple clang that may not support this builtin.
> 
> Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
> Signed-off-by: Will Cohen <wwcohen@gmail.com>
> ---
>  hw/9pfs/9p-local.c       |  2 +-
>  hw/9pfs/9p-util-darwin.c | 96 ++++++++++++++++++++++++++++++++++++++++
>  hw/9pfs/9p-util-linux.c  |  6 +++
>  hw/9pfs/9p-util.h        |  8 ++++
>  hw/9pfs/9p.c             |  1 +
>  5 files changed, 112 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index 2bfff79b12..4268703d05 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -1076,7 +1076,7 @@ static int local_utimensat(FsContext *s, V9fsPath
> *fs_path, goto out;
>      }
> 
> -    ret = utimensat(dirfd, name, buf, AT_SYMLINK_NOFOLLOW);
> +    ret = utimensat_nofollow(dirfd, name, buf);
>      close_preserve_errno(dirfd);
>  out:
>      g_free(dirpath);
> diff --git a/hw/9pfs/9p-util-darwin.c b/hw/9pfs/9p-util-darwin.c
> index cdb4c9e24c..ac414bcbfd 100644
> --- a/hw/9pfs/9p-util-darwin.c
> +++ b/hw/9pfs/9p-util-darwin.c
> @@ -62,3 +62,99 @@ int fsetxattrat_nofollow(int dirfd, const char *filename,
> const char *name, close_preserve_errno(fd);
>      return ret;
>  }
> +
> +#ifndef __has_builtin
> +#define __has_builtin(x) 0
> +#endif
> +
> +static int update_times_from_stat(int fd, struct timespec times[2],
> +                                  int update0, int update1)
> +{
> +    struct stat buf;
> +    int ret = fstat(fd, &buf);
> +    if (ret == -1) {
> +        return ret;
> +    }
> +    if (update0) {
> +        times[0] = buf.st_atimespec;
> +    }
> +    if (update1) {
> +        times[1] = buf.st_mtimespec;
> +    }
> +    return 0;
> +}
> +
> +int utimensat_nofollow(int dirfd, const char *filename,
> +                       const struct timespec times_in[2])
> +{
> +    int ret, fd;
> +    int special0, special1;
> +    struct timeval futimes_buf[2];
> +    struct timespec times[2];
> +    memcpy(times, times_in, 2 * sizeof(struct timespec));
> +
> +/* Check whether we have an SDK version that defines utimensat */
> +#if defined(__MAC_10_13)
> +# if __MAC_OS_X_VERSION_MIN_REQUIRED >= __MAC_10_13
> +#  define UTIMENSAT_AVAILABLE 1
> +# elif __has_builtin(__builtin_available)
> +#  define UTIMENSAT_AVAILABLE __builtin_available(macos 10.13, *)
> +# else
> +#  define UTIMENSAT_AVAILABLE 0
> +# endif
> +    if (UTIMENSAT_AVAILABLE) {
> +        return utimensat(dirfd, filename, times, AT_SYMLINK_NOFOLLOW);
> +    }
> +#endif
> +
> +    /* utimensat not available. Use futimes. */
> +    fd = openat_file(dirfd, filename, O_PATH_9P_UTIL | O_NOFOLLOW, 0);
> +    if (fd == -1) {
> +        return -1;
> +    }
> +
> +    special0 = times[0].tv_nsec == UTIME_OMIT;
> +    special1 = times[1].tv_nsec == UTIME_OMIT;
> +    if (special0 || special1) {
> +        /* If both are set, nothing to do */
> +        if (special0 && special1) {
> +            ret = 0;
> +            goto done;
> +        }
> +
> +        ret = update_times_from_stat(fd, times, special0, special1);
> +        if (ret < 0) {
> +            goto done;
> +        }
> +    }
> +
> +    special0 = times[0].tv_nsec == UTIME_NOW;
> +    special1 = times[1].tv_nsec == UTIME_NOW;
> +    if (special0 || special1) {
> +        ret = futimes(fd, NULL);
> +        if (ret < 0) {
> +            goto done;
> +        }
> +
> +        /* If both are set, we are done */
> +        if (special0 && special1) {
> +            ret = 0;
> +            goto done;
> +        }
> +
> +        ret = update_times_from_stat(fd, times, special0, special1);
> +        if (ret < 0) {
> +            goto done;
> +        }
> +    }
> +
> +    futimes_buf[0].tv_sec = times[0].tv_sec;
> +    futimes_buf[0].tv_usec = times[0].tv_nsec / 1000;
> +    futimes_buf[1].tv_sec = times[1].tv_sec;
> +    futimes_buf[1].tv_usec = times[1].tv_nsec / 1000;
> +    ret = futimes(fd, futimes_buf);
> +
> +done:
> +    close_preserve_errno(fd);
> +    return ret;
> +}
> diff --git a/hw/9pfs/9p-util-linux.c b/hw/9pfs/9p-util-linux.c
> index 398614a5d0..d54bf57a59 100644
> --- a/hw/9pfs/9p-util-linux.c
> +++ b/hw/9pfs/9p-util-linux.c
> @@ -62,3 +62,9 @@ int fsetxattrat_nofollow(int dirfd, const char *filename,
> const char *name, g_free(proc_path);
>      return ret;
>  }
> +
> +int utimensat_nofollow(int dirfd, const char *filename,
> +                       const struct timespec times[2])
> +{
> +    return utimensat(dirfd, filename, times, AT_SYMLINK_NOFOLLOW);
> +}
> diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h
> index 38ef8b289d..1c477a0e66 100644
> --- a/hw/9pfs/9p-util.h
> +++ b/hw/9pfs/9p-util.h
> @@ -36,6 +36,12 @@ static inline int qemu_lsetxattr(const char *path, const
> char *name, #define qemu_lsetxattr lsetxattr
>  #endif
> 
> +/* Compatibility with old SDK Versions for Darwin */
> +#if defined(CONFIG_DARWIN) && !defined(UTIME_NOW)
> +#define UTIME_NOW -1
> +#define UTIME_OMIT -2
> +#endif
> +
>  static inline void close_preserve_errno(int fd)
>  {
>      int serrno = errno;
> @@ -96,5 +102,7 @@ ssize_t flistxattrat_nofollow(int dirfd, const char
> *filename, char *list, size_t size);
>  ssize_t fremovexattrat_nofollow(int dirfd, const char *filename,
>                                  const char *name);
> +int utimensat_nofollow(int dirfd, const char *filename,
> +                       const struct timespec times[2]);
> 
>  #endif
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index d671995aa4..3d676405c7 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"

Re: [PATCH v2 10/11] 9p: darwin: Implement compatibility for mknodat

[Christian Schoenebeck]

On Montag, 22. November 2021 01:49:12 CET 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 as an (unexposed in the
> C library) system call that sets the cwd for the current thread only.
> This should suffice to use mknod safely.
> 
> Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
> [Will Cohen: - Adjust coding style]
> Signed-off-by: Will Cohen <wwcohen@gmail.com>
> ---
>  hw/9pfs/9p-local.c       |  5 +++--
>  hw/9pfs/9p-util-darwin.c | 33 +++++++++++++++++++++++++++++++++
>  hw/9pfs/9p-util-linux.c  |  5 +++++
>  hw/9pfs/9p-util.h        |  2 ++
>  4 files changed, 43 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index 4268703d05..42b65e143b 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -673,7 +673,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;
>          }
> @@ -688,7 +688,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;
>          }
> @@ -701,6 +701,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 ac414bcbfd..25e67d5067 100644
> --- a/hw/9pfs/9p-util-darwin.c
> +++ b/hw/9pfs/9p-util-darwin.c
> @@ -158,3 +158,36 @@ done:
>      close_preserve_errno(fd);
>      return ret;
>  }
> +
> +#ifndef SYS___pthread_fchdir
> +# define SYS___pthread_fchdir 349
> +#endif
> +
> +/*
> + * This is an undocumented OS X syscall. It would be best to avoid it,
> + * but there doesn't seem to be another safe way to implement mknodat.
> + * Dear Apple, please implement mknodat before you remove this syscall.
> + */
> +static int fchdir_thread_local(int fd)

Hooo, that's a brave move. Shouldn't its future and likely becoming absence be 
guarded "somehow"? :)

BTW it might make sense to file a report instead of hoping Apple will just 
read this comment: ;-)
https://feedbackassistant.apple.com/

> +{
> +#pragma clang diagnostic push
> +#pragma clang diagnostic ignored "-Wdeprecated-declarations"
> +    return syscall(SYS___pthread_fchdir, fd);
> +#pragma clang diagnostic pop
> +}

Consider s/clang/GCC/ then it would also work with GCC. In the end most people 
probably just use clang on macOS anyway, but just saying.

> +
> +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev)
> +{
> +    int preserved_errno, err;
> +    if (fchdir_thread_local(dirfd) < 0) {
> +        return -1;
> +    }
> +    err = mknod(filename, mode, dev);
> +    preserved_errno = errno;
> +    /* Stop using the thread-local cwd */
> +    fchdir_thread_local(-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 d54bf57a59..4f57d8c047 100644
> --- a/hw/9pfs/9p-util-linux.c
> +++ b/hw/9pfs/9p-util-linux.c
> @@ -68,3 +68,8 @@ int utimensat_nofollow(int dirfd, const char *filename,
>  {
>      return utimensat(dirfd, filename, times, AT_SYMLINK_NOFOLLOW);
>  }
> +
> +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 1c477a0e66..cac682d335 100644
> --- a/hw/9pfs/9p-util.h
> +++ b/hw/9pfs/9p-util.h
> @@ -105,4 +105,6 @@ ssize_t fremovexattrat_nofollow(int dirfd, const char
> *filename, int utimensat_nofollow(int dirfd, const char *filename,
>                         const struct timespec times[2]);
> 
> +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev);
> +
>  #endif

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

[Christian Schoenebeck]

On Mittwoch, 24. November 2021 16:45:30 CET Michael Roitzsch wrote:
> Hi,
> 
> > Are you sure d_seekoff doesn't work on macOS?
> 
> I just tried on an APFS volume on macOS Monterey and d_seekoff is always 0,
> while telldir() outputs useful values.
> > Because using telldir() instead
> > is not the same thing. Accessing d_*off is just POD access, whereas
> > telldir() is a syscall.
> 
> I am not sure this is the case. I have tried a quick test program:
> 
> #include <dirent.h>
> 
> int main(void)
> {
> 	int result = 0;
> 	DIR *dir = opendir(".");
> 	while (readdir(dir)) {
> 		result += telldir(dir);
> 	}
> 	closedir(dir);
> 	return result;
> }
> 
> I ran it with 'sudo dtruss ./test', which should give me a trace of the
> system calls. The relevant portion is:
> 
> open_nocancel(".\0", 0x1100004, 0x0)		 = 3 0
> sysctlbyname(kern.secure_kernel, 0x12, 0x7FF7BE49810C, 0x7FF7BE498110,
> 0x0)		 = 0 0 fstatfs64(0x3, 0x7FF7BE498110, 0x0)		 = 0 0
> getdirentries64(0x3, 0x7FF4A5808A00, 0x2000)		 = 1472 0
> close_nocancel(0x3)		 = 0 0
> 
> The directory has more than 30 entries, but the loop does not appear to
> cause individual system calls. Instead, readdir() and telldir() appear to
> be library functions powered by this getdirentries64 syscall.

Right, telldir() is part of Apple's libc, no (fs) syscall involved:
https://opensource.apple.com/source/Libc/Libc-167/gen.subproj/telldir.c.auto.html

However instead of changing the (fs driver independent) 9p server [9p.c] code
and using fidp there, I would probably rather address this issue for macOS in
the individual fs drivers [9p-local.c, 9p-synth.c, 9p-proxy.c] directly on
dirent instead, and not rely on fidp not having mutated on server.

And please make sure the 9p test cases pass.

Best regards,
Christian Schoenebeck

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

[Will Cohen]

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

On Wed, Nov 24, 2021 at 9:23 AM Christian Schoenebeck <
qemu_oss@crudebyte.com> wrote:

> On Montag, 22. November 2021 01:49:05 CET Will Cohen wrote:
> > From: Keno Fischer <keno@juliacomputing.com>
> >
> > Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> > Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
> > Signed-off-by: Will Cohen <wwcohen@gmail.com>
> > ---
> >  hw/9pfs/9p-proxy.c | 17 ++++++++++++++---
> >  hw/9pfs/9p-synth.c |  2 ++
> >  hw/9pfs/9p.c       | 16 ++++++++++++++--
> >  3 files changed, 30 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c
> > index 09bd9f1464..be1546c1be 100644
> > --- a/hw/9pfs/9p-proxy.c
> > +++ b/hw/9pfs/9p-proxy.c
> > @@ -123,10 +123,15 @@ 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
> > +    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
> >  }
>
> Please assign some value to f_namelen. You could either use the BSD
> version
> MAXNAMLEN from dirent.h (which you actually use for 9p.c below) or
> NAME_MAX
> from sys/syslimits.h on macOS.
>

statfs on darwin has no f_namelen or f_frsize present (
https://github.com/apple/darwin-xnu/blob/main/bsd/sys/mount.h#L141), which
is why they're excluded here. Given that this is converting to darwin's
statfs structure, is it okay if these stay omitted?


>
> >  /* Converts proxy_stat structure to VFS stat structure */
> > @@ -143,12 +148,18 @@ 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_atim.tv_sec = prstat->st_atim_sec;
>
> Where did that go to? ^-
>

Unless I'm mistaken, I think this logic should still be present. Pre-patch,
it's ordered so it runs through sec and nsec each for atime, mtime, and
ctime respectively. To handle the darwin logic more cleanly, it's now
broken out so it does _sec first, and then only needs one #ifdef to handle
the two _nsec cases.


>
> > -   stbuf->st_atim.tv_nsec = prstat->st_atim_nsec;
> > +   stbuf->st_atime = prstat->st_atim_sec;
> >     stbuf->st_mtime = prstat->st_mtim_sec;
> > -   stbuf->st_mtim.tv_nsec = prstat->st_mtim_nsec;
> >     stbuf->st_ctime = prstat->st_ctim_sec;
> > +#ifdef CONFIG_DARWIN
> > +   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_nsec = prstat->st_atim_nsec;
> > +   stbuf->st_mtim.tv_nsec = prstat->st_mtim_nsec;
> >     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;
> >  }
>
> As mentioned above.
>
> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > index 9c63e14b28..f4f0c200c7 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 = MAXNAMLEN;
> > +#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,
>
>
>

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

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

[Christian Schoenebeck]

On Mittwoch, 1. Dezember 2021 23:46:43 CET Will Cohen wrote:
> On Wed, Nov 24, 2021 at 9:23 AM Christian Schoenebeck <
> 
> qemu_oss@crudebyte.com> wrote:
> > On Montag, 22. November 2021 01:49:05 CET Will Cohen wrote:
> > > From: Keno Fischer <keno@juliacomputing.com>
> > > 
> > > Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> > > Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
> > > Signed-off-by: Will Cohen <wwcohen@gmail.com>
> > > ---
> > > 
> > >  hw/9pfs/9p-proxy.c | 17 ++++++++++++++---
> > >  hw/9pfs/9p-synth.c |  2 ++
> > >  hw/9pfs/9p.c       | 16 ++++++++++++++--
> > >  3 files changed, 30 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c
> > > index 09bd9f1464..be1546c1be 100644
> > > --- a/hw/9pfs/9p-proxy.c
> > > +++ b/hw/9pfs/9p-proxy.c
> > > @@ -123,10 +123,15 @@ 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
> > > +    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
> > > 
> > >  }
> > 
> > Please assign some value to f_namelen. You could either use the BSD
> > version
> > MAXNAMLEN from dirent.h (which you actually use for 9p.c below) or
> > NAME_MAX
> > from sys/syslimits.h on macOS.
> 
> statfs on darwin has no f_namelen or f_frsize present (
> https://github.com/apple/darwin-xnu/blob/main/bsd/sys/mount.h#L141), which
> is why they're excluded here. Given that this is converting to darwin's
> statfs structure, is it okay if these stay omitted?

Yes, that's OK for both.

With this patch f_frsize is set to zero, and 'man 2 statfs' on Linux sais:

	"Fields that are undefined for a particular filesystem are set to 0."

And the Linux kernel (guest side) seems to automatically use f_bsize in its
absence instead:
https://github.com/torvalds/linux/blob/58e1100fdc5990b0cc0d4beaf2562a92e621ac7d/fs/statfs.c#L67

And for f_namelen I realized there is only one place on 9p server where this
field is used at all, which is v9fs_fill_statfs() in 9p.c and you are already
assigning MAXNAMLEN at that place, so OK.

> > >  /* Converts proxy_stat structure to VFS stat structure */
> > > 
> > > @@ -143,12 +148,18 @@ 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_atim.tv_sec = prstat->st_atim_sec;
> > 
> > Where did that go to? ^-
> 
> Unless I'm mistaken, I think this logic should still be present. Pre-patch,
> it's ordered so it runs through sec and nsec each for atime, mtime, and
> ctime respectively. To handle the darwin logic more cleanly, it's now
> broken out so it does _sec first, and then only needs one #ifdef to handle
> the two _nsec cases.

If I understand you correctly, your point is that prstat_to_stat()
(9p-proxy.c) would leave st_[amc]time_sec uninitialized, but eventually
stat_to_v9stat_dotl() (9p.c) would fill them subsequently by:

	v9lstat->st_[amc]time_sec = stbuf->st_[amc]time;

Still wouldn't hurt to initialize these fields right from the start IMO. Which
was apparently never done in 9p-proxy.c for all 3 BTW.

> > > -   stbuf->st_atim.tv_nsec = prstat->st_atim_nsec;
> > > +   stbuf->st_atime = prstat->st_atim_sec;
> > > 
> > >     stbuf->st_mtime = prstat->st_mtim_sec;
> > > 
> > > -   stbuf->st_mtim.tv_nsec = prstat->st_mtim_nsec;
> > > 
> > >     stbuf->st_ctime = prstat->st_ctim_sec;
> > > 
> > > +#ifdef CONFIG_DARWIN
> > > +   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_nsec = prstat->st_atim_nsec;
> > > +   stbuf->st_mtim.tv_nsec = prstat->st_mtim_nsec;
> > > 
> > >     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;
> > >  
> > >  }
> > 
> > As mentioned above.
> > 
> > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > > index 9c63e14b28..f4f0c200c7 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 = MAXNAMLEN;
> > > +#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,

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

[Will Cohen]

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

It's quite possible I'm misreading the man page when double-checking this,
but I believe the l-prefixed functions are for the link itself and
fgetxattr works on the file it refers to. This is why XATTR_NOFOLLOW
doesn't appear on fgetxattr, and I believe this also means that these
definitions line up the macOS behavior with Linux.

On Wed, Nov 24, 2021 at 11:20 AM Christian Schoenebeck <
qemu_oss@crudebyte.com> wrote:

> On Montag, 22. November 2021 01:49:10 CET Will Cohen wrote:
> > 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 1a5e3eed73..2bfff79b12 100644
> > --- a/hw/9pfs/9p-local.c
> > +++ b/hw/9pfs/9p-local.c
> > @@ -781,16 +781,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 627baebaba..38ef8b289d 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)
>
> Why does this not have XATTR_NOFOLLOW and the others do? -^
>
> > +#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;
>
> Hmm, so we would have two different behaviours for Linux vs. macOS here.
>
> If there is a symbolic link on host, Linux currently applies the permission
> map as xattrs to the destination of the symlink, whereas macOS would map
> the
> permissions as xattrs to the symbolic link itself.
>
> Who is right?
>
>
>

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

Re: [PATCH v2 10/11] 9p: darwin: Implement compatibility for mknodat

[Will Cohen]

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

Back when this was being proposed, the original proposer did file such a
report to Apple, but we're still in this situation!

Replacing clang with gcc in v3.

On Wed, Nov 24, 2021 at 12:20 PM Christian Schoenebeck <
qemu_oss@crudebyte.com> wrote:

> On Montag, 22. November 2021 01:49:12 CET 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 as an (unexposed in the
> > C library) system call that sets the cwd for the current thread only.
> > This should suffice to use mknod safely.
> >
> > Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> > Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
> > [Will Cohen: - Adjust coding style]
> > Signed-off-by: Will Cohen <wwcohen@gmail.com>
> > ---
> >  hw/9pfs/9p-local.c       |  5 +++--
> >  hw/9pfs/9p-util-darwin.c | 33 +++++++++++++++++++++++++++++++++
> >  hw/9pfs/9p-util-linux.c  |  5 +++++
> >  hw/9pfs/9p-util.h        |  2 ++
> >  4 files changed, 43 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> > index 4268703d05..42b65e143b 100644
> > --- a/hw/9pfs/9p-local.c
> > +++ b/hw/9pfs/9p-local.c
> > @@ -673,7 +673,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;
> >          }
> > @@ -688,7 +688,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;
> >          }
> > @@ -701,6 +701,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 ac414bcbfd..25e67d5067 100644
> > --- a/hw/9pfs/9p-util-darwin.c
> > +++ b/hw/9pfs/9p-util-darwin.c
> > @@ -158,3 +158,36 @@ done:
> >      close_preserve_errno(fd);
> >      return ret;
> >  }
> > +
> > +#ifndef SYS___pthread_fchdir
> > +# define SYS___pthread_fchdir 349
> > +#endif
> > +
> > +/*
> > + * This is an undocumented OS X syscall. It would be best to avoid it,
> > + * but there doesn't seem to be another safe way to implement mknodat.
> > + * Dear Apple, please implement mknodat before you remove this syscall.
> > + */
> > +static int fchdir_thread_local(int fd)
>
> Hooo, that's a brave move. Shouldn't its future and likely becoming
> absence be
> guarded "somehow"? :)
>
> BTW it might make sense to file a report instead of hoping Apple will just
> read this comment: ;-)
> https://feedbackassistant.apple.com/
>
> > +{
> > +#pragma clang diagnostic push
> > +#pragma clang diagnostic ignored "-Wdeprecated-declarations"
> > +    return syscall(SYS___pthread_fchdir, fd);
> > +#pragma clang diagnostic pop
> > +}
>
> Consider s/clang/GCC/ then it would also work with GCC. In the end most
> people
> probably just use clang on macOS anyway, but just saying.
>
> > +
> > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t
> dev)
> > +{
> > +    int preserved_errno, err;
> > +    if (fchdir_thread_local(dirfd) < 0) {
> > +        return -1;
> > +    }
> > +    err = mknod(filename, mode, dev);
> > +    preserved_errno = errno;
> > +    /* Stop using the thread-local cwd */
> > +    fchdir_thread_local(-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 d54bf57a59..4f57d8c047 100644
> > --- a/hw/9pfs/9p-util-linux.c
> > +++ b/hw/9pfs/9p-util-linux.c
> > @@ -68,3 +68,8 @@ int utimensat_nofollow(int dirfd, const char *filename,
> >  {
> >      return utimensat(dirfd, filename, times, AT_SYMLINK_NOFOLLOW);
> >  }
> > +
> > +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 1c477a0e66..cac682d335 100644
> > --- a/hw/9pfs/9p-util.h
> > +++ b/hw/9pfs/9p-util.h
> > @@ -105,4 +105,6 @@ ssize_t fremovexattrat_nofollow(int dirfd, const char
> > *filename, int utimensat_nofollow(int dirfd, const char *filename,
> >                         const struct timespec times[2]);
> >
> > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t
> dev);
> > +
> >  #endif
>
>
>

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

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

[Will Cohen]

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

If acceptable, we'd still propose leaving these changes as is for
expediency's sake, rather than using our dirent and then translating it all
to save one read from the FS layer.

For v3, the synth tests will pass. We did have to modify the local
fs_unlinkat_dir test to correct AT_REMOVEDIR to P9_DOTL_AT_REMOVEDIR to get
that test to pass, but those now pass as well.


On Wed, Nov 24, 2021 at 2:09 PM Christian Schoenebeck <
qemu_oss@crudebyte.com> wrote:

> On Mittwoch, 24. November 2021 16:45:30 CET Michael Roitzsch wrote:
> > Hi,
> >
> > > Are you sure d_seekoff doesn't work on macOS?
> >
> > I just tried on an APFS volume on macOS Monterey and d_seekoff is always
> 0,
> > while telldir() outputs useful values.
> > > Because using telldir() instead
> > > is not the same thing. Accessing d_*off is just POD access, whereas
> > > telldir() is a syscall.
> >
> > I am not sure this is the case. I have tried a quick test program:
> >
> > #include <dirent.h>
> >
> > int main(void)
> > {
> >       int result = 0;
> >       DIR *dir = opendir(".");
> >       while (readdir(dir)) {
> >               result += telldir(dir);
> >       }
> >       closedir(dir);
> >       return result;
> > }
> >
> > I ran it with 'sudo dtruss ./test', which should give me a trace of the
> > system calls. The relevant portion is:
> >
> > open_nocancel(".\0", 0x1100004, 0x0)           = 3 0
> > sysctlbyname(kern.secure_kernel, 0x12, 0x7FF7BE49810C, 0x7FF7BE498110,
> > 0x0)           = 0 0 fstatfs64(0x3, 0x7FF7BE498110, 0x0)               =
> 0 0
> > getdirentries64(0x3, 0x7FF4A5808A00, 0x2000)           = 1472 0
> > close_nocancel(0x3)            = 0 0
> >
> > The directory has more than 30 entries, but the loop does not appear to
> > cause individual system calls. Instead, readdir() and telldir() appear to
> > be library functions powered by this getdirentries64 syscall.
>
> Right, telldir() is part of Apple's libc, no (fs) syscall involved:
>
> https://opensource.apple.com/source/Libc/Libc-167/gen.subproj/telldir.c.auto.html
>
> However instead of changing the (fs driver independent) 9p server [9p.c]
> code
> and using fidp there, I would probably rather address this issue for macOS
> in
> the individual fs drivers [9p-local.c, 9p-synth.c, 9p-proxy.c] directly on
> dirent instead, and not rely on fidp not having mutated on server.
>
> And please make sure the 9p test cases pass.
>
> Best regards,
> Christian Schoenebeck
>
>
>

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

Re: [PATCH v2 10/11] 9p: darwin: Implement compatibility for mknodat

[Christian Schoenebeck]

On Donnerstag, 27. Januar 2022 22:47:54 CET Will Cohen wrote:
> Back when this was being proposed, the original proposer did file such a
> report to Apple, but we're still in this situation!

Ok, but still, do you find it appropriate to just blindly use a private 
syscall that may or may not exist or might change its behaviour at any time 
without a user being aware?

I am not opposed to using workarounds at all, but what I worry about is that 
Apple might change this in whatever way at any time, and as this sycall is 
currently not guarded in this patch at all, we might one day receive bug 
reports by macOS users with symptoms that might not immediately be obvious to 
relate to this being the root cause.

Options that would come to my mind:
- a test case for this syscall
- a clear runtime error message for ordinary users

Is there a rdar or FB number for the report on Apple's side?

> Replacing clang with gcc in v3.
> 
> On Wed, Nov 24, 2021 at 12:20 PM Christian Schoenebeck <
> 
> qemu_oss@crudebyte.com> wrote:
> > On Montag, 22. November 2021 01:49:12 CET 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 as an (unexposed in the
> > > C library) system call that sets the cwd for the current thread only.
> > > This should suffice to use mknod safely.
> > > 
> > > Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> > > Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
> > > [Will Cohen: - Adjust coding style]
> > > Signed-off-by: Will Cohen <wwcohen@gmail.com>
> > > ---
> > > 
> > >  hw/9pfs/9p-local.c       |  5 +++--
> > >  hw/9pfs/9p-util-darwin.c | 33 +++++++++++++++++++++++++++++++++
> > >  hw/9pfs/9p-util-linux.c  |  5 +++++
> > >  hw/9pfs/9p-util.h        |  2 ++
> > >  4 files changed, 43 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> > > index 4268703d05..42b65e143b 100644
> > > --- a/hw/9pfs/9p-local.c
> > > +++ b/hw/9pfs/9p-local.c
> > > @@ -673,7 +673,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;
> > >          
> > >          }
> > > 
> > > @@ -688,7 +688,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;
> > >          
> > >          }
> > > 
> > > @@ -701,6 +701,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 ac414bcbfd..25e67d5067 100644
> > > --- a/hw/9pfs/9p-util-darwin.c
> > > +++ b/hw/9pfs/9p-util-darwin.c
> > > 
> > > @@ -158,3 +158,36 @@ done:
> > >      close_preserve_errno(fd);
> > >      return ret;
> > >  
> > >  }
> > > 
> > > +
> > > +#ifndef SYS___pthread_fchdir
> > > +# define SYS___pthread_fchdir 349
> > > +#endif
> > > +
> > > +/*
> > > + * This is an undocumented OS X syscall. It would be best to avoid it,
> > > + * but there doesn't seem to be another safe way to implement mknodat.
> > > + * Dear Apple, please implement mknodat before you remove this syscall.
> > > + */
> > > +static int fchdir_thread_local(int fd)
> > 
> > Hooo, that's a brave move. Shouldn't its future and likely becoming
> > absence be
> > guarded "somehow"? :)
> > 
> > BTW it might make sense to file a report instead of hoping Apple will just
> > read this comment: ;-)
> > https://feedbackassistant.apple.com/
> > 
> > > +{
> > > +#pragma clang diagnostic push
> > > +#pragma clang diagnostic ignored "-Wdeprecated-declarations"
> > > +    return syscall(SYS___pthread_fchdir, fd);
> > > +#pragma clang diagnostic pop
> > > +}
> > 
> > Consider s/clang/GCC/ then it would also work with GCC. In the end most
> > people
> > probably just use clang on macOS anyway, but just saying.
> > 
> > > +
> > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t
> > 
> > dev)
> > 
> > > +{
> > > +    int preserved_errno, err;
> > > +    if (fchdir_thread_local(dirfd) < 0) {
> > > +        return -1;
> > > +    }
> > > +    err = mknod(filename, mode, dev);
> > > +    preserved_errno = errno;
> > > +    /* Stop using the thread-local cwd */
> > > +    fchdir_thread_local(-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 d54bf57a59..4f57d8c047 100644
> > > --- a/hw/9pfs/9p-util-linux.c
> > > +++ b/hw/9pfs/9p-util-linux.c
> > > @@ -68,3 +68,8 @@ int utimensat_nofollow(int dirfd, const char
> > > *filename,
> > > 
> > >  {
> > >  
> > >      return utimensat(dirfd, filename, times, AT_SYMLINK_NOFOLLOW);
> > >  
> > >  }
> > > 
> > > +
> > > +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 1c477a0e66..cac682d335 100644
> > > --- a/hw/9pfs/9p-util.h
> > > +++ b/hw/9pfs/9p-util.h
> > > @@ -105,4 +105,6 @@ ssize_t fremovexattrat_nofollow(int dirfd, const
> > > char
> > > *filename, int utimensat_nofollow(int dirfd, const char *filename,
> > > 
> > >                         const struct timespec times[2]);
> > > 
> > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t
> > 
> > dev);
> > 
> > > +
> > > 
> > >  #endif


Best regards,
Christian Schoenebeck

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

[Christian Schoenebeck]

On Donnerstag, 27. Januar 2022 22:48:02 CET Will Cohen wrote:
> If acceptable, we'd still propose leaving these changes as is for
> expediency's sake, rather than using our dirent and then translating it all
> to save one read from the FS layer.

The problem is V9fsFidState *fidp is a shared resource that might become 
mutated by other threads in between and could therefore lead to concurrency 
issues and undefined behaviour, whereas struct dirent is a local resource not 
being shared.

See also BTW:
https://lore.kernel.org/qemu-devel/20220127212734.218900-1-vt@altlinux.org/

Best regards,
Christian Schoenebeck

Re: [PATCH v2 10/11] 9p: darwin: Implement compatibility for mknodat

[Will Cohen]

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

Understood. Since I cannot find the original number, I have submitted a new
report at rdar://FB9862426 <https://openradar.appspot.com/FB9862426> (
https://openradar.appspot.com/FB9862426).

I'll note that and work on a testcase/error message for v4.

Many thanks,
Will

On Fri, Jan 28, 2022 at 10:15 AM Christian Schoenebeck <
qemu_oss@crudebyte.com> wrote:

> On Donnerstag, 27. Januar 2022 22:47:54 CET Will Cohen wrote:
> > Back when this was being proposed, the original proposer did file such a
> > report to Apple, but we're still in this situation!
>
> Ok, but still, do you find it appropriate to just blindly use a private
> syscall that may or may not exist or might change its behaviour at any
> time
> without a user being aware?
>
> I am not opposed to using workarounds at all, but what I worry about is
> that
> Apple might change this in whatever way at any time, and as this sycall is
> currently not guarded in this patch at all, we might one day receive bug
> reports by macOS users with symptoms that might not immediately be obvious
> to
> relate to this being the root cause.
>
> Options that would come to my mind:
> - a test case for this syscall
> - a clear runtime error message for ordinary users
>
> Is there a rdar or FB number for the report on Apple's side?
>
> > Replacing clang with gcc in v3.
> >
> > On Wed, Nov 24, 2021 at 12:20 PM Christian Schoenebeck <
> >
> > qemu_oss@crudebyte.com> wrote:
> > > On Montag, 22. November 2021 01:49:12 CET 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 as an (unexposed in the
> > > > C library) system call that sets the cwd for the current thread only.
> > > > This should suffice to use mknod safely.
> > > >
> > > > Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> > > > Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
> > > > [Will Cohen: - Adjust coding style]
> > > > Signed-off-by: Will Cohen <wwcohen@gmail.com>
> > > > ---
> > > >
> > > >  hw/9pfs/9p-local.c       |  5 +++--
> > > >  hw/9pfs/9p-util-darwin.c | 33 +++++++++++++++++++++++++++++++++
> > > >  hw/9pfs/9p-util-linux.c  |  5 +++++
> > > >  hw/9pfs/9p-util.h        |  2 ++
> > > >  4 files changed, 43 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> > > > index 4268703d05..42b65e143b 100644
> > > > --- a/hw/9pfs/9p-local.c
> > > > +++ b/hw/9pfs/9p-local.c
> > > > @@ -673,7 +673,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;
> > > >
> > > >          }
> > > >
> > > > @@ -688,7 +688,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;
> > > >
> > > >          }
> > > >
> > > > @@ -701,6 +701,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 ac414bcbfd..25e67d5067 100644
> > > > --- a/hw/9pfs/9p-util-darwin.c
> > > > +++ b/hw/9pfs/9p-util-darwin.c
> > > >
> > > > @@ -158,3 +158,36 @@ done:
> > > >      close_preserve_errno(fd);
> > > >      return ret;
> > > >
> > > >  }
> > > >
> > > > +
> > > > +#ifndef SYS___pthread_fchdir
> > > > +# define SYS___pthread_fchdir 349
> > > > +#endif
> > > > +
> > > > +/*
> > > > + * This is an undocumented OS X syscall. It would be best to avoid
> it,
> > > > + * but there doesn't seem to be another safe way to implement
> mknodat.
> > > > + * Dear Apple, please implement mknodat before you remove this
> syscall.
> > > > + */
> > > > +static int fchdir_thread_local(int fd)
> > >
> > > Hooo, that's a brave move. Shouldn't its future and likely becoming
> > > absence be
> > > guarded "somehow"? :)
> > >
> > > BTW it might make sense to file a report instead of hoping Apple will
> just
> > > read this comment: ;-)
> > > https://feedbackassistant.apple.com/
> > >
> > > > +{
> > > > +#pragma clang diagnostic push
> > > > +#pragma clang diagnostic ignored "-Wdeprecated-declarations"
> > > > +    return syscall(SYS___pthread_fchdir, fd);
> > > > +#pragma clang diagnostic pop
> > > > +}
> > >
> > > Consider s/clang/GCC/ then it would also work with GCC. In the end most
> > > people
> > > probably just use clang on macOS anyway, but just saying.
> > >
> > > > +
> > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t
> > >
> > > dev)
> > >
> > > > +{
> > > > +    int preserved_errno, err;
> > > > +    if (fchdir_thread_local(dirfd) < 0) {
> > > > +        return -1;
> > > > +    }
> > > > +    err = mknod(filename, mode, dev);
> > > > +    preserved_errno = errno;
> > > > +    /* Stop using the thread-local cwd */
> > > > +    fchdir_thread_local(-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 d54bf57a59..4f57d8c047 100644
> > > > --- a/hw/9pfs/9p-util-linux.c
> > > > +++ b/hw/9pfs/9p-util-linux.c
> > > > @@ -68,3 +68,8 @@ int utimensat_nofollow(int dirfd, const char
> > > > *filename,
> > > >
> > > >  {
> > > >
> > > >      return utimensat(dirfd, filename, times, AT_SYMLINK_NOFOLLOW);
> > > >
> > > >  }
> > > >
> > > > +
> > > > +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 1c477a0e66..cac682d335 100644
> > > > --- a/hw/9pfs/9p-util.h
> > > > +++ b/hw/9pfs/9p-util.h
> > > > @@ -105,4 +105,6 @@ ssize_t fremovexattrat_nofollow(int dirfd, const
> > > > char
> > > > *filename, int utimensat_nofollow(int dirfd, const char *filename,
> > > >
> > > >                         const struct timespec times[2]);
> > > >
> > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t
> > >
> > > dev);
> > >
> > > > +
> > > >
> > > >  #endif
>
>
> Best regards,
> Christian Schoenebeck
>
>
>

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

Re: [PATCH v2 10/11] 9p: darwin: Implement compatibility for mknodat

[Will Cohen]

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

Upon further review, it looks like since 10.12 there's actually a
(not-heavily-documented) function that wraps this syscall and avoids the
need to call the private syscall directly:
https://opensource.apple.com/source/libpthread/libpthread-218.51.1/src/pthread_cwd.c.auto.html.
Chromium uses it too (
https://chromium.googlesource.com/chromium/src/+/lkgr/base/process/launch_mac.cc#110)
-- given that we're not looking for pre-10.12 compatibility, I'm a little
less worried about the workaround breaking in the future if this wrapper
gets used instead.

Would it work to change to pthread_fchdir_np, remove all the syscall
discussion in the comment, and add a meson check for pthread_fchdir_np as a
prereq for virtfs on darwin?

On Fri, Jan 28, 2022 at 1:28 PM Will Cohen <wwcohen@gmail.com> wrote:

> Understood. Since I cannot find the original number, I have submitted a
> new report at rdar://FB9862426 <https://openradar.appspot.com/FB9862426> (
> https://openradar.appspot.com/FB9862426).
>
> I'll note that and work on a testcase/error message for v4.
>
> Many thanks,
> Will
>
> On Fri, Jan 28, 2022 at 10:15 AM Christian Schoenebeck <
> qemu_oss@crudebyte.com> wrote:
>
>> On Donnerstag, 27. Januar 2022 22:47:54 CET Will Cohen wrote:
>> > Back when this was being proposed, the original proposer did file such a
>> > report to Apple, but we're still in this situation!
>>
>> Ok, but still, do you find it appropriate to just blindly use a private
>> syscall that may or may not exist or might change its behaviour at any
>> time
>> without a user being aware?
>>
>> I am not opposed to using workarounds at all, but what I worry about is
>> that
>> Apple might change this in whatever way at any time, and as this sycall
>> is
>> currently not guarded in this patch at all, we might one day receive bug
>> reports by macOS users with symptoms that might not immediately be
>> obvious to
>> relate to this being the root cause.
>>
>> Options that would come to my mind:
>> - a test case for this syscall
>> - a clear runtime error message for ordinary users
>>
>> Is there a rdar or FB number for the report on Apple's side?
>>
>> > Replacing clang with gcc in v3.
>> >
>> > On Wed, Nov 24, 2021 at 12:20 PM Christian Schoenebeck <
>> >
>> > qemu_oss@crudebyte.com> wrote:
>> > > On Montag, 22. November 2021 01:49:12 CET 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 as an (unexposed in the
>> > > > C library) system call that sets the cwd for the current thread
>> only.
>> > > > This should suffice to use mknod safely.
>> > > >
>> > > > Signed-off-by: Keno Fischer <keno@juliacomputing.com>
>> > > > Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
>> > > > [Will Cohen: - Adjust coding style]
>> > > > Signed-off-by: Will Cohen <wwcohen@gmail.com>
>> > > > ---
>> > > >
>> > > >  hw/9pfs/9p-local.c       |  5 +++--
>> > > >  hw/9pfs/9p-util-darwin.c | 33 +++++++++++++++++++++++++++++++++
>> > > >  hw/9pfs/9p-util-linux.c  |  5 +++++
>> > > >  hw/9pfs/9p-util.h        |  2 ++
>> > > >  4 files changed, 43 insertions(+), 2 deletions(-)
>> > > >
>> > > > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
>> > > > index 4268703d05..42b65e143b 100644
>> > > > --- a/hw/9pfs/9p-local.c
>> > > > +++ b/hw/9pfs/9p-local.c
>> > > > @@ -673,7 +673,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;
>> > > >
>> > > >          }
>> > > >
>> > > > @@ -688,7 +688,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;
>> > > >
>> > > >          }
>> > > >
>> > > > @@ -701,6 +701,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 ac414bcbfd..25e67d5067 100644
>> > > > --- a/hw/9pfs/9p-util-darwin.c
>> > > > +++ b/hw/9pfs/9p-util-darwin.c
>> > > >
>> > > > @@ -158,3 +158,36 @@ done:
>> > > >      close_preserve_errno(fd);
>> > > >      return ret;
>> > > >
>> > > >  }
>> > > >
>> > > > +
>> > > > +#ifndef SYS___pthread_fchdir
>> > > > +# define SYS___pthread_fchdir 349
>> > > > +#endif
>> > > > +
>> > > > +/*
>> > > > + * This is an undocumented OS X syscall. It would be best to avoid
>> it,
>> > > > + * but there doesn't seem to be another safe way to implement
>> mknodat.
>> > > > + * Dear Apple, please implement mknodat before you remove this
>> syscall.
>> > > > + */
>> > > > +static int fchdir_thread_local(int fd)
>> > >
>> > > Hooo, that's a brave move. Shouldn't its future and likely becoming
>> > > absence be
>> > > guarded "somehow"? :)
>> > >
>> > > BTW it might make sense to file a report instead of hoping Apple will
>> just
>> > > read this comment: ;-)
>> > > https://feedbackassistant.apple.com/
>> > >
>> > > > +{
>> > > > +#pragma clang diagnostic push
>> > > > +#pragma clang diagnostic ignored "-Wdeprecated-declarations"
>> > > > +    return syscall(SYS___pthread_fchdir, fd);
>> > > > +#pragma clang diagnostic pop
>> > > > +}
>> > >
>> > > Consider s/clang/GCC/ then it would also work with GCC. In the end
>> most
>> > > people
>> > > probably just use clang on macOS anyway, but just saying.
>> > >
>> > > > +
>> > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode,
>> dev_t
>> > >
>> > > dev)
>> > >
>> > > > +{
>> > > > +    int preserved_errno, err;
>> > > > +    if (fchdir_thread_local(dirfd) < 0) {
>> > > > +        return -1;
>> > > > +    }
>> > > > +    err = mknod(filename, mode, dev);
>> > > > +    preserved_errno = errno;
>> > > > +    /* Stop using the thread-local cwd */
>> > > > +    fchdir_thread_local(-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 d54bf57a59..4f57d8c047 100644
>> > > > --- a/hw/9pfs/9p-util-linux.c
>> > > > +++ b/hw/9pfs/9p-util-linux.c
>> > > > @@ -68,3 +68,8 @@ int utimensat_nofollow(int dirfd, const char
>> > > > *filename,
>> > > >
>> > > >  {
>> > > >
>> > > >      return utimensat(dirfd, filename, times, AT_SYMLINK_NOFOLLOW);
>> > > >
>> > > >  }
>> > > >
>> > > > +
>> > > > +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 1c477a0e66..cac682d335 100644
>> > > > --- a/hw/9pfs/9p-util.h
>> > > > +++ b/hw/9pfs/9p-util.h
>> > > > @@ -105,4 +105,6 @@ ssize_t fremovexattrat_nofollow(int dirfd, const
>> > > > char
>> > > > *filename, int utimensat_nofollow(int dirfd, const char *filename,
>> > > >
>> > > >                         const struct timespec times[2]);
>> > > >
>> > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode,
>> dev_t
>> > >
>> > > dev);
>> > >
>> > > > +
>> > > >
>> > > >  #endif
>>
>>
>> Best regards,
>> Christian Schoenebeck
>>
>>
>>

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

Re: [PATCH v2 10/11] 9p: darwin: Implement compatibility for mknodat

[Christian Schoenebeck]

On Montag, 31. Januar 2022 23:26:46 CET Will Cohen wrote:
> Upon further review, it looks like since 10.12 there's actually a
> (not-heavily-documented) function that wraps this syscall and avoids the
> need to call the private syscall directly:
> https://opensource.apple.com/source/libpthread/libpthread-218.51.1/src/pthre
> ad_cwd.c.auto.html. Chromium uses it too (
> https://chromium.googlesource.com/chromium/src/+/lkgr/base/process/launch_ma
> c.cc#110) -- given that we're not looking for pre-10.12 compatibility, I'm a
> little less worried about the workaround breaking in the future if this
> wrapper gets used instead.
> 
> Would it work to change to pthread_fchdir_np, remove all the syscall
> discussion in the comment, and add a meson check for pthread_fchdir_np as a
> prereq for virtfs on darwin?

Using pthread_fchdir_np() looks like a better solution, yes. It still seems to 
be a private macOS API though. I can't find the function in any of Apple's 
publicly released header file, and Chromium therefore declares the function by 
itself (directly in launch_mac.cc):

extern "C" {
// Changes the current thread's directory to a path or directory file
// descriptor. libpthread only exposes a syscall wrapper starting in
// macOS 10.12, but the system call dates back to macOS 10.5. On older OSes,
// the syscall is issued directly.
int pthread_chdir_np(const char* dir) API_AVAILABLE(macosx(10.12));
int pthread_fchdir_np(int fd) API_AVAILABLE(macosx(10.12));
...
}  // extern "C"

But if you are guarding this with a meson check then sure, no objections from 
my side at least.

Adding Peter on CC just in case.

Best regards,
Christian Schoenebeck