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

[Venkateswararao Jujjuri]

On 04/15/2011 02:42 AM, Stefan Hajnoczi wrote:
> On Fri, Apr 15, 2011 at 10:05 AM, M. Mohan Kumar<address@hidden>  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<address@hidden>
> > ---
> > 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.
This is very good suggestion. I think we should explore this.

> 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.
This is ideal and we should do it where ever possible.

But I am not sure if this is really achievable.
In the case of passthrough we need to pass this information
from chroot() process to qemu. In other security models
we don't have 1-1 correspondence between 9p operations and system calls.
Will catch you on IRC for further discussion.

Thanks,
JV

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