Re: [PATCH 1/2] hw/9pfs: avoid 'path' copy in v9fs_walk()

[Greg Kurz]

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]);
>              }
>          }
>      });