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

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

[Will Cohen]

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

Since v4, the following changes have been made to the following patches:

Patch 4/11: 9p: darwin: Handle struct dirent differences
- Cleanup of telldir error handling
- Remove superfluous error handling for qemu_dirent_off
- Removal of superfluous whitespace
- Adjust codir.c to use qemu_dirent_off instead of duplicating the logic

Patch 9/11: 9p: darwin: Implement compatibility for mknodat
- Move qemu_mknodat from 9p-util to osdep and os-posix

Patch 10/11: 9p: darwin: meson: Allow VirtFS on Darwin
- Add comments to patch commit
- Note that virtfs_proxy_helper does not work on Darwin and adjust
- Adjust meson virtfs error note to specify macOS

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

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

 fsdev/file-op-9p.h                     |  9 +++-
 fsdev/meson.build                      |  1 +
 hw/9pfs/9p-local.c                     | 27 ++++++++---
 hw/9pfs/9p-proxy.c                     | 38 +++++++++++++--
 hw/9pfs/9p-synth.c                     |  6 +++
 hw/9pfs/9p-util-darwin.c               | 64 ++++++++++++++++++++++++++
 hw/9pfs/{9p-util.c => 9p-util-linux.c} |  2 +-
 hw/9pfs/9p-util.h                      | 35 ++++++++++++++
 hw/9pfs/9p.c                           | 42 ++++++++++++++---
 hw/9pfs/9p.h                           | 11 +++++
 hw/9pfs/codir.c                        |  4 +-
 hw/9pfs/meson.build                    |  3 +-
 include/qemu/osdep.h                   | 10 ++++
 include/qemu/xattr.h                   |  4 +-
 meson.build                            | 14 ++++--
 os-posix.c                             | 34 ++++++++++++++
 tests/qtest/virtio-9p-test.c           |  2 +-
 17 files changed, 281 insertions(+), 25 deletions(-)
 create mode 100644 hw/9pfs/9p-util-darwin.c
 rename hw/9pfs/{9p-util.c => 9p-util-linux.c} (97%)

-- 
2.32.0 (Apple Git-132)

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

[Will Cohen]

From: Keno Fischer <keno@juliacomputing.com>

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

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

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

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

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

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

[Will Cohen]

From: Keno Fischer <keno@juliacomputing.com>

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

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

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

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

[Will Cohen]

From: Keno Fischer <keno@juliacomputing.com>

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

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

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

[Will Cohen]

From: Keno Fischer <keno@juliacomputing.com>

On darwin d_seekoff exists, but is optional and does not seem to
be commonly used by file systems. Use `telldir` instead to obtain
the seek offset and inject it into d_seekoff, and create a
qemu_dirent_off helper to call it appropriately when appropriate.

Signed-off-by: Keno Fischer <keno@juliacomputing.com>
[Michael Roitzsch: - Rebase for NixOS]
Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
[Will Cohen: - Adjust to pass testing
             - Ensure that d_seekoff is filled using telldir
               on darwin, and create qemu_dirent_off helper
               to decide which to access]
[Fabian Franz: - Add telldir error handling for darwin]
Signed-off-by: Fabian Franz <fabianfranz.oss@gmail.com>
[Will Cohen: - Ensure that telldir error handling uses
               signed int
             - Cleanup of telldir error handling
             - Remove superfluous error handling for
               qemu_dirent_off
             - Adjust formatting
             - Use qemu_dirent_off in codir.c]
Signed-off-by: Will Cohen <wwcohen@gmail.com>
---
 hw/9pfs/9p-local.c |  9 +++++++++
 hw/9pfs/9p-proxy.c | 16 +++++++++++++++-
 hw/9pfs/9p-synth.c |  4 ++++
 hw/9pfs/9p-util.h  | 16 ++++++++++++++++
 hw/9pfs/9p.c       |  7 +++++--
 hw/9pfs/codir.c    |  4 +++-
 6 files changed, 52 insertions(+), 4 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 1a5e3eed73..f3272f0b43 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -562,6 +562,15 @@ again:
     if (!entry) {
         return NULL;
     }
+#ifdef CONFIG_DARWIN
+    int off;
+    off = telldir(fs->dir.stream);
+    /* If telldir fails, fail the entire readdir call */
+    if (off < 0) {
+        return NULL;
+    }
+    entry->d_seekoff = off;
+#endif
 
     if (ctx->export_flags & V9FS_SM_MAPPED) {
         entry->d_type = DT_UNKNOWN;
diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c
index b1664080d8..8b4b5cf7dc 100644
--- a/hw/9pfs/9p-proxy.c
+++ b/hw/9pfs/9p-proxy.c
@@ -706,7 +706,21 @@ static off_t proxy_telldir(FsContext *ctx, V9fsFidOpenState *fs)
 
 static struct dirent *proxy_readdir(FsContext *ctx, V9fsFidOpenState *fs)
 {
-    return readdir(fs->dir.stream);
+    struct dirent *entry;
+    entry = readdir(fs->dir.stream);
+#ifdef CONFIG_DARWIN
+    if (!entry) {
+        return NULL;
+    }
+    int td;
+    td = telldir(fs->dir.stream);
+    /* If telldir fails, fail the entire readdir call */
+    if (td < 0) {
+        return NULL;
+    }
+    entry->d_seekoff = td;
+#endif
+    return entry;
 }
 
 static void proxy_seekdir(FsContext *ctx, V9fsFidOpenState *fs, off_t off)
diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
index 4a4a776d06..e264a03eef 100644
--- a/hw/9pfs/9p-synth.c
+++ b/hw/9pfs/9p-synth.c
@@ -222,7 +222,11 @@ static void synth_direntry(V9fsSynthNode *node,
 {
     strcpy(entry->d_name, node->name);
     entry->d_ino = node->attr->inode;
+#ifdef CONFIG_DARWIN
+    entry->d_seekoff = off + 1;
+#else
     entry->d_off = off + 1;
+#endif
 }
 
 static struct dirent *synth_get_dentry(V9fsSynthNode *dir,
diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h
index 546f46dc7d..d41f37f085 100644
--- a/hw/9pfs/9p-util.h
+++ b/hw/9pfs/9p-util.h
@@ -79,3 +79,19 @@ ssize_t fremovexattrat_nofollow(int dirfd, const char *filename,
                                 const char *name);
 
 #endif
+
+
+/**
+ * Darwin has d_seekoff, which appears to function similarly to d_off.
+ * However, it does not appear to be supported on all file systems,
+ * so ensure it is manually injected earlier and call here when
+ * needed.
+ */
+inline off_t qemu_dirent_off(struct dirent *dent)
+{
+#ifdef CONFIG_DARWIN
+    return dent->d_seekoff;
+#else
+    return dent->d_off;
+#endif
+}
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 1563d7b7c6..caf3b240fe 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -27,6 +27,7 @@
 #include "virtio-9p.h"
 #include "fsdev/qemu-fsdev.h"
 #include "9p-xattr.h"
+#include "9p-util.h"
 #include "coth.h"
 #include "trace.h"
 #include "migration/blocker.h"
@@ -2281,7 +2282,7 @@ static int coroutine_fn v9fs_do_readdir_with_stat(V9fsPDU *pdu,
         count += len;
         v9fs_stat_free(&v9stat);
         v9fs_path_free(&path);
-        saved_dir_pos = dent->d_off;
+        saved_dir_pos = qemu_dirent_off(dent);
     }
 
     v9fs_readdir_unlock(&fidp->fs.dir);
@@ -2420,6 +2421,7 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu, V9fsFidState *fidp,
     V9fsString name;
     int len, err = 0;
     int32_t count = 0;
+    off_t off;
     struct dirent *dent;
     struct stat *st;
     struct V9fsDirEnt *entries = NULL;
@@ -2480,12 +2482,13 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu, V9fsFidState *fidp,
             qid.version = 0;
         }
 
+        off = qemu_dirent_off(dent);
         v9fs_string_init(&name);
         v9fs_string_sprintf(&name, "%s", dent->d_name);
 
         /* 11 = 7 + 4 (7 = start offset, 4 = space for storing count) */
         len = pdu_marshal(pdu, 11 + count, "Qqbs",
-                          &qid, dent->d_off,
+                          &qid, off,
                           dent->d_type, &name);
 
         v9fs_string_free(&name);
diff --git a/hw/9pfs/codir.c b/hw/9pfs/codir.c
index 032cce04c4..8e66205d9d 100644
--- a/hw/9pfs/codir.c
+++ b/hw/9pfs/codir.c
@@ -22,6 +22,8 @@
 #include "qemu/coroutine.h"
 #include "qemu/main-loop.h"
 #include "coth.h"
+#include "9p-xattr.h"
+#include "9p-util.h"
 
 /*
  * Intended to be called from bottom-half (e.g. background I/O thread)
@@ -167,7 +169,7 @@ static int do_readdir_many(V9fsPDU *pdu, V9fsFidState *fidp,
         }
 
         size += len;
-        saved_dir_pos = dent->d_off;
+        saved_dir_pos = qemu_dirent_off(dent);
     }
 
     /* restore (last) saved position */
-- 
2.32.0 (Apple Git-132)

[PATCH v5 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 d41f37f085..0e445b5d52 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 caf3b240fe..14e84c3bcf 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -138,11 +138,20 @@ static int dotl_to_open_flags(int flags)
         { P9_DOTL_NONBLOCK, O_NONBLOCK } ,
         { P9_DOTL_DSYNC, O_DSYNC },
         { P9_DOTL_FASYNC, FASYNC },
+#ifndef CONFIG_DARWIN
+        { P9_DOTL_NOATIME, O_NOATIME },
+        /*
+         *  On Darwin, we could map to F_NOCACHE, which is
+         *  similar, but doesn't quite have the same
+         *  semantics. However, we don't support O_DIRECT
+         *  even on linux at the moment, so we just ignore
+         *  it here.
+         */
         { P9_DOTL_DIRECT, O_DIRECT },
+#endif
         { P9_DOTL_LARGEFILE, O_LARGEFILE },
         { P9_DOTL_DIRECTORY, O_DIRECTORY },
         { P9_DOTL_NOFOLLOW, O_NOFOLLOW },
-        { P9_DOTL_NOATIME, O_NOATIME },
         { P9_DOTL_SYNC, O_SYNC },
     };
 
@@ -171,10 +180,12 @@ static int get_dotl_openflags(V9fsState *s, int oflags)
      */
     flags = dotl_to_open_flags(oflags);
     flags &= ~(O_NOCTTY | O_ASYNC | O_CREAT);
+#ifndef CONFIG_DARWIN
     /*
      * Ignore direct disk access hint until the server supports it.
      */
     flags &= ~O_DIRECT;
+#endif
     return flags;
 }
 
-- 
2.32.0 (Apple Git-132)

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

[Will Cohen]

From: Keno Fischer <keno@juliacomputing.com>

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

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

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

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

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

[Will Cohen]

From: Keno Fischer <keno@juliacomputing.com>

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

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

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

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

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

[Will Cohen]

From: Keno Fischer <keno@juliacomputing.com>

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

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

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

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index a0d08e5216..d42ce6d8b8 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -682,7 +682,7 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath *dir_path,
 
     if (fs_ctx->export_flags & V9FS_SM_MAPPED ||
         fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
-        err = mknodat(dirfd, name, fs_ctx->fmode | S_IFREG, 0);
+        err = qemu_mknodat(dirfd, name, fs_ctx->fmode | S_IFREG, 0);
         if (err == -1) {
             goto out;
         }
@@ -697,7 +697,7 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath *dir_path,
         }
     } else if (fs_ctx->export_flags & V9FS_SM_PASSTHROUGH ||
                fs_ctx->export_flags & V9FS_SM_NONE) {
-        err = mknodat(dirfd, name, credp->fc_mode, credp->fc_rdev);
+        err = qemu_mknodat(dirfd, name, credp->fc_mode, credp->fc_rdev);
         if (err == -1) {
             goto out;
         }
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index d1660d67fa..f3a8367ece 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -810,3 +810,13 @@ static inline int platform_does_not_support_system(const char *command)
 #endif
 
 #endif
+
+/*
+ * As long as mknodat is not available on macOS, this workaround
+ * using pthread_fchdir_np is needed. qemu_mknodat is defined in
+ * os-posix.c
+ */
+#ifdef CONFIG_DARWIN
+int pthread_fchdir_np(int fd);
+#endif
+int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev);
diff --git a/os-posix.c b/os-posix.c
index ae6c9f2a5e..95c1607065 100644
--- a/os-posix.c
+++ b/os-posix.c
@@ -24,6 +24,7 @@
  */
 
 #include "qemu/osdep.h"
+#include <os/availability.h>
 #include <sys/wait.h>
 #include <pwd.h>
 #include <grp.h>
@@ -332,3 +333,36 @@ int os_mlock(void)
     return -ENOSYS;
 #endif
 }
+
+/*
+ * As long as mknodat is not available on macOS, this workaround
+ * using pthread_fchdir_np is needed.
+ *
+ * Radar filed with Apple for implementing mknodat:
+ * rdar://FB9862426 (https://openradar.appspot.com/FB9862426)
+ */
+#ifdef CONFIG_DARWIN
+
+int pthread_fchdir_np(int fd) API_AVAILABLE(macosx(10.12));
+
+int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev)
+{
+    int preserved_errno, err;
+    if (pthread_fchdir_np(dirfd) < 0) {
+        return -1;
+    }
+    err = mknod(filename, mode, dev);
+    preserved_errno = errno;
+    /* Stop using the thread-local cwd */
+    pthread_fchdir_np(-1);
+    if (err < 0) {
+        errno = preserved_errno;
+    }
+    return err;
+}
+#else
+int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev)
+{
+    return mknodat(dirfd, filename, mode, dev);
+}
+#endif
-- 
2.32.0 (Apple Git-132)

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

[Will Cohen]

From: Keno Fischer <keno@juliacomputing.com>

To allow VirtFS on darwin, we need to check that pthread_fchdir_np is
available, which has only been available since macOS 10.12.

Additionally, virtfs_proxy_helper is disabled on Darwin. This patch
series does not currently provide an implementation of the proxy-helper,
but this functionality could be implemented later on.

Signed-off-by: Keno Fischer <keno@juliacomputing.com>
[Michael Roitzsch: - Rebase for NixOS]
Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
[Will Cohen: - Rebase to master]
Signed-off-by: Will Cohen <wwcohen@gmail.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
[Will Cohen: - Add check for pthread_fchdir_np to virtfs
             - Add comments to patch commit
             - Note that virtfs_proxy_helper does not work
               on macOS
             - Adjust meson virtfs error note to specify macOS]
Signed-off-by: Will Cohen <wwcohen@gmail.com>
---
 fsdev/meson.build |  1 +
 meson.build       | 14 ++++++++++----
 2 files changed, 11 insertions(+), 4 deletions(-)

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

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

[Will Cohen]

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

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

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

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

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

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

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

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

[Christian Schoenebeck]

On Montag, 7. Februar 2022 23:40:22 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 includes a function
> that sets the cwd for the current thread only.
> This should suffice to use mknod safely.
> 
> This function (pthread_fchdir_np) is protected by a check in
> meson in a patch later in tihs series.
> 
> Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
> [Will Cohen: - Adjust coding style
>              - Replace clang references with gcc
>              - Note radar filed with Apple for missing syscall
>              - Replace direct syscall with pthread_fchdir_np and
>                adjust patch notes accordingly
>              - Move qemu_mknodat from 9p-util to osdep and os-posix]
> Signed-off-by: Will Cohen <wwcohen@gmail.com>
> ---

Like already mentioned by me moments ago on previous v4 (just echoing) ...

