[Qemu-devel] [PATCH 1/5] 9pfs-proxy: simplify v9fs_request(), P1

[Qemu-devel] [PATCH 1/5] 9pfs-proxy: simplify v9fs_request(), P1

[Michael Tokarev]

This simplifies code in v9fs_request() a bit by replacing several
ifs with a common variable check and rearranging error/cleanup
code a bit.

Signet-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 hw/9pfs/virtio-9p-proxy.c | 48 ++++++++++++++++++++---------------------------
 1 file changed, 20 insertions(+), 28 deletions(-)

diff --git a/hw/9pfs/virtio-9p-proxy.c b/hw/9pfs/virtio-9p-proxy.c
index 59c7445..f252fe4 100644
--- a/hw/9pfs/virtio-9p-proxy.c
+++ b/hw/9pfs/virtio-9p-proxy.c
@@ -299,7 +299,7 @@ static int v9fs_request(V9fsProxy *proxy, int type,
     dev_t rdev;
     va_list ap;
     int size = 0;
-    int retval = 0;
+    int retval = 0, err;
     uint64_t offset;
     ProxyHeader header = { 0, 0};
     struct timespec spec[2];
@@ -310,10 +310,11 @@ static int v9fs_request(V9fsProxy *proxy, int type,
 
     qemu_mutex_lock(&proxy->mutex);
 
-    if (proxy->sockfd == -1) {
+    if (proxy->sockfd < 0) {
         retval = -EIO;
-        goto err_out;
+        goto out;
     }
+
     iovec = &proxy->out_iovec;
     reply = &proxy->in_iovec;
     va_start(ap, fmt);
@@ -529,15 +530,15 @@ static int v9fs_request(V9fsProxy *proxy, int type,
     va_end(ap);
 
     if (retval < 0) {
-        goto err_out;
+        goto out;
     }
 
     /* marshal the header details */
     proxy_marshal(iovec, 0, "dd", header.type, header.size);
     header.size += PROXY_HDR_SZ;
 
-    retval = qemu_write_full(proxy->sockfd, iovec->iov_base, header.size);
-    if (retval != header.size) {
+    err = qemu_write_full(proxy->sockfd, iovec->iov_base, header.size);
+    if (err != header.size) {
         goto close_error;
     }
 
@@ -548,9 +549,7 @@ static int v9fs_request(V9fsProxy *proxy, int type,
          * A file descriptor is returned as response for
          * T_OPEN,T_CREATE on success
          */
-        if (v9fs_receivefd(proxy->sockfd, &retval) < 0) {
-            goto close_error;
-        }
+        err = v9fs_receivefd(proxy->sockfd, &retval);
         break;
     case T_MKNOD:
     case T_MKDIR:
@@ -564,41 +563,34 @@ static int v9fs_request(V9fsProxy *proxy, int type,
     case T_REMOVE:
     case T_LSETXATTR:
     case T_LREMOVEXATTR:
-        if (v9fs_receive_status(proxy, reply, &retval) < 0) {
-            goto close_error;
-        }
+        err = v9fs_receive_status(proxy, reply, &retval);
         break;
     case T_LSTAT:
     case T_READLINK:
     case T_STATFS:
     case T_GETVERSION:
-        if (v9fs_receive_response(proxy, type, &retval, response) < 0) {
-            goto close_error;
-        }
+        err = v9fs_receive_response(proxy, type, &retval, response);
         break;
     case T_LGETXATTR:
     case T_LLISTXATTR:
         if (!size) {
-            if (v9fs_receive_status(proxy, reply, &retval) < 0) {
-                goto close_error;
-            }
+            err = v9fs_receive_status(proxy, reply, &retval);
         } else {
-            if (v9fs_receive_response(proxy, type, &retval, response) < 0) {
-                goto close_error;
-            }
+            err = v9fs_receive_response(proxy, type, &retval, response);
         }
         break;
     }
 
-err_out:
-    qemu_mutex_unlock(&proxy->mutex);
-    return retval;
-
+    if (err < 0) {
 close_error:
-    close(proxy->sockfd);
-    proxy->sockfd = -1;
+        close(proxy->sockfd);
+        proxy->sockfd = -1;
+        retval = -EIO;
+    }
+
+out:
     qemu_mutex_unlock(&proxy->mutex);
-    return -EIO;
+    return retval;
 }
 
 static int proxy_lstat(FsContext *fs_ctx, V9fsPath *fs_path, struct stat *stbuf)
-- 
2.1.4

[Qemu-devel] [PATCH 2/5] 9pfs-proxy: simplify v9fs_request(), P2

[Michael Tokarev]

Move common code in all cases of the switch statement
to be outside of the statement.

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 hw/9pfs/virtio-9p-proxy.c | 85 +++--------------------------------------------
 1 file changed, 4 insertions(+), 81 deletions(-)

diff --git a/hw/9pfs/virtio-9p-proxy.c b/hw/9pfs/virtio-9p-proxy.c
index f252fe4..13b654d 100644
--- a/hw/9pfs/virtio-9p-proxy.c
+++ b/hw/9pfs/virtio-9p-proxy.c
@@ -323,10 +323,6 @@ static int v9fs_request(V9fsProxy *proxy, int type,
         path = va_arg(ap, V9fsString *);
         flags = va_arg(ap, int);
         retval = proxy_marshal(iovec, PROXY_HDR_SZ, "sd", path, flags);
-        if (retval > 0) {
-            header.size = retval;
-            header.type = T_OPEN;
-        }
         break;
     case T_CREATE:
         path = va_arg(ap, V9fsString *);
@@ -336,10 +332,6 @@ static int v9fs_request(V9fsProxy *proxy, int type,
         gid = va_arg(ap, int);
         retval = proxy_marshal(iovec, PROXY_HDR_SZ, "sdddd", path,
                                     flags, mode, uid, gid);
-        if (retval > 0) {
-            header.size = retval;
-            header.type = T_CREATE;
-        }
         break;
     case T_MKNOD:
         path = va_arg(ap, V9fsString *);
@@ -349,10 +341,6 @@ static int v9fs_request(V9fsProxy *proxy, int type,
         gid = va_arg(ap, int);
         retval = proxy_marshal(iovec, PROXY_HDR_SZ, "ddsdq",
                                     uid, gid, path, mode, rdev);
-        if (retval > 0) {
-            header.size = retval;
-            header.type = T_MKNOD;
-        }
         break;
     case T_MKDIR:
         path = va_arg(ap, V9fsString *);
@@ -361,10 +349,6 @@ static int v9fs_request(V9fsProxy *proxy, int type,
         gid = va_arg(ap, int);
         retval = proxy_marshal(iovec, PROXY_HDR_SZ, "ddsd",
                                     uid, gid, path, mode);
-        if (retval > 0) {
-            header.size = retval;
-            header.type = T_MKDIR;
-        }
         break;
     case T_SYMLINK:
         oldpath = va_arg(ap, V9fsString *);
@@ -373,73 +357,41 @@ static int v9fs_request(V9fsProxy *proxy, int type,
         gid = va_arg(ap, int);
         retval = proxy_marshal(iovec, PROXY_HDR_SZ, "ddss",
                                     uid, gid, oldpath, path);
-        if (retval > 0) {
-            header.size = retval;
-            header.type = T_SYMLINK;
-        }
         break;
     case T_LINK:
         oldpath = va_arg(ap, V9fsString *);
         path = va_arg(ap, V9fsString *);
         retval = proxy_marshal(iovec, PROXY_HDR_SZ, "ss",
                                     oldpath, path);
-        if (retval > 0) {
-            header.size = retval;
-            header.type = T_LINK;
-        }
         break;
     case T_LSTAT:
         path = va_arg(ap, V9fsString *);
         retval = proxy_marshal(iovec, PROXY_HDR_SZ, "s", path);
-        if (retval > 0) {
-            header.size = retval;
-            header.type = T_LSTAT;
-        }
         break;
     case T_READLINK:
         path = va_arg(ap, V9fsString *);
         size = va_arg(ap, int);
         retval = proxy_marshal(iovec, PROXY_HDR_SZ, "sd", path, size);
-        if (retval > 0) {
-            header.size = retval;
-            header.type = T_READLINK;
-        }
         break;
     case T_STATFS:
         path = va_arg(ap, V9fsString *);
         retval = proxy_marshal(iovec, PROXY_HDR_SZ, "s", path);
-        if (retval > 0) {
-            header.size = retval;
-            header.type = T_STATFS;
-        }
         break;
     case T_CHMOD:
         path = va_arg(ap, V9fsString *);
         mode = va_arg(ap, int);
         retval = proxy_marshal(iovec, PROXY_HDR_SZ, "sd", path, mode);
-        if (retval > 0) {
-            header.size = retval;
-            header.type = T_CHMOD;
-        }
         break;
     case T_CHOWN:
         path = va_arg(ap, V9fsString *);
         uid = va_arg(ap, int);
         gid = va_arg(ap, int);
         retval = proxy_marshal(iovec, PROXY_HDR_SZ, "sdd", path, uid, gid);
-        if (retval > 0) {
-            header.size = retval;
-            header.type = T_CHOWN;
-        }
         break;
     case T_TRUNCATE:
         path = va_arg(ap, V9fsString *);
         offset = va_arg(ap, uint64_t);
         retval = proxy_marshal(iovec, PROXY_HDR_SZ, "sq", path, offset);
-        if (retval > 0) {
-            header.size = retval;
-            header.type = T_TRUNCATE;
-        }
         break;
     case T_UTIME:
         path = va_arg(ap, V9fsString *);
@@ -450,27 +402,15 @@ static int v9fs_request(V9fsProxy *proxy, int type,
         retval = proxy_marshal(iovec, PROXY_HDR_SZ, "sqqqq", path,
                                     spec[0].tv_sec, spec[1].tv_nsec,
                                     spec[1].tv_sec, spec[1].tv_nsec);
-        if (retval > 0) {
-            header.size = retval;
-            header.type = T_UTIME;
-        }
         break;
     case T_RENAME:
         oldpath = va_arg(ap, V9fsString *);
         path = va_arg(ap, V9fsString *);
         retval = proxy_marshal(iovec, PROXY_HDR_SZ, "ss", oldpath, path);
-        if (retval > 0) {
-            header.size = retval;
-            header.type = T_RENAME;
-        }
         break;
     case T_REMOVE:
         path = va_arg(ap, V9fsString *);
         retval = proxy_marshal(iovec, PROXY_HDR_SZ, "s", path);
-        if (retval > 0) {
-            header.size = retval;
-            header.type = T_REMOVE;
-        }
         break;
     case T_LGETXATTR:
         size = va_arg(ap, int);
@@ -478,19 +418,11 @@ static int v9fs_request(V9fsProxy *proxy, int type,
         name = va_arg(ap, V9fsString *);
         retval = proxy_marshal(iovec, PROXY_HDR_SZ,
                                     "dss", size, path, name);
-        if (retval > 0) {
-            header.size = retval;
-            header.type = T_LGETXATTR;
-        }
         break;
     case T_LLISTXATTR:
         size = va_arg(ap, int);
         path = va_arg(ap, V9fsString *);
         retval = proxy_marshal(iovec, PROXY_HDR_SZ, "ds", size, path);
-        if (retval > 0) {
-            header.size = retval;
-            header.type = T_LLISTXATTR;
-        }
         break;
     case T_LSETXATTR:
         path = va_arg(ap, V9fsString *);
@@ -500,27 +432,15 @@ static int v9fs_request(V9fsProxy *proxy, int type,
         flags = va_arg(ap, int);
         retval = proxy_marshal(iovec, PROXY_HDR_SZ, "sssdd",
                                     path, name, value, size, flags);
-        if (retval > 0) {
-            header.size = retval;
-            header.type = T_LSETXATTR;
-        }
         break;
     case T_LREMOVEXATTR:
         path = va_arg(ap, V9fsString *);
         name = va_arg(ap, V9fsString *);
         retval = proxy_marshal(iovec, PROXY_HDR_SZ, "ss", path, name);
-        if (retval > 0) {
-            header.size = retval;
-            header.type = T_LREMOVEXATTR;
-        }
         break;
     case T_GETVERSION:
         path = va_arg(ap, V9fsString *);
         retval = proxy_marshal(iovec, PROXY_HDR_SZ, "s", path);
-        if (retval > 0) {
-            header.size = retval;
-            header.type = T_GETVERSION;
-        }
         break;
     default:
         error_report("Invalid type %d", type);
@@ -529,7 +449,10 @@ static int v9fs_request(V9fsProxy *proxy, int type,
     }
     va_end(ap);
 
-    if (retval < 0) {
+    if (retval > 0) {
+        header.size = retval;
+        header.type = type;
+    } else if (retval < 0) {
         goto out;
     }
 
-- 
2.1.4

[Qemu-devel] [PATCH 3/5] 9pfs-proxy: simplify error handling

[Michael Tokarev]

All filesystem methods that call common v9fs_request() function
also convert return value to errno.  Move this conversion to the
common function and remove redundand error handling in methods.

I didn't remove local `retval' variable in simple functions to
keep the code consistent.

Also, proxy_truncate() seem to prefer zero successful return
instead of returning whatever the helper returned, maybe this
should be changed.

This also removes (harmless) double call to v9fs_string_free()
in proxy_mkdir().

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 hw/9pfs/virtio-9p-proxy.c | 74 ++++-------------------------------------------
 1 file changed, 5 insertions(+), 69 deletions(-)

diff --git a/hw/9pfs/virtio-9p-proxy.c b/hw/9pfs/virtio-9p-proxy.c
index 13b654d..59c7650 100644
--- a/hw/9pfs/virtio-9p-proxy.c
+++ b/hw/9pfs/virtio-9p-proxy.c
@@ -513,6 +513,10 @@ close_error:
 
 out:
     qemu_mutex_unlock(&proxy->mutex);
+    if (retval < 0) {
+        errno = -retval;
+        retval = -1;
+    }
     return retval;
 }
 
@@ -520,10 +524,6 @@ static int proxy_lstat(FsContext *fs_ctx, V9fsPath *fs_path, struct stat *stbuf)
 {
     int retval;
     retval = v9fs_request(fs_ctx->private, T_LSTAT, stbuf, "s", fs_path);
-    if (retval < 0) {
-        errno = -retval;
-        return -1;
-    }
     return retval;
 }
 
@@ -534,7 +534,6 @@ static ssize_t proxy_readlink(FsContext *fs_ctx, V9fsPath *fs_path,
     retval = v9fs_request(fs_ctx->private, T_READLINK, buf, "sd",
                           fs_path, bufsz);
     if (retval < 0) {
-        errno = -retval;
         return -1;
     }
     return strlen(buf);
@@ -554,10 +553,6 @@ static int proxy_open(FsContext *ctx, V9fsPath *fs_path,
                       int flags, V9fsFidOpenState *fs)
 {
     fs->fd = v9fs_request(ctx->private, T_OPEN, NULL, "sd", fs_path, flags);
-    if (fs->fd < 0) {
-        errno = -fs->fd;
-        fs->fd = -1;
-    }
     return fs->fd;
 }
 
@@ -569,7 +564,6 @@ static int proxy_opendir(FsContext *ctx,
     fs->dir = NULL;
     fd = v9fs_request(ctx->private, T_OPEN, NULL, "sd", fs_path, O_DIRECTORY);
     if (fd < 0) {
-        errno = -fd;
         return -1;
     }
     fs->dir = fdopendir(fd);
@@ -655,9 +649,6 @@ static int proxy_chmod(FsContext *fs_ctx, V9fsPath *fs_path, FsCred *credp)
     int retval;
     retval = v9fs_request(fs_ctx->private, T_CHMOD, NULL, "sd",
                           fs_path, credp->fc_mode);
-    if (retval < 0) {
-        errno = -retval;
-    }
     return retval;
 }
 
@@ -674,10 +665,6 @@ static int proxy_mknod(FsContext *fs_ctx, V9fsPath *dir_path,
                           &fullname, credp->fc_mode, credp->fc_rdev,
                           credp->fc_uid, credp->fc_gid);
     v9fs_string_free(&fullname);
-    if (retval < 0) {
-        errno = -retval;
-        retval = -1;
-    }
     return retval;
 }
 
@@ -693,11 +680,6 @@ static int proxy_mkdir(FsContext *fs_ctx, V9fsPath *dir_path,
     retval = v9fs_request(fs_ctx->private, T_MKDIR, NULL, "sddd", &fullname,
                           credp->fc_mode, credp->fc_uid, credp->fc_gid);
     v9fs_string_free(&fullname);
-    if (retval < 0) {
-        errno = -retval;
-        retval = -1;
-    }
-    v9fs_string_free(&fullname);
     return retval;
 }
 
@@ -726,10 +708,6 @@ static int proxy_open2(FsContext *fs_ctx, V9fsPath *dir_path, const char *name,
                           &fullname, flags, credp->fc_mode,
                           credp->fc_uid, credp->fc_gid);
     v9fs_string_free(&fullname);
-    if (fs->fd < 0) {
-        errno = -fs->fd;
-        fs->fd = -1;
-    }
     return fs->fd;
 }
 
@@ -749,10 +727,6 @@ static int proxy_symlink(FsContext *fs_ctx, const char *oldpath,
                           &target, &fullname, credp->fc_uid, credp->fc_gid);
     v9fs_string_free(&fullname);
     v9fs_string_free(&target);
-    if (retval < 0) {
-        errno = -retval;
-        retval = -1;
-    }
     return retval;
 }
 
@@ -767,20 +741,14 @@ static int proxy_link(FsContext *ctx, V9fsPath *oldpath,
 
     retval = v9fs_request(ctx->private, T_LINK, NULL, "ss", oldpath, &newpath);
     v9fs_string_free(&newpath);
-    if (retval < 0) {
-        errno = -retval;
-        retval = -1;
-    }
     return retval;
 }
 
 static int proxy_truncate(FsContext *ctx, V9fsPath *fs_path, off_t size)
 {
     int retval;
-
     retval = v9fs_request(ctx->private, T_TRUNCATE, NULL, "sq", fs_path, size);
     if (retval < 0) {
-        errno = -retval;
         return -1;
     }
     return 0;
@@ -801,9 +769,6 @@ static int proxy_rename(FsContext *ctx, const char *oldpath,
                           &oldname, &newname);
     v9fs_string_free(&oldname);
     v9fs_string_free(&newname);
-    if (retval < 0) {
-        errno = -retval;
-    }
     return retval;
 }
 
@@ -812,9 +777,6 @@ static int proxy_chown(FsContext *fs_ctx, V9fsPath *fs_path, FsCred *credp)
     int retval;
     retval = v9fs_request(fs_ctx->private, T_CHOWN, NULL, "sdd",
                           fs_path, credp->fc_uid, credp->fc_gid);
-    if (retval < 0) {
-        errno = -retval;
-    }
     return retval;
 }
 
@@ -826,9 +788,6 @@ static int proxy_utimensat(FsContext *s, V9fsPath *fs_path,
                           fs_path,
                           buf[0].tv_sec, buf[0].tv_nsec,
                           buf[1].tv_sec, buf[1].tv_nsec);
-    if (retval < 0) {
-        errno = -retval;
-    }
     return retval;
 }
 
@@ -840,9 +799,6 @@ static int proxy_remove(FsContext *ctx, const char *path)
     v9fs_string_sprintf(&name, "%s", path);
     retval = v9fs_request(ctx->private, T_REMOVE, NULL, "s", &name);
     v9fs_string_free(&name);
-    if (retval < 0) {
-        errno = -retval;
-    }
     return retval;
 }
 
@@ -868,10 +824,6 @@ static int proxy_statfs(FsContext *s, V9fsPath *fs_path, struct statfs *stbuf)
 {
     int retval;
     retval = v9fs_request(s->private, T_STATFS, stbuf, "s", fs_path);
-    if (retval < 0) {
-        errno = -retval;
-        return -1;
-    }
     return retval;
 }
 
@@ -886,9 +838,6 @@ static ssize_t proxy_lgetxattr(FsContext *ctx, V9fsPath *fs_path,
     retval = v9fs_request(ctx->private, T_LGETXATTR, value, "dss", size,
                           fs_path, &xname);
     v9fs_string_free(&xname);
-    if (retval < 0) {
-        errno = -retval;
-    }
     return retval;
 }
 
@@ -897,10 +846,7 @@ static ssize_t proxy_llistxattr(FsContext *ctx, V9fsPath *fs_path,
 {
     int retval;
     retval = v9fs_request(ctx->private, T_LLISTXATTR, value, "ds", size,
-                        fs_path);
-    if (retval < 0) {
-        errno = -retval;
-    }
+                          fs_path);
     return retval;
 }
 
@@ -922,9 +868,6 @@ static int proxy_lsetxattr(FsContext *ctx, V9fsPath *fs_path, const char *name,
                           fs_path, &xname, &xvalue, size, flags);
     v9fs_string_free(&xname);
     v9fs_string_free(&xvalue);
-    if (retval < 0) {
-        errno = -retval;
-    }
     return retval;
 }
 
@@ -939,9 +882,6 @@ static int proxy_lremovexattr(FsContext *ctx, V9fsPath *fs_path,
     retval = v9fs_request(ctx->private, T_LREMOVEXATTR, NULL, "ss",
                           fs_path, &xname);
     v9fs_string_free(&xname);
-    if (retval < 0) {
-        errno = -retval;
-    }
     return retval;
 }
 
@@ -1005,10 +945,6 @@ static int proxy_ioc_getversion(FsContext *fs_ctx, V9fsPath *path,
         return -1;
     }
     err = v9fs_request(fs_ctx->private, T_GETVERSION, st_gen, "s", path);
-    if (err < 0) {
-        errno = -err;
-        err = -1;
-    }
     return err;
 }
 
-- 
2.1.4

[Qemu-devel] [PATCH 4/5] 9pfs-proxy: rename a few local variables for consistency

[Michael Tokarev]

All functions which deal with v9fs_request() use `retval'
variable to hold result of the request (except some which
use this value for other purposes too), but 3 use different
name (ret or err).  Rename these 3 for consistency.
There's no actual code changes.

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 hw/9pfs/virtio-9p-proxy.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/hw/9pfs/virtio-9p-proxy.c b/hw/9pfs/virtio-9p-proxy.c
index 59c7650..c7b2f16 100644
--- a/hw/9pfs/virtio-9p-proxy.c
+++ b/hw/9pfs/virtio-9p-proxy.c
@@ -903,7 +903,7 @@ static int proxy_renameat(FsContext *ctx, V9fsPath *olddir,
                           const char *old_name, V9fsPath *newdir,
                           const char *new_name)
 {
-    int ret;
+    int retval;
     V9fsString old_full_name, new_full_name;
 
     v9fs_string_init(&old_full_name);
@@ -912,30 +912,30 @@ static int proxy_renameat(FsContext *ctx, V9fsPath *olddir,
     v9fs_string_sprintf(&old_full_name, "%s/%s", olddir->data, old_name);
     v9fs_string_sprintf(&new_full_name, "%s/%s", newdir->data, new_name);
 
-    ret = proxy_rename(ctx, old_full_name.data, new_full_name.data);
+    retval = proxy_rename(ctx, old_full_name.data, new_full_name.data);
     v9fs_string_free(&old_full_name);
     v9fs_string_free(&new_full_name);
-    return ret;
+    return retval;
 }
 
 static int proxy_unlinkat(FsContext *ctx, V9fsPath *dir,
                           const char *name, int flags)
 {
-    int ret;
+    int retval;
     V9fsString fullname;
     v9fs_string_init(&fullname);
 
     v9fs_string_sprintf(&fullname, "%s/%s", dir->data, name);
-    ret = proxy_remove(ctx, fullname.data);
+    retval = proxy_remove(ctx, fullname.data);
     v9fs_string_free(&fullname);
 
-    return ret;
+    return retval;
 }
 
 static int proxy_ioc_getversion(FsContext *fs_ctx, V9fsPath *path,
                                 mode_t st_mode, uint64_t *st_gen)
 {
-    int err;
+    int retval;
 
     /* Do not try to open special files like device nodes, fifos etc
      * we can get fd for regular files and directories only
@@ -944,8 +944,8 @@ static int proxy_ioc_getversion(FsContext *fs_ctx, V9fsPath *path,
         errno = ENOTTY;
         return -1;
     }
-    err = v9fs_request(fs_ctx->private, T_GETVERSION, st_gen, "s", path);
-    return err;
+    retval = v9fs_request(fs_ctx->private, T_GETVERSION, st_gen, "s", path);
+    return retval;
 }
 
 static int connect_namedsocket(const char *path)
-- 
2.1.4

[Qemu-devel] [PATCH 5/5] 9pfs-proxy: tiny cleanups in proxy_pwritev and proxy_preadv

[Michael Tokarev]

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 hw/9pfs/virtio-9p-proxy.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/hw/9pfs/virtio-9p-proxy.c b/hw/9pfs/virtio-9p-proxy.c
index c7b2f16..8e7ce3f 100644
--- a/hw/9pfs/virtio-9p-proxy.c
+++ b/hw/9pfs/virtio-9p-proxy.c
@@ -606,7 +606,7 @@ static ssize_t proxy_preadv(FsContext *ctx, V9fsFidOpenState *fs,
     return preadv(fs->fd, iov, iovcnt, offset);
 #else
     int err = lseek(fs->fd, offset, SEEK_SET);
-    if (err == -1) {
+    if (err < 0)
         return err;
     } else {
         return readv(fs->fd, iov, iovcnt);
@@ -623,10 +623,8 @@ static ssize_t proxy_pwritev(FsContext *ctx, V9fsFidOpenState *fs,
 #ifdef CONFIG_PREADV
     ret = pwritev(fs->fd, iov, iovcnt, offset);
 #else
-    int err = lseek(fs->fd, offset, SEEK_SET);
-    if (err == -1) {
-        return err;
-    } else {
+    ret = lseek(fs->fd, offset, SEEK_SET);
+    if (ret >= 0) {
         ret = writev(fs->fd, iov, iovcnt);
     }
 #endif
-- 
2.1.4

Re: [Qemu-devel] [PATCH 1/5] 9pfs-proxy: simplify v9fs_request(), P1

[Aneesh Kumar K.V]

Michael Tokarev <mjt@tls.msk.ru> writes:

> This simplifies code in v9fs_request() a bit by replacing several
> ifs with a common variable check and rearranging error/cleanup
> code a bit.

Is this -V2 of
http://mid.gmane.org/b98f675750ef0535cab41225240db1657fc2fe00.1425678142.git.mjt@msgid.tls.msk.ru

I am slightly confused with the patch series. It does split the patch as
I wanted, but i am not sure which one is the latest, so that i can start
applying them.

>
> Signet-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
>  hw/9pfs/virtio-9p-proxy.c | 48 ++++++++++++++++++++---------------------------
>  1 file changed, 20 insertions(+), 28 deletions(-)
>
> diff --git a/hw/9pfs/virtio-9p-proxy.c b/hw/9pfs/virtio-9p-proxy.c
> index 59c7445..f252fe4 100644
> --- a/hw/9pfs/virtio-9p-proxy.c
> +++ b/hw/9pfs/virtio-9p-proxy.c
> @@ -299,7 +299,7 @@ static int v9fs_request(V9fsProxy *proxy, int type,
>      dev_t rdev;
>      va_list ap;
>      int size = 0;
> -    int retval = 0;
> +    int retval = 0, err;
>      uint64_t offset;
>      ProxyHeader header = { 0, 0};
>      struct timespec spec[2];
> @@ -310,10 +310,11 @@ static int v9fs_request(V9fsProxy *proxy, int type,
>  
>      qemu_mutex_lock(&proxy->mutex);
>  
> -    if (proxy->sockfd == -1) {
> +    if (proxy->sockfd < 0) {
>          retval = -EIO;
> -        goto err_out;
> +        goto out;
>      }
> +
>      iovec = &proxy->out_iovec;
>      reply = &proxy->in_iovec;
>      va_start(ap, fmt);
> @@ -529,15 +530,15 @@ static int v9fs_request(V9fsProxy *proxy, int type,
>      va_end(ap);
>  
>      if (retval < 0) {
> -        goto err_out;
> +        goto out;
>      }
>  
>      /* marshal the header details */
>      proxy_marshal(iovec, 0, "dd", header.type, header.size);
>      header.size += PROXY_HDR_SZ;
>  
> -    retval = qemu_write_full(proxy->sockfd, iovec->iov_base, header.size);
> -    if (retval != header.size) {
> +    err = qemu_write_full(proxy->sockfd, iovec->iov_base, header.size);
> +    if (err != header.size) {
>          goto close_error;
>      }
>  
> @@ -548,9 +549,7 @@ static int v9fs_request(V9fsProxy *proxy, int type,
>           * A file descriptor is returned as response for
>           * T_OPEN,T_CREATE on success
>           */
> -        if (v9fs_receivefd(proxy->sockfd, &retval) < 0) {
> -            goto close_error;
> -        }
> +        err = v9fs_receivefd(proxy->sockfd, &retval);
>          break;
>      case T_MKNOD:
>      case T_MKDIR:
> @@ -564,41 +563,34 @@ static int v9fs_request(V9fsProxy *proxy, int type,
>      case T_REMOVE:
>      case T_LSETXATTR:
>      case T_LREMOVEXATTR:
> -        if (v9fs_receive_status(proxy, reply, &retval) < 0) {
> -            goto close_error;
> -        }
> +        err = v9fs_receive_status(proxy, reply, &retval);
>          break;
>      case T_LSTAT:
>      case T_READLINK:
>      case T_STATFS:
>      case T_GETVERSION:
> -        if (v9fs_receive_response(proxy, type, &retval, response) < 0) {
> -            goto close_error;
> -        }
> +        err = v9fs_receive_response(proxy, type, &retval, response);
>          break;
>      case T_LGETXATTR:
>      case T_LLISTXATTR:
>          if (!size) {
> -            if (v9fs_receive_status(proxy, reply, &retval) < 0) {
> -                goto close_error;
> -            }
> +            err = v9fs_receive_status(proxy, reply, &retval);
>          } else {
> -            if (v9fs_receive_response(proxy, type, &retval, response) < 0) {
> -                goto close_error;
> -            }
> +            err = v9fs_receive_response(proxy, type, &retval, response);
>          }
>          break;
>      }
>  
> -err_out:
> -    qemu_mutex_unlock(&proxy->mutex);
> -    return retval;
> -
> +    if (err < 0) {
>  close_error:
> -    close(proxy->sockfd);
> -    proxy->sockfd = -1;
> +        close(proxy->sockfd);
> +        proxy->sockfd = -1;
> +        retval = -EIO;
> +    }
> +
> +out:
>      qemu_mutex_unlock(&proxy->mutex);
> -    return -EIO;
> +    return retval;
>  }
>  
>  static int proxy_lstat(FsContext *fs_ctx, V9fsPath *fs_path, struct stat *stbuf)
> -- 
> 2.1.4

Re: [Qemu-devel] [PATCH 1/5] 9pfs-proxy: simplify v9fs_request(), P1

[Michael Tokarev]

08.03.2015 19:39, Aneesh Kumar K.V wrote:
> Michael Tokarev <mjt@tls.msk.ru> writes:
> 
>> This simplifies code in v9fs_request() a bit by replacing several
>> ifs with a common variable check and rearranging error/cleanup
>> code a bit.
> 
> Is this -V2 of
> http://mid.gmane.org/b98f675750ef0535cab41225240db1657fc2fe00.1425678142.git.mjt@msgid.tls.msk.ru

No, this one (simplify v9fs_reqeust(), P1) was a first version.
The above URL points to the v2, a second version.

> I am slightly confused with the patch series. It does split the patch as
> I wanted, but i am not sure which one is the latest, so that i can start
> applying them.

That URL points to last.  I merged several small patches into larger ones.

Initially I thought I just plug a small resource leak so the patch will
be small.  But the more I understood the code, the bigger the changes
were becoming.  So at some point it become unreasonable to keep small
changes since they're related to bigger changes and since bigger changes
touches the same code anyway.

Thanks,

/mjt

Re: [Qemu-devel] [PATCH 1/5] 9pfs-proxy: simplify v9fs_request(), P1

[Michael Tokarev]

10.03.2015 07:19, Michael Tokarev wrote:
> 08.03.2015 19:39, Aneesh Kumar K.V wrote:
>> Michael Tokarev <mjt@tls.msk.ru> writes:
>>
>>> This simplifies code in v9fs_request() a bit by replacing several
>>> ifs with a common variable check and rearranging error/cleanup
>>> code a bit.
>>
>> Is this -V2 of
>> http://mid.gmane.org/b98f675750ef0535cab41225240db1657fc2fe00.1425678142.git.mjt@msgid.tls.msk.ru
> 
> No, this one (simplify v9fs_reqeust(), P1) was a first version.
> The above URL points to the v2, a second version.

Actually the url points to v3, not v2, of the patchset.  Yet it
is still the latest ;)

Thanks,

/mjt

Re: [Qemu-devel] [PATCH 1/5] 9pfs-proxy: simplify v9fs_request(), P1

[Aneesh Kumar K.V]

Michael Tokarev <mjt@tls.msk.ru> writes:

> 08.03.2015 19:39, Aneesh Kumar K.V wrote:
>> Michael Tokarev <mjt@tls.msk.ru> writes:
>> 
>>> This simplifies code in v9fs_request() a bit by replacing several
>>> ifs with a common variable check and rearranging error/cleanup
>>> code a bit.
>> 
>> Is this -V2 of
>> http://mid.gmane.org/b98f675750ef0535cab41225240db1657fc2fe00.1425678142.git.mjt@msgid.tls.msk.ru
>
> No, this one (simplify v9fs_reqeust(), P1) was a first version.
> The above URL points to the v2, a second version.

Please post an update patch series with the variable rename and the v9fs
simplification as two different patches.


>
>> I am slightly confused with the patch series. It does split the patch as
>> I wanted, but i am not sure which one is the latest, so that i can start
>> applying them.
>
> That URL points to last.  I merged several small patches into larger ones.
>
> Initially I thought I just plug a small resource leak so the patch will
> be small.  But the more I understood the code, the bigger the changes
> were becoming.  So at some point it become unreasonable to keep small
> changes since they're related to bigger changes and since bigger changes
> touches the same code anyway.
>

Please keep the patches smaller. If needed i can fold them up when
merging the changes.

-aneesh