[Qemu-devel] Re: [V7 PATCH 9/9] virtio-9p: Chroot environment for other functions

[M. Mohan Kumar]

Thanks Stefan for your review!

On Friday 04 March 2011 5:22:00 pm Stefan Hajnoczi wrote:
> 
> Is this code supposed to build on non-Linux hosts?  If so, then please
> confirm that the *at() system calls used are standard and available on
> other hosts (e.g. FreeBSD, Darwin, Solaris).
> 
No, this (virtio-9p) is not supported on non-linux hosts.

> > +/*
> > + * Returns file descriptor of dirname(path)
> > + * This fd can be used by *at functions
> > + */
> > +static int get_dirfd(FsContext *fs_ctx, const char *path)
> > +{
> > +    V9fsFileObjectRequest request;
> > +    int fd;
> > +    char *dpath = qemu_strdup(path);
> > +
> > +    fd = fill_fileobjectrequest(&request, dirname(dpath), NULL);
> > +    if (fd < 0) {
> > +        return fd;
> > +    }
> 
> Leaks dpath, fails to set errno, and does not return -1.
Will fix in next patchset.

> 
> > @@ -545,53 +621,146 @@ static int local_link(FsContext *fs_ctx, const
> > char *oldpath,
> > 
> >  static int local_truncate(FsContext *ctx, const char *path, off_t size)
> >  {
> > -    return truncate(rpath(ctx, path), size);
> > +    if (ctx->fs_sm == SM_PASSTHROUGH) {
> > +        int fd, retval;
> > +        fd = passthrough_open(ctx, path, O_RDWR);
> > +        if (fd < 0) {
> > +            return -1;
> > +        }
> > +        retval = ftruncate(fd, size);
> > +        close(fd);
> > +        return retval;
> 
> This is an example of where errno is not guaranteed to be preserved.
> When ftruncate(2) fails close(2) is allowed to affect errno and we
> cannot rely on it holding the ftruncate(2) error code.  Please check
> for other cases of this.
Will fix in next patchset.
> 
> >  static int local_rename(FsContext *ctx, const char *oldpath,
> >                         const char *newpath)
> >  {
> > -    char *tmp;
> > -    int err;
> > -
> > -    tmp = qemu_strdup(rpath(ctx, oldpath));
> > +    int err, serrno = 0;
> > 
> > -    err = rename(tmp, rpath(ctx, newpath));
> > -    if (err == -1) {
> > -        int serrno = errno;
> > -        qemu_free(tmp);
> > +    if (ctx->fs_sm == SM_PASSTHROUGH) {
> > +        int opfd, npfd;
> > +        char *old_tmppath, *new_tmppath;
> > +        opfd = get_dirfd(ctx, oldpath);
> > +        if (opfd < 0) {
> > +            return -1;
> > +        }
> > +        npfd = get_dirfd(ctx, newpath);
> > +        if (npfd < 0) {
> > +            close(opfd);
> > +            return -1;
> > +        }
> > +        old_tmppath = qemu_strdup(oldpath);
> > +        new_tmppath = qemu_strdup(newpath);
> > +        err = renameat(opfd, basename(old_tmppath),
> > +                        npfd, basename(new_tmppath));
> > +        if (err == -1) {
> > +            serrno = errno;
> > +        }
> > +        close(npfd);
> > +        close(opfd);
> > +        qemu_free(old_tmppath);
> > +        qemu_free(new_tmppath);
> >         errno = serrno;
> 
> Why can't this be done as a chroot worker operation in a single syscall?
Ok, in next patchset, T_RENAME & T_REMOVE types will be added.

> 
> > -static int local_utimensat(FsContext *s, const char *path,
> > -                           const struct timespec *buf)
> > +static int local_utimensat(FsContext *fs_ctx, const char *path,
> > +                const struct timespec *buf)
> >  {
> > -    return qemu_utimensat(AT_FDCWD, rpath(s, path), buf,
> > AT_SYMLINK_NOFOLLOW); +    if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
> > +        int fd, retval;
> > +        fd = passthrough_open(fs_ctx, path, O_RDONLY | O_NONBLOCK);
> 
> This follows symlinks but the SM_PASSTHROUGH case below does not?
Will fix.
> 
> > +        if (fd < 0) {
> > +            return -1;
> > +        }
> > +        retval = futimens(fd, buf);
> > +        close(fd);
> > +        return retval;
> > +    } else {
> > +        return utimensat(AT_FDCWD, rpath(fs_ctx, path), buf,
> > +                        AT_SYMLINK_NOFOLLOW);
> > +    }
> >  }
> > 
> > -static int local_remove(FsContext *ctx, const char *path)
> > -{
> > -    return remove(rpath(ctx, path));
> > +static int local_remove(FsContext *fs_ctx, const char *path)
> > + {
> > +    if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
> > +        int pfd, err, serrno, flags;
> > +        char *old_path;
> > +        struct stat stbuf;
> > +        pfd = get_dirfd(fs_ctx, path);
> > +        if (pfd < 0) {
> > +            return -1;
> > +        }
> > +        old_path = qemu_strdup(path);
> > +        err = fstatat(pfd, basename(old_path), &stbuf,
> > AT_SYMLINK_NOFOLLOW); +        if (err < 0) {
> 
> old_path and pfd are leaked.
> 
> > +            return -1;
> > +        }
> > +        serrno = flags = 0;
> > +        if ((stbuf.st_mode & S_IFMT) == S_IFDIR) {
> > +            flags = AT_REMOVEDIR;
> > +        } else {
> > +            flags = 0;
> > +        }
> > +        err = unlinkat(pfd, basename(old_path), flags);
> > +        if (err == -1) {
> > +            serrno = errno;
> > +        }
> > +        qemu_free(old_path);
> > +        close(pfd);
> > +        errno = serrno;
> > +        return err;
> 
> Why is SM_PASSTHROUGH so complicated...
> 
> > +    } else {
> > +        return remove(rpath(fs_ctx, path));
> 
> ...but this so simple?
> 
> Could we just send a remove operation to the chroot worker instead?
> 

 

----
M. Mohan Kumar