>  hw/9pfs/9p-local.c   |  4 ++--
>  include/qemu/osdep.h | 10 ++++++++++
>  os-posix.c           | 34 ++++++++++++++++++++++++++++++++++
>  3 files changed, 46 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index a0d08e5216..d42ce6d8b8 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -682,7 +682,7 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath
> *dir_path,
> 
>      if (fs_ctx->export_flags & V9FS_SM_MAPPED ||
>          fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
> -        err = mknodat(dirfd, name, fs_ctx->fmode | S_IFREG, 0);
> +        err = qemu_mknodat(dirfd, name, fs_ctx->fmode | S_IFREG, 0);
>          if (err == -1) {
>              goto out;
>          }
> @@ -697,7 +697,7 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath
> *dir_path, }
>      } else if (fs_ctx->export_flags & V9FS_SM_PASSTHROUGH ||
>                 fs_ctx->export_flags & V9FS_SM_NONE) {
> -        err = mknodat(dirfd, name, credp->fc_mode, credp->fc_rdev);
> +        err = qemu_mknodat(dirfd, name, credp->fc_mode, credp->fc_rdev);
>          if (err == -1) {
>              goto out;
>          }
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index d1660d67fa..f3a8367ece 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -810,3 +810,13 @@ static inline int
> platform_does_not_support_system(const char *command) #endif
> 
>  #endif
> +
> +/*
> + * As long as mknodat is not available on macOS, this workaround
> + * using pthread_fchdir_np is needed. qemu_mknodat is defined in
> + * os-posix.c
> + */
> +#ifdef CONFIG_DARWIN
> +int pthread_fchdir_np(int fd);
> +#endif

I would make that:

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

here and ...

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

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

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

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

[Christian Schoenebeck]

On Montag, 7. Februar 2022 23:40:23 CET Will Cohen wrote:
> From: Keno Fischer <keno@juliacomputing.com>
> 
> To allow VirtFS on darwin, we need to check that pthread_fchdir_np is
> available, which has only been available since macOS 10.12.
> 
> Additionally, virtfs_proxy_helper is disabled on Darwin. This patch
> series does not currently provide an implementation of the proxy-helper,
> but this functionality could be implemented later on.
> 
> Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> [Michael Roitzsch: - Rebase for NixOS]
> Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
> [Will Cohen: - Rebase to master]
> Signed-off-by: Will Cohen <wwcohen@gmail.com>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> [Will Cohen: - Add check for pthread_fchdir_np to virtfs
>              - Add comments to patch commit
>              - Note that virtfs_proxy_helper does not work
>                on macOS
>              - Adjust meson virtfs error note to specify macOS]
> Signed-off-by: Will Cohen <wwcohen@gmail.com>
> ---
>  fsdev/meson.build |  1 +
>  meson.build       | 14 ++++++++++----
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/fsdev/meson.build b/fsdev/meson.build
> index adf57cc43e..b632b66348 100644
> --- a/fsdev/meson.build
> +++ b/fsdev/meson.build
> @@ -7,6 +7,7 @@ fsdev_ss.add(when: ['CONFIG_FSDEV_9P'], if_true: files(
>    'qemu-fsdev.c',
>  ), if_false: files('qemu-fsdev-dummy.c'))
>  softmmu_ss.add_all(when: 'CONFIG_LINUX', if_true: fsdev_ss)
> +softmmu_ss.add_all(when: 'CONFIG_DARWIN', if_true: fsdev_ss)
> 
>  if have_virtfs_proxy_helper
>    executable('virtfs-proxy-helper',
> diff --git a/meson.build b/meson.build
> index 5f43355071..c1d13209ff 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1421,17 +1421,23 @@ if not get_option('dbus_display').disabled()
>    endif
>  endif
> 
> -have_virtfs = (targetos == 'linux' and
> +if targetos == 'darwin' and cc.has_function('pthread_fchdir_np')
> +  have_virtfs = have_system
> +else
> +  have_virtfs = (targetos == 'linux' and
>      have_system and
>      libattr.found() and
>      libcap_ng.found())
> +endif
> 
> -have_virtfs_proxy_helper = have_virtfs and have_tools
> +have_virtfs_proxy_helper = targetos != 'darwin' 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 macOS')
> +    elif targetos == 'darwin' and not cc.has_function('pthread_fchdir_np')
> +      error('virtio-9p (virtfs) on Darwin requires the presence of pthread_fchdir_np')

Maybe call this "macOS" in this error message as well?

    error('virtio-9p (virtfs) requires the presence of pthread_fchdir_np on macOS')

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

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

[Will Cohen]

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

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

> On Montag, 7. Februar 2022 23:40:23 CET Will Cohen wrote:
> > From: Keno Fischer <keno@juliacomputing.com>
> >
> > To allow VirtFS on darwin, we need to check that pthread_fchdir_np is
> > available, which has only been available since macOS 10.12.
> >
> > Additionally, virtfs_proxy_helper is disabled on Darwin. This patch
> > series does not currently provide an implementation of the proxy-helper,
> > but this functionality could be implemented later on.
> >
> > Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> > [Michael Roitzsch: - Rebase for NixOS]
> > Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
> > [Will Cohen: - Rebase to master]
> > Signed-off-by: Will Cohen <wwcohen@gmail.com>
> > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> > [Will Cohen: - Add check for pthread_fchdir_np to virtfs
> >              - Add comments to patch commit
> >              - Note that virtfs_proxy_helper does not work
> >                on macOS
> >              - Adjust meson virtfs error note to specify macOS]
> > Signed-off-by: Will Cohen <wwcohen@gmail.com>
> > ---
> >  fsdev/meson.build |  1 +
> >  meson.build       | 14 ++++++++++----
> >  2 files changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/fsdev/meson.build b/fsdev/meson.build
> > index adf57cc43e..b632b66348 100644
> > --- a/fsdev/meson.build
> > +++ b/fsdev/meson.build
> > @@ -7,6 +7,7 @@ fsdev_ss.add(when: ['CONFIG_FSDEV_9P'], if_true: files(
> >    'qemu-fsdev.c',
> >  ), if_false: files('qemu-fsdev-dummy.c'))
> >  softmmu_ss.add_all(when: 'CONFIG_LINUX', if_true: fsdev_ss)
> > +softmmu_ss.add_all(when: 'CONFIG_DARWIN', if_true: fsdev_ss)
> >
> >  if have_virtfs_proxy_helper
> >    executable('virtfs-proxy-helper',
> > diff --git a/meson.build b/meson.build
> > index 5f43355071..c1d13209ff 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -1421,17 +1421,23 @@ if not get_option('dbus_display').disabled()
> >    endif
> >  endif
> >
> > -have_virtfs = (targetos == 'linux' and
> > +if targetos == 'darwin' and cc.has_function('pthread_fchdir_np')
> > +  have_virtfs = have_system
> > +else
> > +  have_virtfs = (targetos == 'linux' and
> >      have_system and
> >      libattr.found() and
> >      libcap_ng.found())
> > +endif
> >
> > -have_virtfs_proxy_helper = have_virtfs and have_tools
> > +have_virtfs_proxy_helper = targetos != 'darwin' 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 macOS')
> > +    elif targetos == 'darwin' and not
> cc.has_function('pthread_fchdir_np')
> > +      error('virtio-9p (virtfs) on Darwin requires the presence of
> pthread_fchdir_np')
>
> Maybe call this "macOS" in this error message as well?
>
>     error('virtio-9p (virtfs) requires the presence of pthread_fchdir_np
> on macOS')


Agreed — shouldn’t have omitted.

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

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

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

[Greg Kurz]

On Mon,  7 Feb 2022 17:40:24 -0500
Will Cohen <wwcohen@gmail.com> wrote:

> The previous test depended on the assumption that P9_DOTL_AT_REMOVEDIR
> and AT_REMOVEDIR have the same value.
> 
> While this is true on Linux, it is not true everywhere, and leads to an
> incorrect test failure on unlink_at, noticed when adding 9p to darwin:
> 
> Received response 7 (RLERROR) instead of 77 (RUNLINKAT)
> Rlerror has errno 22 (Invalid argument)
> **
> 
> ERROR:../tests/qtest/virtio-9p-test.c:305:v9fs_req_recv: assertion
> failed (hdr.id == id): (7 == 77) Bail out!
> 
> ERROR:../tests/qtest/virtio-9p-test.c:305:v9fs_req_recv: assertion
> failed (hdr.id == id): (7 == 77)
> 
> Signed-off-by: Fabian Franz <fabianfranz.oss@gmail.com>
> [Will Cohen: - Add explanation of patch and description
>                of pre-patch test failure]
> Signed-off-by: Will Cohen <wwcohen@gmail.com>
> Acked-by: Thomas Huth <thuth@redhat.com>
> ---

LGTM but this patch should go before patch 10 that enables
Darwin host support to avoid qtest breakage while bisecting.

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

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

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

[Philippe Mathieu-Daudé via]

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

> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index d1660d67fa..f3a8367ece 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -810,3 +810,13 @@ static inline int platform_does_not_support_system(const char *command)
>   #endif
>   
>   #endif
> +
> +/*
> + * As long as mknodat is not available on macOS, this workaround
> + * using pthread_fchdir_np is needed. qemu_mknodat is defined in
> + * os-posix.c
> + */
> +#ifdef CONFIG_DARWIN
> +int pthread_fchdir_np(int fd);
> +#endif
> +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev);

Misplaced. You want the declaration before the __cplusplus guard.

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

[Christian Schoenebeck]

On Montag, 7. Februar 2022 23:40:19 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>
> 
> Because XATTR_SIZE_MAX is not defined on Darwin,
> create a cross-platform P9_XATTR_SIZE_MAX instead.
> 
> [Will Cohen: - Adjust coding style
>              - Lower XATTR_SIZE_MAX to 64k
>              - Add explanatory context related to XATTR_SIZE_MAX]
> [Fabian Franz: - Move XATTR_SIZE_MAX reference from 9p.c to
>                  P9_XATTR_SIZE_MAX in 9p.h]
> Signed-off-by: Will Cohen <wwcohen@gmail.com>
> Signed-off-by: Fabian Franz <fabianfranz.oss@gmail.com>
> ---
>  hw/9pfs/9p.c |  2 +-
>  hw/9pfs/9p.h | 11 +++++++++++
>  2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 14e84c3bcf..7405352c37 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -3949,7 +3949,7 @@ static void coroutine_fn v9fs_xattrcreate(void
> *opaque) rflags |= XATTR_REPLACE;
>      }
> 
> -    if (size > XATTR_SIZE_MAX) {
> +    if (size > P9_XATTR_SIZE_MAX) {
>          err = -E2BIG;
>          goto out_nofid;
>      }
> diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
> index 1567b67841..6a1856b4dc 100644
> --- a/hw/9pfs/9p.h
> +++ b/hw/9pfs/9p.h
> @@ -479,4 +479,15 @@ struct V9fsTransport {
>      void        (*push_and_notify)(V9fsPDU *pdu);
>  };
> 
> +/*
> + * Darwin doesn't seem to define a maximum xattr size in its user
> + * space header, so manually configure it across platforms as 64k.
> + *
> + * Having no limit at all can lead to QEMU crashing during large g_malloc()
> + * calls. Because QEMU does not currently support macOS guests, the below
> + * preliminary solution only works due to its being a reflection of the limit of
> + * Linux guests.
> + */
> +#define P9_XATTR_SIZE_MAX 65536

It would be cleaner in a way like this:

#if defined(XATTR_SIZE_MAX)
/* Linux */
#define P9_XATTR_SIZE_MAX XATTR_SIZE_MAX
#elif defined(CONFIG_DARWIN)
/* darwin comment goes here */
#define P9_XATTR_SIZE_MAX 65536
#else
#error Missing definition for P9_XATTR_SIZE_MAX for this host system
#endif

Sorry, I haven't noticed that before. You actually had that wrapped into some
ifdefs in v2 before:

#if defined(CONFIG_DARWIN) && !defined(XATTR_SIZE_MAX)
...
#endif

> +
>  #endif

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

[Will Cohen]

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

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

> On Montag, 7. Februar 2022 23:40:22 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 includes a function
> > that sets the cwd for the current thread only.
> > This should suffice to use mknod safely.
> >
> > This function (pthread_fchdir_np) is protected by a check in
> > meson in a patch later in tihs series.
> >
> > Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> > Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
> > [Will Cohen: - Adjust coding style
> >              - Replace clang references with gcc
> >              - Note radar filed with Apple for missing syscall
> >              - Replace direct syscall with pthread_fchdir_np and
> >                adjust patch notes accordingly
> >              - Move qemu_mknodat from 9p-util to osdep and os-posix]
> > Signed-off-by: Will Cohen <wwcohen@gmail.com>
> > ---
>
> Like already mentioned by me moments ago on previous v4 (just echoing) ...
>
> >  hw/9pfs/9p-local.c   |  4 ++--
> >  include/qemu/osdep.h | 10 ++++++++++
> >  os-posix.c           | 34 ++++++++++++++++++++++++++++++++++
> >  3 files changed, 46 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> > index a0d08e5216..d42ce6d8b8 100644
> > --- a/hw/9pfs/9p-local.c
> > +++ b/hw/9pfs/9p-local.c
> > @@ -682,7 +682,7 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath
> > *dir_path,
> >
> >      if (fs_ctx->export_flags & V9FS_SM_MAPPED ||
> >          fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
> > -        err = mknodat(dirfd, name, fs_ctx->fmode | S_IFREG, 0);
> > +        err = qemu_mknodat(dirfd, name, fs_ctx->fmode | S_IFREG, 0);
> >          if (err == -1) {
> >              goto out;
> >          }
> > @@ -697,7 +697,7 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath
> > *dir_path, }
> >      } else if (fs_ctx->export_flags & V9FS_SM_PASSTHROUGH ||
> >                 fs_ctx->export_flags & V9FS_SM_NONE) {
> > -        err = mknodat(dirfd, name, credp->fc_mode, credp->fc_rdev);
> > +        err = qemu_mknodat(dirfd, name, credp->fc_mode, credp->fc_rdev);
> >          if (err == -1) {
> >              goto out;
> >          }
> > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > index d1660d67fa..f3a8367ece 100644
> > --- a/include/qemu/osdep.h
> > +++ b/include/qemu/osdep.h
> > @@ -810,3 +810,13 @@ static inline int
> > platform_does_not_support_system(const char *command) #endif
> >
> >  #endif
> > +
> > +/*
> > + * As long as mknodat is not available on macOS, this workaround
> > + * using pthread_fchdir_np is needed. qemu_mknodat is defined in
> > + * os-posix.c
> > + */
> > +#ifdef CONFIG_DARWIN
> > +int pthread_fchdir_np(int fd);
> > +#endif
>
> I would make that:
>
> #ifdef CONFIG_DARWIN
> int pthread_fchdir_np(int fd) API_AVAILABLE(macosx(10.12));
> #endif
>
> here and ...
>
> > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t
> dev);
> > diff --git a/os-posix.c b/os-posix.c
> > index ae6c9f2a5e..95c1607065 100644
> > --- a/os-posix.c
> > +++ b/os-posix.c
> > @@ -24,6 +24,7 @@
> >   */
> >
> >  #include "qemu/osdep.h"
> > +#include <os/availability.h>
> >  #include <sys/wait.h>
> >  #include <pwd.h>
> >  #include <grp.h>
> > @@ -332,3 +333,36 @@ int os_mlock(void)
> >      return -ENOSYS;
> >  #endif
> >  }
> > +
> > +/*
> > + * As long as mknodat is not available on macOS, this workaround
> > + * using pthread_fchdir_np is needed.
> > + *
> > + * Radar filed with Apple for implementing mknodat:
> > + * rdar://FB9862426 (https://openradar.appspot.com/FB9862426)
> > + */
> > +#ifdef CONFIG_DARWIN
> > +
> > +int pthread_fchdir_np(int fd) API_AVAILABLE(macosx(10.12));
>
> ... drop the duplicate declaration of pthread_fchdir_np() here.
>

Trying this out, it reminds me that this use of API_AVAILABLE in os-posix.c
relies on the added #include <os/availability.h>.

Leaving the include out leads to:
.../include/qemu/osdep.h:820:31: error: expected function body after
function declarator
int pthread_fchdir_np(int fd) API_AVAILABLE(macosx(10.12));
                              ^
1 error generated.
ninja: build stopped: subcommand failed.
make[1]: *** [run-ninja] Error 1
make: *** [all] Error 2

The admonition against modifying osdep.h's includes too much led me to
steer away from putting it all in there. If there's no issue with adding
+#include <os/availability.h> to osdep.h, then this change makes sense to
me.


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

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

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

[Will Cohen]

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

On Tue, Feb 8, 2022 at 7:20 AM Christian Schoenebeck <qemu_oss@crudebyte.com>
wrote:

> On Montag, 7. Februar 2022 23:40:19 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>
> >
> > Because XATTR_SIZE_MAX is not defined on Darwin,
> > create a cross-platform P9_XATTR_SIZE_MAX instead.
> >
> > [Will Cohen: - Adjust coding style
> >              - Lower XATTR_SIZE_MAX to 64k
> >              - Add explanatory context related to XATTR_SIZE_MAX]
> > [Fabian Franz: - Move XATTR_SIZE_MAX reference from 9p.c to
> >                  P9_XATTR_SIZE_MAX in 9p.h]
> > Signed-off-by: Will Cohen <wwcohen@gmail.com>
> > Signed-off-by: Fabian Franz <fabianfranz.oss@gmail.com>
> > ---
> >  hw/9pfs/9p.c |  2 +-
> >  hw/9pfs/9p.h | 11 +++++++++++
> >  2 files changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > index 14e84c3bcf..7405352c37 100644
> > --- a/hw/9pfs/9p.c
> > +++ b/hw/9pfs/9p.c
> > @@ -3949,7 +3949,7 @@ static void coroutine_fn v9fs_xattrcreate(void
> > *opaque) rflags |= XATTR_REPLACE;
> >      }
> >
> > -    if (size > XATTR_SIZE_MAX) {
> > +    if (size > P9_XATTR_SIZE_MAX) {
> >          err = -E2BIG;
> >          goto out_nofid;
> >      }
> > diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
> > index 1567b67841..6a1856b4dc 100644
> > --- a/hw/9pfs/9p.h
> > +++ b/hw/9pfs/9p.h
> > @@ -479,4 +479,15 @@ struct V9fsTransport {
> >      void        (*push_and_notify)(V9fsPDU *pdu);
> >  };
> >
> > +/*
> > + * Darwin doesn't seem to define a maximum xattr size in its user
> > + * space header, so manually configure it across platforms as 64k.
> > + *
> > + * Having no limit at all can lead to QEMU crashing during large
> g_malloc()
> > + * calls. Because QEMU does not currently support macOS guests, the
> below
> > + * preliminary solution only works due to its being a reflection of the
> limit of
> > + * Linux guests.
> > + */
> > +#define P9_XATTR_SIZE_MAX 65536
>
> It would be cleaner in a way like this:
>
> #if defined(XATTR_SIZE_MAX)
> /* Linux */
> #define P9_XATTR_SIZE_MAX XATTR_SIZE_MAX
> #elif defined(CONFIG_DARWIN)
> /* darwin comment goes here */
> #define P9_XATTR_SIZE_MAX 65536
> #else
> #error Missing definition for P9_XATTR_SIZE_MAX for this host system
> #endif
>
> Sorry, I haven't noticed that before. You actually had that wrapped into
> some
> ifdefs in v2 before:
>
> #if defined(CONFIG_DARWIN) && !defined(XATTR_SIZE_MAX)
> ...
> #endif
>
> > +
> >  #endif
>
> Agreed, that is cleaner. Adjusting for the next round.

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

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

[Christian Schoenebeck]

On Dienstag, 8. Februar 2022 14:36:42 CET Will Cohen wrote:
> On Mon, Feb 7, 2022 at 5:56 PM Christian Schoenebeck
> <qemu_oss@crudebyte.com>
> wrote:
> > On Montag, 7. Februar 2022 23:40:22 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 includes a function
> > > that sets the cwd for the current thread only.
> > > This should suffice to use mknod safely.
> > > 
> > > This function (pthread_fchdir_np) is protected by a check in
> > > meson in a patch later in tihs series.
> > > 
> > > Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> > > Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
> > > [Will Cohen: - Adjust coding style
> > > 
> > >              - Replace clang references with gcc
> > >              - Note radar filed with Apple for missing syscall
> > >              - Replace direct syscall with pthread_fchdir_np and
> > >              
> > >                adjust patch notes accordingly
> > >              
> > >              - Move qemu_mknodat from 9p-util to osdep and os-posix]
> > > 
> > > Signed-off-by: Will Cohen <wwcohen@gmail.com>
> > > ---
> > 
> > Like already mentioned by me moments ago on previous v4 (just echoing) ...
> > 
> > >  hw/9pfs/9p-local.c   |  4 ++--
> > >  include/qemu/osdep.h | 10 ++++++++++
> > >  os-posix.c           | 34 ++++++++++++++++++++++++++++++++++
> > >  3 files changed, 46 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> > > index a0d08e5216..d42ce6d8b8 100644
> > > --- a/hw/9pfs/9p-local.c
> > > +++ b/hw/9pfs/9p-local.c
> > > @@ -682,7 +682,7 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath
> > > *dir_path,
> > > 
> > >      if (fs_ctx->export_flags & V9FS_SM_MAPPED ||
> > >      
> > >          fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
> > > 
> > > -        err = mknodat(dirfd, name, fs_ctx->fmode | S_IFREG, 0);
> > > +        err = qemu_mknodat(dirfd, name, fs_ctx->fmode | S_IFREG, 0);
> > > 
> > >          if (err == -1) {
> > >          
> > >              goto out;
> > >          
> > >          }
> > > 
> > > @@ -697,7 +697,7 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath
> > > *dir_path, }
> > > 
> > >      } else if (fs_ctx->export_flags & V9FS_SM_PASSTHROUGH ||
> > >      
> > >                 fs_ctx->export_flags & V9FS_SM_NONE) {
> > > 
> > > -        err = mknodat(dirfd, name, credp->fc_mode, credp->fc_rdev);
> > > +        err = qemu_mknodat(dirfd, name, credp->fc_mode,
> > > credp->fc_rdev);
> > > 
> > >          if (err == -1) {
> > >          
> > >              goto out;
> > >          
> > >          }
> > > 
> > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > > index d1660d67fa..f3a8367ece 100644
> > > --- a/include/qemu/osdep.h
> > > +++ b/include/qemu/osdep.h
> > > @@ -810,3 +810,13 @@ static inline int
> > > platform_does_not_support_system(const char *command) #endif
> > > 
> > >  #endif
> > > 
> > > +
> > > +/*
> > > + * As long as mknodat is not available on macOS, this workaround
> > > + * using pthread_fchdir_np is needed. qemu_mknodat is defined in
> > > + * os-posix.c
> > > + */
> > > +#ifdef CONFIG_DARWIN
> > > +int pthread_fchdir_np(int fd);
> > > +#endif
> > 
> > I would make that:
> > 
> > #ifdef CONFIG_DARWIN
> > int pthread_fchdir_np(int fd) API_AVAILABLE(macosx(10.12));
> > #endif
> > 
> > here and ...
> > 
> > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t
> > 
> > dev);
> > 
> > > diff --git a/os-posix.c b/os-posix.c
> > > index ae6c9f2a5e..95c1607065 100644
> > > --- a/os-posix.c
> > > +++ b/os-posix.c
> > > @@ -24,6 +24,7 @@
> > > 
> > >   */
> > >  
> > >  #include "qemu/osdep.h"
> > > 
> > > +#include <os/availability.h>
> > > 
> > >  #include <sys/wait.h>
> > >  #include <pwd.h>
> > >  #include <grp.h>
> > > 
> > > @@ -332,3 +333,36 @@ int os_mlock(void)
> > > 
> > >      return -ENOSYS;
> > >  
> > >  #endif
> > >  }
> > > 
> > > +
> > > +/*
> > > + * As long as mknodat is not available on macOS, this workaround
> > > + * using pthread_fchdir_np is needed.
> > > + *
> > > + * Radar filed with Apple for implementing mknodat:
> > > + * rdar://FB9862426 (https://openradar.appspot.com/FB9862426)
> > > + */
> > > +#ifdef CONFIG_DARWIN
> > > +
> > > +int pthread_fchdir_np(int fd) API_AVAILABLE(macosx(10.12));
> > 
> > ... drop the duplicate declaration of pthread_fchdir_np() here.
> 
> Trying this out, it reminds me that this use of API_AVAILABLE in os-posix.c
> relies on the added #include <os/availability.h>.
> 
> Leaving the include out leads to:
> .../include/qemu/osdep.h:820:31: error: expected function body after
> function declarator
> int pthread_fchdir_np(int fd) API_AVAILABLE(macosx(10.12));
>                               ^
> 1 error generated.
> ninja: build stopped: subcommand failed.
> make[1]: *** [run-ninja] Error 1
> make: *** [all] Error 2
> 
> The admonition against modifying osdep.h's includes too much led me to
> steer away from putting it all in there. If there's no issue with adding
> +#include <os/availability.h> to osdep.h, then this change makes sense to
> me.

If you embed that include into ifdefs, sure!

#ifdef CONFIG_DARWIN
/* defines API_AVAILABLE(...) */
#include <os/availability.h>
#endif

One more thing though ...

> > > +
> > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t
> > 
> > dev)
> > 
> > > +{
> > > +    int preserved_errno, err;

pthread_fchdir_np() is weakly linked. So I guess here should be a check like:

	if (!pthread_fchdir_np) {
		return -ENOTSUPP;
	}

Before trying to call pthread_fchdir_np() below. As already discussed with the
Chromium [1] example, some do that a bit differently by using
__builtin_available():

	if (__builtin_available(macOS 10.12, *)) {
		return -ENOTSUPP;
	}

Which makes me wonder why they are not doing a simple NULL check?

[1] https://chromium.googlesource.com/chromium/src/+/lkgr/base/process/launch_mac.cc#110

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

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

[Will Cohen]

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

My inclination is to go with the __builtin_available(macOS 10.12, *) path,
if acceptable, since it partially mirrors the API_AVAILABLE macro idea. I
guess it's perhaps a tradeoff between predicting the future unknown
availability of functions versus just ensuring a minimum macOS version and
hoping for the best. With any luck, the distinction between the two
approaches will be moot, if we try to assume that a future macOS version
that removes this also provides mknodat.

On Tue, Feb 8, 2022 at 10:03 AM Christian Schoenebeck <
qemu_oss@crudebyte.com> wrote:

> On Dienstag, 8. Februar 2022 14:36:42 CET Will Cohen wrote:
> > On Mon, Feb 7, 2022 at 5:56 PM Christian Schoenebeck
> > <qemu_oss@crudebyte.com>
> > wrote:
> > > On Montag, 7. Februar 2022 23:40:22 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 includes a function
> > > > that sets the cwd for the current thread only.
> > > > This should suffice to use mknod safely.
> > > >
> > > > This function (pthread_fchdir_np) is protected by a check in
> > > > meson in a patch later in tihs series.
> > > >
> > > > Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> > > > Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
> > > > [Will Cohen: - Adjust coding style
> > > >
> > > >              - Replace clang references with gcc
> > > >              - Note radar filed with Apple for missing syscall
> > > >              - Replace direct syscall with pthread_fchdir_np and
> > > >
> > > >                adjust patch notes accordingly
> > > >
> > > >              - Move qemu_mknodat from 9p-util to osdep and os-posix]
> > > >
> > > > Signed-off-by: Will Cohen <wwcohen@gmail.com>
> > > > ---
> > >
> > > Like already mentioned by me moments ago on previous v4 (just echoing)
> ...
> > >
> > > >  hw/9pfs/9p-local.c   |  4 ++--
> > > >  include/qemu/osdep.h | 10 ++++++++++
> > > >  os-posix.c           | 34 ++++++++++++++++++++++++++++++++++
> > > >  3 files changed, 46 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> > > > index a0d08e5216..d42ce6d8b8 100644
> > > > --- a/hw/9pfs/9p-local.c
> > > > +++ b/hw/9pfs/9p-local.c
> > > > @@ -682,7 +682,7 @@ static int local_mknod(FsContext *fs_ctx,
> V9fsPath
> > > > *dir_path,
> > > >
> > > >      if (fs_ctx->export_flags & V9FS_SM_MAPPED ||
> > > >
> > > >          fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
> > > >
> > > > -        err = mknodat(dirfd, name, fs_ctx->fmode | S_IFREG, 0);
> > > > +        err = qemu_mknodat(dirfd, name, fs_ctx->fmode | S_IFREG, 0);
> > > >
> > > >          if (err == -1) {
> > > >
> > > >              goto out;
> > > >
> > > >          }
> > > >
> > > > @@ -697,7 +697,7 @@ static int local_mknod(FsContext *fs_ctx,
> V9fsPath
> > > > *dir_path, }
> > > >
> > > >      } else if (fs_ctx->export_flags & V9FS_SM_PASSTHROUGH ||
> > > >
> > > >                 fs_ctx->export_flags & V9FS_SM_NONE) {
> > > >
> > > > -        err = mknodat(dirfd, name, credp->fc_mode, credp->fc_rdev);
> > > > +        err = qemu_mknodat(dirfd, name, credp->fc_mode,
> > > > credp->fc_rdev);
> > > >
> > > >          if (err == -1) {
> > > >
> > > >              goto out;
> > > >
> > > >          }
> > > >
> > > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > > > index d1660d67fa..f3a8367ece 100644
> > > > --- a/include/qemu/osdep.h
> > > > +++ b/include/qemu/osdep.h
> > > > @@ -810,3 +810,13 @@ static inline int
> > > > platform_does_not_support_system(const char *command) #endif
> > > >
> > > >  #endif
> > > >
> > > > +
> > > > +/*
> > > > + * As long as mknodat is not available on macOS, this workaround
> > > > + * using pthread_fchdir_np is needed. qemu_mknodat is defined in
> > > > + * os-posix.c
> > > > + */
> > > > +#ifdef CONFIG_DARWIN
> > > > +int pthread_fchdir_np(int fd);
> > > > +#endif
> > >
> > > I would make that:
> > >
> > > #ifdef CONFIG_DARWIN
> > > int pthread_fchdir_np(int fd) API_AVAILABLE(macosx(10.12));
> > > #endif
> > >
> > > here and ...
> > >
> > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t
> > >
> > > dev);
> > >
> > > > diff --git a/os-posix.c b/os-posix.c
> > > > index ae6c9f2a5e..95c1607065 100644
> > > > --- a/os-posix.c
> > > > +++ b/os-posix.c
> > > > @@ -24,6 +24,7 @@
> > > >
> > > >   */
> > > >
> > > >  #include "qemu/osdep.h"
> > > >
> > > > +#include <os/availability.h>
> > > >
> > > >  #include <sys/wait.h>
> > > >  #include <pwd.h>
> > > >  #include <grp.h>
> > > >
> > > > @@ -332,3 +333,36 @@ int os_mlock(void)
> > > >
> > > >      return -ENOSYS;
> > > >
> > > >  #endif
> > > >  }
> > > >
> > > > +
> > > > +/*
> > > > + * As long as mknodat is not available on macOS, this workaround
> > > > + * using pthread_fchdir_np is needed.
> > > > + *
> > > > + * Radar filed with Apple for implementing mknodat:
> > > > + * rdar://FB9862426 (https://openradar.appspot.com/FB9862426)
> > > > + */
> > > > +#ifdef CONFIG_DARWIN
> > > > +
> > > > +int pthread_fchdir_np(int fd) API_AVAILABLE(macosx(10.12));
> > >
> > > ... drop the duplicate declaration of pthread_fchdir_np() here.
> >
> > Trying this out, it reminds me that this use of API_AVAILABLE in
> os-posix.c
> > relies on the added #include <os/availability.h>.
> >
> > Leaving the include out leads to:
> > .../include/qemu/osdep.h:820:31: error: expected function body after
> > function declarator
> > int pthread_fchdir_np(int fd) API_AVAILABLE(macosx(10.12));
> >                               ^
> > 1 error generated.
> > ninja: build stopped: subcommand failed.
> > make[1]: *** [run-ninja] Error 1
> > make: *** [all] Error 2
> >
> > The admonition against modifying osdep.h's includes too much led me to
> > steer away from putting it all in there. If there's no issue with adding
> > +#include <os/availability.h> to osdep.h, then this change makes sense to
> > me.
>
> If you embed that include into ifdefs, sure!
>
> #ifdef CONFIG_DARWIN
> /* defines API_AVAILABLE(...) */
> #include <os/availability.h>
> #endif
>
> One more thing though ...
>
> > > > +
> > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t
> > >
> > > dev)
> > >
> > > > +{
> > > > +    int preserved_errno, err;
>
> pthread_fchdir_np() is weakly linked. So I guess here should be a check
> like:
>
>         if (!pthread_fchdir_np) {
>                 return -ENOTSUPP;
>         }
>
> Before trying to call pthread_fchdir_np() below. As already discussed with
> the
> Chromium [1] example, some do that a bit differently by using
> __builtin_available():
>
>         if (__builtin_available(macOS 10.12, *)) {
>                 return -ENOTSUPP;
>         }
>
> Which makes me wonder why they are not doing a simple NULL check?
>
> [1]
> https://chromium.googlesource.com/chromium/src/+/lkgr/base/process/launch_mac.cc#110
>
> > > > +    if (pthread_fchdir_np(dirfd) < 0) {
> > > > +        return -1;
> > > > +    }
> > > > +    err = mknod(filename, mode, dev);
> > > > +    preserved_errno = errno;
> > > > +    /* Stop using the thread-local cwd */
> > > > +    pthread_fchdir_np(-1);
> > > > +    if (err < 0) {
> > > > +        errno = preserved_errno;
> > > > +    }
> > > > +    return err;
> > > > +}
> > > > +#else
> > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t
> > >
> > > dev)
> > >
> > > > +{
> > > > +    return mknodat(dirfd, filename, mode, dev);
> > > > +}
> > > > +#endif
>
>
>

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

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

