[PATCH 0/5] 9pfs: introduce P9Array

[PATCH 0/5] 9pfs: introduce P9Array

[Christian Schoenebeck]

This is simply a refactored follow-up of the following previous series
("introduce QArray"):
https://lists.gnu.org/archive/html/qemu-devel/2021-08/msg04530.html

There was no consensus about a project wide "QArray" shared utility code,
nor does there seem to be a need for a project wide code, so let's just
refactor QArray -> P9Array and make it a 9P local type for now:
https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg07743.html

Changes QArray v3 -> P9Array v1:

  * Moved header file, i.e. include/qemu/qarray.h -> fsdev/p9array.h

  * Refactored *QArray* -> *P9Array*

  * Refactored P9ARRAY_CREATE -> P9ARRAY_NEW and
    p9array_create_* -> p9array_new_* accordingly

  * Refactored DECLARE_P9ARRAY_TYPE -> P9ARRAY_DECLARE_TYPE

  * Refactored DEFINE_P9ARRAY_TYPE -> P9ARRAY_DEFINE_TYPE

  [No behaviour changes whatsoever]

Christian Schoenebeck (5):
  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       |  19 ++----
 5 files changed, 174 insertions(+), 12 deletions(-)
 create mode 100644 fsdev/p9array.h

-- 
2.20.1

[PATCH 1/5] 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>
---
 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

Re: [PATCH 1/5] 9pfs: introduce P9Array

[Daniel P. Berrangé]

On Fri, Oct 01, 2021 at 04:26:17PM +0200, Christian Schoenebeck wrote:
> 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 */
>   }

As explained before, I'm against the idea of introducing new ways
to automatically free local variables that are not using g_auto*
functionality. It is not following the QEMU wide coding style
that is documented.


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

Re: [PATCH 1/5] 9pfs: introduce P9Array

[Christian Schoenebeck]

On Freitag, 1. Oktober 2021 17:16:47 CEST Daniel P. Berrangé wrote:
> On Fri, Oct 01, 2021 at 04:26:17PM +0200, Christian Schoenebeck wrote:
> > 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 */
> >   
> >   }
> 
> As explained before, I'm against the idea of introducing new ways
> to automatically free local variables that are not using g_auto*
> functionality. It is not following the QEMU wide coding style
> that is documented.

Yes, your concerns are linked in the cover letter. And I also made it clear 
that what you suggested does not fit either. So my position has not changed.

Best regards,
Christian Schoenebeck

[PATCH 2/5] 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>
---
 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

[PATCH 3/5] 9pfs: make V9fsString usable via P9Array 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..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

[PATCH 4/5] 9pfs: make V9fsPath usable via P9Array 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..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 c857b31321..e432c4c007 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

[PATCH 5/5] 9pfs: use P9Array 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 e432c4c007..27c4b8ba78 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;
+    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;
@@ -1734,10 +1735,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) {
@@ -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 0/5] 9pfs: introduce P9Array

[Christian Schoenebeck]

On Freitag, 1. Oktober 2021 16:25:22 CEST Christian Schoenebeck wrote:
> This is simply a refactored follow-up of the following previous series
> ("introduce QArray"):
> https://lists.gnu.org/archive/html/qemu-devel/2021-08/msg04530.html
> 
> There was no consensus about a project wide "QArray" shared utility code,
> nor does there seem to be a need for a project wide code, so let's just
> refactor QArray -> P9Array and make it a 9P local type for now:
> https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg07743.html
> 
> Changes QArray v3 -> P9Array v1:
> 
>   * Moved header file, i.e. include/qemu/qarray.h -> fsdev/p9array.h
> 
>   * Refactored *QArray* -> *P9Array*
> 
>   * Refactored P9ARRAY_CREATE -> P9ARRAY_NEW and
>     p9array_create_* -> p9array_new_* accordingly
> 
>   * Refactored DECLARE_P9ARRAY_TYPE -> P9ARRAY_DECLARE_TYPE
> 
>   * Refactored DEFINE_P9ARRAY_TYPE -> P9ARRAY_DEFINE_TYPE
> 
>   [No behaviour changes whatsoever]
> 
> Christian Schoenebeck (5):
>   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       |  19 ++----
>  5 files changed, 174 insertions(+), 12 deletions(-)
>  create mode 100644 fsdev/p9array.h

Queued on 9p.next:
https://github.com/cschoenebeck/qemu/commits/9p.next

Thanks!

Best regards,
Christian Schoenebeck

Best regards,
Christian Schoenebeck