Re: [Qemu-devel] [PATCH 07/19] aspeed: add support for multiple NICs

[Keno Fischer]

Drive by comment, since I spotted this in my inbox.
When I tried to make this change (two years ago though),
I additionally needed the following. Unfortunately, I don't quite remember
exactly what the issue was, but I think qemu would crash trying to create more
than one nic.

---
 hw/net/ftgmac100.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
index 8127d0532dc..1318de85a4e 100644
--- a/hw/net/ftgmac100.c
+++ b/hw/net/ftgmac100.c
@@ -977,7 +977,8 @@ static void ftgmac100_realize(DeviceState *dev,
Error **errp)
 sysbus_init_irq(sbd, >irq);
 qemu_macaddr_default_if_unset(>conf.macaddr);

-s->conf.peers.ncs[0] = nd_table[0].netdev;
+char *netdev_id = object_property_get_str(OBJECT(dev), "netdev", NULL);
+s->conf.peers.ncs[0] = qemu_find_netdev(netdev_id);

 s->nic = qemu_new_nic(_ftgmac100_info, >conf,
   object_get_typename(OBJECT(dev)), DEVICE(dev)->id,



On Sat, May 25, 2019 at 11:22 AM Cédric Le Goater  wrote:
>
> The Aspeed SoCs have two MACs. Extend the Aspeed model to support a
> second NIC.
>
> Signed-off-by: Cédric Le Goater 
> ---
>  include/hw/arm/aspeed_soc.h |  3 ++-
>  hw/arm/aspeed_soc.c | 33 +++--
>  2 files changed, 21 insertions(+), 15 deletions(-)
>
> diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
> index 7247f6da2505..e8556abf4737 100644
> --- a/include/hw/arm/aspeed_soc.h
> +++ b/include/hw/arm/aspeed_soc.h
> @@ -25,6 +25,7 @@
>  #define ASPEED_SPIS_NUM  2
>  #define ASPEED_WDTS_NUM  3
>  #define ASPEED_CPUS_NUM  2
> +#define ASPEED_MACS_NUM  2
>
>  typedef struct AspeedSoCState {
>  /*< private >*/
> @@ -42,7 +43,7 @@ typedef struct AspeedSoCState {
>  AspeedSMCState spi[ASPEED_SPIS_NUM];
>  AspeedSDMCState sdmc;
>  AspeedWDTState wdt[ASPEED_WDTS_NUM];
> -FTGMAC100State ftgmac100;
> +FTGMAC100State ftgmac100[ASPEED_MACS_NUM];
>  } AspeedSoCState;
>
>  #define TYPE_ASPEED_SOC "aspeed-soc"
> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
> index b983d5efc5d1..8cfe9e9515ed 100644
> --- a/hw/arm/aspeed_soc.c
> +++ b/hw/arm/aspeed_soc.c
> @@ -229,8 +229,10 @@ static void aspeed_soc_init(Object *obj)
>  sc->info->silicon_rev);
>  }
>
> -sysbus_init_child_obj(obj, "ftgmac100", OBJECT(>ftgmac100),
> -  sizeof(s->ftgmac100), TYPE_FTGMAC100);
> +for (i = 0; i < ASPEED_MACS_NUM; i++) {
> +sysbus_init_child_obj(obj, "ftgmac100[*]", OBJECT(>ftgmac100[i]),
> +  sizeof(s->ftgmac100[i]), TYPE_FTGMAC100);
> +}
>  }
>
>  static void aspeed_soc_realize(DeviceState *dev, Error **errp)
> @@ -371,19 +373,22 @@ static void aspeed_soc_realize(DeviceState *dev, Error 
> **errp)
>  }
>
>  /* Net */
> -qdev_set_nic_properties(DEVICE(>ftgmac100), _table[0]);
> -object_property_set_bool(OBJECT(>ftgmac100), true, "aspeed", );
> -object_property_set_bool(OBJECT(>ftgmac100), true, "realized",
> - _err);
> -error_propagate(, local_err);
> -if (err) {
> -error_propagate(errp, err);
> -return;
> +for (i = 0; i < nb_nics; i++) {
> +qdev_set_nic_properties(DEVICE(>ftgmac100[i]), _table[i]);
> +object_property_set_bool(OBJECT(>ftgmac100[i]), true, "aspeed",
> + );
> +object_property_set_bool(OBJECT(>ftgmac100[i]), true, "realized",
> + _err);
> +error_propagate(, local_err);
> +if (err) {
> +error_propagate(errp, err);
> +   return;
> +}
> +sysbus_mmio_map(SYS_BUS_DEVICE(>ftgmac100[i]), 0,
> +sc->info->memmap[ASPEED_ETH1 + i]);
> +sysbus_connect_irq(SYS_BUS_DEVICE(>ftgmac100[i]), 0,
> +   aspeed_soc_get_irq(s, ASPEED_ETH1 + i));
>  }
> -sysbus_mmio_map(SYS_BUS_DEVICE(>ftgmac100), 0,
> -sc->info->memmap[ASPEED_ETH1]);
> -sysbus_connect_irq(SYS_BUS_DEVICE(>ftgmac100), 0,
> -   aspeed_soc_get_irq(s, ASPEED_ETH1));
>  }
>
>  static void aspeed_soc_class_init(ObjectClass *oc, void *data)
> --
> 2.20.1
>
>

[Qemu-devel] [PATCH v3 11/13] 9p: darwin: Implement compatibility for mknodat

[Keno Fischer]

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

Signed-off-by: Keno Fischer 
---

Changes since v2:
 - Silence clang warning for deprecated uses of `syscall`. It is
  unforunate that we have to use this depreacted interface, but
  there does not seem to be a better option.

 hw/9pfs/9p-local.c   |  5 +++--
 hw/9pfs/9p-util-darwin.c | 31 +++
 hw/9pfs/9p-util-linux.c  |  5 +
 hw/9pfs/9p-util.h|  2 ++
 4 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 56bcabf..450f31c 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -668,7 +668,7 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath 
*dir_path,
 
 if (fs_ctx->export_flags & V9FS_SM_MAPPED ||
 fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
-err = mknodat(dirfd, name, fs_ctx->fmode | S_IFREG, 0);
+err = qemu_mknodat(dirfd, name, fs_ctx->fmode | S_IFREG, 0);
 if (err == -1) {
 goto out;
 }
@@ -683,7 +683,7 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath 
*dir_path,
 }
 } else if (fs_ctx->export_flags & V9FS_SM_PASSTHROUGH ||
fs_ctx->export_flags & V9FS_SM_NONE) {
-err = mknodat(dirfd, name, credp->fc_mode, credp->fc_rdev);
+err = qemu_mknodat(dirfd, name, credp->fc_mode, credp->fc_rdev);
 if (err == -1) {
 goto out;
 }
@@ -696,6 +696,7 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath 
*dir_path,
 
 err_end:
 unlinkat_preserve_errno(dirfd, name, 0);
+
 out:
 close_preserve_errno(dirfd);
 return err;
diff --git a/hw/9pfs/9p-util-darwin.c b/hw/9pfs/9p-util-darwin.c
index ac414bc..194f068 100644
--- a/hw/9pfs/9p-util-darwin.c
+++ b/hw/9pfs/9p-util-darwin.c
@@ -158,3 +158,34 @@ done:
 close_preserve_errno(fd);
 return ret;
 }
+
+#ifndef SYS___pthread_fchdir
+# define SYS___pthread_fchdir 349
+#endif
+
+// This is an undocumented OS X syscall. It would be best to avoid it,
+// but there doesn't seem to be another safe way to implement mknodat.
+// Dear Apple, please implement mknodat before you remove this syscall.
+static int fchdir_thread_local(int fd)
+{
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wdeprecated-declarations"
+return syscall(SYS___pthread_fchdir, fd);
+#pragma clang diagnostic pop
+}
+
+int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev)
+{
+int preserved_errno, err;
+if (fchdir_thread_local(dirfd) < 0) {
+return -1;
+}
+err = mknod(filename, mode, dev);
+preserved_errno = errno;
+/* Stop using the thread-local cwd */
+fchdir_thread_local(-1);
+if (err < 0) {
+errno = preserved_errno;
+}
+return err;
+}
diff --git a/hw/9pfs/9p-util-linux.c b/hw/9pfs/9p-util-linux.c
index 3902378..06399c5 100644
--- a/hw/9pfs/9p-util-linux.c
+++ b/hw/9pfs/9p-util-linux.c
@@ -63,3 +63,8 @@ int utimensat_nofollow(int dirfd, const char *filename,
 {
 return utimensat(dirfd, filename, times, AT_SYMLINK_NOFOLLOW);
 }
+
+int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev)
+{
+return mknodat(dirfd, filename, mode, dev);
+}
diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h
index b1dc08a..127564d 100644
--- a/hw/9pfs/9p-util.h
+++ b/hw/9pfs/9p-util.h
@@ -90,4 +90,6 @@ ssize_t fremovexattrat_nofollow(int dirfd, const char 
*filename,
 int utimensat_nofollow(int dirfd, const char *filename,
const struct timespec times[2]);
 
+int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev);
+
 #endif
-- 
2.8.1

[Qemu-devel] [PATCH v3 12/13] 9p: darwin: virtfs-proxy: Implement setuid code for darwin

[Keno Fischer]

Darwin does not have linux capabilities, so make that code linux-only.
Darwin also does not have setresuid/gid. The correct way to temporarily
drop capabilities is to call seteuid/gid.

Also factor out the code that acquires acquire_dac_override into a separate
function in the linux implementation. I had originally done this when
I thought it made sense to have only one `setugid` function, but I retained
this because it seems clearer this way.

Signed-off-by: Keno Fischer 
---
 fsdev/virtfs-proxy-helper.c | 200 +++-
 1 file changed, 125 insertions(+), 75 deletions(-)

diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
index d8dd3f5..6baf2a6 100644
--- a/fsdev/virtfs-proxy-helper.c
+++ b/fsdev/virtfs-proxy-helper.c
@@ -82,6 +82,7 @@ static void do_perror(const char *string)
 }
 }
 
+#ifdef CONFIG_LINUX
 static int do_cap_set(cap_value_t *cap_value, int size, int reset)
 {
 cap_t caps;
@@ -121,6 +122,85 @@ error:
 return -1;
 }
 
+static int acquire_dac_override(void)
+{
+cap_value_t cap_list[] = {
+CAP_DAC_OVERRIDE,
+};
+return do_cap_set(cap_list, ARRAY_SIZE(cap_list), 0);
+}
+
+/*
+ * from man 7 capabilities, section
+ * Effect of User ID Changes on Capabilities:
+ * If the effective user ID is changed from nonzero to 0, then the permitted
+ * set is copied to the effective set.  If the effective user ID is changed
+ * from 0 to nonzero, then all capabilities are are cleared from the effective
+ * set.
+ *
+ * The setfsuid/setfsgid man pages warn that changing the effective user ID may
+ * expose the program to unwanted signals, but this is not true anymore: for an
+ * unprivileged (without CAP_KILL) program to send a signal, the real or
+ * effective user ID of the sending process must equal the real or saved user
+ * ID of the target process.  Even when dropping privileges, it is enough to
+ * keep the saved UID to a "privileged" value and virtfs-proxy-helper won't
+ * be exposed to signals.  So just use setresuid/setresgid.
+ */
+static int setugid(int uid, int gid, int *suid, int *sgid)
+{
+int retval;
+
+*suid = geteuid();
+*sgid = getegid();
+
+if (setresgid(-1, gid, *sgid) == -1) {
+retval = -errno;
+goto err_out;
+}
+
+if (setresuid(-1, uid, *suid) == -1) {
+retval = -errno;
+goto err_sgid;
+}
+
+if (uid != 0 || gid != 0) {
+/*
+* We still need DAC_OVERRIDE because we don't change
+* supplementary group ids, and hence may be subjected DAC rules
+*/
+if (acquire_dac_override() < 0) {
+retval = -errno;
+goto err_suid;
+}
+}
+return 0;
+
+err_suid:
+if (setresuid(-1, *suid, *suid) == -1) {
+abort();
+}
+err_sgid:
+if (setresgid(-1, *sgid, *sgid) == -1) {
+abort();
+}
+err_out:
+return retval;
+}
+
+/*
+ * This is used to reset the ugid back with the saved values
+ * There is nothing much we can do checking error values here.
+ */
+static void resetugid(int suid, int sgid)
+{
+if (setresgid(-1, sgid, sgid) == -1) {
+abort();
+}
+if (setresuid(-1, suid, suid) == -1) {
+abort();
+}
+}
+
 static int init_capabilities(void)
 {
 /* helper needs following capabilities only */
@@ -135,6 +215,51 @@ static int init_capabilities(void)
 };
 return do_cap_set(cap_list, ARRAY_SIZE(cap_list), 1);
 }
+#else
+static int setugid(int uid, int gid, int *suid, int *sgid)
+{
+int retval;
+
+*suid = geteuid();
+*sgid = getegid();
+
+if (setegid(gid) == -1) {
+retval = -errno;
+goto err_out;
+}
+
+if (seteuid(uid) == -1) {
+retval = -errno;
+goto err_sgid;
+}
+
+err_sgid:
+if (setgid(*sgid) == -1) {
+abort();
+}
+err_out:
+return retval;
+}
+
+/*
+ * This is used to reset the ugid back with the saved values
+ * There is nothing much we can do checking error values here.
+ */
+static void resetugid(int suid, int sgid)
+{
+if (setegid(sgid) == -1) {
+abort();
+}
+if (seteuid(suid) == -1) {
+abort();
+}
+}
+
+static int init_capabilities(void)
+{
+return 0;
+}
+#endif
 
 static int socket_read(int sockfd, void *buff, ssize_t size)
 {
@@ -279,81 +404,6 @@ static int send_status(int sockfd, struct iovec *iovec, 
int status)
 }
 
 /*
- * from man 7 capabilities, section
- * Effect of User ID Changes on Capabilities:
- * If the effective user ID is changed from nonzero to 0, then the permitted
- * set is copied to the effective set.  If the effective user ID is changed
- * from 0 to nonzero, then all capabilities are are cleared from the effective
- * set.
- *
- * The setfsuid/setfsgid man pages warn that changing the effective user ID may
- * expose the program to unwanted signals, but this is not true anymore: for an
- * unprivileged (without CAP_KILL) program to send a signal, the real or

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

[Keno Fischer]

Signed-off-by: Keno Fischer 
---
 Makefile.objs |  1 +
 configure | 22 +++---
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/Makefile.objs b/Makefile.objs
index 7a9828d..c968a9a 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 195c9bd..74f593a 100755
--- a/configure
+++ b/configure
@@ -5568,16 +5568,28 @@ 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
@@ -5588,10 +5600,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

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

[Keno Fischer]

On darwin d_seekoff exists, but is optional and does not seem to
be commonly used by file systems. Use `telldir` instead to obtain
the seek offset.

Signed-off-by: Keno Fischer 
---
 hw/9pfs/9p-synth.c |  2 ++
 hw/9pfs/9p.c   | 36 
 2 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
index eb68b42..a312f8c 100644
--- a/hw/9pfs/9p-synth.c
+++ b/hw/9pfs/9p-synth.c
@@ -221,7 +221,9 @@ static void synth_direntry(V9fsSynthNode *node,
 {
 strcpy(entry->d_name, node->name);
 entry->d_ino = node->attr->inode;
+#ifndef CONFIG_DARWIN
 entry->d_off = off + 1;
+#endif
 }
 
 static struct dirent *synth_get_dentry(V9fsSynthNode *dir,
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 8e6b908..06139c9 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1738,6 +1738,25 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, 
V9fsFidState *fidp,
 return offset;
 }
 
+/**
+ * Get the seek offset of a dirent. If not available from the structure itself,
+ * obtain it by calling telldir.
+ */
+static int v9fs_dent_telldir(V9fsPDU *pdu, V9fsFidState *fidp,
+ struct dirent *dent)
+{
+#ifdef CONFIG_DARWIN
+/*
+ * Darwin has d_seekoff, which appears to function similarly to d_off.
+ * However, it does not appear to be supported on all file systems,
+ * so use telldir for correctness.
+ */
+return v9fs_co_telldir(pdu, fidp);
+#else
+return dent->d_off;
+#endif
+}
+
 static int coroutine_fn v9fs_do_readdir_with_stat(V9fsPDU *pdu,
   V9fsFidState *fidp,
   uint32_t max_count)
@@ -1801,7 +1820,11 @@ static int coroutine_fn 
v9fs_do_readdir_with_stat(V9fsPDU *pdu,
 count += len;
 v9fs_stat_free();
 v9fs_path_free();
-saved_dir_pos = dent->d_off;
+saved_dir_pos = v9fs_dent_telldir(pdu, fidp, dent);
+if (saved_dir_pos < 0) {
+err = saved_dir_pos;
+break;
+}
 }
 
 v9fs_readdir_unlock(>fs.dir);
@@ -1915,7 +1938,7 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu, 
V9fsFidState *fidp,
 V9fsString name;
 int len, err = 0;
 int32_t count = 0;
-off_t saved_dir_pos;
+off_t saved_dir_pos, off;
 struct dirent *dent;
 
 /* save the directory position */