[Christian Schoenebeck]

On Dienstag, 8. Februar 2022 16:57:55 CET Will Cohen wrote:
> My inclination is to go with the __builtin_available(macOS 10.12, *) path,
> if acceptable, since it partially mirrors the API_AVAILABLE macro idea. I

OTOH that's duplication of the ">= macOS 10.12" info, plus __builtin_available
is direct use of a clang-only extension, whereas API_AVAILABLE() works (or
more precisely: doesn't error out at least) with other compilers like GCC as
well. GCC is sometimes used for cross-compilation.

Moreover, I would also add an error message in this case, e.g.:

    if (!pthread_fchdir_np) {
        error_report_once("pthread_fchdir_np() is not available on this macOS version");
        return -ENOTSUPP;	
    }

I should elaborate why I think this is needed: you are already doing a Meson
check for the existence of pthread_fchdir_np(), but the system where QEMU is
compiled and the systems where the compiled binary will be running, might be
different ones (i.e. different macOS versions).

Best regards,
Christian Schoenebeck

> guess it's perhaps a tradeoff between predicting the future unknown
> availability of functions versus just ensuring a minimum macOS version and
> hoping for the best. With any luck, the distinction between the two
> approaches will be moot, if we try to assume that a future macOS version
> that removes this also provides mknodat.
> 
> On Tue, Feb 8, 2022 at 10:03 AM Christian Schoenebeck <
> 
> qemu_oss@crudebyte.com> wrote:
> > On Dienstag, 8. Februar 2022 14:36:42 CET Will Cohen wrote:
> > > On Mon, Feb 7, 2022 at 5:56 PM Christian Schoenebeck
> > > <qemu_oss@crudebyte.com>
> > > 
> > > wrote:
> > > > On Montag, 7. Februar 2022 23:40:22 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 includes a function
> > > > > that sets the cwd for the current thread only.
> > > > > This should suffice to use mknod safely.
> > > > > 
> > > > > This function (pthread_fchdir_np) is protected by a check in
> > > > > meson in a patch later in tihs series.
> > > > > 
> > > > > Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> > > > > Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
> > > > > [Will Cohen: - Adjust coding style
> > > > > 
> > > > >              - Replace clang references with gcc
> > > > >              - Note radar filed with Apple for missing syscall
> > > > >              - Replace direct syscall with pthread_fchdir_np and
> > > > >              
> > > > >                adjust patch notes accordingly
> > > > >              
> > > > >              - Move qemu_mknodat from 9p-util to osdep and os-posix]
> > > > > 
> > > > > Signed-off-by: Will Cohen <wwcohen@gmail.com>
> > > > > ---
> > > > 
> > > > Like already mentioned by me moments ago on previous v4 (just echoing)
> > 
> > ...
> > 
> > > > >  hw/9pfs/9p-local.c   |  4 ++--
> > > > >  include/qemu/osdep.h | 10 ++++++++++
> > > > >  os-posix.c           | 34 ++++++++++++++++++++++++++++++++++
> > > > >  3 files changed, 46 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> > > > > index a0d08e5216..d42ce6d8b8 100644
> > > > > --- a/hw/9pfs/9p-local.c
> > > > > +++ b/hw/9pfs/9p-local.c
> > > > > @@ -682,7 +682,7 @@ static int local_mknod(FsContext *fs_ctx,
> > 
> > V9fsPath
> > 
> > > > > *dir_path,
> > > > > 
> > > > >      if (fs_ctx->export_flags & V9FS_SM_MAPPED ||
> > > > >      
> > > > >          fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
> > > > > 
> > > > > -        err = mknodat(dirfd, name, fs_ctx->fmode | S_IFREG, 0);
> > > > > +        err = qemu_mknodat(dirfd, name, fs_ctx->fmode | S_IFREG,
> > > > > 0);
> > > > > 
> > > > >          if (err == -1) {
> > > > >          
> > > > >              goto out;
> > > > >          
> > > > >          }
> > > > > 
> > > > > @@ -697,7 +697,7 @@ static int local_mknod(FsContext *fs_ctx,
> > 
> > V9fsPath
> > 
> > > > > *dir_path, }
> > > > > 
> > > > >      } else if (fs_ctx->export_flags & V9FS_SM_PASSTHROUGH ||
> > > > >      
> > > > >                 fs_ctx->export_flags & V9FS_SM_NONE) {
> > > > > 
> > > > > -        err = mknodat(dirfd, name, credp->fc_mode, credp->fc_rdev);
> > > > > +        err = qemu_mknodat(dirfd, name, credp->fc_mode,
> > > > > credp->fc_rdev);
> > > > > 
> > > > >          if (err == -1) {
> > > > >          
> > > > >              goto out;
> > > > >          
> > > > >          }
> > > > > 
> > > > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > > > > index d1660d67fa..f3a8367ece 100644
> > > > > --- a/include/qemu/osdep.h
> > > > > +++ b/include/qemu/osdep.h
> > > > > @@ -810,3 +810,13 @@ static inline int
> > > > > platform_does_not_support_system(const char *command) #endif
> > > > > 
> > > > >  #endif
> > > > > 
> > > > > +
> > > > > +/*
> > > > > + * As long as mknodat is not available on macOS, this workaround
> > > > > + * using pthread_fchdir_np is needed. qemu_mknodat is defined in
> > > > > + * os-posix.c
> > > > > + */
> > > > > +#ifdef CONFIG_DARWIN
> > > > > +int pthread_fchdir_np(int fd);
> > > > > +#endif
> > > > 
> > > > I would make that:
> > > > 
> > > > #ifdef CONFIG_DARWIN
> > > > int pthread_fchdir_np(int fd) API_AVAILABLE(macosx(10.12));
> > > > #endif
> > > > 
> > > > here and ...
> > > > 
> > > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode,
> > > > > dev_t
> > > > 
> > > > dev);
> > > > 
> > > > > diff --git a/os-posix.c b/os-posix.c
> > > > > index ae6c9f2a5e..95c1607065 100644
> > > > > --- a/os-posix.c
> > > > > +++ b/os-posix.c
> > > > > @@ -24,6 +24,7 @@
> > > > > 
> > > > >   */
> > > > >  
> > > > >  #include "qemu/osdep.h"
> > > > > 
> > > > > +#include <os/availability.h>
> > > > > 
> > > > >  #include <sys/wait.h>
> > > > >  #include <pwd.h>
> > > > >  #include <grp.h>
> > > > > 
> > > > > @@ -332,3 +333,36 @@ int os_mlock(void)
> > > > > 
> > > > >      return -ENOSYS;
> > > > >  
> > > > >  #endif
> > > > >  }
> > > > > 
> > > > > +
> > > > > +/*
> > > > > + * As long as mknodat is not available on macOS, this workaround
> > > > > + * using pthread_fchdir_np is needed.
> > > > > + *
> > > > > + * Radar filed with Apple for implementing mknodat:
> > > > > + * rdar://FB9862426 (https://openradar.appspot.com/FB9862426)
> > > > > + */
> > > > > +#ifdef CONFIG_DARWIN
> > > > > +
> > > > > +int pthread_fchdir_np(int fd) API_AVAILABLE(macosx(10.12));
> > > > 
> > > > ... drop the duplicate declaration of pthread_fchdir_np() here.
> > > 
> > > Trying this out, it reminds me that this use of API_AVAILABLE in
> > 
> > os-posix.c
> > 
> > > relies on the added #include <os/availability.h>.
> > > 
> > > Leaving the include out leads to:
> > > .../include/qemu/osdep.h:820:31: error: expected function body after
> > > function declarator
> > > int pthread_fchdir_np(int fd) API_AVAILABLE(macosx(10.12));
> > > 
> > >                               ^
> > > 
> > > 1 error generated.
> > > ninja: build stopped: subcommand failed.
> > > make[1]: *** [run-ninja] Error 1
> > > make: *** [all] Error 2
> > > 
> > > The admonition against modifying osdep.h's includes too much led me to
> > > steer away from putting it all in there. If there's no issue with adding
> > > +#include <os/availability.h> to osdep.h, then this change makes sense
> > > to
> > > me.
> > 
> > If you embed that include into ifdefs, sure!
> > 
> > #ifdef CONFIG_DARWIN
> > /* defines API_AVAILABLE(...) */
> > #include <os/availability.h>
> > #endif
> > 
> > One more thing though ...
> > 
> > > > > +
> > > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode,
> > > > > dev_t
> > > > 
> > > > dev)
> > > > 
> > > > > +{
> > > > > +    int preserved_errno, err;
> > 
> > pthread_fchdir_np() is weakly linked. So I guess here should be a check
> > 
> > like:
> >         if (!pthread_fchdir_np) {
> >         
> >                 return -ENOTSUPP;
> >         
> >         }
> > 
> > Before trying to call pthread_fchdir_np() below. As already discussed with
> > the
> > Chromium [1] example, some do that a bit differently by using
> > 
> > __builtin_available():
> >         if (__builtin_available(macOS 10.12, *)) {
> >         
> >                 return -ENOTSUPP;
> >         
> >         }
> > 
> > Which makes me wonder why they are not doing a simple NULL check?
> > 
> > [1]
> > https://chromium.googlesource.com/chromium/src/+/lkgr/base/process/launch_
> > mac.cc#110> 
> > > > > +    if (pthread_fchdir_np(dirfd) < 0) {
> > > > > +        return -1;
> > > > > +    }
> > > > > +    err = mknod(filename, mode, dev);
> > > > > +    preserved_errno = errno;
> > > > > +    /* Stop using the thread-local cwd */
> > > > > +    pthread_fchdir_np(-1);
> > > > > +    if (err < 0) {
> > > > > +        errno = preserved_errno;
> > > > > +    }
> > > > > +    return err;
> > > > > +}
> > > > > +#else
> > > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode,
> > > > > dev_t
> > > > 
> > > > dev)
> > > > 
> > > > > +{
> > > > > +    return mknodat(dirfd, filename, mode, dev);
> > > > > +}
> > > > > +#endif

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

[Will Cohen]

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

On Tue, Feb 8, 2022 at 11:11 AM Christian Schoenebeck <
qemu_oss@crudebyte.com> wrote:

> On Dienstag, 8. Februar 2022 16:57:55 CET Will Cohen wrote:
> > My inclination is to go with the __builtin_available(macOS 10.12, *)
> path,
> > if acceptable, since it partially mirrors the API_AVAILABLE macro idea. I
>
> OTOH that's duplication of the ">= macOS 10.12" info, plus
> __builtin_available
> is direct use of a clang-only extension, whereas API_AVAILABLE() works (or
> more precisely: doesn't error out at least) with other compilers like GCC
> as
> well. GCC is sometimes used for cross-compilation.
>
> Moreover, I would also add an error message in this case, e.g.:
>
>     if (!pthread_fchdir_np) {
>         error_report_once("pthread_fchdir_np() is not available on this
> macOS version");
>         return -ENOTSUPP;
>     }
>
> I should elaborate why I think this is needed: you are already doing a
> Meson
> check for the existence of pthread_fchdir_np(), but the system where QEMU
> is
> compiled and the systems where the compiled binary will be running, might
> be
> different ones (i.e. different macOS versions).
>
> Best regards,
> Christian Schoenebeck
>

Agreed, that way actually closes the edge case. Something along these lines
briefly crossed my mind during a previous version, but I quickly got passed
it by assuming that the compiling entity would always be the bottleneck,
which makes no sense in hindsight, so I very much appreciate that you
caught this.


> > guess it's perhaps a tradeoff between predicting the future unknown
> > availability of functions versus just ensuring a minimum macOS version
> and
> > hoping for the best. With any luck, the distinction between the two
> > approaches will be moot, if we try to assume that a future macOS version
> > that removes this also provides mknodat.
> >
> > On Tue, Feb 8, 2022 at 10:03 AM Christian Schoenebeck <
> >
> > qemu_oss@crudebyte.com> wrote:
> > > On Dienstag, 8. Februar 2022 14:36:42 CET Will Cohen wrote:
> > > > On Mon, Feb 7, 2022 at 5:56 PM Christian Schoenebeck
> > > > <qemu_oss@crudebyte.com>
> > > >
> > > > wrote:
> > > > > On Montag, 7. Februar 2022 23:40:22 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 includes a function
> > > > > > that sets the cwd for the current thread only.
> > > > > > This should suffice to use mknod safely.
> > > > > >
> > > > > > This function (pthread_fchdir_np) is protected by a check in
> > > > > > meson in a patch later in tihs series.
> > > > > >
> > > > > > Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> > > > > > Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
> > > > > > [Will Cohen: - Adjust coding style
> > > > > >
> > > > > >              - Replace clang references with gcc
> > > > > >              - Note radar filed with Apple for missing syscall
> > > > > >              - Replace direct syscall with pthread_fchdir_np and
> > > > > >
> > > > > >                adjust patch notes accordingly
> > > > > >
> > > > > >              - Move qemu_mknodat from 9p-util to osdep and
> os-posix]
> > > > > >
> > > > > > Signed-off-by: Will Cohen <wwcohen@gmail.com>
> > > > > > ---
> > > > >
> > > > > Like already mentioned by me moments ago on previous v4 (just
> echoing)
> > >
> > > ...
> > >
> > > > > >  hw/9pfs/9p-local.c   |  4 ++--
> > > > > >  include/qemu/osdep.h | 10 ++++++++++
> > > > > >  os-posix.c           | 34 ++++++++++++++++++++++++++++++++++
> > > > > >  3 files changed, 46 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> > > > > > index a0d08e5216..d42ce6d8b8 100644
> > > > > > --- a/hw/9pfs/9p-local.c
> > > > > > +++ b/hw/9pfs/9p-local.c
> > > > > > @@ -682,7 +682,7 @@ static int local_mknod(FsContext *fs_ctx,
> > >
> > > V9fsPath
> > >
> > > > > > *dir_path,
> > > > > >
> > > > > >      if (fs_ctx->export_flags & V9FS_SM_MAPPED ||
> > > > > >
> > > > > >          fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
> > > > > >
> > > > > > -        err = mknodat(dirfd, name, fs_ctx->fmode | S_IFREG, 0);
> > > > > > +        err = qemu_mknodat(dirfd, name, fs_ctx->fmode | S_IFREG,
> > > > > > 0);
> > > > > >
> > > > > >          if (err == -1) {
> > > > > >
> > > > > >              goto out;
> > > > > >
> > > > > >          }
> > > > > >
> > > > > > @@ -697,7 +697,7 @@ static int local_mknod(FsContext *fs_ctx,
> > >
> > > V9fsPath
> > >
> > > > > > *dir_path, }
> > > > > >
> > > > > >      } else if (fs_ctx->export_flags & V9FS_SM_PASSTHROUGH ||
> > > > > >
> > > > > >                 fs_ctx->export_flags & V9FS_SM_NONE) {
> > > > > >
> > > > > > -        err = mknodat(dirfd, name, credp->fc_mode,
> credp->fc_rdev);
> > > > > > +        err = qemu_mknodat(dirfd, name, credp->fc_mode,
> > > > > > credp->fc_rdev);
> > > > > >
> > > > > >          if (err == -1) {
> > > > > >
> > > > > >              goto out;
> > > > > >
> > > > > >          }
> > > > > >
> > > > > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > > > > > index d1660d67fa..f3a8367ece 100644
> > > > > > --- a/include/qemu/osdep.h
> > > > > > +++ b/include/qemu/osdep.h
> > > > > > @@ -810,3 +810,13 @@ static inline int
> > > > > > platform_does_not_support_system(const char *command) #endif
> > > > > >
> > > > > >  #endif
> > > > > >
> > > > > > +
> > > > > > +/*
> > > > > > + * As long as mknodat is not available on macOS, this workaround
> > > > > > + * using pthread_fchdir_np is needed. qemu_mknodat is defined in
> > > > > > + * os-posix.c
> > > > > > + */
> > > > > > +#ifdef CONFIG_DARWIN
> > > > > > +int pthread_fchdir_np(int fd);
> > > > > > +#endif
> > > > >
> > > > > I would make that:
> > > > >
> > > > > #ifdef CONFIG_DARWIN
> > > > > int pthread_fchdir_np(int fd) API_AVAILABLE(macosx(10.12));
> > > > > #endif
> > > > >
> > > > > here and ...
> > > > >
> > > > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode,
> > > > > > dev_t
> > > > >
> > > > > dev);
> > > > >
> > > > > > diff --git a/os-posix.c b/os-posix.c
> > > > > > index ae6c9f2a5e..95c1607065 100644
> > > > > > --- a/os-posix.c
> > > > > > +++ b/os-posix.c
> > > > > > @@ -24,6 +24,7 @@
> > > > > >
> > > > > >   */
> > > > > >
> > > > > >  #include "qemu/osdep.h"
> > > > > >
> > > > > > +#include <os/availability.h>
> > > > > >
> > > > > >  #include <sys/wait.h>
> > > > > >  #include <pwd.h>
> > > > > >  #include <grp.h>
> > > > > >
> > > > > > @@ -332,3 +333,36 @@ int os_mlock(void)
> > > > > >
> > > > > >      return -ENOSYS;
> > > > > >
> > > > > >  #endif
> > > > > >  }
> > > > > >
> > > > > > +
> > > > > > +/*
> > > > > > + * As long as mknodat is not available on macOS, this workaround
> > > > > > + * using pthread_fchdir_np is needed.
> > > > > > + *
> > > > > > + * Radar filed with Apple for implementing mknodat:
> > > > > > + * rdar://FB9862426 (https://openradar.appspot.com/FB9862426)
> > > > > > + */
> > > > > > +#ifdef CONFIG_DARWIN
> > > > > > +
> > > > > > +int pthread_fchdir_np(int fd) API_AVAILABLE(macosx(10.12));
> > > > >
> > > > > ... drop the duplicate declaration of pthread_fchdir_np() here.
> > > >
> > > > Trying this out, it reminds me that this use of API_AVAILABLE in
> > >
> > > os-posix.c
> > >
> > > > relies on the added #include <os/availability.h>.
> > > >
> > > > Leaving the include out leads to:
> > > > .../include/qemu/osdep.h:820:31: error: expected function body after
> > > > function declarator
> > > > int pthread_fchdir_np(int fd) API_AVAILABLE(macosx(10.12));
> > > >
> > > >                               ^
> > > >
> > > > 1 error generated.
> > > > ninja: build stopped: subcommand failed.
> > > > make[1]: *** [run-ninja] Error 1
> > > > make: *** [all] Error 2
> > > >
> > > > The admonition against modifying osdep.h's includes too much led me
> to
> > > > steer away from putting it all in there. If there's no issue with
> adding
> > > > +#include <os/availability.h> to osdep.h, then this change makes
> sense
> > > > to
> > > > me.
> > >
> > > If you embed that include into ifdefs, sure!
> > >
> > > #ifdef CONFIG_DARWIN
> > > /* defines API_AVAILABLE(...) */
> > > #include <os/availability.h>
> > > #endif
> > >
> > > One more thing though ...
> > >
> > > > > > +
> > > > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode,
> > > > > > dev_t
> > > > >
> > > > > dev)
> > > > >
> > > > > > +{
> > > > > > +    int preserved_errno, err;
> > >
> > > pthread_fchdir_np() is weakly linked. So I guess here should be a check
> > >
> > > like:
> > >         if (!pthread_fchdir_np) {
> > >
> > >                 return -ENOTSUPP;
> > >
> > >         }
> > >
> > > Before trying to call pthread_fchdir_np() below. As already discussed
> with
> > > the
> > > Chromium [1] example, some do that a bit differently by using
> > >
> > > __builtin_available():
> > >         if (__builtin_available(macOS 10.12, *)) {
> > >
> > >                 return -ENOTSUPP;
> > >
> > >         }
> > >
> > > Which makes me wonder why they are not doing a simple NULL check?
> > >
> > > [1]
> > >
> https://chromium.googlesource.com/chromium/src/+/lkgr/base/process/launch_
> > > mac.cc#110>
> > > > > > +    if (pthread_fchdir_np(dirfd) < 0) {
> > > > > > +        return -1;
> > > > > > +    }
> > > > > > +    err = mknod(filename, mode, dev);
> > > > > > +    preserved_errno = errno;
> > > > > > +    /* Stop using the thread-local cwd */
> > > > > > +    pthread_fchdir_np(-1);
> > > > > > +    if (err < 0) {
> > > > > > +        errno = preserved_errno;
> > > > > > +    }
> > > > > > +    return err;
> > > > > > +}
> > > > > > +#else
> > > > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode,
> > > > > > dev_t
> > > > >
> > > > > dev)
> > > > >
> > > > > > +{
> > > > > > +    return mknodat(dirfd, filename, mode, dev);
> > > > > > +}
> > > > > > +#endif
>

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

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

