The v9fs_walk() function resolves all client submitted path nodes to the
local 'pathes' array. Using a separate string scalar variable 'path'
inside the background worker thread loop and copying that local 'path'
string scalar variable subsequently to the 'pathes' array (at the end of
each loop iteration) is not necessary.

Instead simply resolve each path directly to the 'pathes' array and
don't use the string scalar variable 'path' inside the fs worker thread
loop at all.

The only advantage of the 'path' scalar was that in case of an error
the respective 'pathes' element would not be filled. Right now this is
not an issue as the v9fs_walk() function returns as soon as any error
occurs.

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

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 2815257f42..4d642ab12a 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1787,7 +1787,8 @@ static void coroutine_fn v9fs_walk(void *opaque)
                 strcmp("..", wnames[name_idx].data))
             {
                 err = s->ops->name_to_path(&s->ctx, &dpath,
-                                        wnames[name_idx].data, &path);
+                                           wnames[name_idx].data,
+                                           &pathes[name_idx]);
                 if (err < 0) {
                     err = -errno;
                     break;
@@ -1796,14 +1797,13 @@ static void coroutine_fn v9fs_walk(void *opaque)
                     err = -EINTR;
                     break;
                 }
-                err = s->ops->lstat(&s->ctx, &path, &stbuf);
+                err = s->ops->lstat(&s->ctx, &pathes[name_idx], &stbuf);
                 if (err < 0) {
                     err = -errno;
                     break;
                 }
                 stbufs[name_idx] = stbuf;
-                v9fs_path_copy(&dpath, &path);
-                v9fs_path_copy(&pathes[name_idx], &path);
+                v9fs_path_copy(&dpath, &pathes[name_idx]);
             }
         }
     });
-- 
2.20.1

On Tue, 17 Aug 2021 14:38:24 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> The v9fs_walk() function resolves all client submitted path nodes to the
> local 'pathes' array. Using a separate string scalar variable 'path'
> inside the background worker thread loop and copying that local 'path'
> string scalar variable subsequently to the 'pathes' array (at the end of
> each loop iteration) is not necessary.
> 
> Instead simply resolve each path directly to the 'pathes' array and
> don't use the string scalar variable 'path' inside the fs worker thread
> loop at all.
> 
> The only advantage of the 'path' scalar was that in case of an error
> the respective 'pathes' element would not be filled. Right now this is
> not an issue as the v9fs_walk() function returns as soon as any error
> occurs.
> 
> Suggested-by: Greg Kurz <groug@kaod.org>
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---

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

With this change, the path variable is no longer used at all in the
first loop. I see at least an extra possible cleanup : don't set
path before the first loop since it gets reset before the second
one. Maybe we can even get rid of path all the way ? I'll have
a look.

>  hw/9pfs/9p.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 2815257f42..4d642ab12a 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -1787,7 +1787,8 @@ static void coroutine_fn v9fs_walk(void *opaque)
>                  strcmp("..", wnames[name_idx].data))
>              {
>                  err = s->ops->name_to_path(&s->ctx, &dpath,
> -                                        wnames[name_idx].data, &path);
> +                                           wnames[name_idx].data,
> +                                           &pathes[name_idx]);
>                  if (err < 0) {
>                      err = -errno;
>                      break;
> @@ -1796,14 +1797,13 @@ static void coroutine_fn v9fs_walk(void *opaque)
>                      err = -EINTR;
>                      break;
>                  }
> -                err = s->ops->lstat(&s->ctx, &path, &stbuf);
> +                err = s->ops->lstat(&s->ctx, &pathes[name_idx], &stbuf);
>                  if (err < 0) {
>                      err = -errno;
>                      break;
>                  }
>                  stbufs[name_idx] = stbuf;
> -                v9fs_path_copy(&dpath, &path);
> -                v9fs_path_copy(&pathes[name_idx], &path);
> +                v9fs_path_copy(&dpath, &pathes[name_idx]);
>              }
>          }
>      });

On Freitag, 20. August 2021 12:35:49 CEST Greg Kurz wrote:
> On Tue, 17 Aug 2021 14:38:24 +0200
> 
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > The v9fs_walk() function resolves all client submitted path nodes to the
> > local 'pathes' array. Using a separate string scalar variable 'path'
> > inside the background worker thread loop and copying that local 'path'
> > string scalar variable subsequently to the 'pathes' array (at the end of
> > each loop iteration) is not necessary.
> > 
> > Instead simply resolve each path directly to the 'pathes' array and
> > don't use the string scalar variable 'path' inside the fs worker thread
> > loop at all.
> > 
> > The only advantage of the 'path' scalar was that in case of an error
> > the respective 'pathes' element would not be filled. Right now this is
> > not an issue as the v9fs_walk() function returns as soon as any error
> > occurs.
> > 
> > Suggested-by: Greg Kurz <groug@kaod.org>
> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > ---
> 
> Reviewed-by: Greg Kurz <groug@kaod.org>
> 
> With this change, the path variable is no longer used at all in the
> first loop. 

Correct.

> I see at least an extra possible cleanup : don't set
> path before the first loop since it gets reset before the second
> one. 

Also correct.

> Maybe we can even get rid of path all the way ? I'll have
> a look.

Yes, that's the plan.

There is still quite a bunch that can be cleaned up in that function. I just 
didn't want to start with a two-digit patch set right after the long summer 
break. ;-)

If you want to send some cleanup patches, always appreciated.

Best regards,
Christian Schoenebeck