[PATCH 1/5] 9pfs: fix qemu_mknodat(S_IFREG) on macOS

[PATCH 1/5] 9pfs: fix qemu_mknodat(S_IFREG) on macOS

[Christian Schoenebeck]

mknod() on macOS does not support creating regular files, so
divert to openat_file() if S_IFREG is passed with mode argument.

Furthermore, 'man 2 mknodat' on Linux says: "Zero file type is
equivalent to type S_IFREG".

Link: https://lore.kernel.org/qemu-devel/17933734.zYzKuhC07K@silver/
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 hw/9pfs/9p-util-darwin.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/9pfs/9p-util-darwin.c b/hw/9pfs/9p-util-darwin.c
index bec0253474..53e0625501 100644
--- a/hw/9pfs/9p-util-darwin.c
+++ b/hw/9pfs/9p-util-darwin.c
@@ -77,6 +77,10 @@ int fsetxattrat_nofollow(int dirfd, const char *filename, const char *name,
 int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev)
 {
     int preserved_errno, err;
+
+    if (S_ISREG(mode) || !(mode & S_IFMT)) {
+        return openat_file(dirfd, filename, O_CREAT, mode);
+    }
     if (!pthread_fchdir_np) {
         error_report_once("pthread_fchdir_np() not available on this version of macOS");
         return -ENOTSUP;
-- 
2.32.0 (Apple Git-132)

[PATCH 2/5] 9pfs: fix qemu_mknodat(S_IFSOCK) on macOS

[Christian Schoenebeck]

mknod() on macOS does not support creating sockets, so divert to
call sequence socket(), bind() and chmod() respectively if S_IFSOCK
was passed with mode argument.

Link: https://lore.kernel.org/qemu-devel/17933734.zYzKuhC07K@silver/
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 hw/9pfs/9p-util-darwin.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/hw/9pfs/9p-util-darwin.c b/hw/9pfs/9p-util-darwin.c
index 53e0625501..252a6fc5dd 100644
--- a/hw/9pfs/9p-util-darwin.c
+++ b/hw/9pfs/9p-util-darwin.c
@@ -74,6 +74,24 @@ int fsetxattrat_nofollow(int dirfd, const char *filename, const char *name,
  */
 #if defined CONFIG_PTHREAD_FCHDIR_NP
 
+static int create_socket_file_at_cwd(const char *filename, mode_t mode) {
+    int fd, err;
+    struct sockaddr_un addr = {
+        .sun_family = AF_UNIX
+    };
+
+    fd = socket(PF_UNIX, SOCK_DGRAM, 0);
+    if (fd < 0) {
+        return fd;
+    }
+    snprintf(addr.sun_path, sizeof(addr.sun_path), "./%s", filename);
+    err = bind(fd, (struct sockaddr *) &addr, sizeof(addr));
+    if (err < 0) {
+        return err;
+    }
+    return chmod(addr.sun_path, mode);
+}
+
 int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev)
 {
     int preserved_errno, err;
@@ -88,7 +106,11 @@ int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev)
     if (pthread_fchdir_np(dirfd) < 0) {
         return -1;
     }
-    err = mknod(filename, mode, dev);
+    if (S_ISSOCK(mode)) {
+        err = create_socket_file_at_cwd(filename, mode);
+    } else {
+        err = mknod(filename, mode, dev);
+    }
     preserved_errno = errno;
     /* Stop using the thread-local cwd */
     pthread_fchdir_np(-1);
-- 
2.32.0 (Apple Git-132)

[PATCH 3/5] 9pfs: fix wrong encoding of rdev field in Rgetattr on macOS

[Christian Schoenebeck]

The 'rdev' field in 9p reponse 'Rgetattr' is of type dev_t,
which is actually a system dependant type and therefore both the
size and encoding of dev_t differ between macOS and Linux.

So far we have sent 'rdev' to guest in host's dev_t format as-is,
which caused devices to appear with wrong device numbers on
guests running on macOS hosts, eventually leading to various
misbehaviours on guest in conjunction with device files.

This patch fixes this issue by converting the device number from
host's dev_t format to Linux dev_t format. As 9p request
'Tgettattr' is exclusive to protocol version 9p2000.L, it should
be fair to assume that 'rdev' field is assumed to be in Linux dev_t
format by client as well.

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 hw/9pfs/9p.c | 37 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 225f31fc31..d953035e1c 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1318,6 +1318,41 @@ static int32_t stat_to_iounit(const V9fsPDU *pdu, const struct stat *stbuf)
     return blksize_to_iounit(pdu, stbuf->st_blksize);
 }
 
+#if !defined(CONFIG_LINUX)
+
+/*
+ * Generates a Linux device number (a.k.a. dev_t) for given device major
+ * and minor numbers.
+ */
+static uint64_t makedev_dotl(uint32_t dev_major, uint32_t dev_minor)
+{
+    uint64_t dev;
+
+    // from glibc sysmacros.h:
+    dev  = (((uint64_t) (dev_major & 0x00000fffu)) <<  8);
+    dev |= (((uint64_t) (dev_major & 0xfffff000u)) << 32);
+    dev |= (((uint64_t) (dev_minor & 0x000000ffu)) <<  0);
+    dev |= (((uint64_t) (dev_minor & 0xffffff00u)) << 12);
+    return dev;
+}
+
+#endif
+
+/*
+ * Converts given device number from host's device number format to Linux
+ * device number format. As both the size of type dev_t and encoding of
+ * dev_t is system dependant, we have to convert them for Linux guests if
+ * host is not running Linux.
+ */
+static uint64_t host_dev_to_dotl_dev(dev_t dev)
+{
+#ifdef CONFIG_LINUX
+    return dev;
+#else
+    return makedev_dotl(major(dev), minor(dev));
+#endif
+}
+
 static int stat_to_v9stat_dotl(V9fsPDU *pdu, const struct stat *stbuf,
                                 V9fsStatDotl *v9lstat)
 {
@@ -1327,7 +1362,7 @@ static int stat_to_v9stat_dotl(V9fsPDU *pdu, const struct stat *stbuf,
     v9lstat->st_nlink = stbuf->st_nlink;
     v9lstat->st_uid = stbuf->st_uid;
     v9lstat->st_gid = stbuf->st_gid;
-    v9lstat->st_rdev = stbuf->st_rdev;
+    v9lstat->st_rdev = host_dev_to_dotl_dev(stbuf->st_rdev);
     v9lstat->st_size = stbuf->st_size;
     v9lstat->st_blksize = stat_to_iounit(pdu, stbuf);
     v9lstat->st_blocks = stbuf->st_blocks;
-- 
2.32.0 (Apple Git-132)

[PATCH 4/5] 9pfs: fix wrong errno being sent to Linux client on macOS host

[Christian Schoenebeck]

Linux and macOS only share some errno definitions with equal macro
name and value. In fact most mappings for errno are completely
different on the two systems.

This patch converts some important errno values from macOS host to
corresponding Linux errno values before eventually sending such error
codes along with Tlerror replies (if 9p2000.L is used that is), which
fixes a bunch of misbehaviours when running a Linux client on macOS
host.

For instance this patch fixes:

  mount -t 9p -o posixacl ...

on Linux guest if security_mode=mapped was used for 9p server, which
refused to mount successfully, because macOS returned ENOATTR==93
when client tried to retrieve POSIX ACL xattrs, because errno 93
is defined as EPROTONOSUPPORT==93 on Linux, so Linux client believed
that xattrs were not supported by filesystem on host in general.

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 hw/9pfs/9p.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index d953035e1c..becc41cbfd 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -57,6 +57,31 @@ enum {
 
 P9ARRAY_DEFINE_TYPE(V9fsPath, v9fs_path_free);
 
+/* Translates errno from host -> Linux if needed */
+static int errno_to_dotl(int err) {
+#if defined(CONFIG_LINUX)
+    /* nothing to translate (Linux -> Linux) */
+#elif defined(CONFIG_DARWIN)
+    /* translation mandatory for macOS hosts */
+    if (err == ENAMETOOLONG) {
+        err = 36; /* ==ENAMETOOLONG on Linux */
+    } else if (err == ENOTEMPTY) {
+        err = 39; /* ==ENOTEMPTY on Linux */
+    } else if (err == ELOOP) {
+        err = 40; /* ==ELOOP on Linux */
+    } else if (err == ENOATTR) {
+        err = 61; /* ==ENODATA on Linux */
+    } else if (err == ENOTSUP) {
+        err = 95; /* ==EOPNOTSUPP on Linux */
+    } else if (err == EOPNOTSUPP) {
+        err = 95; /* ==EOPNOTSUPP on Linux */
+    }
+#else
+#error Missing errno translation to Linux for this host system
+#endif
+    return err;
+}
+
 static ssize_t pdu_marshal(V9fsPDU *pdu, size_t offset, const char *fmt, ...)
 {
     ssize_t ret;
@@ -1054,6 +1079,8 @@ static void coroutine_fn pdu_complete(V9fsPDU *pdu, ssize_t len)
             }
             len += ret;
             id = P9_RERROR;
+        } else {
+            err = errno_to_dotl(err);
         }
 
         ret = pdu_marshal(pdu, len, "d", err);
-- 
2.32.0 (Apple Git-132)

[PATCH 5/5] 9pfs: fix removing non-existent POSIX ACL xattr on macOS host

[Christian Schoenebeck]

When mapped POSIX ACL is used, we are ignoring errors when trying
to remove a POSIX ACL xattr that does not exist. On Linux hosts we
would get ENODATA in such cases, on macOS hosts however we get
ENOATTR instead, so ignore ENOATTR errors as well.

This patch fixes e.g. a command on Linux guest like:

  cp --preserve=mode old new

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 hw/9pfs/9p-posix-acl.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/9pfs/9p-posix-acl.c b/hw/9pfs/9p-posix-acl.c
index eadae270dd..2bf155f941 100644
--- a/hw/9pfs/9p-posix-acl.c
+++ b/hw/9pfs/9p-posix-acl.c
@@ -65,7 +65,13 @@ static int mp_pacl_removexattr(FsContext *ctx,
     int ret;
 
     ret = local_removexattr_nofollow(ctx, path, MAP_ACL_ACCESS);
-    if (ret == -1 && errno == ENODATA) {
+    if (ret == -1 &&
+          (errno == ENODATA
+#ifdef ENOATTR
+          || errno == ENOATTR
+#endif
+          )
+    ) {
         /*
          * We don't get ENODATA error when trying to remove a
          * posix acl that is not present. So don't throw the error
-- 
2.32.0 (Apple Git-132)

[PATCH 0/5] 9pfs: macOS host fixes

[Christian Schoenebeck]

A bunch of fixes for recently added (QEMU 7.0) 9p support on macOS hosts.

Note: there are still issues to address with case-insensitive file systems
on macOS hosts. But I'll send a separate RFC on that issue.

Christian Schoenebeck (5):
  9pfs: fix qemu_mknodat(S_IFREG) on macOS
  9pfs: fix qemu_mknodat(S_IFSOCK) on macOS
  9pfs: fix wrong encoding of rdev field in Rgetattr on macOS
  9pfs: fix wrong errno being sent to Linux client on macOS host
  9pfs: fix removing non-existent POSIX ACL xattr on macOS host

 hw/9pfs/9p-posix-acl.c   |  8 ++++-
 hw/9pfs/9p-util-darwin.c | 28 +++++++++++++++++-
 hw/9pfs/9p.c             | 64 +++++++++++++++++++++++++++++++++++++++-
 3 files changed, 97 insertions(+), 3 deletions(-)

-- 
2.32.0 (Apple Git-132)

Re: [PATCH 1/5] 9pfs: fix qemu_mknodat(S_IFREG) on macOS

[Will Cohen]

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

On Tue, Apr 19, 2022 at 8:18 AM Christian Schoenebeck <
qemu_oss@crudebyte.com> wrote:

> mknod() on macOS does not support creating regular files, so
> divert to openat_file() if S_IFREG is passed with mode argument.
>
> Furthermore, 'man 2 mknodat' on Linux says: "Zero file type is
> equivalent to type S_IFREG".
>
> Link: https://lore.kernel.org/qemu-devel/17933734.zYzKuhC07K@silver/
> Signed-off-by
> <https://lore.kernel.org/qemu-devel/17933734.zYzKuhC07K@silver/Signed-off-by>:
> Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---
>  hw/9pfs/9p-util-darwin.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/hw/9pfs/9p-util-darwin.c b/hw/9pfs/9p-util-darwin.c
> index bec0253474..53e0625501 100644
> --- a/hw/9pfs/9p-util-darwin.c
> +++ b/hw/9pfs/9p-util-darwin.c
> @@ -77,6 +77,10 @@ int fsetxattrat_nofollow(int dirfd, const char
> *filename, const char *name,
>  int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev)
>  {
>      int preserved_errno, err;
> +
> +    if (S_ISREG(mode) || !(mode & S_IFMT)) {
> +        return openat_file(dirfd, filename, O_CREAT, mode);
> +    }
>      if (!pthread_fchdir_np) {
>          error_report_once("pthread_fchdir_np() not available on this
> version of macOS");
>          return -ENOTSUP;
> --
> 2.32.0 (Apple Git-132)
>
Reviewed-by: Will Cohen <wwcohen@gmail.com>

>
>

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

Re: [PATCH 2/5] 9pfs: fix qemu_mknodat(S_IFSOCK) on macOS

[Will Cohen]

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

On Tue, Apr 19, 2022 at 8:18 AM Christian Schoenebeck <
qemu_oss@crudebyte.com> wrote:

> mknod() on macOS does not support creating sockets, so divert to
> call sequence socket(), bind() and chmod() respectively if S_IFSOCK
> was passed with mode argument.
>
> Link: https://lore.kernel.org/qemu-devel/17933734.zYzKuhC07K@silver/
> Signed-off-by
> <https://lore.kernel.org/qemu-devel/17933734.zYzKuhC07K@silver/Signed-off-by>:
> Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---
>  hw/9pfs/9p-util-darwin.c | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/hw/9pfs/9p-util-darwin.c b/hw/9pfs/9p-util-darwin.c
> index 53e0625501..252a6fc5dd 100644
> --- a/hw/9pfs/9p-util-darwin.c
> +++ b/hw/9pfs/9p-util-darwin.c
> @@ -74,6 +74,24 @@ int fsetxattrat_nofollow(int dirfd, const char
> *filename, const char *name,
>   */
>  #if defined CONFIG_PTHREAD_FCHDIR_NP
>
> +static int create_socket_file_at_cwd(const char *filename, mode_t mode) {
> +    int fd, err;
> +    struct sockaddr_un addr = {
> +        .sun_family = AF_UNIX
> +    };
> +
> +    fd = socket(PF_UNIX, SOCK_DGRAM, 0);
> +    if (fd < 0) {
> +        return fd;
> +    }
> +    snprintf(addr.sun_path, sizeof(addr.sun_path), "./%s", filename);
> +    err = bind(fd, (struct sockaddr *) &addr, sizeof(addr));
> +    if (err < 0) {
> +        return err;
> +    }
> +    return chmod(addr.sun_path, mode);
> +}
> +
>  int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev)
>  {
>      int preserved_errno, err;
> @@ -88,7 +106,11 @@ int qemu_mknodat(int dirfd, const char *filename,
> mode_t mode, dev_t dev)
>      if (pthread_fchdir_np(dirfd) < 0) {
>          return -1;
>      }
> -    err = mknod(filename, mode, dev);
> +    if (S_ISSOCK(mode)) {
> +        err = create_socket_file_at_cwd(filename, mode);
> +    } else {
> +        err = mknod(filename, mode, dev);
> +    }
>      preserved_errno = errno;
>      /* Stop using the thread-local cwd */
>      pthread_fchdir_np(-1);
> --
> 2.32.0 (Apple Git-132)


> Reviewed-by: Will Cohen <wwcohen@gmail.com>

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

Re: [PATCH 1/5] 9pfs: fix qemu_mknodat(S_IFREG) on macOS

[Akihiko Odaki]

On 2022/04/19 20:40, Christian Schoenebeck wrote:
> mknod() on macOS does not support creating regular files, so
> divert to openat_file() if S_IFREG is passed with mode argument.
> 
> Furthermore, 'man 2 mknodat' on Linux says: "Zero file type is
> equivalent to type S_IFREG".
> 
> Link: https://lore.kernel.org/qemu-devel/17933734.zYzKuhC07K@silver/
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---
>   hw/9pfs/9p-util-darwin.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/hw/9pfs/9p-util-darwin.c b/hw/9pfs/9p-util-darwin.c
> index bec0253474..53e0625501 100644
> --- a/hw/9pfs/9p-util-darwin.c
> +++ b/hw/9pfs/9p-util-darwin.c
> @@ -77,6 +77,10 @@ int fsetxattrat_nofollow(int dirfd, const char *filename, const char *name,
>   int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev)
>   {
>       int preserved_errno, err;
> +
> +    if (S_ISREG(mode) || !(mode & S_IFMT)) {
> +        return openat_file(dirfd, filename, O_CREAT, mode);
> +    }
>       if (!pthread_fchdir_np) {
>           error_report_once("pthread_fchdir_np() not available on this version of macOS");
>           return -ENOTSUP;

openat_file returns a file descriptor on success while mknodat returns 0 
on success. The inconsistency should be handled.

Regards,
Akihiko Odaki

Re: [PATCH 1/5] 9pfs: fix qemu_mknodat(S_IFREG) on macOS

[Greg Kurz]

On Wed, 20 Apr 2022 11:03:52 +0900
Akihiko Odaki <akihiko.odaki@gmail.com> wrote:

> On 2022/04/19 20:40, Christian Schoenebeck wrote:
> > mknod() on macOS does not support creating regular files, so
> > divert to openat_file() if S_IFREG is passed with mode argument.
> > 
> > Furthermore, 'man 2 mknodat' on Linux says: "Zero file type is
> > equivalent to type S_IFREG".
> > 
> > Link: https://lore.kernel.org/qemu-devel/17933734.zYzKuhC07K@silver/
> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > ---
> >   hw/9pfs/9p-util-darwin.c | 4 ++++
> >   1 file changed, 4 insertions(+)
> > 
> > diff --git a/hw/9pfs/9p-util-darwin.c b/hw/9pfs/9p-util-darwin.c
> > index bec0253474..53e0625501 100644
> > --- a/hw/9pfs/9p-util-darwin.c
> > +++ b/hw/9pfs/9p-util-darwin.c
> > @@ -77,6 +77,10 @@ int fsetxattrat_nofollow(int dirfd, const char *filename, const char *name,
> >   int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev)
> >   {
> >       int preserved_errno, err;
> > +
> > +    if (S_ISREG(mode) || !(mode & S_IFMT)) {
> > +        return openat_file(dirfd, filename, O_CREAT, mode);
> > +    }
> >       if (!pthread_fchdir_np) {
> >           error_report_once("pthread_fchdir_np() not available on this version of macOS");
> >           return -ENOTSUP;
> 
> openat_file returns a file descriptor on success while mknodat returns 0 
> on success. The inconsistency should be handled.
> 

And most importantly that file descriptor must be closed !

> Regards,
> Akihiko Odaki

Re: [PATCH 2/5] 9pfs: fix qemu_mknodat(S_IFSOCK) on macOS

[Greg Kurz]

On Tue, 19 Apr 2022 13:41:03 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> mknod() on macOS does not support creating sockets, so divert to
> call sequence socket(), bind() and chmod() respectively if S_IFSOCK
> was passed with mode argument.
> 

Hmm... thinking again about this one : QEMU on linux calls the libc
version of mknodat() which doesn't seem to support S_IFSOCK according
to the mknod(3P) manual page. So I'm not sure there's something to
be actually fixed here... what's the observed behavior on linux ?

> Link: https://lore.kernel.org/qemu-devel/17933734.zYzKuhC07K@silver/
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---
>  hw/9pfs/9p-util-darwin.c | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/9pfs/9p-util-darwin.c b/hw/9pfs/9p-util-darwin.c
> index 53e0625501..252a6fc5dd 100644
> --- a/hw/9pfs/9p-util-darwin.c
> +++ b/hw/9pfs/9p-util-darwin.c
> @@ -74,6 +74,24 @@ int fsetxattrat_nofollow(int dirfd, const char *filename, const char *name,
>   */
>  #if defined CONFIG_PTHREAD_FCHDIR_NP
>  
> +static int create_socket_file_at_cwd(const char *filename, mode_t mode) {
> +    int fd, err;
> +    struct sockaddr_un addr = {
> +        .sun_family = AF_UNIX
> +    };
> +
> +    fd = socket(PF_UNIX, SOCK_DGRAM, 0);
> +    if (fd < 0) {
> +        return fd;
> +    }
> +    snprintf(addr.sun_path, sizeof(addr.sun_path), "./%s", filename);
> +    err = bind(fd, (struct sockaddr *) &addr, sizeof(addr));
> +    if (err < 0) {
> +        return err;
> +    }
> +    return chmod(addr.sun_path, mode);
> +}
> +
>  int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev)
>  {
>      int preserved_errno, err;
> @@ -88,7 +106,11 @@ int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev)
>      if (pthread_fchdir_np(dirfd) < 0) {
>          return -1;
>      }
> -    err = mknod(filename, mode, dev);
> +    if (S_ISSOCK(mode)) {
> +        err = create_socket_file_at_cwd(filename, mode);
> +    } else {
> +        err = mknod(filename, mode, dev);
> +    }
>      preserved_errno = errno;
>      /* Stop using the thread-local cwd */
>      pthread_fchdir_np(-1);

Re: [PATCH 1/5] 9pfs: fix qemu_mknodat(S_IFREG) on macOS

[Christian Schoenebeck]

On Mittwoch, 20. April 2022 10:40:05 CEST Greg Kurz wrote:
> On Wed, 20 Apr 2022 11:03:52 +0900
> 
> Akihiko Odaki <akihiko.odaki@gmail.com> wrote:
> > On 2022/04/19 20:40, Christian Schoenebeck wrote:
> > > mknod() on macOS does not support creating regular files, so
> > > divert to openat_file() if S_IFREG is passed with mode argument.
> > > 
> > > Furthermore, 'man 2 mknodat' on Linux says: "Zero file type is
> > > equivalent to type S_IFREG".
> > > 
> > > Link: https://lore.kernel.org/qemu-devel/17933734.zYzKuhC07K@silver/
> > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > > ---
> > > 
> > >   hw/9pfs/9p-util-darwin.c | 4 ++++
> > >   1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/hw/9pfs/9p-util-darwin.c b/hw/9pfs/9p-util-darwin.c
> > > index bec0253474..53e0625501 100644
> > > --- a/hw/9pfs/9p-util-darwin.c
> > > +++ b/hw/9pfs/9p-util-darwin.c
> > > @@ -77,6 +77,10 @@ int fsetxattrat_nofollow(int dirfd, const char
> > > *filename, const char *name,> > 
> > >   int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t
> > >   dev)
> > >   {
> > >   
> > >       int preserved_errno, err;
> > > 
> > > +
> > > +    if (S_ISREG(mode) || !(mode & S_IFMT)) {
> > > +        return openat_file(dirfd, filename, O_CREAT, mode);
> > > +    }
> > > 
> > >       if (!pthread_fchdir_np) {
> > >       
> > >           error_report_once("pthread_fchdir_np() not available on this
> > >           version of macOS"); return -ENOTSUP;
> > 
> > openat_file returns a file descriptor on success while mknodat returns 0
> > on success. The inconsistency should be handled.
> 
> And most importantly that file descriptor must be closed !

Good spot, both of you! Revising -> v2.

Thanks guys!

Best regards,
Christian Schoenebeck

Re: [PATCH 2/5] 9pfs: fix qemu_mknodat(S_IFSOCK) on macOS

[Christian Schoenebeck]

On Mittwoch, 20. April 2022 11:09:46 CEST Greg Kurz wrote:
> On Tue, 19 Apr 2022 13:41:03 +0200
> 
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > mknod() on macOS does not support creating sockets, so divert to
> > call sequence socket(), bind() and chmod() respectively if S_IFSOCK
> > was passed with mode argument.
> 
> Hmm... thinking again about this one : QEMU on linux calls the libc
> version of mknodat() which doesn't seem to support S_IFSOCK according
> to the mknod(3P) manual page. So I'm not sure there's something to
> be actually fixed here... what's the observed behavior on linux ?

It's unclear to me where you got that from. In all Linux man pages I looked up 
so far it said S_IFSOCK was supported. But I also tested this now with 
security_model=none on a Linux host and it works as expected, i.e. it creates 
a file of type socket on the Linux host filesystem.

We are really talking about a Linux host, right?

Best regards,
Christian Schoenebeck

Re: [PATCH 2/5] 9pfs: fix qemu_mknodat(S_IFSOCK) on macOS

[Greg Kurz]

On Wed, 20 Apr 2022 12:28:01 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Mittwoch, 20. April 2022 11:09:46 CEST Greg Kurz wrote:
> > On Tue, 19 Apr 2022 13:41:03 +0200
> > 
> > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > > mknod() on macOS does not support creating sockets, so divert to
> > > call sequence socket(), bind() and chmod() respectively if S_IFSOCK
> > > was passed with mode argument.
> > 
> > Hmm... thinking again about this one : QEMU on linux calls the libc
> > version of mknodat() which doesn't seem to support S_IFSOCK according
> > to the mknod(3P) manual page. So I'm not sure there's something to
> > be actually fixed here... what's the observed behavior on linux ?
> 
> It's unclear to me where you got that from. In all Linux man pages I looked up 
> so far it said S_IFSOCK was supported. But I also tested this now with 
> security_model=none on a Linux host and it works as expected, i.e. it creates 
> a file of type socket on the Linux host filesystem.
> 
> We are really talking about a Linux host, right?
> 

Yes but you can forget this remark. I've checked the glibc sources
and it directly calls the syscall... I guess I got confused by the
mknod(3P) manual page. Sorry for the noise :-)

> Best regards,
> Christian Schoenebeck
> 
> 

Re: [PATCH 2/5] 9pfs: fix qemu_mknodat(S_IFSOCK) on macOS

[Greg Kurz]

On Tue, 19 Apr 2022 13:41:03 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> mknod() on macOS does not support creating sockets, so divert to
> call sequence socket(), bind() and chmod() respectively if S_IFSOCK
> was passed with mode argument.
> 
> Link: https://lore.kernel.org/qemu-devel/17933734.zYzKuhC07K@silver/
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---
>  hw/9pfs/9p-util-darwin.c | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/9pfs/9p-util-darwin.c b/hw/9pfs/9p-util-darwin.c
> index 53e0625501..252a6fc5dd 100644
> --- a/hw/9pfs/9p-util-darwin.c
> +++ b/hw/9pfs/9p-util-darwin.c
> @@ -74,6 +74,24 @@ int fsetxattrat_nofollow(int dirfd, const char *filename, const char *name,
>   */
>  #if defined CONFIG_PTHREAD_FCHDIR_NP
>  
> +static int create_socket_file_at_cwd(const char *filename, mode_t mode) {
> +    int fd, err;
> +    struct sockaddr_un addr = {
> +        .sun_family = AF_UNIX
> +    };
> +
> +    fd = socket(PF_UNIX, SOCK_DGRAM, 0);
> +    if (fd < 0) {
> +        return fd;
> +    }
> +    snprintf(addr.sun_path, sizeof(addr.sun_path), "./%s", filename);
> +    err = bind(fd, (struct sockaddr *) &addr, sizeof(addr));
> +    if (err < 0) {
> +        return err;
> +    }
> +    return chmod(addr.sun_path, mode);

As with the other patch, you should close() the socket.

> +}
> +
>  int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev)
>  {
>      int preserved_errno, err;
> @@ -88,7 +106,11 @@ int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev)
>      if (pthread_fchdir_np(dirfd) < 0) {
>          return -1;
>      }
> -    err = mknod(filename, mode, dev);
> +    if (S_ISSOCK(mode)) {
> +        err = create_socket_file_at_cwd(filename, mode);
> +    } else {
> +        err = mknod(filename, mode, dev);
> +    }
>      preserved_errno = errno;
>      /* Stop using the thread-local cwd */
>      pthread_fchdir_np(-1);

Re: [PATCH 2/5] 9pfs: fix qemu_mknodat(S_IFSOCK) on macOS

[Christian Schoenebeck]

On Mittwoch, 20. April 2022 14:10:44 CEST Greg Kurz wrote:
> On Tue, 19 Apr 2022 13:41:03 +0200
> 
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > mknod() on macOS does not support creating sockets, so divert to
> > call sequence socket(), bind() and chmod() respectively if S_IFSOCK
> > was passed with mode argument.
> > 
> > Link: https://lore.kernel.org/qemu-devel/17933734.zYzKuhC07K@silver/
> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > ---
> > 
> >  hw/9pfs/9p-util-darwin.c | 24 +++++++++++++++++++++++-
> >  1 file changed, 23 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/9pfs/9p-util-darwin.c b/hw/9pfs/9p-util-darwin.c
> > index 53e0625501..252a6fc5dd 100644
> > --- a/hw/9pfs/9p-util-darwin.c
> > +++ b/hw/9pfs/9p-util-darwin.c
> > @@ -74,6 +74,24 @@ int fsetxattrat_nofollow(int dirfd, const char
> > *filename, const char *name,> 
> >   */
> >  
> >  #if defined CONFIG_PTHREAD_FCHDIR_NP
> > 
> > +static int create_socket_file_at_cwd(const char *filename, mode_t mode) {
> > +    int fd, err;
> > +    struct sockaddr_un addr = {
> > +        .sun_family = AF_UNIX
> > +    };
> > +
> > +    fd = socket(PF_UNIX, SOCK_DGRAM, 0);
> > +    if (fd < 0) {
> > +        return fd;
> > +    }
> > +    snprintf(addr.sun_path, sizeof(addr.sun_path), "./%s", filename);
> > +    err = bind(fd, (struct sockaddr *) &addr, sizeof(addr));
> > +    if (err < 0) {
> > +        return err;
> > +    }
> > +    return chmod(addr.sun_path, mode);
> 
> As with the other patch, you should close() the socket.

I was concerned that close(fd) might cause the socket file to disappear. But I 
just tested this now on macOS and it works fine. So will be fixed in v2.

Thanks!

Best regards,
Christian Schoenebeck

Re: [PATCH 3/5] 9pfs: fix wrong encoding of rdev field in Rgetattr on macOS

[Greg Kurz]

On Tue, 19 Apr 2022 13:41:15 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> The 'rdev' field in 9p reponse 'Rgetattr' is of type dev_t,
> which is actually a system dependant type and therefore both the
> size and encoding of dev_t differ between macOS and Linux.
> 
> So far we have sent 'rdev' to guest in host's dev_t format as-is,
> which caused devices to appear with wrong device numbers on
> guests running on macOS hosts, eventually leading to various
> misbehaviours on guest in conjunction with device files.
> 
> This patch fixes this issue by converting the device number from
> host's dev_t format to Linux dev_t format. As 9p request
> 'Tgettattr' is exclusive to protocol version 9p2000.L, it should
> be fair to assume that 'rdev' field is assumed to be in Linux dev_t
> format by client as well.
> 

For the sake of accuracy : this is converting the host's dev_t to glibc's
format (MMMM_Mmmm_mmmM_MMmm, 64 bits) actually, which is compatible with
linux's format (mmmM_MMmm, 32 bits), as described in <bits/sysmacros.h>.

> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---
>  hw/9pfs/9p.c | 37 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 225f31fc31..d953035e1c 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -1318,6 +1318,41 @@ static int32_t stat_to_iounit(const V9fsPDU *pdu, const struct stat *stbuf)
>      return blksize_to_iounit(pdu, stbuf->st_blksize);
>  }
>  
> +#if !defined(CONFIG_LINUX)
> +
> +/*
> + * Generates a Linux device number (a.k.a. dev_t) for given device major
> + * and minor numbers.
> + */
> +static uint64_t makedev_dotl(uint32_t dev_major, uint32_t dev_minor)
> +{
> +    uint64_t dev;
> +
> +    // from glibc sysmacros.h:
> +    dev  = (((uint64_t) (dev_major & 0x00000fffu)) <<  8);
> +    dev |= (((uint64_t) (dev_major & 0xfffff000u)) << 32);
> +    dev |= (((uint64_t) (dev_minor & 0x000000ffu)) <<  0);
> +    dev |= (((uint64_t) (dev_minor & 0xffffff00u)) << 12);
> +    return dev;
> +}
> +
> +#endif
> +
> +/*
> + * Converts given device number from host's device number format to Linux
> + * device number format. As both the size of type dev_t and encoding of
> + * dev_t is system dependant, we have to convert them for Linux guests if
> + * host is not running Linux.
> + */
> +static uint64_t host_dev_to_dotl_dev(dev_t dev)
> +{
> +#ifdef CONFIG_LINUX
> +    return dev;
> +#else
> +    return makedev_dotl(major(dev), minor(dev));
> +#endif
> +}
> +

It is a bit unfortunate to inflate 9p.c, which is large enough, with
glue code. Even if they are only used here, I'd personally put them
in 9p-util.h. No big deal.

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

>  static int stat_to_v9stat_dotl(V9fsPDU *pdu, const struct stat *stbuf,
>                                  V9fsStatDotl *v9lstat)
>  {
> @@ -1327,7 +1362,7 @@ static int stat_to_v9stat_dotl(V9fsPDU *pdu, const struct stat *stbuf,
>      v9lstat->st_nlink = stbuf->st_nlink;
>      v9lstat->st_uid = stbuf->st_uid;
>      v9lstat->st_gid = stbuf->st_gid;
> -    v9lstat->st_rdev = stbuf->st_rdev;
> +    v9lstat->st_rdev = host_dev_to_dotl_dev(stbuf->st_rdev);
>      v9lstat->st_size = stbuf->st_size;
>      v9lstat->st_blksize = stat_to_iounit(pdu, stbuf);
>      v9lstat->st_blocks = stbuf->st_blocks;

Re: [PATCH 5/5] 9pfs: fix removing non-existent POSIX ACL xattr on macOS host

[Greg Kurz]

On Tue, 19 Apr 2022 13:43:30 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> When mapped POSIX ACL is used, we are ignoring errors when trying
> to remove a POSIX ACL xattr that does not exist. On Linux hosts we
> would get ENODATA in such cases, on macOS hosts however we get
> ENOATTR instead, so ignore ENOATTR errors as well.
> 
> This patch fixes e.g. a command on Linux guest like:
> 
>   cp --preserve=mode old new
> 
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---
>  hw/9pfs/9p-posix-acl.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/9pfs/9p-posix-acl.c b/hw/9pfs/9p-posix-acl.c
> index eadae270dd..2bf155f941 100644
> --- a/hw/9pfs/9p-posix-acl.c
> +++ b/hw/9pfs/9p-posix-acl.c
> @@ -65,7 +65,13 @@ static int mp_pacl_removexattr(FsContext *ctx,
>      int ret;
>  
>      ret = local_removexattr_nofollow(ctx, path, MAP_ACL_ACCESS);
> -    if (ret == -1 && errno == ENODATA) {
> +    if (ret == -1 &&
> +          (errno == ENODATA
> +#ifdef ENOATTR
> +          || errno == ENOATTR
> +#endif
> +          )

We already have this in <qemu/xattr.h> which is included by
9p-posix-acl.c :

/*
 * Modern distributions (e.g. Fedora 15), have no libattr.so, place attr.h
 * in /usr/include/sys, and don't have ENOATTR.
 */


#ifdef CONFIG_LIBATTR
#  include <attr/xattr.h>
#else
#  if !defined(ENOATTR)
#    define ENOATTR ENODATA
#  endif
#  include <sys/xattr.h>
#endif

I guess this patch could just s/ENODATA/ENOATTR/ to avoid the
extra ifdefery.

> +    ) {
>          /*
>           * We don't get ENODATA error when trying to remove a
>           * posix acl that is not present. So don't throw the error

Re: [PATCH 3/5] 9pfs: fix wrong encoding of rdev field in Rgetattr on macOS

[Christian Schoenebeck]

On Donnerstag, 21. April 2022 09:30:56 CEST Greg Kurz wrote:
> On Tue, 19 Apr 2022 13:41:15 +0200
> 
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > The 'rdev' field in 9p reponse 'Rgetattr' is of type dev_t,
> > which is actually a system dependant type and therefore both the
> > size and encoding of dev_t differ between macOS and Linux.
> > 
> > So far we have sent 'rdev' to guest in host's dev_t format as-is,
> > which caused devices to appear with wrong device numbers on
> > guests running on macOS hosts, eventually leading to various
> > misbehaviours on guest in conjunction with device files.
> > 
> > This patch fixes this issue by converting the device number from
> > host's dev_t format to Linux dev_t format. As 9p request
> > 'Tgettattr' is exclusive to protocol version 9p2000.L, it should
> > be fair to assume that 'rdev' field is assumed to be in Linux dev_t
> > format by client as well.
> 
> For the sake of accuracy : this is converting the host's dev_t to glibc's
> format (MMMM_Mmmm_mmmM_MMmm, 64 bits) actually, which is compatible with
> linux's format (mmmM_MMmm, 32 bits), as described in <bits/sysmacros.h>.

You want me to put this to the commit log?

> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > ---
> > 
> >  hw/9pfs/9p.c | 37 ++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 36 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > index 225f31fc31..d953035e1c 100644
> > --- a/hw/9pfs/9p.c
> > +++ b/hw/9pfs/9p.c
> > @@ -1318,6 +1318,41 @@ static int32_t stat_to_iounit(const V9fsPDU *pdu,
> > const struct stat *stbuf)> 
> >      return blksize_to_iounit(pdu, stbuf->st_blksize);
> >  
> >  }
> > 
> > +#if !defined(CONFIG_LINUX)
> > +
> > +/*
> > + * Generates a Linux device number (a.k.a. dev_t) for given device major
> > + * and minor numbers.
> > + */
> > +static uint64_t makedev_dotl(uint32_t dev_major, uint32_t dev_minor)
> > +{
> > +    uint64_t dev;
> > +
> > +    // from glibc sysmacros.h:
> > +    dev  = (((uint64_t) (dev_major & 0x00000fffu)) <<  8);
> > +    dev |= (((uint64_t) (dev_major & 0xfffff000u)) << 32);
> > +    dev |= (((uint64_t) (dev_minor & 0x000000ffu)) <<  0);
> > +    dev |= (((uint64_t) (dev_minor & 0xffffff00u)) << 12);
> > +    return dev;
> > +}
> > +
> > +#endif
> > +
> > +/*
> > + * Converts given device number from host's device number format to Linux
> > + * device number format. As both the size of type dev_t and encoding of
> > + * dev_t is system dependant, we have to convert them for Linux guests if
> > + * host is not running Linux.
> > + */
> > +static uint64_t host_dev_to_dotl_dev(dev_t dev)
> > +{
> > +#ifdef CONFIG_LINUX
> > +    return dev;
> > +#else
> > +    return makedev_dotl(major(dev), minor(dev));
> > +#endif
> > +}
> > +
> 
> It is a bit unfortunate to inflate 9p.c, which is large enough, with
> glue code. Even if they are only used here, I'd personally put them
> in 9p-util.h. No big deal.

Makes sense, I'll move it.

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

Thanks!

> >  static int stat_to_v9stat_dotl(V9fsPDU *pdu, const struct stat *stbuf,
> >  
> >                                  V9fsStatDotl *v9lstat)
> >  
> >  {
> > 
> > @@ -1327,7 +1362,7 @@ static int stat_to_v9stat_dotl(V9fsPDU *pdu, const
> > struct stat *stbuf,> 
> >      v9lstat->st_nlink = stbuf->st_nlink;
> >      v9lstat->st_uid = stbuf->st_uid;
> >      v9lstat->st_gid = stbuf->st_gid;
> > 
> > -    v9lstat->st_rdev = stbuf->st_rdev;
> > +    v9lstat->st_rdev = host_dev_to_dotl_dev(stbuf->st_rdev);
> > 
> >      v9lstat->st_size = stbuf->st_size;
> >      v9lstat->st_blksize = stat_to_iounit(pdu, stbuf);
> >      v9lstat->st_blocks = stbuf->st_blocks;

Re: [PATCH 3/5] 9pfs: fix wrong encoding of rdev field in Rgetattr on macOS

[Greg Kurz]

On Thu, 21 Apr 2022 12:25:23 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Donnerstag, 21. April 2022 09:30:56 CEST Greg Kurz wrote:
> > On Tue, 19 Apr 2022 13:41:15 +0200
> > 
> > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > > The 'rdev' field in 9p reponse 'Rgetattr' is of type dev_t,
> > > which is actually a system dependant type and therefore both the
> > > size and encoding of dev_t differ between macOS and Linux.
> > > 
> > > So far we have sent 'rdev' to guest in host's dev_t format as-is,
> > > which caused devices to appear with wrong device numbers on
> > > guests running on macOS hosts, eventually leading to various
> > > misbehaviours on guest in conjunction with device files.
> > > 
> > > This patch fixes this issue by converting the device number from
> > > host's dev_t format to Linux dev_t format. As 9p request
> > > 'Tgettattr' is exclusive to protocol version 9p2000.L, it should
> > > be fair to assume that 'rdev' field is assumed to be in Linux dev_t
> > > format by client as well.
> > 
> > For the sake of accuracy : this is converting the host's dev_t to glibc's
> > format (MMMM_Mmmm_mmmM_MMmm, 64 bits) actually, which is compatible with
> > linux's format (mmmM_MMmm, 32 bits), as described in <bits/sysmacros.h>.
> 
> You want me to put this to the commit log?
> 

Maybe closer to the code that does the actual magic...

> > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > > ---
> > > 
> > >  hw/9pfs/9p.c | 37 ++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 36 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > > index 225f31fc31..d953035e1c 100644
> > > --- a/hw/9pfs/9p.c
> > > +++ b/hw/9pfs/9p.c
> > > @@ -1318,6 +1318,41 @@ static int32_t stat_to_iounit(const V9fsPDU *pdu,
> > > const struct stat *stbuf)> 
> > >      return blksize_to_iounit(pdu, stbuf->st_blksize);
> > >  
> > >  }
> > > 
> > > +#if !defined(CONFIG_LINUX)
> > > +
> > > +/*
> > > + * Generates a Linux device number (a.k.a. dev_t) for given device major
> > > + * and minor numbers.

... here ^^.

> > > + */
> > > +static uint64_t makedev_dotl(uint32_t dev_major, uint32_t dev_minor)
> > > +{
> > > +    uint64_t dev;
> > > +
> > > +    // from glibc sysmacros.h:
> > > +    dev  = (((uint64_t) (dev_major & 0x00000fffu)) <<  8);
> > > +    dev |= (((uint64_t) (dev_major & 0xfffff000u)) << 32);
> > > +    dev |= (((uint64_t) (dev_minor & 0x000000ffu)) <<  0);
> > > +    dev |= (((uint64_t) (dev_minor & 0xffffff00u)) << 12);
> > > +    return dev;
> > > +}
> > > +
> > > +#endif
> > > +
> > > +/*
> > > + * Converts given device number from host's device number format to Linux
> > > + * device number format. As both the size of type dev_t and encoding of
> > > + * dev_t is system dependant, we have to convert them for Linux guests if
> > > + * host is not running Linux.
> > > + */
> > > +static uint64_t host_dev_to_dotl_dev(dev_t dev)
> > > +{
> > > +#ifdef CONFIG_LINUX
> > > +    return dev;
> > > +#else
> > > +    return makedev_dotl(major(dev), minor(dev));
> > > +#endif
> > > +}
> > > +
> > 
> > It is a bit unfortunate to inflate 9p.c, which is large enough, with
> > glue code. Even if they are only used here, I'd personally put them
> > in 9p-util.h. No big deal.
> 
> Makes sense, I'll move it.
> 
> > Reviewed-by: Greg Kurz <groug@kaod.org>
> 
> Thanks!
> 
> > >  static int stat_to_v9stat_dotl(V9fsPDU *pdu, const struct stat *stbuf,
> > >  
> > >                                  V9fsStatDotl *v9lstat)
> > >  
> > >  {
> > > 
> > > @@ -1327,7 +1362,7 @@ static int stat_to_v9stat_dotl(V9fsPDU *pdu, const
> > > struct stat *stbuf,> 
> > >      v9lstat->st_nlink = stbuf->st_nlink;
> > >      v9lstat->st_uid = stbuf->st_uid;
> > >      v9lstat->st_gid = stbuf->st_gid;
> > > 
> > > -    v9lstat->st_rdev = stbuf->st_rdev;
> > > +    v9lstat->st_rdev = host_dev_to_dotl_dev(stbuf->st_rdev);
> > > 
> > >      v9lstat->st_size = stbuf->st_size;
> > >      v9lstat->st_blksize = stat_to_iounit(pdu, stbuf);
> > >      v9lstat->st_blocks = stbuf->st_blocks;
> 
> 

Re: [PATCH 4/5] 9pfs: fix wrong errno being sent to Linux client on macOS host

[Greg Kurz]

On Tue, 19 Apr 2022 13:41:59 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> Linux and macOS only share some errno definitions with equal macro
> name and value. In fact most mappings for errno are completely
> different on the two systems.
> 
> This patch converts some important errno values from macOS host to
> corresponding Linux errno values before eventually sending such error
> codes along with Tlerror replies (if 9p2000.L is used that is), which
> fixes a bunch of misbehaviours when running a Linux client on macOS
> host.
> 

This even fixes an actual protocol violation :

lerror -- return error code

size[4] Rlerror tag[2] ecode[4]

lerror replaces the reply message used in a successful call. ecode is a
numerical Linux errno.
^^^^^^^^^^^^^^^^^^^^^

Taken from https://github.com/chaos/diod/wiki/protocol#lerror----return-error-code


> For instance this patch fixes:
> 
>   mount -t 9p -o posixacl ...
> 
> on Linux guest if security_mode=mapped was used for 9p server, which
> refused to mount successfully, because macOS returned ENOATTR==93
> when client tried to retrieve POSIX ACL xattrs, because errno 93
> is defined as EPROTONOSUPPORT==93 on Linux, so Linux client believed
> that xattrs were not supported by filesystem on host in general.
> 
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---
>  hw/9pfs/9p.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index d953035e1c..becc41cbfd 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -57,6 +57,31 @@ enum {
>  
>  P9ARRAY_DEFINE_TYPE(V9fsPath, v9fs_path_free);
>  
> +/* Translates errno from host -> Linux if needed */
> +static int errno_to_dotl(int err) {
> +#if defined(CONFIG_LINUX)
> +    /* nothing to translate (Linux -> Linux) */
> +#elif defined(CONFIG_DARWIN)
> +    /* translation mandatory for macOS hosts */
> +    if (err == ENAMETOOLONG) {
> +        err = 36; /* ==ENAMETOOLONG on Linux */
> +    } else if (err == ENOTEMPTY) {
> +        err = 39; /* ==ENOTEMPTY on Linux */
> +    } else if (err == ELOOP) {
> +        err = 40; /* ==ELOOP on Linux */
> +    } else if (err == ENOATTR) {
> +        err = 61; /* ==ENODATA on Linux */
> +    } else if (err == ENOTSUP) {
> +        err = 95; /* ==EOPNOTSUPP on Linux */
> +    } else if (err == EOPNOTSUPP) {
> +        err = 95; /* ==EOPNOTSUPP on Linux */
> +    }

I'm assuming you have audited all errnos, right ? Just to be sure
that this won't bite anymore.

> +#else
> +#error Missing errno translation to Linux for this host system
> +#endif
> +    return err;
> +}
> +

As with the other patch, I'd rather move this magic to 9p-util.h .

>  static ssize_t pdu_marshal(V9fsPDU *pdu, size_t offset, const char *fmt, ...)
>  {
>      ssize_t ret;
> @@ -1054,6 +1079,8 @@ static void coroutine_fn pdu_complete(V9fsPDU *pdu, ssize_t len)
>              }
>              len += ret;
>              id = P9_RERROR;
> +        } else {
> +            err = errno_to_dotl(err);
>          }
>  
>          ret = pdu_marshal(pdu, len, "d", err);

Re: [PATCH 5/5] 9pfs: fix removing non-existent POSIX ACL xattr on macOS host

[Christian Schoenebeck]

On Donnerstag, 21. April 2022 10:26:11 CEST Greg Kurz wrote:
> On Tue, 19 Apr 2022 13:43:30 +0200
> 
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > When mapped POSIX ACL is used, we are ignoring errors when trying
> > to remove a POSIX ACL xattr that does not exist. On Linux hosts we
> > would get ENODATA in such cases, on macOS hosts however we get
> > ENOATTR instead, so ignore ENOATTR errors as well.
> > 
> > This patch fixes e.g. a command on Linux guest like:
> >   cp --preserve=mode old new
> > 
> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > ---
> > 
> >  hw/9pfs/9p-posix-acl.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/9pfs/9p-posix-acl.c b/hw/9pfs/9p-posix-acl.c
> > index eadae270dd..2bf155f941 100644
> > --- a/hw/9pfs/9p-posix-acl.c
> > +++ b/hw/9pfs/9p-posix-acl.c
> > @@ -65,7 +65,13 @@ static int mp_pacl_removexattr(FsContext *ctx,
> > 
> >      int ret;
> >      
> >      ret = local_removexattr_nofollow(ctx, path, MAP_ACL_ACCESS);
> > 
> > -    if (ret == -1 && errno == ENODATA) {
> > +    if (ret == -1 &&
> > +          (errno == ENODATA
> > +#ifdef ENOATTR
> > +          || errno == ENOATTR
> > +#endif
> > +          )
> 
> We already have this in <qemu/xattr.h> which is included by
> 9p-posix-acl.c :
> 
> /*
>  * Modern distributions (e.g. Fedora 15), have no libattr.so, place attr.h
>  * in /usr/include/sys, and don't have ENOATTR.
>  */
> 
> 
> #ifdef CONFIG_LIBATTR
> #  include <attr/xattr.h>
> #else
> #  if !defined(ENOATTR)
> #    define ENOATTR ENODATA
> #  endif
> #  include <sys/xattr.h>
> #endif
> 
> I guess this patch could just s/ENODATA/ENOATTR/ to avoid the
> extra ifdefery.

Not viable, because macOS does have both ENODATA==96 and ENOATTR==93. On Linux 
the two macros were historically defined to the same numeric values, that's 
why it worked there.

Maybe I should define a separate macro like:

#if ...
# define P9_ENOATTR ENOATTR
#else
# define P9_ENOATTR ENODATA
#end

?

Actually good that you pointed me at this, because I just realized there is a 
2nd place in 9p-posix-acl.c which would require this as well. For some reason 
the 2nd place just did not trigger while I was testing it on macOS.

Best regards,
Christian Schoenebeck

Re: [PATCH 4/5] 9pfs: fix wrong errno being sent to Linux client on macOS host

[Christian Schoenebeck]

On Donnerstag, 21. April 2022 12:48:35 CEST Greg Kurz wrote:
> On Tue, 19 Apr 2022 13:41:59 +0200
> 
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > Linux and macOS only share some errno definitions with equal macro
> > name and value. In fact most mappings for errno are completely
> > different on the two systems.
> > 
> > This patch converts some important errno values from macOS host to
> > corresponding Linux errno values before eventually sending such error
> > codes along with Tlerror replies (if 9p2000.L is used that is), which
> > fixes a bunch of misbehaviours when running a Linux client on macOS
> > host.
> 
> This even fixes an actual protocol violation :
> 
> lerror -- return error code
> 
> size[4] Rlerror tag[2] ecode[4]
> 
> lerror replaces the reply message used in a successful call. ecode is a
> numerical Linux errno.
> ^^^^^^^^^^^^^^^^^^^^^
> 
> Taken from
> https://github.com/chaos/diod/wiki/protocol#lerror----return-error-code

Again, something to add to the commit log?

> > For instance this patch fixes:
> >   mount -t 9p -o posixacl ...
> > 
> > on Linux guest if security_mode=mapped was used for 9p server, which
> > refused to mount successfully, because macOS returned ENOATTR==93
> > when client tried to retrieve POSIX ACL xattrs, because errno 93
> > is defined as EPROTONOSUPPORT==93 on Linux, so Linux client believed
> > that xattrs were not supported by filesystem on host in general.
> > 
> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > ---
> > 
> >  hw/9pfs/9p.c | 27 +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> > 
> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > index d953035e1c..becc41cbfd 100644
> > --- a/hw/9pfs/9p.c
> > +++ b/hw/9pfs/9p.c
> > @@ -57,6 +57,31 @@ enum {
> > 
> >  P9ARRAY_DEFINE_TYPE(V9fsPath, v9fs_path_free);
> > 
> > +/* Translates errno from host -> Linux if needed */
> > +static int errno_to_dotl(int err) {
> > +#if defined(CONFIG_LINUX)
> > +    /* nothing to translate (Linux -> Linux) */
> > +#elif defined(CONFIG_DARWIN)
> > +    /* translation mandatory for macOS hosts */
> > +    if (err == ENAMETOOLONG) {
> > +        err = 36; /* ==ENAMETOOLONG on Linux */
> > +    } else if (err == ENOTEMPTY) {
> > +        err = 39; /* ==ENOTEMPTY on Linux */
> > +    } else if (err == ELOOP) {
> > +        err = 40; /* ==ELOOP on Linux */
> > +    } else if (err == ENOATTR) {
> > +        err = 61; /* ==ENODATA on Linux */
> > +    } else if (err == ENOTSUP) {
> > +        err = 95; /* ==EOPNOTSUPP on Linux */
> > +    } else if (err == EOPNOTSUPP) {
> > +        err = 95; /* ==EOPNOTSUPP on Linux */
> > +    }
> 
> I'm assuming you have audited all errnos, right ? Just to be sure
> that this won't bite anymore.

Depends on what you mean with "all". Like I wrote in the commit log, for now I 
translated in this patch those errnos that I identified as important, and 
those are audited by me of course (checked the man pages for what errors are 
expected as result from calls on Linux vs. macOS side and looked up numeric 
values from header files on both sides, tests).

However if you rather mean really all errnos that were ever defined on Linux 
and macOS, then the answer is no. That would probably quite some work, and I'm 
not sure if you could just try to translate them dry, i.e. by just looking at 
the headers or something.

So yes, your concern is justified. The question is, should this all be 
translated right now already, or would it be OK to address this minimum set of 
errno translation for now (especially for qemu-stable) and then later address 
the rest?

On the long term you would probably import the Linux errno header file into 
the code base, then prefix the individual macros with something like 
DOTL_ENODATA, etc. and then use those macros for translating the errnos 
instead of using literal numerics, maybe using GCC's designated array 
initializers then.

> > +#else
> > +#error Missing errno translation to Linux for this host system
> > +#endif
> > +    return err;
> > +}
> > +
> 
> As with the other patch, I'd rather move this magic to 9p-util.h .

Makes sense. There is indeed already too much utility code piled up in 9p.c.

> >  static ssize_t pdu_marshal(V9fsPDU *pdu, size_t offset, const char *fmt,
> >  ...) {
> >  
> >      ssize_t ret;
> > 
> > @@ -1054,6 +1079,8 @@ static void coroutine_fn pdu_complete(V9fsPDU *pdu,
> > ssize_t len)> 
> >              }
> >              len += ret;
> >              id = P9_RERROR;
> > 
> > +        } else {
> > +            err = errno_to_dotl(err);
> > 
> >          }
> >          
> >          ret = pdu_marshal(pdu, len, "d", err);

Re: [PATCH 4/5] 9pfs: fix wrong errno being sent to Linux client on macOS host

[Greg Kurz]

On Thu, 21 Apr 2022 13:13:08 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Donnerstag, 21. April 2022 12:48:35 CEST Greg Kurz wrote:
> > On Tue, 19 Apr 2022 13:41:59 +0200
> > 
> > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > > Linux and macOS only share some errno definitions with equal macro
> > > name and value. In fact most mappings for errno are completely
> > > different on the two systems.
> > > 
> > > This patch converts some important errno values from macOS host to
> > > corresponding Linux errno values before eventually sending such error
> > > codes along with Tlerror replies (if 9p2000.L is used that is), which
> > > fixes a bunch of misbehaviours when running a Linux client on macOS
> > > host.
> > 
> > This even fixes an actual protocol violation :
> > 
> > lerror -- return error code
> > 
> > size[4] Rlerror tag[2] ecode[4]
> > 
> > lerror replaces the reply message used in a successful call. ecode is a
> > numerical Linux errno.
> > ^^^^^^^^^^^^^^^^^^^^^
> > 
> > Taken from
> > https://github.com/chaos/diod/wiki/protocol#lerror----return-error-code
> 
> Again, something to add to the commit log?
> 

IMHO a protocol violation is important enough to be mentioned but
I'll leave it to you.

> > > For instance this patch fixes:
> > >   mount -t 9p -o posixacl ...
> > > 
> > > on Linux guest if security_mode=mapped was used for 9p server, which
> > > refused to mount successfully, because macOS returned ENOATTR==93
> > > when client tried to retrieve POSIX ACL xattrs, because errno 93
> > > is defined as EPROTONOSUPPORT==93 on Linux, so Linux client believed
> > > that xattrs were not supported by filesystem on host in general.
> > > 
> > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > > ---
> > > 
> > >  hw/9pfs/9p.c | 27 +++++++++++++++++++++++++++
> > >  1 file changed, 27 insertions(+)
> > > 
> > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > > index d953035e1c..becc41cbfd 100644
> > > --- a/hw/9pfs/9p.c
> > > +++ b/hw/9pfs/9p.c
> > > @@ -57,6 +57,31 @@ enum {
> > > 
> > >  P9ARRAY_DEFINE_TYPE(V9fsPath, v9fs_path_free);
> > > 
> > > +/* Translates errno from host -> Linux if needed */
> > > +static int errno_to_dotl(int err) {
> > > +#if defined(CONFIG_LINUX)
> > > +    /* nothing to translate (Linux -> Linux) */
> > > +#elif defined(CONFIG_DARWIN)
> > > +    /* translation mandatory for macOS hosts */
> > > +    if (err == ENAMETOOLONG) {
> > > +        err = 36; /* ==ENAMETOOLONG on Linux */
> > > +    } else if (err == ENOTEMPTY) {
> > > +        err = 39; /* ==ENOTEMPTY on Linux */
> > > +    } else if (err == ELOOP) {
> > > +        err = 40; /* ==ELOOP on Linux */
> > > +    } else if (err == ENOATTR) {
> > > +        err = 61; /* ==ENODATA on Linux */
> > > +    } else if (err == ENOTSUP) {
> > > +        err = 95; /* ==EOPNOTSUPP on Linux */
> > > +    } else if (err == EOPNOTSUPP) {
> > > +        err = 95; /* ==EOPNOTSUPP on Linux */
> > > +    }
> > 
> > I'm assuming you have audited all errnos, right ? Just to be sure
> > that this won't bite anymore.
> 
> Depends on what you mean with "all". Like I wrote in the commit log, for now I 
> translated in this patch those errnos that I identified as important, and 
> those are audited by me of course (checked the man pages for what errors are 
> expected as result from calls on Linux vs. macOS side and looked up numeric 
> values from header files on both sides, tests).
> 

I was pretty sure you'd have done that at least :-)

> However if you rather mean really all errnos that were ever defined on Linux 
> and macOS, then the answer is no. That would probably quite some work, and I'm 
> not sure if you could just try to translate them dry, i.e. by just looking at 
> the headers or something.
> 

But yes I was rather meaning the full errno set.

> So yes, your concern is justified. The question is, should this all be 
> translated right now already, or would it be OK to address this minimum set of 
> errno translation for now (especially for qemu-stable) and then later address 
> the rest?
> 

My concern is about the maintenance burden of investigating future
implementations of this issue, so I'd say it is mostly up to you
as the principal maintainer. Maybe leave a FIXME comment in the code
that the list of translated errnos isn't exhaustive at least ?

> On the long term you would probably import the Linux errno header file into 
> the code base, then prefix the individual macros with something like 
> DOTL_ENODATA, etc. and then use those macros for translating the errnos 
> instead of using literal numerics, maybe using GCC's designated array 
> initializers then.
> 

This would make sense indeed since 9p2000.L explicitly mentions the
numerical linux errnos.

> > > +#else
> > > +#error Missing errno translation to Linux for this host system
> > > +#endif
> > > +    return err;
> > > +}
> > > +
> > 
> > As with the other patch, I'd rather move this magic to 9p-util.h .
> 
> Makes sense. There is indeed already too much utility code piled up in 9p.c.
> 
> > >  static ssize_t pdu_marshal(V9fsPDU *pdu, size_t offset, const char *fmt,
> > >  ...) {
> > >  
> > >      ssize_t ret;
> > > 
> > > @@ -1054,6 +1079,8 @@ static void coroutine_fn pdu_complete(V9fsPDU *pdu,
> > > ssize_t len)> 
> > >              }
> > >              len += ret;
> > >              id = P9_RERROR;
> > > 
> > > +        } else {
> > > +            err = errno_to_dotl(err);
> > > 
> > >          }
> > >          
> > >          ret = pdu_marshal(pdu, len, "d", err);
> 
> 

Re: [PATCH 4/5] 9pfs: fix wrong errno being sent to Linux client on macOS host

[Christian Schoenebeck]

On Donnerstag, 21. April 2022 13:46:26 CEST Greg Kurz wrote:
> On Thu, 21 Apr 2022 13:13:08 +0200
> 
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > On Donnerstag, 21. April 2022 12:48:35 CEST Greg Kurz wrote:
> > > On Tue, 19 Apr 2022 13:41:59 +0200
> > > 
> > > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > > > Linux and macOS only share some errno definitions with equal macro
> > > > name and value. In fact most mappings for errno are completely
> > > > different on the two systems.
> > > > 
> > > > This patch converts some important errno values from macOS host to
> > > > corresponding Linux errno values before eventually sending such error
> > > > codes along with Tlerror replies (if 9p2000.L is used that is), which
> > > > fixes a bunch of misbehaviours when running a Linux client on macOS
> > > > host.
> > > 
> > > This even fixes an actual protocol violation :
> > > 
> > > lerror -- return error code
> > > 
> > > size[4] Rlerror tag[2] ecode[4]
> > > 
> > > lerror replaces the reply message used in a successful call. ecode is a
> > > numerical Linux errno.
> > > ^^^^^^^^^^^^^^^^^^^^^
> > > 
> > > Taken from
> > > https://github.com/chaos/diod/wiki/protocol#lerror----return-error-code
> > 
> > Again, something to add to the commit log?
> 
> IMHO a protocol violation is important enough to be mentioned but
> I'll leave it to you.

Ok, will do.

> > > > For instance this patch fixes:
> > > >   mount -t 9p -o posixacl ...
> > > > 
> > > > on Linux guest if security_mode=mapped was used for 9p server, which
> > > > refused to mount successfully, because macOS returned ENOATTR==93
> > > > when client tried to retrieve POSIX ACL xattrs, because errno 93
> > > > is defined as EPROTONOSUPPORT==93 on Linux, so Linux client believed
> > > > that xattrs were not supported by filesystem on host in general.
> > > > 
> > > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > > > ---
> > > > 
> > > >  hw/9pfs/9p.c | 27 +++++++++++++++++++++++++++
> > > >  1 file changed, 27 insertions(+)
> > > > 
> > > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > > > index d953035e1c..becc41cbfd 100644
> > > > --- a/hw/9pfs/9p.c
> > > > +++ b/hw/9pfs/9p.c
> > > > @@ -57,6 +57,31 @@ enum {
> > > > 
> > > >  P9ARRAY_DEFINE_TYPE(V9fsPath, v9fs_path_free);
> > > > 
> > > > +/* Translates errno from host -> Linux if needed */
> > > > +static int errno_to_dotl(int err) {
> > > > +#if defined(CONFIG_LINUX)
> > > > +    /* nothing to translate (Linux -> Linux) */
> > > > +#elif defined(CONFIG_DARWIN)
> > > > +    /* translation mandatory for macOS hosts */
> > > > +    if (err == ENAMETOOLONG) {
> > > > +        err = 36; /* ==ENAMETOOLONG on Linux */
> > > > +    } else if (err == ENOTEMPTY) {
> > > > +        err = 39; /* ==ENOTEMPTY on Linux */
> > > > +    } else if (err == ELOOP) {
> > > > +        err = 40; /* ==ELOOP on Linux */
> > > > +    } else if (err == ENOATTR) {
> > > > +        err = 61; /* ==ENODATA on Linux */
> > > > +    } else if (err == ENOTSUP) {
> > > > +        err = 95; /* ==EOPNOTSUPP on Linux */
> > > > +    } else if (err == EOPNOTSUPP) {
> > > > +        err = 95; /* ==EOPNOTSUPP on Linux */
> > > > +    }
> > > 
> > > I'm assuming you have audited all errnos, right ? Just to be sure
> > > that this won't bite anymore.
> > 
> > Depends on what you mean with "all". Like I wrote in the commit log, for
> > now I translated in this patch those errnos that I identified as
> > important, and those are audited by me of course (checked the man pages
> > for what errors are expected as result from calls on Linux vs. macOS side
> > and looked up numeric values from header files on both sides, tests).
> 
> I was pretty sure you'd have done that at least :-)
> 
> > However if you rather mean really all errnos that were ever defined on
> > Linux and macOS, then the answer is no. That would probably quite some
> > work, and I'm not sure if you could just try to translate them dry, i.e.
> > by just looking at the headers or something.
> 
> But yes I was rather meaning the full errno set.
> 
> > So yes, your concern is justified. The question is, should this all be
> > translated right now already, or would it be OK to address this minimum
> > set of errno translation for now (especially for qemu-stable) and then
> > later address the rest?
> 
> My concern is about the maintenance burden of investigating future
> implementations of this issue, so I'd say it is mostly up to you
> as the principal maintainer. Maybe leave a FIXME comment in the code
> that the list of translated errnos isn't exhaustive at least ?

I stick to this minimal approach with FIXME comment then for now. I have 
tested this patch thoroughly on macOS and did not encounter similar issues. So 
I think it is good enough as first-aid patch at least.

The aforementioned case-insensitive filesystem issues are a higher priority 
from my PoV, because they trigger various misbehaviour in my tests.

Best regards,
Christian Schoenebeck

> > On the long term you would probably import the Linux errno header file
> > into
> > the code base, then prefix the individual macros with something like
> > DOTL_ENODATA, etc. and then use those macros for translating the errnos
> > instead of using literal numerics, maybe using GCC's designated array
> > initializers then.
> 
> This would make sense indeed since 9p2000.L explicitly mentions the
> numerical linux errnos.
> 
> > > > +#else
> > > > +#error Missing errno translation to Linux for this host system
> > > > +#endif
> > > > +    return err;
> > > > +}
> > > > +
> > > 
> > > As with the other patch, I'd rather move this magic to 9p-util.h .
> > 
> > Makes sense. There is indeed already too much utility code piled up in
> > 9p.c.> 
> > > >  static ssize_t pdu_marshal(V9fsPDU *pdu, size_t offset, const char
> > > >  *fmt,
> > > >  ...) {
> > > >  
> > > >      ssize_t ret;
> > > > 
> > > > @@ -1054,6 +1079,8 @@ static void coroutine_fn pdu_complete(V9fsPDU
> > > > *pdu,
> > > > ssize_t len)>
> > > > 
> > > >              }
> > > >              len += ret;
> > > >              id = P9_RERROR;
> > > > 
> > > > +        } else {
> > > > +            err = errno_to_dotl(err);
> > > > 
> > > >          }
> > > >          
> > > >          ret = pdu_marshal(pdu, len, "d", err);

Re: [PATCH 5/5] 9pfs: fix removing non-existent POSIX ACL xattr on macOS host

[Greg Kurz]

On Thu, 21 Apr 2022 12:55:24 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Donnerstag, 21. April 2022 10:26:11 CEST Greg Kurz wrote:
> > On Tue, 19 Apr 2022 13:43:30 +0200
> > 
> > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > > When mapped POSIX ACL is used, we are ignoring errors when trying
> > > to remove a POSIX ACL xattr that does not exist. On Linux hosts we
> > > would get ENODATA in such cases, on macOS hosts however we get
> > > ENOATTR instead, so ignore ENOATTR errors as well.
> > > 
> > > This patch fixes e.g. a command on Linux guest like:
> > >   cp --preserve=mode old new
> > > 
> > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > > ---
> > > 
> > >  hw/9pfs/9p-posix-acl.c | 8 +++++++-
> > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/9pfs/9p-posix-acl.c b/hw/9pfs/9p-posix-acl.c
> > > index eadae270dd..2bf155f941 100644
> > > --- a/hw/9pfs/9p-posix-acl.c
> > > +++ b/hw/9pfs/9p-posix-acl.c
> > > @@ -65,7 +65,13 @@ static int mp_pacl_removexattr(FsContext *ctx,
> > > 
> > >      int ret;
> > >      
> > >      ret = local_removexattr_nofollow(ctx, path, MAP_ACL_ACCESS);
> > > 
> > > -    if (ret == -1 && errno == ENODATA) {
> > > +    if (ret == -1 &&
> > > +          (errno == ENODATA
> > > +#ifdef ENOATTR
> > > +          || errno == ENOATTR
> > > +#endif
> > > +          )
> > 
> > We already have this in <qemu/xattr.h> which is included by
> > 9p-posix-acl.c :
> > 
> > /*
> >  * Modern distributions (e.g. Fedora 15), have no libattr.so, place attr.h
> >  * in /usr/include/sys, and don't have ENOATTR.
> >  */
> > 
> > 
> > #ifdef CONFIG_LIBATTR
> > #  include <attr/xattr.h>
> > #else
> > #  if !defined(ENOATTR)
> > #    define ENOATTR ENODATA
> > #  endif
> > #  include <sys/xattr.h>
> > #endif
> > 
> > I guess this patch could just s/ENODATA/ENOATTR/ to avoid the
> > extra ifdefery.
> 
> Not viable, because macOS does have both ENODATA==96 and ENOATTR==93. On Linux 
> the two macros were historically defined to the same numeric values, that's 
> why it worked there.
> 

I was meaning your current fix could simply do:

-    if (ret == -1 && errno == ENODATA) {
+    if (ret == -1 && errno == ENOATTR) {

since ENOATTR works in all cases, but this is rather a hack.

Another solution would be to ensure that local_removexattr_nofollow() only
reports linux errnos. This could be handled cleanly in the
fremovexattrat_nofollow() implementation in 9p-util-darwin.c.

Since the 9p code base mostly assumes the host is linux, this should
probably be generalized to other places where we check errno.

> Maybe I should define a separate macro like:
> 
> #if ...
> # define P9_ENOATTR ENOATTR
> #else
> # define P9_ENOATTR ENODATA
> #end
> 
> ?
> 
> Actually good that you pointed me at this, because I just realized there is a 
> 2nd place in 9p-posix-acl.c which would require this as well. For some reason 
> the 2nd place just did not trigger while I was testing it on macOS.
> 
> Best regards,
> Christian Schoenebeck
> 
> 

Re: [PATCH 5/5] 9pfs: fix removing non-existent POSIX ACL xattr on macOS host

[Christian Schoenebeck]

On Donnerstag, 21. April 2022 14:26:37 CEST Greg Kurz wrote:
> On Thu, 21 Apr 2022 12:55:24 +0200
> 
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > On Donnerstag, 21. April 2022 10:26:11 CEST Greg Kurz wrote:
> > > On Tue, 19 Apr 2022 13:43:30 +0200
> > > 
> > > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > > > When mapped POSIX ACL is used, we are ignoring errors when trying
> > > > to remove a POSIX ACL xattr that does not exist. On Linux hosts we
> > > > would get ENODATA in such cases, on macOS hosts however we get
> > > > ENOATTR instead, so ignore ENOATTR errors as well.
> > > > 
> > > > This patch fixes e.g. a command on Linux guest like:
> > > >   cp --preserve=mode old new
> > > > 
> > > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > > > ---
> > > > 
> > > >  hw/9pfs/9p-posix-acl.c | 8 +++++++-
> > > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/hw/9pfs/9p-posix-acl.c b/hw/9pfs/9p-posix-acl.c
> > > > index eadae270dd..2bf155f941 100644
> > > > --- a/hw/9pfs/9p-posix-acl.c
> > > > +++ b/hw/9pfs/9p-posix-acl.c
> > > > @@ -65,7 +65,13 @@ static int mp_pacl_removexattr(FsContext *ctx,
> > > > 
> > > >      int ret;
> > > >      
> > > >      ret = local_removexattr_nofollow(ctx, path, MAP_ACL_ACCESS);
> > > > 
> > > > -    if (ret == -1 && errno == ENODATA) {
> > > > +    if (ret == -1 &&
> > > > +          (errno == ENODATA
> > > > +#ifdef ENOATTR
> > > > +          || errno == ENOATTR
> > > > +#endif
> > > > +          )
> > > 
> > > We already have this in <qemu/xattr.h> which is included by
> > > 9p-posix-acl.c :
> > > 
> > > /*
> > > 
> > >  * Modern distributions (e.g. Fedora 15), have no libattr.so, place
> > >  attr.h
> > >  * in /usr/include/sys, and don't have ENOATTR.
> > >  */
> > > 
> > > #ifdef CONFIG_LIBATTR
> > > #  include <attr/xattr.h>
> > > #else
> > > #  if !defined(ENOATTR)
> > > #    define ENOATTR ENODATA
> > > #  endif
> > > #  include <sys/xattr.h>
> > > #endif
> > > 
> > > I guess this patch could just s/ENODATA/ENOATTR/ to avoid the
> > > extra ifdefery.
> > 
> > Not viable, because macOS does have both ENODATA==96 and ENOATTR==93. On
> > Linux the two macros were historically defined to the same numeric
> > values, that's why it worked there.
> 
> I was meaning your current fix could simply do:
> 
> -    if (ret == -1 && errno == ENODATA) {
> +    if (ret == -1 && errno == ENOATTR) {
> 
> since ENOATTR works in all cases, but this is rather a hack.
> 
> Another solution would be to ensure that local_removexattr_nofollow() only
> reports linux errnos. This could be handled cleanly in the
> fremovexattrat_nofollow() implementation in 9p-util-darwin.c.
> 
> Since the 9p code base mostly assumes the host is linux, this should
> probably be generalized to other places where we check errno.

Got it. I tend to go with the former (checking errno == ENOATTR and defining 
ENOATTR if non existent). I find that a bit cleaner than the latter which 
would have the potential to mask another error (ENODATA).

> > Maybe I should define a separate macro like:
> > 
> > #if ...
> > # define P9_ENOATTR ENOATTR
> > #else
> > # define P9_ENOATTR ENODATA
> > #end
> > 
> > ?
> > 
> > Actually good that you pointed me at this, because I just realized there
> > is a 2nd place in 9p-posix-acl.c which would require this as well. For
> > some reason the 2nd place just did not trigger while I was testing it on
> > macOS.
> > 
> > Best regards,
> > Christian Schoenebeck