[Qemu-devel] [PATCH v3 0/3] RFC: 9pfs-proxy: simplify/cleanup

[Qemu-devel] [PATCH v3 0/3] RFC: 9pfs-proxy: simplify/cleanup

[Michael Tokarev]

Try to make the code a bit less ugly.  First by moving
errno = -retval to a common place, and second by simplifying
common place a lot.  What's left is v9fs_receive_response().

V3: merge several small patches into larger ones,
    drop trivial stuff

Michael Tokarev (3):
  9pfs-proxy: simplify error handling
  fsdev: introduce v9fs_vmarshal() and v9fs_vunmarshal()
  9pfs-proxy: remove one half of redundrand code

 fsdev/virtio-9p-marshal.c |  38 +++--
 fsdev/virtio-9p-marshal.h |   6 +
 hw/9pfs/virtio-9p-proxy.c | 405 +++++-----------------------------------------
 3 files changed, 77 insertions(+), 372 deletions(-)

-- 
2.1.4

[Qemu-devel] [PATCH 1/3] 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(), and renames local variables in some functions
for consistency.

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

diff --git a/hw/9pfs/virtio-9p-proxy.c b/hw/9pfs/virtio-9p-proxy.c
index 0904130..13064b6 100644
--- a/hw/9pfs/virtio-9p-proxy.c
+++ b/hw/9pfs/virtio-9p-proxy.c
@@ -291,7 +291,7 @@ static int v9fs_receive_status(V9fsProxy *proxy,
 /*
  * Proxy->header and proxy->request written to socket by QEMU process.
  * This request read by proxy helper process
- * returns 0 on success and -errno on error
+ * returns 0 on success and -1 (setting errno) on error
  */
 static int v9fs_request(V9fsProxy *proxy, int type,
                         void *response, const char *fmt, ...)
@@ -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, 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,51 +563,44 @@ 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:
+    if (retval < 0) {
+        errno = -retval;
+        retval = -1;
+    }
     qemu_mutex_unlock(&proxy->mutex);
-    return -EIO;
+    return retval;
 }
 
 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;
 }
 
@@ -619,7 +611,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);
@@ -639,10 +630,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;
 }
 
@@ -654,7 +641,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);
@@ -738,9 +724,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;
 }
 
@@ -757,10 +740,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;
 }
 
@@ -776,11 +755,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;
 }
 
@@ -809,10 +783,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;
 }
 
@@ -832,10 +802,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;
 }
 
@@ -850,20 +816,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;
@@ -884,9 +844,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;
 }
 
@@ -895,9 +852,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;
 }
 
@@ -909,9 +863,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;
 }
 
@@ -923,9 +874,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;
 }
 
@@ -951,10 +899,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;
 }
 
@@ -969,9 +913,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;
 }
 
@@ -980,10 +921,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;
 }
 
@@ -1005,9 +943,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;
 }
 
@@ -1022,9 +957,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;
 }
 
@@ -1046,7 +978,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);
@@ -1055,30 +987,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
@@ -1087,12 +1019,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);
-    if (err < 0) {
-        errno = -err;
-        err = -1;
-    }
-    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

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 387 bytes --]

On 03/06/2015 02:43 PM, Michael Tokarev wrote:
> 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.

s/redundand/redundant/

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Michael Tokarev]

07.03.2015 23:37, Eric Blake wrote:
> On 03/06/2015 02:43 PM, Michael Tokarev wrote:
>> 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.
> 
> s/redundand/redundant/

Heh.  Is this all that I can say about this patch? ;)

Actually, after reading almost whole 9pfs and fsdev code, I can
say with great confidence this code is nearly hopeless.
Patch 3 shows just one (huge) example.  There are so many issues
with this code, I'm afraid I don't have know the words to express
it.

Again, patch 3 shows a good example.  Another example:

static int v9fs_receive_status(V9fsProxy *proxy,
                               struct iovec *reply, int *status)
...
    proxy_unmarshal(reply, 0, "dd", &header.type, &header.size);
    if (header.size != sizeof(int)) {
        *status = -ENOBUFS;
    }
...
    proxy_unmarshal(reply, PROXY_HDR_SZ, "d", status);

proxy_unmarshall(), for "d" element, expects an int32_t
pointer.  Here we have int pointer, and compare its
size with sizeof(int).  This is a generic problem of whole
v9fs_(un)marshall interface, which is in the core of 9pfs...
this is a status return, which is int32.

