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

[Venkateswararao Jujjuri <jvrao@linux.vnet.ibm.com>]

On 04/15/2011 02:42 AM, Stefan Hajnoczi wrote:
> 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.
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
>