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

[Stefan Hajnoczi]

On Mon, Feb 28, 2011 at 11:22 AM, M. Mohan Kumar <address@hidden> wrote:
> This patch adds both chroot deamon and qemu side support to open a file/
> directory in the chroot environment
>
> Signed-off-by: M. Mohan Kumar <address@hidden>
> ---
>  hw/9pfs/virtio-9p-chroot-qemu.c |   24 +++++++++++-----
>  hw/9pfs/virtio-9p-chroot.h      |    2 +-
>  hw/9pfs/virtio-9p-local.c       |   58 
> ++++++++++++++++++++++++++++++++++++---
>  3 files changed, 71 insertions(+), 13 deletions(-)
>
> diff --git a/hw/9pfs/virtio-9p-chroot-qemu.c b/hw/9pfs/virtio-9p-chroot-qemu.c
> index d2d8ab9..41f9db2 100644
> --- a/hw/9pfs/virtio-9p-chroot-qemu.c
> +++ b/hw/9pfs/virtio-9p-chroot-qemu.c
> @@ -85,13 +85,21 @@ static int v9fs_write_request(int sockfd, 
> V9fsFileObjectRequest *request)
>     return 0;
>  }
>
> -/*
> - * This patch adds v9fs_receivefd and v9fs_write_request functions,
> - * but there is no callers. To avoid compiler warning message,
> - * refer these two functions
> - */
> -void chroot_dummy(void)
> +/* Return opened file descriptor on success or -errno on error */
> +int v9fs_request(FsContext *fs_ctx, V9fsFileObjectRequest *request)
>  {
> -    (void)v9fs_receivefd;
> -    (void)v9fs_write_request;
> +    int fd, sock_error;
> +    qemu_mutex_lock(&fs_ctx->chroot_mutex);
> +    if (fs_ctx->chroot_ioerror) {
> +        fd = -EIO;
> +        goto unlock;
> +    }
> +    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.

Instead send back only an int -errno return code from the chroot child.

You mentioned wanting to distinguish between socket errors and
blocking syscall errors but since there isn't any real error handling
that makes use of that information (and I'm not sure there is a good
error handling strategy that could be used), this is all just adding
complexity.

> +/* Helper routine to fill V9fsFileObjectRequest structure */
> +static int fill_fileobjectrequest(V9fsFileObjectRequest *request,
> +                const char *path, FsCred *credp)
> +{
> +    if (strlen(path) > PATH_MAX) {
> +        return -ENAMETOOLONG;
> +    }

Off-by-one error.  It should strlen(path) >= PATH_MAX to account for
the NUL terminator.

> +    memset(request, 0, sizeof(*request));
> +    request->data.path_len = strlen(path);
> +    strcpy(request->path.path, path);
> +    if (credp) {
> +        request->data.mode = credp->fc_mode;
> +        request->data.uid = credp->fc_uid;
> +        request->data.gid = credp->fc_gid;
> +        request->data.dev = credp->fc_rdev;
> +    }
> +    return 0;
> +}
> +
> +static int passthrough_open(FsContext *fs_ctx, const char *path, int flags)
> +{
> +    V9fsFileObjectRequest request;
> +    int fd;
> +
> +    fd = fill_fileobjectrequest(&request, path, NULL);
> +    if (fd < 0) {
> +        errno = -fd;

Please don't use errno to communicate errors back.  In this function
it would be perfectly fine to return fd here since it is already a
-errno.

It's not safe to use errno since it's value can be lost by calling any
external function - its value may be modified even if no error occurs.
 I quoted from the errno(3) man page in a previous review:
"a function that succeeds *is* allowed to change errno"

> +        return -1;
> +    }
> +    request.data.flags = flags;
> +    request.data.type = T_OPEN;
> +    fd = v9fs_request(fs_ctx, &request);
> +    return fd;
> +}
>
>  static int local_lstat(FsContext *fs_ctx, const char *path, struct stat 
> *stbuf)
>  {
> @@ -138,14 +175,27 @@ static int local_closedir(FsContext *ctx, DIR *dir)
>     return closedir(dir);
>  }
>
> -static int local_open(FsContext *ctx, const char *path, int flags)
> +static int local_open(FsContext *fs_ctx, const char *path, int flags)
>  {
> -    return open(rpath(ctx, path), flags);
> +    if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
> +        return passthrough_open(fs_ctx, path, flags);
> +    } else {
> +        return open(rpath(fs_ctx, path), flags);
> +    }
>  }
>
> -static DIR *local_opendir(FsContext *ctx, const char *path)
> +static DIR *local_opendir(FsContext *fs_ctx, const char *path)
>  {
> -    return opendir(rpath(ctx, path));
> +    if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
> +        int fd;
> +        fd = passthrough_open(fs_ctx, path, O_DIRECTORY);
> +        if (fd < 0) {
> +            return NULL;
> +        }
> +        return fdopendir(fd);
> +    } else {
> +        return opendir(rpath(fs_ctx, path));
> +    }

Perhaps we should use a local_operations struct or similar function
pointer table instead of adding fs_ctx->fs_sm conditionals everywhere.

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.

Stefan