Oh well.  I've no idea how this code has been accepted.
It is absolute crap.

/mjt

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

[Michael Tokarev]

08.03.2015 01:10, Michael Tokarev wrote:
[]
> Heh.  Is this all that I can say about this patch? ;)

s/I/you/ ofcourse ;)

/mjt

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

[Aneesh Kumar K.V]

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

> 07.03.2015 23:37, Eric Blake wrote:
>> On 03/06/2015 02:43 PM, Michael Tokarev wrote:
>>> 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.
>> 
>> s/redundand/redundant/
>
> Heh.  Is this all that I can say about this patch? ;)
>
> Actually, after reading almost whole 9pfs and fsdev code, I can
> say with great confidence this code is nearly hopeless.

Is that about the 9pfs-proxy code, or the rest of 9pfs. I understand
that the error handling can definitely get some cleanup. I mentioned
that in my previous mail
mail. http://mid.gmane.org/87oav7iy5v.fsf@linux.vnet.ibm.com


> Patch 3 shows just one (huge) example.  There are so many issues
> with this code, I'm afraid I don't have know the words to express
> it.
>
> Again, patch 3 shows a good example.  Another example:
>
> static int v9fs_receive_status(V9fsProxy *proxy,
>                                struct iovec *reply, int *status)
> ...
>     proxy_unmarshal(reply, 0, "dd", &header.type, &header.size);
>     if (header.size != sizeof(int)) {
>         *status = -ENOBUFS;
>     }
> ...
>     proxy_unmarshal(reply, PROXY_HDR_SZ, "d", status);
>
> proxy_unmarshall(), for "d" element, expects an int32_t
> pointer.  Here we have int pointer, and compare its
> size with sizeof(int).  This is a generic problem of whole
> v9fs_(un)marshall interface, which is in the core of 9pfs...
> this is a status return, which is int32.
>
> Oh well.  I've no idea how this code has been accepted.
> It is absolute crap.
>

-aneesh

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

[Michael Tokarev]

08.03.2015 19:27, Aneesh Kumar K.V wrote:
> Michael Tokarev <mjt@tls.msk.ru> writes:
[]
>> Actually, after reading almost whole 9pfs and fsdev code, I can
>> say with great confidence this code is nearly hopeless.
> 
> Is that about the 9pfs-proxy code, or the rest of 9pfs. I understand

Well. the marshal/unmarshal interface is in core code as far as
I can see, and it is very fragile at best, as the below example of
its usage shows.  I haven't dug deeper.  So far, it was only the
9pfs proxy code.

> that the error handling can definitely get some cleanup. I mentioned
> that in my previous mail
> mail. http://mid.gmane.org/87oav7iy5v.fsf@linux.vnet.ibm.com

I've shown probs in the code itself, not the visible behavour.
Visible behavour is much easier to fix here.

Thanks,

/mjt

>> Patch 3 shows just one (huge) example.  There are so many issues
>> with this code, I'm afraid I don't have know the words to express
>> it.
>>
>> Again, patch 3 shows a good example.  Another example:
>>
>> static int v9fs_receive_status(V9fsProxy *proxy,
>>                                struct iovec *reply, int *status)
>> ...
>>     proxy_unmarshal(reply, 0, "dd", &header.type, &header.size);
>>     if (header.size != sizeof(int)) {
>>         *status = -ENOBUFS;
>>     }
>> ...
>>     proxy_unmarshal(reply, PROXY_HDR_SZ, "d", status);
>>
>> proxy_unmarshall(), for "d" element, expects an int32_t
>> pointer.  Here we have int pointer, and compare its
>> size with sizeof(int).  This is a generic problem of whole
>> v9fs_(un)marshall interface, which is in the core of 9pfs...
>> this is a status return, which is int32.
>>
>> Oh well.  I've no idea how this code has been accepted.
>> It is absolute crap.
>>
> 
> -aneesh
> 

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

[Aneesh Kumar K.V]

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

> 08.03.2015 19:27, Aneesh Kumar K.V wrote:
>> Michael Tokarev <mjt@tls.msk.ru> writes:
> []
>>> Actually, after reading almost whole 9pfs and fsdev code, I can
>>> say with great confidence this code is nearly hopeless.
>> 
>> Is that about the 9pfs-proxy code, or the rest of 9pfs. I understand
>
> Well. the marshal/unmarshal interface is in core code as far as
> I can see, and it is very fragile at best, as the below example of
> its usage shows.  I haven't dug deeper.  So far, it was only the
> 9pfs proxy code.

