Suggested-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 hw/9pfs/9p.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 4d642ab12a..c857b31321 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1703,11 +1703,12 @@ static bool same_stat_id(const struct stat *a, const struct stat *b)
 static void coroutine_fn v9fs_walk(void *opaque)
 {
     int name_idx;
-    V9fsQID *qids = NULL;
+    g_autofree V9fsQID *qids = NULL;
     int i, err = 0;
     V9fsPath dpath, path, *pathes = NULL;
     uint16_t nwnames;
-    struct stat stbuf, fidst, *stbufs = NULL;
+    struct stat stbuf, fidst;
+    g_autofree struct stat *stbufs = NULL;
     size_t offset = 7;
     int32_t fid, newfid;
     V9fsString *wnames = NULL;
@@ -1872,8 +1873,6 @@ out_nofid:
             v9fs_path_free(&pathes[name_idx]);
         }
         g_free(wnames);
-        g_free(qids);
-        g_free(stbufs);
         g_free(pathes);
     }
 }
-- 
2.20.1

On 8/17/21 3:46 PM, Christian Schoenebeck wrote:
> Suggested-by: Greg Kurz <groug@kaod.org>
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---
>  hw/9pfs/9p.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

On Tue, 17 Aug 2021 15:46:50 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> Suggested-by: Greg Kurz <groug@kaod.org>
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---
>  hw/9pfs/9p.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 4d642ab12a..c857b31321 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -1703,11 +1703,12 @@ static bool same_stat_id(const struct stat *a, const struct stat *b)
>  static void coroutine_fn v9fs_walk(void *opaque)
>  {
>      int name_idx;
> -    V9fsQID *qids = NULL;
> +    g_autofree V9fsQID *qids = NULL;
>      int i, err = 0;
>      V9fsPath dpath, path, *pathes = NULL;
>      uint16_t nwnames;
> -    struct stat stbuf, fidst, *stbufs = NULL;
> +    struct stat stbuf, fidst;
> +    g_autofree struct stat *stbufs = NULL;
>      size_t offset = 7;
>      int32_t fid, newfid;
>      V9fsString *wnames = NULL;
> @@ -1872,8 +1873,6 @@ out_nofid:
>              v9fs_path_free(&pathes[name_idx]);
>          }
>          g_free(wnames);
> -        g_free(qids);
> -        g_free(stbufs);
>          g_free(pathes);

It seems that wnames and pathes could be converted to
g_autofree as well or I'm missing something ?

Feel free to add my R-b with or without that extra change.

Reviewed-by: Greg Kurz <groug@kaod.org>

>      }
>  }

On Freitag, 20. August 2021 12:40:31 CEST Greg Kurz wrote:
> On Tue, 17 Aug 2021 15:46:50 +0200
> 
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > Suggested-by: Greg Kurz <groug@kaod.org>
> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > ---
> > 
> >  hw/9pfs/9p.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > index 4d642ab12a..c857b31321 100644
> > --- a/hw/9pfs/9p.c
> > +++ b/hw/9pfs/9p.c
> > @@ -1703,11 +1703,12 @@ static bool same_stat_id(const struct stat *a,
> > const struct stat *b)> 
> >  static void coroutine_fn v9fs_walk(void *opaque)
> >  {
> >  
> >      int name_idx;
> > 
> > -    V9fsQID *qids = NULL;
> > +    g_autofree V9fsQID *qids = NULL;
> > 
> >      int i, err = 0;
> >      V9fsPath dpath, path, *pathes = NULL;
> >      uint16_t nwnames;
> > 
> > -    struct stat stbuf, fidst, *stbufs = NULL;
> > +    struct stat stbuf, fidst;
> > +    g_autofree struct stat *stbufs = NULL;
> > 
> >      size_t offset = 7;
> >      int32_t fid, newfid;
> >      V9fsString *wnames = NULL;
> > 
> > @@ -1872,8 +1873,6 @@ out_nofid:
> >              v9fs_path_free(&pathes[name_idx]);
> >          
> >          }
> >          g_free(wnames);
> > 
> > -        g_free(qids);
> > -        g_free(stbufs);
> > 
> >          g_free(pathes);
> 
> It seems that wnames and pathes could be converted to
> g_autofree as well or I'm missing something ?

Yeah, I mentioned that in the cover letter. Those two are omitted in this 
patch because they contain dynamically allocated memory per array element 
which need to be freed individually before freeing the array.

So those two would probably require custom cleanup handlers.

> 
> Feel free to add my R-b with or without that extra change.
> 
> Reviewed-by: Greg Kurz <groug@kaod.org>

Thanks!

> 
> >      }
> >  
> >  }

Best regards,
Christian Schoenebeck