@@ -1951,10 +1974,15 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu, 
V9fsFidState *fidp,
 /* Fill the other fields with dummy values */
 qid.type = 0;
 qid.version = 0;
+off = v9fs_dent_telldir(pdu, fidp, dent);
+if (off < 0) {
+err = off;
+break;
+}
 
 /* 11 = 7 + 4 (7 = start offset, 4 = space for storing count) */
 len = pdu_marshal(pdu, 11 + count, "Qqbs",
-  , dent->d_off,
+  , off,
   dent->d_type, );
 
 v9fs_readdir_unlock(>fs.dir);
@@ -1966,7 +1994,7 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu, 
V9fsFidState *fidp,
 }
 count += len;
 v9fs_string_free();
-saved_dir_pos = dent->d_off;
+saved_dir_pos = off;
 }
 
 v9fs_readdir_unlock(>fs.dir);
-- 
2.8.1

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

[Keno Fischer]

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

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

Signed-off-by: Keno Fischer 
---
 fsdev/virtfs-proxy-helper.c |  3 +-
 hw/9pfs/9p-local.c  |  2 +-
 hw/9pfs/9p-util-darwin.c| 96 +
 hw/9pfs/9p-util-linux.c |  6 +++
 hw/9pfs/9p-util.h   |  8 
 hw/9pfs/9p.c|  1 +
 6 files changed, 113 insertions(+), 3 deletions(-)

diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
index a26f8b8..d8dd3f5 100644
--- a/fsdev/virtfs-proxy-helper.c
+++ b/fsdev/virtfs-proxy-helper.c
@@ -957,8 +957,7 @@ static int process_requests(int sock)
  [0].tv_sec, [0].tv_nsec,
  [1].tv_sec, [1].tv_nsec);
 if (retval > 0) {
-retval = utimensat(AT_FDCWD, path.data, spec,
-   AT_SYMLINK_NOFOLLOW);
+retval = utimensat_nofollow(AT_FDCWD, path.data, spec);
 if (retval < 0) {
 retval = -errno;
 }
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 768ef6f..56bcabf 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1071,7 +1071,7 @@ static int local_utimensat(FsContext *s, V9fsPath 
*fs_path,
 goto out;
 }
 
-ret = utimensat(dirfd, name, buf, AT_SYMLINK_NOFOLLOW);
+ret = utimensat_nofollow(dirfd, name, buf);
 close_preserve_errno(dirfd);
 out:
 g_free(dirpath);
diff --git a/hw/9pfs/9p-util-darwin.c b/hw/9pfs/9p-util-darwin.c
index cdb4c9e..ac414bc 100644
--- a/hw/9pfs/9p-util-darwin.c
+++ b/hw/9pfs/9p-util-darwin.c
@@ -62,3 +62,99 @@ int fsetxattrat_nofollow(int dirfd, const char *filename, 
const char *name,
 close_preserve_errno(fd);
 return ret;
 }
+
+#ifndef __has_builtin
+#define __has_builtin(x) 0
+#endif
+
+static int update_times_from_stat(int fd, struct timespec times[2],
+  int update0, int update1)
+{
+struct stat buf;
+int ret = fstat(fd, );
+if (ret == -1) {
+return ret;
+}
+if (update0) {
+times[0] = buf.st_atimespec;
+}
+if (update1) {
+times[1] = buf.st_mtimespec;
+}
+return 0;
+}
+
+int utimensat_nofollow(int dirfd, const char *filename,
+   const struct timespec times_in[2])
+{
+int ret, fd;
+int special0, special1;
+struct timeval futimes_buf[2];
+struct timespec times[2];
+memcpy(times, times_in, 2 * sizeof(struct timespec));
+
+/* Check whether we have an SDK version that defines utimensat */
+#if defined(__MAC_10_13)
+# if __MAC_OS_X_VERSION_MIN_REQUIRED >= __MAC_10_13
+#  define UTIMENSAT_AVAILABLE 1
+# elif __has_builtin(__builtin_available)
+#  define UTIMENSAT_AVAILABLE __builtin_available(macos 10.13, *)
+# else
+#  define UTIMENSAT_AVAILABLE 0
+# endif
+if (UTIMENSAT_AVAILABLE) {
+return utimensat(dirfd, filename, times, AT_SYMLINK_NOFOLLOW);
+}
+#endif
+
+/* utimensat not available. Use futimes. */
+fd = openat_file(dirfd, filename, O_PATH_9P_UTIL | O_NOFOLLOW, 0);
+if (fd == -1) {
+return -1;
+}
+
+special0 = times[0].tv_nsec == UTIME_OMIT;
+special1 = times[1].tv_nsec == UTIME_OMIT;
+if (special0 || special1) {
+/* If both are set, nothing to do */
+if (special0 && special1) {
+ret = 0;
+goto done;
+}
+
+ret = update_times_from_stat(fd, times, special0, special1);
+if (ret < 0) {
+goto done;
+}
+}
+
+special0 = times[0].tv_nsec == UTIME_NOW;
+special1 = times[1].tv_nsec == UTIME_NOW;
+if (special0 || special1) {
+ret = futimes(fd, NULL);
+if (ret < 0) {
+goto done;
+}
+
+/* If both are set, we are done */
+if (special0 && special1) {
+ret = 0;
+goto done;
+}
+
+ret = update_times_from_stat(fd, times, special0, special1);
+if (ret < 0) {
+goto done;
+ 

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

[Keno Fischer]

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

Signed-off-by: Keno Fischer 
---
 hw/9pfs/9p-util-darwin.c | 64 
 hw/9pfs/Makefile.objs|  1 +
 2 files changed, 65 insertions(+)
 create mode 100644 hw/9pfs/9p-util-darwin.c

diff --git a/hw/9pfs/9p-util-darwin.c b/hw/9pfs/9p-util-darwin.c
new file mode 100644
index 000..cdb4c9e
--- /dev/null
+++ b/hw/9pfs/9p-util-darwin.c
@@ -0,0 +1,64 @@
+/*
+ * 9p utilities (Darwin Implementation)
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/xattr.h"
+#include "9p-util.h"
+
+ssize_t fgetxattrat_nofollow(int dirfd, const char *filename, const char *name,
+ void *value, size_t size)
+{
+int ret;
+int fd = openat_file(dirfd, filename,
+ O_RDONLY | O_PATH_9P_UTIL | O_NOFOLLOW, 0);
+if (fd == -1) {
+return -1;
+}
+ret = fgetxattr(fd, name, value, size, 0, 0);
+close_preserve_errno(fd);
+return ret;
+}
+
+ssize_t flistxattrat_nofollow(int dirfd, const char *filename,
+  char *list, size_t size)
+{
+int ret;
+int fd = openat_file(dirfd, filename,
+ O_RDONLY | O_PATH_9P_UTIL | O_NOFOLLOW, 0);
+if (fd == -1) {
+return -1;
+}
+ret = flistxattr(fd, list, size, 0);
+close_preserve_errno(fd);
+return ret;
+}
+
+ssize_t fremovexattrat_nofollow(int dirfd, const char *filename,
+const char *name)
+{
+int ret;
+int fd = openat_file(dirfd, filename, O_PATH_9P_UTIL | O_NOFOLLOW, 0);
+if (fd == -1) {
+return -1;
+}
+ret = fremovexattr(fd, name, 0);
+close_preserve_errno(fd);
+return ret;
+}
+
+int fsetxattrat_nofollow(int dirfd, const char *filename, const char *name,
+ void *value, size_t size, int flags)
+{
+int ret;
+int fd = openat_file(dirfd, filename, O_PATH_9P_UTIL | O_NOFOLLOW, 0);
+if (fd == -1) {
+return -1;
+}
+ret = fsetxattr(fd, name, value, size, 0, flags);
+close_preserve_errno(fd);
+return ret;
+}
diff --git a/hw/9pfs/Makefile.objs b/hw/9pfs/Makefile.objs
index 95e3bc0..0de39af 100644
--- a/hw/9pfs/Makefile.objs
+++ b/hw/9pfs/Makefile.objs
@@ -1,6 +1,7 @@
 ifeq ($(call lor,$(CONFIG_VIRTIO_9P),$(CONFIG_XEN)),y)
 common-obj-y  = 9p.o
 common-obj-$(CONFIG_LINUX) += 9p-util-linux.o
+common-obj-$(CONFIG_DARWIN) += 9p-util-darwin.o
 common-obj-y += 9p-local.o 9p-xattr.o
 common-obj-y += 9p-xattr-user.o 9p-posix-acl.o
 common-obj-y += coth.o cofs.o codir.o cofile.o
-- 
2.8.1

[Qemu-devel] [PATCH v3 09/13] 9p: darwin: Compatibility for f/l*xattr

[Keno Fischer]

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

Signed-off-by: Keno Fischer 
---
 Makefile|  6 ++
 fsdev/virtfs-proxy-helper.c |  9 +
 hw/9pfs/9p-local.c  | 12 
 hw/9pfs/9p-util.h   | 17 +
 4 files changed, 36 insertions(+), 8 deletions(-)

diff --git a/Makefile b/Makefile
index e46f2b6..046e553 100644
--- a/Makefile
+++ b/Makefile
@@ -545,7 +545,13 @@ qemu-bridge-helper$(EXESUF): qemu-bridge-helper.o 
$(COMMON_LDADDS)
 qemu-keymap$(EXESUF): qemu-keymap.o ui/input-keymap.o $(COMMON_LDADDS)
 
 fsdev/virtfs-proxy-helper$(EXESUF): fsdev/virtfs-proxy-helper.o 
fsdev/9p-marshal.o fsdev/9p-iov-marshal.o $(COMMON_LDADDS)
+ifdef CONFIG_DARWIN
+fsdev/virtfs-proxy-helper$(EXESUF): hw/9pfs/9p-util-darwin.o
+endif
+ifdef CONFIG_LINUX
+fsdev/virtfs-proxy-helper$(EXESUF): hw/9pfs/9p-util-linux.o
 fsdev/virtfs-proxy-helper$(EXESUF): LIBS += -lcap
+endif
 
 scsi/qemu-pr-helper$(EXESUF): scsi/qemu-pr-helper.o scsi/utils.o 
$(crypto-obj-y) $(io-obj-y) $(qom-obj-y) $(COMMON_LDADDS)
 ifdef CONFIG_MPATH
diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
index 3bc1269..a26f8b8 100644
--- a/fsdev/virtfs-proxy-helper.c
+++ b/fsdev/virtfs-proxy-helper.c
@@ -28,6 +28,7 @@
 #include "qemu/statfs.h"
 #include "9p-iov-marshal.h"
 #include "hw/9pfs/9p-proxy.h"
+#include "hw/9pfs/9p-util.h"
 #include "fsdev/9p-iov-marshal.h"
 
 #define PROGNAME "virtfs-proxy-helper"
@@ -459,7 +460,7 @@ static int do_getxattr(int type, struct iovec *iovec, 
struct iovec *out_iovec)
 v9fs_string_init();
 retval = proxy_unmarshal(iovec, offset, "s", );
 if (retval > 0) {
-retval = lgetxattr(path.data, name.data, xattr.data, size);
+retval = qemu_lgetxattr(path.data, name.data, xattr.data, size);
 if (retval < 0) {
 retval = -errno;
 } else {
@@ -469,7 +470,7 @@ static int do_getxattr(int type, struct iovec *iovec, 
struct iovec *out_iovec)
 v9fs_string_free();
 break;
 case T_LLISTXATTR:
-retval = llistxattr(path.data, xattr.data, size);
+retval = qemu_llistxattr(path.data, xattr.data, size);
 if (retval < 0) {
 retval = -errno;
 } else {
@@ -1000,7 +1001,7 @@ static int process_requests(int sock)
 retval = proxy_unmarshal(_iovec, PROXY_HDR_SZ, "sssdd", ,
  , , , );
 if (retval > 0) {
-retval = lsetxattr(path.data,
+retval = qemu_lsetxattr(path.data,
name.data, value.data, size, flags);
 if (retval < 0) {
 retval = -errno;
@@ -1016,7 +1017,7 @@ static int process_requests(int sock)
 retval = proxy_unmarshal(_iovec,
  PROXY_HDR_SZ, "ss", , );
 if (retval > 0) {
-retval = lremovexattr(path.data, name.data);
+retval = qemu_lremovexattr(path.data, name.data);
 if (retval < 0) {
 retval = -errno;
 }
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 98d4073..768ef6f 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -776,16 +776,20 @@ static int local_fstat(FsContext *fs_ctx, int fid_type,
 mode_t tmp_mode;
 dev_t tmp_dev;
 
-if (fgetxattr(fd, "user.virtfs.uid", _uid, sizeof(uid_t)) > 0) {
+if (qemu_fgetxattr(fd, "user.virtfs.uid",
+   _uid, sizeof(uid_t)) > 0) {
 stbuf->st_uid = le32_to_cpu(tmp_uid);
 }
-if (fgetxattr(fd, "user.virtfs.gid", _gid, sizeof(gid_t)) > 0) {
+if (qemu_fgetxattr(fd, "user.virtfs.gid",
+   _gid, sizeof(gid_t)) > 0) {
 stbuf->st_gid = le32_to_cpu(tmp_gid);
 }
-if (fgetxattr(fd, "user.virtfs.mode", _mode, sizeof(mode_t)) > 0) {
+if (qemu_fgetxattr(fd, "user.virtfs.mode",
+   _mode, sizeof(mode_t)) > 0) {
 stbuf->st_mode = le32_to_cpu(tmp_mode);
 }
-if (fgetxattr(fd, "user.virtfs.rdev", _dev, sizeof(dev_t)) > 0) {
+if (qemu_fgetxattr(fd, "user.virtfs.rdev",
+   _dev, sizeof(dev_t)) > 0) {
 stbuf->st_rdev = le64_to_cpu(tmp_dev);
 }
 } else if (fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h
index 79ed6b2..50a03c7 100644
--- a/hw/9pfs/9p-util.h
+++ b/hw/9pfs/9p-util.h
@@ -19,6 +19,23 @@
 #define O_PATH_9P_UTIL 0
 #endif
 
+#ifdef CON

[Qemu-devel] [PATCH v3 02/13] 9p: Rename 9p-util -> 9p-util-linux

[Keno Fischer]

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

Signed-off-by: Keno Fischer 
---
 hw/9pfs/9p-util-linux.c | 59 +
 hw/9pfs/9p-util.c   | 59 -
 hw/9pfs/Makefile.objs   |  3 ++-
 3 files changed, 61 insertions(+), 60 deletions(-)
 create mode 100644 hw/9pfs/9p-util-linux.c
 delete mode 100644 hw/9pfs/9p-util.c

diff --git a/hw/9pfs/9p-util-linux.c b/hw/9pfs/9p-util-linux.c
new file mode 100644
index 000..defa3a4
--- /dev/null
+++ b/hw/9pfs/9p-util-linux.c
@@ -0,0 +1,59 @@
+/*
+ * 9p utilities (Linux Implementation)
+ *
+ * Copyright IBM, Corp. 2017
+ *
+ * Authors:
+ *  Greg Kurz 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/xattr.h"
+#include "9p-util.h"
+
+ssize_t fgetxattrat_nofollow(int dirfd, const char *filename, const char *name,
+ void *value, size_t size)
+{
+char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
+int ret;
+
+ret = lgetxattr(proc_path, name, value, size);
+g_free(proc_path);
+return ret;
+}
+
+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.c b/hw/9pfs/9p-util.c
deleted file mode 100644
index 614b7fc..000
--- a/hw/9pfs/9p-util.c
+++ /dev/null
@@ -1,59 +0,0 @@
-/*
- * 9p utilities
- *
- * Copyright IBM, Corp. 2017
- *
- * Authors:
- *  Greg Kurz 
- *
- * This work is licensed under the terms of the GNU GPL, version 2 or later.
- * See the COPYING file in the top-level directory.
- */
-
-#include "qemu/osdep.h"
-#include "qemu/xattr.h"
-#include "9p-util.h"
-
-ssize_t fgetxattrat_nofollow(int dirfd, const char *filename, const char *name,
- void *value, size_t size)
-{
-char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
-int ret;
-
-ret = lgetxattr(proc_path, name, value, size);
-g_free(proc_path);
-return ret;
-}
-
-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/Makefile.objs b/hw/9pfs/Makefile.objs
index e3fa673..95e3bc0 100644
--- a/hw/9pfs/Makefile.objs
+++ b/hw/9pfs/Makefile.objs
@@ -1,5 +1,6 @@
 ifeq ($(call lor,$(CONFIG_VIRTIO_9P),$(CONFIG_XEN)),y)
-common-obj-y  = 9p.o 9p-util.o
+common-obj-y  = 9p.o
+common-obj-$(CONFIG_LINUX) += 9p-util-linux.o
 common-obj-y += 9p-local.o 9p-xattr.o
 common-obj-y += 9p-xattr-user.o 9p-posix-acl.o
 common-obj-y += coth.o cofs.o codir.o cofile.o
-- 
2.8.1

[Qemu-devel] [PATCH v3 05/13] 9p: darwin: Explicitly cast comparisons of mode_t with -1

[Keno Fischer]

Comparisons of mode_t with -1 require an explicit cast, since mode_t
is unsigned on Darwin.

Signed-off-by: Keno Fischer 
---
 hw/9pfs/9p-local.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index d713983..98d4073 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -310,7 +310,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 +416,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", _mode,
sizeof(mode_t), 0);
-- 
2.8.1

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

[Keno Fischer]

From: Keno Fischer 

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

Signed-off-by: Keno Fischer 
---
 fsdev/file-op-9p.h  |  2 +-
 fsdev/virtfs-proxy-helper.c |  4 +++-
 hw/9pfs/9p-local.c  |  2 ++
 include/qemu/statfs.h   | 19 +++
 include/qemu/xattr.h|  4 +++-
 5 files changed, 28 insertions(+), 3 deletions(-)
 create mode 100644 include/qemu/statfs.h

diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
index 3fa062b..111f804 100644
--- a/fsdev/file-op-9p.h
+++ b/fsdev/file-op-9p.h
@@ -16,7 +16,7 @@
 
 #include 
 #include 
-#include 
+#include "qemu/statfs.h"
 #include "qemu-fsdev-throttle.h"
 
 #define SM_LOCAL_MODE_BITS0600
diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
index 6f132c5..94fb069 100644
--- a/fsdev/virtfs-proxy-helper.c
+++ b/fsdev/virtfs-proxy-helper.c
@@ -13,17 +13,19 @@
 #include 
 #include 
 #include 
+#ifdef CONFIG_LINUX
 #include 
 #include 
-#include 
 #include 
 #include 
 #ifdef CONFIG_LINUX_MAGIC_H
 #include 
 #endif
+#endif
 #include "qemu-common.h"
 #include "qemu/sockets.h"
 #include "qemu/xattr.h"
+#include "qemu/statfs.h"
 #include "9p-iov-marshal.h"
 #include "hw/9pfs/9p-proxy.h"
 #include "fsdev/9p-iov-marshal.h"
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 828e8d6..d713983 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 
+#ifdef CONFIG_LINUX
 #include 
 #ifdef CONFIG_LINUX_MAGIC_H
 #include 
 #endif
+#endif
 #include 
 
 #ifndef XFS_SUPER_MAGIC
diff --git a/include/qemu/statfs.h b/include/qemu/statfs.h
new file mode 100644
index 000..dde289f
--- /dev/null
+++ b/include/qemu/statfs.h
@@ -0,0 +1,19 @@
+/*
+ * Host statfs header abstraction
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2, or any
+ * later version.  See the COPYING file in the top-level directory.
+ *
+ */
+#ifndef QEMU_STATFS_H
+#define QEMU_STATFS_H
+
+#ifdef CONFIG_LINUX
+# include 
+#endif
+#ifdef CONFIG_DARWIN
+# include 
+# include 
+#endif
+
+#endif
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 
 #else
-#  define ENOATTR ENODATA
+#  if !defined(ENOATTR)
+#define ENOATTR ENODATA
+#  endif
 #  include 
 #endif
 
-- 
2.8.1

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

[Keno Fischer]

Hi Greg,

this is the rebased version of the patch series adding
support for building the 9p server on Darwin. As you
know a number of patches from the v2 version of this
series are already landed. This is the remaining patches.
Other than rebasing, there is onnly one minor change
in patch 11.

Keno

Keno Fischer (13):
  9p: linux: Fix a couple Linux assumptions
  9p: Rename 9p-util -> 9p-util-linux
  9p: darwin: Handle struct stat(fs) differences
  9p: darwin: Handle struct dirent differences
  9p: darwin: Explicitly cast comparisons of mode_t with -1
  9p: darwin: Ignore O_{NOATIME, DIRECT}
  9p: darwin: Provide a compatibility definition for XATTR_SIZE_MAX
  9p: darwin: *xattr_nofollow implementations
  9p: darwin: Compatibility for f/l*xattr
  9p: darwin: Provide a fallback implementation for utimensat
  9p: darwin: Implement compatibility for mknodat
  9p: darwin: virtfs-proxy: Implement setuid code for darwin
  9p: darwin: configure: Allow VirtFS on Darwin

 Makefile|   6 ++
 Makefile.objs   |   1 +
 configure   |  22 +++--
 fsdev/file-op-9p.h  |   2 +-
 fsdev/virtfs-proxy-helper.c | 230 
 hw/9pfs/9p-local.c  |  25 +++--
 hw/9pfs/9p-proxy.c  |  17 +++-
 hw/9pfs/9p-synth.c  |   4 +
 hw/9pfs/9p-util-darwin.c| 191 
 hw/9pfs/9p-util-linux.c |  70 ++
 hw/9pfs/9p-util.c   |  59 
 hw/9pfs/9p-util.h   |  27 ++
 hw/9pfs/9p.c|  71 --
 hw/9pfs/Makefile.objs   |   4 +-
 include/qemu/statfs.h   |  19 
 include/qemu/xattr.h|   4 +-
 16 files changed, 579 insertions(+), 173 deletions(-)
 create mode 100644 hw/9pfs/9p-util-darwin.c
 create mode 100644 hw/9pfs/9p-util-linux.c
 delete mode 100644 hw/9pfs/9p-util.c
 create mode 100644 include/qemu/statfs.h

-- 
2.8.1

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

[Keno Fischer]

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

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index e650459..abfb8dc 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -3374,6 +3374,13 @@ out_nofid:
 v9fs_string_free();
 }
 
+#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, rflags = 0;
-- 
2.8.1

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

[Keno Fischer]

Signed-off-by: Keno Fischer 
---
 fsdev/virtfs-proxy-helper.c | 14 +++---
 hw/9pfs/9p-proxy.c  | 17 ++---
 hw/9pfs/9p-synth.c  |  2 ++
 hw/9pfs/9p.c| 16 ++--
 4 files changed, 41 insertions(+), 8 deletions(-)

diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
index 94fb069..3bc1269 100644
--- a/fsdev/virtfs-proxy-helper.c
+++ b/fsdev/virtfs-proxy-helper.c
@@ -506,12 +506,15 @@ static void stat_to_prstat(ProxyStat *pr_stat, struct 
stat *stat)
 pr_stat->st_size = stat->st_size;
 pr_stat->st_blksize = stat->st_blksize;
 pr_stat->st_blocks = stat->st_blocks;
+#ifdef CONFIG_DARWIN
+pr_stat->st_atim_nsec = stat->st_atimespec.tv_nsec;
+pr_stat->st_mtim_nsec = stat->st_mtimespec.tv_nsec;
+pr_stat->st_ctim_nsec = stat->st_ctimespec.tv_nsec;
+#else
 pr_stat->st_atim_sec = stat->st_atim.tv_sec;
-pr_stat->st_atim_nsec = stat->st_atim.tv_nsec;
 pr_stat->st_mtim_sec = stat->st_mtim.tv_sec;
-pr_stat->st_mtim_nsec = stat->st_mtim.tv_nsec;
 pr_stat->st_ctim_sec = stat->st_ctim.tv_sec;
-pr_stat->st_ctim_nsec = stat->st_ctim.tv_nsec;
+#endif
 }
 
 static void statfs_to_prstatfs(ProxyStatFS *pr_stfs, struct statfs *stfs)
@@ -524,10 +527,15 @@ static void statfs_to_prstatfs(ProxyStatFS *pr_stfs, 
struct statfs *stfs)
 pr_stfs->f_bavail = stfs->f_bavail;
 pr_stfs->f_files = stfs->f_files;
 pr_stfs->f_ffree = stfs->f_ffree;
+#ifdef CONFIG_DARWIN
+pr_stfs->f_fsid[0] = stfs->f_fsid.val[0];
+pr_stfs->f_fsid[1] = stfs->f_fsid.val[1];
+#else
 pr_stfs->f_fsid[0] = stfs->f_fsid.__val[0];
 pr_stfs->f_fsid[1] = stfs->f_fsid.__val[1];
 pr_stfs->f_namelen = stfs->f_namelen;
 pr_stfs->f_frsize = stfs->f_frsize;
+#endif
 }
 
 /*
diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c
index 47a94e0..8a2c174 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] & 0xU;
+stfs->f_fsid.val[1] = prstfs->f_fsid[1] >> 32 & 0xU;
+#else
 stfs->f_fsid.__val[0] = prstfs->f_fsid[0] & 0xU;
 stfs->f_fsid.__val[1] = prstfs->f_fsid[1] >> 32 & 0xU;
 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 eef289e..8e6b908 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

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

[Keno Fischer]

Darwin doesn't have either of these flags. Darwin does have
F_NOCACHE, which is similar to O_DIRECT, but has different
enough semantics that other projects don't generally map
them automatically. In any case, we don't support O_DIRECT
on Linux at the moment either.

Signed-off-by: Keno Fischer 
---
 hw/9pfs/9p.c | 11 ++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 06139c9..e650459 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -123,11 +123,18 @@ static int dotl_to_open_flags(int flags)
 { P9_DOTL_NONBLOCK, O_NONBLOCK } ,
 { P9_DOTL_DSYNC, O_DSYNC },
 { P9_DOTL_FASYNC, FASYNC },
+#ifndef CONFIG_DARWIN
+{ P9_DOTL_NOATIME, O_NOATIME },
+/* On Darwin, we could map to F_NOCACHE, which is
+   similar, but doesn't quite have the same
+   semantics. However, we don't support O_DIRECT
+   even on linux at the moment, so we just ignore
+   it here. */
 { P9_DOTL_DIRECT, O_DIRECT },