May be i am missing something here. Can you help me understand the
issue. 

> static int v9fs_receive_status(V9fsProxy *proxy,
>                                struct iovec *reply, int *status)
> ...
>     proxy_unmarshal(reply, 0, "dd", &header.type, &header.size);
>     if (header.size != sizeof(int)) {
>         *status = -ENOBUFS;
>     }
> ...
>     proxy_unmarshal(reply, PROXY_HDR_SZ, "d", status);
>
> proxy_unmarshall(), for "d" element, expects an int32_t
> pointer.  Here we have int pointer, and compare its
> size with sizeof(int).  This is a generic problem of whole
> v9fs_(un)marshall interface, which is in the core of 9pfs...
> this is a status return, which is int32.

Proxy helper do write sizeof(int) as a part of header response. So
it read the header.size and check whether it is same as what it is
expecting. If not it error out. So i am not sure what the issue you are
listing here.

-aneesh

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

[Michael Tokarev]

10.03.2015 20:41, Aneesh Kumar K.V пишет:
> Michael Tokarev <mjt@tls.msk.ru> writes:
> 
>> 08.03.2015 19:27, Aneesh Kumar K.V wrote:
>>> Michael Tokarev <mjt@tls.msk.ru> writes:
>> []
>>>> Actually, after reading almost whole 9pfs and fsdev code, I can
>>>> say with great confidence this code is nearly hopeless.
>>>
>>> Is that about the 9pfs-proxy code, or the rest of 9pfs. I understand
>>
>> Well. the marshal/unmarshal interface is in core code as far as
>> I can see, and it is very fragile at best, as the below example of
>> its usage shows.  I haven't dug deeper.  So far, it was only the
>> 9pfs proxy code.
> 
> May be i am missing something here. Can you help me understand the
> issue. 
> 
>> static int v9fs_receive_status(V9fsProxy *proxy,
>>                                struct iovec *reply, int *status)
>> ...
>>     proxy_unmarshal(reply, 0, "dd", &header.type, &header.size);
>>     if (header.size != sizeof(int)) {
>>         *status = -ENOBUFS;
>>     }
>> ...
>>     proxy_unmarshal(reply, PROXY_HDR_SZ, "d", status);
>>
>> proxy_unmarshall(), for "d" element, expects an int32_t
>> pointer.  Here we have int pointer, and compare its
>> size with sizeof(int).  This is a generic problem of whole
>> v9fs_(un)marshall interface, which is in the core of 9pfs...
>> this is a status return, which is int32.
> 
> Proxy helper do write sizeof(int) as a part of header response. So
> it read the header.size and check whether it is same as what it is
> expecting. If not it error out. So i am not sure what the issue you are
> listing here.

Nope.  Both proxy helper and qemu uses v9fs_marshal/unmarshal interface
which writes/reads a 4-byte uint32, which is the same size no matter
which compiler/architecture you're on.  Not only these functions uses
this size "on wire", they also expect the same type of argument to the
function.

However, as shown in the above example, at least some users of this
interface uses int* instead of int32_t*, and sizeof(int) is compiler-
and architecture-dependent, it is not fixed to 4 bytes (or else we'd
not need a int32_t to start with, and life would be much simpler).

The rule is that if you exchange data with some other process, unless
it is a forked version of your own process, you should use some fixed
interface, which is not dependent on some conditions.

This is not some made-up situation really, even in this context: for
example, in Debian we allow to install packages of different architectures
on the same machine, and they work together.  We'we qemu-system-common
package which contains the proxy helper, and eg qemu-system-x86 or
qemu-system-arm, and it is perfectly valid to have 64bit qemu-system-common
working together with 32bit qemu-system-mips, so that 32bit qemu binary
will talk with 64bit proxy.

Well, you can treat the proxy helper to be internal part of qemu which
can't be intermixed between versions and architectures -- after all,
little-endian-arch qemu can't talk to big-endian proxy since there's
no byte swapping is going on -- it's a good idea to mention this in
the package. ;)

But your v9fs_marshal interface supposed to be generic and should be
used right to start with.  All args of v9fs_(un)marshall should carry
the same types of arguments these functions expect, which are
int32_t not int, int64_t not long or long long or timespec_t or any
other special type and so on.  Or else BadThings will happen, and
often you wont even notice.

