[PATCH v3 0/5] introduce QArray

[PATCH v3 0/5] introduce QArray

[Christian Schoenebeck]

Patches 1 and 2 introduce include/qemu/qarray.h which implements a deep auto
free mechanism for arrays. See commit log of patch 1 for a detailed
explanation and motivation for introducing QArray.

Patches 3..5 are provided (e.g. as example) for 9p being the first user of
this new QArray API. These particular patches 3..5 are rebased on my
current 9p queue: https://github.com/cschoenebeck/qemu/commits/9p.next
which are basically just the following two queued patches:

https://github.com/cschoenebeck/qemu/commit/7772715d43908235940f5b7dec68d0458b1ccdf4
https://github.com/cschoenebeck/qemu/commit/838b55e392ea7d52e714fdba1db777f658aee2cc

v2 -> v3:

    * Refactor QArrayRef() -> QARRAY_REF() [patch 1], [patch 5].

    * Commit log: Add more thorough explanation for the motivation of QArray,
      along with example for advantage over GArray [patch 1].

    * Commit log: Add reason for using MIT license for qarray.h instead of
      the standard QEMU license GPLv2+ [patch 1].

    * API doc comments: use 'size_t' type consistently in API doc example
      code [patch 1].

v1 -> v2:

    * Minor API comment changes [patch 1].

    * Perform strong type check by using __builtin_types_compatible_p()
      instead of a weak check using sizeof() [patch 2].

Christian Schoenebeck (5):
  qemu/qarray.h: introduce QArray
  qemu/qarray.h: check scalar type in QARRAY_CREATE()
  9pfs: make V9fsString usable via QArray API
  9pfs: make V9fsPath usable via QArray API
  9pfs: use QArray in v9fs_walk()

 fsdev/9p-marshal.c    |   2 +
 fsdev/9p-marshal.h    |   3 +
 fsdev/file-op-9p.h    |   2 +
 hw/9pfs/9p.c          |  19 ++---
 include/qemu/qarray.h | 160 ++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 174 insertions(+), 12 deletions(-)
 create mode 100644 include/qemu/qarray.h

-- 
2.20.1

[PATCH v3 1/5] qemu/qarray.h: introduce QArray

[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) {
      QARRAY_REF(Foo) foos = NULL;
      QARRAY_CREATE(Foo, foos, n);
      for (...) {
          foos[i].s = malloc(...);
          if (...) {
              return; /* array auto freed here */
          }
      }
      /* array auto freed here */
  }

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

  C-array = QArray:    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 QArray 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 QArray##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 QArray 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>
---
 include/qemu/qarray.h | 154 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 154 insertions(+)
 create mode 100644 include/qemu/qarray.h

diff --git a/include/qemu/qarray.h b/include/qemu/qarray.h
new file mode 100644
index 0000000000..25af97a8a6
--- /dev/null
+++ b/include/qemu/qarray.h
@@ -0,0 +1,154 @@
+/*
+ * QArray - 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_QARRAY_H
+#define QEMU_QARRAY_H
+
+/**
+ * QArray 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
+ * DECLARE_QARRAY_TYPE(Foo);
+ * @endcode
+ * and the following to a C unit file:
+ * @code
+ * DEFINE_QARRAY_TYPE(Foo, free_foo);
+ * @endcode
+ * Finally the array may then be used like this:
+ * @code
+ * void doSomething(size_t n) {
+ *     QARRAY_REF(Foo) foos = NULL;
+ *     QARRAY_CREATE(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 DECLARE_QARRAY_TYPE(scalar_type) \
+    typedef struct QArray##scalar_type { \
+        size_t len; \
+        scalar_type first[]; \
+    } QArray##scalar_type; \
+    \
+    void qarray_create_##scalar_type(scalar_type **auto_var, size_t len); \
+    void qarray_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 DEFINE_QARRAY_TYPE(scalar_type, scalar_cleanup_func) \
+    void qarray_create_##scalar_type(scalar_type **auto_var, size_t len) \
+    { \
+        qarray_auto_free_##scalar_type(auto_var); \
+        QArray##scalar_type *arr = g_malloc0(sizeof(QArray##scalar_type) + \
+            len * sizeof(scalar_type)); \
+        arr->len = len; \
+        *auto_var = &arr->first[0]; \
+    } \
+    \
+    void qarray_auto_free_##scalar_type(scalar_type **auto_var) \
+    { \
+        scalar_type *first = (*auto_var); \
+        if (!first) { \
+            return; \
+        } \
+        QArray##scalar_type *arr = (QArray##scalar_type *) ( \
+            ((char *)first) - offsetof(QArray##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 QARRAY_REF(scalar_type) \
+    __attribute((__cleanup__(qarray_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 QARRAY_CREATE(scalar_type, auto_var, len) \
+    qarray_create_##scalar_type((&auto_var), len)
+
+#endif /* QEMU_QARRAY_H */
-- 
2.20.1

