From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=46682 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PxEU0-0007bU-R7 for qemu-devel@nongnu.org; Wed, 09 Mar 2011 03:11:02 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PxETy-0001zi-9e for qemu-devel@nongnu.org; Wed, 09 Mar 2011 03:10:59 -0500 Received: from e23smtp05.au.ibm.com ([202.81.31.147]:46146) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PxETx-0001zQ-O3 for qemu-devel@nongnu.org; Wed, 09 Mar 2011 03:10:58 -0500 Received: from d23relay04.au.ibm.com (d23relay04.au.ibm.com [202.81.31.246]) by e23smtp05.au.ibm.com (8.14.4/8.13.1) with ESMTP id p2985Jpw016585 for ; Wed, 9 Mar 2011 19:05:19 +1100 Received: from d23av04.au.ibm.com (d23av04.au.ibm.com [9.190.235.139]) by d23relay04.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p298At9X1945688 for ; Wed, 9 Mar 2011 19:10:55 +1100 Received: from d23av04.au.ibm.com (loopback [127.0.0.1]) by d23av04.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p298AteO017155 for ; Wed, 9 Mar 2011 19:10:55 +1100 From: "M. Mohan Kumar" Date: Wed, 9 Mar 2011 13:40:52 +0530 References: <1299230756-1644-1-git-send-email-mohan@in.ibm.com> <1299230756-1644-10-git-send-email-mohan@in.ibm.com> In-Reply-To: MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201103091340.53088.mohan@in.ibm.com> Subject: [Qemu-devel] Re: [V7 PATCH 9/9] virtio-9p: Chroot environment for other functions List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: qemu-devel@nongnu.org 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