[Qemu-devel] [PATCH 00/13] 9p: Add support for Darwin

[Qemu-devel] [PATCH 00/13] 9p: Add support for Darwin

[keno]

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

Hi Greg,

this series adds support for building the 9p code on Mac OS X.
It seems to work decently well (tested by booting up a linux
guest and building a copy of qemu on a 9p mount in the guest),
but there are probably corner cases I got wrong (particular
in the xattr support). Is there a stress test you recommend
running for those corner cases?

I've split the commits rather finely to hopefully ease review,
of each individual concern I ran into while porting. Happy to
merge commits back together if you would prefer.

Lastly, I should remark that I'm not super familiar with the qemu
code base, so please let me know if there's a better place for
some of the code (particularly some of the compatibility code).

Keno Fischer (13):
  9p: linux: Fix a couple Linux assumptions
  9p: Avoid warning if FS_IOC_GETVERSION is not defined
  9p: Move a couple xattr functions to 9p-util
  9p: darwin: Handle struct stat(fs) differences
  9p: darwin: Handle struct dirent differences
  9p: darwin: Address minor differences
  9p: darwin: Properly translate AT_REMOVEDIR flag
  9p: darwin: Ignore O_{NOATIME, DIRECT}
  9p: darwin: Provide a compatibility definition for XATTR_SIZE_MAX
  9p: darwin: *xattr_nofollow implementations
  9p: darwin: Mark mknod as unsupported
  9p: darwin: Provide a fallback implementation for utimensat
  9p: darwin: configure: Allow VirtFS on Darwin

 Makefile.objs        |   1 +
 configure            |  23 ++++++----
 fsdev/file-op-9p.h   |   6 +++
 hw/9pfs/9p-local.c   |  84 +++++++++++++++++++++++-------------
 hw/9pfs/9p-proxy.c   |  17 ++++++--
 hw/9pfs/9p-synth.c   |   6 +++
 hw/9pfs/9p-util.c    | 120 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 hw/9pfs/9p-util.h    |  13 ++++++
 hw/9pfs/9p-xattr.c   |  33 --------------
 hw/9pfs/9p.c         |  79 +++++++++++++++++++++++++--------
 include/qemu/xattr.h |   4 +-
 11 files changed, 293 insertions(+), 93 deletions(-)

-- 
2.8.1

[Qemu-devel] [PATCH 01/13] 9p: linux: Fix a couple Linux assumptions