(Note: I haven't audited this interface usage in 9pfs-local.)

But the most problematic part here is that you don't see if you made
a mistake and used different argument type, or missed an argument,
to one of these 2 functions.  There's no argument checking in this
interface, whatsoever.  There's a reason why we have __attribute__(format)
gcc extension to check printf arguments.

That's 2 reasons why I proposed to make these marshal/unmarshal
function to deal with one argument at a time, but with an explicitly
typed argument.  It all will be transparent and obvious and compiler
will tell you the mistakes.

Thanks,

/mjt

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

[Aneesh Kumar K.V]

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

> 10.03.2015 20:41, Aneesh Kumar K.V пишет:
>> Michael Tokarev <mjt@tls.msk.ru> writes:
>> 
>>> 08.03.2015 19:27, Aneesh Kumar K.V wrote:
>>>> Michael Tokarev <mjt@tls.msk.ru> writes:
>>> []
>>>>> Actually, after reading almost whole 9pfs and fsdev code, I can
>>>>> say with great confidence this code is nearly hopeless.
>>>>
>>>> Is that about the 9pfs-proxy code, or the rest of 9pfs. I understand
>>>
>>> Well. the marshal/unmarshal interface is in core code as far as
>>> I can see, and it is very fragile at best, as the below example of
>>> its usage shows.  I haven't dug deeper.  So far, it was only the
>>> 9pfs proxy code.
>> 
>> May be i am missing something here. Can you help me understand the
>> issue. 
>> 
>>> static int v9fs_receive_status(V9fsProxy *proxy,
>>>                                struct iovec *reply, int *status)
>>> ...
>>>     proxy_unmarshal(reply, 0, "dd", &header.type, &header.size);
>>>     if (header.size != sizeof(int)) {
>>>         *status = -ENOBUFS;
>>>     }
>>> ...
>>>     proxy_unmarshal(reply, PROXY_HDR_SZ, "d", status);
>>>
>>> proxy_unmarshall(), for "d" element, expects an int32_t
>>> pointer.  Here we have int pointer, and compare its
>>> size with sizeof(int).  This is a generic problem of whole
>>> v9fs_(un)marshall interface, which is in the core of 9pfs...
>>> this is a status return, which is int32.
>> 
>> Proxy helper do write sizeof(int) as a part of header response. So
>> it read the header.size and check whether it is same as what it is
>> expecting. If not it error out. So i am not sure what the issue you are
>> listing here.
>
> Nope.  Both proxy helper and qemu uses v9fs_marshal/unmarshal interface
> which writes/reads a 4-byte uint32, which is the same size no matter
> which compiler/architecture you're on.  Not only these functions uses
> this size "on wire", they also expect the same type of argument to the
> function.

That is better. So what you are looking at is only the proxy marshal and
unmarshal code. I was wondering what issues you were pointing out by

   " This is a generic problem of whole
   v9fs_(un)marshall interface, which is in the core of 9pfs...
   this is a status return, which is int32."

>
> However, as shown in the above example, at least some users of this
> interface uses int* instead of int32_t*, and sizeof(int) is compiler-
> and architecture-dependent, it is not fixed to 4 bytes (or else we'd
> not need a int32_t to start with, and life would be much simpler).
>
> The rule is that if you exchange data with some other process, unless
> it is a forked version of your own process, you should use some fixed
> interface, which is not dependent on some conditions.
>
> This is not some made-up situation really, even in this context: for
> example, in Debian we allow to install packages of different architectures
> on the same machine, and they work together.  We'we qemu-system-common
> package which contains the proxy helper, and eg qemu-system-x86 or
> qemu-system-arm, and it is perfectly valid to have 64bit qemu-system-common
> working together with 32bit qemu-system-mips, so that 32bit qemu binary
> will talk with 64bit proxy.

The code was originally written to be used on same os/cpu combination to
help virtfs usage via libvirt. AFAIU here we don't have issues
across 32 and 64 bit, because int remains 32 bit in both the
place. Which os/cpu combination are we looking w.r.t ILP64 ?. Please
note that the code is written primary on x86_64. So some assumptions
could be wrong. Rather than saying

   "Oh well.  I've no idea how this code has been accepted.
   It is absolute crap."

it would be nice if we take up specific issues, w.r.t the os/cpu
combination where we are running into issues, we may be able to fix this
easily.


>
> Well, you can treat the proxy helper to be internal part of qemu which
> can't be intermixed between versions and architectures -- after all,
> little-endian-arch qemu can't talk to big-endian proxy since there's
> no byte swapping is going on -- it's a good idea to mention this in
> the package. ;)

