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

["M. Mohan Kumar" <mohan@in.ibm.com>]

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.

> 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.
> 
Ok, I Will change!

> > +    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.

That would have more code duplication when compared to additional 
conditionals. i.e, We need to create local_passthrough_opendir, 
local_passthrough_open, .. etc.
 
> 
> 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?

----
M. Mohan Kumar