Re: [PATCH] hw/9pfs: add cleanup operation for 9p-synth

[Christian Schoenebeck <qemu_oss@crudebyte.com>]

On Monday, March 31, 2025 3:52:31 PM CEST Zheng Huang wrote:
> Hi,

Hi!

> This patch adds a cleanup operation for 9p-synth, which fixes a memory
> leak bug in synth_init() and other related operations. 

Which other operations?

> All child nodes
> of synth_root need to be freed before the entire filesystem exits.

I assume this is a theoretical fix, because I currently don't see how this
could result in memory being leaked in practice. The synth fs driver is just
used for 9pfs's automated test cases. Shortly after cleanup handler would be
called, the entire process is torn down anyway, and with that all memory
being freed automatically.

> If you have any better ideas for the implementation, please feel free
> to share them.

By using two nested loops in synth_cleanup()? One loop for walking vertically
(child) and one loop for walking horizontally (sibling). Then you could just
open code everything within synth_cleanup() instead.

However I don't see a real reason for this patch in the first place.

> Signed-off-by: Zheng Huang <hz1624917200@gmail.com>
> 
> 
> ---
>  hw/9pfs/9p-synth.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
> index 2abaf3a291..ead8b9e3be 100644
> --- a/hw/9pfs/9p-synth.c
> +++ b/hw/9pfs/9p-synth.c
> @@ -24,6 +24,7 @@
>  #include "qemu/rcu.h"
>  #include "qemu/rcu_queue.h"
>  #include "qemu/cutils.h"
> +#include "qobject/qlist.h"
>  #include "system/qtest.h"
>  
>  /* Root node for synth file system */
> @@ -136,6 +137,19 @@ int qemu_v9fs_synth_add_file(V9fsSynthNode *parent, int mode,
>      return 0;
>  }
>  
> +// Must call after get synth_mutex
> +static void v9fs_recursive_free_node(V9fsSynthNode *node)
> +{
> +    V9fsSynthNode *entry;
> +
> +    for (entry = QLIST_FIRST(&node->child); entry;) {
> +        V9fsSynthNode *next = QLIST_NEXT(entry, sibling);
> +        v9fs_recursive_free_node(entry);
> +        g_free(entry);
> +        entry = next;
> +    }
> +}
> +
>  static void synth_fill_statbuf(V9fsSynthNode *node, struct stat *stbuf)
>  {
>      stbuf->st_dev = 0;
> @@ -615,8 +629,22 @@ static int synth_init(FsContext *ctx, Error **errp)
>      return 0;
>  }
>  
> +
> +static void synth_cleanup(FsContext *ctx)
> +{
> +    // recursively free all child nodes of synth_root
> +    // V9fsSynthNode *tmp;
> +    QEMU_LOCK_GUARD(&synth_mutex);
> +    v9fs_recursive_free_node(&synth_root);
> +    // QLIST_FOREACH(tmp, &synth_root.child, sibling) {
> +    //     v9fs_recursive_free_node(tmp);
> +    // }

No commented code in patch submissions, please.

/Christian

> +    QLIST_INIT(&synth_root.child);
> +}
> +
>  FileOperations synth_ops = {
>      .init         = synth_init,
> +    .cleanup      = synth_cleanup,
>      .lstat        = synth_lstat,
>      .readlink     = synth_readlink,
>      .close        = synth_close,
>