Hi JV,
Any progress regarding merging this patch (and the fsync patch I submitted)?
Is there anything I can do to assist/speed the process?
Thanks
Sassan
On 8 June 2011 17:21, Sassan Panahinejad <sas...@sassan.me.uk> wrote:
> 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 <sas...@sassan.me.uk>
>
> ---
> 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;
> --
> 1.7.4.1
>
>