[Will Cohen]

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

On Tue, Feb 8, 2022 at 11:19 AM Will Cohen <wwcohen@gmail.com> wrote:

> On Tue, Feb 8, 2022 at 11:11 AM Christian Schoenebeck <
> qemu_oss@crudebyte.com> wrote:
>
>> On Dienstag, 8. Februar 2022 16:57:55 CET Will Cohen wrote:
>> > My inclination is to go with the __builtin_available(macOS 10.12, *)
>> path,
>> > if acceptable, since it partially mirrors the API_AVAILABLE macro idea.
>> I
>>
>> OTOH that's duplication of the ">= macOS 10.12" info, plus
>> __builtin_available
>> is direct use of a clang-only extension, whereas API_AVAILABLE() works (or
>> more precisely: doesn't error out at least) with other compilers like GCC
>> as
>> well. GCC is sometimes used for cross-compilation.
>>
>> Moreover, I would also add an error message in this case, e.g.:
>>
>>     if (!pthread_fchdir_np) {
>>         error_report_once("pthread_fchdir_np() is not available on this
>> macOS version");
>>         return -ENOTSUPP;
>>     }
>>
>> I should elaborate why I think this is needed: you are already doing a
>> Meson
>> check for the existence of pthread_fchdir_np(), but the system where QEMU
>> is
>> compiled and the systems where the compiled binary will be running, might
>> be
>> different ones (i.e. different macOS versions).
>>
>> Best regards,
>> Christian Schoenebeck
>>
>
> Agreed, that way actually closes the edge case. Something along these
> lines briefly crossed my mind during a previous version, but I quickly got
> passed it by assuming that the compiling entity would always be the
> bottleneck, which makes no sense in hindsight, so I very much appreciate
> that you caught this.
>

Ah, rebuilding leads to a compiler error:

../os-posix.c:348:10: warning: address of function 'pthread_fchdir_np' will
always evaluate to 'true' [-Wpointer-bool-conversion]
    if (!pthread_fchdir_np) {
        ~^~~~~~~~~~~~~~~~~

I don't have a machine that's pre-10.12 so I can't see what the result is
there, but this might be why the __builtin_available approach got taken.


>
>
>> > guess it's perhaps a tradeoff between predicting the future unknown
>> > availability of functions versus just ensuring a minimum macOS version
>> and
>> > hoping for the best. With any luck, the distinction between the two
>> > approaches will be moot, if we try to assume that a future macOS version
>> > that removes this also provides mknodat.
>> >
>> > On Tue, Feb 8, 2022 at 10:03 AM Christian Schoenebeck <
>> >
>> > qemu_oss@crudebyte.com> wrote:
>> > > On Dienstag, 8. Februar 2022 14:36:42 CET Will Cohen wrote:
>> > > > On Mon, Feb 7, 2022 at 5:56 PM Christian Schoenebeck
>> > > > <qemu_oss@crudebyte.com>
>> > > >
>> > > > wrote:
>> > > > > On Montag, 7. Februar 2022 23:40:22 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 includes a function
>> > > > > > that sets the cwd for the current thread only.
>> > > > > > This should suffice to use mknod safely.
>> > > > > >
>> > > > > > This function (pthread_fchdir_np) is protected by a check in
>> > > > > > meson in a patch later in tihs series.
>> > > > > >
>> > > > > > Signed-off-by: Keno Fischer <keno@juliacomputing.com>
>> > > > > > Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
>> > > > > > [Will Cohen: - Adjust coding style
>> > > > > >
>> > > > > >              - Replace clang references with gcc
>> > > > > >              - Note radar filed with Apple for missing syscall
>> > > > > >              - Replace direct syscall with pthread_fchdir_np and
>> > > > > >
>> > > > > >                adjust patch notes accordingly
>> > > > > >
>> > > > > >              - Move qemu_mknodat from 9p-util to osdep and
>> os-posix]
>> > > > > >
>> > > > > > Signed-off-by: Will Cohen <wwcohen@gmail.com>
>> > > > > > ---
>> > > > >
>> > > > > Like already mentioned by me moments ago on previous v4 (just
>> echoing)
>> > >
>> > > ...
>> > >
>> > > > > >  hw/9pfs/9p-local.c   |  4 ++--
>> > > > > >  include/qemu/osdep.h | 10 ++++++++++
>> > > > > >  os-posix.c           | 34 ++++++++++++++++++++++++++++++++++
>> > > > > >  3 files changed, 46 insertions(+), 2 deletions(-)
>> > > > > >
>> > > > > > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
>> > > > > > index a0d08e5216..d42ce6d8b8 100644
>> > > > > > --- a/hw/9pfs/9p-local.c
>> > > > > > +++ b/hw/9pfs/9p-local.c
>> > > > > > @@ -682,7 +682,7 @@ static int local_mknod(FsContext *fs_ctx,
>> > >
>> > > V9fsPath
>> > >
>> > > > > > *dir_path,
>> > > > > >
>> > > > > >      if (fs_ctx->export_flags & V9FS_SM_MAPPED ||
>> > > > > >
>> > > > > >          fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
>> > > > > >
>> > > > > > -        err = mknodat(dirfd, name, fs_ctx->fmode | S_IFREG, 0);
>> > > > > > +        err = qemu_mknodat(dirfd, name, fs_ctx->fmode |
>> S_IFREG,
>> > > > > > 0);
>> > > > > >
>> > > > > >          if (err == -1) {
>> > > > > >
>> > > > > >              goto out;
>> > > > > >
>> > > > > >          }
>> > > > > >
>> > > > > > @@ -697,7 +697,7 @@ static int local_mknod(FsContext *fs_ctx,
>> > >
>> > > V9fsPath
>> > >
>> > > > > > *dir_path, }
>> > > > > >
>> > > > > >      } else if (fs_ctx->export_flags & V9FS_SM_PASSTHROUGH ||
>> > > > > >
>> > > > > >                 fs_ctx->export_flags & V9FS_SM_NONE) {
>> > > > > >
>> > > > > > -        err = mknodat(dirfd, name, credp->fc_mode,
>> credp->fc_rdev);
>> > > > > > +        err = qemu_mknodat(dirfd, name, credp->fc_mode,
>> > > > > > credp->fc_rdev);
>> > > > > >
>> > > > > >          if (err == -1) {
>> > > > > >
>> > > > > >              goto out;
>> > > > > >
>> > > > > >          }
>> > > > > >
>> > > > > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
>> > > > > > index d1660d67fa..f3a8367ece 100644
>> > > > > > --- a/include/qemu/osdep.h
>> > > > > > +++ b/include/qemu/osdep.h
>> > > > > > @@ -810,3 +810,13 @@ static inline int
>> > > > > > platform_does_not_support_system(const char *command) #endif
>> > > > > >
>> > > > > >  #endif
>> > > > > >
>> > > > > > +
>> > > > > > +/*
>> > > > > > + * As long as mknodat is not available on macOS, this
>> workaround
>> > > > > > + * using pthread_fchdir_np is needed. qemu_mknodat is defined
>> in
>> > > > > > + * os-posix.c
>> > > > > > + */
>> > > > > > +#ifdef CONFIG_DARWIN
>> > > > > > +int pthread_fchdir_np(int fd);
>> > > > > > +#endif
>> > > > >
>> > > > > I would make that:
>> > > > >
>> > > > > #ifdef CONFIG_DARWIN
>> > > > > int pthread_fchdir_np(int fd) API_AVAILABLE(macosx(10.12));
>> > > > > #endif
>> > > > >
>> > > > > here and ...
>> > > > >
>> > > > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode,
>> > > > > > dev_t
>> > > > >
>> > > > > dev);
>> > > > >
>> > > > > > diff --git a/os-posix.c b/os-posix.c
>> > > > > > index ae6c9f2a5e..95c1607065 100644
>> > > > > > --- a/os-posix.c
>> > > > > > +++ b/os-posix.c
>> > > > > > @@ -24,6 +24,7 @@
>> > > > > >
>> > > > > >   */
>> > > > > >
>> > > > > >  #include "qemu/osdep.h"
>> > > > > >
>> > > > > > +#include <os/availability.h>
>> > > > > >
>> > > > > >  #include <sys/wait.h>
>> > > > > >  #include <pwd.h>
>> > > > > >  #include <grp.h>
>> > > > > >
>> > > > > > @@ -332,3 +333,36 @@ int os_mlock(void)
>> > > > > >
>> > > > > >      return -ENOSYS;
>> > > > > >
>> > > > > >  #endif
>> > > > > >  }
>> > > > > >
>> > > > > > +
>> > > > > > +/*
>> > > > > > + * As long as mknodat is not available on macOS, this
>> workaround
>> > > > > > + * using pthread_fchdir_np is needed.
>> > > > > > + *
>> > > > > > + * Radar filed with Apple for implementing mknodat:
>> > > > > > + * rdar://FB9862426 (https://openradar.appspot.com/FB9862426)
>> > > > > > + */
>> > > > > > +#ifdef CONFIG_DARWIN
>> > > > > > +
>> > > > > > +int pthread_fchdir_np(int fd) API_AVAILABLE(macosx(10.12));
>> > > > >
>> > > > > ... drop the duplicate declaration of pthread_fchdir_np() here.
>> > > >
>> > > > Trying this out, it reminds me that this use of API_AVAILABLE in
>> > >
>> > > os-posix.c
>> > >
>> > > > relies on the added #include <os/availability.h>.
>> > > >
>> > > > Leaving the include out leads to:
>> > > > .../include/qemu/osdep.h:820:31: error: expected function body after
>> > > > function declarator
>> > > > int pthread_fchdir_np(int fd) API_AVAILABLE(macosx(10.12));
>> > > >
>> > > >                               ^
>> > > >
>> > > > 1 error generated.
>> > > > ninja: build stopped: subcommand failed.
>> > > > make[1]: *** [run-ninja] Error 1
>> > > > make: *** [all] Error 2
>> > > >
>> > > > The admonition against modifying osdep.h's includes too much led me
>> to
>> > > > steer away from putting it all in there. If there's no issue with
>> adding
>> > > > +#include <os/availability.h> to osdep.h, then this change makes
>> sense
>> > > > to
>> > > > me.
>> > >
>> > > If you embed that include into ifdefs, sure!
>> > >
>> > > #ifdef CONFIG_DARWIN
>> > > /* defines API_AVAILABLE(...) */
>> > > #include <os/availability.h>
>> > > #endif
>> > >
>> > > One more thing though ...
>> > >
>> > > > > > +
>> > > > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode,
>> > > > > > dev_t
>> > > > >
>> > > > > dev)
>> > > > >
>> > > > > > +{
>> > > > > > +    int preserved_errno, err;
>> > >
>> > > pthread_fchdir_np() is weakly linked. So I guess here should be a
>> check
>> > >
>> > > like:
>> > >         if (!pthread_fchdir_np) {
>> > >
>> > >                 return -ENOTSUPP;
>> > >
>> > >         }
>> > >
>> > > Before trying to call pthread_fchdir_np() below. As already discussed
>> with
>> > > the
>> > > Chromium [1] example, some do that a bit differently by using
>> > >
>> > > __builtin_available():
>> > >         if (__builtin_available(macOS 10.12, *)) {
>> > >
>> > >                 return -ENOTSUPP;
>> > >
>> > >         }
>> > >
>> > > Which makes me wonder why they are not doing a simple NULL check?
>> > >
>> > > [1]
>> > >
>> https://chromium.googlesource.com/chromium/src/+/lkgr/base/process/launch_
>> > > mac.cc#110>
>> > > > > > +    if (pthread_fchdir_np(dirfd) < 0) {
>> > > > > > +        return -1;
>> > > > > > +    }
>> > > > > > +    err = mknod(filename, mode, dev);
>> > > > > > +    preserved_errno = errno;
>> > > > > > +    /* Stop using the thread-local cwd */
>> > > > > > +    pthread_fchdir_np(-1);
>> > > > > > +    if (err < 0) {
>> > > > > > +        errno = preserved_errno;
>> > > > > > +    }
>> > > > > > +    return err;
>> > > > > > +}
>> > > > > > +#else
>> > > > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode,
>> > > > > > dev_t
>> > > > >
>> > > > > dev)
>> > > > >
>> > > > > > +{
>> > > > > > +    return mknodat(dirfd, filename, mode, dev);
>> > > > > > +}
>> > > > > > +#endif
>>
>

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

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

[Christian Schoenebeck]

On Dienstag, 8. Februar 2022 19:04:31 CET Will Cohen wrote:
> On Tue, Feb 8, 2022 at 11:19 AM Will Cohen <wwcohen@gmail.com> wrote:
> > On Tue, Feb 8, 2022 at 11:11 AM Christian Schoenebeck <
> > 
> > qemu_oss@crudebyte.com> wrote:
> >> On Dienstag, 8. Februar 2022 16:57:55 CET Will Cohen wrote:
> >> > My inclination is to go with the __builtin_available(macOS 10.12, *)
> >> 
> >> path,
> >> 
> >> > if acceptable, since it partially mirrors the API_AVAILABLE macro idea.
> >> 
> >> I
> >> 
> >> OTOH that's duplication of the ">= macOS 10.12" info, plus
> >> __builtin_available
> >> is direct use of a clang-only extension, whereas API_AVAILABLE() works
> >> (or
> >> more precisely: doesn't error out at least) with other compilers like GCC
> >> as
> >> well. GCC is sometimes used for cross-compilation.
> >> 
> >> Moreover, I would also add an error message in this case, e.g.:
> >>     if (!pthread_fchdir_np) {
> >>     
> >>         error_report_once("pthread_fchdir_np() is not available on this
> >> 
> >> macOS version");
> >> 
> >>         return -ENOTSUPP;
> >>     
> >>     }
> >> 
> >> I should elaborate why I think this is needed: you are already doing a
> >> Meson
> >> check for the existence of pthread_fchdir_np(), but the system where QEMU
> >> is
> >> compiled and the systems where the compiled binary will be running, might
> >> be
> >> different ones (i.e. different macOS versions).
> >> 
> >> Best regards,
> >> Christian Schoenebeck
> > 
> > Agreed, that way actually closes the edge case. Something along these
> > lines briefly crossed my mind during a previous version, but I quickly got
> > passed it by assuming that the compiling entity would always be the
> > bottleneck, which makes no sense in hindsight, so I very much appreciate
> > that you caught this.
> 
> Ah, rebuilding leads to a compiler error:
> 
> ../os-posix.c:348:10: warning: address of function 'pthread_fchdir_np' will
> always evaluate to 'true' [-Wpointer-bool-conversion]
>     if (!pthread_fchdir_np) {
>         ~^~~~~~~~~~~~~~~~~
> 
> I don't have a machine that's pre-10.12 so I can't see what the result is
> there, but this might be why the __builtin_available approach got taken.

I guess that's because you are compiling QEMU with minimum deployment target 
being macOS >= 10.12 already. In this case the compiler won't make 
pthread_fchdir_np a weak link, it only does emit a weak link if you are 
targeting macOS versions prior than the defined availablity attribute,
hence the address would never be NULL here and hence the compiler warning.

So I guess it is okay if you just omit checking presence of pthread_fchdir_np 
at runtime and just assume it exists.

Added Akihiko on CC, just in case he would have something to add on this macOS 
issue here. :)

Best regards,
Christian Schoenebeck

> >> > guess it's perhaps a tradeoff between predicting the future unknown
> >> > availability of functions versus just ensuring a minimum macOS version
> >> 
> >> and
> >> 
> >> > hoping for the best. With any luck, the distinction between the two
> >> > approaches will be moot, if we try to assume that a future macOS
> >> > version
> >> > that removes this also provides mknodat.
> >> > 
> >> > On Tue, Feb 8, 2022 at 10:03 AM Christian Schoenebeck <
> >> > 
> >> > qemu_oss@crudebyte.com> wrote:
> >> > > On Dienstag, 8. Februar 2022 14:36:42 CET Will Cohen wrote:
> >> > > > On Mon, Feb 7, 2022 at 5:56 PM Christian Schoenebeck
> >> > > > <qemu_oss@crudebyte.com>
> >> > > > 
> >> > > > wrote:
> >> > > > > On Montag, 7. Februar 2022 23:40:22 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 includes a
> >> > > > > > function
> >> > > > > > that sets the cwd for the current thread only.
> >> > > > > > This should suffice to use mknod safely.
> >> > > > > > 
> >> > > > > > This function (pthread_fchdir_np) is protected by a check in
> >> > > > > > meson in a patch later in tihs series.
> >> > > > > > 
> >> > > > > > Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> >> > > > > > Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
> >> > > > > > [Will Cohen: - Adjust coding style
> >> > > > > > 
> >> > > > > >              - Replace clang references with gcc
> >> > > > > >              - Note radar filed with Apple for missing syscall
> >> > > > > >              - Replace direct syscall with pthread_fchdir_np
> >> > > > > >              and
> >> > > > > >              
> >> > > > > >                adjust patch notes accordingly
> >> > > > > >              
> >> > > > > >              - Move qemu_mknodat from 9p-util to osdep and
> >> 
> >> os-posix]
> >> 
> >> > > > > > Signed-off-by: Will Cohen <wwcohen@gmail.com>
> >> > > > > > ---
> >> > > > > 
> >> > > > > Like already mentioned by me moments ago on previous v4 (just
> >> 
> >> echoing)
> >> 
> >> > > ...
> >> > > 
> >> > > > > >  hw/9pfs/9p-local.c   |  4 ++--
> >> > > > > >  include/qemu/osdep.h | 10 ++++++++++
> >> > > > > >  os-posix.c           | 34 ++++++++++++++++++++++++++++++++++
> >> > > > > >  3 files changed, 46 insertions(+), 2 deletions(-)
> >> > > > > > 
> >> > > > > > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> >> > > > > > index a0d08e5216..d42ce6d8b8 100644
> >> > > > > > --- a/hw/9pfs/9p-local.c
> >> > > > > > +++ b/hw/9pfs/9p-local.c
> >> > > > > > @@ -682,7 +682,7 @@ static int local_mknod(FsContext *fs_ctx,
> >> > > 
> >> > > V9fsPath
> >> > > 
> >> > > > > > *dir_path,
> >> > > > > > 
> >> > > > > >      if (fs_ctx->export_flags & V9FS_SM_MAPPED ||
> >> > > > > >      
> >> > > > > >          fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
> >> > > > > > 
> >> > > > > > -        err = mknodat(dirfd, name, fs_ctx->fmode | S_IFREG,
> >> > > > > > 0);
> >> > > > > > +        err = qemu_mknodat(dirfd, name, fs_ctx->fmode |
> >> 
> >> S_IFREG,
> >> 
> >> > > > > > 0);
> >> > > > > > 
> >> > > > > >          if (err == -1) {
> >> > > > > >          
> >> > > > > >              goto out;
> >> > > > > >          
> >> > > > > >          }
> >> > > > > > 
> >> > > > > > @@ -697,7 +697,7 @@ static int local_mknod(FsContext *fs_ctx,
> >> > > 
> >> > > V9fsPath
> >> > > 
> >> > > > > > *dir_path, }
> >> > > > > > 
> >> > > > > >      } else if (fs_ctx->export_flags & V9FS_SM_PASSTHROUGH ||
> >> > > > > >      
> >> > > > > >                 fs_ctx->export_flags & V9FS_SM_NONE) {
> >> > > > > > 
> >> > > > > > -        err = mknodat(dirfd, name, credp->fc_mode,
> >> 
> >> credp->fc_rdev);
> >> 
> >> > > > > > +        err = qemu_mknodat(dirfd, name, credp->fc_mode,
> >> > > > > > credp->fc_rdev);
> >> > > > > > 
> >> > > > > >          if (err == -1) {
> >> > > > > >          
> >> > > > > >              goto out;
> >> > > > > >          
> >> > > > > >          }
> >> > > > > > 
> >> > > > > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> >> > > > > > index d1660d67fa..f3a8367ece 100644
> >> > > > > > --- a/include/qemu/osdep.h
> >> > > > > > +++ b/include/qemu/osdep.h
> >> > > > > > @@ -810,3 +810,13 @@ static inline int
> >> > > > > > platform_does_not_support_system(const char *command) #endif
> >> > > > > > 
> >> > > > > >  #endif
> >> > > > > > 
> >> > > > > > +
> >> > > > > > +/*
> >> > > > > > + * As long as mknodat is not available on macOS, this
> >> 
> >> workaround
> >> 
> >> > > > > > + * using pthread_fchdir_np is needed. qemu_mknodat is defined
> >> 
> >> in
> >> 
> >> > > > > > + * os-posix.c
> >> > > > > > + */
> >> > > > > > +#ifdef CONFIG_DARWIN
> >> > > > > > +int pthread_fchdir_np(int fd);
> >> > > > > > +#endif
> >> > > > > 
> >> > > > > I would make that:
> >> > > > > 
> >> > > > > #ifdef CONFIG_DARWIN
> >> > > > > int pthread_fchdir_np(int fd) API_AVAILABLE(macosx(10.12));
> >> > > > > #endif
> >> > > > > 
> >> > > > > here and ...
> >> > > > > 
> >> > > > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode,
> >> > > > > > dev_t
> >> > > > > 
> >> > > > > dev);
> >> > > > > 
> >> > > > > > diff --git a/os-posix.c b/os-posix.c
> >> > > > > > index ae6c9f2a5e..95c1607065 100644
> >> > > > > > --- a/os-posix.c
> >> > > > > > +++ b/os-posix.c
> >> > > > > > @@ -24,6 +24,7 @@
> >> > > > > > 
> >> > > > > >   */
> >> > > > > >  
> >> > > > > >  #include "qemu/osdep.h"
> >> > > > > > 
> >> > > > > > +#include <os/availability.h>
> >> > > > > > 
> >> > > > > >  #include <sys/wait.h>
> >> > > > > >  #include <pwd.h>
> >> > > > > >  #include <grp.h>
> >> > > > > > 
> >> > > > > > @@ -332,3 +333,36 @@ int os_mlock(void)
> >> > > > > > 
> >> > > > > >      return -ENOSYS;
> >> > > > > >  
> >> > > > > >  #endif
> >> > > > > >  }
> >> > > > > > 
> >> > > > > > +
> >> > > > > > +/*
> >> > > > > > + * As long as mknodat is not available on macOS, this
> >> 
> >> workaround
> >> 
> >> > > > > > + * using pthread_fchdir_np is needed.
> >> > > > > > + *
> >> > > > > > + * Radar filed with Apple for implementing mknodat:
> >> > > > > > + * rdar://FB9862426 (https://openradar.appspot.com/FB9862426)
> >> > > > > > + */
> >> > > > > > +#ifdef CONFIG_DARWIN
> >> > > > > > +
> >> > > > > > +int pthread_fchdir_np(int fd) API_AVAILABLE(macosx(10.12));
> >> > > > > 
> >> > > > > ... drop the duplicate declaration of pthread_fchdir_np() here.
> >> > > > 
> >> > > > Trying this out, it reminds me that this use of API_AVAILABLE in
> >> > > 
> >> > > os-posix.c
> >> > > 
> >> > > > relies on the added #include <os/availability.h>.
> >> > > > 
> >> > > > Leaving the include out leads to:
> >> > > > .../include/qemu/osdep.h:820:31: error: expected function body
> >> > > > after
> >> > > > function declarator
> >> > > > int pthread_fchdir_np(int fd) API_AVAILABLE(macosx(10.12));
> >> > > > 
> >> > > >                               ^
> >> > > > 
> >> > > > 1 error generated.
> >> > > > ninja: build stopped: subcommand failed.
> >> > > > make[1]: *** [run-ninja] Error 1
> >> > > > make: *** [all] Error 2
> >> > > > 
> >> > > > The admonition against modifying osdep.h's includes too much led me
> >> 
> >> to
> >> 
> >> > > > steer away from putting it all in there. If there's no issue with
> >> 
> >> adding
> >> 
> >> > > > +#include <os/availability.h> to osdep.h, then this change makes
> >> 
> >> sense
> >> 
> >> > > > to
> >> > > > me.
> >> > > 
> >> > > If you embed that include into ifdefs, sure!
> >> > > 
> >> > > #ifdef CONFIG_DARWIN
> >> > > /* defines API_AVAILABLE(...) */
> >> > > #include <os/availability.h>
> >> > > #endif
> >> > > 
> >> > > One more thing though ...
> >> > > 
> >> > > > > > +
> >> > > > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode,
> >> > > > > > dev_t
> >> > > > > 
> >> > > > > dev)
> >> > > > > 
> >> > > > > > +{
> >> > > > > > +    int preserved_errno, err;
> >> > > 
> >> > > pthread_fchdir_np() is weakly linked. So I guess here should be a
> >> 
> >> check
> >> 
> >> > > like:
> >> > >         if (!pthread_fchdir_np) {
> >> > >         
> >> > >                 return -ENOTSUPP;
> >> > >         
> >> > >         }
> >> > > 
> >> > > Before trying to call pthread_fchdir_np() below. As already discussed
> >> 
> >> with
> >> 
> >> > > the
> >> > > Chromium [1] example, some do that a bit differently by using
> >> > > 
> >> > > __builtin_available():
> >> > >         if (__builtin_available(macOS 10.12, *)) {
> >> > >         
> >> > >                 return -ENOTSUPP;
> >> > >         
> >> > >         }
> >> > > 
> >> > > Which makes me wonder why they are not doing a simple NULL check?
> >> > > 
> >> > > [1]
> >> 
> >> https://chromium.googlesource.com/chromium/src/+/lkgr/base/process/launch
> >> _
> >> 
> >> > > mac.cc#110>
> >> > > 
> >> > > > > > +    if (pthread_fchdir_np(dirfd) < 0) {
> >> > > > > > +        return -1;
> >> > > > > > +    }
> >> > > > > > +    err = mknod(filename, mode, dev);
> >> > > > > > +    preserved_errno = errno;
> >> > > > > > +    /* Stop using the thread-local cwd */
> >> > > > > > +    pthread_fchdir_np(-1);
> >> > > > > > +    if (err < 0) {
> >> > > > > > +        errno = preserved_errno;
> >> > > > > > +    }
> >> > > > > > +    return err;
> >> > > > > > +}
> >> > > > > > +#else
> >> > > > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode,
> >> > > > > > dev_t
> >> > > > > 
> >> > > > > dev)
> >> > > > > 
> >> > > > > > +{
> >> > > > > > +    return mknodat(dirfd, filename, mode, dev);
> >> > > > > > +}
> >> > > > > > +#endif


Best regards,
Christian Schoenebeck

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

[Christian Schoenebeck]

On Dienstag, 8. Februar 2022 19:28:21 CET Christian Schoenebeck wrote:
> On Dienstag, 8. Februar 2022 19:04:31 CET Will Cohen wrote:
> > On Tue, Feb 8, 2022 at 11:19 AM Will Cohen <wwcohen@gmail.com> wrote:
> > > On Tue, Feb 8, 2022 at 11:11 AM Christian Schoenebeck <
> > > 
> > > qemu_oss@crudebyte.com> wrote:
> > >> On Dienstag, 8. Februar 2022 16:57:55 CET Will Cohen wrote:
> > >> > My inclination is to go with the __builtin_available(macOS 10.12, *)
> > >> 
> > >> path,
> > >> 
> > >> > if acceptable, since it partially mirrors the API_AVAILABLE macro
> > >> > idea.
> > >> 
> > >> I
> > >> 
> > >> OTOH that's duplication of the ">= macOS 10.12" info, plus
> > >> __builtin_available
> > >> is direct use of a clang-only extension, whereas API_AVAILABLE() works
> > >> (or
> > >> more precisely: doesn't error out at least) with other compilers like
> > >> GCC
> > >> as
> > >> well. GCC is sometimes used for cross-compilation.
> > >> 
> > >> Moreover, I would also add an error message in this case, e.g.:
> > >>     if (!pthread_fchdir_np) {
> > >>     
> > >>         error_report_once("pthread_fchdir_np() is not available on this
> > >> 
> > >> macOS version");
> > >> 
> > >>         return -ENOTSUPP;
> > >>     
> > >>     }
> > >> 
> > >> I should elaborate why I think this is needed: you are already doing a
> > >> Meson
> > >> check for the existence of pthread_fchdir_np(), but the system where
> > >> QEMU
> > >> is
> > >> compiled and the systems where the compiled binary will be running,
> > >> might
> > >> be
> > >> different ones (i.e. different macOS versions).
> > >> 
> > >> Best regards,
> > >> Christian Schoenebeck
> > > 
> > > Agreed, that way actually closes the edge case. Something along these
> > > lines briefly crossed my mind during a previous version, but I quickly
> > > got
> > > passed it by assuming that the compiling entity would always be the
> > > bottleneck, which makes no sense in hindsight, so I very much appreciate
> > > that you caught this.
> > 
> > Ah, rebuilding leads to a compiler error:
> > 
> > ../os-posix.c:348:10: warning: address of function 'pthread_fchdir_np'
> > will
> > always evaluate to 'true' [-Wpointer-bool-conversion]
> > 
> >     if (!pthread_fchdir_np) {
> >     
> >         ~^~~~~~~~~~~~~~~~~
> > 
> > I don't have a machine that's pre-10.12 so I can't see what the result is
> > there, but this might be why the __builtin_available approach got taken.
> 
> I guess that's because you are compiling QEMU with minimum deployment target
> being macOS >= 10.12 already. In this case the compiler won't make
> pthread_fchdir_np a weak link, it only does emit a weak link if you are
> targeting macOS versions prior than the defined availablity attribute,
> hence the address would never be NULL here and hence the compiler warning.
> 
> So I guess it is okay if you just omit checking presence of
> pthread_fchdir_np at runtime and just assume it exists.
> 
> Added Akihiko on CC, just in case he would have something to add on this
> macOS issue here. :)

On a second thought: this case a bit special. Are we worried that 
pthread_fchdir_np() is "not yet" available on macOS, or "no longer" available. 
Probably both, right?

So maybe it would make sense to replace the API_AVAILABLE() attribute directly 
with a __attribute__((weak)) attribute. Then the runtime check with the 
proposed error message would also trigger if a bleeding edge macOS version no 
longer has pthread_fchdir_np().

Also keep in mind: there are always also the MAC_OS_X_VERSION_MIN_REQUIRED and 
MAC_OS_X_VERSION_MAX_ALLOWED macros to query the deployment target at compile 
time to wrap deployment target dependent code accordingly.

On doubt you could just make some tests there by simply "inventing" a non-
existent function.

Best regards,
Christian Schoenebeck

> > >> > guess it's perhaps a tradeoff between predicting the future unknown
> > >> > availability of functions versus just ensuring a minimum macOS
> > >> > version
> > >> 
> > >> and
> > >> 
> > >> > hoping for the best. With any luck, the distinction between the two
> > >> > approaches will be moot, if we try to assume that a future macOS
> > >> > version
> > >> > that removes this also provides mknodat.
> > >> > 
> > >> > On Tue, Feb 8, 2022 at 10:03 AM Christian Schoenebeck <
> > >> > 
> > >> > qemu_oss@crudebyte.com> wrote:
> > >> > > On Dienstag, 8. Februar 2022 14:36:42 CET Will Cohen wrote:
> > >> > > > On Mon, Feb 7, 2022 at 5:56 PM Christian Schoenebeck
> > >> > > > <qemu_oss@crudebyte.com>
> > >> > > > 
> > >> > > > wrote:
> > >> > > > > On Montag, 7. Februar 2022 23:40:22 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 includes a
> > >> > > > > > function
> > >> > > > > > that sets the cwd for the current thread only.
> > >> > > > > > This should suffice to use mknod safely.
> > >> > > > > > 
> > >> > > > > > This function (pthread_fchdir_np) is protected by a check in
> > >> > > > > > meson in a patch later in tihs series.
> > >> > > > > > 
> > >> > > > > > Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> > >> > > > > > Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
> > >> > > > > > [Will Cohen: - Adjust coding style
> > >> > > > > > 
> > >> > > > > >              - Replace clang references with gcc
> > >> > > > > >              - Note radar filed with Apple for missing
> > >> > > > > >              syscall
> > >> > > > > >              - Replace direct syscall with pthread_fchdir_np
> > >> > > > > >              and
> > >> > > > > >              
> > >> > > > > >                adjust patch notes accordingly
> > >> > > > > >              
> > >> > > > > >              - Move qemu_mknodat from 9p-util to osdep and
> > >> 
> > >> os-posix]
> > >> 
> > >> > > > > > Signed-off-by: Will Cohen <wwcohen@gmail.com>
> > >> > > > > > ---
> > >> > > > > 
> > >> > > > > Like already mentioned by me moments ago on previous v4 (just
> > >> 
> > >> echoing)
> > >> 
> > >> > > ...
> > >> > > 
> > >> > > > > >  hw/9pfs/9p-local.c   |  4 ++--
> > >> > > > > >  include/qemu/osdep.h | 10 ++++++++++
> > >> > > > > >  os-posix.c           | 34 ++++++++++++++++++++++++++++++++++
> > >> > > > > >  3 files changed, 46 insertions(+), 2 deletions(-)
> > >> > > > > > 
> > >> > > > > > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> > >> > > > > > index a0d08e5216..d42ce6d8b8 100644
> > >> > > > > > --- a/hw/9pfs/9p-local.c
> > >> > > > > > +++ b/hw/9pfs/9p-local.c
> > >> > > > > > @@ -682,7 +682,7 @@ static int local_mknod(FsContext *fs_ctx,
> > >> > > 
> > >> > > V9fsPath
> > >> > > 
> > >> > > > > > *dir_path,
> > >> > > > > > 
> > >> > > > > >      if (fs_ctx->export_flags & V9FS_SM_MAPPED ||
> > >> > > > > >      
> > >> > > > > >          fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
> > >> > > > > > 
> > >> > > > > > -        err = mknodat(dirfd, name, fs_ctx->fmode | S_IFREG,
> > >> > > > > > 0);
> > >> > > > > > +        err = qemu_mknodat(dirfd, name, fs_ctx->fmode |
> > >> 
> > >> S_IFREG,
> > >> 
> > >> > > > > > 0);
> > >> > > > > > 
> > >> > > > > >          if (err == -1) {
> > >> > > > > >          
> > >> > > > > >              goto out;
> > >> > > > > >          
> > >> > > > > >          }
> > >> > > > > > 
> > >> > > > > > @@ -697,7 +697,7 @@ static int local_mknod(FsContext *fs_ctx,
> > >> > > 
> > >> > > V9fsPath
> > >> > > 
> > >> > > > > > *dir_path, }
> > >> > > > > > 
> > >> > > > > >      } else if (fs_ctx->export_flags & V9FS_SM_PASSTHROUGH ||
> > >> > > > > >      
> > >> > > > > >                 fs_ctx->export_flags & V9FS_SM_NONE) {
> > >> > > > > > 
> > >> > > > > > -        err = mknodat(dirfd, name, credp->fc_mode,
> > >> 
> > >> credp->fc_rdev);
> > >> 
> > >> > > > > > +        err = qemu_mknodat(dirfd, name, credp->fc_mode,
> > >> > > > > > credp->fc_rdev);
> > >> > > > > > 
> > >> > > > > >          if (err == -1) {
> > >> > > > > >          
> > >> > > > > >              goto out;
> > >> > > > > >          
> > >> > > > > >          }
> > >> > > > > > 
> > >> > > > > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > >> > > > > > index d1660d67fa..f3a8367ece 100644
> > >> > > > > > --- a/include/qemu/osdep.h
> > >> > > > > > +++ b/include/qemu/osdep.h
> > >> > > > > > @@ -810,3 +810,13 @@ static inline int
> > >> > > > > > platform_does_not_support_system(const char *command) #endif
> > >> > > > > > 
> > >> > > > > >  #endif
> > >> > > > > > 
> > >> > > > > > +
> > >> > > > > > +/*
> > >> > > > > > + * As long as mknodat is not available on macOS, this
> > >> 
> > >> workaround
> > >> 
> > >> > > > > > + * using pthread_fchdir_np is needed. qemu_mknodat is
> > >> > > > > > defined
> > >> 
> > >> in
> > >> 
> > >> > > > > > + * os-posix.c
> > >> > > > > > + */
> > >> > > > > > +#ifdef CONFIG_DARWIN
> > >> > > > > > +int pthread_fchdir_np(int fd);
> > >> > > > > > +#endif
> > >> > > > > 
> > >> > > > > I would make that:
> > >> > > > > 
> > >> > > > > #ifdef CONFIG_DARWIN
> > >> > > > > int pthread_fchdir_np(int fd) API_AVAILABLE(macosx(10.12));
> > >> > > > > #endif
> > >> > > > > 
> > >> > > > > here and ...
> > >> > > > > 
> > >> > > > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t
> > >> > > > > > mode,
> > >> > > > > > dev_t
> > >> > > > > 
> > >> > > > > dev);
> > >> > > > > 
> > >> > > > > > diff --git a/os-posix.c b/os-posix.c
> > >> > > > > > index ae6c9f2a5e..95c1607065 100644
> > >> > > > > > --- a/os-posix.c
> > >> > > > > > +++ b/os-posix.c
> > >> > > > > > @@ -24,6 +24,7 @@
> > >> > > > > > 
> > >> > > > > >   */
> > >> > > > > >  
> > >> > > > > >  #include "qemu/osdep.h"
> > >> > > > > > 
> > >> > > > > > +#include <os/availability.h>
> > >> > > > > > 
> > >> > > > > >  #include <sys/wait.h>
> > >> > > > > >  #include <pwd.h>
> > >> > > > > >  #include <grp.h>
> > >> > > > > > 
> > >> > > > > > @@ -332,3 +333,36 @@ int os_mlock(void)
> > >> > > > > > 
> > >> > > > > >      return -ENOSYS;
> > >> > > > > >  
> > >> > > > > >  #endif
> > >> > > > > >  }
> > >> > > > > > 
> > >> > > > > > +
> > >> > > > > > +/*
> > >> > > > > > + * As long as mknodat is not available on macOS, this
> > >> 
> > >> workaround
> > >> 
> > >> > > > > > + * using pthread_fchdir_np is needed.
> > >> > > > > > + *
> > >> > > > > > + * Radar filed with Apple for implementing mknodat:
> > >> > > > > > + * rdar://FB9862426
> > >> > > > > > (https://openradar.appspot.com/FB9862426)
> > >> > > > > > + */
> > >> > > > > > +#ifdef CONFIG_DARWIN
> > >> > > > > > +
> > >> > > > > > +int pthread_fchdir_np(int fd) API_AVAILABLE(macosx(10.12));
> > >> > > > > 
> > >> > > > > ... drop the duplicate declaration of pthread_fchdir_np() here.
> > >> > > > 
> > >> > > > Trying this out, it reminds me that this use of API_AVAILABLE in
> > >> > > 
> > >> > > os-posix.c
> > >> > > 
> > >> > > > relies on the added #include <os/availability.h>.
> > >> > > > 
> > >> > > > Leaving the include out leads to:
> > >> > > > .../include/qemu/osdep.h:820:31: error: expected function body
> > >> > > > after
> > >> > > > function declarator
> > >> > > > int pthread_fchdir_np(int fd) API_AVAILABLE(macosx(10.12));
> > >> > > > 
> > >> > > >                               ^
> > >> > > > 
> > >> > > > 1 error generated.
> > >> > > > ninja: build stopped: subcommand failed.
> > >> > > > make[1]: *** [run-ninja] Error 1
> > >> > > > make: *** [all] Error 2
> > >> > > > 
> > >> > > > The admonition against modifying osdep.h's includes too much led
> > >> > > > me
> > >> 
> > >> to
> > >> 
> > >> > > > steer away from putting it all in there. If there's no issue with
> > >> 
> > >> adding
> > >> 
> > >> > > > +#include <os/availability.h> to osdep.h, then this change makes
> > >> 
> > >> sense
> > >> 
> > >> > > > to
> > >> > > > me.
> > >> > > 
> > >> > > If you embed that include into ifdefs, sure!
> > >> > > 
> > >> > > #ifdef CONFIG_DARWIN
> > >> > > /* defines API_AVAILABLE(...) */
> > >> > > #include <os/availability.h>
> > >> > > #endif
> > >> > > 
> > >> > > One more thing though ...
> > >> > > 
> > >> > > > > > +
> > >> > > > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t
> > >> > > > > > mode,
> > >> > > > > > dev_t
> > >> > > > > 
> > >> > > > > dev)
> > >> > > > > 
> > >> > > > > > +{
> > >> > > > > > +    int preserved_errno, err;
> > >> > > 
> > >> > > pthread_fchdir_np() is weakly linked. So I guess here should be a
> > >> 
> > >> check
> > >> 
> > >> > > like:
> > >> > >         if (!pthread_fchdir_np) {
> > >> > >         
> > >> > >                 return -ENOTSUPP;
> > >> > >         
> > >> > >         }
> > >> > > 
> > >> > > Before trying to call pthread_fchdir_np() below. As already
> > >> > > discussed
> > >> 
> > >> with
> > >> 
> > >> > > the
> > >> > > Chromium [1] example, some do that a bit differently by using
> > >> > > 
> > >> > > __builtin_available():
> > >> > >         if (__builtin_available(macOS 10.12, *)) {
> > >> > >         
> > >> > >                 return -ENOTSUPP;
> > >> > >         
> > >> > >         }
> > >> > > 
> > >> > > Which makes me wonder why they are not doing a simple NULL check?
> > >> > > 
> > >> > > [1]
> > >> 
> > >> https://chromium.googlesource.com/chromium/src/+/lkgr/base/process/laun
> > >> ch
> > >> _
> > >> 
> > >> > > mac.cc#110>
> > >> > > 
> > >> > > > > > +    if (pthread_fchdir_np(dirfd) < 0) {
> > >> > > > > > +        return -1;
> > >> > > > > > +    }
> > >> > > > > > +    err = mknod(filename, mode, dev);
> > >> > > > > > +    preserved_errno = errno;
> > >> > > > > > +    /* Stop using the thread-local cwd */
> > >> > > > > > +    pthread_fchdir_np(-1);
> > >> > > > > > +    if (err < 0) {
> > >> > > > > > +        errno = preserved_errno;
> > >> > > > > > +    }
> > >> > > > > > +    return err;
> > >> > > > > > +}
> > >> > > > > > +#else
> > >> > > > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t
> > >> > > > > > mode,
> > >> > > > > > dev_t
> > >> > > > > 
> > >> > > > > dev)
> > >> > > > > 
> > >> > > > > > +{
> > >> > > > > > +    return mknodat(dirfd, filename, mode, dev);
> > >> > > > > > +}
> > >> > > > > > +#endif
> 
> Best regards,
> Christian Schoenebeck


Best regards,
Christian Schoenebeck

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

[Will Cohen]

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

On Tue, Feb 8, 2022 at 2:49 PM Christian Schoenebeck <qemu_oss@crudebyte.com>
wrote:

> On Dienstag, 8. Februar 2022 19:28:21 CET Christian Schoenebeck wrote:
> > On Dienstag, 8. Februar 2022 19:04:31 CET Will Cohen wrote:
> > > On Tue, Feb 8, 2022 at 11:19 AM Will Cohen <wwcohen@gmail.com> wrote:
> > > > On Tue, Feb 8, 2022 at 11:11 AM Christian Schoenebeck <
> > > >
> > > > qemu_oss@crudebyte.com> wrote:
> > > >> On Dienstag, 8. Februar 2022 16:57:55 CET Will Cohen wrote:
> > > >> > My inclination is to go with the __builtin_available(macOS 10.12,
> *)
> > > >>
> > > >> path,
> > > >>
> > > >> > if acceptable, since it partially mirrors the API_AVAILABLE macro
> > > >> > idea.
> > > >>
> > > >> I
> > > >>
> > > >> OTOH that's duplication of the ">= macOS 10.12" info, plus
> > > >> __builtin_available
> > > >> is direct use of a clang-only extension, whereas API_AVAILABLE()
> works
> > > >> (or
> > > >> more precisely: doesn't error out at least) with other compilers
> like
> > > >> GCC
> > > >> as
> > > >> well. GCC is sometimes used for cross-compilation.
> > > >>
> > > >> Moreover, I would also add an error message in this case, e.g.:
> > > >>     if (!pthread_fchdir_np) {
> > > >>
> > > >>         error_report_once("pthread_fchdir_np() is not available on
> this
> > > >>
> > > >> macOS version");
> > > >>
> > > >>         return -ENOTSUPP;
> > > >>
> > > >>     }
> > > >>
> > > >> I should elaborate why I think this is needed: you are already
> doing a
> > > >> Meson
> > > >> check for the existence of pthread_fchdir_np(), but the system where
> > > >> QEMU
> > > >> is
> > > >> compiled and the systems where the compiled binary will be running,
> > > >> might
> > > >> be
> > > >> different ones (i.e. different macOS versions).
> > > >>
> > > >> Best regards,
> > > >> Christian Schoenebeck
> > > >
> > > > Agreed, that way actually closes the edge case. Something along these
> > > > lines briefly crossed my mind during a previous version, but I
> quickly
> > > > got
> > > > passed it by assuming that the compiling entity would always be the
> > > > bottleneck, which makes no sense in hindsight, so I very much
> appreciate
> > > > that you caught this.
> > >
> > > Ah, rebuilding leads to a compiler error:
> > >
> > > ../os-posix.c:348:10: warning: address of function 'pthread_fchdir_np'
> > > will
> > > always evaluate to 'true' [-Wpointer-bool-conversion]
> > >
> > >     if (!pthread_fchdir_np) {
> > >
> > >         ~^~~~~~~~~~~~~~~~~
> > >
> > > I don't have a machine that's pre-10.12 so I can't see what the result
> is
> > > there, but this might be why the __builtin_available approach got
> taken.
> >
> > I guess that's because you are compiling QEMU with minimum deployment
> target
> > being macOS >= 10.12 already. In this case the compiler won't make
> > pthread_fchdir_np a weak link, it only does emit a weak link if you are
> > targeting macOS versions prior than the defined availablity attribute,
> > hence the address would never be NULL here and hence the compiler
> warning.
> >
> > So I guess it is okay if you just omit checking presence of
> > pthread_fchdir_np at runtime and just assume it exists.
> >
> > Added Akihiko on CC, just in case he would have something to add on this
> > macOS issue here. :)
>
> On a second thought: this case a bit special. Are we worried that
> pthread_fchdir_np() is "not yet" available on macOS, or "no longer"
> available.
> Probably both, right?
>
> So maybe it would make sense to replace the API_AVAILABLE() attribute
> directly
> with a __attribute__((weak)) attribute. Then the runtime check with the
> proposed error message would also trigger if a bleeding edge macOS version
> no
> longer has pthread_fchdir_np().
>
> Also keep in mind: there are always also the MAC_OS_X_VERSION_MIN_REQUIRED
> and
> MAC_OS_X_VERSION_MAX_ALLOWED macros to query the deployment target at
> compile
> time to wrap deployment target dependent code accordingly.
>
> On doubt you could just make some tests there by simply "inventing" a non-
> existent function.
>
> Best regards,
> Christian Schoenebeck
>

I like the idea of switching it to __attribute__((weak)). I should note
that I'm not sure that I can actually fully test this out since I'm getting
stuck with the linker noting my undefined fake function during the build,
but the idea does make logical sense to me for the future fail case and the
happy case builds again when implemented with actual pthread_fchdir_np:

[1075/2909] Linking target qemu-nbd
FAILED: qemu-nbd
cc -m64 -mcx16  -o qemu-nbd qemu-nbd.p/qemu-nbd.c.o -Wl,-dead_strip_dylibs
-Wl,-headerpad_max_install_names -Wl,-undefined,error -Wl,-force_load
libblockdev.fa -Wl,-force_load libblock.fa -Wl,-force_load libcrypto.fa
-Wl,-force_load libauthz.fa -Wl,-force_load libqom.fa -Wl,-force_load
libio.fa -fstack-protector-strong
-Wl,-rpath,/usr/local/Cellar/gnutls/3.7.3/lib
-Wl,-rpath,/usr/local/Cellar/pixman/0.40.0/lib libqemuutil.a libblockdev.fa
libblock.fa libcrypto.fa libauthz.fa libqom.fa libio.fa @block.syms
/usr/local/Cellar/gnutls/3.7.3/lib/libgnutls.dylib -lutil
-L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib -lgio-2.0
-lgobject-2.0 -lglib-2.0 -lintl -L/usr/local/Cellar/glib/2.70.3/lib
-L/usr/local/opt/gettext/lib -lgio-2.0 -lgobject-2.0 -lglib-2.0 -lintl -lm
-L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib
-lgmodule-2.0 -lglib-2.0 -lintl
/usr/local/Cellar/pixman/0.40.0/lib/libpixman-1.dylib -lz -lxml2 -framework
CoreFoundation -framework IOKit -lcurl -L/usr/local/Cellar/glib/2.70.3/lib
-L/usr/local/opt/gettext/lib -lgmodule-2.0 -lglib-2.0 -lintl -lbz2
/usr/local/Cellar/libssh/0.9.6/lib/libssh.dylib -lpam
Undefined symbols for architecture x86_64:
  "_pthread_fchdir_npfoo", referenced from:
      _qemu_mknodat in libblockdev.fa(os-posix.c.o)
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see
invocation)
ninja: build stopped: subcommand failed.
make[1]: *** [run-ninja] Error 1
make: *** [all] Error 2

With that caveat re testing in mind, unless there's another recommended
path forward, I think it makes sense to stick with __attribute__((weak))
and prepare v6 which incorporates this and all the other feedback from this
round.


>
> > > >> > guess it's perhaps a tradeoff between predicting the future
> unknown
> > > >> > availability of functions versus just ensuring a minimum macOS
> > > >> > version
> > > >>
> > > >> and
> > > >>
> > > >> > hoping for the best. With any luck, the distinction between the
> two
> > > >> > approaches will be moot, if we try to assume that a future macOS
> > > >> > version
> > > >> > that removes this also provides mknodat.
> > > >> >
> > > >> > On Tue, Feb 8, 2022 at 10:03 AM Christian Schoenebeck <
> > > >> >
> > > >> > qemu_oss@crudebyte.com> wrote:
> > > >> > > On Dienstag, 8. Februar 2022 14:36:42 CET Will Cohen wrote:
> > > >> > > > On Mon, Feb 7, 2022 at 5:56 PM Christian Schoenebeck
> > > >> > > > <qemu_oss@crudebyte.com>
> > > >> > > >
> > > >> > > > wrote:
> > > >> > > > > On Montag, 7. Februar 2022 23:40:22 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 includes a
> > > >> > > > > > function
> > > >> > > > > > that sets the cwd for the current thread only.
> > > >> > > > > > This should suffice to use mknod safely.
> > > >> > > > > >
> > > >> > > > > > This function (pthread_fchdir_np) is protected by a check
> in
> > > >> > > > > > meson in a patch later in tihs series.
> > > >> > > > > >
> > > >> > > > > > Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> > > >> > > > > > Signed-off-by: Michael Roitzsch <
> reactorcontrol@icloud.com>
> > > >> > > > > > [Will Cohen: - Adjust coding style
> > > >> > > > > >
> > > >> > > > > >              - Replace clang references with gcc
> > > >> > > > > >              - Note radar filed with Apple for missing
> > > >> > > > > >              syscall
> > > >> > > > > >              - Replace direct syscall with
> pthread_fchdir_np
> > > >> > > > > >              and
> > > >> > > > > >
> > > >> > > > > >                adjust patch notes accordingly
> > > >> > > > > >
> > > >> > > > > >              - Move qemu_mknodat from 9p-util to osdep and
> > > >>
> > > >> os-posix]
> > > >>
> > > >> > > > > > Signed-off-by: Will Cohen <wwcohen@gmail.com>
> > > >> > > > > > ---
> > > >> > > > >
> > > >> > > > > Like already mentioned by me moments ago on previous v4
> (just
> > > >>
> > > >> echoing)
> > > >>
> > > >> > > ...
> > > >> > >
> > > >> > > > > >  hw/9pfs/9p-local.c   |  4 ++--
> > > >> > > > > >  include/qemu/osdep.h | 10 ++++++++++
> > > >> > > > > >  os-posix.c           | 34
> ++++++++++++++++++++++++++++++++++
> > > >> > > > > >  3 files changed, 46 insertions(+), 2 deletions(-)
> > > >> > > > > >
> > > >> > > > > > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> > > >> > > > > > index a0d08e5216..d42ce6d8b8 100644
> > > >> > > > > > --- a/hw/9pfs/9p-local.c
> > > >> > > > > > +++ b/hw/9pfs/9p-local.c
> > > >> > > > > > @@ -682,7 +682,7 @@ static int local_mknod(FsContext
> *fs_ctx,
> > > >> > >
> > > >> > > V9fsPath
> > > >> > >
> > > >> > > > > > *dir_path,
> > > >> > > > > >
> > > >> > > > > >      if (fs_ctx->export_flags & V9FS_SM_MAPPED ||
> > > >> > > > > >
> > > >> > > > > >          fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
> > > >> > > > > >
> > > >> > > > > > -        err = mknodat(dirfd, name, fs_ctx->fmode |
> S_IFREG,
> > > >> > > > > > 0);
> > > >> > > > > > +        err = qemu_mknodat(dirfd, name, fs_ctx->fmode |
> > > >>
> > > >> S_IFREG,
> > > >>
> > > >> > > > > > 0);
> > > >> > > > > >
> > > >> > > > > >          if (err == -1) {
> > > >> > > > > >
> > > >> > > > > >              goto out;
> > > >> > > > > >
> > > >> > > > > >          }
> > > >> > > > > >
> > > >> > > > > > @@ -697,7 +697,7 @@ static int local_mknod(FsContext
> *fs_ctx,
> > > >> > >
> > > >> > > V9fsPath
> > > >> > >
> > > >> > > > > > *dir_path, }
> > > >> > > > > >
> > > >> > > > > >      } else if (fs_ctx->export_flags &
> V9FS_SM_PASSTHROUGH ||
> > > >> > > > > >
> > > >> > > > > >                 fs_ctx->export_flags & V9FS_SM_NONE) {
> > > >> > > > > >
> > > >> > > > > > -        err = mknodat(dirfd, name, credp->fc_mode,
> > > >>
> > > >> credp->fc_rdev);
> > > >>
> > > >> > > > > > +        err = qemu_mknodat(dirfd, name, credp->fc_mode,
> > > >> > > > > > credp->fc_rdev);
> > > >> > > > > >
> > > >> > > > > >          if (err == -1) {
> > > >> > > > > >
> > > >> > > > > >              goto out;
> > > >> > > > > >
> > > >> > > > > >          }
> > > >> > > > > >
> > > >> > > > > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > > >> > > > > > index d1660d67fa..f3a8367ece 100644
> > > >> > > > > > --- a/include/qemu/osdep.h
> > > >> > > > > > +++ b/include/qemu/osdep.h
> > > >> > > > > > @@ -810,3 +810,13 @@ static inline int
> > > >> > > > > > platform_does_not_support_system(const char *command)
> #endif
> > > >> > > > > >
> > > >> > > > > >  #endif
> > > >> > > > > >
> > > >> > > > > > +
> > > >> > > > > > +/*
> > > >> > > > > > + * As long as mknodat is not available on macOS, this
> > > >>
> > > >> workaround
> > > >>
> > > >> > > > > > + * using pthread_fchdir_np is needed. qemu_mknodat is
> > > >> > > > > > defined
> > > >>
> > > >> in
> > > >>
> > > >> > > > > > + * os-posix.c
> > > >> > > > > > + */
> > > >> > > > > > +#ifdef CONFIG_DARWIN
> > > >> > > > > > +int pthread_fchdir_np(int fd);
> > > >> > > > > > +#endif
> > > >> > > > >
> > > >> > > > > I would make that:
> > > >> > > > >
> > > >> > > > > #ifdef CONFIG_DARWIN
> > > >> > > > > int pthread_fchdir_np(int fd) API_AVAILABLE(macosx(10.12));
> > > >> > > > > #endif
> > > >> > > > >
> > > >> > > > > here and ...
> > > >> > > > >
> > > >> > > > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t
> > > >> > > > > > mode,
> > > >> > > > > > dev_t
> > > >> > > > >
> > > >> > > > > dev);
> > > >> > > > >
> > > >> > > > > > diff --git a/os-posix.c b/os-posix.c
> > > >> > > > > > index ae6c9f2a5e..95c1607065 100644
> > > >> > > > > > --- a/os-posix.c
> > > >> > > > > > +++ b/os-posix.c
> > > >> > > > > > @@ -24,6 +24,7 @@
> > > >> > > > > >
> > > >> > > > > >   */
> > > >> > > > > >
> > > >> > > > > >  #include "qemu/osdep.h"
> > > >> > > > > >
> > > >> > > > > > +#include <os/availability.h>
> > > >> > > > > >
> > > >> > > > > >  #include <sys/wait.h>
> > > >> > > > > >  #include <pwd.h>
> > > >> > > > > >  #include <grp.h>
> > > >> > > > > >
> > > >> > > > > > @@ -332,3 +333,36 @@ int os_mlock(void)
> > > >> > > > > >
> > > >> > > > > >      return -ENOSYS;
> > > >> > > > > >
> > > >> > > > > >  #endif
> > > >> > > > > >  }
> > > >> > > > > >
> > > >> > > > > > +
> > > >> > > > > > +/*
> > > >> > > > > > + * As long as mknodat is not available on macOS, this
> > > >>
> > > >> workaround
> > > >>
> > > >> > > > > > + * using pthread_fchdir_np is needed.
> > > >> > > > > > + *
> > > >> > > > > > + * Radar filed with Apple for implementing mknodat:
> > > >> > > > > > + * rdar://FB9862426
> > > >> > > > > > (https://openradar.appspot.com/FB9862426)
> > > >> > > > > > + */
> > > >> > > > > > +#ifdef CONFIG_DARWIN
> > > >> > > > > > +
> > > >> > > > > > +int pthread_fchdir_np(int fd)
> API_AVAILABLE(macosx(10.12));
> > > >> > > > >
> > > >> > > > > ... drop the duplicate declaration of pthread_fchdir_np()
> here.
> > > >> > > >
> > > >> > > > Trying this out, it reminds me that this use of API_AVAILABLE
> in
> > > >> > >
> > > >> > > os-posix.c
> > > >> > >
> > > >> > > > relies on the added #include <os/availability.h>.
> > > >> > > >
> > > >> > > > Leaving the include out leads to:
> > > >> > > > .../include/qemu/osdep.h:820:31: error: expected function body
> > > >> > > > after
> > > >> > > > function declarator
> > > >> > > > int pthread_fchdir_np(int fd) API_AVAILABLE(macosx(10.12));
> > > >> > > >
> > > >> > > >                               ^
> > > >> > > >
> > > >> > > > 1 error generated.
> > > >> > > > ninja: build stopped: subcommand failed.
> > > >> > > > make[1]: *** [run-ninja] Error 1
> > > >> > > > make: *** [all] Error 2
> > > >> > > >
> > > >> > > > The admonition against modifying osdep.h's includes too much
> led
> > > >> > > > me
> > > >>
> > > >> to
> > > >>
> > > >> > > > steer away from putting it all in there. If there's no issue
> with
> > > >>
> > > >> adding
> > > >>
> > > >> > > > +#include <os/availability.h> to osdep.h, then this change
> makes
> > > >>
> > > >> sense
> > > >>
> > > >> > > > to
> > > >> > > > me.
> > > >> > >
> > > >> > > If you embed that include into ifdefs, sure!
> > > >> > >
> > > >> > > #ifdef CONFIG_DARWIN
> > > >> > > /* defines API_AVAILABLE(...) */
> > > >> > > #include <os/availability.h>
> > > >> > > #endif
> > > >> > >
> > > >> > > One more thing though ...
> > > >> > >
> > > >> > > > > > +
> > > >> > > > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t
> > > >> > > > > > mode,
> > > >> > > > > > dev_t
> > > >> > > > >
> > > >> > > > > dev)
> > > >> > > > >
> > > >> > > > > > +{
> > > >> > > > > > +    int preserved_errno, err;
> > > >> > >
> > > >> > > pthread_fchdir_np() is weakly linked. So I guess here should be
> a
> > > >>
> > > >> check
> > > >>
> > > >> > > like:
> > > >> > >         if (!pthread_fchdir_np) {
> > > >> > >
> > > >> > >                 return -ENOTSUPP;
> > > >> > >
> > > >> > >         }
> > > >> > >
> > > >> > > Before trying to call pthread_fchdir_np() below. As already
> > > >> > > discussed
> > > >>
> > > >> with
> > > >>
> > > >> > > the
> > > >> > > Chromium [1] example, some do that a bit differently by using
> > > >> > >
> > > >> > > __builtin_available():
> > > >> > >         if (__builtin_available(macOS 10.12, *)) {
> > > >> > >
> > > >> > >                 return -ENOTSUPP;
> > > >> > >
> > > >> > >         }
> > > >> > >
> > > >> > > Which makes me wonder why they are not doing a simple NULL
> check?
> > > >> > >
> > > >> > > [1]
> > > >>
> > > >>
> https://chromium.googlesource.com/chromium/src/+/lkgr/base/process/laun
> > > >> ch
> > > >> _
> > > >>
> > > >> > > mac.cc#110>
> > > >> > >
> > > >> > > > > > +    if (pthread_fchdir_np(dirfd) < 0) {
> > > >> > > > > > +        return -1;
> > > >> > > > > > +    }
> > > >> > > > > > +    err = mknod(filename, mode, dev);
> > > >> > > > > > +    preserved_errno = errno;
> > > >> > > > > > +    /* Stop using the thread-local cwd */
> > > >> > > > > > +    pthread_fchdir_np(-1);
> > > >> > > > > > +    if (err < 0) {
> > > >> > > > > > +        errno = preserved_errno;
> > > >> > > > > > +    }
> > > >> > > > > > +    return err;
> > > >> > > > > > +}
> > > >> > > > > > +#else
> > > >> > > > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t
> > > >> > > > > > mode,
> > > >> > > > > > dev_t
> > > >> > > > >
> > > >> > > > > dev)
> > > >> > > > >
> > > >> > > > > > +{
> > > >> > > > > > +    return mknodat(dirfd, filename, mode, dev);
> > > >> > > > > > +}
> > > >> > > > > > +#endif
> >
> > Best regards,
> > Christian Schoenebeck
>
>
> Best regards,
> Christian Schoenebeck
>
>
>

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

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

