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

Will Cohen posted 11 patches 4 years, 2 months ago
Maintainers: Greg Kurz <groug@kaod.org>, Christian Schoenebeck <qemu_oss@crudebyte.com>
There is a newer version of this series
[PATCH v2 08/11] 9p: darwin: Compatibility for f/l*xattr
Posted by Will Cohen 4 years, 2 months ago
From: Keno Fischer <keno@juliacomputing.com>

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

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

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


Re: [PATCH v2 08/11] 9p: darwin: Compatibility for f/l*xattr
Posted by Christian Schoenebeck 4 years, 2 months ago
On Montag, 22. November 2021 01:49:10 CET Will Cohen wrote:
> From: Keno Fischer <keno@juliacomputing.com>
> 
> On darwin `fgetxattr` takes two extra optional arguments,
> and the l* variants are not defined (in favor of an extra
> flag to the regular variants.
> 
> Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> [Michael Roitzsch: - Rebase for NixOS]
> Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
> Signed-off-by: Will Cohen <wwcohen@gmail.com>
> ---
>  hw/9pfs/9p-local.c | 12 ++++++++----
>  hw/9pfs/9p-util.h  | 17 +++++++++++++++++
>  2 files changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index 1a5e3eed73..2bfff79b12 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -781,16 +781,20 @@ static int local_fstat(FsContext *fs_ctx, int
> fid_type, mode_t tmp_mode;
>          dev_t tmp_dev;
> 
> -        if (fgetxattr(fd, "user.virtfs.uid", &tmp_uid, sizeof(uid_t)) > 0)
> { +        if (qemu_fgetxattr(fd, "user.virtfs.uid",
> +                           &tmp_uid, sizeof(uid_t)) > 0) {
>              stbuf->st_uid = le32_to_cpu(tmp_uid);
>          }
> -        if (fgetxattr(fd, "user.virtfs.gid", &tmp_gid, sizeof(gid_t)) > 0)
> { +        if (qemu_fgetxattr(fd, "user.virtfs.gid",
> +                           &tmp_gid, sizeof(gid_t)) > 0) {
>              stbuf->st_gid = le32_to_cpu(tmp_gid);
>          }
> -        if (fgetxattr(fd, "user.virtfs.mode", &tmp_mode, sizeof(mode_t)) >
> 0) { +        if (qemu_fgetxattr(fd, "user.virtfs.mode",
> +                           &tmp_mode, sizeof(mode_t)) > 0) {
>              stbuf->st_mode = le32_to_cpu(tmp_mode);
>          }
> -        if (fgetxattr(fd, "user.virtfs.rdev", &tmp_dev, sizeof(dev_t)) > 0)
> { +        if (qemu_fgetxattr(fd, "user.virtfs.rdev",
> +                           &tmp_dev, sizeof(dev_t)) > 0) {
>              stbuf->st_rdev = le64_to_cpu(tmp_dev);
>          }
>      } else if (fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
> diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h
> index 627baebaba..38ef8b289d 100644
> --- a/hw/9pfs/9p-util.h
> +++ b/hw/9pfs/9p-util.h
> @@ -19,6 +19,23 @@
>  #define O_PATH_9P_UTIL 0
>  #endif
> 
> +#ifdef CONFIG_DARWIN
> +#define qemu_fgetxattr(...) fgetxattr(__VA_ARGS__, 0, 0)

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

> +#define qemu_lgetxattr(...) getxattr(__VA_ARGS__, 0, XATTR_NOFOLLOW)
> +#define qemu_llistxattr(...) listxattr(__VA_ARGS__, XATTR_NOFOLLOW)
> +#define qemu_lremovexattr(...) removexattr(__VA_ARGS__, XATTR_NOFOLLOW)
> +static inline int qemu_lsetxattr(const char *path, const char *name,
> +                                 const void *value, size_t size, int flags)
> { +    return setxattr(path, name, value, size, 0, flags | XATTR_NOFOLLOW);
> +}
> +#else
> +#define qemu_fgetxattr fgetxattr
> +#define qemu_lgetxattr lgetxattr
> +#define qemu_llistxattr llistxattr
> +#define qemu_lremovexattr lremovexattr
> +#define qemu_lsetxattr lsetxattr
> +#endif
> +
>  static inline void close_preserve_errno(int fd)
>  {
>      int serrno = errno;

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

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

Who is right?



Re: [PATCH v2 08/11] 9p: darwin: Compatibility for f/l*xattr
Posted by Will Cohen 4 years ago
It's quite possible I'm misreading the man page when double-checking this,
but I believe the l-prefixed functions are for the link itself and
fgetxattr works on the file it refers to. This is why XATTR_NOFOLLOW
doesn't appear on fgetxattr, and I believe this also means that these
definitions line up the macOS behavior with Linux.

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

> On Montag, 22. November 2021 01:49:10 CET Will Cohen wrote:
> > From: Keno Fischer <keno@juliacomputing.com>
> >
> > On darwin `fgetxattr` takes two extra optional arguments,
> > and the l* variants are not defined (in favor of an extra
> > flag to the regular variants.
> >
> > Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> > [Michael Roitzsch: - Rebase for NixOS]
> > Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
> > Signed-off-by: Will Cohen <wwcohen@gmail.com>
> > ---
> >  hw/9pfs/9p-local.c | 12 ++++++++----
> >  hw/9pfs/9p-util.h  | 17 +++++++++++++++++
> >  2 files changed, 25 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> > index 1a5e3eed73..2bfff79b12 100644
> > --- a/hw/9pfs/9p-local.c
> > +++ b/hw/9pfs/9p-local.c
> > @@ -781,16 +781,20 @@ static int local_fstat(FsContext *fs_ctx, int
> > fid_type, mode_t tmp_mode;
> >          dev_t tmp_dev;
> >
> > -        if (fgetxattr(fd, "user.virtfs.uid", &tmp_uid, sizeof(uid_t)) >
> 0)
> > { +        if (qemu_fgetxattr(fd, "user.virtfs.uid",
> > +                           &tmp_uid, sizeof(uid_t)) > 0) {
> >              stbuf->st_uid = le32_to_cpu(tmp_uid);
> >          }
> > -        if (fgetxattr(fd, "user.virtfs.gid", &tmp_gid, sizeof(gid_t)) >
> 0)
> > { +        if (qemu_fgetxattr(fd, "user.virtfs.gid",
> > +                           &tmp_gid, sizeof(gid_t)) > 0) {
> >              stbuf->st_gid = le32_to_cpu(tmp_gid);
> >          }
> > -        if (fgetxattr(fd, "user.virtfs.mode", &tmp_mode,
> sizeof(mode_t)) >
> > 0) { +        if (qemu_fgetxattr(fd, "user.virtfs.mode",
> > +                           &tmp_mode, sizeof(mode_t)) > 0) {
> >              stbuf->st_mode = le32_to_cpu(tmp_mode);
> >          }
> > -        if (fgetxattr(fd, "user.virtfs.rdev", &tmp_dev, sizeof(dev_t))
> > 0)
> > { +        if (qemu_fgetxattr(fd, "user.virtfs.rdev",
> > +                           &tmp_dev, sizeof(dev_t)) > 0) {
> >              stbuf->st_rdev = le64_to_cpu(tmp_dev);
> >          }
> >      } else if (fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
> > diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h
> > index 627baebaba..38ef8b289d 100644
> > --- a/hw/9pfs/9p-util.h
> > +++ b/hw/9pfs/9p-util.h
> > @@ -19,6 +19,23 @@
> >  #define O_PATH_9P_UTIL 0
> >  #endif
> >
> > +#ifdef CONFIG_DARWIN
> > +#define qemu_fgetxattr(...) fgetxattr(__VA_ARGS__, 0, 0)
>
> Why does this not have XATTR_NOFOLLOW and the others do? -^
>
> > +#define qemu_lgetxattr(...) getxattr(__VA_ARGS__, 0, XATTR_NOFOLLOW)
> > +#define qemu_llistxattr(...) listxattr(__VA_ARGS__, XATTR_NOFOLLOW)
> > +#define qemu_lremovexattr(...) removexattr(__VA_ARGS__, XATTR_NOFOLLOW)
> > +static inline int qemu_lsetxattr(const char *path, const char *name,
> > +                                 const void *value, size_t size, int
> flags)
> > { +    return setxattr(path, name, value, size, 0, flags |
> XATTR_NOFOLLOW);
> > +}
> > +#else
> > +#define qemu_fgetxattr fgetxattr
> > +#define qemu_lgetxattr lgetxattr
> > +#define qemu_llistxattr llistxattr
> > +#define qemu_lremovexattr lremovexattr
> > +#define qemu_lsetxattr lsetxattr
> > +#endif
> > +
> >  static inline void close_preserve_errno(int fd)
> >  {
> >      int serrno = errno;
>
> Hmm, so we would have two different behaviours for Linux vs. macOS here.
>
> If there is a symbolic link on host, Linux currently applies the permission
> map as xattrs to the destination of the symlink, whereas macOS would map
> the
> permissions as xattrs to the symbolic link itself.
>
> Who is right?
>
>
>