[keno]

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

 - Guard two Linux only headers.
 - Define `ENOATTR` only if not only defined
   (it's defined in system headers on Darwin).

Signed-off-by: Keno Fischer <keno@juliacomputing.com>
---
 fsdev/file-op-9p.h   | 2 ++
 hw/9pfs/9p-local.c   | 2 ++
 include/qemu/xattr.h | 4 +++-
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
index 3fa062b..a13e729 100644
--- a/fsdev/file-op-9p.h
+++ b/fsdev/file-op-9p.h
@@ -16,7 +16,9 @@
 
 #include <dirent.h>
 #include <utime.h>
+#ifdef CONFIG_LINUX
 #include <sys/vfs.h>
+#endif
 #include "qemu-fsdev-throttle.h"
 
 #define SM_LOCAL_MODE_BITS    0600
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index b37b1db..f6c7526 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -27,10 +27,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/include/qemu/xattr.h b/include/qemu/xattr.h
index a83fe8e..f1d0f7b 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.8.1

Re: [Qemu-devel] [PATCH 01/13] 9p: linux: Fix a couple Linux assumptions

[Philippe Mathieu-Daudé]

Hi Keno,

On 05/26/2018 02:23 AM, keno@juliacomputing.com wrote:
> From: Keno Fischer <keno@alumni.harvard.edu>
> 
>  - Guard two Linux only headers.
>  - Define `ENOATTR` only if not only defined
>    (it's defined in system headers on Darwin).
> 
> Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> ---
>  fsdev/file-op-9p.h   | 2 ++
>  hw/9pfs/9p-local.c   | 2 ++
>  include/qemu/xattr.h | 4 +++-
>  3 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
> index 3fa062b..a13e729 100644
> --- a/fsdev/file-op-9p.h
> +++ b/fsdev/file-op-9p.h
> @@ -16,7 +16,9 @@
>  
>  #include <dirent.h>
>  #include <utime.h>
> +#ifdef CONFIG_LINUX

What about a less restrictive:

#ifndef __APPLE__

>  #include <sys/vfs.h>
> +#endif
>  #include "qemu-fsdev-throttle.h"
>  
>  #define SM_LOCAL_MODE_BITS    0600
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index b37b1db..f6c7526 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -27,10 +27,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/include/qemu/xattr.h b/include/qemu/xattr.h
> index a83fe8e..f1d0f7b 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
>  

Rest looks correct.

Re: [Qemu-devel] [PATCH 01/13] 9p: linux: Fix a couple Linux assumptions

[Peter Maydell]

On 26 May 2018 at 07:30, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Hi Keno,
>
> On 05/26/2018 02:23 AM, keno@juliacomputing.com wrote:
>> From: Keno Fischer <keno@alumni.harvard.edu>
>>
>>  - Guard two Linux only headers.
>>  - Define `ENOATTR` only if not only defined
>>    (it's defined in system headers on Darwin).
>>
>> Signed-off-by: Keno Fischer <keno@juliacomputing.com>
>> ---
>>  fsdev/file-op-9p.h   | 2 ++
>>  hw/9pfs/9p-local.c   | 2 ++
>>  include/qemu/xattr.h | 4 +++-
>>  3 files changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
>> index 3fa062b..a13e729 100644
>> --- a/fsdev/file-op-9p.h
>> +++ b/fsdev/file-op-9p.h
>> @@ -16,7 +16,9 @@
>>
>>  #include <dirent.h>
>>  #include <utime.h>
>> +#ifdef CONFIG_LINUX
>
> What about a less restrictive:
>
> #ifndef __APPLE__

In general I would recommend checking for specific
features (usually in configure), rather than adding
ifdef tests for particular OSes. In this case presumably
we're including these headers because we're after
a specific function or define or whatever, so we can
check in configure for what header that's in (or
if it's not available at all).

thanks
-- PMM

Re: [Qemu-devel] [PATCH 01/13] 9p: linux: Fix a couple Linux assumptions

[Keno Fischer]

>>> diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
>>> index 3fa062b..a13e729 100644
>>> --- a/fsdev/file-op-9p.h
>>> +++ b/fsdev/file-op-9p.h
>>> @@ -16,7 +16,9 @@
>>>
>>>  #include <dirent.h>
>>>  #include <utime.h>
>>> +#ifdef CONFIG_LINUX
>>
>> What about a less restrictive:
>>
>> #ifndef __APPLE__
>
> In general I would recommend checking for specific
> features (usually in configure), rather than adding
> ifdef tests for particular OSes. In this case presumably
> we're including these headers because we're after
> a specific function or define or whatever, so we can
> check in configure for what header that's in (or
> if it's not available at all).
>
> thanks
> -- PMM

This header is used for struct statfs. I would be fine
with a configure check for this, though it looks like
other places in the code base that use this header
(e.g. util/mmap-alloc.c) also guard it in
CONFIG_LINUX, so that seemed like the right check
to me given the current code base.

Would you like me to submit a patch to switch these
to a configure check?

Re: [Qemu-devel] [PATCH 01/13] 9p: linux: Fix a couple Linux assumptions

[Greg Kurz]

On Sat, 26 May 2018 01:23:03 -0400
keno@juliacomputing.com wrote:

> From: Keno Fischer <keno@alumni.harvard.edu>
> 
>  - Guard two Linux only headers.
>  - Define `ENOATTR` only if not only defined
>    (it's defined in system headers on Darwin).
> 
> Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> ---
>  fsdev/file-op-9p.h   | 2 ++
>  hw/9pfs/9p-local.c   | 2 ++
>  include/qemu/xattr.h | 4 +++-

Irrespectively of the discussion on checking this in configure, 
there's another user of <sys/vfs.h> in fsdev/virtfs-proxy-helper.c.
I see in patch 13 that the helper won't be built on Darwin, but
this could change, and anyway, I'd like the handling of <sys/vfs.h>
to stay consistent across the VirtFS code.

>  3 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
> index 3fa062b..a13e729 100644
> --- a/fsdev/file-op-9p.h
> +++ b/fsdev/file-op-9p.h
> @@ -16,7 +16,9 @@
>  
>  #include <dirent.h>
>  #include <utime.h>
> +#ifdef CONFIG_LINUX
>  #include <sys/vfs.h>
> +#endif
>  #include "qemu-fsdev-throttle.h"
>  
>  #define SM_LOCAL_MODE_BITS    0600
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index b37b1db..f6c7526 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -27,10 +27,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/include/qemu/xattr.h b/include/qemu/xattr.h
> index a83fe8e..f1d0f7b 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
>  

[Qemu-devel] [PATCH 02/13] 9p: Avoid warning if FS_IOC_GETVERSION is not defined

[keno]

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

Signed-off-by: Keno Fischer <keno@juliacomputing.com>
---
 hw/9pfs/9p-local.c | 39 +++++++++++++++++++--------------------
 1 file changed, 19 insertions(+), 20 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index f6c7526..7592f8d 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1375,10 +1375,10 @@ static int local_unlinkat(FsContext *ctx, V9fsPath *dir,
     return ret;
 }
 
+#ifdef FS_IOC_GETVERSION
 static int local_ioc_getversion(FsContext *ctx, V9fsPath *path,
                                 mode_t st_mode, uint64_t *st_gen)
 {
-#ifdef FS_IOC_GETVERSION
     int err;
     V9fsFidOpenState fid_open;
 
@@ -1397,15 +1397,11 @@ static int local_ioc_getversion(FsContext *ctx, V9fsPath *path,
     err = ioctl(fid_open.fd, FS_IOC_GETVERSION, st_gen);
     local_close(ctx, &fid_open);
     return err;
-#else
-    errno = ENOTTY;
-    return -1;
-#endif
 }
+#endif
 
 static int local_init(FsContext *ctx, Error **errp)
 {
-    struct statfs stbuf;
     LocalData *data = g_malloc(sizeof(*data));
 
     data->mountfd = open(ctx->fs_root, O_DIRECTORY | O_RDONLY);
@@ -1415,20 +1411,23 @@ static int local_init(FsContext *ctx, Error **errp)
     }
 
 #ifdef FS_IOC_GETVERSION
-    /*
-     * use ioc_getversion only if the ioctl is definied
-     */
-    if (fstatfs(data->mountfd, &stbuf) < 0) {
-        close_preserve_errno(data->mountfd);
-        goto err;
-    }
-    switch (stbuf.f_type) {
-    case EXT2_SUPER_MAGIC:
-    case BTRFS_SUPER_MAGIC:
-    case REISERFS_SUPER_MAGIC:
-    case XFS_SUPER_MAGIC:
-        ctx->exops.get_st_gen = local_ioc_getversion;
-        break;
+    {
+        struct statfs stbuf;
+        /*
+        * use ioc_getversion only if the ioctl is definied
+        */
+        if (fstatfs(data->mountfd, &stbuf) < 0) {
+            close_preserve_errno(data->mountfd);
+            goto err;
+        }
+        switch (stbuf.f_type) {
+        case EXT2_SUPER_MAGIC:
+        case BTRFS_SUPER_MAGIC:
+        case REISERFS_SUPER_MAGIC:
+        case XFS_SUPER_MAGIC:
+            ctx->exops.get_st_gen = local_ioc_getversion;
+            break;
+        }
     }
 #endif
 
-- 
2.8.1

Re: [Qemu-devel] [PATCH 02/13] 9p: Avoid warning if FS_IOC_GETVERSION is not defined

[Greg Kurz]

On Sat, 26 May 2018 01:23:04 -0400
keno@juliacomputing.com wrote:

> From: Keno Fischer <keno@alumni.harvard.edu>
> 
> Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> ---
>  hw/9pfs/9p-local.c | 39 +++++++++++++++++++--------------------
>  1 file changed, 19 insertions(+), 20 deletions(-)
> 
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index f6c7526..7592f8d 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -1375,10 +1375,10 @@ static int local_unlinkat(FsContext *ctx, V9fsPath *dir,
>      return ret;
>  }
>  
> +#ifdef FS_IOC_GETVERSION
>  static int local_ioc_getversion(FsContext *ctx, V9fsPath *path,
>                                  mode_t st_mode, uint64_t *st_gen)
>  {
> -#ifdef FS_IOC_GETVERSION
>      int err;
>      V9fsFidOpenState fid_open;
>  
> @@ -1397,15 +1397,11 @@ static int local_ioc_getversion(FsContext *ctx, V9fsPath *path,
>      err = ioctl(fid_open.fd, FS_IOC_GETVERSION, st_gen);
>      local_close(ctx, &fid_open);
>      return err;
> -#else
> -    errno = ENOTTY;
> -    return -1;
> -#endif
>  }
> +#endif
>  
>  static int local_init(FsContext *ctx, Error **errp)
>  {
> -    struct statfs stbuf;
>      LocalData *data = g_malloc(sizeof(*data));
>  
>      data->mountfd = open(ctx->fs_root, O_DIRECTORY | O_RDONLY);
> @@ -1415,20 +1411,23 @@ static int local_init(FsContext *ctx, Error **errp)
>      }
>  
>  #ifdef FS_IOC_GETVERSION
> -    /*
> -     * use ioc_getversion only if the ioctl is definied
> -     */
> -    if (fstatfs(data->mountfd, &stbuf) < 0) {
> -        close_preserve_errno(data->mountfd);

Hmm... I now realize that this path doesn't set errp, which means that
we could possibly fail the device realization without reporting any
error to the caller. Could you please fix this in a preparatory patch ?

> -        goto err;
> -    }
> -    switch (stbuf.f_type) {
> -    case EXT2_SUPER_MAGIC:
> -    case BTRFS_SUPER_MAGIC:
> -    case REISERFS_SUPER_MAGIC:
> -    case XFS_SUPER_MAGIC:
> -        ctx->exops.get_st_gen = local_ioc_getversion;
> -        break;
> +    {
> +        struct statfs stbuf;
> +        /*
> +        * use ioc_getversion only if the ioctl is definied
> +        */
> +        if (fstatfs(data->mountfd, &stbuf) < 0) {
> +            close_preserve_errno(data->mountfd);
> +            goto err;
> +        }
> +        switch (stbuf.f_type) {
> +        case EXT2_SUPER_MAGIC:
> +        case BTRFS_SUPER_MAGIC:
> +        case REISERFS_SUPER_MAGIC:
> +        case XFS_SUPER_MAGIC:
> +            ctx->exops.get_st_gen = local_ioc_getversion;
> +            break;
> +        }

Please move this to a separate local_ioc_getversion_init() function, that would
be empty if FS_IOC_GETVERSION is not defined.

>      }
>  #endif
>  

[Qemu-devel] [PATCH 03/13] 9p: Move a couple xattr functions to 9p-util

[keno]

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

These functions will need custom implementations on Darwin. Since the
implementation is very similar among all of them, and 9p-util already
has the _nofollow version of fgetxattrat, let's move them all there.

Additionally, introduce a _follow version of fgetxattr and use it.
On darwin, fgetxattr has a more general interface, so we'll need to factor
it out anyway.

Signed-off-by: Keno Fischer <keno@juliacomputing.com>
---
 hw/9pfs/9p-local.c | 11 +++++++----
 hw/9pfs/9p-util.c  | 39 +++++++++++++++++++++++++++++++++++++++
 hw/9pfs/9p-util.h  |  6 ++++++
 hw/9pfs/9p-xattr.c | 33 ---------------------------------
 4 files changed, 52 insertions(+), 37 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 7592f8d..fd65d04 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -776,16 +776,19 @@ 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 (fgetxattr_follow(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 (fgetxattr_follow(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 (fgetxattr_follow(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 (fgetxattr_follow(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.c b/hw/9pfs/9p-util.c
index f709c27..8cf5554 100644
--- a/hw/9pfs/9p-util.c
+++ b/hw/9pfs/9p-util.c
@@ -24,3 +24,42 @@ ssize_t fgetxattrat_nofollow(int dirfd, const char *filename, const char *name,
     g_free(proc_path);
     return ret;
 }
+
+ssize_t fgetxattr_follow(int fd, const char *name,
+                         void *value, size_t size)
+{
+    return fgetxattr(fd, name, value, size);
+}
+
+ssize_t flistxattrat_nofollow(int dirfd, const char *filename,
+                              char *list, size_t size)
+{
+    char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
+    int ret;
+
+    ret = llistxattr(proc_path, list, size);
+    g_free(proc_path);
+    return ret;
+}
+
+ssize_t fremovexattrat_nofollow(int dirfd, const char *filename,
+                                const char *name)
+{
+    char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
+    int ret;
+
+    ret = lremovexattr(proc_path, name);
+    g_free(proc_path);
+    return ret;
+}
+
+int fsetxattrat_nofollow(int dirfd, const char *filename, const char *name,
+                         void *value, size_t size, int flags)
+{
+    char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
+    int ret;
+
+    ret = lsetxattr(proc_path, name, value, size, flags);
+    g_free(proc_path);
+    return ret;
+}
diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h
index dc0d2e2..cb26343 100644
--- a/hw/9pfs/9p-util.h
+++ b/hw/9pfs/9p-util.h
@@ -58,7 +58,13 @@ static inline int openat_file(int dirfd, const char *name, int flags,
 
 ssize_t fgetxattrat_nofollow(int dirfd, const char *path, const char *name,
                              void *value, size_t size);
+ssize_t fgetxattr_follow(int fd, const char *name,
+                         void *value, size_t size);
 int fsetxattrat_nofollow(int dirfd, const char *path, const char *name,
                          void *value, size_t size, int flags);
+ssize_t flistxattrat_nofollow(int dirfd, const char *filename,
+                              char *list, size_t size);
+ssize_t fremovexattrat_nofollow(int dirfd, const char *filename,
+                                const char *name);
 
 #endif
diff --git a/hw/9pfs/9p-xattr.c b/hw/9pfs/9p-xattr.c
index d05c1a1..c696d8f 100644
--- a/hw/9pfs/9p-xattr.c
+++ b/hw/9pfs/9p-xattr.c
@@ -60,17 +60,6 @@ ssize_t pt_listxattr(FsContext *ctx, const char *path,
     return name_size;
 }
 
-static ssize_t flistxattrat_nofollow(int dirfd, const char *filename,
-                                     char *list, size_t size)
-{
-    char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
-    int ret;
-
-    ret = llistxattr(proc_path, list, size);
-    g_free(proc_path);
-    return ret;
-}
-
 /*
  * Get the list and pass to each layer to find out whether
  * to send the data or not
@@ -196,17 +185,6 @@ ssize_t pt_getxattr(FsContext *ctx, const char *path, const char *name,
     return local_getxattr_nofollow(ctx, path, name, value, size);
 }
 
-int fsetxattrat_nofollow(int dirfd, const char *filename, const char *name,
-                         void *value, size_t size, int flags)
-{
-    char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
-    int ret;
-
-    ret = lsetxattr(proc_path, name, value, size, flags);
-    g_free(proc_path);
-    return ret;
-}
-
 ssize_t local_setxattr_nofollow(FsContext *ctx, const char *path,
                                 const char *name, void *value, size_t size,
                                 int flags)
@@ -235,17 +213,6 @@ int pt_setxattr(FsContext *ctx, const char *path, const char *name, void *value,
     return local_setxattr_nofollow(ctx, path, name, value, size, flags);
 }
 
-static ssize_t fremovexattrat_nofollow(int dirfd, const char *filename,
-                                       const char *name)
-{
-    char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
-    int ret;
-
-    ret = lremovexattr(proc_path, name);
-    g_free(proc_path);
-    return ret;
-}
-
 ssize_t local_removexattr_nofollow(FsContext *ctx, const char *path,
                                    const char *name)
 {
-- 
2.8.1

Re: [Qemu-devel] [PATCH 03/13] 9p: Move a couple xattr functions to 9p-util

[Greg Kurz]

On Sat, 26 May 2018 01:23:05 -0400
keno@juliacomputing.com wrote:

> From: Keno Fischer <keno@alumni.harvard.edu>
> 
> These functions will need custom implementations on Darwin. Since the
> implementation is very similar among all of them, and 9p-util already
> has the _nofollow version of fgetxattrat, let's move them all there.
> 

I'm ok with this move, but if the functions need to have distinct
implementations, and they really do according to patch 10, then
I'd rather have distinct files and rely on conditional building in
the makefile. Maybe rename the current file to 9p-util-linux.c
and introduce a 9p-util-darwin.c later.

> Additionally, introduce a _follow version of fgetxattr and use it.
> On darwin, fgetxattr has a more general interface, so we'll need to factor
> it out anyway.
> 

No need for the _follow version in this case.

> Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> ---
>  hw/9pfs/9p-local.c | 11 +++++++----
>  hw/9pfs/9p-util.c  | 39 +++++++++++++++++++++++++++++++++++++++
>  hw/9pfs/9p-util.h  |  6 ++++++
>  hw/9pfs/9p-xattr.c | 33 ---------------------------------
>  4 files changed, 52 insertions(+), 37 deletions(-)
> 
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index 7592f8d..fd65d04 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -776,16 +776,19 @@ 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 (fgetxattr_follow(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 (fgetxattr_follow(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 (fgetxattr_follow(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 (fgetxattr_follow(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.c b/hw/9pfs/9p-util.c
> index f709c27..8cf5554 100644
> --- a/hw/9pfs/9p-util.c
> +++ b/hw/9pfs/9p-util.c
> @@ -24,3 +24,42 @@ ssize_t fgetxattrat_nofollow(int dirfd, const char *filename, const char *name,
>      g_free(proc_path);
>      return ret;
>  }
> +
> +ssize_t fgetxattr_follow(int fd, const char *name,
> +                         void *value, size_t size)
> +{
> +    return fgetxattr(fd, name, value, size);
> +}
> +
> +ssize_t flistxattrat_nofollow(int dirfd, const char *filename,
> +                              char *list, size_t size)
> +{
> +    char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
> +    int ret;
> +
> +    ret = llistxattr(proc_path, list, size);
> +    g_free(proc_path);
> +    return ret;
> +}
> +
> +ssize_t fremovexattrat_nofollow(int dirfd, const char *filename,
> +                                const char *name)
> +{
> +    char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
> +    int ret;
> +
> +    ret = lremovexattr(proc_path, name);
> +    g_free(proc_path);
> +    return ret;
> +}
> +
> +int fsetxattrat_nofollow(int dirfd, const char *filename, const char *name,
> +                         void *value, size_t size, int flags)
> +{
> +    char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
> +    int ret;
> +
> +    ret = lsetxattr(proc_path, name, value, size, flags);
> +    g_free(proc_path);
> +    return ret;
> +}
> diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h
> index dc0d2e2..cb26343 100644
> --- a/hw/9pfs/9p-util.h
> +++ b/hw/9pfs/9p-util.h
> @@ -58,7 +58,13 @@ static inline int openat_file(int dirfd, const char *name, int flags,
>  
>  ssize_t fgetxattrat_nofollow(int dirfd, const char *path, const char *name,
>                               void *value, size_t size);
> +ssize_t fgetxattr_follow(int fd, const char *name,
> +                         void *value, size_t size);
>  int fsetxattrat_nofollow(int dirfd, const char *path, const char *name,
>                           void *value, size_t size, int flags);
> +ssize_t flistxattrat_nofollow(int dirfd, const char *filename,
> +                              char *list, size_t size);
> +ssize_t fremovexattrat_nofollow(int dirfd, const char *filename,
> +                                const char *name);
>  
>  #endif
> diff --git a/hw/9pfs/9p-xattr.c b/hw/9pfs/9p-xattr.c
> index d05c1a1..c696d8f 100644
> --- a/hw/9pfs/9p-xattr.c
> +++ b/hw/9pfs/9p-xattr.c
> @@ -60,17 +60,6 @@ ssize_t pt_listxattr(FsContext *ctx, const char *path,
>      return name_size;
>  }
>  
> -static ssize_t flistxattrat_nofollow(int dirfd, const char *filename,
> -                                     char *list, size_t size)
> -{
> -    char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
> -    int ret;
> -
> -    ret = llistxattr(proc_path, list, size);
> -    g_free(proc_path);
> -    return ret;
> -}
> -
>  /*
>   * Get the list and pass to each layer to find out whether
>   * to send the data or not
> @@ -196,17 +185,6 @@ ssize_t pt_getxattr(FsContext *ctx, const char *path, const char *name,
>      return local_getxattr_nofollow(ctx, path, name, value, size);
>  }
>  
> -int fsetxattrat_nofollow(int dirfd, const char *filename, const char *name,
> -                         void *value, size_t size, int flags)
> -{
> -    char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
> -    int ret;
> -
> -    ret = lsetxattr(proc_path, name, value, size, flags);
> -    g_free(proc_path);
> -    return ret;
> -}
> -
>  ssize_t local_setxattr_nofollow(FsContext *ctx, const char *path,
>                                  const char *name, void *value, size_t size,
>                                  int flags)
> @@ -235,17 +213,6 @@ int pt_setxattr(FsContext *ctx, const char *path, const char *name, void *value,
>      return local_setxattr_nofollow(ctx, path, name, value, size, flags);
>  }
>  
> -static ssize_t fremovexattrat_nofollow(int dirfd, const char *filename,
> -                                       const char *name)
> -{
> -    char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
> -    int ret;
> -
> -    ret = lremovexattr(proc_path, name);
> -    g_free(proc_path);
> -    return ret;
> -}
> -
>  ssize_t local_removexattr_nofollow(FsContext *ctx, const char *path,
>                                     const char *name)
>  {

Re: [Qemu-devel] [PATCH 03/13] 9p: Move a couple xattr functions to 9p-util

[Keno Fischer]

> I'm ok with this move, but if the functions need to have distinct
> implementations, and they really do according to patch 10, then
> I'd rather have distinct files and rely on conditional building in
> the makefile. Maybe rename the current file to 9p-util-linux.c
> and introduce a 9p-util-darwin.c later.

Sounds good, I will make this change.

>> Additionally, introduce a _follow version of fgetxattr and use it.
>> On darwin, fgetxattr has a more general interface, so we'll need to factor
>> it out anyway.
>>
>
> No need for the _follow version in this case.

I'm not entirely sure what you're proposing. On darwin `fgetxattr`
takes two extra
arguments, so I believe it needs to be factored out nevertheless. Are you just
proposing doing that later in the patch series?

Re: [Qemu-devel] [PATCH 03/13] 9p: Move a couple xattr functions to 9p-util

[Greg Kurz]

On Thu, 31 May 2018 12:14:30 -0400
Keno Fischer <keno@juliacomputing.com> wrote:

> > I'm ok with this move, but if the functions need to have distinct
> > implementations, and they really do according to patch 10, then
> > I'd rather have distinct files and rely on conditional building in
> > the makefile. Maybe rename the current file to 9p-util-linux.c
> > and introduce a 9p-util-darwin.c later.  
> 
> Sounds good, I will make this change.
> 
> >> Additionally, introduce a _follow version of fgetxattr and use it.
> >> On darwin, fgetxattr has a more general interface, so we'll need to factor
> >> it out anyway.
> >>  
> >
> > No need for the _follow version in this case.  
> 
> I'm not entirely sure what you're proposing. On darwin `fgetxattr`
> takes two extra
> arguments,

Oops you're right... so we indeed need to handle this conflicting APIs,
but fgetxattr_follow() is inapropriate, because fgetxattr() has nothing
to follow since it already has an fd... The same goes with Darwin's
version actually. The "nofollow" thingy only makes sense for those calls
that have a dirfd and pathname argument.

The OSX manpage for fgetxattr() seem to mention the XATTR_NOFOLLOW flag
for getxattr() only.

https://www.unix.com/man-page/osx/2/fgetxattr/

XATTR_NOFOLLOW  do not follow symbolic links.  getxattr() normally returns
		information from the target of path if it is a symbolic link.
		With this option, getxattr() will return extended attribute
		data from the symbolic link instead.

> so I believe it needs to be factored out nevertheless. Are you just
> proposing doing that later in the patch series?

Maybe introduce a qemu_fgetxattr() API in 9p-utils.h ? In a separate patch, or
maybe in patch 10.

#ifdef CONFIG_DARWIN
#define qemu_fgetxattr(...) fgetxattr(__VA_ARGS__, 0, 0)
#else
#define qemu_fgetxattr fgetxattr
#endif

Re: [Qemu-devel] [PATCH 03/13] 9p: Move a couple xattr functions to 9p-util

[Keno Fischer]

> Oops you're right... so we indeed need to handle this conflicting APIs,
> but fgetxattr_follow() is inapropriate, because fgetxattr() has nothing
> to follow since it already has an fd... The same goes with Darwin's
> version actually. The "nofollow" thingy only makes sense for those calls
> that have a dirfd and pathname argument.
>
> The OSX manpage for fgetxattr() seem to mention the XATTR_NOFOLLOW flag
> for getxattr() only.
>
> https://www.unix.com/man-page/osx/2/fgetxattr/
>
> XATTR_NOFOLLOW  do not follow symbolic links.  getxattr() normally returns
>                 information from the target of path if it is a symbolic link.
>                 With this option, getxattr() will return extended attribute
>                 data from the symbolic link instead.

Ah sorry, you're correct of course. Will fix.

>> so I believe it needs to be factored out nevertheless. Are you just
>> proposing doing that later in the patch series?
>
> Maybe introduce a qemu_fgetxattr() API in 9p-utils.h ? In a separate patch, or
> maybe in patch 10.
>
> #ifdef CONFIG_DARWIN
> #define qemu_fgetxattr(...) fgetxattr(__VA_ARGS__, 0, 0)
> #else
> #define qemu_fgetxattr fgetxattr
> #endif
>

Sounds good. I'll do this in a separate patch, since the *at versions
are all similar while
this is a bit different.

[Qemu-devel] [PATCH 04/13] 9p: darwin: Handle struct stat(fs) differences

[keno]

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

Signed-off-by: Keno Fischer <keno@juliacomputing.com>
---
 fsdev/file-op-9p.h |  4 ++++
 hw/9pfs/9p-proxy.c | 17 ++++++++++++++---
 hw/9pfs/9p-synth.c |  2 ++
 hw/9pfs/9p.c       | 16 ++++++++++++++--
 4 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
index a13e729..cf9d18d 100644
--- a/fsdev/file-op-9p.h
+++ b/fsdev/file-op-9p.h
@@ -19,6 +19,10 @@
 #ifdef CONFIG_LINUX
 #include <sys/vfs.h>
 #endif
+#ifdef CONFIG_DARWIN
+# include <sys/param.h>
+# include <sys/mount.h>
+#endif
 #include "qemu-fsdev-throttle.h"
 
 #define SM_LOCAL_MODE_BITS    0600
diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c
index e2e0329..d4572fb 100644
--- a/hw/9pfs/9p-proxy.c
+++ b/hw/9pfs/9p-proxy.c
@@ -117,10 +117,15 @@ static void prstatfs_to_statfs(struct statfs *stfs, ProxyStatFS *prstfs)
     stfs->f_bavail = prstfs->f_bavail;
     stfs->f_files = prstfs->f_files;
     stfs->f_ffree = prstfs->f_ffree;
+#ifdef CONFIG_DARWIN
+    stfs->f_fsid.val[0] = prstfs->f_fsid[0] & 0xFFFFFFFFU;
+    stfs->f_fsid.val[1] = prstfs->f_fsid[1] >> 32 & 0xFFFFFFFFU;
+#else
     stfs->f_fsid.__val[0] = prstfs->f_fsid[0] & 0xFFFFFFFFU;
     stfs->f_fsid.__val[1] = prstfs->f_fsid[1] >> 32 & 0xFFFFFFFFU;
     stfs->f_namelen = prstfs->f_namelen;
     stfs->f_frsize = prstfs->f_frsize;
+#endif
 }
 
 /* Converts proxy_stat structure to VFS stat structure */
@@ -137,12 +142,18 @@ static void prstat_to_stat(struct stat *stbuf, ProxyStat *prstat)
    stbuf->st_size = prstat->st_size;
    stbuf->st_blksize = prstat->st_blksize;
    stbuf->st_blocks = prstat->st_blocks;
-   stbuf->st_atim.tv_sec = prstat->st_atim_sec;
-   stbuf->st_atim.tv_nsec = prstat->st_atim_nsec;
+   stbuf->st_atime = prstat->st_atim_sec;
    stbuf->st_mtime = prstat->st_mtim_sec;
-   stbuf->st_mtim.tv_nsec = prstat->st_mtim_nsec;
    stbuf->st_ctime = prstat->st_ctim_sec;
+#ifdef CONFIG_DARWIN
+   stbuf->st_atimespec.tv_nsec = prstat->st_atim_nsec;
+   stbuf->st_mtimespec.tv_nsec = prstat->st_mtim_nsec;
+   stbuf->st_ctimespec.tv_nsec = prstat->st_ctim_nsec;
+#else
+   stbuf->st_atim.tv_nsec = prstat->st_atim_nsec;
+   stbuf->st_mtim.tv_nsec = prstat->st_mtim_nsec;
    stbuf->st_ctim.tv_nsec = prstat->st_ctim_nsec;
+#endif
 }
 
 /*
diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
index 54239c9..eb68b42 100644
--- a/hw/9pfs/9p-synth.c
+++ b/hw/9pfs/9p-synth.c
@@ -426,7 +426,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 d74302d..1cfdf7d 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -905,11 +905,17 @@ static void stat_to_v9stat_dotl(V9fsState *s, const struct stat *stbuf,
     v9lstat->st_blksize = stbuf->st_blksize;
     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;
 
@@ -2950,9 +2956,15 @@ static int v9fs_fill_statfs(V9fsState *s, V9fsPDU *pdu, struct statfs *stbuf)
     f_bavail = stbuf->f_bavail/bsize_factor;
     f_files  = stbuf->f_files;
     f_ffree  = stbuf->f_ffree;
+#ifdef CONFIG_DARWIN
+    fsid_val = (unsigned int)stbuf->f_fsid.val[0] |
+               (unsigned long long)stbuf->f_fsid.val[1] << 32;
+    f_namelen = MAXNAMLEN;
+#else
     fsid_val = (unsigned int) stbuf->f_fsid.__val[0] |
                (unsigned long long)stbuf->f_fsid.__val[1] << 32;
     f_namelen = stbuf->f_namelen;
+#endif
 
     return pdu_marshal(pdu, offset, "ddqqqqqqd",
                        f_type, f_bsize, f_blocks, f_bfree,
-- 
2.8.1

[Qemu-devel] [PATCH 05/13] 9p: darwin: Handle struct dirent differences

[keno]

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

Signed-off-by: Keno Fischer <keno@juliacomputing.com>
---
 hw/9pfs/9p-synth.c |  4 ++++
 hw/9pfs/9p.c       | 18 ++++++++++++++++--
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
index eb68b42..3c0a6d8 100644
--- a/hw/9pfs/9p-synth.c
+++ b/hw/9pfs/9p-synth.c
@@ -221,7 +221,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.c b/hw/9pfs/9p.c
index 1cfdf7d..49654ae 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1801,7 +1801,11 @@ static int coroutine_fn v9fs_do_readdir_with_stat(V9fsPDU *pdu,
         count += len;
         v9fs_stat_free(&v9stat);
         v9fs_path_free(&path);
+#ifdef CONFIG_DARWIN
+        saved_dir_pos = v9fs_co_telldir(pdu, fidp);
+#else
         saved_dir_pos = dent->d_off;
+#endif
     }
 
     v9fs_readdir_unlock(&fidp->fs.dir);
@@ -1952,9 +1956,19 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu, V9fsFidState *fidp,
         qid.type = 0;
         qid.version = 0;
 
+#ifdef CONFIG_DARWIN
+        // Darwin has d_seekoff, which appears to function
+        // analogously to d_off. However, it does not appear
+        // to be supported on all file systems, so use
+        // telldir for correctness.
+        off_t off = v9fs_co_telldir(pdu, fidp);
+#else
+        off_t off = dent->d_off;
+#endif
+
         /* 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_readdir_unlock(&fidp->fs.dir);
@@ -1966,7 +1980,7 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu, V9fsFidState *fidp,
         }
         count += len;
         v9fs_string_free(&name);
-        saved_dir_pos = dent->d_off;
+        saved_dir_pos = off;
     }
 
     v9fs_readdir_unlock(&fidp->fs.dir);
-- 
2.8.1

Re: [Qemu-devel] [PATCH 05/13] 9p: darwin: Handle struct dirent differences

[Greg Kurz]

On Sat, 26 May 2018 01:23:07 -0400
keno@juliacomputing.com wrote:

> From: Keno Fischer <keno@alumni.harvard.edu>
> 
> Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> ---
>  hw/9pfs/9p-synth.c |  4 ++++
>  hw/9pfs/9p.c       | 18 ++++++++++++++++--
>  2 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
> index eb68b42..3c0a6d8 100644
> --- a/hw/9pfs/9p-synth.c
> +++ b/hw/9pfs/9p-synth.c
> @@ -221,7 +221,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;

Hmm... what's that for ? No users in the patchset and the comment
below seem to indicate this wouldn't be trusted anyway.

> +#else
>      entry->d_off = off + 1;
> +#endif
>  }
>  
>  static struct dirent *synth_get_dentry(V9fsSynthNode *dir,
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 1cfdf7d..49654ae 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -1801,7 +1801,11 @@ static int coroutine_fn v9fs_do_readdir_with_stat(V9fsPDU *pdu,
>          count += len;
>          v9fs_stat_free(&v9stat);
>          v9fs_path_free(&path);
> +#ifdef CONFIG_DARWIN
> +        saved_dir_pos = v9fs_co_telldir(pdu, fidp);
> +#else
>          saved_dir_pos = dent->d_off;
> +#endif
>      }
>  
>      v9fs_readdir_unlock(&fidp->fs.dir);
> @@ -1952,9 +1956,19 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu, V9fsFidState *fidp,
>          qid.type = 0;
>          qid.version = 0;
>  
> +#ifdef CONFIG_DARWIN
> +        // Darwin has d_seekoff, which appears to function
> +        // analogously to d_off. However, it does not appear
> +        // to be supported on all file systems, so use
> +        // telldir for correctness.

Please use /* */

> +        off_t off = v9fs_co_telldir(pdu, fidp);

Please declare local variables at the beginning of the function. Also,
v9fs_co_telldir() can fail. This requires proper error handling.

> +#else
> +        off_t off = dent->d_off;
> +#endif

Please make this a helper and call it in v9fs_do_readdir_with_stat() as well.

> +
>          /* 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_readdir_unlock(&fidp->fs.dir);
> @@ -1966,7 +1980,7 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu, V9fsFidState *fidp,
>          }
>          count += len;
>          v9fs_string_free(&name);
> -        saved_dir_pos = dent->d_off;
> +        saved_dir_pos = off;
>      }
>  
>      v9fs_readdir_unlock(&fidp->fs.dir);

Re: [Qemu-devel] [PATCH 05/13] 9p: darwin: Handle struct dirent differences

[Keno Fischer]

>> diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
>> index eb68b42..3c0a6d8 100644
>> --- a/hw/9pfs/9p-synth.c
>> +++ b/hw/9pfs/9p-synth.c
>> @@ -221,7 +221,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;
>
> Hmm... what's that for ? No users in the patchset and the comment
> below seem to indicate this wouldn't be trusted anyway.

d_off isn't available on Darwin, so an appropriate ifdef
is required here anyway. I figured if the offset is available
anyway, I might as well set it, but I can drop
this code path if you would prefer.

>> +        off_t off = v9fs_co_telldir(pdu, fidp);
>
> Please declare local variables at the beginning of the function. Also,
> v9fs_co_telldir() can fail. This requires proper error handling.

Will do.

>> +#else
>> +        off_t off = dent->d_off;
>> +#endif
>
> Please make this a helper and call it in v9fs_do_readdir_with_stat() as well.
>

Will do.

Re: [Qemu-devel] [PATCH 05/13] 9p: darwin: Handle struct dirent differences

[Greg Kurz]

On Thu, 31 May 2018 12:20:28 -0400
Keno Fischer <keno@juliacomputing.com> wrote:

> >> diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
> >> index eb68b42..3c0a6d8 100644
> >> --- a/hw/9pfs/9p-synth.c
> >> +++ b/hw/9pfs/9p-synth.c
> >> @@ -221,7 +221,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;  
> >
> > Hmm... what's that for ? No users in the patchset and the comment
> > below seem to indicate this wouldn't be trusted anyway.  
> 
> d_off isn't available on Darwin, so an appropriate ifdef
> is required here anyway. I figured if the offset is available
> anyway, I might as well set it, but I can drop
> this code path if you would prefer.
> 

Yeah I think I prefer you drop it.

> >> +        off_t off = v9fs_co_telldir(pdu, fidp);  
> >
> > Please declare local variables at the beginning of the function. Also,
> > v9fs_co_telldir() can fail. This requires proper error handling.  
> 
> Will do.
> 
> >> +#else
> >> +        off_t off = dent->d_off;
> >> +#endif  
> >
> > Please make this a helper and call it in v9fs_do_readdir_with_stat() as well.
> >  
> 
> Will do.

[Qemu-devel] [PATCH 06/13] 9p: darwin: Address minor differences

[keno]

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

- Darwin doesn't have strchrnul
- Comparisons of mode_t with -1 require an explicit cast, since mode_t
  is unsigned on Darwin.

Signed-off-by: Keno Fischer <keno@juliacomputing.com>
---
 hw/9pfs/9p-local.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index fd65d04..6e0b2e8 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -67,7 +67,10 @@ int local_open_nofollow(FsContext *fs_ctx, const char *path, int flags,
         assert(*path != '/');
 
         head = g_strdup(path);
-        c = strchrnul(path, '/');
+        /* equivalent to strchrnul(), but that is not available on Darwin */
+        c = strchr(path, '/');
+        if (!c)
+            c = path + strlen(path);
         if (*c) {
             /* Intermediate path element */
             head[c - path] = 0;
@@ -310,7 +313,7 @@ update_map_file:
     if (credp->fc_gid != -1) {
         gid = credp->fc_gid;
     }
-    if (credp->fc_mode != -1) {
+    if (credp->fc_mode != (mode_t)-1) {
         mode = credp->fc_mode;
     }
     if (credp->fc_rdev != -1) {
@@ -416,7 +419,7 @@ static int local_set_xattrat(int dirfd, const char *path, FsCred *credp)
             return err;
         }
     }
-    if (credp->fc_mode != -1) {
+    if (credp->fc_mode != (mode_t)-1) {
         uint32_t tmp_mode = cpu_to_le32(credp->fc_mode);
         err = fsetxattrat_nofollow(dirfd, path, "user.virtfs.mode", &tmp_mode,
                                    sizeof(mode_t), 0);
-- 
2.8.1

Re: [Qemu-devel] [PATCH 06/13] 9p: darwin: Address minor differences

[Greg Kurz]

On Sat, 26 May 2018 01:23:08 -0400
keno@juliacomputing.com wrote:

> From: Keno Fischer <keno@alumni.harvard.edu>
> 
> - Darwin doesn't have strchrnul
> - Comparisons of mode_t with -1 require an explicit cast, since mode_t
>   is unsigned on Darwin.
> 
> Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> ---
>  hw/9pfs/9p-local.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index fd65d04..6e0b2e8 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -67,7 +67,10 @@ int local_open_nofollow(FsContext *fs_ctx, const char *path, int flags,
>          assert(*path != '/');
>  
>          head = g_strdup(path);
> -        c = strchrnul(path, '/');
> +        /* equivalent to strchrnul(), but that is not available on Darwin */

Please make a qemu_strchrnul() helper with a separate implementation for Darwin
then. I guess you can put it in this file since there aren't any other users in
the QEMU code base.

> +        c = strchr(path, '/');
> +        if (!c)
> +            c = path + strlen(path);
>          if (*c) {
>              /* Intermediate path element */
>              head[c - path] = 0;
> @@ -310,7 +313,7 @@ update_map_file:
>      if (credp->fc_gid != -1) {
>          gid = credp->fc_gid;
>      }
> -    if (credp->fc_mode != -1) {
> +    if (credp->fc_mode != (mode_t)-1) {
>          mode = credp->fc_mode;
>      }
>      if (credp->fc_rdev != -1) {
> @@ -416,7 +419,7 @@ static int local_set_xattrat(int dirfd, const char *path, FsCred *credp)
>              return err;
>          }
>      }
> -    if (credp->fc_mode != -1) {
> +    if (credp->fc_mode != (mode_t)-1) {
>          uint32_t tmp_mode = cpu_to_le32(credp->fc_mode);
>          err = fsetxattrat_nofollow(dirfd, path, "user.virtfs.mode", &tmp_mode,
>                                     sizeof(mode_t), 0);

Re: [Qemu-devel] [PATCH 06/13] 9p: darwin: Address minor differences

[Keno Fischer]

>> --- a/hw/9pfs/9p-local.c
>> +++ b/hw/9pfs/9p-local.c
>> @@ -67,7 +67,10 @@ int local_open_nofollow(FsContext *fs_ctx, const char *path, int flags,
>>          assert(*path != '/');
>>
>>          head = g_strdup(path);
>> -        c = strchrnul(path, '/');
>> +        /* equivalent to strchrnul(), but that is not available on Darwin */
>
> Please make a qemu_strchrnul() helper with a separate implementation for Darwin
> then. I guess you can put it in this file since there aren't any other users in
> the QEMU code base.

There actually are, but they also use this pattern. Could you
suggest the best place to put this utility? I can submit a patch
to switch all instances of this pattern over.

Re: [Qemu-devel] [PATCH 06/13] 9p: darwin: Address minor differences

[Greg Kurz]

On Thu, 31 May 2018 12:27:35 -0400
Keno Fischer <keno@juliacomputing.com> wrote:

> >> --- a/hw/9pfs/9p-local.c
> >> +++ b/hw/9pfs/9p-local.c
> >> @@ -67,7 +67,10 @@ int local_open_nofollow(FsContext *fs_ctx, const char *path, int flags,
> >>          assert(*path != '/');
> >>
> >>          head = g_strdup(path);
> >> -        c = strchrnul(path, '/');
> >> +        /* equivalent to strchrnul(), but that is not available on Darwin */  
> >
> > Please make a qemu_strchrnul() helper with a separate implementation for Darwin
> > then. I guess you can put it in this file since there aren't any other users in
> > the QEMU code base.  
> 
> There actually are, but they also use this pattern. Could you
> suggest the best place to put this utility? I can submit a patch
> to switch all instances of this pattern over.

Oh if the pattern is already used in other places, it's probably not
worth the pain... so please forget this :)

Re: [Qemu-devel] [PATCH 06/13] 9p: darwin: Address minor differences

[Keno Fischer]

Well, I do have the patch already to switch this and the other patterns,
so let me know if you want it or not ;).

On Thu, May 31, 2018 at 3:22 PM, Greg Kurz <groug@kaod.org> wrote:
> On Thu, 31 May 2018 12:27:35 -0400
> Keno Fischer <keno@juliacomputing.com> wrote:
>
>> >> --- a/hw/9pfs/9p-local.c
>> >> +++ b/hw/9pfs/9p-local.c
>> >> @@ -67,7 +67,10 @@ int local_open_nofollow(FsContext *fs_ctx, const char *path, int flags,
>> >>          assert(*path != '/');
>> >>
>> >>          head = g_strdup(path);
>> >> -        c = strchrnul(path, '/');
>> >> +        /* equivalent to strchrnul(), but that is not available on Darwin */
>> >
>> > Please make a qemu_strchrnul() helper with a separate implementation for Darwin
>> > then. I guess you can put it in this file since there aren't any other users in
>> > the QEMU code base.
>>
>> There actually are, but they also use this pattern. Could you
>> suggest the best place to put this utility? I can submit a patch
>> to switch all instances of this pattern over.
>
> Oh if the pattern is already used in other places, it's probably not
> worth the pain... so please forget this :)

Re: [Qemu-devel] [PATCH 06/13] 9p: darwin: Address minor differences

[Greg Kurz]

On Thu, 31 May 2018 15:23:45 -0400
Keno Fischer <keno@juliacomputing.com> wrote:

> Well, I do have the patch already to switch this and the other patterns,
> so let me know if you want it or not ;).
> 

Post it then and we'll see if people are happy with that :)

> On Thu, May 31, 2018 at 3:22 PM, Greg Kurz <groug@kaod.org> wrote:
> > On Thu, 31 May 2018 12:27:35 -0400
> > Keno Fischer <keno@juliacomputing.com> wrote:
> >  
> >> >> --- a/hw/9pfs/9p-local.c
> >> >> +++ b/hw/9pfs/9p-local.c
> >> >> @@ -67,7 +67,10 @@ int local_open_nofollow(FsContext *fs_ctx, const char *path, int flags,
> >> >>          assert(*path != '/');
> >> >>
> >> >>          head = g_strdup(path);
> >> >> -        c = strchrnul(path, '/');
> >> >> +        /* equivalent to strchrnul(), but that is not available on Darwin */  
> >> >
> >> > Please make a qemu_strchrnul() helper with a separate implementation for Darwin
> >> > then. I guess you can put it in this file since there aren't any other users in
> >> > the QEMU code base.  
> >>
> >> There actually are, but they also use this pattern. Could you
> >> suggest the best place to put this utility? I can submit a patch
> >> to switch all instances of this pattern over.  
> >
> > Oh if the pattern is already used in other places, it's probably not
> > worth the pain... so please forget this :)  

[Qemu-devel] [PATCH 07/13] 9p: darwin: Properly translate AT_REMOVEDIR flag

[keno]

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

This code relied on P9_DOTL_AT_REMOVEDIR and AT_REMOVEDIR having the same
numerical value, but on Darwin, they do not.

Signed-off-by: Keno Fischer <keno@juliacomputing.com>
---
 hw/9pfs/9p-local.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 6e0b2e8..c55ea25 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1376,7 +1376,17 @@ static int local_unlinkat(FsContext *ctx, V9fsPath *dir,
         return -1;
     }
 
-    ret = local_unlinkat_common(ctx, dirfd, name, flags);
+    if ((flags & ~P9_DOTL_AT_REMOVEDIR) != 0) {
+        errno = EINVAL;
+        return -1;
+    }
+
+    size_t rflags = 0;
+    if (flags & P9_DOTL_AT_REMOVEDIR) {
+        rflags |= AT_REMOVEDIR;
+    }
+
+    ret = local_unlinkat_common(ctx, dirfd, name, rflags);
     close_preserve_errno(dirfd);
     return ret;
 }
-- 
2.8.1

Re: [Qemu-devel] [PATCH 07/13] 9p: darwin: Properly translate AT_REMOVEDIR flag

[Greg Kurz]

On Sat, 26 May 2018 01:23:09 -0400
keno@juliacomputing.com wrote:

> From: Keno Fischer <keno@alumni.harvard.edu>
> 
> This code relied on P9_DOTL_AT_REMOVEDIR and AT_REMOVEDIR having the same
> numerical value, but on Darwin, they do not.
> 
> Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> ---
>  hw/9pfs/9p-local.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index 6e0b2e8..c55ea25 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -1376,7 +1376,17 @@ static int local_unlinkat(FsContext *ctx, V9fsPath *dir,
>          return -1;
>      }
>  
> -    ret = local_unlinkat_common(ctx, dirfd, name, flags);
> +    if ((flags & ~P9_DOTL_AT_REMOVEDIR) != 0) {

The != 0 isn't needed but...

> +        errno = EINVAL;
> +        return -1;

... I'm more concerned about this new error path. How can this happen ?

> +    }
> +
> +    size_t rflags = 0;

Please declare this at the beginning of the function.

> +    if (flags & P9_DOTL_AT_REMOVEDIR) {
> +        rflags |= AT_REMOVEDIR;
> +    }
> +
> +    ret = local_unlinkat_common(ctx, dirfd, name, rflags);
>      close_preserve_errno(dirfd);
>      return ret;
>  }

Re: [Qemu-devel] [PATCH 07/13] 9p: darwin: Properly translate AT_REMOVEDIR flag

[Keno Fischer]

>> +        errno = EINVAL;
>> +        return -1;
>
> ... I'm more concerned about this new error path. How can this happen ?
>

As far as I can tell, the flags come from the client without any
intermediate error
checking. Since the Darwin constants do not match the Linux constants (which
have the same numerical values as the 9p constants), we need to perform this
checking/translation somewhere to ensure correct behavior.
Is there a more appropriate place to put this check?

Re: [Qemu-devel] [PATCH 07/13] 9p: darwin: Properly translate AT_REMOVEDIR flag

[Greg Kurz]

On Thu, 31 May 2018 12:25:49 -0400
Keno Fischer <keno@juliacomputing.com> wrote:

> >> +        errno = EINVAL;
> >> +        return -1;  
> >
> > ... I'm more concerned about this new error path. How can this happen ?
> >  
> 
> As far as I can tell, the flags come from the client without any
> intermediate error
> checking. 

Indeed :-\

> Since the Darwin constants do not match the Linux constants (which
> have the same numerical values as the 9p constants), we need to perform this
> checking/translation somewhere to ensure correct behavior.
> Is there a more appropriate place to put this check?

The right thing to do would be to check and translate the flag from
P9_DOTL_AT_REMOVEDIR to AT_REMOVEDIR in the core 9p server code.

[Qemu-devel] [PATCH 08/13] 9p: darwin: Ignore O_{NOATIME, DIRECT}

[keno]

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

Signed-off-by: Keno Fischer <keno@juliacomputing.com>
---
 hw/9pfs/9p.c | 37 +++++++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 49654ae..f5f00aa 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -115,20 +115,27 @@ static int dotl_to_open_flags(int flags)
     int oflags = flags & O_ACCMODE;
 
     DotlOpenflagMap dotl_oflag_map[] = {
-        { P9_DOTL_CREATE, O_CREAT },
-        { P9_DOTL_EXCL, O_EXCL },
-        { P9_DOTL_NOCTTY , O_NOCTTY },
-        { P9_DOTL_TRUNC, O_TRUNC },
-        { P9_DOTL_APPEND, O_APPEND },
-        { P9_DOTL_NONBLOCK, O_NONBLOCK } ,
-        { P9_DOTL_DSYNC, O_DSYNC },
-        { P9_DOTL_FASYNC, FASYNC },
-        { P9_DOTL_DIRECT, O_DIRECT },
-        { 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 },
+        {P9_DOTL_CREATE, O_CREAT},
+        {P9_DOTL_EXCL, O_EXCL},
+        {P9_DOTL_NOCTTY, O_NOCTTY},
+        {P9_DOTL_TRUNC, O_TRUNC},
+        {P9_DOTL_APPEND, O_APPEND},
+        {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_SYNC, O_SYNC},
     };
 
     for (i = 0; i < ARRAY_SIZE(dotl_oflag_map); i++) {
@@ -156,10 +163,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.8.1

Re: [Qemu-devel] [PATCH 08/13] 9p: darwin: Ignore O_{NOATIME, DIRECT}

[Greg Kurz]

On Sat, 26 May 2018 01:23:10 -0400
keno@juliacomputing.com wrote:

> From: Keno Fischer <keno@alumni.harvard.edu>
> 
> Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> ---
>  hw/9pfs/9p.c | 37 +++++++++++++++++++++++--------------
>  1 file changed, 23 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 49654ae..f5f00aa 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -115,20 +115,27 @@ static int dotl_to_open_flags(int flags)
>      int oflags = flags & O_ACCMODE;
>  
>      DotlOpenflagMap dotl_oflag_map[] = {
> -        { P9_DOTL_CREATE, O_CREAT },
> -        { P9_DOTL_EXCL, O_EXCL },
> -        { P9_DOTL_NOCTTY , O_NOCTTY },
> -        { P9_DOTL_TRUNC, O_TRUNC },
> -        { P9_DOTL_APPEND, O_APPEND },
> -        { P9_DOTL_NONBLOCK, O_NONBLOCK } ,
> -        { P9_DOTL_DSYNC, O_DSYNC },
> -        { P9_DOTL_FASYNC, FASYNC },
> -        { P9_DOTL_DIRECT, O_DIRECT },
> -        { 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 },
> +        {P9_DOTL_CREATE, O_CREAT},
> +        {P9_DOTL_EXCL, O_EXCL},
> +        {P9_DOTL_NOCTTY, O_NOCTTY},
> +        {P9_DOTL_TRUNC, O_TRUNC},
> +        {P9_DOTL_APPEND, O_APPEND},
> +        {P9_DOTL_NONBLOCK, O_NONBLOCK},
> +        {P9_DOTL_DSYNC, O_DSYNC},
> +        {P9_DOTL_FASYNC, FASYNC},

Please don't kill the spaces.

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

But are these semantics worse than dumping the flag ?

> +           even on linux at the moment, so we just ignore
> +           it here. */

Yeah, and I doubt we'll ever support it on linux either. But,
anyway, why filter these out ? Do they cause a build break ?

> +        {P9_DOTL_DIRECT, O_DIRECT},
> +#endif
> +        {P9_DOTL_LARGEFILE, O_LARGEFILE},
> +        {P9_DOTL_DIRECTORY, O_DIRECTORY},
> +        {P9_DOTL_NOFOLLOW, O_NOFOLLOW},
> +        {P9_DOTL_SYNC, O_SYNC},
>      };
>  
>      for (i = 0; i < ARRAY_SIZE(dotl_oflag_map); i++) {
> @@ -156,10 +163,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;
>  }
>  

Re: [Qemu-devel] [PATCH 08/13] 9p: darwin: Ignore O_{NOATIME, DIRECT}

[Keno Fischer]

>
> Please don't kill the spaces.
>

Sorry, will undo. My editor has strong opinions about styling.

>> +#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
>
> But are these semantics worse than dumping the flag ?
>

I don't know. I looked around a bit and most OS abstraction
layers tend to not do this translation automatically:

https://github.com/libuv/libuv/issues/1600

>> +           even on linux at the moment, so we just ignore
>> +           it here. */
>
> Yeah, and I doubt we'll ever support it on linux either. But,
> anyway, why filter these out ? Do they cause a build break ?
>

Yes, neither O_DIRECT nor O_NOATIME are defined on Darwin,
so trying to use them causes errors.

[Qemu-devel] [PATCH 09/13] 9p: darwin: Provide a compatibility definition for XATTR_SIZE_MAX

[keno]

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

Signed-off-by: Keno Fischer <keno@juliacomputing.com>
---
 hw/9pfs/9p.c | 7 +++++++
 1 file changed, 7 insertions(+)

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

Re: [Qemu-devel] [PATCH 09/13] 9p: darwin: Provide a compatibility definition for XATTR_SIZE_MAX

[Peter Maydell]

On 26 May 2018 at 06:23,  <keno@juliacomputing.com> wrote:
> From: Keno Fischer <keno@alumni.harvard.edu>
>
> Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> ---
>  hw/9pfs/9p.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index f5f00aa..4ae4da6 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -3351,6 +3351,13 @@ out_nofid:
>      v9fs_string_free(&name);
>  }
>
> +#if defined(CONFIG_DARWIN) && !defined(XATTR_SIZE_MAX)
> +/* Darwin doesn't seem to define a maximum xattr size in its user
> +   user space header, but looking at the kernel source, HFS supports
> +   up to INT32_MAX, so use that as the maximum.
> +*/
> +#define XATTR_SIZE_MAX INT32_MAX
> +#endif

Do we really need the CONFIG_DARWIN part of this check?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 09/13] 9p: darwin: Provide a compatibility definition for XATTR_SIZE_MAX

[Keno Fischer]

> > +#if defined(CONFIG_DARWIN) && !defined(XATTR_SIZE_MAX)
> > +/* Darwin doesn't seem to define a maximum xattr size in its user
> > +   user space header, but looking at the kernel source, HFS supports
> > +   up to INT32_MAX, so use that as the maximum.
> > +*/
> > +#define XATTR_SIZE_MAX INT32_MAX
> > +#endif
>
> Do we really need the CONFIG_DARWIN part of this check?

Right now this code only runs on Linux (and Darwin after this series).
On Linux it's always defined,
but I'd rather this code give an error when somebody tries to port it
to a new OS than have it
silently use an incorrect value. The ` !defined(XATTR_SIZE_MAX)` is
just there in case Apple ever
decides to define it in their headers. I can remove that part if you
would prefer.

[Qemu-devel] [PATCH 10/13] 9p: darwin: *xattr_nofollow implementations

[keno]

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

Signed-off-by: Keno Fischer <keno@juliacomputing.com>
---
 hw/9pfs/9p-util.c | 49 +++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 45 insertions(+), 4 deletions(-)

diff --git a/hw/9pfs/9p-util.c b/hw/9pfs/9p-util.c
index 8cf5554..98004ac 100644
--- a/hw/9pfs/9p-util.c
+++ b/hw/9pfs/9p-util.c
@@ -17,49 +17,90 @@
 ssize_t fgetxattrat_nofollow(int dirfd, const char *filename, const char *name,
                              void *value, size_t size)
 {
-    char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
     int ret;
+#ifdef CONFIG_DARWIN
+    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, XATTR_NOFOLLOW);
+    close_preserve_errno(fd);
+#else
+    char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
 
     ret = lgetxattr(proc_path, name, value, size);
     g_free(proc_path);
+#endif
     return ret;
 }
 
 ssize_t fgetxattr_follow(int fd, const char *name,
                          void *value, size_t size)
 {
+#ifdef CONFIG_DARWIN
+    return fgetxattr(fd, name, value, size, 0, 0);
+#else
     return fgetxattr(fd, name, value, size);
+#endif
 }
 
 ssize_t flistxattrat_nofollow(int dirfd, const char *filename,
                               char *list, size_t size)
 {
-    char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
     int ret;
+#ifdef CONFIG_DARWIN
+    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, XATTR_NOFOLLOW);
+    close_preserve_errno(fd);
+#else
+    char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
 
     ret = llistxattr(proc_path, list, size);
     g_free(proc_path);
+#endif
     return ret;
 }
 
 ssize_t fremovexattrat_nofollow(int dirfd, const char *filename,
                                 const char *name)
 {
-    char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
     int ret;
+#ifdef CONFIG_DARWIN
+    int fd = openat_file(dirfd, filename, O_PATH_9P_UTIL | O_NOFOLLOW, 0);
+    if (fd == -1)
+        return -1;
+
+    ret = fremovexattr(fd, name, XATTR_NOFOLLOW);
+    close_preserve_errno(fd);
+    return ret;
+#else
+    char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
 
     ret = lremovexattr(proc_path, name);
     g_free(proc_path);
     return ret;
+#endif
 }
 
 int fsetxattrat_nofollow(int dirfd, const char *filename, const char *name,
                          void *value, size_t size, int flags)
 {
-    char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
     int ret;
+#ifdef CONFIG_DARWIN
+    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, XATTR_NOFOLLOW);
+    close_preserve_errno(fd);
+#else
+    char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
 
     ret = lsetxattr(proc_path, name, value, size, flags);
     g_free(proc_path);
+#endif
     return ret;
 }
-- 
2.8.1

Re: [Qemu-devel] [PATCH 10/13] 9p: darwin: *xattr_nofollow implementations

[Greg Kurz]

On Sat, 26 May 2018 01:23:12 -0400
keno@juliacomputing.com wrote:

> From: Keno Fischer <keno@alumni.harvard.edu>
> 
> Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> ---

As mentioned in patch 3, this should go to 9p-util-darwin.c

>  hw/9pfs/9p-util.c | 49 +++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 45 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/9pfs/9p-util.c b/hw/9pfs/9p-util.c
> index 8cf5554..98004ac 100644
> --- a/hw/9pfs/9p-util.c
> +++ b/hw/9pfs/9p-util.c
> @@ -17,49 +17,90 @@
>  ssize_t fgetxattrat_nofollow(int dirfd, const char *filename, const char *name,
>                               void *value, size_t size)
>  {
> -    char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
>      int ret;
> +#ifdef CONFIG_DARWIN
> +    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, XATTR_NOFOLLOW);
> +    close_preserve_errno(fd);
> +#else
> +    char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
>  
>      ret = lgetxattr(proc_path, name, value, size);
>      g_free(proc_path);
> +#endif
>      return ret;
>  }
>  
>  ssize_t fgetxattr_follow(int fd, const char *name,
>                           void *value, size_t size)
>  {
> +#ifdef CONFIG_DARWIN
> +    return fgetxattr(fd, name, value, size, 0, 0);
> +#else
>      return fgetxattr(fd, name, value, size);
> +#endif
>  }
>  
>  ssize_t flistxattrat_nofollow(int dirfd, const char *filename,
>                                char *list, size_t size)
>  {
> -    char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
>      int ret;
> +#ifdef CONFIG_DARWIN
> +    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, XATTR_NOFOLLOW);
> +    close_preserve_errno(fd);
> +#else
> +    char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
>  
>      ret = llistxattr(proc_path, list, size);
>      g_free(proc_path);
> +#endif
>      return ret;
>  }
>  
>  ssize_t fremovexattrat_nofollow(int dirfd, const char *filename,
>                                  const char *name)
>  {
> -    char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
>      int ret;
> +#ifdef CONFIG_DARWIN
> +    int fd = openat_file(dirfd, filename, O_PATH_9P_UTIL | O_NOFOLLOW, 0);
> +    if (fd == -1)
> +        return -1;
> +
> +    ret = fremovexattr(fd, name, XATTR_NOFOLLOW);
> +    close_preserve_errno(fd);
> +    return ret;
> +#else
> +    char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
>  
>      ret = lremovexattr(proc_path, name);
>      g_free(proc_path);
>      return ret;
> +#endif
>  }
>  
>  int fsetxattrat_nofollow(int dirfd, const char *filename, const char *name,
>                           void *value, size_t size, int flags)
>  {
> -    char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
>      int ret;
> +#ifdef CONFIG_DARWIN
> +    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, XATTR_NOFOLLOW);
> +    close_preserve_errno(fd);
> +#else
> +    char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
>  
>      ret = lsetxattr(proc_path, name, value, size, flags);
>      g_free(proc_path);
> +#endif
>      return ret;
>  }

[Qemu-devel] [PATCH 11/13] 9p: darwin: Mark mknod as unsupported

[keno]

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

Signed-off-by: Keno Fischer <keno@juliacomputing.com>
---
 hw/9pfs/9p-local.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index c55ea25..3e358b7 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -669,6 +669,13 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath *dir_path,
         return -1;
     }
 
+#ifdef CONFIG_DARWIN
+    /* Darwin doesn't have mknodat and it's unlikely to work anyway,
+       so let's just mark it as unsupported */
+    err = -1;
+    errno = EOPNOTSUPP;
+    goto out;
+#else
     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);
@@ -699,6 +706,8 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath *dir_path,
 
 err_end:
     unlinkat_preserve_errno(dirfd, name, 0);
+#endif
+
 out:
     close_preserve_errno(dirfd);
     return err;
-- 
2.8.1

Re: [Qemu-devel] [PATCH 11/13] 9p: darwin: Mark mknod as unsupported

[Greg Kurz]

On Sat, 26 May 2018 01:23:13 -0400
keno@juliacomputing.com wrote:

> From: Keno Fischer <keno@alumni.harvard.edu>
> 
> Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> ---
>  hw/9pfs/9p-local.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index c55ea25..3e358b7 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -669,6 +669,13 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath *dir_path,
>          return -1;
>      }
>  
> +#ifdef CONFIG_DARWIN
> +    /* Darwin doesn't have mknodat and it's unlikely to work anyway,

What's unlikely to work ?

> +       so let's just mark it as unsupported */
> +    err = -1;
> +    errno = EOPNOTSUPP;
> +    goto out;
> +#else

Please introduce qemu_mknodat() with distinct implementations for linux
and darwin.

>      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);
> @@ -699,6 +706,8 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath *dir_path,
>  
>  err_end:
>      unlinkat_preserve_errno(dirfd, name, 0);
> +#endif
> +
>  out:
>      close_preserve_errno(dirfd);
>      return err;

Re: [Qemu-devel] [PATCH 11/13] 9p: darwin: Mark mknod as unsupported

[Keno Fischer]

>> +#ifdef CONFIG_DARWIN
>> +    /* Darwin doesn't have mknodat and it's unlikely to work anyway,
>
> What's unlikely to work ?
>

My concern was that allowing this would cause unexpected
behavior, since the device numbers will differ between OS X
and Linux. Though maybe this isn't the place to worry about
that.

Re: [Qemu-devel] [PATCH 11/13] 9p: darwin: Mark mknod as unsupported

[Greg Kurz]

On Thu, 31 May 2018 12:37:56 -0400
Keno Fischer <keno@juliacomputing.com> wrote:

> >> +#ifdef CONFIG_DARWIN
> >> +    /* Darwin doesn't have mknodat and it's unlikely to work anyway,  
> >
> > What's unlikely to work ?
> >  
> 
> My concern was that allowing this would cause unexpected
> behavior, since the device numbers will differ between OS X
> and Linux. Though maybe this isn't the place to worry about
> that.

The numbers may differ indeed but we don't really care since the
server never opens device files. This is just a directory entry.

Re: [Qemu-devel] [PATCH 11/13] 9p: darwin: Mark mknod as unsupported

[Keno Fischer]

>> My concern was that allowing this would cause unexpected
>> behavior, since the device numbers will differ between OS X
>> and Linux. Though maybe this isn't the place to worry about
>> that.
>
> The numbers may differ indeed but we don't really care since the
> server never opens device files. This is just a directory entry.

Ok, let me try to implement it. However, I don't think it is possible
to implement mknodat (or at least I can't think of how) on Darwin
directly. I could use regular mknod, but presumably, this is used
to avoid a race condition between creating the device and setting
the permissions. Can you think of a good way to resolve that?

Re: [Qemu-devel] [PATCH 11/13] 9p: darwin: Mark mknod as unsupported

[Keno Fischer]

On Thu, May 31, 2018 at 6:56 PM, Keno Fischer <keno@juliacomputing.com> wrote:
>>> My concern was that allowing this would cause unexpected
>>> behavior, since the device numbers will differ between OS X
>>> and Linux. Though maybe this isn't the place to worry about
>>> that.
>>
>> The numbers may differ indeed but we don't really care since the
>> server never opens device files. This is just a directory entry.
>
> Ok, let me try to implement it. However, I don't think it is possible
> to implement mknodat (or at least I can't think of how) on Darwin
> directly. I could use regular mknod, but presumably, this is used
> to avoid a race condition between creating the device and setting
> the permissions. Can you think of a good way to resolve that?

Would it work to fchdir in to the directory and use a cwd-relative
mknod, then fchdir back? Or do we need to maintain the cwd
while in this code?

Re: [Qemu-devel] [PATCH 11/13] 9p: darwin: Mark mknod as unsupported

[Keno Fischer]

On Thu, May 31, 2018 at 7:06 PM, Keno Fischer <keno@juliacomputing.com> wrote:
> On Thu, May 31, 2018 at 6:56 PM, Keno Fischer <keno@juliacomputing.com> wrote:
>>>> My concern was that allowing this would cause unexpected
>>>> behavior, since the device numbers will differ between OS X
>>>> and Linux. Though maybe this isn't the place to worry about
>>>> that.
>>>
>>> The numbers may differ indeed but we don't really care since the
>>> server never opens device files. This is just a directory entry.
>>
>> Ok, let me try to implement it. However, I don't think it is possible
>> to implement mknodat (or at least I can't think of how) on Darwin
>> directly. I could use regular mknod, but presumably, this is used
>> to avoid a race condition between creating the device and setting
>> the permissions. Can you think of a good way to resolve that?
>
> Would it work to fchdir in to the directory and use a cwd-relative
> mknod, then fchdir back? Or do we need to maintain the cwd
> while in this code?

Sorry for the triple-self-post here, but I took a look at the Darwin kernel
source and there's an unexposed (from the Darwin C library) syscall that
only changes the current thread's cwd. That seems like it should be safe,
so I'll go ahead and use that to implement mknodat. I'll also submit a
feature request to apple to implement mknodat.

[Qemu-devel] [PATCH 12/13] 9p: darwin: Provide a fallback implementation for utimensat

[keno]

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

This function is new in Mac OS 10.13. Provide a fallback implementation
when building against older SDKs.

Signed-off-by: Keno Fischer <keno@juliacomputing.com>
---
 hw/9pfs/9p-local.c |  2 +-
 hw/9pfs/9p-util.c  | 38 ++++++++++++++++++++++++++++++++++++++
 hw/9pfs/9p-util.h  |  7 +++++++
 hw/9pfs/9p.c       |  1 +
 4 files changed, 47 insertions(+), 1 deletion(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 3e358b7..70ab541 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1082,7 +1082,7 @@ static int local_utimensat(FsContext *s, V9fsPath *fs_path,
         goto out;
     }
 
-    ret = utimensat(dirfd, name, buf, AT_SYMLINK_NOFOLLOW);
+    ret = utimensat_nofollow(dirfd, name, buf);
     close_preserve_errno(dirfd);
 out:
     g_free(dirpath);
diff --git a/hw/9pfs/9p-util.c b/hw/9pfs/9p-util.c
index 98004ac..8403f5f 100644
--- a/hw/9pfs/9p-util.c
+++ b/hw/9pfs/9p-util.c
@@ -104,3 +104,41 @@ int fsetxattrat_nofollow(int dirfd, const char *filename, const char *name,
 #endif
     return ret;
 }
+
+#ifndef __has_builtin
+#define __has_builtin(x) 0
+#endif
+
+int utimensat_nofollow(int dirfd, const char *filename, const struct timespec times[2])
+{
+#ifdef CONFIG_DARWIN
+#if defined(__MAC_10_13) /* Check whether we have an SDK version that defines utimensat */
+#if __MAC_OS_X_VERSION_MIN_REQUIRED >= __MAC_10_13
+#define UTIMENSAT_AVAILABLE 1
+#elif __has_builtin(__builtin_available)
+#define UTIMENSAT_AVAILABLE __builtin_available(macos 10.13, *)
+#else
+#define UTIMENSAT_AVAILABLE 0
+#endif
+    if (UTIMENSAT_AVAILABLE)
+    {
+        return utimensat(dirfd, filename, times, AT_SYMLINK_NOFOLLOW);
+    }
+#endif
+    // utimensat not available. Use futimes.
+    int fd = openat_file(dirfd, filename, O_PATH_9P_UTIL | O_NOFOLLOW, 0);
+    if (fd == -1)
+        return -1;
+
+    struct timeval futimes_buf[2];
+    futimes_buf[0].tv_sec = times[0].tv_sec;
+    futimes_buf[0].tv_usec = times[0].tv_nsec * 1000;
+    futimes_buf[1].tv_sec = times[1].tv_sec;
+    futimes_buf[1].tv_usec = times[1].tv_nsec * 1000;
+    int ret = futimes(fd, futimes_buf);
+    close_preserve_errno(fd);
+    return ret;
+#else
+    return utimensat(dirfd, filename, times, AT_SYMLINK_NOFOLLOW);
+#endif
+}
diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h
index cb26343..2329c82 100644
--- a/hw/9pfs/9p-util.h
+++ b/hw/9pfs/9p-util.h
@@ -19,6 +19,12 @@
 #define O_PATH_9P_UTIL 0
 #endif
 
+/* Compatibility with OLD SDK Versions for Darwin */
+#if defined(CONFIG_DARWIN) && !defined(UTIME_NOW)
+#define UTIME_NOW -1
+#define UTIME_OMIT -2
+#endif
+
 static inline void close_preserve_errno(int fd)
 {
     int serrno = errno;
@@ -66,5 +72,6 @@ ssize_t flistxattrat_nofollow(int dirfd, const char *filename,
                               char *list, size_t size);
 ssize_t fremovexattrat_nofollow(int dirfd, const char *filename,
                                 const char *name);
+int utimensat_nofollow(int dirfd, const char *filename, const struct timespec times[2]);
 
 #endif
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 4ae4da6..8e0594a 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -21,6 +21,7 @@
 #include "virtio-9p.h"
 #include "fsdev/qemu-fsdev.h"
 #include "9p-xattr.h"
+#include "9p-util.h"
 #include "coth.h"
 #include "trace.h"
 #include "migration/blocker.h"
-- 
2.8.1

Re: [Qemu-devel] [PATCH 12/13] 9p: darwin: Provide a fallback implementation for utimensat

[Greg Kurz]

On Sat, 26 May 2018 01:23:14 -0400
keno@juliacomputing.com wrote:

> From: Keno Fischer <keno@alumni.harvard.edu>
> 
> This function is new in Mac OS 10.13. Provide a fallback implementation
> when building against older SDKs.
> 
> Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> ---

As with patch 10, this should go to 9p-util-darwin.c

>  hw/9pfs/9p-local.c |  2 +-
>  hw/9pfs/9p-util.c  | 38 ++++++++++++++++++++++++++++++++++++++
>  hw/9pfs/9p-util.h  |  7 +++++++
>  hw/9pfs/9p.c       |  1 +
>  4 files changed, 47 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index 3e358b7..70ab541 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -1082,7 +1082,7 @@ static int local_utimensat(FsContext *s, V9fsPath *fs_path,
>          goto out;
>      }
>  
> -    ret = utimensat(dirfd, name, buf, AT_SYMLINK_NOFOLLOW);
> +    ret = utimensat_nofollow(dirfd, name, buf);
>      close_preserve_errno(dirfd);
>  out:
>      g_free(dirpath);
> diff --git a/hw/9pfs/9p-util.c b/hw/9pfs/9p-util.c
> index 98004ac..8403f5f 100644
> --- a/hw/9pfs/9p-util.c
> +++ b/hw/9pfs/9p-util.c
> @@ -104,3 +104,41 @@ int fsetxattrat_nofollow(int dirfd, const char *filename, const char *name,
>  #endif
>      return ret;
>  }
> +
> +#ifndef __has_builtin
> +#define __has_builtin(x) 0
> +#endif
> +
> +int utimensat_nofollow(int dirfd, const char *filename, const struct timespec times[2])
> +{
> +#ifdef CONFIG_DARWIN
> +#if defined(__MAC_10_13) /* Check whether we have an SDK version that defines utimensat */
> +#if __MAC_OS_X_VERSION_MIN_REQUIRED >= __MAC_10_13
> +#define UTIMENSAT_AVAILABLE 1
> +#elif __has_builtin(__builtin_available)
> +#define UTIMENSAT_AVAILABLE __builtin_available(macos 10.13, *)
> +#else
> +#define UTIMENSAT_AVAILABLE 0
> +#endif
> +    if (UTIMENSAT_AVAILABLE)
> +    {
> +        return utimensat(dirfd, filename, times, AT_SYMLINK_NOFOLLOW);
> +    }
> +#endif
> +    // utimensat not available. Use futimes.
> +    int fd = openat_file(dirfd, filename, O_PATH_9P_UTIL | O_NOFOLLOW, 0);
> +    if (fd == -1)
> +        return -1;
> +
> +    struct timeval futimes_buf[2];
> +    futimes_buf[0].tv_sec = times[0].tv_sec;
> +    futimes_buf[0].tv_usec = times[0].tv_nsec * 1000;
> +    futimes_buf[1].tv_sec = times[1].tv_sec;
> +    futimes_buf[1].tv_usec = times[1].tv_nsec * 1000;
> +    int ret = futimes(fd, futimes_buf);
> +    close_preserve_errno(fd);
> +    return ret;
> +#else
> +    return utimensat(dirfd, filename, times, AT_SYMLINK_NOFOLLOW);
> +#endif
> +}
> diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h
> index cb26343..2329c82 100644
> --- a/hw/9pfs/9p-util.h
> +++ b/hw/9pfs/9p-util.h
> @@ -19,6 +19,12 @@
>  #define O_PATH_9P_UTIL 0
>  #endif
>  
> +/* Compatibility with OLD SDK Versions for Darwin */
> +#if defined(CONFIG_DARWIN) && !defined(UTIME_NOW)
> +#define UTIME_NOW -1
> +#define UTIME_OMIT -2
> +#endif
> +
>  static inline void close_preserve_errno(int fd)
>  {
>      int serrno = errno;
> @@ -66,5 +72,6 @@ ssize_t flistxattrat_nofollow(int dirfd, const char *filename,
>                                char *list, size_t size);
>  ssize_t fremovexattrat_nofollow(int dirfd, const char *filename,
>                                  const char *name);
> +int utimensat_nofollow(int dirfd, const char *filename, const struct timespec times[2]);
>  
>  #endif
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 4ae4da6..8e0594a 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -21,6 +21,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"

[Qemu-devel] [PATCH 13/13] 9p: darwin: configure: Allow VirtFS on Darwin

[keno]

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

Signed-off-by: Keno Fischer <keno@juliacomputing.com>
---
 Makefile.objs |  1 +
 configure     | 23 +++++++++++++++--------
 2 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/Makefile.objs b/Makefile.objs
index c6c3554..a2245c9 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -104,6 +104,7 @@ common-obj-$(CONFIG_WIN32) += os-win32.o
 common-obj-$(CONFIG_POSIX) += os-posix.o
 
 common-obj-$(CONFIG_LINUX) += fsdev/
+common-obj-$(CONFIG_DARWIN) += fsdev/
 
 common-obj-y += migration/
 
diff --git a/configure b/configure
index a8498ab..eb7328c 100755
--- a/configure
+++ b/configure
@@ -5535,16 +5535,27 @@ if test "$want_tools" = "yes" ; then
   fi
 fi
 if test "$softmmu" = yes ; then
-  if test "$linux" = yes; then
-    if test "$virtfs" != no && test "$cap" = yes && test "$attr" = yes ; then
+  if test "$virtfs" != no; then
+    if test "$linux" = yes; then
+      if test "$cap" = yes && test "$attr" = yes ; then
+        virtfs=yes
+        tools="$tools fsdev/virtfs-proxy-helper\$(EXESUF)"
+      else
+        if test "$virtfs" = yes; then
+          error_exit "VirtFS requires libcap devel and libattr devel under Linux"
+        fi
+        virtfs=no
+      fi
+    elif test "$darwin" = yes; then
       virtfs=yes
-      tools="$tools fsdev/virtfs-proxy-helper\$(EXESUF)"
     else
       if test "$virtfs" = yes; then
-        error_exit "VirtFS requires libcap devel and libattr devel"
+        error_exit "VirtFS is supported only on Linux and Darwin"
       fi
       virtfs=no
     fi
+  fi
+  if test "$linux" = yes; then
     if test "$mpath" != no && test "$mpathpersist" = yes ; then
       mpath=yes
     else
@@ -5555,10 +5566,6 @@ if test "$softmmu" = yes ; then
     fi
     tools="$tools scsi/qemu-pr-helper\$(EXESUF)"
   else
-    if test "$virtfs" = yes; then
-      error_exit "VirtFS is supported only on Linux"
-    fi
-    virtfs=no
     if test "$mpath" = yes; then
       error_exit "Multipath is supported only on Linux"
     fi
-- 
2.8.1

Re: [Qemu-devel] [PATCH 13/13] 9p: darwin: configure: Allow VirtFS on Darwin

[Greg Kurz]

On Sat, 26 May 2018 01:23:15 -0400
keno@juliacomputing.com wrote:

> From: Keno Fischer <keno@alumni.harvard.edu>
> 
> Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> ---
>  Makefile.objs |  1 +
>  configure     | 23 +++++++++++++++--------
>  2 files changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/Makefile.objs b/Makefile.objs
> index c6c3554..a2245c9 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -104,6 +104,7 @@ common-obj-$(CONFIG_WIN32) += os-win32.o
>  common-obj-$(CONFIG_POSIX) += os-posix.o
>  
>  common-obj-$(CONFIG_LINUX) += fsdev/
> +common-obj-$(CONFIG_DARWIN) += fsdev/
>  
>  common-obj-y += migration/
>  
> diff --git a/configure b/configure
> index a8498ab..eb7328c 100755
> --- a/configure
> +++ b/configure
> @@ -5535,16 +5535,27 @@ if test "$want_tools" = "yes" ; then
>    fi
>  fi
>  if test "$softmmu" = yes ; then
> -  if test "$linux" = yes; then
> -    if test "$virtfs" != no && test "$cap" = yes && test "$attr" = yes ; then
> +  if test "$virtfs" != no; then
> +    if test "$linux" = yes; then
> +      if test "$cap" = yes && test "$attr" = yes ; then
> +        virtfs=yes
> +        tools="$tools fsdev/virtfs-proxy-helper\$(EXESUF)"
> +      else
> +        if test "$virtfs" = yes; then
> +          error_exit "VirtFS requires libcap devel and libattr devel under Linux"
> +        fi
> +        virtfs=no
> +      fi
> +    elif test "$darwin" = yes; then
>        virtfs=yes
> -      tools="$tools fsdev/virtfs-proxy-helper\$(EXESUF)"

So, no proxy helper on Darwin ? Why ? The limitation should be mentioned in
the changelog at least.

>      else
>        if test "$virtfs" = yes; then
> -        error_exit "VirtFS requires libcap devel and libattr devel"
> +        error_exit "VirtFS is supported only on Linux and Darwin"
>        fi
>        virtfs=no
>      fi
> +  fi
> +  if test "$linux" = yes; then
>      if test "$mpath" != no && test "$mpathpersist" = yes ; then
>        mpath=yes
>      else
> @@ -5555,10 +5566,6 @@ if test "$softmmu" = yes ; then
>      fi
>      tools="$tools scsi/qemu-pr-helper\$(EXESUF)"
>    else
> -    if test "$virtfs" = yes; then
> -      error_exit "VirtFS is supported only on Linux"
> -    fi
> -    virtfs=no
>      if test "$mpath" = yes; then
>        error_exit "Multipath is supported only on Linux"
>      fi

Re: [Qemu-devel] [PATCH 13/13] 9p: darwin: configure: Allow VirtFS on Darwin

[Keno Fischer]

>> +    elif test "$darwin" = yes; then
>>        virtfs=yes
>> -      tools="$tools fsdev/virtfs-proxy-helper\$(EXESUF)"
>
> So, no proxy helper on Darwin ? Why ? The limitation should be mentioned in
> the changelog at least.

I just had no use for it. I'll try to build it and see what happens.

Re: [Qemu-devel] [PATCH 13/13] 9p: darwin: configure: Allow VirtFS on Darwin

[Greg Kurz]

On Thu, 31 May 2018 13:46:29 -0400
Keno Fischer <keno@juliacomputing.com> wrote:

> >> +    elif test "$darwin" = yes; then
> >>        virtfs=yes
> >> -      tools="$tools fsdev/virtfs-proxy-helper\$(EXESUF)"  
> >
> > So, no proxy helper on Darwin ? Why ? The limitation should be mentioned in
> > the changelog at least.  
> 
> I just had no use for it. I'll try to build it and see what happens.

Please do.

Re: [Qemu-devel] [PATCH 00/13] 9p: Add support for Darwin

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: cover.1527310210.git.keno@alumni.harvard.edu
Subject: [Qemu-devel] [PATCH 00/13] 9p: Add support for Darwin

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 t [tag update]            patchew/1527266793-301361-1-git-send-email-mst@redhat.com -> patchew/1527266793-301361-1-git-send-email-mst@redhat.com
 * [new tag]               patchew/cover.1527310210.git.keno@alumni.harvard.edu -> patchew/cover.1527310210.git.keno@alumni.harvard.edu
Switched to a new branch 'test'
d5026b2480 9p: darwin: configure: Allow VirtFS on Darwin
e375fe9d7a 9p: darwin: Provide a fallback implementation for utimensat
7e0b2ee759 9p: darwin: Mark mknod as unsupported
e78d656057 9p: darwin: *xattr_nofollow implementations
dff94417c5 9p: darwin: Provide a compatibility definition for XATTR_SIZE_MAX
2630a2f4ea 9p: darwin: Ignore O_{NOATIME, DIRECT}
7277fdd263 9p: darwin: Properly translate AT_REMOVEDIR flag
1ee1435360 9p: darwin: Address minor differences
a03d05d35a 9p: darwin: Handle struct dirent differences
160cefc589 9p: darwin: Handle struct stat(fs) differences
f920617dc0 9p: Move a couple xattr functions to 9p-util
3b4130c0b6 9p: Avoid warning if FS_IOC_GETVERSION is not defined
46cf5c5c4d 9p: linux: Fix a couple Linux assumptions

=== OUTPUT BEGIN ===
Checking PATCH 1/13: 9p: linux: Fix a couple Linux assumptions...
Checking PATCH 2/13: 9p: Avoid warning if FS_IOC_GETVERSION is not defined...
Checking PATCH 3/13: 9p: Move a couple xattr functions to 9p-util...
WARNING: line over 80 characters
#26: FILE: hw/9pfs/9p-local.c:779:
+        if (fgetxattr_follow(fd, "user.virtfs.uid", &tmp_uid, sizeof(uid_t)) > 0) {

WARNING: line over 80 characters
#30: FILE: hw/9pfs/9p-local.c:782:
+        if (fgetxattr_follow(fd, "user.virtfs.gid", &tmp_gid, sizeof(gid_t)) > 0)

WARNING: line over 80 characters
#35: FILE: hw/9pfs/9p-local.c:786:
+        if (fgetxattr_follow(fd, "user.virtfs.mode", &tmp_mode, sizeof(mode_t)) > 0)

WARNING: line over 80 characters
#40: FILE: hw/9pfs/9p-local.c:790:
+        if (fgetxattr_follow(fd, "user.virtfs.rdev", &tmp_dev, sizeof(dev_t)) > 0)

total: 0 errors, 4 warnings, 129 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 4/13: 9p: darwin: Handle struct stat(fs) differences...
Checking PATCH 5/13: 9p: darwin: Handle struct dirent differences...
ERROR: do not use C99 // comments
#46: FILE: hw/9pfs/9p.c:1960:
+        // Darwin has d_seekoff, which appears to function

ERROR: do not use C99 // comments
#47: FILE: hw/9pfs/9p.c:1961:
+        // analogously to d_off. However, it does not appear

ERROR: do not use C99 // comments
#48: FILE: hw/9pfs/9p.c:1962:
+        // to be supported on all file systems, so use

ERROR: do not use C99 // comments
#49: FILE: hw/9pfs/9p.c:1963:
+        // telldir for correctness.

total: 4 errors, 0 warnings, 50 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 6/13: 9p: darwin: Address minor differences...
Checking PATCH 7/13: 9p: darwin: Properly translate AT_REMOVEDIR flag...
Checking PATCH 8/13: 9p: darwin: Ignore O_{NOATIME, DIRECT}...
Checking PATCH 9/13: 9p: darwin: Provide a compatibility definition for XATTR_SIZE_MAX...
Checking PATCH 10/13: 9p: darwin: *xattr_nofollow implementations...
WARNING: line over 80 characters
#20: FILE: hw/9pfs/9p-util.c:22:
+    int fd = openat_file(dirfd, filename, O_RDONLY | O_PATH_9P_UTIL | O_NOFOLLOW, 0);

ERROR: braces {} are necessary for all arms of this statement
#21: FILE: hw/9pfs/9p-util.c:23:
+    if (fd == -1)
[...]

WARNING: line over 80 characters
#51: FILE: hw/9pfs/9p-util.c:52:
+    int fd = openat_file(dirfd, filename, O_RDONLY | O_PATH_9P_UTIL | O_NOFOLLOW, 0);

ERROR: braces {} are necessary for all arms of this statement
#52: FILE: hw/9pfs/9p-util.c:53:
+    if (fd == -1)
[...]

ERROR: braces {} are necessary for all arms of this statement
#73: FILE: hw/9pfs/9p-util.c:73:
+    if (fd == -1)
[...]

ERROR: braces {} are necessary for all arms of this statement
#95: FILE: hw/9pfs/9p-util.c:94:
+    if (fd == -1)
[...]

total: 4 errors, 2 warnings, 94 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 11/13: 9p: darwin: Mark mknod as unsupported...
Checking PATCH 12/13: 9p: darwin: Provide a fallback implementation for utimensat...
WARNING: architecture specific defines should be avoided
#34: FILE: hw/9pfs/9p-util.c:108:
+#ifndef __has_builtin

WARNING: line over 80 characters
#38: FILE: hw/9pfs/9p-util.c:112:
+int utimensat_nofollow(int dirfd, const char *filename, const struct timespec times[2])

WARNING: line over 80 characters
#41: FILE: hw/9pfs/9p-util.c:115:
+#if defined(__MAC_10_13) /* Check whether we have an SDK version that defines utimensat */

WARNING: architecture specific defines should be avoided
#41: FILE: hw/9pfs/9p-util.c:115:
+#if defined(__MAC_10_13) /* Check whether we have an SDK version that defines utimensat */

WARNING: architecture specific defines should be avoided
#42: FILE: hw/9pfs/9p-util.c:116:
+#if __MAC_OS_X_VERSION_MIN_REQUIRED >= __MAC_10_13

ERROR: that open brace { should be on the previous line
#49: FILE: hw/9pfs/9p-util.c:123:
+    if (UTIMENSAT_AVAILABLE)
+    {

ERROR: do not use C99 // comments
#54: FILE: hw/9pfs/9p-util.c:128:
+    // utimensat not available. Use futimes.

ERROR: braces {} are necessary for all arms of this statement
#56: FILE: hw/9pfs/9p-util.c:130:
+    if (fd == -1)
[...]

WARNING: line over 80 characters
#92: FILE: hw/9pfs/9p-util.h:75:
+int utimensat_nofollow(int dirfd, const char *filename, const struct timespec times[2]);

total: 3 errors, 6 warnings, 74 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 13/13: 9p: darwin: configure: Allow VirtFS on Darwin...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com