Re: [Qemu-devel] [PATCH] Clean up virtio-9p error handling code

On 28 June 2011 15:22, Venkateswararao Jujjuri <address@hidden> wrote:

On 06/28/2011 05:25 AM, Sassan Panahinejad wrote:
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?

Sussan, Thanks for the ping. :)

Everything is on hold waiting for 0.15 tag; Including threading/coroutine patches.
As soon as we go in with the coroutines, you can just rebase your patch and we should be
ready to go. I am waiting too. :-D

Thanks for the info :)

Thanks,
JV

Thanks
Sassan

On 8 June 2011 17:21, Sassan Panahinejad <address@hidden> 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 <address@hidden>

---
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

From:	Sassan Panahinejad
Subject:	Re: [Qemu-devel] [PATCH] Clean up virtio-9p error handling code
Date:	Tue, 28 Jun 2011 18:00:03 +0100