[PULL 5/8] fsdev/p9array.h: check scalar type in P9ARRAY_NEW()

[PULL 5/8] fsdev/p9array.h: check scalar type in P9ARRAY_NEW()

[Christian Schoenebeck]

Make sure at compile time that the scalar type of the array
requested to be created via P9ARRAY_NEW() matches the scalar
type of the passed auto reference variable (unique pointer).

Suggested-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
Message-Id: <c1965e2a096835dc9e1d4d659dfb15d96755cbe0.1633097129.git.qemu_oss@crudebyte.com>
---
 fsdev/p9array.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fsdev/p9array.h b/fsdev/p9array.h
index fff946a3d7..6aa25327ca 100644
--- a/fsdev/p9array.h
+++ b/fsdev/p9array.h
@@ -27,6 +27,8 @@
 #ifndef QEMU_P9ARRAY_H
 #define QEMU_P9ARRAY_H
 
+#include "qemu/compiler.h"
+
 /**
  * P9Array provides a mechanism to access arrays in common C-style (e.g. by
  * square bracket [] operator) in conjunction with reference variables that
@@ -149,6 +151,10 @@
  * @param len - amount of array elements to be allocated immediately
  */
 #define P9ARRAY_NEW(scalar_type, auto_var, len) \
+    QEMU_BUILD_BUG_MSG( \
+        !__builtin_types_compatible_p(scalar_type, typeof(*auto_var)), \
+        "P9Array scalar type mismatch" \
+    ); \
     p9array_new_##scalar_type((&auto_var), len)
 
 #endif /* QEMU_P9ARRAY_H */
-- 
2.20.1

[PULL 0/8] 9p queue 2021-10-27

[Christian Schoenebeck]

The following changes since commit 931ce30859176f0f7daac6bac255dae5eb21284e:

  Merge remote-tracking branch 'remotes/dagrh/tags/pull-virtiofs-20211026' into staging (2021-10-26 07:38:41 -0700)

are available in the Git repository at:

  https://github.com/cschoenebeck/qemu.git tags/pull-9p-20211027

for you to fetch changes up to 7e985780aaab93d2c5be9b62d8d386568dfb071e:

  9pfs: use P9Array in v9fs_walk() (2021-10-27 14:45:22 +0200)

----------------------------------------------------------------
9pfs: performance fix and cleanup

* First patch fixes suboptimal I/O performance on guest due to previously
  incorrect block size being transmitted to 9p client.

* Subsequent patches are cleanup ones intended to reduce code complexity.

----------------------------------------------------------------
Christian Schoenebeck (8):
      9pfs: fix wrong I/O block size in Rgetattr
      9pfs: deduplicate iounit code
      9pfs: simplify blksize_to_iounit()
      9pfs: introduce P9Array
      fsdev/p9array.h: check scalar type in P9ARRAY_NEW()
      9pfs: make V9fsString usable via P9Array API
      9pfs: make V9fsPath usable via P9Array API
      9pfs: use P9Array in v9fs_walk()

 fsdev/9p-marshal.c |   2 +
 fsdev/9p-marshal.h |   3 +
 fsdev/file-op-9p.h |   2 +
 fsdev/p9array.h    | 160 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/9pfs/9p.c       |  70 +++++++++++++----------
 5 files changed, 208 insertions(+), 29 deletions(-)
 create mode 100644 fsdev/p9array.h

[PULL 2/8] 9pfs: deduplicate iounit code

[Christian Schoenebeck]