On Fri, 20 Aug 2021 14:23:26 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Freitag, 20. August 2021 12:40:31 CEST Greg Kurz wrote:
> > On Tue, 17 Aug 2021 15:46:50 +0200
> > 
> > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > > Suggested-by: Greg Kurz <groug@kaod.org>
> > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > > ---
> > > 
> > >  hw/9pfs/9p.c | 7 +++----
> > >  1 file changed, 3 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > > index 4d642ab12a..c857b31321 100644
> > > --- a/hw/9pfs/9p.c
> > > +++ b/hw/9pfs/9p.c
> > > @@ -1703,11 +1703,12 @@ static bool same_stat_id(const struct stat *a,
> > > const struct stat *b)> 
> > >  static void coroutine_fn v9fs_walk(void *opaque)
> > >  {
> > >  
> > >      int name_idx;
> > > 
> > > -    V9fsQID *qids = NULL;
> > > +    g_autofree V9fsQID *qids = NULL;
> > > 
> > >      int i, err = 0;
> > >      V9fsPath dpath, path, *pathes = NULL;
> > >      uint16_t nwnames;
> > > 
> > > -    struct stat stbuf, fidst, *stbufs = NULL;
> > > +    struct stat stbuf, fidst;
> > > +    g_autofree struct stat *stbufs = NULL;
> > > 
> > >      size_t offset = 7;
> > >      int32_t fid, newfid;
> > >      V9fsString *wnames = NULL;
> > > 
> > > @@ -1872,8 +1873,6 @@ out_nofid:
> > >              v9fs_path_free(&pathes[name_idx]);
> > >          
> > >          }
> > >          g_free(wnames);
> > > 
> > > -        g_free(qids);
> > > -        g_free(stbufs);
> > > 
> > >          g_free(pathes);
> > 
> > It seems that wnames and pathes could be converted to
> > g_autofree as well or I'm missing something ?
> 
> Yeah, I mentioned that in the cover letter. Those two are omitted in this 
> patch because they contain dynamically allocated memory per array element 
> which need to be freed individually before freeing the array.
> 
> So those two would probably require custom cleanup handlers.
> 

The freeing of the individual elements is already handled in the loop above
the g_free() calls. The wnames and pathes pointers can thus be treated like
the other ones.

> > 
> > Feel free to add my R-b with or without that extra change.
> > 
> > Reviewed-by: Greg Kurz <groug@kaod.org>
> 
> Thanks!
> 
> > 
> > >      }
> > >  
> > >  }
> 
> Best regards,
> Christian Schoenebeck
> 
> 

On Freitag, 20. August 2021 14:34:11 CEST Greg Kurz wrote:
> On Fri, 20 Aug 2021 14:23:26 +0200
> 
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > On Freitag, 20. August 2021 12:40:31 CEST Greg Kurz wrote:
> > > On Tue, 17 Aug 2021 15:46:50 +0200
> > > 
> > > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > > > Suggested-by: Greg Kurz <groug@kaod.org>
> > > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > > > ---
> > > > 
> > > >  hw/9pfs/9p.c | 7 +++----
> > > >  1 file changed, 3 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > > > index 4d642ab12a..c857b31321 100644
> > > > --- a/hw/9pfs/9p.c
> > > > +++ b/hw/9pfs/9p.c
> > > > @@ -1703,11 +1703,12 @@ static bool same_stat_id(const struct stat *a,
> > > > const struct stat *b)>
> > > > 
> > > >  static void coroutine_fn v9fs_walk(void *opaque)
> > > >  {
> > > >  
> > > >      int name_idx;
> > > > 
> > > > -    V9fsQID *qids = NULL;
> > > > +    g_autofree V9fsQID *qids = NULL;
> > > > 
> > > >      int i, err = 0;
> > > >      V9fsPath dpath, path, *pathes = NULL;
> > > >      uint16_t nwnames;
> > > > 
> > > > -    struct stat stbuf, fidst, *stbufs = NULL;
> > > > +    struct stat stbuf, fidst;
> > > > +    g_autofree struct stat *stbufs = NULL;
> > > > 
> > > >      size_t offset = 7;
> > > >      int32_t fid, newfid;
> > > >      V9fsString *wnames = NULL;
> > > > 
> > > > @@ -1872,8 +1873,6 @@ out_nofid:
> > > >              v9fs_path_free(&pathes[name_idx]);
> > > >          
> > > >          }
> > > >          g_free(wnames);
> > > > 
> > > > -        g_free(qids);
> > > > -        g_free(stbufs);
> > > > 
> > > >          g_free(pathes);
> > > 
> > > It seems that wnames and pathes could be converted to
> > > g_autofree as well or I'm missing something ?
> > 
> > Yeah, I mentioned that in the cover letter. Those two are omitted in this
> > patch because they contain dynamically allocated memory per array element
> > which need to be freed individually before freeing the array.
> > 
> > So those two would probably require custom cleanup handlers.
> 
> The freeing of the individual elements is already handled in the loop above
> the g_free() calls. The wnames and pathes pointers can thus be treated like
> the other ones.

Yes I know. I was just considering to make that in a safer way that would 
allow simple returns in future without goto out_something; branches. But yes, 
as it is right now they could be converted in the exact same way yet.

Best regards,
Christian Schoenebeck