hw/9pfs: add cleanup operation for 9p-synth

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

Posted by Zheng Huang 7 months, 2 weeks ago

Hi,
This patch adds a cleanup operation for 9p-synth, which fixes a memory
leak bug in synth_init() and other related operations. All child nodes
of synth_root need to be freed before the entire filesystem exits.

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

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);
+    // }
+    QLIST_INIT(&synth_root.child);
+}
+
 FileOperations synth_ops = {
     .init         = synth_init,
+    .cleanup      = synth_cleanup,
     .lstat        = synth_lstat,
     .readlink     = synth_readlink,
     .close        = synth_close,
-- 
2.34.1

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

Posted by Christian Schoenebeck 7 months, 2 weeks ago

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

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

Posted by Greg Kurz 7 months, 2 weeks ago

On Fri, 04 Apr 2025 15:59:54 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

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

Hi Zheng !

We certainly appreciate politeness here but we only want relevant
details in what will become the commit message. Anything else
you want to share that doesn't carry useful information about the
patch itself should go below the '---' after your Signed-off-by.

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

FWIW it can be considered a good general practice to match every g_new and
friends with a g_free. This would silent any suspicion of memory leak ;-)

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

We can imagine this could be required if we decide to support hot unplug
of 9p backends but I don't believe this is something we need anytime soon.

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

Cheers,

-- 
Greg