Re: [Qemu-devel] [PATCH] virtio-9p: Make rpath function thread safe

[Stefan Hajnoczi <stefanha@gmail.com>]

On Fri, Apr 15, 2011 at 10:05 AM, M. Mohan Kumar <mohan@in.ibm.com> wrote:
> Use dynamically allocated memory to return concatenated path
> (fs_root and file path) instead of a static buffer. Caller has to free
> the memory.
>
> Signed-off-by: M. Mohan Kumar <mohan@in.ibm.com>
> ---
> This patch depends on my chroot patchset.
> http://patchwork.ozlabs.org/patch/89033/
>
>  hw/9pfs/virtio-9p-local.c |  551 ++++++++++++++++++++++++++-------------------
>  hw/file-op-9p.h           |   10 +-
>  2 files changed, 322 insertions(+), 239 deletions(-)

Would it be possible to build a const char *fs_ctx->path once in the
PDU handler for each operation that takes a path instead of pushing so
many rpath() calls down into virtio-9p-local.c?  That way you don't
need to worry about constructing and freeing paths over and over.

I think it's time to kill errno once and for all in virtio-9p.  The
errno global variable usage is getting out of hand; pretty much every
patch is adding boilerplate code to juggle around errno and avoid
trampling it.  This new qemu_free() call means we need to juggle in
new places.

Please stop adding more errno juggling and just use -errno return
values.  That change needs to happen before this patch.

> @@ -137,29 +164,10 @@ static int local_lstat(FsContext *fs_ctx, const char *path, struct stat *stbuf)
>         }
>         if (getxattr(rpath(fs_ctx, path), "user.virtfs.rdev", &tmp_dev,
>                         sizeof(dev_t)) > 0) {

Leak.

>         tsize = readlink(rpath(fs_ctx, path), buf, bufsz);

Leak.

> -static void local_rewinddir(FsContext *ctx, DIR *dir)
> +static void local_rewinddir(FsContext *fs_ctx, DIR *dir)
>  {
>     return rewinddir(dir);
>  }

Please do this in a separate patch.

>         err = local_set_xattr(rpath(fs_ctx, newpath), credp);

Leak.

Stefan