That is correct. It was not written to be used in such scenarios

>
> But your v9fs_marshal interface supposed to be generic and should be
> used right to start with.  All args of v9fs_(un)marshall should carry
> the same types of arguments these functions expect, which are
> int32_t not int, int64_t not long or long long or timespec_t or any
> other special type and so on.  Or else BadThings will happen, and
> often you wont even notice.

Note: At this point i haven't picked any of the patches posted, so if
you have anything specific you want me to take up, please repost then
with correct version number (v1, v2...)

-aneesh

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

[Michael Tokarev]

11.03.2015 11:46, Aneesh Kumar K.V wrote:
[]
> ... Rather than saying
> 
>    "Oh well.  I've no idea how this code has been accepted.
>    It is absolute crap."
> 
> it would be nice if we take up specific issues, w.r.t the os/cpu
> combination where we are running into issues, we may be able to fix this
> easily.

The specific issues were outlined and partially fixed by the v3
patchset.  This is, for example, the way v9fs_request() is called
with arguments for v9fs_marshal function, which are ignored and
new arguments are constructed inside _request.  That's what I
refer to when saying it is a complete crap.  Do you not agree? :)

[]
> Note: At this point i haven't picked any of the patches posted, so if
> you have anything specific you want me to take up, please repost then
> with correct version number (v1, v2...)

I sent a v3 (last) with correct version number, it has just 3 (rather
big) patches: http://thread.gmane.org/gmane.comp.emulators.qemu/324157 .

Thanks,

/mjt

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

[Michael Tokarev]

11.03.2015 23:05, Michael Tokarev wrote:
> I sent a v3 (last) with correct version number, it has just 3 (rather
> big) patches: http://thread.gmane.org/gmane.comp.emulators.qemu/324157 .

And this is what I think should be done (part of that thread), at least
for the proxy code:
http://article.gmane.org/gmane.comp.emulators.qemu/324585

Thanks,

/mjt

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

[Aneesh Kumar K.V]

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

> 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(), and renames local variables in some functions
> for consistency.

Can you keep the variable rename as a separate patch. That will make it
easier to review. Let me know if you want me to do that for you.

-aneesh

[Qemu-devel] [PATCH 2/3] fsdev: introduce v9fs_vmarshal() and v9fs_vunmarshal()

[Michael Tokarev]

This splits existing functions which expects any argument
into pairs, second being one which accepts va_list, to
be used later.

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 fsdev/virtio-9p-marshal.c | 38 ++++++++++++++++++++++++++++----------
 fsdev/virtio-9p-marshal.h |  6 ++++++
 2 files changed, 34 insertions(+), 10 deletions(-)

diff --git a/fsdev/virtio-9p-marshal.c b/fsdev/virtio-9p-marshal.c
index 20f308b..757a79a 100644
--- a/fsdev/virtio-9p-marshal.c
+++ b/fsdev/virtio-9p-marshal.c
@@ -108,15 +108,13 @@ ssize_t v9fs_pack(struct iovec *in_sg, int in_num, size_t offset,
     return v9fs_packunpack((void *)src, in_sg, in_num, offset, size, 1);
 }
 
