From patchwork Wed Jun 8 16:21:59 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Sassan Panahinejad X-Patchwork-Id: 99510 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [140.186.70.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id A7A52B6FCC for ; Thu, 9 Jun 2011 03:10:29 +1000 (EST) Received: from localhost ([::1]:40426 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QUMGv-0001ih-3N for incoming@patchwork.ozlabs.org; Wed, 08 Jun 2011 13:10:25 -0400 Received: from eggs.gnu.org ([140.186.70.92]:58206) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QULWT-0005zA-Ou for qemu-devel@nongnu.org; Wed, 08 Jun 2011 12:22:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QULWQ-0004MQ-56 for qemu-devel@nongnu.org; Wed, 08 Jun 2011 12:22:25 -0400 Received: from mail-wy0-f173.google.com ([74.125.82.173]:56720) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QULWP-0004Lo-3s for qemu-devel@nongnu.org; Wed, 08 Jun 2011 12:22:21 -0400 Received: by wyb42 with SMTP id 42so546363wyb.4 for ; Wed, 08 Jun 2011 09:22:19 -0700 (PDT) Received: by 10.227.143.134 with SMTP id v6mr7730223wbu.55.1307550139081; Wed, 08 Jun 2011 09:22:19 -0700 (PDT) Received: from localhost.localdomain (cpc2-aztw23-2-0-cust797.aztw.cable.virginmedia.com [94.171.235.30]) by mx.google.com with ESMTPS id c17sm537099wbh.12.2011.06.08.09.22.15 (version=SSLv3 cipher=OTHER); Wed, 08 Jun 2011 09:22:16 -0700 (PDT) From: Sassan Panahinejad To: qemu-devel@nongnu.org Date: Wed, 8 Jun 2011 17:21:59 +0100 Message-Id: <1307550119-6111-1-git-send-email-sassan@sassan.me.uk> X-Mailer: git-send-email 1.7.4.1 In-Reply-To: <4DD2D1E6.7030300@linux.vnet.ibm.com> References: <4DD2D1E6.7030300@linux.vnet.ibm.com> X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6 (newer, 2) X-Received-From: 74.125.82.173 Cc: Sassan Panahinejad , jvrao@linux.vnet.ibm.com Subject: [Qemu-devel] [PATCH] Clean up virtio-9p error handling code X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org In a lot of cases, the handling of errors was quite ugly. This patch moves reading of errno to immediately after the system calls and passes it up through the system more cleanly. Also, in the case of the xattr functions (and possibly others), completely the wrong error was being returned. This patch is created against your 9p-coroutine-bh branch, as requested. Sorry for the delay, I was unexpectedly required to work abroad for 2 weeks. Signed-off-by: Sassan Panahinejad --- fsdev/file-op-9p.h | 4 +- hw/9pfs/codir.c | 14 +---- hw/9pfs/virtio-9p-local.c | 123 +++++++++++++++++++++++++-------------------- hw/9pfs/virtio-9p-xattr.c | 21 +++++++- 4 files changed, 90 insertions(+), 72 deletions(-) diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h index af9daf7..3d9575b 100644 --- a/fsdev/file-op-9p.h +++ b/fsdev/file-op-9p.h @@ -73,12 +73,12 @@ typedef struct FileOperations int (*setuid)(FsContext *, uid_t); int (*close)(FsContext *, int); int (*closedir)(FsContext *, DIR *); - DIR *(*opendir)(FsContext *, const char *); + int (*opendir)(FsContext *, const char *, DIR **); int (*open)(FsContext *, const char *, int); int (*open2)(FsContext *, const char *, int, FsCred *); void (*rewinddir)(FsContext *, DIR *); off_t (*telldir)(FsContext *, DIR *); - struct dirent *(*readdir)(FsContext *, DIR *); + int (*readdir)(FsContext *, DIR *, struct dirent **); void (*seekdir)(FsContext *, DIR *, off_t); ssize_t (*preadv)(FsContext *, int, const struct iovec *, int, off_t); ssize_t (*pwritev)(FsContext *, int, const struct iovec *, int, off_t); diff --git a/hw/9pfs/codir.c b/hw/9pfs/codir.c index 110289f..acbbb39 100644 --- a/hw/9pfs/codir.c +++ b/hw/9pfs/codir.c @@ -25,12 +25,7 @@ int v9fs_co_readdir(V9fsState *s, V9fsFidState *fidp, struct dirent **dent) { errno = 0; /*FIXME!! need to switch to readdir_r */ - *dent = s->ops->readdir(&s->ctx, fidp->fs.dir); - if (!*dent && errno) { - err = -errno; - } else { - err = 0; - } + err = s->ops->readdir(&s->ctx, fidp->fs.dir, dent); }); return err; } @@ -93,12 +88,7 @@ int v9fs_co_opendir(V9fsState *s, V9fsFidState *fidp) v9fs_co_run_in_worker( { - dir = s->ops->opendir(&s->ctx, fidp->path.data); - if (!dir) { - err = -errno; - } else { - err = 0; - } + err = s->ops->opendir(&s->ctx, fidp->path.data, &dir); }); fidp->fs.dir = dir; return err; diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c index 77904c3..65f35eb 100644 --- a/hw/9pfs/virtio-9p-local.c +++ b/hw/9pfs/virtio-9p-local.c @@ -28,7 +28,7 @@ static int local_lstat(FsContext *fs_ctx, const char *path, struct stat *stbuf) char buffer[PATH_MAX]; err = lstat(rpath(fs_ctx, path, buffer), stbuf); if (err) { - return err; + return -errno; } if (fs_ctx->fs_sm == SM_MAPPED) { /* Actual credentials are part of extended attrs */ @@ -53,7 +53,7 @@ static int local_lstat(FsContext *fs_ctx, const char *path, struct stat *stbuf) stbuf->st_rdev = tmp_dev; } } - return err; + return 0; } static int local_set_xattr(const char *path, FsCred *credp) @@ -63,28 +63,28 @@ static int local_set_xattr(const char *path, FsCred *credp) err = setxattr(path, "user.virtfs.uid", &credp->fc_uid, sizeof(uid_t), 0); if (err) { - return err; + return -errno; } } if (credp->fc_gid != -1) { err = setxattr(path, "user.virtfs.gid", &credp->fc_gid, sizeof(gid_t), 0); if (err) { - return err; + return -errno; } } if (credp->fc_mode != -1) { err = setxattr(path, "user.virtfs.mode", &credp->fc_mode, sizeof(mode_t), 0); if (err) { - return err; + return -errno; } } if (credp->fc_rdev != -1) { err = setxattr(path, "user.virtfs.rdev", &credp->fc_rdev, sizeof(dev_t), 0); if (err) { - return err; + return -errno; } } return 0; @@ -95,7 +95,7 @@ static int local_post_create_passthrough(FsContext *fs_ctx, const char *path, { char buffer[PATH_MAX]; if (chmod(rpath(fs_ctx, path, buffer), credp->fc_mode & 07777) < 0) { - return -1; + return -errno; } if (lchown(rpath(fs_ctx, path, buffer), credp->fc_uid, credp->fc_gid) < 0) { @@ -104,7 +104,7 @@ static int local_post_create_passthrough(FsContext *fs_ctx, const char *path, * using security model none. Ignore the error */ if (fs_ctx->fs_sm != SM_NONE) { - return -1; + return -errno; } } return 0; @@ -119,40 +119,42 @@ static ssize_t local_readlink(FsContext *fs_ctx, const char *path, int fd; fd = open(rpath(fs_ctx, path, buffer), O_RDONLY); if (fd == -1) { - return -1; + return -errno; } do { tsize = read(fd, (void *)buf, bufsz); } while (tsize == -1 && errno == EINTR); close(fd); - return tsize; + return tsize == -1 ? -errno : tsize; } else if ((fs_ctx->fs_sm == SM_PASSTHROUGH) || (fs_ctx->fs_sm == SM_NONE)) { tsize = readlink(rpath(fs_ctx, path, buffer), buf, bufsz); } - return tsize; + return tsize == -1 ? -errno : tsize; } static int local_close(FsContext *ctx, int fd) { - return close(fd); + return close(fd) == -1 ? -errno : 0; } static int local_closedir(FsContext *ctx, DIR *dir) { - return closedir(dir); + return closedir(dir) == -1 ? -errno : 0; } static int local_open(FsContext *ctx, const char *path, int flags) { char buffer[PATH_MAX]; - return open(rpath(ctx, path, buffer), flags); + int ret = open(rpath(ctx, path, buffer), flags); + return ret == -1 ? -errno : ret; } -static DIR *local_opendir(FsContext *ctx, const char *path) +static int local_opendir(FsContext *ctx, const char *path, DIR **dir) { char buffer[PATH_MAX]; - return opendir(rpath(ctx, path, buffer)); + *dir = opendir(rpath(ctx, path, buffer)); + return *dir ? 0 : -errno; } static void local_rewinddir(FsContext *ctx, DIR *dir) @@ -162,12 +164,15 @@ static void local_rewinddir(FsContext *ctx, DIR *dir) static off_t local_telldir(FsContext *ctx, DIR *dir) { - return telldir(dir); + int ret = telldir(dir); + return ret == -1 ? -errno : ret; } -static struct dirent *local_readdir(FsContext *ctx, DIR *dir) +static int local_readdir(FsContext *ctx, DIR *dir, struct dirent **dirent) { - return readdir(dir); + *dirent = readdir(dir); + return *dirent ? 0 : -errno; + } static void local_seekdir(FsContext *ctx, DIR *dir, off_t off) @@ -178,14 +183,17 @@ static void local_seekdir(FsContext *ctx, DIR *dir, off_t off) static ssize_t local_preadv(FsContext *ctx, int fd, const struct iovec *iov, int iovcnt, off_t offset) { + int err; #ifdef CONFIG_PREADV - return preadv(fd, iov, iovcnt, offset); + err = preadv(fd, iov, iovcnt, offset); + return err == -1 ? -errno : err; #else int err = lseek(fd, offset, SEEK_SET); if (err == -1) { - return err; + return -errno; } else { - return readv(fd, iov, iovcnt); + err = readv(fd, iov, iovcnt); + return err == -1 ? -errno : err; } #endif } @@ -193,14 +201,17 @@ static ssize_t local_preadv(FsContext *ctx, int fd, const struct iovec *iov, static ssize_t local_pwritev(FsContext *ctx, int fd, const struct iovec *iov, int iovcnt, off_t offset) { + int err; #ifdef CONFIG_PREADV - return pwritev(fd, iov, iovcnt, offset); + err = pwritev(fd, iov, iovcnt, offset); + return err == -1 ? -errno : err; #else int err = lseek(fd, offset, SEEK_SET); if (err == -1) { - return err; + return -errno; } else { - return writev(fd, iov, iovcnt); + err = writev(fd, iov, iovcnt); + return err == -1 ? -errno : err; } #endif } @@ -208,13 +219,16 @@ static ssize_t local_pwritev(FsContext *ctx, int fd, const struct iovec *iov, static int local_chmod(FsContext *fs_ctx, const char *path, FsCred *credp) { char buffer[PATH_MAX]; + int ret; + if (fs_ctx->fs_sm == SM_MAPPED) { return local_set_xattr(rpath(fs_ctx, path, buffer), credp); } else if ((fs_ctx->fs_sm == SM_PASSTHROUGH) || (fs_ctx->fs_sm == SM_NONE)) { - return chmod(rpath(fs_ctx, path, buffer), credp->fc_mode); + ret = chmod(rpath(fs_ctx, path, buffer), credp->fc_mode); + return ret == -1 ? -errno : ret; } - return -1; + return -ENOTSUP; } static int local_mknod(FsContext *fs_ctx, const char *path, FsCred *credp) @@ -228,7 +242,7 @@ static int local_mknod(FsContext *fs_ctx, const char *path, FsCred *credp) err = mknod(rpath(fs_ctx, path, buffer), SM_LOCAL_MODE_BITS|S_IFREG, 0); if (err == -1) { - return err; + return -errno; } local_set_xattr(rpath(fs_ctx, path, buffer), credp); if (err == -1) { @@ -240,7 +254,7 @@ static int local_mknod(FsContext *fs_ctx, const char *path, FsCred *credp) err = mknod(rpath(fs_ctx, path, buffer), credp->fc_mode, credp->fc_rdev); if (err == -1) { - return err; + return -errno; } err = local_post_create_passthrough(fs_ctx, path, credp); if (err == -1) { @@ -248,12 +262,12 @@ static int local_mknod(FsContext *fs_ctx, const char *path, FsCred *credp) goto err_end; } } - return err; + return 0; err_end: remove(rpath(fs_ctx, path, buffer)); errno = serrno; - return err; + return -errno; } static int local_mkdir(FsContext *fs_ctx, const char *path, FsCred *credp) @@ -266,7 +280,7 @@ static int local_mkdir(FsContext *fs_ctx, const char *path, FsCred *credp) if (fs_ctx->fs_sm == SM_MAPPED) { err = mkdir(rpath(fs_ctx, path, buffer), SM_LOCAL_DIR_MODE_BITS); if (err == -1) { - return err; + return -errno; } credp->fc_mode = credp->fc_mode|S_IFDIR; err = local_set_xattr(rpath(fs_ctx, path, buffer), credp); @@ -278,7 +292,7 @@ static int local_mkdir(FsContext *fs_ctx, const char *path, FsCred *credp) (fs_ctx->fs_sm == SM_NONE)) { err = mkdir(rpath(fs_ctx, path, buffer), credp->fc_mode); if (err == -1) { - return err; + return -errno; } err = local_post_create_passthrough(fs_ctx, path, credp); if (err == -1) { @@ -286,12 +300,12 @@ static int local_mkdir(FsContext *fs_ctx, const char *path, FsCred *credp) goto err_end; } } - return err; + return 0; err_end: remove(rpath(fs_ctx, path, buffer)); errno = serrno; - return err; + return -errno; } static int local_fstat(FsContext *fs_ctx, int fd, struct stat *stbuf) @@ -299,7 +313,7 @@ static int local_fstat(FsContext *fs_ctx, int fd, struct stat *stbuf) int err; err = fstat(fd, stbuf); if (err) { - return err; + return -errno; } if (fs_ctx->fs_sm == SM_MAPPED) { /* Actual credentials are part of extended attrs */ @@ -321,7 +335,7 @@ static int local_fstat(FsContext *fs_ctx, int fd, struct stat *stbuf) stbuf->st_rdev = tmp_dev; } } - return err; + return 0; } static int local_open2(FsContext *fs_ctx, const char *path, int flags, @@ -336,7 +350,7 @@ static int local_open2(FsContext *fs_ctx, const char *path, int flags, if (fs_ctx->fs_sm == SM_MAPPED) { fd = open(rpath(fs_ctx, path, buffer), flags, SM_LOCAL_MODE_BITS); if (fd == -1) { - return fd; + return -errno; } credp->fc_mode = credp->fc_mode|S_IFREG; /* Set cleint credentials in xattr */ @@ -349,7 +363,7 @@ static int local_open2(FsContext *fs_ctx, const char *path, int flags, (fs_ctx->fs_sm == SM_NONE)) { fd = open(rpath(fs_ctx, path, buffer), flags, credp->fc_mode); if (fd == -1) { - return fd; + return -errno; } err = local_post_create_passthrough(fs_ctx, path, credp); if (err == -1) { @@ -363,7 +377,7 @@ err_end: close(fd); remove(rpath(fs_ctx, path, buffer)); errno = serrno; - return err; + return -errno; } @@ -381,7 +395,7 @@ static int local_symlink(FsContext *fs_ctx, const char *oldpath, fd = open(rpath(fs_ctx, newpath, buffer), O_CREAT|O_EXCL|O_RDWR, SM_LOCAL_MODE_BITS); if (fd == -1) { - return fd; + return -errno; } /* Write the oldpath (target) to the file. */ oldpath_size = strlen(oldpath); @@ -407,7 +421,7 @@ static int local_symlink(FsContext *fs_ctx, const char *oldpath, (fs_ctx->fs_sm == SM_NONE)) { err = symlink(oldpath, rpath(fs_ctx, newpath, buffer)); if (err) { - return err; + return -errno; } err = lchown(rpath(fs_ctx, newpath, buffer), credp->fc_uid, credp->fc_gid); @@ -423,25 +437,24 @@ static int local_symlink(FsContext *fs_ctx, const char *oldpath, err = 0; } } - return err; + return 0; err_end: remove(rpath(fs_ctx, newpath, buffer)); errno = serrno; - return err; + return -errno; } static int local_link(FsContext *ctx, const char *oldpath, const char *newpath) { char buffer[PATH_MAX], buffer1[PATH_MAX]; - - return link(rpath(ctx, oldpath, buffer), rpath(ctx, newpath, buffer1)); + return link(rpath(ctx, oldpath, buffer), rpath(ctx, newpath, buffer1)) == -1 ? -errno : 0; } static int local_truncate(FsContext *ctx, const char *path, off_t size) { char buffer[PATH_MAX]; - return truncate(rpath(ctx, path, buffer), size); + return truncate(rpath(ctx, path, buffer), size) == -1 ? -errno : 0; } static int local_rename(FsContext *ctx, const char *oldpath, @@ -449,7 +462,7 @@ static int local_rename(FsContext *ctx, const char *oldpath, { char buffer[PATH_MAX], buffer1[PATH_MAX]; - return rename(rpath(ctx, oldpath, buffer), rpath(ctx, newpath, buffer1)); + return rename(rpath(ctx, oldpath, buffer), rpath(ctx, newpath, buffer1)) == -1 ? -errno : 0; } static int local_chown(FsContext *fs_ctx, const char *path, FsCred *credp) @@ -458,15 +471,15 @@ static int local_chown(FsContext *fs_ctx, const char *path, FsCred *credp) if ((credp->fc_uid == -1 && credp->fc_gid == -1) || (fs_ctx->fs_sm == SM_PASSTHROUGH)) { return lchown(rpath(fs_ctx, path, buffer), credp->fc_uid, - credp->fc_gid); + credp->fc_gid) == -1 ? -errno : 0; } else if (fs_ctx->fs_sm == SM_MAPPED) { return local_set_xattr(rpath(fs_ctx, path, buffer), credp); } else if ((fs_ctx->fs_sm == SM_PASSTHROUGH) || (fs_ctx->fs_sm == SM_NONE)) { return lchown(rpath(fs_ctx, path, buffer), credp->fc_uid, - credp->fc_gid); + credp->fc_gid) == -1 ? -errno : 0; } - return -1; + return -EINVAL; } static int local_utimensat(FsContext *s, const char *path, @@ -480,22 +493,22 @@ static int local_utimensat(FsContext *s, const char *path, static int local_remove(FsContext *ctx, const char *path) { char buffer[PATH_MAX]; - return remove(rpath(ctx, path, buffer)); + return remove(rpath(ctx, path, buffer)) == -1 ? -errno : 0; } static int local_fsync(FsContext *ctx, int fd, int datasync) { if (datasync) { - return qemu_fdatasync(fd); + return qemu_fdatasync(fd) == -1 ? -errno : 0; } else { - return fsync(fd); + return fsync(fd) == -1 ? -errno : 0; } } static int local_statfs(FsContext *s, const char *path, struct statfs *stbuf) { char buffer[PATH_MAX]; - return statfs(rpath(s, path, buffer), stbuf); + return statfs(rpath(s, path, buffer), stbuf) == -1 ? -errno : 0; } static ssize_t local_lgetxattr(FsContext *ctx, const char *path, diff --git a/hw/9pfs/virtio-9p-xattr.c b/hw/9pfs/virtio-9p-xattr.c index bde0b7f..a5a6134 100644 --- a/hw/9pfs/virtio-9p-xattr.c +++ b/hw/9pfs/virtio-9p-xattr.c @@ -32,9 +32,14 @@ static XattrOperations *get_xattr_operations(XattrOperations **h, ssize_t v9fs_get_xattr(FsContext *ctx, const char *path, const char *name, void *value, size_t size) { + int ret; XattrOperations *xops = get_xattr_operations(ctx->xops, name); if (xops) { - return xops->getxattr(ctx, path, name, value, size); + ret = xops->getxattr(ctx, path, name, value, size); + if (ret < 0) { + return -errno; + } + return ret; } errno = -EOPNOTSUPP; return -1; @@ -118,9 +123,14 @@ err_out: int v9fs_set_xattr(FsContext *ctx, const char *path, const char *name, void *value, size_t size, int flags) { + int ret; XattrOperations *xops = get_xattr_operations(ctx->xops, name); if (xops) { - return xops->setxattr(ctx, path, name, value, size, flags); + ret = xops->setxattr(ctx, path, name, value, size, flags); + if (ret < 0) { + return -errno; + } + return ret; } errno = -EOPNOTSUPP; return -1; @@ -130,9 +140,14 @@ int v9fs_set_xattr(FsContext *ctx, const char *path, const char *name, int v9fs_remove_xattr(FsContext *ctx, const char *path, const char *name) { + int ret; XattrOperations *xops = get_xattr_operations(ctx->xops, name); if (xops) { - return xops->removexattr(ctx, path, name); + ret = xops->removexattr(ctx, path, name); + if (ret < 0) { + return -errno; + } + return ret; } errno = -EOPNOTSUPP; return -1;