Remove redundant code that translates host fileystem's block
size into 9p client (guest side) block size.

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
Message-Id: <129bb71d5119e61d335f1e3107e472e4beea223a.1632758315.git.qemu_oss@crudebyte.com>
---
 hw/9pfs/9p.c | 41 ++++++++++++++++++++---------------------
 1 file changed, 20 insertions(+), 21 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 708b030474..5c57344667 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1262,18 +1262,26 @@ static int coroutine_fn stat_to_v9stat(V9fsPDU *pdu, V9fsPath *path,
 #define P9_STATS_ALL           0x00003fffULL /* Mask for All fields above */
 
 
-static int32_t stat_to_iounit(const V9fsPDU *pdu, const struct stat *stbuf)
+/**
+ * Convert host filesystem's block size into an appropriate block size for
+ * 9p client (guest OS side). The value returned suggests an "optimum" block
+ * size for 9p I/O, i.e. to maximize performance.
+ *
+ * @pdu: 9p client request
+ * @blksize: host filesystem's block size
+ */
+static int32_t blksize_to_iounit(const V9fsPDU *pdu, int32_t blksize)
 {
     int32_t iounit = 0;
     V9fsState *s = pdu->s;
 
     /*
-     * iounit should be multiples of st_blksize (host filesystem block size)
+     * iounit should be multiples of blksize (host filesystem block size)
      * as well as less than (client msize - P9_IOHDRSZ)
      */
-    if (stbuf->st_blksize) {
-        iounit = stbuf->st_blksize;
-        iounit *= (s->msize - P9_IOHDRSZ) / stbuf->st_blksize;
+    if (blksize) {
+        iounit = blksize;
+        iounit *= (s->msize - P9_IOHDRSZ) / blksize;
     }
     if (!iounit) {
         iounit = s->msize - P9_IOHDRSZ;
@@ -1281,6 +1289,11 @@ static int32_t stat_to_iounit(const V9fsPDU *pdu, const struct stat *stbuf)
     return iounit;
 }
 
+static int32_t stat_to_iounit(const V9fsPDU *pdu, const struct stat *stbuf)
+{
+    return blksize_to_iounit(pdu, stbuf->st_blksize);
+}
+
 static int stat_to_v9stat_dotl(V9fsPDU *pdu, const struct stat *stbuf,
                                 V9fsStatDotl *v9lstat)
 {
@@ -1899,23 +1912,9 @@ out_nofid:
 static int32_t coroutine_fn get_iounit(V9fsPDU *pdu, V9fsPath *path)
 {
     struct statfs stbuf;
-    int32_t iounit = 0;
-    V9fsState *s = pdu->s;
+    int err = v9fs_co_statfs(pdu, path, &stbuf);
 
-    /*
-     * iounit should be multiples of f_bsize (host filesystem block size
-     * and as well as less than (client msize - P9_IOHDRSZ))
-     */
-    if (!v9fs_co_statfs(pdu, path, &stbuf)) {
-        if (stbuf.f_bsize) {
-            iounit = stbuf.f_bsize;
-            iounit *= (s->msize - P9_IOHDRSZ) / stbuf.f_bsize;
-        }
-    }
-    if (!iounit) {
-        iounit = s->msize - P9_IOHDRSZ;
-    }
-    return iounit;
+    return blksize_to_iounit(pdu, (err >= 0) ? stbuf.f_bsize : 0);
 }
 
 static void coroutine_fn v9fs_open(void *opaque)
-- 
2.20.1

[PULL 3/8] 9pfs: simplify blksize_to_iounit()

[Christian Schoenebeck]

Use QEMU_ALIGN_DOWN() macro to reduce code and to make it
more human readable.

Suggested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-Id: <b84eb324d2ebdcc6f9c442c97b5b4d01eecb4f43.1632758315.git.qemu_oss@crudebyte.com>
---
 hw/9pfs/9p.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 5c57344667..e874899ef5 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1280,8 +1280,7 @@ static int32_t blksize_to_iounit(const V9fsPDU *pdu, int32_t blksize)
      * as well as less than (client msize - P9_IOHDRSZ)
      */
     if (blksize) {
-        iounit = blksize;
-        iounit *= (s->msize - P9_IOHDRSZ) / blksize;
+        iounit = QEMU_ALIGN_DOWN(s->msize - P9_IOHDRSZ, blksize);
     }
     if (!iounit) {
         iounit = s->msize - P9_IOHDRSZ;
-- 
2.20.1

[PULL 4/8] 9pfs: introduce P9Array

[Christian Schoenebeck]

Implements deep auto free of arrays while retaining common C-style
squared bracket access. Main purpose of this API is to get rid of
error prone individual array deallocation pathes in user code, i.e.
turning something like this:

  void doSomething(size_t n) {
      Foo *foos = malloc(n * sizeof(Foo));
      for (...) {
          foos[i].s = malloc(...);
          if (...) {
              goto out;
          }
      }
  out:
      if (...) {
          for (...) {
              /* deep deallocation */
              free(foos[i].s);
          }
          /* array deallocation */
          free(foos);
      }
  }

into something more simple and safer like:

  void doSomething(size_t n) {
      P9ARRAY_REF(Foo) foos = NULL;
      P9ARRAY_NEW(Foo, foos, n);
      for (...) {
          foos[i].s = malloc(...);
          if (...) {
              return; /* array auto freed here */
          }
      }
      /* array auto freed here */
  }

Unlike GArray, P9Array does not require special macros, function
calls or struct member dereferencing to access the individual array
elements:

  C-array = P9Array:   vs.  GArray:

  for (...) {           |   for (...) {
      ... = arr[i].m;   |       ... = g_array_index(arr, Foo, i).m;
      arr[i].m = ... ;  |       g_array_index(arr, Foo, i).m = ... ;
  }                     |   }

So existing C-style array code can be retained with only very little
changes; basically limited to replacing array allocation call and of
course removing individual array deallocation pathes.

In this initial version P9Array only supports the concept of unique
pointers, i.e. it does not support reference counting. The array (and
all dynamically allocated memory of individual array elements) is auto
freed once execution leaves the scope of the reference variable (unique
pointer) associated with the array.

Internally a flex array struct is used in combination with macros
spanned over a continuous memory space for both the array's meta data
(private) and the actual C-array user data (public):

  struct P9Array##scalar_type {
    size_t len;            /* private, hidden from user code */
    scalar_type first[];   /* public, directly exposed to user code */
  };

Which has the advantage that the compiler automatically takes care
about correct padding, alignment and overall size for all scalar data
types on all systems and that the user space exposed pointer can
directly be translated back and forth between user space C-array
pointer and internal P9Array struct whenever needed, in a type-safe
manner.

This header file is released under MIT license, to allow this file
being used in other C-projects as well. The common QEMU license
GPL2+ might have construed a conflict for other projects.

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
Message-Id: <a954ef47b5ac26085a16c5c2aec8695374e0424d.1633097129.git.qemu_oss@crudebyte.com>
---
 fsdev/p9array.h | 154 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 154 insertions(+)
 create mode 100644 fsdev/p9array.h

diff --git a/fsdev/p9array.h b/fsdev/p9array.h
new file mode 100644
index 0000000000..fff946a3d7
--- /dev/null
+++ b/fsdev/p9array.h
@@ -0,0 +1,154 @@
+/*
+ * P9Array - deep auto free C-array
+ *
+ * Copyright (c) 2021 Crudebyte
+ *
+ * Authors:
+ *   Christian Schoenebeck <qemu_oss@crudebyte.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+#ifndef QEMU_P9ARRAY_H
+#define QEMU_P9ARRAY_H
+
+/**
+ * P9Array provides a mechanism to access arrays in common C-style (e.g. by
+ * square bracket [] operator) in conjunction with reference variables that
+ * perform deep auto free of the array when leaving the scope of the auto
+ * reference variable. That means not only is the array itself automatically
+ * freed, but also memory dynamically allocated by the individual array
+ * elements.
+ *
+ * Example:
+ *
+ * Consider the following user struct @c Foo which shall be used as scalar
+ * (element) type of an array:
+ * @code
+ * typedef struct Foo {
+ *     int i;
+ *     char *s;
+ * } Foo;
+ * @endcode
+ * and assume it has the following function to free memory allocated by @c Foo
+ * instances:
+ * @code
+ * void free_foo(Foo *foo) {
+ *     free(foo->s);
+ * }
+ * @endcode
+ * Add the following to a shared header file:
+ * @code
+ * P9ARRAY_DECLARE_TYPE(Foo);
+ * @endcode
+ * and the following to a C unit file:
+ * @code
+ * P9ARRAY_DEFINE_TYPE(Foo, free_foo);
+ * @endcode
+ * Finally the array may then be used like this:
+ * @code
+ * void doSomething(size_t n) {
+ *     P9ARRAY_REF(Foo) foos = NULL;
+ *     P9ARRAY_NEW(Foo, foos, n);
+ *     for (size_t i = 0; i < n; ++i) {
+ *         foos[i].i = i;
+ *         foos[i].s = calloc(4096, 1);
+ *         snprintf(foos[i].s, 4096, "foo %d", i);
+ *         if (...) {
+ *             return; // array auto freed here
+ *         }
+ *     }
+ *     // array auto freed here
+ * }
+ * @endcode
+ */
+
+/**
+ * Declares an array type for the passed @a scalar_type.
+ *
+ * This is typically used from a shared header file.
+ *
+ * @param scalar_type - type of the individual array elements
+ */
+#define P9ARRAY_DECLARE_TYPE(scalar_type) \
+    typedef struct P9Array##scalar_type { \
+        size_t len; \
+        scalar_type first[]; \
+    } P9Array##scalar_type; \
+    \
+    void p9array_new_##scalar_type(scalar_type **auto_var, size_t len); \
+    void p9array_auto_free_##scalar_type(scalar_type **auto_var); \
+
+/**
+ * Defines an array type for the passed @a scalar_type and appropriate
+ * @a scalar_cleanup_func.
+ *
+ * This is typically used from a C unit file.
+ *
+ * @param scalar_type - type of the individual array elements
+ * @param scalar_cleanup_func - appropriate function to free memory dynamically
+ *                              allocated by individual array elements before
+ */
+#define P9ARRAY_DEFINE_TYPE(scalar_type, scalar_cleanup_func) \
+    void p9array_new_##scalar_type(scalar_type **auto_var, size_t len) \
+    { \
+        p9array_auto_free_##scalar_type(auto_var); \
+        P9Array##scalar_type *arr = g_malloc0(sizeof(P9Array##scalar_type) + \
+            len * sizeof(scalar_type)); \
+        arr->len = len; \
+        *auto_var = &arr->first[0]; \
+    } \
+    \
+    void p9array_auto_free_##scalar_type(scalar_type **auto_var) \
+    { \
+        scalar_type *first = (*auto_var); \
+        if (!first) { \
+            return; \
+        } \
+        P9Array##scalar_type *arr = (P9Array##scalar_type *) ( \
+            ((char *)first) - offsetof(P9Array##scalar_type, first) \
+        ); \
+        for (size_t i = 0; i < arr->len; ++i) { \
+            scalar_cleanup_func(&arr->first[i]); \
+        } \
+        g_free(arr); \
+    } \
+
+/**
+ * Used to declare a reference variable (unique pointer) for an array. After
+ * leaving the scope of the reference variable, the associated array is
+ * automatically freed.
+ *
+ * @param scalar_type - type of the individual array elements
+ */
+#define P9ARRAY_REF(scalar_type) \
+    __attribute((__cleanup__(p9array_auto_free_##scalar_type))) scalar_type*
+
+/**
+ * Allocates a new array of passed @a scalar_type with @a len number of array
+ * elements and assigns the created array to the reference variable
+ * @a auto_var.
+ *
+ * @param scalar_type - type of the individual array elements
+ * @param auto_var - destination reference variable
+ * @param len - amount of array elements to be allocated immediately
+ */
+#define P9ARRAY_NEW(scalar_type, auto_var, len) \
+    p9array_new_##scalar_type((&auto_var), len)
+
+#endif /* QEMU_P9ARRAY_H */
-- 
2.20.1

[PULL 1/8] 9pfs: fix wrong I/O block size in Rgetattr

[Christian Schoenebeck]

When client sent a 9p Tgetattr request then the wrong I/O block
size value was returned by 9p server; instead of host file
system's I/O block size it should rather return an I/O block
size according to 9p session's 'msize' value, because the value
returned to client should be an "optimum" block size for I/O
(i.e. to maximize performance), it should not reflect the actual
physical block size of the underlying storage media.

The I/O block size of a host filesystem is typically 4k, so the
value returned was far too low for good 9p I/O performance.

This patch adds stat_to_iounit() with a similar approach as the
existing get_iounit() function.

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
Message-Id: <E1mT2Js-0000DW-OH@lizzy.crudebyte.com>
---
 hw/9pfs/9p.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index c857b31321..708b030474 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1262,6 +1262,25 @@ static int coroutine_fn stat_to_v9stat(V9fsPDU *pdu, V9fsPath *path,
 #define P9_STATS_ALL           0x00003fffULL /* Mask for All fields above */
 
 
+static int32_t stat_to_iounit(const V9fsPDU *pdu, const struct stat *stbuf)
+{
+    int32_t iounit = 0;
+    V9fsState *s = pdu->s;
+
+    /*
+     * iounit should be multiples of st_blksize (host filesystem block size)
+     * as well as less than (client msize - P9_IOHDRSZ)
+     */
+    if (stbuf->st_blksize) {
+        iounit = stbuf->st_blksize;
+        iounit *= (s->msize - P9_IOHDRSZ) / stbuf->st_blksize;
+    }
+    if (!iounit) {
+        iounit = s->msize - P9_IOHDRSZ;
+    }
+    return iounit;
+}
+
 static int stat_to_v9stat_dotl(V9fsPDU *pdu, const struct stat *stbuf,
                                 V9fsStatDotl *v9lstat)
 {
@@ -1273,7 +1292,7 @@ static int stat_to_v9stat_dotl(V9fsPDU *pdu, const struct stat *stbuf,
     v9lstat->st_gid = stbuf->st_gid;
     v9lstat->st_rdev = stbuf->st_rdev;
     v9lstat->st_size = stbuf->st_size;
-    v9lstat->st_blksize = stbuf->st_blksize;
+    v9lstat->st_blksize = stat_to_iounit(pdu, stbuf);
     v9lstat->st_blocks = stbuf->st_blocks;
     v9lstat->st_atime_sec = stbuf->st_atime;
     v9lstat->st_atime_nsec = stbuf->st_atim.tv_nsec;
-- 
2.20.1

[PULL 6/8] 9pfs: make V9fsString usable via P9Array API

[Christian Schoenebeck]

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
Message-Id: <ce9f7a0a63585dc27f4545c485109efbec1251da.1633097129.git.qemu_oss@crudebyte.com>
---
 fsdev/9p-marshal.c | 2 ++
 fsdev/9p-marshal.h | 3 +++
 2 files changed, 5 insertions(+)

diff --git a/fsdev/9p-marshal.c b/fsdev/9p-marshal.c
index a01bba6908..51881fe220 100644
--- a/fsdev/9p-marshal.c
+++ b/fsdev/9p-marshal.c
@@ -18,6 +18,8 @@
 
 #include "9p-marshal.h"
 
+P9ARRAY_DEFINE_TYPE(V9fsString, v9fs_string_free);
+
 void v9fs_string_free(V9fsString *str)
 {
     g_free(str->data);
diff --git a/fsdev/9p-marshal.h b/fsdev/9p-marshal.h
index ceaf2f521e..f1abbe151c 100644
--- a/fsdev/9p-marshal.h
+++ b/fsdev/9p-marshal.h
@@ -1,10 +1,13 @@
 #ifndef QEMU_9P_MARSHAL_H
 #define QEMU_9P_MARSHAL_H
 
+#include "p9array.h"
+
 typedef struct V9fsString {
     uint16_t size;
     char *data;
 } V9fsString;
+P9ARRAY_DECLARE_TYPE(V9fsString);
 
 typedef struct V9fsQID {
     uint8_t type;
-- 
2.20.1

[PULL 7/8] 9pfs: make V9fsPath usable via P9Array API

[Christian Schoenebeck]

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
Message-Id: <79a0ddf8375f6c95f0565ef155a1bf1e9387664f.1633097129.git.qemu_oss@crudebyte.com>
---
 fsdev/file-op-9p.h | 2 ++
 hw/9pfs/9p.c       | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
index 42f677cf38..8fd89f0447 100644
--- a/fsdev/file-op-9p.h
+++ b/fsdev/file-op-9p.h
@@ -18,6 +18,7 @@
 #include <utime.h>
 #include <sys/vfs.h>
 #include "qemu-fsdev-throttle.h"
+#include "p9array.h"
 
 #define SM_LOCAL_MODE_BITS    0600
 #define SM_LOCAL_DIR_MODE_BITS    0700
@@ -105,6 +106,7 @@ struct V9fsPath {
     uint16_t size;
     char *data;
 };
+P9ARRAY_DECLARE_TYPE(V9fsPath);
 
 typedef union V9fsFidOpenState V9fsFidOpenState;
 
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index e874899ef5..15bb16f466 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -50,6 +50,8 @@ enum {
     Oappend = 0x80,
 };
 
+P9ARRAY_DEFINE_TYPE(V9fsPath, v9fs_path_free);
+
 static ssize_t pdu_marshal(V9fsPDU *pdu, size_t offset, const char *fmt, ...)
 {
     ssize_t ret;
-- 
2.20.1

[PULL 8/8] 9pfs: use P9Array in v9fs_walk()

[Christian Schoenebeck]

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
Message-Id: <90c65d1c1ca11c1b434bb981b1fc7966f7711c8f.1633097129.git.qemu_oss@crudebyte.com>
---
 hw/9pfs/9p.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 15bb16f466..15b3f4d385 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1738,13 +1738,14 @@ static void coroutine_fn v9fs_walk(void *opaque)
     int name_idx;
     g_autofree V9fsQID *qids = NULL;
     int i, err = 0;
-    V9fsPath dpath, path, *pathes = NULL;
+    V9fsPath dpath, path;
+    P9ARRAY_REF(V9fsPath) pathes = NULL;
     uint16_t nwnames;
     struct stat stbuf, fidst;
     g_autofree struct stat *stbufs = NULL;
     size_t offset = 7;
     int32_t fid, newfid;
-    V9fsString *wnames = NULL;
+    P9ARRAY_REF(V9fsString) wnames = NULL;
     V9fsFidState *fidp;
     V9fsFidState *newfidp = NULL;
     V9fsPDU *pdu = opaque;
@@ -1765,10 +1766,10 @@ static void coroutine_fn v9fs_walk(void *opaque)
         goto out_nofid;
     }
     if (nwnames) {
-        wnames = g_new0(V9fsString, nwnames);
+        P9ARRAY_NEW(V9fsString, wnames, nwnames);
         qids   = g_new0(V9fsQID, nwnames);
         stbufs = g_new0(struct stat, nwnames);
-        pathes = g_new0(V9fsPath, nwnames);
+        P9ARRAY_NEW(V9fsPath, pathes, nwnames);
         for (i = 0; i < nwnames; i++) {
             err = pdu_unmarshal(pdu, offset, "s", &wnames[i]);
             if (err < 0) {
@@ -1900,14 +1901,6 @@ out:
     v9fs_path_free(&path);
 out_nofid:
     pdu_complete(pdu, err);
-    if (nwnames && nwnames <= P9_MAXWELEM) {
-        for (name_idx = 0; name_idx < nwnames; name_idx++) {
-            v9fs_string_free(&wnames[name_idx]);
-            v9fs_path_free(&pathes[name_idx]);
-        }
-        g_free(wnames);
-        g_free(pathes);
-    }
 }
 
 static int32_t coroutine_fn get_iounit(V9fsPDU *pdu, V9fsPath *path)
-- 
2.20.1

Re: [PULL 0/8] 9p queue 2021-10-27

[Christian Schoenebeck]

On Mittwoch, 27. Oktober 2021 15:18:33 CEST Christian Schoenebeck wrote:
> The following changes since commit 931ce30859176f0f7daac6bac255dae5eb21284e:
> 
>   Merge remote-tracking branch 'remotes/dagrh/tags/pull-virtiofs-20211026'
> into staging (2021-10-26 07:38:41 -0700)
> 
> are available in the Git repository at:
> 
>   https://github.com/cschoenebeck/qemu.git tags/pull-9p-20211027
> 
> for you to fetch changes up to 7e985780aaab93d2c5be9b62d8d386568dfb071e:
> 
>   9pfs: use P9Array in v9fs_walk() (2021-10-27 14:45:22 +0200)
> 
> ----------------------------------------------------------------
> 9pfs: performance fix and cleanup
> 
> * First patch fixes suboptimal I/O performance on guest due to previously
>   incorrect block size being transmitted to 9p client.
> 
> * Subsequent patches are cleanup ones intended to reduce code complexity.
> 
> ----------------------------------------------------------------
> Christian Schoenebeck (8):
>       9pfs: fix wrong I/O block size in Rgetattr
>       9pfs: deduplicate iounit code
>       9pfs: simplify blksize_to_iounit()
>       9pfs: introduce P9Array
>       fsdev/p9array.h: check scalar type in P9ARRAY_NEW()
>       9pfs: make V9fsString usable via P9Array API
>       9pfs: make V9fsPath usable via P9Array API
>       9pfs: use P9Array in v9fs_walk()
> 
>  fsdev/9p-marshal.c |   2 +
>  fsdev/9p-marshal.h |   3 +
>  fsdev/file-op-9p.h |   2 +
>  fsdev/p9array.h    | 160
> +++++++++++++++++++++++++++++++++++++++++++++++++++++ hw/9pfs/9p.c       | 
> 70 +++++++++++++----------
>  5 files changed, 208 insertions(+), 29 deletions(-)
>  create mode 100644 fsdev/p9array.h

Regarding last 5 patches: Daniel raised a concern that not using g_autoptr 
would deviate from current QEMU coding patterns:
https://lists.gnu.org/archive/html/qemu-devel/2021-10/msg00081.html

Unfortunately I saw no way to address his concern without adding unnecessary 
code complexity, so I decided to make this a 9p local type (QArray -> P9Array) 
for now, which can easily be replaced in future (e.g. when there will be 
something appropriate on glib side).

Best regards,
Christian Schoenebeck

Re: [PULL 0/8] 9p queue 2021-10-27

[Philippe Mathieu-Daudé]

Hi Christian,

On 10/27/21 16:05, Christian Schoenebeck wrote:
> On Mittwoch, 27. Oktober 2021 15:18:33 CEST Christian Schoenebeck wrote:
>> The following changes since commit 931ce30859176f0f7daac6bac255dae5eb21284e:
>>
>>   Merge remote-tracking branch 'remotes/dagrh/tags/pull-virtiofs-20211026'
>> into staging (2021-10-26 07:38:41 -0700)
>>
>> are available in the Git repository at:
>>
>>   https://github.com/cschoenebeck/qemu.git tags/pull-9p-20211027
>>
>> for you to fetch changes up to 7e985780aaab93d2c5be9b62d8d386568dfb071e:
>>
>>   9pfs: use P9Array in v9fs_walk() (2021-10-27 14:45:22 +0200)
>>
>> ----------------------------------------------------------------
>> 9pfs: performance fix and cleanup
>>
>> * First patch fixes suboptimal I/O performance on guest due to previously
>>   incorrect block size being transmitted to 9p client.
>>
>> * Subsequent patches are cleanup ones intended to reduce code complexity.
>>
>> ----------------------------------------------------------------
>> Christian Schoenebeck (8):
>>       9pfs: fix wrong I/O block size in Rgetattr
>>       9pfs: deduplicate iounit code
>>       9pfs: simplify blksize_to_iounit()
>>       9pfs: introduce P9Array
>>       fsdev/p9array.h: check scalar type in P9ARRAY_NEW()
>>       9pfs: make V9fsString usable via P9Array API
>>       9pfs: make V9fsPath usable via P9Array API
>>       9pfs: use P9Array in v9fs_walk()
>>
>>  fsdev/9p-marshal.c |   2 +
>>  fsdev/9p-marshal.h |   3 +
>>  fsdev/file-op-9p.h |   2 +
>>  fsdev/p9array.h    | 160
>> +++++++++++++++++++++++++++++++++++++++++++++++++++++ hw/9pfs/9p.c       | 
>> 70 +++++++++++++----------
>>  5 files changed, 208 insertions(+), 29 deletions(-)
>>  create mode 100644 fsdev/p9array.h
> 
> Regarding last 5 patches: Daniel raised a concern that not using g_autoptr 
> would deviate from current QEMU coding patterns:
> https://lists.gnu.org/archive/html/qemu-devel/2021-10/msg00081.html
> 
> Unfortunately I saw no way to address his concern without adding unnecessary 
> code complexity, so I decided to make this a 9p local type (QArray -> P9Array) 
> for now, which can easily be replaced in future (e.g. when there will be 
> something appropriate on glib side).

Hmm various patches aren't reviewed yet... In particular
patch #5 has a Suggested-by tag without Reviewed-by, this
looks odd.

See https://wiki.qemu.org/Contribute/SubmitAPullRequest:

  Don't send pull requests for code that hasn't passed review.
  A pull request says these patches are ready to go into QEMU now,
  so they must have passed the standard code review processes. In
  particular if you've corrected issues in one round of code review,
  you need to send your fixed patch series as normal to the list;
  you can't put it in a pull request until it's gone through.
  (Extremely trivial fixes may be OK to just fix in passing, but
  if in doubt err on the side of not.)

Re: [PULL 0/8] 9p queue 2021-10-27

[Greg Kurz]

On Wed, 27 Oct 2021 16:05:39 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Mittwoch, 27. Oktober 2021 15:18:33 CEST Christian Schoenebeck wrote:
> > The following changes since commit 931ce30859176f0f7daac6bac255dae5eb21284e:
> > 
> >   Merge remote-tracking branch 'remotes/dagrh/tags/pull-virtiofs-20211026'
> > into staging (2021-10-26 07:38:41 -0700)
> > 
> > are available in the Git repository at:
> > 
> >   https://github.com/cschoenebeck/qemu.git tags/pull-9p-20211027
> > 
> > for you to fetch changes up to 7e985780aaab93d2c5be9b62d8d386568dfb071e:
> > 
> >   9pfs: use P9Array in v9fs_walk() (2021-10-27 14:45:22 +0200)
> > 
> > ----------------------------------------------------------------
> > 9pfs: performance fix and cleanup
> > 
> > * First patch fixes suboptimal I/O performance on guest due to previously
> >   incorrect block size being transmitted to 9p client.
> > 
> > * Subsequent patches are cleanup ones intended to reduce code complexity.
> > 
> > ----------------------------------------------------------------
> > Christian Schoenebeck (8):
> >       9pfs: fix wrong I/O block size in Rgetattr
> >       9pfs: deduplicate iounit code
> >       9pfs: simplify blksize_to_iounit()
> >       9pfs: introduce P9Array
> >       fsdev/p9array.h: check scalar type in P9ARRAY_NEW()
> >       9pfs: make V9fsString usable via P9Array API
> >       9pfs: make V9fsPath usable via P9Array API
> >       9pfs: use P9Array in v9fs_walk()
> > 
> >  fsdev/9p-marshal.c |   2 +
> >  fsdev/9p-marshal.h |   3 +
> >  fsdev/file-op-9p.h |   2 +
> >  fsdev/p9array.h    | 160
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++ hw/9pfs/9p.c       | 
> > 70 +++++++++++++----------
> >  5 files changed, 208 insertions(+), 29 deletions(-)
> >  create mode 100644 fsdev/p9array.h
> 
> Regarding last 5 patches: Daniel raised a concern that not using g_autoptr 
> would deviate from current QEMU coding patterns:
> https://lists.gnu.org/archive/html/qemu-devel/2021-10/msg00081.html
> 
> Unfortunately I saw no way to address his concern without adding unnecessary 
> code complexity, so I decided to make this a 9p local type (QArray -> P9Array) 
> for now, which can easily be replaced in future (e.g. when there will be 
> something appropriate on glib side).
> 

Christian,

Given that the P9Array is controversial and that the current benefit in
patch 8 is relatively small, I'd suggest that you just drop patches 4 to
8 for now.

Cheers,

--
Greg

> Best regards,
> Christian Schoenebeck
> 
> 

Re: [PULL 0/8] 9p queue 2021-10-27

[Christian Schoenebeck]

On Mittwoch, 27. Oktober 2021 17:36:03 CEST Philippe Mathieu-Daudé wrote:
> Hi Christian,
> 
> On 10/27/21 16:05, Christian Schoenebeck wrote:
> > On Mittwoch, 27. Oktober 2021 15:18:33 CEST Christian Schoenebeck wrote:
> >> The following changes since commit 
931ce30859176f0f7daac6bac255dae5eb21284e:
> >>   Merge remote-tracking branch
> >>   'remotes/dagrh/tags/pull-virtiofs-20211026'
> >> 
> >> into staging (2021-10-26 07:38:41 -0700)
> >> 
> >> are available in the Git repository at:
> >>   https://github.com/cschoenebeck/qemu.git tags/pull-9p-20211027
> >> 
> >> for you to fetch changes up to 7e985780aaab93d2c5be9b62d8d386568dfb071e:
> >>   9pfs: use P9Array in v9fs_walk() (2021-10-27 14:45:22 +0200)
> >> 
> >> ----------------------------------------------------------------
> >> 9pfs: performance fix and cleanup
> >> 
> >> * First patch fixes suboptimal I/O performance on guest due to previously
> >> 
> >>   incorrect block size being transmitted to 9p client.
> >> 
> >> * Subsequent patches are cleanup ones intended to reduce code complexity.
> >> 
> >> ----------------------------------------------------------------
> >> 
> >> Christian Schoenebeck (8):
> >>       9pfs: fix wrong I/O block size in Rgetattr
> >>       9pfs: deduplicate iounit code
> >>       9pfs: simplify blksize_to_iounit()
> >>       9pfs: introduce P9Array
> >>       fsdev/p9array.h: check scalar type in P9ARRAY_NEW()
> >>       9pfs: make V9fsString usable via P9Array API
> >>       9pfs: make V9fsPath usable via P9Array API
> >>       9pfs: use P9Array in v9fs_walk()
> >>  
> >>  fsdev/9p-marshal.c |   2 +
> >>  fsdev/9p-marshal.h |   3 +
> >>  fsdev/file-op-9p.h |   2 +
> >>  fsdev/p9array.h    | 160
> >> 
> >> +++++++++++++++++++++++++++++++++++++++++++++++++++++ hw/9pfs/9p.c      
> >> |
> >> 70 +++++++++++++----------
> >> 
> >>  5 files changed, 208 insertions(+), 29 deletions(-)
> >>  create mode 100644 fsdev/p9array.h
> > 
> > Regarding last 5 patches: Daniel raised a concern that not using g_autoptr
> > would deviate from current QEMU coding patterns:
> > https://lists.gnu.org/archive/html/qemu-devel/2021-10/msg00081.html
> > 
> > Unfortunately I saw no way to address his concern without adding
> > unnecessary code complexity, so I decided to make this a 9p local type
> > (QArray -> P9Array) for now, which can easily be replaced in future (e.g.
> > when there will be something appropriate on glib side).
> 
> Hmm various patches aren't reviewed yet... In particular
> patch #5 has a Suggested-by tag without Reviewed-by, this
> looks odd.
> 
> See https://wiki.qemu.org/Contribute/SubmitAPullRequest:
> 
>   Don't send pull requests for code that hasn't passed review.
>   A pull request says these patches are ready to go into QEMU now,
>   so they must have passed the standard code review processes. In
>   particular if you've corrected issues in one round of code review,
>   you need to send your fixed patch series as normal to the list;
>   you can't put it in a pull request until it's gone through.
>   (Extremely trivial fixes may be OK to just fix in passing, but
>   if in doubt err on the side of not.)

There are in general exactly two persons adding their RBs to 9p patches, which 
is either Greg or me, and Greg made it already clear that he barely has time 
for anything above trivial set.

So what do you suggest? You want to participate and review 9p patches?

Best regards,
Christian Schoenebeck

Re: [PULL 0/8] 9p queue 2021-10-27

[Philippe Mathieu-Daudé]

On 10/27/21 18:21, Christian Schoenebeck wrote:
> On Mittwoch, 27. Oktober 2021 17:36:03 CEST Philippe Mathieu-Daudé wrote:
>> Hi Christian,
>>
>> On 10/27/21 16:05, Christian Schoenebeck wrote:
>>> On Mittwoch, 27. Oktober 2021 15:18:33 CEST Christian Schoenebeck wrote:
>>>> The following changes since commit 
> 931ce30859176f0f7daac6bac255dae5eb21284e:
>>>>   Merge remote-tracking branch
>>>>   'remotes/dagrh/tags/pull-virtiofs-20211026'
>>>>
>>>> into staging (2021-10-26 07:38:41 -0700)
>>>>
>>>> are available in the Git repository at:
>>>>   https://github.com/cschoenebeck/qemu.git tags/pull-9p-20211027
>>>>
>>>> for you to fetch changes up to 7e985780aaab93d2c5be9b62d8d386568dfb071e:
>>>>   9pfs: use P9Array in v9fs_walk() (2021-10-27 14:45:22 +0200)
>>>>
>>>> ----------------------------------------------------------------
>>>> 9pfs: performance fix and cleanup
>>>>
>>>> * First patch fixes suboptimal I/O performance on guest due to previously
>>>>
>>>>   incorrect block size being transmitted to 9p client.
>>>>
>>>> * Subsequent patches are cleanup ones intended to reduce code complexity.
>>>>
>>>> ----------------------------------------------------------------
>>>>
>>>> Christian Schoenebeck (8):
>>>>       9pfs: fix wrong I/O block size in Rgetattr
>>>>       9pfs: deduplicate iounit code
>>>>       9pfs: simplify blksize_to_iounit()
>>>>       9pfs: introduce P9Array
>>>>       fsdev/p9array.h: check scalar type in P9ARRAY_NEW()
>>>>       9pfs: make V9fsString usable via P9Array API
>>>>       9pfs: make V9fsPath usable via P9Array API
>>>>       9pfs: use P9Array in v9fs_walk()
>>>>  
>>>>  fsdev/9p-marshal.c |   2 +
>>>>  fsdev/9p-marshal.h |   3 +
>>>>  fsdev/file-op-9p.h |   2 +
>>>>  fsdev/p9array.h    | 160
>>>>
>>>> +++++++++++++++++++++++++++++++++++++++++++++++++++++ hw/9pfs/9p.c      
>>>> |
>>>> 70 +++++++++++++----------
>>>>
>>>>  5 files changed, 208 insertions(+), 29 deletions(-)
>>>>  create mode 100644 fsdev/p9array.h
>>>
>>> Regarding last 5 patches: Daniel raised a concern that not using g_autoptr
>>> would deviate from current QEMU coding patterns:
>>> https://lists.gnu.org/archive/html/qemu-devel/2021-10/msg00081.html
>>>
>>> Unfortunately I saw no way to address his concern without adding
>>> unnecessary code complexity, so I decided to make this a 9p local type
>>> (QArray -> P9Array) for now, which can easily be replaced in future (e.g.
>>> when there will be something appropriate on glib side).
>>
>> Hmm various patches aren't reviewed yet... In particular
>> patch #5 has a Suggested-by tag without Reviewed-by, this
>> looks odd.
>>
>> See https://wiki.qemu.org/Contribute/SubmitAPullRequest:
>>
>>   Don't send pull requests for code that hasn't passed review.
>>   A pull request says these patches are ready to go into QEMU now,
>>   so they must have passed the standard code review processes. In
>>   particular if you've corrected issues in one round of code review,
>>   you need to send your fixed patch series as normal to the list;
>>   you can't put it in a pull request until it's gone through.
>>   (Extremely trivial fixes may be OK to just fix in passing, but
>>   if in doubt err on the side of not.)
> 
> There are in general exactly two persons adding their RBs to 9p patches, which 
> is either Greg or me, and Greg made it already clear that he barely has time 
> for anything above trivial set.
> 
> So what do you suggest? You want to participate and review 9p patches?

Well I am a bit surprised...

$ git log --oneline \
    --grep='Reviewed-by: Philippe Mathieu-Daudé' -- hw/9pfs/ | wc -l
18

I also reviewed patch #3 if this pull request...


Now I see you posted this 4 times in 2 months, so indeed eventual
reviewers had plenty of time to look at your patches.

Note I haven't said I'd NAck your pull request, I noticed your own
concern wrt Daniel comment, so I looked at the patch and realized
it was not reviewed, and simply said this is this is odd.

Regards,

Phil.

Re: [PULL 0/8] 9p queue 2021-10-27

[Christian Schoenebeck]

On Mittwoch, 27. Oktober 2021 18:48:10 CEST Philippe Mathieu-Daudé wrote:
> On 10/27/21 18:21, Christian Schoenebeck wrote:
> > On Mittwoch, 27. Oktober 2021 17:36:03 CEST Philippe Mathieu-Daudé wrote:
> >> Hi Christian,
> >> 
> >> On 10/27/21 16:05, Christian Schoenebeck wrote:
> >>> On Mittwoch, 27. Oktober 2021 15:18:33 CEST Christian Schoenebeck wrote:
> >>>> The following changes since commit
> > 
> > 931ce30859176f0f7daac6bac255dae5eb21284e:
> >>>>   Merge remote-tracking branch
> >>>>   'remotes/dagrh/tags/pull-virtiofs-20211026'
> >>>> 
> >>>> into staging (2021-10-26 07:38:41 -0700)
> >>>> 
> >>>> are available in the Git repository at:
> >>>>   https://github.com/cschoenebeck/qemu.git tags/pull-9p-20211027
> >>>> 
> >>>> for you to fetch changes up to 
7e985780aaab93d2c5be9b62d8d386568dfb071e:
> >>>>   9pfs: use P9Array in v9fs_walk() (2021-10-27 14:45:22 +0200)
> >>>> 
> >>>> ----------------------------------------------------------------
> >>>> 9pfs: performance fix and cleanup
> >>>> 
> >>>> * First patch fixes suboptimal I/O performance on guest due to
> >>>> previously
> >>>> 
> >>>>   incorrect block size being transmitted to 9p client.
> >>>> 
> >>>> * Subsequent patches are cleanup ones intended to reduce code
> >>>> complexity.
> >>>> 
> >>>> ----------------------------------------------------------------
> >>>> 
> >>>> Christian Schoenebeck (8):
> >>>>       9pfs: fix wrong I/O block size in Rgetattr
> >>>>       9pfs: deduplicate iounit code
> >>>>       9pfs: simplify blksize_to_iounit()
> >>>>       9pfs: introduce P9Array
> >>>>       fsdev/p9array.h: check scalar type in P9ARRAY_NEW()
> >>>>       9pfs: make V9fsString usable via P9Array API
> >>>>       9pfs: make V9fsPath usable via P9Array API
> >>>>       9pfs: use P9Array in v9fs_walk()
> >>>>  
> >>>>  fsdev/9p-marshal.c |   2 +
> >>>>  fsdev/9p-marshal.h |   3 +
> >>>>  fsdev/file-op-9p.h |   2 +
> >>>>  fsdev/p9array.h    | 160
> >>>> 
> >>>> +++++++++++++++++++++++++++++++++++++++++++++++++++++ hw/9pfs/9p.c
> >>>> 
> >>>> 70 +++++++++++++----------
> >>>> 
> >>>>  5 files changed, 208 insertions(+), 29 deletions(-)
> >>>>  create mode 100644 fsdev/p9array.h
> >>> 
> >>> Regarding last 5 patches: Daniel raised a concern that not using
> >>> g_autoptr
> >>> would deviate from current QEMU coding patterns:
> >>> https://lists.gnu.org/archive/html/qemu-devel/2021-10/msg00081.html
> >>> 
> >>> Unfortunately I saw no way to address his concern without adding
> >>> unnecessary code complexity, so I decided to make this a 9p local type
> >>> (QArray -> P9Array) for now, which can easily be replaced in future
> >>> (e.g.
> >>> when there will be something appropriate on glib side).
> >> 
> >> Hmm various patches aren't reviewed yet... In particular
> >> patch #5 has a Suggested-by tag without Reviewed-by, this
> >> looks odd.
> >> 
> >> See https://wiki.qemu.org/Contribute/SubmitAPullRequest:
> >>   Don't send pull requests for code that hasn't passed review.
> >>   A pull request says these patches are ready to go into QEMU now,
> >>   so they must have passed the standard code review processes. In
> >>   particular if you've corrected issues in one round of code review,
> >>   you need to send your fixed patch series as normal to the list;
> >>   you can't put it in a pull request until it's gone through.
> >>   (Extremely trivial fixes may be OK to just fix in passing, but
> >>   if in doubt err on the side of not.)
> > 
> > There are in general exactly two persons adding their RBs to 9p patches,
> > which is either Greg or me, and Greg made it already clear that he barely
> > has time for anything above trivial set.
> > 
> > So what do you suggest? You want to participate and review 9p patches?
> 
> Well I am a bit surprised...
> 
> $ git log --oneline \
>     --grep='Reviewed-by: Philippe Mathieu-Daudé' -- hw/9pfs/ | wc -l
> 18
> 
> I also reviewed patch #3 if this pull request...
> 
> 
> Now I see you posted this 4 times in 2 months, so indeed eventual
> reviewers had plenty of time to look at your patches.
> 
> Note I haven't said I'd NAck your pull request, I noticed your own
> concern wrt Daniel comment, so I looked at the patch and realized
> it was not reviewed, and simply said this is this is odd.
> 
> Regards,
> 
> Phil.

Philippe, of course I understand why this looks odd to you, and yes you 
reviewed that particular patch. But the situation on the 9p front is like this 
for >2 years now: people quickly come by to nack patches, but even after 
having addressed their concerns they barely add their RBs afterwards. That 
means I am currently forced to send out PRs without RBs once in a while.

The mentioned 5 patches look like overkill on first sight, but they are just 
intended as preparatory ones. I actually should fix a protocol implementation 
deficit in "Twalk" request handling, and that in turn means I will have to add 
complexity to function v9fs_walk(). But before even daring to do so, I should 
get rid of as much of complexity as possible. Because we already had a bunch 
of issues in that function before. I believe these are trivial 5 patches. But 
I can also accompany them with test cases if somebody is worried.

Greg, I could also drop them now, but the general issue will retain: Reality 
is that I am the only person working on 9p right now and a fact that I cannot 
change. The rest is for other people to decide.

Best regards,
Christian Schoenebeck

Re: [PULL 0/8] 9p queue 2021-10-27

[Philippe Mathieu-Daudé]

Hi Richard,

On 10/27/21 19:29, Christian Schoenebeck wrote:
> On Mittwoch, 27. Oktober 2021 18:48:10 CEST Philippe Mathieu-Daudé wrote:
>> On 10/27/21 18:21, Christian Schoenebeck wrote:
>>> On Mittwoch, 27. Oktober 2021 17:36:03 CEST Philippe Mathieu-Daudé wrote:
>>>> On 10/27/21 16:05, Christian Schoenebeck wrote:

>>>>> Regarding last 5 patches: Daniel raised a concern that not using
>>>>> g_autoptr
>>>>> would deviate from current QEMU coding patterns:
>>>>> https://lists.gnu.org/archive/html/qemu-devel/2021-10/msg00081.html
>>>>>
>>>>> Unfortunately I saw no way to address his concern without adding
>>>>> unnecessary code complexity, so I decided to make this a 9p local type
>>>>> (QArray -> P9Array) for now, which can easily be replaced in future
>>>>> (e.g.
>>>>> when there will be something appropriate on glib side).
>>>>
>>>> Hmm various patches aren't reviewed yet... In particular
>>>> patch #5 has a Suggested-by tag without Reviewed-by, this
>>>> looks odd.
>>>>
>>>> See https://wiki.qemu.org/Contribute/SubmitAPullRequest:
>>>>   Don't send pull requests for code that hasn't passed review.
>>>>   A pull request says these patches are ready to go into QEMU now,
>>>>   so they must have passed the standard code review processes. In
>>>>   particular if you've corrected issues in one round of code review,
>>>>   you need to send your fixed patch series as normal to the list;
>>>>   you can't put it in a pull request until it's gone through.
>>>>   (Extremely trivial fixes may be OK to just fix in passing, but
>>>>   if in doubt err on the side of not.)
>>>
>>> There are in general exactly two persons adding their RBs to 9p patches,
>>> which is either Greg or me, and Greg made it already clear that he barely
>>> has time for anything above trivial set.
>>>
>>> So what do you suggest? You want to participate and review 9p patches?
>>
>> Well I am a bit surprised...
>>
>> $ git log --oneline \
>>     --grep='Reviewed-by: Philippe Mathieu-Daudé' -- hw/9pfs/ | wc -l
>> 18
>>
>> I also reviewed patch #3 if this pull request...
>>
>>
>> Now I see you posted this 4 times in 2 months, so indeed eventual
>> reviewers had plenty of time to look at your patches.
>>
>> Note I haven't said I'd NAck your pull request, I noticed your own
>> concern wrt Daniel comment, so I looked at the patch and realized
>> it was not reviewed, and simply said this is this is odd.
>>
>> Regards,
>>
>> Phil.
> 
> Philippe, of course I understand why this looks odd to you, and yes you 
> reviewed that particular patch. But the situation on the 9p front is like this 
> for >2 years now: people quickly come by to nack patches, but even after 
> having addressed their concerns they barely add their RBs afterwards. That 
> means I am currently forced to send out PRs without RBs once in a while.
> 
> The mentioned 5 patches look like overkill on first sight, but they are just 
> intended as preparatory ones. I actually should fix a protocol implementation 
> deficit in "Twalk" request handling, and that in turn means I will have to add 
> complexity to function v9fs_walk(). But before even daring to do so, I should 
> get rid of as much of complexity as possible. Because we already had a bunch 
> of issues in that function before. I believe these are trivial 5 patches. But 
> I can also accompany them with test cases if somebody is worried.
> 
> Greg, I could also drop them now, but the general issue will retain: Reality 
> is that I am the only person working on 9p right now and a fact that I cannot 
> change. The rest is for other people to decide.

To be explicit, I just asked clarifications to Christian. I understand
the 9pfs subsystem situation, and I am not against (Nacking) this pull
request being merged.

Thanks both,

Phil.

Re: [PULL 0/8] 9p queue 2021-10-27

[Richard Henderson]

On 10/27/21 10:29 AM, Christian Schoenebeck wrote:
> On Mittwoch, 27. Oktober 2021 18:48:10 CEST Philippe Mathieu-Daudé wrote:
>> On 10/27/21 18:21, Christian Schoenebeck wrote:
>>> On Mittwoch, 27. Oktober 2021 17:36:03 CEST Philippe Mathieu-Daudé wrote:
>>>> Hi Christian,
>>>>
>>>> On 10/27/21 16:05, Christian Schoenebeck wrote:
>>>>> On Mittwoch, 27. Oktober 2021 15:18:33 CEST Christian Schoenebeck wrote:
>>>>>> The following changes since commit
>>>
>>> 931ce30859176f0f7daac6bac255dae5eb21284e:
>>>>>>    Merge remote-tracking branch
>>>>>>    'remotes/dagrh/tags/pull-virtiofs-20211026'
>>>>>>
>>>>>> into staging (2021-10-26 07:38:41 -0700)
>>>>>>
>>>>>> are available in the Git repository at:
>>>>>>    https://github.com/cschoenebeck/qemu.git tags/pull-9p-20211027
>>>>>>
>>>>>> for you to fetch changes up to
> 7e985780aaab93d2c5be9b62d8d386568dfb071e:
>>>>>>    9pfs: use P9Array in v9fs_walk() (2021-10-27 14:45:22 +0200)
>>>>>>
>>>>>> ----------------------------------------------------------------
>>>>>> 9pfs: performance fix and cleanup
>>>>>>
>>>>>> * First patch fixes suboptimal I/O performance on guest due to
>>>>>> previously
>>>>>>
>>>>>>    incorrect block size being transmitted to 9p client.
>>>>>>
>>>>>> * Subsequent patches are cleanup ones intended to reduce code
>>>>>> complexity.
>>>>>>
>>>>>> ----------------------------------------------------------------
>>>>>>
>>>>>> Christian Schoenebeck (8):
>>>>>>        9pfs: fix wrong I/O block size in Rgetattr
>>>>>>        9pfs: deduplicate iounit code
>>>>>>        9pfs: simplify blksize_to_iounit()
>>>>>>        9pfs: introduce P9Array
>>>>>>        fsdev/p9array.h: check scalar type in P9ARRAY_NEW()
>>>>>>        9pfs: make V9fsString usable via P9Array API
>>>>>>        9pfs: make V9fsPath usable via P9Array API
>>>>>>        9pfs: use P9Array in v9fs_walk()
>>>>>>   
>>>>>>   fsdev/9p-marshal.c |   2 +
>>>>>>   fsdev/9p-marshal.h |   3 +
>>>>>>   fsdev/file-op-9p.h |   2 +
>>>>>>   fsdev/p9array.h    | 160
>>>>>>
>>>>>> +++++++++++++++++++++++++++++++++++++++++++++++++++++ hw/9pfs/9p.c
>>>>>>
>>>>>> 70 +++++++++++++----------
>>>>>>
>>>>>>   5 files changed, 208 insertions(+), 29 deletions(-)
>>>>>>   create mode 100644 fsdev/p9array.h
>>>>>
>>>>> Regarding last 5 patches: Daniel raised a concern that not using
>>>>> g_autoptr
>>>>> would deviate from current QEMU coding patterns:
>>>>> https://lists.gnu.org/archive/html/qemu-devel/2021-10/msg00081.html
>>>>>
>>>>> Unfortunately I saw no way to address his concern without adding
>>>>> unnecessary code complexity, so I decided to make this a 9p local type
>>>>> (QArray -> P9Array) for now, which can easily be replaced in future
>>>>> (e.g.
>>>>> when there will be something appropriate on glib side).
>>>>
>>>> Hmm various patches aren't reviewed yet... In particular
>>>> patch #5 has a Suggested-by tag without Reviewed-by, this
>>>> looks odd.
>>>>
>>>> See https://wiki.qemu.org/Contribute/SubmitAPullRequest:
>>>>    Don't send pull requests for code that hasn't passed review.
>>>>    A pull request says these patches are ready to go into QEMU now,
>>>>    so they must have passed the standard code review processes. In
>>>>    particular if you've corrected issues in one round of code review,
>>>>    you need to send your fixed patch series as normal to the list;
>>>>    you can't put it in a pull request until it's gone through.
>>>>    (Extremely trivial fixes may be OK to just fix in passing, but
>>>>    if in doubt err on the side of not.)
>>>
>>> There are in general exactly two persons adding their RBs to 9p patches,
>>> which is either Greg or me, and Greg made it already clear that he barely
>>> has time for anything above trivial set.
>>>
>>> So what do you suggest? You want to participate and review 9p patches?
>>
>> Well I am a bit surprised...
>>
>> $ git log --oneline \
>>      --grep='Reviewed-by: Philippe Mathieu-Daudé' -- hw/9pfs/ | wc -l
>> 18
>>
>> I also reviewed patch #3 if this pull request...
>>
>>
>> Now I see you posted this 4 times in 2 months, so indeed eventual
>> reviewers had plenty of time to look at your patches.
>>
>> Note I haven't said I'd NAck your pull request, I noticed your own
>> concern wrt Daniel comment, so I looked at the patch and realized
>> it was not reviewed, and simply said this is this is odd.
>>
>> Regards,
>>
>> Phil.
> 
> Philippe, of course I understand why this looks odd to you, and yes you
> reviewed that particular patch. But the situation on the 9p front is like this
> for >2 years now: people quickly come by to nack patches, but even after
> having addressed their concerns they barely add their RBs afterwards. That
> means I am currently forced to send out PRs without RBs once in a while.

In know the feeling -- it takes quite some prodding to get tcg patches reviewed, and I 
have also sent out PRs that are incompletely reviewed.

I see that patch 5 was something I suggested myself, which I then failed to review 
afterward.  In recompense, I have reviewed the whole patch set:

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

I think the P9Array is fairly clever, and I do prefer it over glib ugliness.  I can 
imagine only C++ being an improvement over what you've created.

Rather than force you to re-create the PR, I'll simply apply this along with the S-o-b, to 
the merge object.


r~

Re: [PULL 0/8] 9p queue 2021-10-27

[Richard Henderson]

On 10/27/21 6:18 AM, Christian Schoenebeck wrote:
> The following changes since commit 931ce30859176f0f7daac6bac255dae5eb21284e:
> 
>    Merge remote-tracking branch 'remotes/dagrh/tags/pull-virtiofs-20211026' into staging (2021-10-26 07:38:41 -0700)
> 
> are available in the Git repository at:
> 
>    https://github.com/cschoenebeck/qemu.git tags/pull-9p-20211027
> 
> for you to fetch changes up to 7e985780aaab93d2c5be9b62d8d386568dfb071e:
> 
>    9pfs: use P9Array in v9fs_walk() (2021-10-27 14:45:22 +0200)
> 
> ----------------------------------------------------------------
> 9pfs: performance fix and cleanup
> 
> * First patch fixes suboptimal I/O performance on guest due to previously
>    incorrect block size being transmitted to 9p client.
> 
> * Subsequent patches are cleanup ones intended to reduce code complexity.
> 
> ----------------------------------------------------------------
> Christian Schoenebeck (8):
>        9pfs: fix wrong I/O block size in Rgetattr
>        9pfs: deduplicate iounit code
>        9pfs: simplify blksize_to_iounit()
>        9pfs: introduce P9Array
>        fsdev/p9array.h: check scalar type in P9ARRAY_NEW()
>        9pfs: make V9fsString usable via P9Array API
>        9pfs: make V9fsPath usable via P9Array API
>        9pfs: use P9Array in v9fs_walk()
> 
>   fsdev/9p-marshal.c |   2 +
>   fsdev/9p-marshal.h |   3 +
>   fsdev/file-op-9p.h |   2 +
>   fsdev/p9array.h    | 160 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>   hw/9pfs/9p.c       |  70 +++++++++++++----------
>   5 files changed, 208 insertions(+), 29 deletions(-)
>   create mode 100644 fsdev/p9array.h

Applied, with my R-b as I promised upthread.  Thanks.


r~

Re: [PULL 0/8] 9p queue 2021-10-27

[Christian Schoenebeck]

On Mittwoch, 27. Oktober 2021 20:44:52 CEST Richard Henderson wrote:
> On 10/27/21 10:29 AM, Christian Schoenebeck wrote:
> > On Mittwoch, 27. Oktober 2021 18:48:10 CEST Philippe Mathieu-Daudé wrote:
> >> On 10/27/21 18:21, Christian Schoenebeck wrote:
> >>> On Mittwoch, 27. Oktober 2021 17:36:03 CEST Philippe Mathieu-Daudé 
wrote:
> >>>> Hi Christian,
> >>>> 
> >>>> On 10/27/21 16:05, Christian Schoenebeck wrote:
> >>>>> On Mittwoch, 27. Oktober 2021 15:18:33 CEST Christian Schoenebeck 
wrote:
> >>>>>> The following changes since commit
> >>> 
> >>> 931ce30859176f0f7daac6bac255dae5eb21284e:
> >>>>>>    Merge remote-tracking branch
> >>>>>>    'remotes/dagrh/tags/pull-virtiofs-20211026'
> >>>>>> 
> >>>>>> into staging (2021-10-26 07:38:41 -0700)
> >>>>>> 
> >>>>>> are available in the Git repository at:
> >>>>>>    https://github.com/cschoenebeck/qemu.git tags/pull-9p-20211027
> >>>>>> 
> >>>>>> for you to fetch changes up to
> > 
> > 7e985780aaab93d2c5be9b62d8d386568dfb071e:
> >>>>>>    9pfs: use P9Array in v9fs_walk() (2021-10-27 14:45:22 +0200)
> >>>>>> 
> >>>>>> ----------------------------------------------------------------
> >>>>>> 9pfs: performance fix and cleanup
> >>>>>> 
> >>>>>> * First patch fixes suboptimal I/O performance on guest due to
> >>>>>> previously
> >>>>>> 
> >>>>>>    incorrect block size being transmitted to 9p client.
> >>>>>> 
> >>>>>> * Subsequent patches are cleanup ones intended to reduce code
> >>>>>> complexity.
> >>>>>> 
> >>>>>> ----------------------------------------------------------------
> >>>>>> 
> >>>>>> Christian Schoenebeck (8):
> >>>>>>        9pfs: fix wrong I/O block size in Rgetattr
> >>>>>>        9pfs: deduplicate iounit code
> >>>>>>        9pfs: simplify blksize_to_iounit()
> >>>>>>        9pfs: introduce P9Array
> >>>>>>        fsdev/p9array.h: check scalar type in P9ARRAY_NEW()
> >>>>>>        9pfs: make V9fsString usable via P9Array API
> >>>>>>        9pfs: make V9fsPath usable via P9Array API
> >>>>>>        9pfs: use P9Array in v9fs_walk()
> >>>>>>   
> >>>>>>   fsdev/9p-marshal.c |   2 +
> >>>>>>   fsdev/9p-marshal.h |   3 +
> >>>>>>   fsdev/file-op-9p.h |   2 +
> >>>>>>   fsdev/p9array.h    | 160
> >>>>>> 
> >>>>>> +++++++++++++++++++++++++++++++++++++++++++++++++++++ hw/9pfs/9p.c
> >>>>>> 
> >>>>>> 70 +++++++++++++----------
> >>>>>> 
> >>>>>>   5 files changed, 208 insertions(+), 29 deletions(-)
> >>>>>>   create mode 100644 fsdev/p9array.h
> >>>>> 
> >>>>> Regarding last 5 patches: Daniel raised a concern that not using
> >>>>> g_autoptr
> >>>>> would deviate from current QEMU coding patterns:
> >>>>> https://lists.gnu.org/archive/html/qemu-devel/2021-10/msg00081.html
> >>>>> 
> >>>>> Unfortunately I saw no way to address his concern without adding
> >>>>> unnecessary code complexity, so I decided to make this a 9p local type
> >>>>> (QArray -> P9Array) for now, which can easily be replaced in future
> >>>>> (e.g.
> >>>>> when there will be something appropriate on glib side).
> >>>> 
> >>>> Hmm various patches aren't reviewed yet... In particular
> >>>> patch #5 has a Suggested-by tag without Reviewed-by, this
> >>>> looks odd.
> >>>> 
> >>>> See https://wiki.qemu.org/Contribute/SubmitAPullRequest:
> >>>>    Don't send pull requests for code that hasn't passed review.
> >>>>    A pull request says these patches are ready to go into QEMU now,
> >>>>    so they must have passed the standard code review processes. In
> >>>>    particular if you've corrected issues in one round of code review,
> >>>>    you need to send your fixed patch series as normal to the list;
> >>>>    you can't put it in a pull request until it's gone through.
> >>>>    (Extremely trivial fixes may be OK to just fix in passing, but
> >>>>    if in doubt err on the side of not.)
> >>> 
> >>> There are in general exactly two persons adding their RBs to 9p patches,
> >>> which is either Greg or me, and Greg made it already clear that he
> >>> barely
> >>> has time for anything above trivial set.
> >>> 
> >>> So what do you suggest? You want to participate and review 9p patches?
> >> 
> >> Well I am a bit surprised...
> >> 
> >> $ git log --oneline \
> >> 
> >>      --grep='Reviewed-by: Philippe Mathieu-Daudé' -- hw/9pfs/ | wc -l
> >> 
> >> 18
> >> 
> >> I also reviewed patch #3 if this pull request...
> >> 
> >> 
> >> Now I see you posted this 4 times in 2 months, so indeed eventual
> >> reviewers had plenty of time to look at your patches.
> >> 
> >> Note I haven't said I'd NAck your pull request, I noticed your own
> >> concern wrt Daniel comment, so I looked at the patch and realized
> >> it was not reviewed, and simply said this is this is odd.
> >> 
> >> Regards,
> >> 
> >> Phil.
> > 
> > Philippe, of course I understand why this looks odd to you, and yes you
> > reviewed that particular patch. But the situation on the 9p front is like
> > this for >2 years now: people quickly come by to nack patches, but even
> > after having addressed their concerns they barely add their RBs
> > afterwards. That means I am currently forced to send out PRs without RBs
> > once in a while.
> In know the feeling -- it takes quite some prodding to get tcg patches
> reviewed, and I have also sent out PRs that are incompletely reviewed.
> 
> I see that patch 5 was something I suggested myself, which I then failed to
> review afterward.  In recompense, I have reviewed the whole patch set:
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> I think the P9Array is fairly clever, and I do prefer it over glib ugliness.
>  I can imagine only C++ being an improvement over what you've created.
> 
> Rather than force you to re-create the PR, I'll simply apply this along with
> the S-o-b, to the merge object.
> 
> 
> r~

Thanks Richard, I highly appreciate that!

Best regards,
Christian Schoenebeck