+#endif
 { P9_DOTL_LARGEFILE, O_LARGEFILE },
 { P9_DOTL_DIRECTORY, O_DIRECTORY },
 { P9_DOTL_NOFOLLOW, O_NOFOLLOW },
-{ P9_DOTL_NOATIME, O_NOATIME },
 { P9_DOTL_SYNC, O_SYNC },
 };
 
@@ -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

[Qemu-devel] [PATCH] ui: darwin: gtk: Add missing input keymap

[Keno Fischer]

In appears the input keymap for osx was forgotten in the commit that
converted the gtk frontend to keycodemapdb. Add it.

Fixes: 2ec78706 ("ui: convert GTK and SDL1 frontends to keycodemapdb")
CC: Daniel P. Berrange 
Signed-off-by: Keno Fischer 
---
 Makefile   | 1 +
 include/ui/input.h | 3 +++
 ui/input-keymap.c  | 1 +
 3 files changed, 5 insertions(+)

diff --git a/Makefile b/Makefile
index 6a5fff0..1e79914 100644
--- a/Makefile
+++ b/Makefile
@@ -322,6 +322,7 @@ KEYCODEMAP_FILES = \
 ui/input-keymap-xorgkbd-to-qcode.c \
 ui/input-keymap-xorgxquartz-to-qcode.c \
 ui/input-keymap-xorgxwin-to-qcode.c \
+ui/input-keymap-osx-to-qcode.c \
 $(NULL)
 
 GENERATED_FILES += $(KEYCODEMAP_FILES)
diff --git a/include/ui/input.h b/include/ui/input.h
index 16395ab..34ebc67 100644
--- a/include/ui/input.h
+++ b/include/ui/input.h
@@ -116,4 +116,7 @@ extern const guint16 qemu_input_map_xorgxquartz_to_qcode[];
 extern const guint qemu_input_map_xorgxwin_to_qcode_len;
 extern const guint16 qemu_input_map_xorgxwin_to_qcode[];
 
+extern const guint qemu_input_map_osx_to_qcode_len;
+extern const guint16 qemu_input_map_osx_to_qcode[];
+
 #endif /* INPUT_H */
diff --git a/ui/input-keymap.c b/ui/input-keymap.c
index 87cc301..db5ccff 100644
--- a/ui/input-keymap.c
+++ b/ui/input-keymap.c
@@ -21,6 +21,7 @@
 #include "ui/input-keymap-xorgkbd-to-qcode.c"
 #include "ui/input-keymap-xorgxquartz-to-qcode.c"
 #include "ui/input-keymap-xorgxwin-to-qcode.c"
+#include "ui/input-keymap-osx-to-qcode.c"
 
 int qemu_input_linux_to_qcode(unsigned int lnx)
 {
-- 
2.8.1

[Qemu-devel] [PATCH v5] cutils: Provide strchrnul

[Keno Fischer]

strchrnul is a GNU extension and thus unavailable on a number of targets.
In the review for a commit removing strchrnul from 9p, I was asked to
create a qemu_strchrnul helper to factor out this functionality.
Do so, and use it in a number of other places in the code base that inlined
the replacement pattern in a place where strchrnul could be used.

Signed-off-by: Keno Fischer 
Acked-by: Greg Kurz 
---

Changes since v4:
 - `CONFIG_STRCHRNUL` -> `HAVE_STRCHRNUL` name change
 - In the strchrnul configure check
   - Return the value of strchrnul to avoid it being optimized away
   - Use `` (i.e. the assembly of the generated function as
 the haystack) to avoid concerns about constant folding. `main`
 seemed like the simplest symbol guaranteed to exist, but with
 non-foldable content.

 configure | 18 ++
 hmp.c |  8 
 hw/9pfs/9p-local.c|  2 +-
 include/qemu/cutils.h |  8 
 monitor.c |  8 ++--
 util/cutils.c | 15 +++
 util/qemu-option.c|  6 +-
 util/uri.c|  6 ++
 8 files changed, 51 insertions(+), 20 deletions(-)

diff --git a/configure b/configure
index 14b1113..8a75745 100755
--- a/configure
+++ b/configure
@@ -4747,6 +4747,21 @@ if compile_prog "" "" ; then
 fi
 
 ##
+# check if we have strchrnul
+
+strchrnul=no
+cat > $TMPC << EOF
+#include 
+int main(void);
+// Use a haystack that the compiler shouldn't be able to constant fold
+char *haystack = (char*)
+int main(void) { return strchrnul(haystack, 'x') != [6]; }
+EOF
+if compile_prog "" "" ; then
+strchrnul=yes
+fi
+
+##
 # check if trace backend exists
 
 $python "$source_path/scripts/tracetool.py" "--backends=$trace_backends" 
--check-backends  > /dev/null 2> /dev/null
@@ -6229,6 +6244,9 @@ fi
 if test "$sem_timedwait" = "yes" ; then
   echo "CONFIG_SEM_TIMEDWAIT=y" >> $config_host_mak
 fi
+if test "$strchrnul" = "yes" ; then
+  echo "HAVE_STRCHRNUL=y" >> $config_host_mak
+fi
 if test "$byteswap_h" = "yes" ; then
   echo "CONFIG_BYTESWAP_H=y" >> $config_host_mak
 fi
diff --git a/hmp.c b/hmp.c
index ef93f48..416d4c9 100644
--- a/hmp.c
+++ b/hmp.c
@@ -2123,12 +2123,12 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict)
 int has_hold_time = qdict_haskey(qdict, "hold-time");
 int hold_time = qdict_get_try_int(qdict, "hold-time", -1);
 Error *err = NULL;
-char *separator;
+const char *separator;
 int keyname_len;
 
 while (1) {
-separator = strchr(keys, '-');
-keyname_len = separator ? separator - keys : strlen(keys);
+separator = qemu_strchrnul(keys, '-');
+keyname_len = separator - keys;
 
 /* Be compatible with old interface, convert user inputted "<" */
 if (keys[0] == '<' && keyname_len == 1) {
@@ -2165,7 +2165,7 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict)
 keylist->value->u.qcode.data = idx;
 }
 