-ssize_t v9fs_unmarshal(struct iovec *out_sg, int out_num, size_t offset,
-                       int bswap, const char *fmt, ...)
+ssize_t v9fs_vunmarshal(struct iovec *out_sg, int out_num, size_t offset,
+                        int bswap, const char *fmt, va_list ap)
 {
     int i;
-    va_list ap;
     ssize_t copied = 0;
     size_t old_offset = offset;
 
-    va_start(ap, fmt);
     for (i = 0; fmt[i]; i++) {
         switch (fmt[i]) {
         case 'b': {
@@ -212,20 +210,28 @@ ssize_t v9fs_unmarshal(struct iovec *out_sg, int out_num, size_t offset,
         }
         offset += copied;
     }
-    va_end(ap);
 
     return offset - old_offset;
 }
 
-ssize_t v9fs_marshal(struct iovec *in_sg, int in_num, size_t offset,
-                     int bswap, const char *fmt, ...)
+ssize_t v9fs_unmarshal(struct iovec *in_sg, int in_num, size_t offset,
+                      int bswap, const char *fmt, ...)
 {
-    int i;
+    ssize_t ret;
     va_list ap;
+    va_start(ap, fmt);
+    ret = v9fs_vunmarshal(in_sg, in_num, offset, bswap, fmt, ap);
+    va_end(ap);
+    return ret;
+}
+
+ssize_t v9fs_vmarshal(struct iovec *in_sg, int in_num, size_t offset,
+                     int bswap, const char *fmt, va_list ap)
+{
+    int i;
     ssize_t copied = 0;
     size_t old_offset = offset;
 
-    va_start(ap, fmt);
     for (i = 0; fmt[i]; i++) {
         switch (fmt[i]) {
         case 'b': {
@@ -317,7 +323,19 @@ ssize_t v9fs_marshal(struct iovec *in_sg, int in_num, size_t offset,
         }
         offset += copied;
     }
-    va_end(ap);
 
     return offset - old_offset;
 }
+
+ssize_t v9fs_marshal(struct iovec *in_sg, int in_num, size_t offset,
+                    int bswap, const char *fmt, ...)
+{
+    ssize_t ret;
+    va_list ap;
+    va_start(ap, fmt);
+    ret = v9fs_vmarshal(in_sg, in_num, offset, bswap, fmt, ap);
+    va_end(ap);
+    return ret;
+}
+
+
diff --git a/fsdev/virtio-9p-marshal.h b/fsdev/virtio-9p-marshal.h
index 5df65a8..90d48ca 100644
--- a/fsdev/virtio-9p-marshal.h
+++ b/fsdev/virtio-9p-marshal.h
@@ -1,6 +1,8 @@
 #ifndef _QEMU_VIRTIO_9P_MARSHAL_H
 #define _QEMU_VIRTIO_9P_MARSHAL_H
 
+#include <stdarg.h>
+
 typedef struct V9fsString
 {
     uint16_t size;
@@ -85,6 +87,10 @@ ssize_t v9fs_pack(struct iovec *in_sg, int in_num, size_t offset,
                   const void *src, size_t size);
 ssize_t v9fs_unmarshal(struct iovec *out_sg, int out_num, size_t offset,
                        int bswap, const char *fmt, ...);
+ssize_t v9fs_vunmarshal(struct iovec *out_sg, int out_num, size_t offset,
+                        int bswap, const char *fmt, va_list ap);
 ssize_t v9fs_marshal(struct iovec *in_sg, int in_num, size_t offset,
                      int bswap, const char *fmt, ...);
+ssize_t v9fs_vmarshal(struct iovec *in_sg, int in_num, size_t offset,
+                      int bswap, const char *fmt, va_list ap);
 #endif
-- 
2.1.4

[Qemu-devel] [PATCH 3/3] 9pfs-proxy: remove one half of redundrand code

[Michael Tokarev]

The 9pfs-proxy code is actually terrible: it does the same
things again and again, ignoring function arguments and
re-inventing the same values again, and a lot of code must
be consistent with each other.

In particular, lots of filesystem methods use common v9fs_request()
function and pass all method args together with pack string to it.
However, v9fs_request() just ignores this, pop the values out of
stack and sets pack string once more.  This is sort of absurd.

This patch removes per-request-type argument marshalling from
v9fs_request() and keeps it only in the individual request
handling methods.

It also removes method-switch from response receiving and moves
the response/fd/status receiving decision also to the individual
request handling methods.

What's left is to do something similar with receiving response,
v9fs_receive_response() function.

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

diff --git a/hw/9pfs/virtio-9p-proxy.c b/hw/9pfs/virtio-9p-proxy.c
index 13064b6..3dbd568 100644
--- a/hw/9pfs/virtio-9p-proxy.c
+++ b/hw/9pfs/virtio-9p-proxy.c
@@ -296,17 +296,8 @@ static int v9fs_receive_status(V9fsProxy *proxy,
 static int v9fs_request(V9fsProxy *proxy, int type,
                         void *response, const char *fmt, ...)
 {
-    dev_t rdev;
     va_list ap;
-    int size = 0;
     int retval, err;
-    uint64_t offset;
-    ProxyHeader header = { 0, 0};
-    struct timespec spec[2];
-    int flags, mode, uid, gid;
-    V9fsString *name, *value;
-    V9fsString *path, *oldpath;
-    struct iovec *iovec = NULL, *reply = NULL;
 
     qemu_mutex_lock(&proxy->mutex);
 
@@ -315,218 +306,8 @@ static int v9fs_request(V9fsProxy *proxy, int type,
         goto out;
     }
 
-    iovec = &proxy->out_iovec;
-    reply = &proxy->in_iovec;
     va_start(ap, fmt);
-    switch (type) {
-    case T_OPEN:
-        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 *);
-        flags = va_arg(ap, int);
-        mode = va_arg(ap, int);
-        uid = va_arg(ap, int);
-        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 *);
-        mode = va_arg(ap, int);
-        rdev = va_arg(ap, long int);
-        uid = va_arg(ap, int);
-        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 *);
-        mode = va_arg(ap, int);
-        uid = va_arg(ap, int);
-        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 *);
-        path = va_arg(ap, V9fsString *);
-        uid = va_arg(ap, int);
-        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 *);
-        spec[0].tv_sec = va_arg(ap, long);
-        spec[0].tv_nsec = va_arg(ap, long);
-        spec[1].tv_sec = va_arg(ap, long);
-        spec[1].tv_nsec = va_arg(ap, long);
-        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);
-        path = va_arg(ap, V9fsString *);
-        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 *);
-        name = va_arg(ap, V9fsString *);
-        value = va_arg(ap, V9fsString *);
-        size = va_arg(ap, int);
-        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);
-        retval = -EINVAL;
-        break;
-    }
+    retval = v9fs_marshal(&proxy->out_iovec, 1, PROXY_HDR_SZ, 0, fmt, ap);
     va_end(ap);
 
     if (retval < 0) {
@@ -534,51 +315,23 @@ static int v9fs_request(V9fsProxy *proxy, int type,
     }
 
     /* marshal the header details */
-    proxy_marshal(iovec, 0, "dd", header.type, header.size);
-    header.size += PROXY_HDR_SZ;
+    v9fs_marshal(&proxy->out_iovec, 1, 0, 0, "dd", type, retval);
+    retval += PROXY_HDR_SZ;
 
-    err = qemu_write_full(proxy->sockfd, iovec->iov_base, header.size);
-    if (err != header.size) {
+    err = qemu_write_full(proxy->sockfd, proxy->out_iovec.iov_base, retval);
+    if (err != retval) {
         goto close_error;
     }
 
-    switch (type) {
-    case T_OPEN:
-    case T_CREATE:
-        /*
-         * A file descriptor is returned as response for
+    if (type == T_OPEN || type == T_CREATE) {
+        /* A file descriptor is returned as response for
          * T_OPEN,T_CREATE on success
          */
         err = v9fs_receivefd(proxy->sockfd, &retval);
-        break;
-    case T_MKNOD:
-    case T_MKDIR:
-    case T_SYMLINK:
-    case T_LINK:
-    case T_CHMOD:
-    case T_CHOWN:
-    case T_RENAME:
-    case T_TRUNCATE:
-    case T_UTIME:
-    case T_REMOVE:
-    case T_LSETXATTR:
-    case T_LREMOVEXATTR:
-        err = v9fs_receive_status(proxy, reply, &retval);
-        break;
-    case T_LSTAT:
-    case T_READLINK:
-    case T_STATFS:
-    case T_GETVERSION:
+    } else if (response) {
         err = v9fs_receive_response(proxy, type, &retval, response);
-        break;
-    case T_LGETXATTR:
-    case T_LLISTXATTR:
-        if (!size) {
-            err = v9fs_receive_status(proxy, reply, &retval);
-        } else {
-            err = v9fs_receive_response(proxy, type, &retval, response);
-        }
-        break;
+    } else {
+        err = v9fs_receive_status(proxy, &proxy->in_iovec, &retval);
     }
 
     if (err < 0) {
@@ -910,8 +663,8 @@ static ssize_t proxy_lgetxattr(FsContext *ctx, V9fsPath *fs_path,
 
     v9fs_string_init(&xname);
     v9fs_string_sprintf(&xname, "%s", name);
-    retval = v9fs_request(ctx->private, T_LGETXATTR, value, "dss", size,
-                          fs_path, &xname);
+    retval = v9fs_request(ctx->private, T_LGETXATTR, size ? value : NULL,
+                         "dss", size, fs_path, &xname);
     v9fs_string_free(&xname);
     return retval;
 }
@@ -920,8 +673,8 @@ static ssize_t proxy_llistxattr(FsContext *ctx, V9fsPath *fs_path,
                                 void *value, size_t size)
 {
     int retval;
-    retval = v9fs_request(ctx->private, T_LLISTXATTR, value, "ds", size,
-                          fs_path);
+    retval = v9fs_request(ctx->private, T_LLISTXATTR, size ? value : NULL,
+                          "ds", size, fs_path);
     return retval;
 }
 
-- 
2.1.4

Re: [Qemu-devel] [PATCH 3/3] 9pfs-proxy: remove one half of redundrand code

[Michael Tokarev]

07.03.2015 00:43, Michael Tokarev пишет:

> +    retval = v9fs_marshal(&proxy->out_iovec, 1, PROXY_HDR_SZ, 0, fmt, ap);

This should be the new v9fs_vmarshal() ofcourse.

/mjt

Re: [Qemu-devel] [PATCH 3/3] 9pfs-proxy: remove one half of redundrand code

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 487 bytes --]

On 03/06/2015 02:43 PM, Michael Tokarev wrote:
> The 9pfs-proxy code is actually terrible: it does the same
> things again and again, ignoring function arguments and
> re-inventing the same values again, and a lot of code must
> be consistent with each other.

s/redundrand/redundant/ in the subject (a different typo than in 1/3, so
my reply isn't quite redundant :)

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 0/3] RFC: 9pfs-proxy: simplify/cleanup

[Michael Tokarev]

Now when I reviewed the whole thing, I'd drop the error
handling change too, and fold it into even bigger change.
What I'm thinking is to refactor this code to look
significantly different, namely:

For the proxy code:

 o each filesystem method first call proxy_send_request(type, fmt, ...),
   and proxy_send_request() locks the proxy object, marshals the
   arguments according to fmt and sends the request out.

 o after sending the request, each method calls
   proxy_receive_reply(fmt, ...) (or proxy_receive_fd() -- there's
   no need in proxy_receive_status(), it is proxy_recive_reply()
   with fmt=NULL).  This proxy_receive_reply() receives the
   reply according to the fmt argument and unlocks the
   proxy object.

This way. locking code will be split into two methods, but
_all_ filesystem-method-specific code, for each method,
will be in the same function which is the only place which
"knows" everything about the method.

For proxy code it might also be a good idea to have two
v9fs_string objects embedded in the proxy structure, to
be used by the methods, so there's no need to init and
free the local-to-function strings in every method.

At the general level, v9fs_string handling is bad, it
shouldn't really free+alloc a string every time something
gets printed into it.  v9fs_path type should be dropped
completely and replaced with v9fs_string, especially since
one is often being type-cast to another.

marshal/unmarshal interface should be printf-like with %s,
so that the compiler will be able to check if the arguments
are of the right type.  Alternatively, it should be re-
factored to accept just one, typed, argument to pack/unpack,
without vararg part.  Here, an iovec iterator might be used
(to keep current position in iovec) to speed things up.

marshal/unmarshal interface should not allocate/free strings
frantically like it does now -- this allocation/freeing takes
significant time and slows down 9pfs code.  The same is true
for some other parts of 9pfs too.

Also marshal/unmarshal interface - is it really necessary to
support I/O where individual eiements (a file name, size of
that file name, or even all the fields of some guest-visible
structure) are split between several iovec elements?  Can't
we say that a single element (with some definition of "element",
which can be a single string or this string together with its
size in front, one element of a structure like stat or whole
stat structure, etc) must not be split into multiple iovecs?
If it's possible, this code can be simplified and sped up
greatly, for example, we won't need to copy the strings to
a temp buffer (especially multiple times!) and can just point
inside of a single iovec...

That's just some random thoughts.  All without acually understanding
how 9pfs communicates with the guest...  Can you describe this part
briefly please?  Or maybe I should just shut up and pretend I never
seen this code.. ;)  (which is, actually, a good possibility - I don't
really have much time to deal with it, and 9pfs is not my big interest... ;)

Thanks,

/mjt

07.03.2015 00:43, Michael Tokarev wrote:
> Try to make the code a bit less ugly.  First by moving
> errno = -retval to a common place, and second by simplifying
> common place a lot.  What's left is v9fs_receive_response().
> 
> V3: merge several small patches into larger ones,
>     drop trivial stuff
> 
> Michael Tokarev (3):
>   9pfs-proxy: simplify error handling
>   fsdev: introduce v9fs_vmarshal() and v9fs_vunmarshal()
>   9pfs-proxy: remove one half of redundrand code
> 
>  fsdev/virtio-9p-marshal.c |  38 +++--
>  fsdev/virtio-9p-marshal.h |   6 +
>  hw/9pfs/virtio-9p-proxy.c | 405 +++++-----------------------------------------
>  3 files changed, 77 insertions(+), 372 deletions(-)
>