[PATCH v3 2/5] qemu/qarray.h: check scalar type in QARRAY_CREATE()

[Christian Schoenebeck]

Make sure at compile time that the scalar type of the array
requested to be created via QARRAY_CREATE() 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>
---
 include/qemu/qarray.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/qemu/qarray.h b/include/qemu/qarray.h
index 25af97a8a6..7adf28c03c 100644
--- a/include/qemu/qarray.h
+++ b/include/qemu/qarray.h
@@ -27,6 +27,8 @@
 #ifndef QEMU_QARRAY_H
 #define QEMU_QARRAY_H
 
+#include "qemu/compiler.h"
+
 /**
  * QArray 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 QARRAY_CREATE(scalar_type, auto_var, len) \
+    QEMU_BUILD_BUG_MSG( \
+        !__builtin_types_compatible_p(scalar_type, typeof(*auto_var)), \
+        "QArray scalar type mismatch" \
+    ); \
     qarray_create_##scalar_type((&auto_var), len)
 
 #endif /* QEMU_QARRAY_H */
-- 
2.20.1

[PATCH v3 3/5] 9pfs: make V9fsString usable via QArray API

[Christian Schoenebeck]

Signed-off-by: Christian Schoenebeck <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..fbfc2a62cd 100644
--- a/fsdev/9p-marshal.c
+++ b/fsdev/9p-marshal.c
@@ -18,6 +18,8 @@
 
 #include "9p-marshal.h"
 
+DEFINE_QARRAY_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..7229e4e617 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 "qemu/qarray.h"
+
 typedef struct V9fsString {
     uint16_t size;
     char *data;
 } V9fsString;
+DECLARE_QARRAY_TYPE(V9fsString);
 
 typedef struct V9fsQID {
     uint8_t type;
-- 
2.20.1

[PATCH v3 4/5] 9pfs: make V9fsPath usable via QArray API

[Christian Schoenebeck]

Signed-off-by: Christian Schoenebeck <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..7630f0e538 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 "qemu/qarray.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;
 };
+DECLARE_QARRAY_TYPE(V9fsPath);
 
 typedef union V9fsFidOpenState V9fsFidOpenState;
 
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index c857b31321..b59572fa79 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -50,6 +50,8 @@ enum {
     Oappend = 0x80,
 };
 