-if (!separator) {
+if (!*separator) {
 break;
 }
 keys = separator + 1;
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 5721eff..828e8d6 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -65,7 +65,7 @@ int local_open_nofollow(FsContext *fs_ctx, const char *path, 
int flags,
 assert(*path != '/');
 
 head = g_strdup(path);
-c = strchrnul(path, '/');
+c = qemu_strchrnul(path, '/');
 if (*c) {
 /* Intermediate path element */
 head[c - path] = 0;
diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
index a663340..274d419 100644
--- a/include/qemu/cutils.h
+++ b/include/qemu/cutils.h
@@ -122,6 +122,14 @@ int qemu_strnlen(const char *s, int max_len);
  * Returns: the pointer originally in @input.
  */
 char *qemu_strsep(char **input, const char *delim);
+#ifdef HAVE_STRCHRNUL
+static inline const char *qemu_strchrnul(const char *s, int c)
+{
+return strchrnul(s, c);
+}
+#else
+const char *qemu_strchrnul(const char *s, int c);
+#endif
 time_t mktimegm(struct tm *tm);
 int qemu_fdatasync(int fd);
 int fcntl_setfl(int fd, int flag);
diff --git a/monitor.c b/monitor.c
index 6d0cec5..4484d74 100644
--- a/monitor.c
+++ b/monitor.c
@@ -797,9 +797,7 @@ static int compare_cmd(const char *name, const char *list)
 p = list;
 for(;;) {
 pstart = p;
-p = strchr(p, '|');
-if (!p)
-p = pstart + strlen(pstart);
+p = qemu_strchrnul(p, '|');
 if ((p - pstart) == len && !memcmp(pstart, name, len))
 return 1;
 if (*p == '\0')
@@ -3400,9 +3398,7 @@ static void cmd_completion(Monitor *mon, const char 

Re: [Qemu-devel] [PATCH v4] cutils: Provide strchrnul

[Keno Fischer]

> Suggest return strchrnul("Hello World", 'W') != 6, to avoid worries
> about a sufficiently smart compilers optimizing out a call that would
> otherwise fail to link, say because headers don't match libraries.

I'm happy to do that, but then again, a sufficiently smart compiler might
constant fold this call entirely, so to be completely safe maybe we need

extern char *haystack;
extern char needle;
int main(void) { return strchrnul(haystack, needle) != 6; }

Though frankly if you're in a position for this to be a problem, you've
got bigger problems. Happy to change this though.

> Should this be named HAVE_STRCHRNUL?  It's how it would be named with
> Autoconf...

Ok, I will rename this.

>> +const char *qemu_strchrnul(const char *s, int c)
>> +{
>> +const char *e = strchr(s, c);
>> +if (!e) {
>> +e = s + strlen(s);
>> +}
>> +return e;
>
> Stupidest solution that could possibly work.  Okay :)

Well, it's the pattern that was used everywhere in place of this function,
so certainly from a commit factoring this seemed like the most sensible
thing to do.

> How did you find the spots to convert to strchrnul()?

I audited uses of `strchr` and checked for whether they were really doing
`strchrnul` (plus the one use case in code that used to only ever be compiled
on Linux).

[Qemu-devel] [PATCH v4] cutils: Provide strchrnul

[Keno Fischer]

strchrnul is a GNU extension and thus unavailable on a number of targets.
In the review for a commit removing strchrnul from 9p, I was asked to
create a qemu_strchrnul helper to factor out this functionality.
Do so, and use it in a number of other places in the code base that inlined
the replacement pattern in a place where strchrnul could be used.

Signed-off-by: Keno Fischer 
Acked-by: Greg Kurz 
---

Changes since v3:
 - Fix bug in configure check

 configure | 15 +++
 hmp.c |  8 
 hw/9pfs/9p-local.c|  2 +-
 include/qemu/cutils.h |  8 
 monitor.c |  8 ++--
 util/cutils.c | 15 +++
 util/qemu-option.c|  6 +-
 util/uri.c|  6 ++
 8 files changed, 48 insertions(+), 20 deletions(-)

diff --git a/configure b/configure
index 14b1113..f5ca850 100755
--- a/configure
+++ b/configure
@@ -4747,6 +4747,18 @@ if compile_prog "" "" ; then
 fi
 
 ##
+# check if we have strchrnul
+
+strchrnul=no
+cat > $TMPC << EOF
+#include 
+int main(void) { (void)strchrnul("Hello World", 'W'); return 0; }
+EOF
+if compile_prog "" "" ; then
+strchrnul=yes
+fi
+
+##
 # check if trace backend exists
 
 $python "$source_path/scripts/tracetool.py" "--backends=$trace_backends" 
--check-backends  > /dev/null 2> /dev/null
@@ -6229,6 +6241,9 @@ fi
 if test "$sem_timedwait" = "yes" ; then
   echo "CONFIG_SEM_TIMEDWAIT=y" >> $config_host_mak
 fi
+if test "$strchrnul" = "yes" ; then
+  echo "CONFIG_STRCHRNUL=y" >> $config_host_mak
+fi
 if test "$byteswap_h" = "yes" ; then
   echo "CONFIG_BYTESWAP_H=y" >> $config_host_mak
 fi
diff --git a/hmp.c b/hmp.c
index ef93f48..416d4c9 100644
--- a/hmp.c
+++ b/hmp.c
@@ -2123,12 +2123,12 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict)
 int has_hold_time = qdict_haskey(qdict, "hold-time");
 int hold_time = qdict_get_try_int(qdict, "hold-time", -1);
 Error *err = NULL;
-char *separator;
+const char *separator;
 int keyname_len;
 
 while (1) {
-separator = strchr(keys, '-');
-keyname_len = separator ? separator - keys : strlen(keys);
+separator = qemu_strchrnul(keys, '-');
+keyname_len = separator - keys;
 
 /* Be compatible with old interface, convert user inputted "<" */
 if (keys[0] == '<' && keyname_len == 1) {
@@ -2165,7 +2165,7 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict)
 keylist->value->u.qcode.data = idx;
 }
 
-if (!separator) {
+if (!*separator) {
 break;
 }
 keys = separator + 1;
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 5721eff..828e8d6 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -65,7 +65,7 @@ int local_open_nofollow(FsContext *fs_ctx, const char *path, 
int flags,
 assert(*path != '/');
 
 head = g_strdup(path);
-c = strchrnul(path, '/');
+c = qemu_strchrnul(path, '/');
 if (*c) {
 /* Intermediate path element */
 head[c - path] = 0;
diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
index a663340..809090c 100644
--- a/include/qemu/cutils.h
+++ b/include/qemu/cutils.h
@@ -122,6 +122,14 @@ int qemu_strnlen(const char *s, int max_len);
  * Returns: the pointer originally in @input.
  */
 char *qemu_strsep(char **input, const char *delim);
+#ifdef CONFIG_STRCHRNUL
+static inline const char *qemu_strchrnul(const char *s, int c)
+{
+return strchrnul(s, c);
+}
+#else
+const char *qemu_strchrnul(const char *s, int c);
+#endif
 time_t mktimegm(struct tm *tm);
 int qemu_fdatasync(int fd);
 int fcntl_setfl(int fd, int flag);
diff --git a/monitor.c b/monitor.c
index 6d0cec5..4484d74 100644
--- a/monitor.c
+++ b/monitor.c
@@ -797,9 +797,7 @@ static int compare_cmd(const char *name, const char *list)
 p = list;
 for(;;) {
 pstart = p;
-p = strchr(p, '|');
-if (!p)
-p = pstart + strlen(pstart);
+p = qemu_strchrnul(p, '|');
 if ((p - pstart) == len && !memcmp(pstart, name, len))
 return 1;
 if (*p == '\0')
@@ -3400,9 +3398,7 @@ static void cmd_completion(Monitor *mon, const char 
*name, const char *list)
 p = list;
 for(;;) {
 pstart = p;
-p = strchr(p, '|');
-if (!p)
-p = pstart + strlen(pstart);
+p = qemu_strchrnul(p, '|');
 len = p - pstart;
 if (len > sizeof(cmd) - 2)
 len = sizeof(cmd) - 2;
diff --git a/util/cutils.c b/util/cutils.c
index 0de69e6..c365ddb 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -545,6 +545,21 @@ int qemu_strtou64(co

[Qemu-devel] [PATCH v3 1/5] cutils: Provide strchrnul

[Keno Fischer]

strchrnul is a GNU extension and thus unavailable on a number of targets.
In the review for a commit removing strchrnul from 9p, I was asked to
create a qemu_strchrnul helper to factor out this functionality.
Do so, and use it in a number of other places in the code base that inlined
the replacement pattern in a place where strchrnul could be used.

Signed-off-by: Keno Fischer 
Acked-by: Greg Kurz 
---

Changes since v2:
 * Add configure check as suggested by Greg Kurz ,
   and requested by Eric Blake 
 * Use qemu_strchrnul in hmp_sendkey as suggested by
   Dr. David Alan Gilbert 

 configure | 15 +++
 hmp.c |  8 
 hw/9pfs/9p-local.c|  2 +-
 include/qemu/cutils.h |  8 
 monitor.c |  8 ++--
 util/cutils.c | 15 +++
 util/qemu-option.c|  6 +-
 util/uri.c|  6 ++
 8 files changed, 48 insertions(+), 20 deletions(-)

diff --git a/configure b/configure
index a6a4616..1b3ca4e 100755
--- a/configure
+++ b/configure
@@ -4754,6 +4754,18 @@ if compile_prog "" "" ; then
 fi
 
 ##
+# check if we have strchrnul
+
+strchrnul=no
+cat > $TMPC << EOF
+#include 
+int main(void) { (void)strchrnul("Hello World", 'W'); }
+EOF
+if compile_prog "" "" ; then
+strchrnul=yes
+fi
+
+##
 # check if trace backend exists
 
 $python "$source_path/scripts/tracetool.py" "--backends=$trace_backends" 
--check-backends  > /dev/null 2> /dev/null
@@ -6210,6 +6222,9 @@ fi
 if test "$sem_timedwait" = "yes" ; then
   echo "CONFIG_SEM_TIMEDWAIT=y" >> $config_host_mak
 fi
+if test "$strchrnul" = "yes" ; then
+  echo "CONFIG_STRCHRNUL=y" >> $config_host_mak
+fi
 if test "$byteswap_h" = "yes" ; then
   echo "CONFIG_BYTESWAP_H=y" >> $config_host_mak
 fi
diff --git a/hmp.c b/hmp.c
index ef93f48..416d4c9 100644
--- a/hmp.c
+++ b/hmp.c
@@ -2123,12 +2123,12 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict)
 int has_hold_time = qdict_haskey(qdict, "hold-time");
 int hold_time = qdict_get_try_int(qdict, "hold-time", -1);
 Error *err = NULL;
-char *separator;
+const char *separator;
 int keyname_len;
 
 while (1) {
-separator = strchr(keys, '-');
-keyname_len = separator ? separator - keys : strlen(keys);
+separator = qemu_strchrnul(keys, '-');
+keyname_len = separator - keys;
 
 /* Be compatible with old interface, convert user inputted "<" */
 if (keys[0] == '<' && keyname_len == 1) {
@@ -2165,7 +2165,7 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict)
 keylist->value->u.qcode.data = idx;
 }
 
-if (!separator) {
+if (!*separator) {
 break;
 }
 keys = separator + 1;
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 7758c38..304ef72 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -65,7 +65,7 @@ int local_open_nofollow(FsContext *fs_ctx, const char *path, 
int flags,
 assert(*path != '/');
 
 head = g_strdup(path);
-c = strchrnul(path, '/');
+c = qemu_strchrnul(path, '/');
 if (*c) {
 /* Intermediate path element */
 head[c - path] = 0;
diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
index a663340..809090c 100644
--- a/include/qemu/cutils.h
+++ b/include/qemu/cutils.h
@@ -122,6 +122,14 @@ int qemu_strnlen(const char *s, int max_len);
  * Returns: the pointer originally in @input.
  */
 char *qemu_strsep(char **input, const char *delim);
+#ifdef CONFIG_STRCHRNUL
+static inline const char *qemu_strchrnul(const char *s, int c)
+{
+return strchrnul(s, c);
+}
+#else
+const char *qemu_strchrnul(const char *s, int c);
+#endif
 time_t mktimegm(struct tm *tm);
 int qemu_fdatasync(int fd);
 int fcntl_setfl(int fd, int flag);
diff --git a/monitor.c b/monitor.c
index 922cfc0..e1f01c4 100644
--- a/monitor.c
+++ b/monitor.c
@@ -798,9 +798,7 @@ static int compare_cmd(const char *name, const char *list)
 p = list;
 for(;;) {
 pstart = p;
-p = strchr(p, '|');
-if (!p)
-p = pstart + strlen(pstart);
+p = qemu_strchrnul(p, '|');
 if ((p - pstart) == len && !memcmp(pstart, name, len))
 return 1;
 if (*p == '\0')
@@ -3401,9 +3399,7 @@ static void cmd_completion(Monitor *mon, const char 
*name, const char *list)
 p = list;
 for(;;) {
 pstart = p;
-p = strchr(p, '|');
-if (!p)
-p = pstart + strlen(pstart);
+p = qemu_strchrnul(p, '|');
 len = p - pstart;
 if (len > sizeof(cmd) - 2)
 len = sizeof(cmd) - 2;
diff --git a/util/cuti

[Qemu-devel] [PATCH v3 5/5] 9p: xattr: Properly translate xattrcreate flags

[Keno Fischer]

As with unlinkat, these flags come from the client and need to
be translated to their host values. The protocol values happen
to match linux, but that need not be true in general.

Signed-off-by: Keno Fischer 
---

Changes since v2: New patch

 hw/9pfs/9p.c | 17 +++--
 hw/9pfs/9p.h |  4 
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index c842ec5..eef289e 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -3327,7 +3327,7 @@ out_nofid:
 
 static void coroutine_fn v9fs_xattrcreate(void *opaque)
 {
-int flags;
+int flags, rflags = 0;
 int32_t fid;
 uint64_t size;
 ssize_t err = 0;
@@ -3344,6 +3344,19 @@ static void coroutine_fn v9fs_xattrcreate(void *opaque)
 }
 trace_v9fs_xattrcreate(pdu->tag, pdu->id, fid, name.data, size, flags);
 
+if (flags & ~(P9_XATTR_CREATE | P9_XATTR_REPLACE)) {
+err = -EINVAL;
+goto out_nofid;
+}
+
+if (flags & P9_XATTR_CREATE) {
+rflags |= XATTR_CREATE;
+}
+
+if (flags & P9_XATTR_REPLACE) {
+rflags |= XATTR_REPLACE;
+}
+
 if (size > XATTR_SIZE_MAX) {
 err = -E2BIG;
 goto out_nofid;
@@ -3365,7 +3378,7 @@ static void coroutine_fn v9fs_xattrcreate(void *opaque)
 xattr_fidp->fs.xattr.copied_len = 0;
 xattr_fidp->fs.xattr.xattrwalk_fid = false;
 xattr_fidp->fs.xattr.len = size;
-xattr_fidp->fs.xattr.flags = flags;
+xattr_fidp->fs.xattr.flags = rflags;
 v9fs_string_init(_fidp->fs.xattr.name);
 v9fs_string_copy(_fidp->fs.xattr.name, );
 xattr_fidp->fs.xattr.value = g_malloc0(size);
diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index bad8ee7..6081b0d 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -169,6 +169,10 @@ typedef struct V9fsConf
 char *fsdev_id;
 } V9fsConf;
 
+/* 9p2000.L xattr flags (matches Linux values) */
+#define P9_XATTR_CREATE 1
+#define P9_XATTR_REPLACE 2
+
 typedef struct V9fsXattr
 {
 uint64_t copied_len;
-- 
2.8.1

[Qemu-devel] [PATCH v3 4/5] 9p: Properly check/translate flags in unlinkat

[Keno Fischer]

The 9p-local code previously relied on P9_DOTL_AT_REMOVEDIR and AT_REMOVEDIR
having the same numerical value and deferred any errorchecking to the
syscall itself. However, while the former assumption is true on Linux,
it is not true in general. 9p-handle did this properly however. Move
the translation code to the generic 9p server code and add an error
if unrecognized flags are passed.

Signed-off-by: Keno Fischer 
---

Changes since v2:
 * Remove 9p-handle code that did the same translation and is now incorrect.

 hw/9pfs/9p-handle.c |  8 +---
 hw/9pfs/9p.c| 13 +++--
 2 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/hw/9pfs/9p-handle.c b/hw/9pfs/9p-handle.c
index 4dc0d2b..f3641db 100644
--- a/hw/9pfs/9p-handle.c
+++ b/hw/9pfs/9p-handle.c
@@ -559,19 +559,13 @@ static int handle_unlinkat(FsContext *ctx, V9fsPath *dir,
 {
 int dirfd, ret;
 HandleData *data = (HandleData *) ctx->private;
-int rflags;
 
 dirfd = open_by_handle(data->mountfd, dir->data, O_PATH);
 if (dirfd < 0) {
 return dirfd;
 }
 
-rflags = 0;
-if (flags & P9_DOTL_AT_REMOVEDIR) {
-rflags |= AT_REMOVEDIR;
-}
-
-ret = unlinkat(dirfd, name, rflags);
+ret = unlinkat(dirfd, name, flags);
 
 close(dirfd);
 return ret;
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 4386d69..c842ec5 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -2522,7 +2522,7 @@ static void coroutine_fn v9fs_unlinkat(void *opaque)
 {
 int err = 0;
 V9fsString name;
-int32_t dfid, flags;
+int32_t dfid, flags, rflags = 0;
 size_t offset = 7;
 V9fsPath path;
 V9fsFidState *dfidp;
@@ -2549,6 +2549,15 @@ static void coroutine_fn v9fs_unlinkat(void *opaque)
 goto out_nofid;
 }
 
+if (flags & ~P9_DOTL_AT_REMOVEDIR) {
+err = -EINVAL;
+goto out_nofid;
+}
+
+if (flags & P9_DOTL_AT_REMOVEDIR) {
+rflags |= AT_REMOVEDIR;
+}
+
 dfidp = get_fid(pdu, dfid);
 if (dfidp == NULL) {
 err = -EINVAL;
@@ -2567,7 +2576,7 @@ static void coroutine_fn v9fs_unlinkat(void *opaque)
 if (err < 0) {
 goto out_err;
 }
-err = v9fs_co_unlinkat(pdu, >path, , flags);
+err = v9fs_co_unlinkat(pdu, >path, , rflags);
 if (!err) {
 err = offset;
 }
-- 
2.8.1

[Qemu-devel] [PATCH v3 3/5] 9p: local: Avoid warning if FS_IOC_GETVERSION is not defined

[Keno Fischer]

Both `stbuf` and `local_ioc_getversion` where unused when
FS_IOC_GETVERSION was not defined, causing a compiler warning.

Reorganize the code to avoid this warning.

Signed-off-by: Keno Fischer 
---
 hw/9pfs/9p-local.c | 40 +++-
 1 file changed, 23 insertions(+), 17 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 304ef72..828e8d6 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1373,10 +1373,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;
 
@@ -1395,32 +1395,21 @@ static int local_ioc_getversion(FsContext *ctx, 
V9fsPath *path,
 err = ioctl(fid_open.fd, FS_IOC_GETVERSION, st_gen);
 local_close(ctx, _open);
 return err;
-#else
-errno = ENOTTY;
-return -1;
-#endif
 }
+#endif
 
-static int local_init(FsContext *ctx, Error **errp)
+static int local_ioc_getversion_init(FsContext *ctx, LocalData *data, Error 
**errp)
 {
+#ifdef FS_IOC_GETVERSION
 struct statfs stbuf;
-LocalData *data = g_malloc(sizeof(*data));
 
-data->mountfd = open(ctx->fs_root, O_DIRECTORY | O_RDONLY);
-if (data->mountfd == -1) {
-error_setg_errno(errp, errno, "failed to open '%s'", ctx->fs_root);
-goto err;
-}
-
-#ifdef FS_IOC_GETVERSION
 /*
  * use ioc_getversion only if the ioctl is definied
  */
 if (fstatfs(data->mountfd, ) < 0) {
-close_preserve_errno(data->mountfd);
 error_setg_errno(errp, errno,
-"failed to stat file system at '%s'", ctx->fs_root);
-goto err;
+ "failed to stat file system at '%s'", ctx->fs_root);
+return -1;
 }
 switch (stbuf.f_type) {
 case EXT2_SUPER_MAGIC:
@@ -1431,6 +1420,23 @@ static int local_init(FsContext *ctx, Error **errp)
 break;
 }
 #endif
+return 0;
+}
+
+static int local_init(FsContext *ctx, Error **errp)
+{
+LocalData *data = g_malloc(sizeof(*data));
+
+data->mountfd = open(ctx->fs_root, O_DIRECTORY | O_RDONLY);
+if (data->mountfd == -1) {
+error_setg_errno(errp, errno, "failed to open '%s'", ctx->fs_root);
+goto err;
+}
+
+if (local_ioc_getversion_init(ctx, data, errp) < 0) {
+close(data->mountfd);
+goto err;
+}
 
 if (ctx->export_flags & V9FS_SM_PASSTHROUGH) {
 ctx->xops = passthrough_xattr_ops;
-- 
2.8.1

[Qemu-devel] [PATCH v3 2/5] 9p: xattr: Fix crashes due to free of uninitialized value

[Keno Fischer]

If the size returned from llistxattr/lgetxattr is 0, we skipped
the malloc call, leaving xattr.value uninitialized. However, this
value is later passed to `g_free` without any further checks,
causing an error. Fix that by always calling g_malloc unconditionally.
If `size` is 0, it will return NULL, which is safe to pass to g_free.

Signed-off-by: Keno Fischer 
---

Changes since v2:
 * Fix another instance of the problematic pattern later in the same function.

 hw/9pfs/9p.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index d74302d..4386d69 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -3256,8 +3256,8 @@ static void coroutine_fn v9fs_xattrwalk(void *opaque)
 xattr_fidp->fs.xattr.len = size;
 xattr_fidp->fid_type = P9_FID_XATTR;
 xattr_fidp->fs.xattr.xattrwalk_fid = true;
+xattr_fidp->fs.xattr.value = g_malloc0(size);
 if (size) {
-xattr_fidp->fs.xattr.value = g_malloc0(size);
 err = v9fs_co_llistxattr(pdu, _fidp->path,
  xattr_fidp->fs.xattr.value,
  xattr_fidp->fs.xattr.len);
@@ -3289,8 +3289,8 @@ static void coroutine_fn v9fs_xattrwalk(void *opaque)
 xattr_fidp->fs.xattr.len = size;
 xattr_fidp->fid_type = P9_FID_XATTR;
 xattr_fidp->fs.xattr.xattrwalk_fid = true;
+xattr_fidp->fs.xattr.value = g_malloc0(size);
 if (size) {
-xattr_fidp->fs.xattr.value = g_malloc0(size);
 err = v9fs_co_lgetxattr(pdu, _fidp->path,
 , xattr_fidp->fs.xattr.value,
 xattr_fidp->fs.xattr.len);
-- 
2.8.1

[Qemu-devel] [PATCH v3 0/5] Prepratory cleanup for 9p darwin support

[Keno Fischer]

Hi Greg,

this is a respin of just the first couple of patches of my 9p Darwin
series. These patches should be applicable independent of the darwin
work.

Keno Fischer (5):
  cutils: Provide strchrnul
  9p: xattr: Fix crashes due to free of uninitialized value
  9p: local: Avoid warning if FS_IOC_GETVERSION is not defined
  9p: Properly error check and translate flags in unlinkat
  9p: xattr: Properly translate xattrcreate flags

 configure | 15 +++
 hmp.c |  8 
 hw/9pfs/9p-handle.c   |  8 +---
 hw/9pfs/9p-local.c| 42 --
 hw/9pfs/9p.c  | 34 --
 hw/9pfs/9p.h  |  4 
 include/qemu/cutils.h |  8 
 monitor.c |  8 ++--
 util/cutils.c | 15 +++
 util/qemu-option.c|  6 +-
 util/uri.c|  6 ++
 11 files changed, 104 insertions(+), 50 deletions(-)

-- 
2.8.1

Re: [Qemu-devel] [PATCH v2 15/20] 9p: darwin: *xattr_nofollow implementations

[Keno Fischer]

> I guess this calls for some defines in 9p.h:
>
> /* 9p2000.L says that the 'flags' argument of operation 'xattrcreate'
>  * are derived from Linux setxattr.
>  */
> #define P9_XATTR_CREATE  1
> #define P9_XATTR_REPLACE 2
>
> Please do that in a preparatory patch.
>
> I would also appreciate you look at other 9P operations and
> check if we have other places where we need to translate
> some flags.

I will include this additional patch in the next respin of the series.
I took a look at the remaining protocol messages and it looks
like with the exception of this and unlinkat (the other patch in the
series), flags/open modes are translated properly.

[Qemu-devel] [PATCH v2 16/20] 9p: darwin: Compatibility for f/l*xattr

[Keno Fischer]

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

Signed-off-by: Keno Fischer 
---

Changes from v1: New patch, qemu_fgetxattr had previously been
 moved to 9p-util as fgetxattr_follow. The remaining functions
 are required by the proxy-helper.

 Makefile|  6 ++
 fsdev/virtfs-proxy-helper.c |  9 +
 hw/9pfs/9p-local.c  | 12 
 hw/9pfs/9p-util.h   | 17 +
 4 files changed, 36 insertions(+), 8 deletions(-)

diff --git a/Makefile b/Makefile
index 6d588d1..dac6efd 100644
--- a/Makefile
+++ b/Makefile
@@ -544,7 +544,13 @@ qemu-bridge-helper$(EXESUF): qemu-bridge-helper.o 
$(COMMON_LDADDS)
 qemu-keymap$(EXESUF): qemu-keymap.o ui/input-keymap.o $(COMMON_LDADDS)
 
 fsdev/virtfs-proxy-helper$(EXESUF): fsdev/virtfs-proxy-helper.o 
fsdev/9p-marshal.o fsdev/9p-iov-marshal.o $(COMMON_LDADDS)
+ifdef CONFIG_DARWIN
+fsdev/virtfs-proxy-helper$(EXESUF): hw/9pfs/9p-util-darwin.o
+endif
+ifdef CONFIG_LINUX
+fsdev/virtfs-proxy-helper$(EXESUF): hw/9pfs/9p-util-linux.o
 fsdev/virtfs-proxy-helper$(EXESUF): LIBS += -lcap
+endif
 
 scsi/qemu-pr-helper$(EXESUF): scsi/qemu-pr-helper.o scsi/utils.o 
$(crypto-obj-y) $(io-obj-y) $(qom-obj-y) $(COMMON_LDADDS)
 ifdef CONFIG_MPATH
diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
index 3bc1269..a26f8b8 100644
--- a/fsdev/virtfs-proxy-helper.c
+++ b/fsdev/virtfs-proxy-helper.c
@@ -28,6 +28,7 @@
 #include "qemu/statfs.h"
 #include "9p-iov-marshal.h"
 #include "hw/9pfs/9p-proxy.h"
+#include "hw/9pfs/9p-util.h"
 #include "fsdev/9p-iov-marshal.h"
 
 #define PROGNAME "virtfs-proxy-helper"
@@ -459,7 +460,7 @@ static int do_getxattr(int type, struct iovec *iovec, 
struct iovec *out_iovec)
 v9fs_string_init();
 retval = proxy_unmarshal(iovec, offset, "s", );
 if (retval > 0) {
-retval = lgetxattr(path.data, name.data, xattr.data, size);
+retval = qemu_lgetxattr(path.data, name.data, xattr.data, size);
 if (retval < 0) {
 retval = -errno;
 } else {
@@ -469,7 +470,7 @@ static int do_getxattr(int type, struct iovec *iovec, 
struct iovec *out_iovec)
 v9fs_string_free();
 break;
 case T_LLISTXATTR:
-retval = llistxattr(path.data, xattr.data, size);
+retval = qemu_llistxattr(path.data, xattr.data, size);
 if (retval < 0) {
 retval = -errno;
 } else {
@@ -1000,7 +1001,7 @@ static int process_requests(int sock)
 retval = proxy_unmarshal(_iovec, PROXY_HDR_SZ, "sssdd", ,
  , , , );
 if (retval > 0) {
-retval = lsetxattr(path.data,
+retval = qemu_lsetxattr(path.data,
name.data, value.data, size, flags);
 if (retval < 0) {
 retval = -errno;
@@ -1016,7 +1017,7 @@ static int process_requests(int sock)
 retval = proxy_unmarshal(_iovec,
  PROXY_HDR_SZ, "ss", , );
 if (retval > 0) {
-retval = lremovexattr(path.data, name.data);
+retval = qemu_lremovexattr(path.data, name.data);
 if (retval < 0) {
 retval = -errno;
 }
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 1f0b1b0..7830526 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -776,16 +776,20 @@ static int local_fstat(FsContext *fs_ctx, int fid_type,
 mode_t tmp_mode;
 dev_t tmp_dev;
 
-if (fgetxattr(fd, "user.virtfs.uid", _uid, sizeof(uid_t)) > 0) {
+if (qemu_fgetxattr(fd, "user.virtfs.uid",
+   _uid, sizeof(uid_t)) > 0) {
 stbuf->st_uid = le32_to_cpu(tmp_uid);
 }
-if (fgetxattr(fd, "user.virtfs.gid", _gid, sizeof(gid_t)) > 0) {
+if (qemu_fgetxattr(fd, "user.virtfs.gid",
+   _gid, sizeof(gid_t)) > 0) {
 stbuf->st_gid = le32_to_cpu(tmp_gid);
 }
-if (fgetxattr(fd, "user.virtfs.mode", _mode, sizeof(mode_t)) > 0) {
+if (qemu_fgetxattr(fd, "user.virtfs.mode",
+   _mode, sizeof(mode_t)) > 0) {
 stbuf->st_mode = le32_to_cpu(tmp_mode);
 }
-if (fgetxattr(fd, "user.virtfs.rdev", _dev, sizeof(dev_t)) > 0) {
+if (qemu_fgetxattr(fd, "user.virtfs.rdev",
+   _dev, sizeof(dev_t)) > 0) {
 stbuf->st_rdev = le64_to_cpu(tmp_dev);
 }
 } else if (fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
diff --git a/hw/9pfs/9p-util.h b/hw/9p

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

[Keno Fischer]

Signed-off-by: Keno Fischer 
---

Changes from v1: Now builds the proxy-helper on Darwin.

 Makefile.objs |  1 +
 configure | 22 +++---
 2 files changed, 16 insertions(+), 7 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 a6a4616..4808459 100755
--- a/configure
+++ b/configure
@@ -5535,16 +5535,28 @@ 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
@@ -,10 +5567,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

[Qemu-devel] [PATCH v2 18/20] 9p: darwin: Implement compatibility for mknodat

[Keno Fischer]

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

Signed-off-by: Keno Fischer 
---

Changes from v1: New patch. The previous series marked mknodat unsupported.

 hw/9pfs/9p-local.c   |  5 +++--
 hw/9pfs/9p-util-darwin.c | 25 +
 hw/9pfs/9p-util-linux.c  |  5 +
 hw/9pfs/9p-util.h|  2 ++
 4 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 47e8580..c7a2b08 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -668,7 +668,7 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath 
*dir_path,
 
 if (fs_ctx->export_flags & V9FS_SM_MAPPED ||
 fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
-err = mknodat(dirfd, name, fs_ctx->fmode | S_IFREG, 0);
+err = qemu_mknodat(dirfd, name, fs_ctx->fmode | S_IFREG, 0);
 if (err == -1) {
 goto out;
 }
@@ -683,7 +683,7 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath 
*dir_path,
 }
 } else if (fs_ctx->export_flags & V9FS_SM_PASSTHROUGH ||
fs_ctx->export_flags & V9FS_SM_NONE) {
-err = mknodat(dirfd, name, credp->fc_mode, credp->fc_rdev);
+err = qemu_mknodat(dirfd, name, credp->fc_mode, credp->fc_rdev);
 if (err == -1) {
 goto out;
 }
@@ -696,6 +696,7 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath 
*dir_path,
 
 err_end:
 unlinkat_preserve_errno(dirfd, name, 0);
+
 out:
 close_preserve_errno(dirfd);
 return err;
diff --git a/hw/9pfs/9p-util-darwin.c b/hw/9pfs/9p-util-darwin.c
index ac414bc..49fe7d3 100644
--- a/hw/9pfs/9p-util-darwin.c
+++ b/hw/9pfs/9p-util-darwin.c
@@ -158,3 +158,28 @@ done:
 close_preserve_errno(fd);
 return ret;
 }
+
+#ifndef SYS___pthread_fchdir
+# define SYS___pthread_fchdir 349
+#endif
+
+static int fchdir_thread_local(int fd)
+{
+return syscall(SYS___pthread_fchdir, fd);
+}
+
+int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev)
+{
+int preserved_errno, err;
+if (fchdir_thread_local(dirfd) < 0) {
+return -1;
+}
+err = mknod(filename, mode, dev);
+preserved_errno = errno;
+/* Stop using the thread-local cwd */
+fchdir_thread_local(-1);
+if (err < 0) {
+errno = preserved_errno;
+}
+return err;
+}
diff --git a/hw/9pfs/9p-util-linux.c b/hw/9pfs/9p-util-linux.c
index 3902378..06399c5 100644
--- a/hw/9pfs/9p-util-linux.c
+++ b/hw/9pfs/9p-util-linux.c
@@ -63,3 +63,8 @@ int utimensat_nofollow(int dirfd, const char *filename,
 {
 return utimensat(dirfd, filename, times, AT_SYMLINK_NOFOLLOW);
 }
+
+int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev)
+{
+return mknodat(dirfd, filename, mode, dev);
+}
diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h
index b1dc08a..127564d 100644
--- a/hw/9pfs/9p-util.h
+++ b/hw/9pfs/9p-util.h
@@ -90,4 +90,6 @@ ssize_t fremovexattrat_nofollow(int dirfd, const char 
*filename,
 int utimensat_nofollow(int dirfd, const char *filename,
const struct timespec times[2]);
 
+int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev);
+
 #endif
-- 
2.8.1

[Qemu-devel] [PATCH v2 19/20] 9p: darwin: virtfs-proxy: Implement setuid code for darwin

[Keno Fischer]

Darwin does not have linux capabilities, so make that code linux-only.
Darwin also does not have setresuid/gid. The correct way to temporarily
drop capabilities is to call seteuid/gid.

Also factor out the code that acquires acquire_dac_override into a separate
function in the linux implementation. I had originally done this when
I thought it made sense to have only one `setugid` function, but I retained
this because it seems clearer this way.

Signed-off-by: Keno Fischer 
---

Changes from v1: New patch.

 fsdev/virtfs-proxy-helper.c | 200 +++-
 1 file changed, 125 insertions(+), 75 deletions(-)

diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
index d8dd3f5..6baf2a6 100644
--- a/fsdev/virtfs-proxy-helper.c
+++ b/fsdev/virtfs-proxy-helper.c
@@ -82,6 +82,7 @@ static void do_perror(const char *string)
 }
 }
 
+#ifdef CONFIG_LINUX
 static int do_cap_set(cap_value_t *cap_value, int size, int reset)
 {
 cap_t caps;
@@ -121,6 +122,85 @@ error:
 return -1;
 }
 
+static int acquire_dac_override(void)
+{
+cap_value_t cap_list[] = {
+CAP_DAC_OVERRIDE,
+};
+return do_cap_set(cap_list, ARRAY_SIZE(cap_list), 0);
+}
+
+/*
+ * from man 7 capabilities, section
+ * Effect of User ID Changes on Capabilities:
+ * If the effective user ID is changed from nonzero to 0, then the permitted
+ * set is copied to the effective set.  If the effective user ID is changed
+ * from 0 to nonzero, then all capabilities are are cleared from the effective
+ * set.
+ *
+ * The setfsuid/setfsgid man pages warn that changing the effective user ID may
+ * expose the program to unwanted signals, but this is not true anymore: for an
+ * unprivileged (without CAP_KILL) program to send a signal, the real or
+ * effective user ID of the sending process must equal the real or saved user
+ * ID of the target process.  Even when dropping privileges, it is enough to
+ * keep the saved UID to a "privileged" value and virtfs-proxy-helper won't
+ * be exposed to signals.  So just use setresuid/setresgid.
+ */
+static int setugid(int uid, int gid, int *suid, int *sgid)
+{
+int retval;
+
+*suid = geteuid();
+*sgid = getegid();
+
+if (setresgid(-1, gid, *sgid) == -1) {
+retval = -errno;
+goto err_out;
+}
+
+if (setresuid(-1, uid, *suid) == -1) {
+retval = -errno;
+goto err_sgid;
+}
+
+if (uid != 0 || gid != 0) {
+/*
+* We still need DAC_OVERRIDE because we don't change
+* supplementary group ids, and hence may be subjected DAC rules
+*/
+if (acquire_dac_override() < 0) {
+retval = -errno;
+goto err_suid;
+}
+}
+return 0;
+
+err_suid:
+if (setresuid(-1, *suid, *suid) == -1) {
+abort();
+}
+err_sgid:
+if (setresgid(-1, *sgid, *sgid) == -1) {
+abort();
+}
+err_out:
+return retval;
+}
+
+/*
+ * This is used to reset the ugid back with the saved values
+ * There is nothing much we can do checking error values here.
+ */
+static void resetugid(int suid, int sgid)
+{
+if (setresgid(-1, sgid, sgid) == -1) {
+abort();
+}
+if (setresuid(-1, suid, suid) == -1) {
+abort();
+}
+}
+
 static int init_capabilities(void)
 {
 /* helper needs following capabilities only */
@@ -135,6 +215,51 @@ static int init_capabilities(void)
 };
 return do_cap_set(cap_list, ARRAY_SIZE(cap_list), 1);
 }
+#else
+static int setugid(int uid, int gid, int *suid, int *sgid)
+{
+int retval;
+
+*suid = geteuid();
+*sgid = getegid();
+
+if (setegid(gid) == -1) {
+retval = -errno;
+goto err_out;
+}
+
+if (seteuid(uid) == -1) {
+retval = -errno;
+goto err_sgid;
+}
+
+err_sgid:
+if (setgid(*sgid) == -1) {
+abort();
+}
+err_out:
+return retval;
+}
+
+/*
+ * This is used to reset the ugid back with the saved values
+ * There is nothing much we can do checking error values here.
+ */
+static void resetugid(int suid, int sgid)
+{
+if (setegid(sgid) == -1) {
+abort();
+}
+if (seteuid(suid) == -1) {
+abort();
+}
+}
+
+static int init_capabilities(void)
+{
+return 0;
+}
+#endif
 
 static int socket_read(int sockfd, void *buff, ssize_t size)
 {
@@ -279,81 +404,6 @@ static int send_status(int sockfd, struct iovec *iovec, 
int status)
 }
 
 /*
- * from man 7 capabilities, section
- * Effect of User ID Changes on Capabilities:
- * If the effective user ID is changed from nonzero to 0, then the permitted
- * set is copied to the effective set.  If the effective user ID is changed
- * from 0 to nonzero, then all capabilities are are cleared from the effective
- * set.
- *
- * The setfsuid/setfsgid man pages warn that changing the effective user ID may
- * expose the program to unwanted signals, but this is not true anymore: for an
- * unprivileged (without CAP_KILL) pr

[Qemu-devel] [PATCH v2 12/20] 9p: darwin: Explicitly cast comparisons of mode_t with -1

[Keno Fischer]

Comparisons of mode_t with -1 require an explicit cast, since mode_t
is unsigned on Darwin.

Signed-off-by: Keno Fischer 
---

Changes from v1: Split from strchrnul change

 hw/9pfs/9p-local.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 6222891..1f0b1b0 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -310,7 +310,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 +416,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", _mode,
sizeof(mode_t), 0);
-- 
2.8.1

[Qemu-devel] [PATCH v2 15/20] 9p: darwin: *xattr_nofollow implementations

[Keno Fischer]

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

Signed-off-by: Keno Fischer 
---

Changes from v1:
 * New 9p-util-darwin.c rather than ifdefs in 9p-util.c
 * Drop incorrect AT_NOFOLLOW from the actual call

 hw/9pfs/9p-util-darwin.c | 64 
 hw/9pfs/Makefile.objs|  1 +
 2 files changed, 65 insertions(+)
 create mode 100644 hw/9pfs/9p-util-darwin.c

diff --git a/hw/9pfs/9p-util-darwin.c b/hw/9pfs/9p-util-darwin.c
new file mode 100644
index 000..cdb4c9e
--- /dev/null
+++ b/hw/9pfs/9p-util-darwin.c
@@ -0,0 +1,64 @@
+/*
+ * 9p utilities (Darwin Implementation)
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/xattr.h"
+#include "9p-util.h"
+
+ssize_t fgetxattrat_nofollow(int dirfd, const char *filename, const char *name,
+ void *value, size_t size)
+{
+int ret;
+int fd = openat_file(dirfd, filename,
+ O_RDONLY | O_PATH_9P_UTIL | O_NOFOLLOW, 0);
+if (fd == -1) {
+return -1;
+}
+ret = fgetxattr(fd, name, value, size, 0, 0);
+close_preserve_errno(fd);
+return ret;
+}
+
+ssize_t flistxattrat_nofollow(int dirfd, const char *filename,
+  char *list, size_t size)
+{
+int ret;
+int fd = openat_file(dirfd, filename,
+ O_RDONLY | O_PATH_9P_UTIL | O_NOFOLLOW, 0);
+if (fd == -1) {
+return -1;
+}
+ret = flistxattr(fd, list, size, 0);
+close_preserve_errno(fd);
+return ret;
+}
+
+ssize_t fremovexattrat_nofollow(int dirfd, const char *filename,
+const char *name)
+{
+int ret;
+int fd = openat_file(dirfd, filename, O_PATH_9P_UTIL | O_NOFOLLOW, 0);
+if (fd == -1) {
+return -1;
+}
+ret = fremovexattr(fd, name, 0);
+close_preserve_errno(fd);
+return ret;
+}
+
+int fsetxattrat_nofollow(int dirfd, const char *filename, const char *name,
+ void *value, size_t size, int flags)
+{
+int ret;
+int fd = openat_file(dirfd, filename, O_PATH_9P_UTIL | O_NOFOLLOW, 0);
+if (fd == -1) {
+return -1;
+}
+ret = fsetxattr(fd, name, value, size, 0, flags);
+close_preserve_errno(fd);
+return ret;
+}
diff --git a/hw/9pfs/Makefile.objs b/hw/9pfs/Makefile.objs
index 083508f..24a8695 100644
--- a/hw/9pfs/Makefile.objs
+++ b/hw/9pfs/Makefile.objs
@@ -1,5 +1,6 @@
 common-obj-y  = 9p.o
 common-obj-$(CONFIG_LINUX) += 9p-util-linux.o
+common-obj-$(CONFIG_DARWIN) += 9p-util-darwin.o
 common-obj-y += 9p-local.o 9p-xattr.o
 common-obj-y += 9p-xattr-user.o 9p-posix-acl.o
 common-obj-y += coth.o cofs.o codir.o cofile.o
-- 
2.8.1

[Qemu-devel] [PATCH v2 17/20] 9p: darwin: Provide a fallback implementation for utimensat

[Keno Fischer]

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

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

Signed-off-by: Keno Fischer 
---

Changes from v1:
 * Correct calculation of tv_usec
 * Support UTIME_NOW/UTIME/OMIT
 * Now covers fsdev/virtfs-proxy-helper.c

 fsdev/virtfs-proxy-helper.c |  3 +-
 hw/9pfs/9p-local.c  |  2 +-
 hw/9pfs/9p-util-darwin.c| 96 +
 hw/9pfs/9p-util-linux.c |  6 +++
 hw/9pfs/9p-util.h   |  8 
 hw/9pfs/9p.c|  1 +
 6 files changed, 113 insertions(+), 3 deletions(-)

diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
index a26f8b8..d8dd3f5 100644
--- a/fsdev/virtfs-proxy-helper.c
+++ b/fsdev/virtfs-proxy-helper.c
@@ -957,8 +957,7 @@ static int process_requests(int sock)
  [0].tv_sec, [0].tv_nsec,
  [1].tv_sec, [1].tv_nsec);
 if (retval > 0) {
-retval = utimensat(AT_FDCWD, path.data, spec,
-   AT_SYMLINK_NOFOLLOW);
+retval = utimensat_nofollow(AT_FDCWD, path.data, spec);
 if (retval < 0) {
 retval = -errno;
 }
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 7830526..47e8580 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1071,7 +1071,7 @@ static int local_utimensat(FsContext *s, V9fsPath 
*fs_path,
 goto out;
 }
 
-ret = utimensat(dirfd, name, buf, AT_SYMLINK_NOFOLLOW);
+ret = utimensat_nofollow(dirfd, name, buf);
 close_preserve_errno(dirfd);
 out:
 g_free(dirpath);
diff --git a/hw/9pfs/9p-util-darwin.c b/hw/9pfs/9p-util-darwin.c
index cdb4c9e..ac414bc 100644
--- a/hw/9pfs/9p-util-darwin.c
+++ b/hw/9pfs/9p-util-darwin.c
@@ -62,3 +62,99 @@ int fsetxattrat_nofollow(int dirfd, const char *filename, 
const char *name,
 close_preserve_errno(fd);
 return ret;
 }
+
+#ifndef __has_builtin
+#define __has_builtin(x) 0
+#endif
+
+static int update_times_from_stat(int fd, struct timespec times[2],
+  int update0, int update1)
+{
+struct stat buf;
+int ret = fstat(fd, );
+if (ret == -1) {
+return ret;
+}
+if (update0) {
+times[0] = buf.st_atimespec;
+}
+if (update1) {
+times[1] = buf.st_mtimespec;
+}
+return 0;
+}
+
+int utimensat_nofollow(int dirfd, const char *filename,
+   const struct timespec times_in[2])
+{
+int ret, fd;
+int special0, special1;
+struct timeval futimes_buf[2];
+struct timespec times[2];
+memcpy(times, times_in, 2 * sizeof(struct timespec));
+
+/* Check whether we have an SDK version that defines utimensat */
+#if defined(__MAC_10_13)
+# if __MAC_OS_X_VERSION_MIN_REQUIRED >= __MAC_10_13
+#  define UTIMENSAT_AVAILABLE 1
+# elif __has_builtin(__builtin_available)
+#  define UTIMENSAT_AVAILABLE __builtin_available(macos 10.13, *)
+# else
+#  define UTIMENSAT_AVAILABLE 0
+# endif
+if (UTIMENSAT_AVAILABLE) {
+return utimensat(dirfd, filename, times, AT_SYMLINK_NOFOLLOW);
+}
+#endif
+
+/* utimensat not available. Use futimes. */
+fd = openat_file(dirfd, filename, O_PATH_9P_UTIL | O_NOFOLLOW, 0);
+if (fd == -1) {
+return -1;
+}
+
+special0 = times[0].tv_nsec == UTIME_OMIT;
+special1 = times[1].tv_nsec == UTIME_OMIT;
+if (special0 || special1) {
+/* If both are set, nothing to do */
+if (special0 && special1) {
+ret = 0;
+goto done;
+}
+
+ret = update_times_from_stat(fd, times, special0, special1);
+if (ret < 0) {
+goto done;
+}
+}
+
+special0 = times[0].tv_nsec == UTIME_NOW;
+special1 = times[1].tv_nsec == UTIME_NOW;
+if (special0 || special1) {
+ret = futimes(fd, NULL);
+if (ret < 0) {
+goto done;
+}
+
+/* If both are set, we are done */
+if (special0 && special1) {
+ret = 0;
+goto 

[Qemu-devel] [PATCH v2 10/20] 9p: darwin: Handle struct stat(fs) differences

[Keno Fischer]

Signed-off-by: Keno Fischer 
---

Changes since v1:
 * Now also covers fsdev/virtfs-proxy-helper.c

 fsdev/virtfs-proxy-helper.c | 14 +++---
 hw/9pfs/9p-proxy.c  | 17 ++---
 hw/9pfs/9p-synth.c  |  2 ++
 hw/9pfs/9p.c| 16 ++--
 4 files changed, 41 insertions(+), 8 deletions(-)

diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
index 94fb069..3bc1269 100644
--- a/fsdev/virtfs-proxy-helper.c
+++ b/fsdev/virtfs-proxy-helper.c
@@ -506,12 +506,15 @@ static void stat_to_prstat(ProxyStat *pr_stat, struct 
stat *stat)
 pr_stat->st_size = stat->st_size;
 pr_stat->st_blksize = stat->st_blksize;
 pr_stat->st_blocks = stat->st_blocks;
+#ifdef CONFIG_DARWIN
+pr_stat->st_atim_nsec = stat->st_atimespec.tv_nsec;
+pr_stat->st_mtim_nsec = stat->st_mtimespec.tv_nsec;
+pr_stat->st_ctim_nsec = stat->st_ctimespec.tv_nsec;
+#else
 pr_stat->st_atim_sec = stat->st_atim.tv_sec;
-pr_stat->st_atim_nsec = stat->st_atim.tv_nsec;
 pr_stat->st_mtim_sec = stat->st_mtim.tv_sec;
-pr_stat->st_mtim_nsec = stat->st_mtim.tv_nsec;
 pr_stat->st_ctim_sec = stat->st_ctim.tv_sec;
-pr_stat->st_ctim_nsec = stat->st_ctim.tv_nsec;
+#endif
 }
 
 static void statfs_to_prstatfs(ProxyStatFS *pr_stfs, struct statfs *stfs)
@@ -524,10 +527,15 @@ static void statfs_to_prstatfs(ProxyStatFS *pr_stfs, 
struct statfs *stfs)
 pr_stfs->f_bavail = stfs->f_bavail;
 pr_stfs->f_files = stfs->f_files;
 pr_stfs->f_ffree = stfs->f_ffree;
+#ifdef CONFIG_DARWIN
+pr_stfs->f_fsid[0] = stfs->f_fsid.val[0];
+pr_stfs->f_fsid[1] = stfs->f_fsid.val[1];
+#else
 pr_stfs->f_fsid[0] = stfs->f_fsid.__val[0];
 pr_stfs->f_fsid[1] = stfs->f_fsid.__val[1];
 pr_stfs->f_namelen = stfs->f_namelen;
 pr_stfs->f_frsize = stfs->f_frsize;
+#endif
 }
 
 /*
diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c
index 47a94e0..8a2c174 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] & 0xU;
+stfs->f_fsid.val[1] = prstfs->f_fsid[1] >> 32 & 0xU;
+#else
 stfs->f_fsid.__val[0] = prstfs->f_fsid[0] & 0xU;
 stfs->f_fsid.__val[1] = prstfs->f_fsid[1] >> 32 & 0xU;
 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 a757374..212f569 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.

[Qemu-devel] [PATCH v2 09/20] 9p: Properly check/translate flags in unlinkat

[Keno Fischer]

This code previously relied on P9_DOTL_AT_REMOVEDIR and AT_REMOVEDIR
having the same numerical value and deferred any errorchecking to the
syscall itself. However, while the former assumption is true on Linux,
it is not true in general. Thus, add appropriate error checking and
translation to the 9p unlinkat server code.

Signed-off-by: Keno Fischer 
---

Changes since v1:
 * Code was moved from 9p-local.c to server entry point in 9p.c

 hw/9pfs/9p.c | 13 +++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index b80db65..a757374 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -2522,7 +2522,7 @@ static void coroutine_fn v9fs_unlinkat(void *opaque)
 {
 int err = 0;
 V9fsString name;
-int32_t dfid, flags;
+int32_t dfid, flags, rflags = 0;
 size_t offset = 7;
 V9fsPath path;
 V9fsFidState *dfidp;
@@ -2549,6 +2549,15 @@ static void coroutine_fn v9fs_unlinkat(void *opaque)
 goto out_nofid;
 }
 
+if (flags & ~P9_DOTL_AT_REMOVEDIR) {
+err = -EINVAL;
+goto out_nofid;
+}
+
+if (flags & P9_DOTL_AT_REMOVEDIR) {
+rflags |= AT_REMOVEDIR;
+}
+
 dfidp = get_fid(pdu, dfid);
 if (dfidp == NULL) {
 err = -EINVAL;
@@ -2567,7 +2576,7 @@ static void coroutine_fn v9fs_unlinkat(void *opaque)
 if (err < 0) {
 goto out_err;
 }
-err = v9fs_co_unlinkat(pdu, >path, , flags);
+err = v9fs_co_unlinkat(pdu, >path, , rflags);
 if (!err) {
 err = offset;
 }
-- 
2.8.1

[Qemu-devel] [PATCH v2 14/20] 9p: darwin: Provide a compatibility definition for XATTR_SIZE_MAX

[Keno Fischer]

Signed-off-by: Keno Fischer 
---

No change from v1.

 hw/9pfs/9p.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 70cfab9..24802b9 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -3374,6 +3374,13 @@ out_nofid:
 v9fs_string_free();
 }
 
+#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

[Qemu-devel] [PATCH v2 08/20] 9p: Rename 9p-util -> 9p-util-linux

[Keno Fischer]

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

Signed-off-by: Keno Fischer 
---

Changes since v1: New patch

 hw/9pfs/9p-util-linux.c | 59 +
 hw/9pfs/9p-util.c   | 59 -
 hw/9pfs/Makefile.objs   |  3 ++-
 3 files changed, 61 insertions(+), 60 deletions(-)
 create mode 100644 hw/9pfs/9p-util-linux.c
 delete mode 100644 hw/9pfs/9p-util.c

diff --git a/hw/9pfs/9p-util-linux.c b/hw/9pfs/9p-util-linux.c
new file mode 100644
index 000..defa3a4
--- /dev/null
+++ b/hw/9pfs/9p-util-linux.c
@@ -0,0 +1,59 @@
+/*
+ * 9p utilities (Linux Implementation)
+ *
+ * Copyright IBM, Corp. 2017
+ *
+ * Authors:
+ *  Greg Kurz 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/xattr.h"
+#include "9p-util.h"
+
+ssize_t fgetxattrat_nofollow(int dirfd, const char *filename, const char *name,
+ void *value, size_t size)
+{
+char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
+int ret;
+
+ret = lgetxattr(proc_path, name, value, size);
+g_free(proc_path);
+return ret;
+}
+
+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.c b/hw/9pfs/9p-util.c
deleted file mode 100644
index 614b7fc..000
--- a/hw/9pfs/9p-util.c
+++ /dev/null
@@ -1,59 +0,0 @@
-/*
- * 9p utilities
- *
- * Copyright IBM, Corp. 2017
- *
- * Authors:
- *  Greg Kurz 
- *
- * This work is licensed under the terms of the GNU GPL, version 2 or later.
- * See the COPYING file in the top-level directory.
- */
-
-#include "qemu/osdep.h"
-#include "qemu/xattr.h"
-#include "9p-util.h"
-
-ssize_t fgetxattrat_nofollow(int dirfd, const char *filename, const char *name,
- void *value, size_t size)
-{
-char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
-int ret;
-
-ret = lgetxattr(proc_path, name, value, size);
-g_free(proc_path);
-return ret;
-}
-
-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/Makefile.objs b/hw/9pfs/Makefile.objs
index fd90b62..083508f 100644
--- a/hw/9pfs/Makefile.objs
+++ b/hw/9pfs/Makefile.objs
@@ -1,4 +1,5 @@
-common-obj-y  = 9p.o 9p-util.o
+common-obj-y  = 9p.o
+common-obj-$(CONFIG_LINUX) += 9p-util-linux.o
 common-obj-y += 9p-local.o 9p-xattr.o
 common-obj-y += 9p-xattr-user.o 9p-posix-acl.o
 common-obj-y += coth.o cofs.o codir.o cofile.o
-- 
2.8.1

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

[Keno Fischer]

Darwin doesn't have either of these flags. Darwin does have
F_NOCACHE, which is similar to O_DIRECT, but has different
enough semantics that other projects don't generally map
them automatically. In any case, we don't support O_DIRECT
on Linux at the moment either.

Signed-off-by: Keno Fischer 
---

Changes from v1: Undo accidental formatting change

 hw/9pfs/9p.c | 11 ++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 9751246..70cfab9 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -123,11 +123,18 @@ static int dotl_to_open_flags(int flags)
 { P9_DOTL_NONBLOCK, O_NONBLOCK } ,
 { P9_DOTL_DSYNC, O_DSYNC },
 { P9_DOTL_FASYNC, FASYNC },
+#ifndef CONFIG_DARWIN
+{ P9_DOTL_NOATIME, O_NOATIME },
+/* On Darwin, we could map to F_NOCACHE, which is
+   similar, but doesn't quite have the same
+   semantics. However, we don't support O_DIRECT
+   even on linux at the moment, so we just ignore
+   it here. */
 { P9_DOTL_DIRECT, O_DIRECT },
+#endif
 { P9_DOTL_LARGEFILE, O_LARGEFILE },
 { P9_DOTL_DIRECTORY, O_DIRECTORY },
 { P9_DOTL_NOFOLLOW, O_NOFOLLOW },
-{ P9_DOTL_NOATIME, O_NOATIME },
 { P9_DOTL_SYNC, O_SYNC },
 };
 
@@ -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

[Qemu-devel] [PATCH v2 07/20] 9p: Move a couple xattr functions to 9p-util

[Keno Fischer]

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.

Signed-off-by: Keno Fischer 
---

Changes since v1:
 * fgetxattr_follow is dropped in favor of a different approach
   later in the series.

 hw/9pfs/9p-util.c  | 33 +
 hw/9pfs/9p-util.h  |  4 
 hw/9pfs/9p-xattr.c | 33 -
 3 files changed, 37 insertions(+), 33 deletions(-)

diff --git a/hw/9pfs/9p-util.c b/hw/9pfs/9p-util.c
index f709c27..614b7fc 100644
--- a/hw/9pfs/9p-util.c
+++ b/hw/9pfs/9p-util.c
@@ -24,3 +24,36 @@ ssize_t fgetxattrat_nofollow(int dirfd, const char 
*filename, const char *name,
 g_free(proc_path);
 return ret;
 }
+
+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..79ed6b2 100644
--- a/hw/9pfs/9p-util.h
+++ b/hw/9pfs/9p-util.h
@@ -60,5 +60,9 @@ ssize_t fgetxattrat_nofollow(int dirfd, const char *path, 
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

[Qemu-devel] [PATCH v2 05/20] 9p: Properly set errp in fstatfs error path

[Keno Fischer]

In the review of

9p: Avoid warning if FS_IOC_GETVERSION is not defined

Grep Kurz noted this error path was failing to set errp.
Fix that.

Signed-off-by: Keno Fischer 
---

Changes since v1: New patch

 hw/9pfs/9p-local.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index adc169a..576c8e3 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1420,6 +1420,8 @@ static int local_init(FsContext *ctx, Error **errp)
  */
 if (fstatfs(data->mountfd, ) < 0) {
 close_preserve_errno(data->mountfd);
+error_setg_errno(errp, errno,
+"failed to stat file system at '%s'", ctx->fs_root);
 goto err;
 }
 switch (stbuf.f_type) {
-- 
2.8.1

[Qemu-devel] [PATCH v2 06/20] 9p: Avoid warning if FS_IOC_GETVERSION is not defined

[Keno Fischer]

Both `stbuf` and `local_ioc_getversion` where unused when
FS_IOC_GETVERSION was not defined, causing a compiler warning.

Reorgnaize the code to avoid this warning.

Signed-off-by: Keno Fischer 
---

Changes since v1:
 * As request in review, logic is factored into a
   local_ioc_getversion_init function.

 hw/9pfs/9p-local.c | 43 +--
 1 file changed, 25 insertions(+), 18 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 576c8e3..6222891 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,32 +1397,19 @@ static int local_ioc_getversion(FsContext *ctx, 
V9fsPath *path,
 err = ioctl(fid_open.fd, FS_IOC_GETVERSION, st_gen);
 local_close(ctx, _open);
 return err;
-#else
-errno = ENOTTY;
-return -1;
-#endif
 }
+#endif
 
-static int local_init(FsContext *ctx, Error **errp)
+static int local_ioc_getversion_init(FsContext *ctx, LocalData *data)
 {
+#ifdef FS_IOC_GETVERSION
 struct statfs stbuf;
-LocalData *data = g_malloc(sizeof(*data));
 
-data->mountfd = open(ctx->fs_root, O_DIRECTORY | O_RDONLY);
-if (data->mountfd == -1) {
-error_setg_errno(errp, errno, "failed to open '%s'", ctx->fs_root);
-goto err;
-}
-
-#ifdef FS_IOC_GETVERSION
 /*
  * use ioc_getversion only if the ioctl is definied
  */
 if (fstatfs(data->mountfd, ) < 0) {
-close_preserve_errno(data->mountfd);
-error_setg_errno(errp, errno,
-"failed to stat file system at '%s'", ctx->fs_root);
-goto err;
+return -1;
 }
 switch (stbuf.f_type) {
 case EXT2_SUPER_MAGIC:
@@ -1433,6 +1420,26 @@ static int local_init(FsContext *ctx, Error **errp)
 break;
 }
 #endif
+return 0;
+}
+
+static int local_init(FsContext *ctx, Error **errp)
+{
+LocalData *data = g_malloc(sizeof(*data));
+
+data->mountfd = open(ctx->fs_root, O_DIRECTORY | O_RDONLY);
+if (data->mountfd == -1) {
+error_setg_errno(errp, errno, "failed to open '%s'", ctx->fs_root);
+goto err;
+}
+
+if (local_ioc_getversion_init(ctx, data) < 0) {
+close_preserve_errno(data->mountfd);
+error_setg_errno(errp, errno,
+"failed initialize ioc_getversion for file system at '%s'",
+ctx->fs_root);
+goto err;
+}
 
 if (ctx->export_flags & V9FS_SM_PASSTHROUGH) {
 ctx->xops = passthrough_xattr_ops;
-- 
2.8.1

[Qemu-devel] [PATCH v2 11/20] 9p: darwin: Handle struct dirent differences

[Keno Fischer]

On darwin d_seekoff exists, but is optional and does not seem to
be commonly used by file systems. Use `telldir` instead to obtain
the seek offset.

Signed-off-by: Keno Fischer 
---

Changes since v1:
 * Drop setting d_seekoff in synth_direntry
 * Factor telldir vs d_off logic into v9fs_dent_telldir
 * Error path for telldir failure

 hw/9pfs/9p-synth.c |  2 ++
 hw/9pfs/9p.c   | 36 
 2 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
index eb68b42..a312f8c 100644
--- a/hw/9pfs/9p-synth.c
+++ b/hw/9pfs/9p-synth.c
@@ -221,7 +221,9 @@ static void synth_direntry(V9fsSynthNode *node,
 {
 strcpy(entry->d_name, node->name);
 entry->d_ino = node->attr->inode;
+#ifndef CONFIG_DARWIN
 entry->d_off = off + 1;
+#endif
 }
 
 static struct dirent *synth_get_dentry(V9fsSynthNode *dir,
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 212f569..9751246 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1738,6 +1738,25 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, 
V9fsFidState *fidp,
 return offset;
 }
 
+/**
+ * Get the seek offset of a dirent. If not available from the structure itself,
+ * obtain it by calling telldir.
+ */
+static int v9fs_dent_telldir(V9fsPDU *pdu, V9fsFidState *fidp,
+ struct dirent *dent)
+{
+#ifdef CONFIG_DARWIN
+/*
+ * Darwin has d_seekoff, which appears to function similarly to d_off.
+ * However, it does not appear to be supported on all file systems,
+ * so use telldir for correctness.
+ */
+return v9fs_co_telldir(pdu, fidp);
+#else
+return dent->d_off;
+#endif
+}
+
 static int coroutine_fn v9fs_do_readdir_with_stat(V9fsPDU *pdu,
   V9fsFidState *fidp,
   uint32_t max_count)
@@ -1801,7 +1820,11 @@ static int coroutine_fn 
v9fs_do_readdir_with_stat(V9fsPDU *pdu,
 count += len;
 v9fs_stat_free();
 v9fs_path_free();
-saved_dir_pos = dent->d_off;
+saved_dir_pos = v9fs_dent_telldir(pdu, fidp, dent);
+if (saved_dir_pos < 0) {
+err = saved_dir_pos;
+break;
+}
 }
 
 v9fs_readdir_unlock(>fs.dir);
@@ -1915,7 +1938,7 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu, 
V9fsFidState *fidp,
 V9fsString name;
 int len, err = 0;
 int32_t count = 0;
-off_t saved_dir_pos;
+off_t saved_dir_pos, off;
 struct dirent *dent;
 
 /* save the directory position */
@@ -1951,10 +1974,15 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu, 
V9fsFidState *fidp,
 /* Fill the other fields with dummy values */
 qid.type = 0;
 qid.version = 0;
+off = v9fs_dent_telldir(pdu, fidp, dent);
+if (off < 0) {
+err = off;
+break;
+}
 
 /* 11 = 7 + 4 (7 = start offset, 4 = space for storing count) */
 len = pdu_marshal(pdu, 11 + count, "Qqbs",
-  , dent->d_off,
+  , off,
   dent->d_type, );
 
 v9fs_readdir_unlock(>fs.dir);
@@ -1966,7 +1994,7 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu, 
V9fsFidState *fidp,
 }
 count += len;
 v9fs_string_free();
-saved_dir_pos = dent->d_off;
+saved_dir_pos = off;
 }
 
 v9fs_readdir_unlock(>fs.dir);
-- 
2.8.1

[Qemu-devel] [PATCH v2 02/20] 9p: proxy: Fix size passed to `connect`

[Keno Fischer]

The size to pass to the `connect` call is the size of the entire
`struct sockaddr_un`. Passing anything shorter than this causes errors
on darwin.

Signed-off-by: Keno Fischer 
---

Changes since v1: New patch

 hw/9pfs/9p-proxy.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c
index e2e0329..47a94e0 100644
--- a/hw/9pfs/9p-proxy.c
+++ b/hw/9pfs/9p-proxy.c
@@ -1088,7 +1088,7 @@ static int proxy_ioc_getversion(FsContext *fs_ctx, 
V9fsPath *path,
 
 static int connect_namedsocket(const char *path, Error **errp)
 {
-int sockfd, size;
+int sockfd;
 struct sockaddr_un helper;
 
 if (strlen(path) >= sizeof(helper.sun_path)) {
@@ -1102,8 +1102,7 @@ static int connect_namedsocket(const char *path, Error 
**errp)
 }
 strcpy(helper.sun_path, path);
 helper.sun_family = AF_UNIX;
-size = strlen(helper.sun_path) + sizeof(helper.sun_family);
-if (connect(sockfd, (struct sockaddr *), size) < 0) {
+if (connect(sockfd, (struct sockaddr *), sizeof(helper)) < 0) {
 error_setg_errno(errp, errno, "failed to connect to '%s'", path);
 close(sockfd);
 return -1;
-- 
2.8.1

[Qemu-devel] [PATCH v2 04/20] 9p: linux: Fix a couple Linux assumptions

[Keno Fischer]

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

Signed-off-by: Keno Fischer 
---

Changes since v1:
 * New qemu/statfs.h header to factor out the header selection
   to a common place. I did not write a configure check,
   since the rest of 9p only supports linux/darwin anyway,
   so there didn't seem to be much point, but I can write
   one if requested.
 * Now also covers fsdev/virtfs-proxy-helper.c

 fsdev/file-op-9p.h  |  2 +-
 fsdev/virtfs-proxy-helper.c |  4 +++-
 hw/9pfs/9p-local.c  |  2 ++
 include/qemu/statfs.h   | 19 +++
 include/qemu/xattr.h|  4 +++-
 5 files changed, 28 insertions(+), 3 deletions(-)
 create mode 100644 include/qemu/statfs.h

diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
index 3fa062b..111f804 100644
--- a/fsdev/file-op-9p.h
+++ b/fsdev/file-op-9p.h
@@ -16,7 +16,7 @@
 
 #include 
 #include 
-#include 
+#include "qemu/statfs.h"
 #include "qemu-fsdev-throttle.h"
 
 #define SM_LOCAL_MODE_BITS0600
diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
index 6f132c5..94fb069 100644
--- a/fsdev/virtfs-proxy-helper.c
+++ b/fsdev/virtfs-proxy-helper.c
@@ -13,17 +13,19 @@
 #include 
 #include 
 #include 
+#ifdef CONFIG_LINUX
 #include 
 #include 
-#include 
 #include 
 #include 
 #ifdef CONFIG_LINUX_MAGIC_H
 #include 
 #endif
+#endif
 #include "qemu-common.h"
 #include "qemu/sockets.h"
 #include "qemu/xattr.h"
+#include "qemu/statfs.h"
 #include "9p-iov-marshal.h"
 #include "hw/9pfs/9p-proxy.h"
 #include "fsdev/9p-iov-marshal.h"
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index bcf2798..adc169a 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 
+#ifdef CONFIG_LINUX
 #include 
 #ifdef CONFIG_LINUX_MAGIC_H
 #include 
 #endif
+#endif
 #include 
 
 #ifndef XFS_SUPER_MAGIC
diff --git a/include/qemu/statfs.h b/include/qemu/statfs.h
new file mode 100644
index 000..dde289f
--- /dev/null
+++ b/include/qemu/statfs.h
@@ -0,0 +1,19 @@
+/*
+ * Host statfs header abstraction
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2, or any
+ * later version.  See the COPYING file in the top-level directory.
+ *
+ */
+#ifndef QEMU_STATFS_H
+#define QEMU_STATFS_H
+
+#ifdef CONFIG_LINUX
+# include 
+#endif
+#ifdef CONFIG_DARWIN
+# include 
+# include 
+#endif
+
+#endif
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 
 #else
-#  define ENOATTR ENODATA
+#  if !defined(ENOATTR)
+#define ENOATTR ENODATA
+#  endif
 #  include 
 #endif
 
-- 
2.8.1

[Qemu-devel] [PATCH v2 03/20] 9p: xattr: Fix crash due to free of uninitialized value

[Keno Fischer]

If the size returned from llistxattr is 0, we skipped the malloc
call, leaving xattr.value uninitialized. However, this value is
later passed to `g_free` without any further checks, causing an
error. Fix that by always calling g_malloc unconditionally. If
`size` is 0, it will return a pointer that is safe to pass to g_free,
likely NULL.

Signed-off-by: Keno Fischer 
---

Changes since v1: New patch

 hw/9pfs/9p.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index d74302d..b80db65 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -3256,8 +3256,8 @@ static void coroutine_fn v9fs_xattrwalk(void *opaque)
 xattr_fidp->fs.xattr.len = size;
 xattr_fidp->fid_type = P9_FID_XATTR;
 xattr_fidp->fs.xattr.xattrwalk_fid = true;
+xattr_fidp->fs.xattr.value = g_malloc0(size);
 if (size) {
-xattr_fidp->fs.xattr.value = g_malloc0(size);
 err = v9fs_co_llistxattr(pdu, _fidp->path,
  xattr_fidp->fs.xattr.value,
  xattr_fidp->fs.xattr.len);
-- 
2.8.1

[Qemu-devel] [PATCH v2 01/20] cutils: Provide strchrnul

[Keno Fischer]

strchrnul is a GNU extension and thus unavailable on a number of targets.
In the review for a commit removing strchrnul from 9p, I was asked to
create a qemu_strchrnul helper to factor out this functionality.
Do so, and use it in a number of other places in the code base that inlined
the replacement pattern in a place where strchrnul could be used.

Signed-off-by: Keno Fischer 
---

Changes since v1: New patch

 hw/9pfs/9p-local.c|  2 +-
 include/qemu/cutils.h |  1 +
 monitor.c |  8 ++--
 util/cutils.c | 13 +
 util/qemu-option.c|  6 +-
 util/uri.c|  6 ++
 6 files changed, 20 insertions(+), 16 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index b37b1db..bcf2798 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -65,7 +65,7 @@ int local_open_nofollow(FsContext *fs_ctx, const char *path, 
int flags,
 assert(*path != '/');
 
 head = g_strdup(path);
-c = strchrnul(path, '/');
+c = qemu_strchrnul(path, '/');
 if (*c) {
 /* Intermediate path element */
 head[c - path] = 0;
diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
index a663340..bc40c30 100644
--- a/include/qemu/cutils.h
+++ b/include/qemu/cutils.h
@@ -122,6 +122,7 @@ int qemu_strnlen(const char *s, int max_len);
  * Returns: the pointer originally in @input.
  */
 char *qemu_strsep(char **input, const char *delim);
+const char *qemu_strchrnul(const char *s, int c);
 time_t mktimegm(struct tm *tm);
 int qemu_fdatasync(int fd);
 int fcntl_setfl(int fd, int flag);
diff --git a/monitor.c b/monitor.c
index 922cfc0..e1f01c4 100644
--- a/monitor.c
+++ b/monitor.c
@@ -798,9 +798,7 @@ static int compare_cmd(const char *name, const char *list)
 p = list;
 for(;;) {
 pstart = p;
-p = strchr(p, '|');
-if (!p)
-p = pstart + strlen(pstart);
+p = qemu_strchrnul(p, '|');
 if ((p - pstart) == len && !memcmp(pstart, name, len))
 return 1;
 if (*p == '\0')
@@ -3401,9 +3399,7 @@ static void cmd_completion(Monitor *mon, const char 
*name, const char *list)
 p = list;
 for(;;) {
 pstart = p;
-p = strchr(p, '|');
-if (!p)
-p = pstart + strlen(pstart);
+p = qemu_strchrnul(p, '|');
 len = p - pstart;
 if (len > sizeof(cmd) - 2)
 len = sizeof(cmd) - 2;
diff --git a/util/cutils.c b/util/cutils.c
index 0de69e6..6e078b0 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -545,6 +545,19 @@ int qemu_strtou64(const char *nptr, const char **endptr, 
int base,
 }
 
 /**
+ * Searches for the first occurrence of 'c' in 's', and returns a pointer
+ * to the trailing null byte if none was found.
+ */
+const char *qemu_strchrnul(const char *s, int c)
+{
+const char *e = strchr(s, c);
+if (!e) {
+e = s + strlen(s);
+}
+return e;
+}
+
+/**
  * parse_uint:
  *
  * @s: String to parse
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 58d1c23..54eca12 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -77,11 +77,7 @@ const char *get_opt_value(const char *p, char **value)
 
 *value = NULL;
 while (1) {
-offset = strchr(p, ',');
-if (!offset) {
-offset = p + strlen(p);
-}
-
+offset = qemu_strchrnul(p, ',');
 length = offset - p;
 if (*offset != '\0' && *(offset + 1) == ',') {
 length++;
diff --git a/util/uri.c b/util/uri.c
index 8624a7a..8bdef84 100644
--- a/util/uri.c
+++ b/util/uri.c
@@ -52,6 +52,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/cutils.h"
 
 #include "qemu/uri.h"
 
@@ -2266,10 +2267,7 @@ struct QueryParams *query_params_parse(const char *query)
 /* Find the next separator, or end of the string. */
 end = strchr(query, '&');
 if (!end) {
-end = strchr(query, ';');
-}
-if (!end) {
-end = query + strlen(query);
+end = qemu_strchrnul(query, ';');
 }
 
 /* Find the first '=' character between here and end. */
-- 
2.8.1

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

[Keno Fischer]

This is v2 of my patch series to provide 9p server
support for Darwin.

The patches in this series address review from v1,
now support building the virtfs proxy, as well
as fix bugs found since v1.

Keno Fischer (20):
  cutils: Provide strchrnul
  9p: proxy: Fix size passed to `connect`
  9p: xattr: Fix crash due to free of uninitialized value
  9p: linux: Fix a couple Linux assumptions
  9p: Properly set errp in fstatfs error path
  9p: Avoid warning if FS_IOC_GETVERSION is not defined
  9p: Move a couple xattr functions to 9p-util
  9p: Rename 9p-util -> 9p-util-linux
  9p: Properly error check and translate flags in unlinkat
  9p: darwin: Handle struct stat(fs) differences
  9p: darwin: Handle struct dirent differences
  9p: darwin: Explicitly cast comparisons of mode_t with -1
  9p: darwin: Ignore O_{NOATIME, DIRECT}
  9p: darwin: Provide a compatibility definition for XATTR_SIZE_MAX
  9p: darwin: *xattr_nofollow implementations
  9p: darwin: Compatibility for f/l*xattr
  9p: darwin: Provide a fallback implementation for utimensat
  9p: darwin: Implement compatibility for mknodat
  9p: darwin: virtfs-proxy: Implement setuid code for darwin
  9p: darwin: configure: Allow VirtFS on Darwin

 Makefile|   6 ++
 Makefile.objs   |   1 +
 configure   |  22 +++--
 fsdev/file-op-9p.h  |   2 +-
 fsdev/virtfs-proxy-helper.c | 230 
 hw/9pfs/9p-local.c  |  68 -
 hw/9pfs/9p-proxy.c  |  22 +++--
 hw/9pfs/9p-synth.c  |   4 +
 hw/9pfs/9p-util-darwin.c| 185 +++
 hw/9pfs/9p-util-linux.c |  70 ++
 hw/9pfs/9p-util.c   |  26 -
 hw/9pfs/9p-util.h   |  31 ++
 hw/9pfs/9p-xattr.c  |  33 ---
 hw/9pfs/9p.c|  86 +++--
 hw/9pfs/Makefile.objs   |   4 +-
 include/qemu/cutils.h   |   1 +
 include/qemu/statfs.h   |  19 
 include/qemu/xattr.h|   4 +-
 monitor.c   |   8 +-
 util/cutils.c   |  13 +++
 util/qemu-option.c  |   6 +-
 util/uri.c  |   6 +-
 22 files changed, 636 insertions(+), 211 deletions(-)
 create mode 100644 hw/9pfs/9p-util-darwin.c
 create mode 100644 hw/9pfs/9p-util-linux.c
 delete mode 100644 hw/9pfs/9p-util.c
 create mode 100644 include/qemu/statfs.h

-- 
2.8.1

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  wrote:
> On Thu, May 31, 2018 at 6:56 PM, Keno Fischer  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.

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  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]

>> 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 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  wrote:
> On Thu, 31 May 2018 12:27:35 -0400
> Keno Fischer  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 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 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.

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 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.

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 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 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 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 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 
>>>  #include 
>>> +#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 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 v2] 9pfs: Correctly handle cancelled requests

[Keno Fischer]

 # Background

I was investigating spurious non-deterministic EINTR returns from
various 9p file system operations in a Linux guest served from the
qemu 9p server.

 ## EINTR, ERESTARTSYS and the linux kernel

When a signal arrives that the Linux kernel needs to deliver to user-space
while a given thread is blocked (in the 9p case waiting for a reply to its
request in 9p_client_rpc -> wait_event_interruptible), it asks whatever
driver is currently running to abort its current operation (in the 9p case
causing the submission of a TFLUSH message) and return to user space.
In these situations, the error message reported is generally ERESTARTSYS.
If the userspace processes specified SA_RESTART, this means that the
system call will get restarted upon completion of the signal handler
delivery (assuming the signal handler doesn't modify the process state
in complicated ways not relevant here). If SA_RESTART is not specified,
ERESTARTSYS gets translated to EINTR and user space is expected to handle
the restart itself.

 ## The 9p TFLISH command

The 9p TFLUSH commands requests that the server abort an ongoing operation.
The man page [1] specifies:

```
If it recognizes oldtag as the tag of a pending transaction, it should abort any
pending response and discard that tag.
[...]
When the client sends a Tflush, it must wait to receive the corresponding Rflush
before reusing oldtag for subsequent messages. If a response to the flushed 
request
is received before the Rflush, the client must honor the response as if it had 
not
been flushed, since the completed request may signify a state change in the 
server
```

In particular, this means that the server must not send a reply with the orignal
tag in response to the cancellation request, because the client is obligated
to interpret such a reply as a coincidental reply to the original request.

 # The bug

When qemu receives a TFlush request, it sets the `cancelled` flag on the 
relevant
pdu. This flag is periodically checked, e.g. in `v9fs_co_name_to_path`, and if
set, the operation is aborted and the error is set to EINTR. However, the server
then violates the spec, by returning to the client an Rerror response, rather
than discarding the message entirely. As a result, the client is required
to assume that said Rerror response is a result of the original request, not
a result of the cancellation and thus passes the EINTR error back to user space.
This is not the worst thing it could do, however as discussed above, the correct
error code would have been ERESTARTSYS, such that user space programs with
SA_RESTART set get correctly restarted upon completion of the signal handler.
Instead, such programs get spurious EINTR results that they were not expecting
to handle.

It should be noted that there are plenty of user space programs that do not
set SA_RESTART and do not correctly handle EINTR either. However, that is then
a userspace bug. It should also be noted that this bug has been mitigated by
a recent commit to the Linux kernel [2], which essentially prevents the kernel
from sending Tflush requests unless the process is about to die (in which case
the process likely doesn't care about the response). Nevertheless, for older
kernels and to comply with the spec, I believe this change is beneficial.

 # Implementation

The fix is fairly simple, just skipping notification of a reply if
the pdu was previously cancelled. We do however, also notify the transport
layer that we're doing this, so it can clean up any resources it may be
holding. I also added a new trace event to distinguish
operations that caused an error reply from those that were cancelled.

One complication is that we only omit sending the message on EINTR errors in
order to avoid confusing the rest of the code (which may assume that a
client knows about a fid if it sucessfully passed it off to pud_complete
without checking for cancellation status). This does mean that if the server
acts upon the cancellation flag, it always needs to set err to EINTR. I believe
this is true of the current code.

[1] https://9fans.github.io/plan9port/man/man9/flush.html
[2] 
https://github.com/torvalds/linux/commit/9523feac272ccad2ad8186ba4fcc89103754de52

Signed-off-by: Keno Fischer <k...@juliacomputing.com>
---

Changes from v1:
 - In reponse to review by Greg Kurz, add a new transport
   layer operation to discard the buffer. I also attempted
   an implementation for xen, but I have done no verification
   on that beyond making sure it compiles, since I don't know
   how to use xen. Please review closely.

 hw/9pfs/9p.c   | 18 ++
 hw/9pfs/9p.h   |  1 +
 hw/9pfs/trace-events   |  1 +
 hw/9pfs/virtio-9p-device.c | 14 ++
 hw/9pfs/xen-9p-backend.c   | 11 +++
 5 files changed, 45 insertions(+)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 710cd91..daa8519 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -648,6 +648,23 @@ static void coroutine_fn pdu_complet

[Qemu-devel] [PATCH] 9pfs: Correctly handle cancelled requests

[Keno Fischer]

 # Background

I was investigating spurious, non-deterministic EINTR returns from
various 9p file system operations in a Linux guest served from the
qemu 9p server.

 ## EINTR, ERESTARTSYS and the linux kernel

When a signal arrives that the Linux kernel needs to deliver to user-space
while a given thread is blocked (in the 9p case waiting for a reply to its
request in 9p_client_rpc -> wait_event_interruptible), it asks whatever
driver is currently running to abort its current operation (in the 9p case
causing the submission of a TFLUSH message) and return to user space.
In these situations, the error message reported is generally ERESTARTSYS.
If the userspace processes specified SA_RESTART, this means that the
system call will get restarted upon completion of the signal handler
delivery (assuming the signal handler doesn't modify the process state
in complicated ways not relevant here). If SA_RESTART is not specified,
ERESTARTSYS gets translated to EINTR and user space is expected to handle
the restart itself.

 ## The 9p TFLISH command

The 9p TFLUSH commands requests that the server abort an ongoing operation.
The man page [1] specifies:

```
If it recognizes oldtag as the tag of a pending transaction, it should abort any
pending response and discard that tag.
[...]
When the client sends a Tflush, it must wait to receive the corresponding Rflush
before reusing oldtag for subsequent messages. If a response to the flushed 
request
is received before the Rflush, the client must honor the response as if it had 
not
been flushed, since the completed request may signify a state change in the 
server
```

In particular, this means that the server must not send a reply with the orignal
tag in response to the cancellation request, because the client is obligated
to interpret such a reply as a coincidental reply to the original request.

 # The bug

When qemu receives a TFlush request, it sets the `cancelled` flag on the 
relevant
pdu. This flag is periodically checked, e.g. in `v9fs_co_name_to_path`, and if
set, the operation is aborted and the error is set to EINTR. However, the server
then violates the spec, by returning to the client an Rerror response, rather
than discarding the message entirely. As a result, the client is required
to assume that said Rerror response is a result of the original request, not
a result of the cancellation and thus passes the EINTR error back to user space.
This is not the worst thing it could do, however as discussed above, the correct
error code would have been ERESTARTSYS, such that user space programs with
SA_RESTART set get correctly restarted upon completion of the signal handler.
Instead, such programs get spurious EINTR results that they were not expecting
to handle.

It should be noted that there are plenty of user space programs that do not
set SA_RESTART and do not correctly handle EINTR either. However, that is then
a userspace bug. It should also be noted that this bug has been mitigated by
a recent commit to the Linux kernel [2], which essentially prevents the kernel
from sending Tflush requests unless the process is about to die (in which case
the process likely doesn't care about the response). Nevertheless, for older
kernels and to comply with the spec, I believe this change is beneficial.

 # Implementation

The fix is fairly simple, just skipping notification of a reply if
the pdu was previously cancelled. I also added a new trace event to distinguish
operations that caused an error reply from those that were cancelled.

One complication is that we only omit sending the message on EINTR errors in
order to avoid confusing the rest of the code (which may assume that a
client knows about a fid if it sucessfully passed it off to pud_complete
without checking for cancellation status). This does mean that if the server
acts upon the cancellation flag, it always needs to set err to EINTR. I believe
this is true of the current code.

[1] https://9fans.github.io/plan9port/man/man9/flush.html
[2] 
https://github.com/torvalds/linux/commit/9523feac272ccad2ad8186ba4fcc89103754de52

Signed-off-by: Keno Fischer <k...@juliacomputing.com>
---
 hw/9pfs/9p.c | 17 +
 hw/9pfs/trace-events |  1 +
 2 files changed, 18 insertions(+)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 710cd91..46f406b 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -648,6 +648,22 @@ static void coroutine_fn pdu_complete(V9fsPDU *pdu, 
ssize_t len)
 V9fsState *s = pdu->s;
 int ret;
 
+/*
+ * The 9p spec requires that successfully cancelled pdus receive no reply.
+ * Sending a reply would confuse clients because they would
+ * assume that any EINTR is the actual result of the operation,
+ * rather than a consequence of the cancellation. However, if
+ * the operation completed (succesfully or with an error other
+ * than caused be cancellation), we do send out that reply, both
+ * for efficiency and to avoid confusing the rest