| Message ID | 1298892156-11667-6-git-send-email-mohan@in.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Feb 28, 2011 at 11:22 AM, M. Mohan Kumar <mohan@in.ibm.com> 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 <mohan@in.ibm.com> > --- > hw/9pfs/virtio-9p-chroot-qemu.c | 24 +++++++++++----- > hw/9pfs/virtio-9p-chroot.h | 2 +- > hw/9pfs/virtio-9p-local.c | 58 ++++++++++++++++++++++++++++++++++++--- > 3 files changed, 71 insertions(+), 13 deletions(-) > > diff --git a/hw/9pfs/virtio-9p-chroot-qemu.c b/hw/9pfs/virtio-9p-chroot-qemu.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, V9fsFileObjectRequest *request) > return 0; > } > > -/* > - * 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) > { > - (void)v9fs_receivefd; > - (void)v9fs_write_request; > + int fd, sock_error; > + qemu_mutex_lock(&fs_ctx->chroot_mutex); > + if (fs_ctx->chroot_ioerror) { > + fd = -EIO; > + goto unlock; > + } > + 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. 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. > + 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. 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
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
On Thu, Mar 3, 2011 at 1:54 PM, M. Mohan Kumar <mohan@in.ibm.com> wrote: > 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. The fd is optionally passed via SCM_RIGHTS, no as an FdInfo field. The QEMU side can detect whether or not the fd has been passed, it doesn't need anything in FdInfo other than int -errno. > That would have more code duplication when compared to additional > conditionals. i.e, We need to create local_passthrough_opendir, > local_passthrough_open, .. etc. The choice of conditionals vs function pointers isn't really about code duplication, it's about putting all the passthrough code in one place and all the non-passthrough code in another place rather than interleaving everywhere. This is just a suggestion though and it's up to you. >> 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? This is more of a speculative suggestion but I'm imagining calling the same open, mkdir, etc helper code in the chroot child as get executed when the chroot helper is not in use. It looks like we're splitting code paths even further than they already are today with this patchset. Stefan
diff --git a/hw/9pfs/virtio-9p-chroot-qemu.c b/hw/9pfs/virtio-9p-chroot-qemu.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, V9fsFileObjectRequest *request) return 0; } -/* - * 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) { - (void)v9fs_receivefd; - (void)v9fs_write_request; + int fd, sock_error; + qemu_mutex_lock(&fs_ctx->chroot_mutex); + if (fs_ctx->chroot_ioerror) { + fd = -EIO; + goto unlock; + } + 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; + } +unlock: + qemu_mutex_unlock(&fs_ctx->chroot_mutex); + return fd; } diff --git a/hw/9pfs/virtio-9p-chroot.h b/hw/9pfs/virtio-9p-chroot.h index bd186dd..4592807 100644 --- a/hw/9pfs/virtio-9p-chroot.h +++ b/hw/9pfs/virtio-9p-chroot.h @@ -53,6 +53,6 @@ typedef struct V9fsFileObjectRequest } V9fsFileObjectRequest; int v9fs_chroot(FsContext *fs_ctx); -void chroot_dummy(void); +int v9fs_request(FsContext *fs_ctx, V9fsFileObjectRequest *or); #endif /* _QEMU_VIRTIO_9P_CHROOT_H */ diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c index 0a015de..0c55d35 100644 --- a/hw/9pfs/virtio-9p-local.c +++ b/hw/9pfs/virtio-9p-local.c @@ -13,6 +13,9 @@ #include "virtio.h" #include "virtio-9p.h" #include "virtio-9p-xattr.h" +#include "qemu_socket.h" +#include "fsdev/qemu-fsdev.h" +#include "virtio-9p-chroot.h" #include <arpa/inet.h> #include <pwd.h> #include <grp.h> @@ -20,6 +23,40 @@ #include <sys/un.h> #include <attr/xattr.h> +/* Helper routine to fill V9fsFileObjectRequest structure */ +static int fill_fileobjectrequest(V9fsFileObjectRequest *request, + const char *path, FsCred *credp) +{ + if (strlen(path) > PATH_MAX) { + return -ENAMETOOLONG; + } + 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; + 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)); + } } static void local_rewinddir(FsContext *ctx, DIR *dir)
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 <mohan@in.ibm.com> --- hw/9pfs/virtio-9p-chroot-qemu.c | 24 +++++++++++----- hw/9pfs/virtio-9p-chroot.h | 2 +- hw/9pfs/virtio-9p-local.c | 58 ++++++++++++++++++++++++++++++++++++--- 3 files changed, 71 insertions(+), 13 deletions(-)