[Akihiko Odaki]

On Wed, Feb 9, 2022 at 7:57 AM Will Cohen <wwcohen@gmail.com> wrote:
>
> On Tue, Feb 8, 2022 at 2:49 PM Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
>>
>> On Dienstag, 8. Februar 2022 19:28:21 CET Christian Schoenebeck wrote:
>> > On Dienstag, 8. Februar 2022 19:04:31 CET Will Cohen wrote:
>> > > On Tue, Feb 8, 2022 at 11:19 AM Will Cohen <wwcohen@gmail.com> wrote:
>> > > > On Tue, Feb 8, 2022 at 11:11 AM Christian Schoenebeck <
>> > > >
>> > > > qemu_oss@crudebyte.com> wrote:
>> > > >> On Dienstag, 8. Februar 2022 16:57:55 CET Will Cohen wrote:
>> > > >> > My inclination is to go with the __builtin_available(macOS 10.12, *)
>> > > >>
>> > > >> path,
>> > > >>
>> > > >> > if acceptable, since it partially mirrors the API_AVAILABLE macro
>> > > >> > idea.
>> > > >>
>> > > >> I
>> > > >>
>> > > >> OTOH that's duplication of the ">= macOS 10.12" info, plus
>> > > >> __builtin_available
>> > > >> is direct use of a clang-only extension, whereas API_AVAILABLE() works
>> > > >> (or
>> > > >> more precisely: doesn't error out at least) with other compilers like
>> > > >> GCC
>> > > >> as
>> > > >> well. GCC is sometimes used for cross-compilation.
>> > > >>
>> > > >> Moreover, I would also add an error message in this case, e.g.:
>> > > >>     if (!pthread_fchdir_np) {
>> > > >>
>> > > >>         error_report_once("pthread_fchdir_np() is not available on this
>> > > >>
>> > > >> macOS version");
>> > > >>
>> > > >>         return -ENOTSUPP;
>> > > >>
>> > > >>     }
>> > > >>
>> > > >> I should elaborate why I think this is needed: you are already doing a
>> > > >> Meson
>> > > >> check for the existence of pthread_fchdir_np(), but the system where
>> > > >> QEMU
>> > > >> is
>> > > >> compiled and the systems where the compiled binary will be running,
>> > > >> might
>> > > >> be
>> > > >> different ones (i.e. different macOS versions).
>> > > >>
>> > > >> Best regards,
>> > > >> Christian Schoenebeck
>> > > >
>> > > > Agreed, that way actually closes the edge case. Something along these
>> > > > lines briefly crossed my mind during a previous version, but I quickly
>> > > > got
>> > > > passed it by assuming that the compiling entity would always be the
>> > > > bottleneck, which makes no sense in hindsight, so I very much appreciate
>> > > > that you caught this.
>> > >
>> > > Ah, rebuilding leads to a compiler error:
>> > >
>> > > ../os-posix.c:348:10: warning: address of function 'pthread_fchdir_np'
>> > > will
>> > > always evaluate to 'true' [-Wpointer-bool-conversion]
>> > >
>> > >     if (!pthread_fchdir_np) {
>> > >
>> > >         ~^~~~~~~~~~~~~~~~~
>> > >
>> > > I don't have a machine that's pre-10.12 so I can't see what the result is
>> > > there, but this might be why the __builtin_available approach got taken.
>> >
>> > I guess that's because you are compiling QEMU with minimum deployment target
>> > being macOS >= 10.12 already. In this case the compiler won't make
>> > pthread_fchdir_np a weak link, it only does emit a weak link if you are
>> > targeting macOS versions prior than the defined availablity attribute,
>> > hence the address would never be NULL here and hence the compiler warning.
>> >
>> > So I guess it is okay if you just omit checking presence of
>> > pthread_fchdir_np at runtime and just assume it exists.
>> >
>> > Added Akihiko on CC, just in case he would have something to add on this
>> > macOS issue here. :)

Hi, Thanks for CCing me.

>>
>> On a second thought: this case a bit special. Are we worried that
>> pthread_fchdir_np() is "not yet" available on macOS, or "no longer" available.
>> Probably both, right?
>>
>> So maybe it would make sense to replace the API_AVAILABLE() attribute directly
>> with a __attribute__((weak)) attribute. Then the runtime check with the
>> proposed error message would also trigger if a bleeding edge macOS version no
>> longer has pthread_fchdir_np().
>>
>> Also keep in mind: there are always also the MAC_OS_X_VERSION_MIN_REQUIRED and
>> MAC_OS_X_VERSION_MAX_ALLOWED macros to query the deployment target at compile
>> time to wrap deployment target dependent code accordingly.
>>
>> On doubt you could just make some tests there by simply "inventing" a non-
>> existent function.
>>
>> Best regards,
>> Christian Schoenebeck
>
>
> I like the idea of switching it to __attribute__((weak)). I should note that I'm not sure that I can actually fully test this out since I'm getting stuck with the linker noting my undefined fake function during the build, but the idea does make logical sense to me for the future fail case and the happy case builds again when implemented with actual pthread_fchdir_np:
>
> [1075/2909] Linking target qemu-nbd
> FAILED: qemu-nbd
> cc -m64 -mcx16  -o qemu-nbd qemu-nbd.p/qemu-nbd.c.o -Wl,-dead_strip_dylibs -Wl,-headerpad_max_install_names -Wl,-undefined,error -Wl,-force_load libblockdev.fa -Wl,-force_load libblock.fa -Wl,-force_load libcrypto.fa -Wl,-force_load libauthz.fa -Wl,-force_load libqom.fa -Wl,-force_load libio.fa -fstack-protector-strong -Wl,-rpath,/usr/local/Cellar/gnutls/3.7.3/lib -Wl,-rpath,/usr/local/Cellar/pixman/0.40.0/lib libqemuutil.a libblockdev.fa libblock.fa libcrypto.fa libauthz.fa libqom.fa libio.fa @block.syms /usr/local/Cellar/gnutls/3.7.3/lib/libgnutls.dylib -lutil -L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib -lgio-2.0 -lgobject-2.0 -lglib-2.0 -lintl -L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib -lgio-2.0 -lgobject-2.0 -lglib-2.0 -lintl -lm -L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib -lgmodule-2.0 -lglib-2.0 -lintl /usr/local/Cellar/pixman/0.40.0/lib/libpixman-1.dylib -lz -lxml2 -framework CoreFoundation -framework IOKit -lcurl -L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib -lgmodule-2.0 -lglib-2.0 -lintl -lbz2 /usr/local/Cellar/libssh/0.9.6/lib/libssh.dylib -lpam
> Undefined symbols for architecture x86_64:
>   "_pthread_fchdir_npfoo", referenced from:
>       _qemu_mknodat in libblockdev.fa(os-posix.c.o)
> ld: symbol(s) not found for architecture x86_64
> clang: error: linker command failed with exit code 1 (use -v to see invocation)
> ninja: build stopped: subcommand failed.
> make[1]: *** [run-ninja] Error 1
> make: *** [all] Error 2
>
> With that caveat re testing in mind, unless there's another recommended path forward, I think it makes sense to stick with __attribute__((weak)) and prepare v6 which incorporates this and all the other feedback from this round.

__attribute__((weak_import)), which explicitly marks the function as
external, is more appropriate here. It is feature-equivalent with the
availability attribute when the minimum deployment target is lower
than the version which introduced the function.

Regards,
Akihiko Odaki

>
>>
>>
>> > > >> > guess it's perhaps a tradeoff between predicting the future unknown
>> > > >> > availability of functions versus just ensuring a minimum macOS
>> > > >> > version
>> > > >>
>> > > >> and
>> > > >>
>> > > >> > hoping for the best. With any luck, the distinction between the two
>> > > >> > approaches will be moot, if we try to assume that a future macOS
>> > > >> > version
>> > > >> > that removes this also provides mknodat.
>> > > >> >
>> > > >> > On Tue, Feb 8, 2022 at 10:03 AM Christian Schoenebeck <
>> > > >> >
>> > > >> > qemu_oss@crudebyte.com> wrote:
>> > > >> > > On Dienstag, 8. Februar 2022 14:36:42 CET Will Cohen wrote:
>> > > >> > > > On Mon, Feb 7, 2022 at 5:56 PM Christian Schoenebeck
>> > > >> > > > <qemu_oss@crudebyte.com>
>> > > >> > > >
>> > > >> > > > wrote:
>> > > >> > > > > On Montag, 7. Februar 2022 23:40:22 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 includes a
>> > > >> > > > > > function
>> > > >> > > > > > that sets the cwd for the current thread only.
>> > > >> > > > > > This should suffice to use mknod safely.
>> > > >> > > > > >
>> > > >> > > > > > This function (pthread_fchdir_np) is protected by a check in
>> > > >> > > > > > meson in a patch later in tihs series.
>> > > >> > > > > >
>> > > >> > > > > > Signed-off-by: Keno Fischer <keno@juliacomputing.com>
>> > > >> > > > > > Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
>> > > >> > > > > > [Will Cohen: - Adjust coding style
>> > > >> > > > > >
>> > > >> > > > > >              - Replace clang references with gcc
>> > > >> > > > > >              - Note radar filed with Apple for missing
>> > > >> > > > > >              syscall
>> > > >> > > > > >              - Replace direct syscall with pthread_fchdir_np
>> > > >> > > > > >              and
>> > > >> > > > > >
>> > > >> > > > > >                adjust patch notes accordingly
>> > > >> > > > > >
>> > > >> > > > > >              - Move qemu_mknodat from 9p-util to osdep and
>> > > >>
>> > > >> os-posix]
>> > > >>
>> > > >> > > > > > Signed-off-by: Will Cohen <wwcohen@gmail.com>
>> > > >> > > > > > ---
>> > > >> > > > >
>> > > >> > > > > Like already mentioned by me moments ago on previous v4 (just
>> > > >>
>> > > >> echoing)
>> > > >>
>> > > >> > > ...
>> > > >> > >
>> > > >> > > > > >  hw/9pfs/9p-local.c   |  4 ++--
>> > > >> > > > > >  include/qemu/osdep.h | 10 ++++++++++
>> > > >> > > > > >  os-posix.c           | 34 ++++++++++++++++++++++++++++++++++
>> > > >> > > > > >  3 files changed, 46 insertions(+), 2 deletions(-)
>> > > >> > > > > >
>> > > >> > > > > > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
>> > > >> > > > > > index a0d08e5216..d42ce6d8b8 100644
>> > > >> > > > > > --- a/hw/9pfs/9p-local.c
>> > > >> > > > > > +++ b/hw/9pfs/9p-local.c
>> > > >> > > > > > @@ -682,7 +682,7 @@ static int local_mknod(FsContext *fs_ctx,
>> > > >> > >
>> > > >> > > V9fsPath
>> > > >> > >
>> > > >> > > > > > *dir_path,
>> > > >> > > > > >
>> > > >> > > > > >      if (fs_ctx->export_flags & V9FS_SM_MAPPED ||
>> > > >> > > > > >
>> > > >> > > > > >          fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
>> > > >> > > > > >
>> > > >> > > > > > -        err = mknodat(dirfd, name, fs_ctx->fmode | S_IFREG,
>> > > >> > > > > > 0);
>> > > >> > > > > > +        err = qemu_mknodat(dirfd, name, fs_ctx->fmode |
>> > > >>
>> > > >> S_IFREG,
>> > > >>
>> > > >> > > > > > 0);
>> > > >> > > > > >
>> > > >> > > > > >          if (err == -1) {
>> > > >> > > > > >
>> > > >> > > > > >              goto out;
>> > > >> > > > > >
>> > > >> > > > > >          }
>> > > >> > > > > >
>> > > >> > > > > > @@ -697,7 +697,7 @@ static int local_mknod(FsContext *fs_ctx,
>> > > >> > >
>> > > >> > > V9fsPath
>> > > >> > >
>> > > >> > > > > > *dir_path, }
>> > > >> > > > > >
>> > > >> > > > > >      } else if (fs_ctx->export_flags & V9FS_SM_PASSTHROUGH ||
>> > > >> > > > > >
>> > > >> > > > > >                 fs_ctx->export_flags & V9FS_SM_NONE) {
>> > > >> > > > > >
>> > > >> > > > > > -        err = mknodat(dirfd, name, credp->fc_mode,
>> > > >>
>> > > >> credp->fc_rdev);
>> > > >>
>> > > >> > > > > > +        err = qemu_mknodat(dirfd, name, credp->fc_mode,
>> > > >> > > > > > credp->fc_rdev);
>> > > >> > > > > >
>> > > >> > > > > >          if (err == -1) {
>> > > >> > > > > >
>> > > >> > > > > >              goto out;
>> > > >> > > > > >
>> > > >> > > > > >          }
>> > > >> > > > > >
>> > > >> > > > > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
>> > > >> > > > > > index d1660d67fa..f3a8367ece 100644
>> > > >> > > > > > --- a/include/qemu/osdep.h
>> > > >> > > > > > +++ b/include/qemu/osdep.h
>> > > >> > > > > > @@ -810,3 +810,13 @@ static inline int
>> > > >> > > > > > platform_does_not_support_system(const char *command) #endif
>> > > >> > > > > >
>> > > >> > > > > >  #endif
>> > > >> > > > > >
>> > > >> > > > > > +
>> > > >> > > > > > +/*
>> > > >> > > > > > + * As long as mknodat is not available on macOS, this
>> > > >>
>> > > >> workaround
>> > > >>
>> > > >> > > > > > + * using pthread_fchdir_np is needed. qemu_mknodat is
>> > > >> > > > > > defined
>> > > >>
>> > > >> in
>> > > >>
>> > > >> > > > > > + * os-posix.c
>> > > >> > > > > > + */
>> > > >> > > > > > +#ifdef CONFIG_DARWIN
>> > > >> > > > > > +int pthread_fchdir_np(int fd);
>> > > >> > > > > > +#endif
>> > > >> > > > >
>> > > >> > > > > I would make that:
>> > > >> > > > >
>> > > >> > > > > #ifdef CONFIG_DARWIN
>> > > >> > > > > int pthread_fchdir_np(int fd) API_AVAILABLE(macosx(10.12));
>> > > >> > > > > #endif
>> > > >> > > > >
>> > > >> > > > > here and ...
>> > > >> > > > >
>> > > >> > > > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t
>> > > >> > > > > > mode,
>> > > >> > > > > > dev_t
>> > > >> > > > >
>> > > >> > > > > dev);
>> > > >> > > > >
>> > > >> > > > > > diff --git a/os-posix.c b/os-posix.c
>> > > >> > > > > > index ae6c9f2a5e..95c1607065 100644
>> > > >> > > > > > --- a/os-posix.c
>> > > >> > > > > > +++ b/os-posix.c
>> > > >> > > > > > @@ -24,6 +24,7 @@
>> > > >> > > > > >
>> > > >> > > > > >   */
>> > > >> > > > > >
>> > > >> > > > > >  #include "qemu/osdep.h"
>> > > >> > > > > >
>> > > >> > > > > > +#include <os/availability.h>
>> > > >> > > > > >
>> > > >> > > > > >  #include <sys/wait.h>
>> > > >> > > > > >  #include <pwd.h>
>> > > >> > > > > >  #include <grp.h>
>> > > >> > > > > >
>> > > >> > > > > > @@ -332,3 +333,36 @@ int os_mlock(void)
>> > > >> > > > > >
>> > > >> > > > > >      return -ENOSYS;
>> > > >> > > > > >
>> > > >> > > > > >  #endif
>> > > >> > > > > >  }
>> > > >> > > > > >
>> > > >> > > > > > +
>> > > >> > > > > > +/*
>> > > >> > > > > > + * As long as mknodat is not available on macOS, this
>> > > >>
>> > > >> workaround
>> > > >>
>> > > >> > > > > > + * using pthread_fchdir_np is needed.
>> > > >> > > > > > + *
>> > > >> > > > > > + * Radar filed with Apple for implementing mknodat:
>> > > >> > > > > > + * rdar://FB9862426
>> > > >> > > > > > (https://openradar.appspot.com/FB9862426)
>> > > >> > > > > > + */
>> > > >> > > > > > +#ifdef CONFIG_DARWIN
>> > > >> > > > > > +
>> > > >> > > > > > +int pthread_fchdir_np(int fd) API_AVAILABLE(macosx(10.12));
>> > > >> > > > >
>> > > >> > > > > ... drop the duplicate declaration of pthread_fchdir_np() here.
>> > > >> > > >
>> > > >> > > > Trying this out, it reminds me that this use of API_AVAILABLE in
>> > > >> > >
>> > > >> > > os-posix.c
>> > > >> > >
>> > > >> > > > relies on the added #include <os/availability.h>.
>> > > >> > > >
>> > > >> > > > Leaving the include out leads to:
>> > > >> > > > .../include/qemu/osdep.h:820:31: error: expected function body
>> > > >> > > > after
>> > > >> > > > function declarator
>> > > >> > > > int pthread_fchdir_np(int fd) API_AVAILABLE(macosx(10.12));
>> > > >> > > >
>> > > >> > > >                               ^
>> > > >> > > >
>> > > >> > > > 1 error generated.
>> > > >> > > > ninja: build stopped: subcommand failed.
>> > > >> > > > make[1]: *** [run-ninja] Error 1
>> > > >> > > > make: *** [all] Error 2
>> > > >> > > >
>> > > >> > > > The admonition against modifying osdep.h's includes too much led
>> > > >> > > > me
>> > > >>
>> > > >> to
>> > > >>
>> > > >> > > > steer away from putting it all in there. If there's no issue with
>> > > >>
>> > > >> adding
>> > > >>
>> > > >> > > > +#include <os/availability.h> to osdep.h, then this change makes
>> > > >>
>> > > >> sense
>> > > >>
>> > > >> > > > to
>> > > >> > > > me.
>> > > >> > >
>> > > >> > > If you embed that include into ifdefs, sure!
>> > > >> > >
>> > > >> > > #ifdef CONFIG_DARWIN
>> > > >> > > /* defines API_AVAILABLE(...) */
>> > > >> > > #include <os/availability.h>
>> > > >> > > #endif
>> > > >> > >
>> > > >> > > One more thing though ...
>> > > >> > >
>> > > >> > > > > > +
>> > > >> > > > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t
>> > > >> > > > > > mode,
>> > > >> > > > > > dev_t
>> > > >> > > > >
>> > > >> > > > > dev)
>> > > >> > > > >
>> > > >> > > > > > +{
>> > > >> > > > > > +    int preserved_errno, err;
>> > > >> > >
>> > > >> > > pthread_fchdir_np() is weakly linked. So I guess here should be a
>> > > >>
>> > > >> check
>> > > >>
>> > > >> > > like:
>> > > >> > >         if (!pthread_fchdir_np) {
>> > > >> > >
>> > > >> > >                 return -ENOTSUPP;
>> > > >> > >
>> > > >> > >         }
>> > > >> > >
>> > > >> > > Before trying to call pthread_fchdir_np() below. As already
>> > > >> > > discussed
>> > > >>
>> > > >> with
>> > > >>
>> > > >> > > the
>> > > >> > > Chromium [1] example, some do that a bit differently by using
>> > > >> > >
>> > > >> > > __builtin_available():
>> > > >> > >         if (__builtin_available(macOS 10.12, *)) {
>> > > >> > >
>> > > >> > >                 return -ENOTSUPP;
>> > > >> > >
>> > > >> > >         }
>> > > >> > >
>> > > >> > > Which makes me wonder why they are not doing a simple NULL check?
>> > > >> > >
>> > > >> > > [1]
>> > > >>
>> > > >> https://chromium.googlesource.com/chromium/src/+/lkgr/base/process/laun
>> > > >> ch
>> > > >> _
>> > > >>
>> > > >> > > mac.cc#110>
>> > > >> > >
>> > > >> > > > > > +    if (pthread_fchdir_np(dirfd) < 0) {
>> > > >> > > > > > +        return -1;
>> > > >> > > > > > +    }
>> > > >> > > > > > +    err = mknod(filename, mode, dev);
>> > > >> > > > > > +    preserved_errno = errno;
>> > > >> > > > > > +    /* Stop using the thread-local cwd */
>> > > >> > > > > > +    pthread_fchdir_np(-1);
>> > > >> > > > > > +    if (err < 0) {
>> > > >> > > > > > +        errno = preserved_errno;
>> > > >> > > > > > +    }
>> > > >> > > > > > +    return err;
>> > > >> > > > > > +}
>> > > >> > > > > > +#else
>> > > >> > > > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t
>> > > >> > > > > > mode,
>> > > >> > > > > > dev_t
>> > > >> > > > >
>> > > >> > > > > dev)
>> > > >> > > > >
>> > > >> > > > > > +{
>> > > >> > > > > > +    return mknodat(dirfd, filename, mode, dev);
>> > > >> > > > > > +}
>> > > >> > > > > > +#endif
>> >
>> > Best regards,
>> > Christian Schoenebeck
>>
>>
>> Best regards,
>> Christian Schoenebeck
>>
>>

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