+DEFINE_QARRAY_TYPE(V9fsPath, v9fs_path_free);
+
 static ssize_t pdu_marshal(V9fsPDU *pdu, size_t offset, const char *fmt, ...)
 {
     ssize_t ret;
-- 
2.20.1

[PATCH v3 5/5] 9pfs: use QArray in v9fs_walk()

[Christian Schoenebeck]

Signed-off-by: Christian Schoenebeck <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 b59572fa79..9275c23df0 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1707,13 +1707,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;
+    QARRAY_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;
+    QARRAY_REF(V9fsString) wnames = NULL;
     V9fsFidState *fidp;
     V9fsFidState *newfidp = NULL;
     V9fsPDU *pdu = opaque;
@@ -1734,10 +1735,10 @@ static void coroutine_fn v9fs_walk(void *opaque)
         goto out_nofid;
     }
     if (nwnames) {
-        wnames = g_new0(V9fsString, nwnames);
+        QARRAY_CREATE(V9fsString, wnames, nwnames);
         qids   = g_new0(V9fsQID, nwnames);
         stbufs = g_new0(struct stat, nwnames);
-        pathes = g_new0(V9fsPath, nwnames);
+        QARRAY_CREATE(V9fsPath, pathes, nwnames);
         for (i = 0; i < nwnames; i++) {
             err = pdu_unmarshal(pdu, offset, "s", &wnames[i]);
             if (err < 0) {
@@ -1869,14 +1870,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: [PATCH v3 0/5] introduce QArray

[Greg Kurz]

On Thu, 26 Aug 2021 14:47:26 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> Patches 1 and 2 introduce include/qemu/qarray.h which implements a deep auto
> free mechanism for arrays. See commit log of patch 1 for a detailed
> explanation and motivation for introducing QArray.
> 
> Patches 3..5 are provided (e.g. as example) for 9p being the first user of
> this new QArray API. These particular patches 3..5 are rebased on my
> current 9p queue: https://github.com/cschoenebeck/qemu/commits/9p.next
> which are basically just the following two queued patches:
> 

This looks nice indeed but I have the impression the same could be
achieved using glib's g_autoptr framework with less code being added
to QEMU (at the cost of being less generic maybe).

Anyway, we should likely sort out the SEGV issue you're hitting
before going forward with supplementary changes in v9fs_walk().

> https://github.com/cschoenebeck/qemu/commit/7772715d43908235940f5b7dec68d0458b1ccdf4
> https://github.com/cschoenebeck/qemu/commit/838b55e392ea7d52e714fdba1db777f658aee2cc
> 
> v2 -> v3:
> 
>     * Refactor QArrayRef() -> QARRAY_REF() [patch 1], [patch 5].
> 
>     * Commit log: Add more thorough explanation for the motivation of QArray,
>       along with example for advantage over GArray [patch 1].
> 
>     * Commit log: Add reason for using MIT license for qarray.h instead of
>       the standard QEMU license GPLv2+ [patch 1].
> 
>     * API doc comments: use 'size_t' type consistently in API doc example
>       code [patch 1].
> 
> v1 -> v2:
> 
>     * Minor API comment changes [patch 1].
> 
>     * Perform strong type check by using __builtin_types_compatible_p()
>       instead of a weak check using sizeof() [patch 2].
> 
> Christian Schoenebeck (5):
>   qemu/qarray.h: introduce QArray
>   qemu/qarray.h: check scalar type in QARRAY_CREATE()
>   9pfs: make V9fsString usable via QArray API
>   9pfs: make V9fsPath usable via QArray API
>   9pfs: use QArray in v9fs_walk()
> 
>  fsdev/9p-marshal.c    |   2 +
>  fsdev/9p-marshal.h    |   3 +
>  fsdev/file-op-9p.h    |   2 +
>  hw/9pfs/9p.c          |  19 ++---
>  include/qemu/qarray.h | 160 ++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 174 insertions(+), 12 deletions(-)
>  create mode 100644 include/qemu/qarray.h
> 

Re: [PATCH v3 0/5] introduce QArray

[Christian Schoenebeck]

On Dienstag, 31. August 2021 13:58:02 CEST Greg Kurz wrote:
> On Thu, 26 Aug 2021 14:47:26 +0200
> 
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > Patches 1 and 2 introduce include/qemu/qarray.h which implements a deep
> > auto free mechanism for arrays. See commit log of patch 1 for a detailed
> > explanation and motivation for introducing QArray.
> > 
> > Patches 3..5 are provided (e.g. as example) for 9p being the first user of
> > this new QArray API. These particular patches 3..5 are rebased on my
> > current 9p queue: https://github.com/cschoenebeck/qemu/commits/9p.next
> 
> > which are basically just the following two queued patches:
> This looks nice indeed but I have the impression the same could be
> achieved using glib's g_autoptr framework with less code being added
> to QEMU (at the cost of being less generic maybe).

I haven't seen a way doing this with glib, except of GArray which has some 
disadvantages. But who knows, maybe I was missing something.

> Anyway, we should likely sort out the SEGV issue you're hitting
> before going forward with supplementary changes in v9fs_walk().

Yeah, let's postpone this series here. I'll look into the Twalk crash issue 
more closely today and will get back on that issue first.

Best regards,
Christian Schoenebeck

Re: [PATCH v3 0/5] introduce QArray

[Christian Schoenebeck]

On Dienstag, 31. August 2021 14:25:04 CEST Christian Schoenebeck wrote:
> On Dienstag, 31. August 2021 13:58:02 CEST Greg Kurz wrote:
> > On Thu, 26 Aug 2021 14:47:26 +0200
> > 
> > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > > Patches 1 and 2 introduce include/qemu/qarray.h which implements a deep
> > > auto free mechanism for arrays. See commit log of patch 1 for a detailed
> > > explanation and motivation for introducing QArray.
> > > 
> > > Patches 3..5 are provided (e.g. as example) for 9p being the first user
> > > of
> > > this new QArray API. These particular patches 3..5 are rebased on my
> > > current 9p queue: https://github.com/cschoenebeck/qemu/commits/9p.next
> > 
> > > which are basically just the following two queued patches:
> > This looks nice indeed but I have the impression the same could be
> > achieved using glib's g_autoptr framework with less code being added
> > to QEMU (at the cost of being less generic maybe).
> 
> I haven't seen a way doing this with glib, except of GArray which has some
> disadvantages. But who knows, maybe I was missing something.

Ping

Let's do this?

> > Anyway, we should likely sort out the SEGV issue you're hitting
> > before going forward with supplementary changes in v9fs_walk().
> 
> Yeah, let's postpone this series here. I'll look into the Twalk crash issue
> more closely today and will get back on that issue first.
> 
> Best regards,
> Christian Schoenebeck

Re: [PATCH v3 0/5] introduce QArray

[Greg Kurz]

On Mon, 27 Sep 2021 12:35:16 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Dienstag, 31. August 2021 14:25:04 CEST Christian Schoenebeck wrote:
> > On Dienstag, 31. August 2021 13:58:02 CEST Greg Kurz wrote:
> > > On Thu, 26 Aug 2021 14:47:26 +0200
> > > 
> > > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > > > Patches 1 and 2 introduce include/qemu/qarray.h which implements a deep
> > > > auto free mechanism for arrays. See commit log of patch 1 for a detailed
> > > > explanation and motivation for introducing QArray.
> > > > 
> > > > Patches 3..5 are provided (e.g. as example) for 9p being the first user
> > > > of
> > > > this new QArray API. These particular patches 3..5 are rebased on my
> > > > current 9p queue: https://github.com/cschoenebeck/qemu/commits/9p.next
> > > 
> > > > which are basically just the following two queued patches:
> > > This looks nice indeed but I have the impression the same could be
> > > achieved using glib's g_autoptr framework with less code being added
> > > to QEMU (at the cost of being less generic maybe).
> > 
> > I haven't seen a way doing this with glib, except of GArray which has some
> > disadvantages. But who knows, maybe I was missing something.
> 
> Ping
> 
> Let's do this?
> 

Hi Christian,

Sorry I don't have enough bandwidth to review or to look for an alternate
way... :-\ So I suggest you just go forward with this series. Hopefully
you can get some reviews from Markus and/or Richard.

Cheers,

--
Greg

> > > Anyway, we should likely sort out the SEGV issue you're hitting
> > > before going forward with supplementary changes in v9fs_walk().
> > 
> > Yeah, let's postpone this series here. I'll look into the Twalk crash issue
> > more closely today and will get back on that issue first.
> > 
> > Best regards,
> > Christian Schoenebeck
> 
> 

Re: [PATCH v3 0/5] introduce QArray

[Christian Schoenebeck]

On Montag, 27. September 2021 12:59:40 CEST Greg Kurz wrote:
> On Mon, 27 Sep 2021 12:35:16 +0200
> 
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > On Dienstag, 31. August 2021 14:25:04 CEST Christian Schoenebeck wrote:
> > > On Dienstag, 31. August 2021 13:58:02 CEST Greg Kurz wrote:
> > > > On Thu, 26 Aug 2021 14:47:26 +0200
> > > > 
> > > > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > > > > Patches 1 and 2 introduce include/qemu/qarray.h which implements a
> > > > > deep
> > > > > auto free mechanism for arrays. See commit log of patch 1 for a
> > > > > detailed
> > > > > explanation and motivation for introducing QArray.
> > > > > 
> > > > > Patches 3..5 are provided (e.g. as example) for 9p being the first
> > > > > user
> > > > > of
> > > > > this new QArray API. These particular patches 3..5 are rebased on my
> > > > > current 9p queue:
> > > > > https://github.com/cschoenebeck/qemu/commits/9p.next
> > > > 
> > > > > which are basically just the following two queued patches:
> > > > This looks nice indeed but I have the impression the same could be
> > > > achieved using glib's g_autoptr framework with less code being added
> > > > to QEMU (at the cost of being less generic maybe).
> > > 
> > > I haven't seen a way doing this with glib, except of GArray which has
> > > some
> > > disadvantages. But who knows, maybe I was missing something.
> > 
> > Ping
> > 
> > Let's do this?
> 
> Hi Christian,
> 
> Sorry I don't have enough bandwidth to review or to look for an alternate
> way... :-\ So I suggest you just go forward with this series. Hopefully
> you can get some reviews from Markus and/or Richard.

Ok, then I wait for few more days, and if there are no repsonses, nor vetos 
then I'll queue these patches anyway.

Best regards,
Christian Schoenebeck

Re: [PATCH v3 0/5] introduce QArray

[Alex Bennée]

Christian Schoenebeck <qemu_oss@crudebyte.com> writes:

> On Montag, 27. September 2021 12:59:40 CEST Greg Kurz wrote:
>> On Mon, 27 Sep 2021 12:35:16 +0200
>> 
>> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
>> > On Dienstag, 31. August 2021 14:25:04 CEST Christian Schoenebeck wrote:
>> > > On Dienstag, 31. August 2021 13:58:02 CEST Greg Kurz wrote:
>> > > > On Thu, 26 Aug 2021 14:47:26 +0200
>> > > > 
>> > > > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
>> > > > > Patches 1 and 2 introduce include/qemu/qarray.h which implements a
>> > > > > deep
>> > > > > auto free mechanism for arrays. See commit log of patch 1 for a
>> > > > > detailed
>> > > > > explanation and motivation for introducing QArray.
>> > > > > 
>> > > > > Patches 3..5 are provided (e.g. as example) for 9p being the first
>> > > > > user
>> > > > > of
>> > > > > this new QArray API. These particular patches 3..5 are rebased on my
>> > > > > current 9p queue:
>> > > > > https://github.com/cschoenebeck/qemu/commits/9p.next
>> > > > 
>> > > > > which are basically just the following two queued patches:
>> > > > This looks nice indeed but I have the impression the same could be
>> > > > achieved using glib's g_autoptr framework with less code being added
>> > > > to QEMU (at the cost of being less generic maybe).
>> > > 
>> > > I haven't seen a way doing this with glib, except of GArray which has
>> > > some
>> > > disadvantages. But who knows, maybe I was missing something.
>> > 
>> > Ping
>> > 
>> > Let's do this?
>> 
>> Hi Christian,
>> 
>> Sorry I don't have enough bandwidth to review or to look for an alternate
>> way... :-\ So I suggest you just go forward with this series. Hopefully
>> you can get some reviews from Markus and/or Richard.
>
> Ok, then I wait for few more days, and if there are no repsonses, nor vetos 
> then I'll queue these patches anyway.

As far as I can make out the main argument for introducing a QEMU
specific array handler is to avoid needing to use g_array_index() to
reference the elements of the array. This in itself doesn't seem enough
justification to me.

I also see you handle deep frees which I admit is not something I've
really done with GArray as usually I'm using it when I want to have
everything local to each other.

>
> Best regards,
> Christian Schoenebeck


-- 
Alex Bennée

Re: [PATCH v3 0/5] introduce QArray

[Christian Schoenebeck]

On Dienstag, 28. September 2021 15:37:45 CEST Alex Bennée wrote:
> Christian Schoenebeck <qemu_oss@crudebyte.com> writes:
> > On Montag, 27. September 2021 12:59:40 CEST Greg Kurz wrote:
> >> On Mon, 27 Sep 2021 12:35:16 +0200
> >> 
> >> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> >> > On Dienstag, 31. August 2021 14:25:04 CEST Christian Schoenebeck wrote:
> >> > > On Dienstag, 31. August 2021 13:58:02 CEST Greg Kurz wrote:
> >> > > > On Thu, 26 Aug 2021 14:47:26 +0200
> >> > > > 
> >> > > > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> >> > > > > Patches 1 and 2 introduce include/qemu/qarray.h which implements
> >> > > > > a
> >> > > > > deep
> >> > > > > auto free mechanism for arrays. See commit log of patch 1 for a
> >> > > > > detailed
> >> > > > > explanation and motivation for introducing QArray.
> >> > > > > 
> >> > > > > Patches 3..5 are provided (e.g. as example) for 9p being the
> >> > > > > first
> >> > > > > user
> >> > > > > of
> >> > > > > this new QArray API. These particular patches 3..5 are rebased on
> >> > > > > my
> >> > > > > current 9p queue:
> >> > > > > https://github.com/cschoenebeck/qemu/commits/9p.next
> >> > > > 
> >> > > > > which are basically just the following two queued patches:
> >> > > > This looks nice indeed but I have the impression the same could be
> >> > > > achieved using glib's g_autoptr framework with less code being
> >> > > > added
> >> > > > to QEMU (at the cost of being less generic maybe).
> >> > > 
> >> > > I haven't seen a way doing this with glib, except of GArray which has
> >> > > some
> >> > > disadvantages. But who knows, maybe I was missing something.
> >> > 
> >> > Ping
> >> > 
> >> > Let's do this?
> >> 
> >> Hi Christian,
> >> 
> >> Sorry I don't have enough bandwidth to review or to look for an alternate
> >> way... :-\ So I suggest you just go forward with this series. Hopefully
> >> you can get some reviews from Markus and/or Richard.
> > 
> > Ok, then I wait for few more days, and if there are no repsonses, nor
> > vetos
> > then I'll queue these patches anyway.
> 
> As far as I can make out the main argument for introducing a QEMU
> specific array handler is to avoid needing to use g_array_index() to
> reference the elements of the array. This in itself doesn't seem enough
> justification to me.
> 
> I also see you handle deep frees which I admit is not something I've
> really done with GArray as usually I'm using it when I want to have
> everything local to each other.

The main motivation for this API is to simplify and reduce code, which also 
increases safety.

Most of the suggested header file are really just comments. If you strip those 
comments away, you actually only have a few lines of code left.

There are user code functions which are already complex enough on its own, 
where I don't want to additionally need to think about for *each* individual 
branch whether I need to free something here and there, and if yes what 
exactly, how deep, and whether there are any exceptions I have to take care 
about, etc.

Best regards,
Christian Schoenebeck

Re: [PATCH v3 0/5] introduce QArray

[Daniel P. Berrangé]

On Tue, Sep 28, 2021 at 02:25:57PM +0200, Christian Schoenebeck wrote:
> On Montag, 27. September 2021 12:59:40 CEST Greg Kurz wrote:
> > On Mon, 27 Sep 2021 12:35:16 +0200
> > 
> > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > > On Dienstag, 31. August 2021 14:25:04 CEST Christian Schoenebeck wrote:
> > > > On Dienstag, 31. August 2021 13:58:02 CEST Greg Kurz wrote:
> > > > > On Thu, 26 Aug 2021 14:47:26 +0200
> > > > > 
> > > > > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > > > > > Patches 1 and 2 introduce include/qemu/qarray.h which implements a
> > > > > > deep
> > > > > > auto free mechanism for arrays. See commit log of patch 1 for a
> > > > > > detailed
> > > > > > explanation and motivation for introducing QArray.
> > > > > > 
> > > > > > Patches 3..5 are provided (e.g. as example) for 9p being the first
> > > > > > user
> > > > > > of
> > > > > > this new QArray API. These particular patches 3..5 are rebased on my
> > > > > > current 9p queue:
> > > > > > https://github.com/cschoenebeck/qemu/commits/9p.next
> > > > > 
> > > > > > which are basically just the following two queued patches:
> > > > > This looks nice indeed but I have the impression the same could be
> > > > > achieved using glib's g_autoptr framework with less code being added
> > > > > to QEMU (at the cost of being less generic maybe).
> > > > 
> > > > I haven't seen a way doing this with glib, except of GArray which has
> > > > some
> > > > disadvantages. But who knows, maybe I was missing something.
> > > 
> > > Ping
> > > 
> > > Let's do this?
> > 
> > Hi Christian,
> > 
> > Sorry I don't have enough bandwidth to review or to look for an alternate
> > way... :-\ So I suggest you just go forward with this series. Hopefully
> > you can get some reviews from Markus and/or Richard.
> 
> Ok, then I wait for few more days, and if there are no repsonses, nor vetos 
> then I'll queue these patches anyway.

I'm in favour of the general idea of an QArray concept, as i don't think
GArray quite does enough, but I've made suggestions on how I think QArray
needs to be improved first.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|