[Qemu-devel] Re: [V6 PATCH 5/9] virtio-9p: Add support to open a file in chroot environment

[Stefan Hajnoczi]

On Thu, Mar 3, 2011 at 1:54 PM, M. Mohan Kumar <address@hidden> wrote:
> On Thursday 03 March 2011 5:39:55 pm Stefan Hajnoczi wrote:
>> > +    v9fs_write_request(fs_ctx->chroot_socket, request);
>> > +    fd = v9fs_receivefd(fs_ctx->chroot_socket, &sock_error);
>> > +    if (fd < 0 && sock_error) {
>> > +        fs_ctx->chroot_ioerror = 1;
>> > +    }
>>
>> chroot_ioerror, sock_error, and their FdInfo bits are redundant code.
>> The chroot child could just exit on error and the parent would get
>> errors when writing the request here, which is the same effect as
>> manually returning -EIO in this function.  There is no need to
>> introduce variables and bits to communicate this failure mode.
>>
>> Once that simplification has been made, FdInfo becomes just an -errno
>> value to pass back the result of open(2) and friends.  That means we
>> can completely drop FdInfo and the fi_fd field which doesn't actually
>> hold a useful fd value for the QEMU process but just a -errno.
>>
>
> But we need to pass the fd to qemu process,  so it could not be -errno. When
> fd >= 0 its a valid fd otherwise its a errno.

The fd is optionally passed via SCM_RIGHTS, no as an FdInfo field.
The QEMU side can detect whether or not the fd has been passed, it
doesn't need anything in FdInfo other than int -errno.

> That would have more code duplication when compared to additional
> conditionals. i.e, We need to create local_passthrough_opendir,
> local_passthrough_open, .. etc.

The choice of conditionals vs function pointers isn't really about
code duplication, it's about putting all the passthrough code in one
place and all the non-passthrough code in another place rather than
interleaving everywhere.  This is just a suggestion though and it's up
to you.

>> Also it would be neat to reuse the local implementations on the chroot
>> child side instead of instead of splitting code paths, but I'm not
>> sure whether that is possible.
> I am not sure what do you mean by reusing the virtio-9p-local.c implementation
> in chilld side. That may involve breaking down existing  local functions?

This is more of a speculative suggestion but I'm imagining calling the
same open, mkdir, etc helper code in the chroot child as get executed
when the chroot helper is not in use.  It looks like we're splitting
code paths even further than they already are today with this
patchset.

Stefan