[Christian Schoenebeck]

On Dienstag, 8. Februar 2022 23:57:32 CET Will Cohen wrote:
> > On a second thought: this case a bit special. Are we worried that
> > pthread_fchdir_np() is "not yet" available on macOS, or "no longer"
> > available.
> > Probably both, right?
> > 
> > So maybe it would make sense to replace the API_AVAILABLE() attribute
> > directly
> > with a __attribute__((weak)) attribute. Then the runtime check with the
> > proposed error message would also trigger if a bleeding edge macOS version
> > no
> > longer has pthread_fchdir_np().
> > 
> > Also keep in mind: there are always also the MAC_OS_X_VERSION_MIN_REQUIRED
> > and
> > MAC_OS_X_VERSION_MAX_ALLOWED macros to query the deployment target at
> > compile
> > time to wrap deployment target dependent code accordingly.
> > 
> > On doubt you could just make some tests there by simply "inventing" a non-
> > existent function.
> > 
> > Best regards,
> > Christian Schoenebeck
> 
> I like the idea of switching it to __attribute__((weak)). I should note
> that I'm not sure that I can actually fully test this out since I'm getting
> stuck with the linker noting my undefined fake function during the build,
> but the idea does make logical sense to me for the future fail case and the
> happy case builds again when implemented with actual pthread_fchdir_np:
> 
> [1075/2909] Linking target qemu-nbd
> FAILED: qemu-nbd
> cc -m64 -mcx16  -o qemu-nbd qemu-nbd.p/qemu-nbd.c.o -Wl,-dead_strip_dylibs
> -Wl,-headerpad_max_install_names -Wl,-undefined,error -Wl,-force_load
> libblockdev.fa -Wl,-force_load libblock.fa -Wl,-force_load libcrypto.fa
> -Wl,-force_load libauthz.fa -Wl,-force_load libqom.fa -Wl,-force_load
> libio.fa -fstack-protector-strong
> -Wl,-rpath,/usr/local/Cellar/gnutls/3.7.3/lib
> -Wl,-rpath,/usr/local/Cellar/pixman/0.40.0/lib libqemuutil.a libblockdev.fa
> libblock.fa libcrypto.fa libauthz.fa libqom.fa libio.fa @block.syms
> /usr/local/Cellar/gnutls/3.7.3/lib/libgnutls.dylib -lutil
> -L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib -lgio-2.0
> -lgobject-2.0 -lglib-2.0 -lintl -L/usr/local/Cellar/glib/2.70.3/lib
> -L/usr/local/opt/gettext/lib -lgio-2.0 -lgobject-2.0 -lglib-2.0 -lintl -lm
> -L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib
> -lgmodule-2.0 -lglib-2.0 -lintl
> /usr/local/Cellar/pixman/0.40.0/lib/libpixman-1.dylib -lz -lxml2 -framework
> CoreFoundation -framework IOKit -lcurl -L/usr/local/Cellar/glib/2.70.3/lib
> -L/usr/local/opt/gettext/lib -lgmodule-2.0 -lglib-2.0 -lintl -lbz2
> /usr/local/Cellar/libssh/0.9.6/lib/libssh.dylib -lpam
> Undefined symbols for architecture x86_64:
>   "_pthread_fchdir_npfoo", referenced from:
>       _qemu_mknodat in libblockdev.fa(os-posix.c.o)
> ld: symbol(s) not found for architecture x86_64
> clang: error: linker command failed with exit code 1 (use -v to see

Even when "weak" linking, the respective symbol must still at least exist at 
compile time. To make this more clear, the following test works for me:

$ cat a.c
#include <stdio.h>

void aaa(void) {
    printf("aaa\n");
}
$ cat ab.c
#include <stdio.h>

void aaa(void) {
    printf("aaa\n");
}

void bbb(void) {
    printf("bbb\n");
}
$ cat main.c
#include <stdio.h>

void aaa(void);
void bbb(void) __attribute__((weak));

int main() {
    aaa();
    if (bbb)
        bbb();
    else
        printf("bbb() not supported\n");
    return 0;
}
$ clang -dynamiclib ab.c -o foo.1.dylib
$ clang main.c -o weaktest foo.1.dylib
$ ./weaktest
aaa
bbb
$ clang -dynamiclib a.c -o foo.1.dylib
$ ./weaktest
aaa
bbb() not supported
$

> With that caveat re testing in mind, unless there's another recommended
> path forward, I think it makes sense to stick with __attribute__((weak))
> and prepare v6 which incorporates this and all the other feedback from this
> round.

Just make this clear with an appropriate comment why it is weakly linked: to 
guard that function pthread_fchdir_np might simply disappear in future macOS 
versions, as it is simply a private API.

Best regards,
Christian Schoenebeck

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

[Christian Schoenebeck]

On Mittwoch, 9. Februar 2022 14:33:25 CET Akihiko Odaki wrote:
> > I like the idea of switching it to __attribute__((weak)). I should note
> > that I'm not sure that I can actually fully test this out since I'm
> > getting stuck with the linker noting my undefined fake function during
> > the build, but the idea does make logical sense to me for the future fail
> > case and the happy case builds again when implemented with actual
> > pthread_fchdir_np:
> > 
> > [1075/2909] Linking target qemu-nbd
> > FAILED: qemu-nbd
> > cc -m64 -mcx16  -o qemu-nbd qemu-nbd.p/qemu-nbd.c.o -Wl,-dead_strip_dylibs
> > -Wl,-headerpad_max_install_names -Wl,-undefined,error -Wl,-force_load
> > libblockdev.fa -Wl,-force_load libblock.fa -Wl,-force_load libcrypto.fa
> > -Wl,-force_load libauthz.fa -Wl,-force_load libqom.fa -Wl,-force_load
> > libio.fa -fstack-protector-strong
> > -Wl,-rpath,/usr/local/Cellar/gnutls/3.7.3/lib
> > -Wl,-rpath,/usr/local/Cellar/pixman/0.40.0/lib libqemuutil.a
> > libblockdev.fa libblock.fa libcrypto.fa libauthz.fa libqom.fa libio.fa
> > @block.syms /usr/local/Cellar/gnutls/3.7.3/lib/libgnutls.dylib -lutil
> > -L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib
> > -lgio-2.0 -lgobject-2.0 -lglib-2.0 -lintl
> > -L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib
> > -lgio-2.0 -lgobject-2.0 -lglib-2.0 -lintl -lm
> > -L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib
> > -lgmodule-2.0 -lglib-2.0 -lintl
> > /usr/local/Cellar/pixman/0.40.0/lib/libpixman-1.dylib -lz -lxml2
> > -framework CoreFoundation -framework IOKit -lcurl
> > -L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib
> > -lgmodule-2.0 -lglib-2.0 -lintl -lbz2
> > /usr/local/Cellar/libssh/0.9.6/lib/libssh.dylib -lpam> 
> > Undefined symbols for architecture x86_64:
> >   "_pthread_fchdir_npfoo", referenced from:
> >       _qemu_mknodat in libblockdev.fa(os-posix.c.o)
> > 
> > ld: symbol(s) not found for architecture x86_64
> > clang: error: linker command failed with exit code 1 (use -v to see
> > invocation) ninja: build stopped: subcommand failed.
> > make[1]: *** [run-ninja] Error 1
> > make: *** [all] Error 2
> > 
> > With that caveat re testing in mind, unless there's another recommended
> > path forward, I think it makes sense to stick with __attribute__((weak))
> > and prepare v6 which incorporates this and all the other feedback from
> > this round.
> __attribute__((weak_import)), which explicitly marks the function as
> external, is more appropriate here. It is feature-equivalent with the
> availability attribute when the minimum deployment target is lower
> than the version which introduced the function.

Thanks for chiming in on this macOS issue Akihiko!

Are you sure that "weak_import" is still the preferred way? From behaviour PoV 
I do not see any difference at all. I can't even tell what the intended 
difference was, and QEMU currently only seems to use "weak" in the entire code 
base.

Googling around, "weak_import" seems to be required on ancient OS X versions 
only and that it's now deprecated in favour of the more common "weak", no?

Best regards,
Christian Schoenebeck

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

[Will Cohen]

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

On Wed, Feb 9, 2022 at 9:08 AM Christian Schoenebeck <qemu_oss@crudebyte.com>
wrote:

> On Mittwoch, 9. Februar 2022 14:33:25 CET Akihiko Odaki wrote:
> > > I like the idea of switching it to __attribute__((weak)). I should note
> > > that I'm not sure that I can actually fully test this out since I'm
> > > getting stuck with the linker noting my undefined fake function during
> > > the build, but the idea does make logical sense to me for the future
> fail
> > > case and the happy case builds again when implemented with actual
> > > pthread_fchdir_np:
> > >
> > > [1075/2909] Linking target qemu-nbd
> > > FAILED: qemu-nbd
> > > cc -m64 -mcx16  -o qemu-nbd qemu-nbd.p/qemu-nbd.c.o
> -Wl,-dead_strip_dylibs
> > > -Wl,-headerpad_max_install_names -Wl,-undefined,error -Wl,-force_load
> > > libblockdev.fa -Wl,-force_load libblock.fa -Wl,-force_load libcrypto.fa
> > > -Wl,-force_load libauthz.fa -Wl,-force_load libqom.fa -Wl,-force_load
> > > libio.fa -fstack-protector-strong
> > > -Wl,-rpath,/usr/local/Cellar/gnutls/3.7.3/lib
> > > -Wl,-rpath,/usr/local/Cellar/pixman/0.40.0/lib libqemuutil.a
> > > libblockdev.fa libblock.fa libcrypto.fa libauthz.fa libqom.fa libio.fa
> > > @block.syms /usr/local/Cellar/gnutls/3.7.3/lib/libgnutls.dylib -lutil
> > > -L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib
> > > -lgio-2.0 -lgobject-2.0 -lglib-2.0 -lintl
> > > -L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib
> > > -lgio-2.0 -lgobject-2.0 -lglib-2.0 -lintl -lm
> > > -L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib
> > > -lgmodule-2.0 -lglib-2.0 -lintl
> > > /usr/local/Cellar/pixman/0.40.0/lib/libpixman-1.dylib -lz -lxml2
> > > -framework CoreFoundation -framework IOKit -lcurl
> > > -L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib
> > > -lgmodule-2.0 -lglib-2.0 -lintl -lbz2
> > > /usr/local/Cellar/libssh/0.9.6/lib/libssh.dylib -lpam>
> > > Undefined symbols for architecture x86_64:
> > >   "_pthread_fchdir_npfoo", referenced from:
> > >       _qemu_mknodat in libblockdev.fa(os-posix.c.o)
> > >
> > > ld: symbol(s) not found for architecture x86_64
> > > clang: error: linker command failed with exit code 1 (use -v to see
> > > invocation) ninja: build stopped: subcommand failed.
> > > make[1]: *** [run-ninja] Error 1
> > > make: *** [all] Error 2
> > >
> > > With that caveat re testing in mind, unless there's another recommended
> > > path forward, I think it makes sense to stick with
> __attribute__((weak))
> > > and prepare v6 which incorporates this and all the other feedback from
> > > this round.
> > __attribute__((weak_import)), which explicitly marks the function as
> > external, is more appropriate here. It is feature-equivalent with the
> > availability attribute when the minimum deployment target is lower
> > than the version which introduced the function.
>
> Thanks for chiming in on this macOS issue Akihiko!
>
> Are you sure that "weak_import" is still the preferred way? From behaviour
> PoV
> I do not see any difference at all. I can't even tell what the intended
> difference was, and QEMU currently only seems to use "weak" in the entire
> code
> base.
>
> Googling around, "weak_import" seems to be required on ancient OS X
> versions
> only and that it's now deprecated in favour of the more common "weak", no?
>
> Best regards,
> Christian Schoenebeck
>

Either way seems reasonable to me. For reference, what I'm seeing on Google
and what Christian may be referring to is a circa-2016 conversation on GCC
bugzilla, where a tentative conclusion seems to be that the distinction
between the two is small and weak is probably preferred now:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69179

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

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

[Akihiko Odaki]

On Thu, Feb 10, 2022 at 3:20 AM Will Cohen <wwcohen@gmail.com> wrote:
>
> On Wed, Feb 9, 2022 at 9:08 AM Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
>>
>> On Mittwoch, 9. Februar 2022 14:33:25 CET Akihiko Odaki wrote:
>> > > I like the idea of switching it to __attribute__((weak)). I should note
>> > > that I'm not sure that I can actually fully test this out since I'm
>> > > getting stuck with the linker noting my undefined fake function during
>> > > the build, but the idea does make logical sense to me for the future fail
>> > > case and the happy case builds again when implemented with actual
>> > > pthread_fchdir_np:
>> > >
>> > > [1075/2909] Linking target qemu-nbd
>> > > FAILED: qemu-nbd
>> > > cc -m64 -mcx16  -o qemu-nbd qemu-nbd.p/qemu-nbd.c.o -Wl,-dead_strip_dylibs
>> > > -Wl,-headerpad_max_install_names -Wl,-undefined,error -Wl,-force_load
>> > > libblockdev.fa -Wl,-force_load libblock.fa -Wl,-force_load libcrypto.fa
>> > > -Wl,-force_load libauthz.fa -Wl,-force_load libqom.fa -Wl,-force_load
>> > > libio.fa -fstack-protector-strong
>> > > -Wl,-rpath,/usr/local/Cellar/gnutls/3.7.3/lib
>> > > -Wl,-rpath,/usr/local/Cellar/pixman/0.40.0/lib libqemuutil.a
>> > > libblockdev.fa libblock.fa libcrypto.fa libauthz.fa libqom.fa libio.fa
>> > > @block.syms /usr/local/Cellar/gnutls/3.7.3/lib/libgnutls.dylib -lutil
>> > > -L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib
>> > > -lgio-2.0 -lgobject-2.0 -lglib-2.0 -lintl
>> > > -L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib
>> > > -lgio-2.0 -lgobject-2.0 -lglib-2.0 -lintl -lm
>> > > -L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib
>> > > -lgmodule-2.0 -lglib-2.0 -lintl
>> > > /usr/local/Cellar/pixman/0.40.0/lib/libpixman-1.dylib -lz -lxml2
>> > > -framework CoreFoundation -framework IOKit -lcurl
>> > > -L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib
>> > > -lgmodule-2.0 -lglib-2.0 -lintl -lbz2
>> > > /usr/local/Cellar/libssh/0.9.6/lib/libssh.dylib -lpam>
>> > > Undefined symbols for architecture x86_64:
>> > >   "_pthread_fchdir_npfoo", referenced from:
>> > >       _qemu_mknodat in libblockdev.fa(os-posix.c.o)
>> > >
>> > > ld: symbol(s) not found for architecture x86_64
>> > > clang: error: linker command failed with exit code 1 (use -v to see
>> > > invocation) ninja: build stopped: subcommand failed.
>> > > make[1]: *** [run-ninja] Error 1
>> > > make: *** [all] Error 2
>> > >
>> > > With that caveat re testing in mind, unless there's another recommended
>> > > path forward, I think it makes sense to stick with __attribute__((weak))
>> > > and prepare v6 which incorporates this and all the other feedback from
>> > > this round.
>> > __attribute__((weak_import)), which explicitly marks the function as
>> > external, is more appropriate here. It is feature-equivalent with the
>> > availability attribute when the minimum deployment target is lower
>> > than the version which introduced the function.
>>
>> Thanks for chiming in on this macOS issue Akihiko!
>>
>> Are you sure that "weak_import" is still the preferred way? From behaviour PoV
>> I do not see any difference at all. I can't even tell what the intended
>> difference was, and QEMU currently only seems to use "weak" in the entire code
>> base.
>>
>> Googling around, "weak_import" seems to be required on ancient OS X versions
>> only and that it's now deprecated in favour of the more common "weak", no?
>>
>> Best regards,
>> Christian Schoenebeck
>
>
> Either way seems reasonable to me. For reference, what I'm seeing on Google and what Christian may be referring to is a circa-2016 conversation on GCC bugzilla, where a tentative conclusion seems to be that the distinction between the two is small and weak is probably preferred now: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69179
>

GCC doesn't maintain features specific to Apple well so we should look
at clang. Compiling QEMU for macOS with GCC would result in errors
anyway because QEMU uses clang extensions like availability checks and
blocks for Apple's ABIs/APIs. clang still distinguishes
__attribute__((weak)) and __attribute__((weak_import)).

The present uses of __attribute__((weak)) in QEMU are correct and
shouldn't be replaced with __attribute__((weak_import)) even when
targeting macOS since they have default implementations and are
statically resolved.

Regards,
Akihiko Odaki

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

[Will Cohen]

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

On Wed, Feb 9, 2022 at 6:10 PM Akihiko Odaki <akihiko.odaki@gmail.com>
wrote:

> On Thu, Feb 10, 2022 at 3:20 AM Will Cohen <wwcohen@gmail.com> wrote:
> >
> > On Wed, Feb 9, 2022 at 9:08 AM Christian Schoenebeck <
> qemu_oss@crudebyte.com> wrote:
> >>
> >> On Mittwoch, 9. Februar 2022 14:33:25 CET Akihiko Odaki wrote:
> >> > > I like the idea of switching it to __attribute__((weak)). I should
> note
> >> > > that I'm not sure that I can actually fully test this out since I'm
> >> > > getting stuck with the linker noting my undefined fake function
> during
> >> > > the build, but the idea does make logical sense to me for the
> future fail
> >> > > case and the happy case builds again when implemented with actual
> >> > > pthread_fchdir_np:
> >> > >
> >> > > [1075/2909] Linking target qemu-nbd
> >> > > FAILED: qemu-nbd
> >> > > cc -m64 -mcx16  -o qemu-nbd qemu-nbd.p/qemu-nbd.c.o
> -Wl,-dead_strip_dylibs
> >> > > -Wl,-headerpad_max_install_names -Wl,-undefined,error
> -Wl,-force_load
> >> > > libblockdev.fa -Wl,-force_load libblock.fa -Wl,-force_load
> libcrypto.fa
> >> > > -Wl,-force_load libauthz.fa -Wl,-force_load libqom.fa
> -Wl,-force_load
> >> > > libio.fa -fstack-protector-strong
> >> > > -Wl,-rpath,/usr/local/Cellar/gnutls/3.7.3/lib
> >> > > -Wl,-rpath,/usr/local/Cellar/pixman/0.40.0/lib libqemuutil.a
> >> > > libblockdev.fa libblock.fa libcrypto.fa libauthz.fa libqom.fa
> libio.fa
> >> > > @block.syms /usr/local/Cellar/gnutls/3.7.3/lib/libgnutls.dylib
> -lutil
> >> > > -L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib
> >> > > -lgio-2.0 -lgobject-2.0 -lglib-2.0 -lintl
> >> > > -L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib
> >> > > -lgio-2.0 -lgobject-2.0 -lglib-2.0 -lintl -lm
> >> > > -L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib
> >> > > -lgmodule-2.0 -lglib-2.0 -lintl
> >> > > /usr/local/Cellar/pixman/0.40.0/lib/libpixman-1.dylib -lz -lxml2
> >> > > -framework CoreFoundation -framework IOKit -lcurl
> >> > > -L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib
> >> > > -lgmodule-2.0 -lglib-2.0 -lintl -lbz2
> >> > > /usr/local/Cellar/libssh/0.9.6/lib/libssh.dylib -lpam>
> >> > > Undefined symbols for architecture x86_64:
> >> > >   "_pthread_fchdir_npfoo", referenced from:
> >> > >       _qemu_mknodat in libblockdev.fa(os-posix.c.o)
> >> > >
> >> > > ld: symbol(s) not found for architecture x86_64
> >> > > clang: error: linker command failed with exit code 1 (use -v to see
> >> > > invocation) ninja: build stopped: subcommand failed.
> >> > > make[1]: *** [run-ninja] Error 1
> >> > > make: *** [all] Error 2
> >> > >
> >> > > With that caveat re testing in mind, unless there's another
> recommended
> >> > > path forward, I think it makes sense to stick with
> __attribute__((weak))
> >> > > and prepare v6 which incorporates this and all the other feedback
> from
> >> > > this round.
> >> > __attribute__((weak_import)), which explicitly marks the function as
> >> > external, is more appropriate here. It is feature-equivalent with the
> >> > availability attribute when the minimum deployment target is lower
> >> > than the version which introduced the function.
> >>
> >> Thanks for chiming in on this macOS issue Akihiko!
> >>
> >> Are you sure that "weak_import" is still the preferred way? From
> behaviour PoV
> >> I do not see any difference at all. I can't even tell what the intended
> >> difference was, and QEMU currently only seems to use "weak" in the
> entire code
> >> base.
> >>
> >> Googling around, "weak_import" seems to be required on ancient OS X
> versions
> >> only and that it's now deprecated in favour of the more common "weak",
> no?
> >>
> >> Best regards,
> >> Christian Schoenebeck
> >
> >
> > Either way seems reasonable to me. For reference, what I'm seeing on
> Google and what Christian may be referring to is a circa-2016 conversation
> on GCC bugzilla, where a tentative conclusion seems to be that the
> distinction between the two is small and weak is probably preferred now:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69179
> >
>
> GCC doesn't maintain features specific to Apple well so we should look
> at clang. Compiling QEMU for macOS with GCC would result in errors
> anyway because QEMU uses clang extensions like availability checks and
> blocks for Apple's ABIs/APIs. clang still distinguishes
> __attribute__((weak)) and __attribute__((weak_import)).
>
> The present uses of __attribute__((weak)) in QEMU are correct and
> shouldn't be replaced with __attribute__((weak_import)) even when
> targeting macOS since they have default implementations and are
> statically resolved.
>
> Regards,
> Akihiko Odaki
>

Noted. v6 adjusts to use weak_import. Many thanks!

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