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

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
> 

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

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

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

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

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

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

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