From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=41944 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Pv7MH-0004wo-3D for qemu-devel@nongnu.org; Thu, 03 Mar 2011 07:10:33 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Pv7Lw-0005y9-69 for qemu-devel@nongnu.org; Thu, 03 Mar 2011 07:10:16 -0500 Received: from mail-yx0-f173.google.com ([209.85.213.173]:63897) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Pv7Lv-0005xk-Om for qemu-devel@nongnu.org; Thu, 03 Mar 2011 07:09:55 -0500 Received: by yxk8 with SMTP id 8so408238yxk.4 for ; Thu, 03 Mar 2011 04:09:55 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <1298892156-11667-6-git-send-email-mohan@in.ibm.com> References: <1298892156-11667-1-git-send-email-mohan@in.ibm.com> <1298892156-11667-6-git-send-email-mohan@in.ibm.com> Date: Thu, 3 Mar 2011 12:09:55 +0000 Message-ID: From: Stefan Hajnoczi Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: [Qemu-devel] Re: [V6 PATCH 5/9] virtio-9p: Add support to open a file in chroot environment List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "M. Mohan Kumar" Cc: qemu-devel@nongnu.org On Mon, Feb 28, 2011 at 11:22 AM, M. Mohan Kumar 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 > --- > =A0hw/9pfs/virtio-9p-chroot-qemu.c | =A0 24 +++++++++++----- > =A0hw/9pfs/virtio-9p-chroot.h =A0 =A0 =A0| =A0 =A02 +- > =A0hw/9pfs/virtio-9p-local.c =A0 =A0 =A0 | =A0 58 +++++++++++++++++++++++= +++++++++++++--- > =A03 files changed, 71 insertions(+), 13 deletions(-) > > diff --git a/hw/9pfs/virtio-9p-chroot-qemu.c b/hw/9pfs/virtio-9p-chroot-q= emu.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, V9fsFileObj= ectRequest *request) > =A0 =A0 return 0; > =A0} > > -/* > - * 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) > =A0{ > - =A0 =A0(void)v9fs_receivefd; > - =A0 =A0(void)v9fs_write_request; > + =A0 =A0int fd, sock_error; > + =A0 =A0qemu_mutex_lock(&fs_ctx->chroot_mutex); > + =A0 =A0if (fs_ctx->chroot_ioerror) { > + =A0 =A0 =A0 =A0fd =3D -EIO; > + =A0 =A0 =A0 =A0goto unlock; > + =A0 =A0} > + =A0 =A0v9fs_write_request(fs_ctx->chroot_socket, request); > + =A0 =A0fd =3D v9fs_receivefd(fs_ctx->chroot_socket, &sock_error); > + =A0 =A0if (fd < 0 && sock_error) { > + =A0 =A0 =A0 =A0fs_ctx->chroot_ioerror =3D 1; > + =A0 =A0} 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, > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0const char *path, FsCred *credp) > +{ > + =A0 =A0if (strlen(path) > PATH_MAX) { > + =A0 =A0 =A0 =A0return -ENAMETOOLONG; > + =A0 =A0} Off-by-one error. It should strlen(path) >=3D PATH_MAX to account for the NUL terminator. > + =A0 =A0memset(request, 0, sizeof(*request)); > + =A0 =A0request->data.path_len =3D strlen(path); > + =A0 =A0strcpy(request->path.path, path); > + =A0 =A0if (credp) { > + =A0 =A0 =A0 =A0request->data.mode =3D credp->fc_mode; > + =A0 =A0 =A0 =A0request->data.uid =3D credp->fc_uid; > + =A0 =A0 =A0 =A0request->data.gid =3D credp->fc_gid; > + =A0 =A0 =A0 =A0request->data.dev =3D credp->fc_rdev; > + =A0 =A0} > + =A0 =A0return 0; > +} > + > +static int passthrough_open(FsContext *fs_ctx, const char *path, int fla= gs) > +{ > + =A0 =A0V9fsFileObjectRequest request; > + =A0 =A0int fd; > + > + =A0 =A0fd =3D fill_fileobjectrequest(&request, path, NULL); > + =A0 =A0if (fd < 0) { > + =A0 =A0 =A0 =A0errno =3D -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" > + =A0 =A0 =A0 =A0return -1; > + =A0 =A0} > + =A0 =A0request.data.flags =3D flags; > + =A0 =A0request.data.type =3D T_OPEN; > + =A0 =A0fd =3D v9fs_request(fs_ctx, &request); > + =A0 =A0return fd; > +} > > =A0static int local_lstat(FsContext *fs_ctx, const char *path, struct sta= t *stbuf) > =A0{ > @@ -138,14 +175,27 @@ static int local_closedir(FsContext *ctx, DIR *dir) > =A0 =A0 return closedir(dir); > =A0} > > -static int local_open(FsContext *ctx, const char *path, int flags) > +static int local_open(FsContext *fs_ctx, const char *path, int flags) > =A0{ > - =A0 =A0return open(rpath(ctx, path), flags); > + =A0 =A0if (fs_ctx->fs_sm =3D=3D SM_PASSTHROUGH) { > + =A0 =A0 =A0 =A0return passthrough_open(fs_ctx, path, flags); > + =A0 =A0} else { > + =A0 =A0 =A0 =A0return open(rpath(fs_ctx, path), flags); > + =A0 =A0} > =A0} > > -static DIR *local_opendir(FsContext *ctx, const char *path) > +static DIR *local_opendir(FsContext *fs_ctx, const char *path) > =A0{ > - =A0 =A0return opendir(rpath(ctx, path)); > + =A0 =A0if (fs_ctx->fs_sm =3D=3D SM_PASSTHROUGH) { > + =A0 =A0 =A0 =A0int fd; > + =A0 =A0 =A0 =A0fd =3D passthrough_open(fs_ctx, path, O_DIRECTORY); > + =A0 =A0 =A0 =A0if (fd < 0) { > + =A0 =A0 =A0 =A0 =A0 =A0return NULL; > + =A0 =A0 =A0 =A0} > + =A0 =A0 =A0 =A0return fdopendir(fd); > + =A0 =A0} else { > + =A0 =A0 =A0 =A0return opendir(rpath(fs_ctx, path)); > + =A0 =A0} 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