[Qemu-devel] Re: [V7 PATCH 7/9] virtio-9p: Support for creating special files

[M. Mohan Kumar]

On Friday 04 March 2011 4:36:35 pm Stefan Hajnoczi wrote:
> On Fri, Mar 4, 2011 at 9:25 AM, M. Mohan Kumar <address@hidden> wrote:
> > +static int chroot_do_create_special(V9fsFileObjectRequest *request)
> > +{
> > +    int cur_uid, cur_gid;
> > +    int retval = -1;
> > +
> > +    cur_uid = geteuid();
> > +    cur_gid = getegid();
> > +
> > +    if (setfsuid(request->data.uid) < 0) {
> > +        return -errno;
> > +    }
> > +    if (setfsgid(request->data.gid) < 0) {
> > +        retval = -errno;
> > +        goto unset_uid;
> > +    }
> > +
> > +    switch (request->data.type) {
> > +    case T_MKDIR:
> > +        retval = mkdir(request->path.path, request->data.mode);
> > +        break;
> > +    case T_SYMLINK:
> > +        retval = symlink(request->path.old_path, request->path.path);
> > +        break;
> > +    case T_LINK:
> > +        retval = link(request->path.old_path, request->path.path);
> > +        break;
> > +    default:
> > +        retval = mknod(request->path.path, request->data.mode,
> > +                        request->data.dev);
> > +        break;
> > +    }
> > +
> > +    if (retval < 0) {
> > +        retval = -errno;
> > +    }
> > +    setfsgid(cur_gid);
> > +unset_uid:
> > +    setfsuid(cur_uid);
> > +    return retval;
> > +}
> 
> It would be nice to take this one step further and move file create
> and open here too.  The prototype we need is:
> 
> static int chroot_handle_request(V9fsFileObjectRequest *request, int *fd)
> {
>     *fd = -1;
> 
> It returns 0 on success or -errno and *fd >= 0 if a file descriptor
> was opened and -1 otherwise.
> 
> This function becomes the main request processing function called from
> v9fs_chroot() and the switch statement there can be eliminated.
>

We don't need setfsgid, setfsuid for normal open. Also I think having separate 
function based on the functionality helps better code readability. 
 
> Sending the response back to QEMU then gets a cleaned up prototype:
> chroot_sendfd(int chroot_sock, int result, int fd) where result is 0
> on success or -errno and fd >= 0 if present or -1 if not.
> 
> > +int v9fs_create_special(FsContext *fs_ctx, V9fsFileObjectRequest
> > *request) +{
> > +    int retval, sock_error;
> > +    qemu_mutex_lock(&fs_ctx->chroot_mutex);
> > +    if (fs_ctx->chroot_ioerror) {
> > +        retval = -EIO;
> > +        goto unlock;
> > +    }
> > +    if (v9fs_write_request(fs_ctx->chroot_socket, request) < 0) {
> > +        fs_ctx->chroot_ioerror = 1;
> > +        retval = -EIO;
> > +        goto unlock;
> > +    }
> > +    retval = v9fs_receivefd(fs_ctx->chroot_socket, &sock_error);
> > +    if (retval < 0 && sock_error) {
> > +        fs_ctx->chroot_ioerror = 1;
> > +    }
> > +unlock:
> > +    qemu_mutex_unlock(&fs_ctx->chroot_mutex);
> > +    return retval;
> > +}
> 
> This function is a duplicate of v9fs_request().  Can't there be just
> one function?
> 
Yeah, I will make it as a single function.

----
M. Mohan Kumar