[Qemu-devel] [PATCH v3 0/3] 9pfs: disentangling virtio and generic code

[Qemu-devel] [PATCH v3 0/3] 9pfs: disentangling virtio and generic code

[Wei Liu]

Hi all

This is version 3 of this series. It is based on

  https://github.com/kvaneesh/qemu/commits/upstream-v9fs

so it only contains patches that are not in that branch.

These three patches were tested with Turex POSIX test suite 20080816 and
20090130-rc. It passed all tests in 20080816 and got expected xacl test
failures in 20090130-rc.

I will spare copying and pasting the same content here again.  Previous cover
letters can be found at:

v2: https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg00822.html
v1: https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg00374.html

Wei.
---
Cc: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
Cc: Greg Kurz <gkurz@linux.vnet.ibm.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---

Wei Liu (3):
  fsdev: 9p-marshal: introduce V9fsBlob
  9pfs: use V9fsBlob to transmit xattr
  9pfs: introduce V9fsVirtioState

 fsdev/9p-iov-marshal.c     | 30 ++++++++++++++++--
 fsdev/9p-iov-marshal.h     |  3 --
 fsdev/9p-marshal.c         |  7 +++++
 fsdev/9p-marshal.h         | 14 +++++++++
 hw/9pfs/9p.c               | 27 ++++++++++------
 hw/9pfs/9p.h               |  6 +---
 hw/9pfs/virtio-9p-device.c | 78 +++++++++++++++++++++++++++++-----------------
 hw/9pfs/virtio-9p.h        | 12 ++++++-
 hw/s390x/virtio-ccw.h      |  2 +-
 hw/virtio/virtio-pci.h     |  2 +-
 10 files changed, 129 insertions(+), 52 deletions(-)

-- 
2.1.4

[Qemu-devel] [PATCH v3 1/3] fsdev: 9p-marshal: introduce V9fsBlob

[Wei Liu]

Introduce a concept of blob. It will be used to pack / unpack xattr
value.

With this change there is no need to expose v9fs_pack to device code
anymore.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
v3: use 'd' to encode / decode blob size
---
 fsdev/9p-iov-marshal.c | 26 ++++++++++++++++++++++++++
 fsdev/9p-marshal.c     |  7 +++++++
 fsdev/9p-marshal.h     | 14 ++++++++++++++
 3 files changed, 47 insertions(+)

diff --git a/fsdev/9p-iov-marshal.c b/fsdev/9p-iov-marshal.c
index 08d783c..1f9edf3 100644
--- a/fsdev/9p-iov-marshal.c
+++ b/fsdev/9p-iov-marshal.c
@@ -140,6 +140,21 @@ ssize_t v9fs_iov_vunmarshal(struct iovec *out_sg, int out_num, size_t offset,
             }
             break;
         }
+        case 'B': {
+            V9fsBlob *blob = va_arg(ap, V9fsBlob *);
+            copied = v9fs_iov_unmarshal(out_sg, out_num, offset, bswap,
+                                        "d", &blob->size);
+            if (copied > 0) {
+                offset += copied;
+                blob->data = g_malloc(blob->size);
+                copied = v9fs_unpack(blob->data, out_sg, out_num, offset,
+                                     blob->size);
+                if (copied < 0) {
+                    v9fs_blob_free(blob);
+                }
+            }
+            break;
+        }
         case 'Q': {
             V9fsQID *qidp = va_arg(ap, V9fsQID *);
             copied = v9fs_iov_unmarshal(out_sg, out_num, offset, bswap,
@@ -253,6 +268,17 @@ ssize_t v9fs_iov_vmarshal(struct iovec *in_sg, int in_num, size_t offset,
             }
             break;
         }
+        case 'B': {
+            V9fsBlob *blob = va_arg(ap, V9fsBlob *);
+            copied = v9fs_iov_marshal(in_sg, in_num, offset, bswap,
+                                      "d", blob->size);
+            if (copied > 0) {
+                offset += copied;
+                copied = v9fs_pack(in_sg, in_num, offset, blob->data,
+                                   blob->size);
+            }
+            break;
+        }
         case 'Q': {
             V9fsQID *qidp = va_arg(ap, V9fsQID *);
             copied = v9fs_iov_marshal(in_sg, in_num, offset, bswap, "bdq",
diff --git a/fsdev/9p-marshal.c b/fsdev/9p-marshal.c
index 991e35d..a914244 100644
--- a/fsdev/9p-marshal.c
+++ b/fsdev/9p-marshal.c
@@ -54,3 +54,10 @@ void v9fs_string_copy(V9fsString *lhs, V9fsString *rhs)
     v9fs_string_free(lhs);
     v9fs_string_sprintf(lhs, "%s", rhs->data);
 }
+
+void v9fs_blob_free(V9fsBlob *blob)
+{
+    g_free(blob->data);
+    blob->data = NULL;
+    blob->size = 0;
+}
diff --git a/fsdev/9p-marshal.h b/fsdev/9p-marshal.h
index e91b24e..54148f4 100644
--- a/fsdev/9p-marshal.h
+++ b/fsdev/9p-marshal.h
@@ -7,6 +7,12 @@ typedef struct V9fsString
     char *data;
 } V9fsString;
 
+typedef struct V9fsBlob
+{
+    uint32_t size;
+    void *data;
+} V9fsBlob;
+
 typedef struct V9fsQID
 {
     int8_t type;
@@ -81,4 +87,12 @@ extern void v9fs_string_null(V9fsString *str);
 extern void v9fs_string_sprintf(V9fsString *str, const char *fmt, ...);
 extern void v9fs_string_copy(V9fsString *lhs, V9fsString *rhs);
 
+static inline void v9fs_blob_init(V9fsBlob *blob)
+{
+    blob->data = NULL;
+    blob->size = 0;
+}
+
+extern void v9fs_blob_free(V9fsBlob *blob);
+
 #endif
-- 
2.1.4

[Qemu-devel] [PATCH v3 2/3] 9pfs: use V9fsBlob to transmit xattr

[Wei Liu]

And make v9fs_pack static function. Now we only need to export
v9fs_{,un}marshal to device.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
v3: fix bug discovered by Aneesh
---
 fsdev/9p-iov-marshal.c |  4 ++--
 fsdev/9p-iov-marshal.h |  3 ---
 hw/9pfs/9p.c           | 21 +++++++++++++--------
 3 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/fsdev/9p-iov-marshal.c b/fsdev/9p-iov-marshal.c
index 1f9edf3..5c911c8 100644
--- a/fsdev/9p-iov-marshal.c
+++ b/fsdev/9p-iov-marshal.c
@@ -70,8 +70,8 @@ static ssize_t v9fs_unpack(void *dst, struct iovec *out_sg, int out_num,
     return v9fs_packunpack(dst, out_sg, out_num, offset, size, 0);
 }
 
-ssize_t v9fs_pack(struct iovec *in_sg, int in_num, size_t offset,
-                  const void *src, size_t size)
+static ssize_t v9fs_pack(struct iovec *in_sg, int in_num, size_t offset,
+                         const void *src, size_t size)
 {
     return v9fs_packunpack((void *)src, in_sg, in_num, offset, size, 1);
 }
diff --git a/fsdev/9p-iov-marshal.h b/fsdev/9p-iov-marshal.h
index 6bccbfb..410a1ea 100644
--- a/fsdev/9p-iov-marshal.h
+++ b/fsdev/9p-iov-marshal.h
@@ -3,9 +3,6 @@
 
 #include "9p-marshal.h"
 
-
-ssize_t v9fs_pack(struct iovec *in_sg, int in_num, size_t offset,
-                  const void *src, size_t size);
 ssize_t v9fs_iov_unmarshal(struct iovec *out_sg, int out_num, size_t offset,
                            int bswap, const char *fmt, ...);
 ssize_t v9fs_iov_marshal(struct iovec *in_sg, int in_num, size_t offset,
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index a904403..84cb1d9 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1585,6 +1585,7 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp,
     size_t offset = 7;
     int read_count;
     int64_t xattr_len;
+    V9fsBlob blob;
 
     xattr_len = fidp->fs.xattr.len;
     read_count = xattr_len - off;
@@ -1596,14 +1597,18 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp,
          */
         read_count = 0;
     }
-    err = pdu_marshal(pdu, offset, "d", read_count);
-    if (err < 0) {
-        return err;
-    }
-    offset += err;
-    err = v9fs_pack(pdu->elem.in_sg, pdu->elem.in_num, offset,
-                    ((char *)fidp->fs.xattr.value) + off,
-                    read_count);
+
+    v9fs_blob_init(&blob);
+
+    blob.data = g_malloc(read_count);
+    memcpy(blob.data, ((char *)fidp->fs.xattr.value) + off,
+           read_count);
+    blob.size = read_count;
+
+    err = pdu_marshal(pdu, offset, "B", &blob);
+
+    v9fs_blob_free(&blob);
+
     if (err < 0) {
         return err;
     }
-- 
2.1.4

[Qemu-devel] [PATCH v3 3/3] 9pfs: introduce V9fsVirtioState

[Wei Liu]

V9fsState now only contains generic fields. Introduce V9fsVirtioState
for virtio transport.  Change virtio-pci and virtio-ccw to use
V9fsVirtioState.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
v3: only include code to introduce V9fsVirtioState
---
 hw/9pfs/9p.c               |  6 ++--
 hw/9pfs/9p.h               |  6 +---
 hw/9pfs/virtio-9p-device.c | 78 +++++++++++++++++++++++++++++-----------------
 hw/9pfs/virtio-9p.h        | 12 ++++++-
 hw/s390x/virtio-ccw.h      |  2 +-
 hw/virtio/virtio-pci.h     |  2 +-
 6 files changed, 67 insertions(+), 39 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 84cb1d9..77f95f2 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -3274,6 +3274,7 @@ void pdu_submit(V9fsPDU *pdu)
 /* Returns 0 on success, 1 on failure. */
 int v9fs_device_realize_common(V9fsState *s, Error **errp)
 {
+    V9fsVirtioState *v = container_of(s, V9fsVirtioState, state);
     int i, len;
     struct stat stat;
     FsDriverEntry *fse;
@@ -3284,8 +3285,9 @@ int v9fs_device_realize_common(V9fsState *s, Error **errp)
     QLIST_INIT(&s->free_list);
     QLIST_INIT(&s->active_list);
     for (i = 0; i < (MAX_REQ - 1); i++) {
-        QLIST_INSERT_HEAD(&s->free_list, &s->pdus[i], next);
-        s->pdus[i].s = s;
+        QLIST_INSERT_HEAD(&s->free_list, &v->pdus[i], next);
+        v->pdus[i].s = s;
+        v->pdus[i].idx = i;
     }
 
     v9fs_path_init(&path);
diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index 3fe4da4..edcd51b 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -131,9 +131,9 @@ struct V9fsPDU
     uint8_t id;
     uint8_t cancelled;
     CoQueue complete;
-    VirtQueueElement elem;
     struct V9fsState *s;
     QLIST_ENTRY(V9fsPDU) next;
+    uint32_t idx;
 };
 
 
@@ -205,16 +205,12 @@ struct V9fsFidState
 
 typedef struct V9fsState
 {
-    VirtIODevice parent_obj;
-    VirtQueue *vq;
-    V9fsPDU pdus[MAX_REQ];
     QLIST_HEAD(, V9fsPDU) free_list;
     QLIST_HEAD(, V9fsPDU) active_list;
     V9fsFidState *fid_list;
     FileOperations *ops;
     FsContext ctx;
     char *tag;
-    size_t config_size;
     enum p9_proto_version proto_version;
     int32_t msize;
     /*
diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
index 4aa8a6b..5643fd5 100644
--- a/hw/9pfs/virtio-9p-device.c
+++ b/hw/9pfs/virtio-9p-device.c
@@ -24,33 +24,41 @@
 void virtio_9p_push_and_notify(V9fsPDU *pdu)
 {
     V9fsState *s = pdu->s;
+    V9fsVirtioState *v = container_of(s, V9fsVirtioState, state);
+    VirtQueueElement *elem = &v->elems[pdu->idx];
 
     /* push onto queue and notify */
-    virtqueue_push(s->vq, &pdu->elem, pdu->size);
+    virtqueue_push(v->vq, elem, pdu->size);
 
     /* FIXME: we should batch these completions */
-    virtio_notify(VIRTIO_DEVICE(s), s->vq);
+    virtio_notify(VIRTIO_DEVICE(v), v->vq);
 }
 
 static void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq)
 {
-    V9fsState *s = (V9fsState *)vdev;
+    V9fsVirtioState *v = (V9fsVirtioState *)vdev;
+    V9fsState *s = &v->state;
     V9fsPDU *pdu;
     ssize_t len;
 
-    while ((pdu = pdu_alloc(s)) &&
-            (len = virtqueue_pop(vq, &pdu->elem)) != 0) {
+    while ((pdu = pdu_alloc(s))) {
         struct {
             uint32_t size_le;
             uint8_t id;
             uint16_t tag_le;
         } QEMU_PACKED out;
-        int len;
+        VirtQueueElement *elem = &v->elems[pdu->idx];
 
-        BUG_ON(pdu->elem.out_num == 0 || pdu->elem.in_num == 0);
+        len = virtqueue_pop(vq, elem);
+        if (!len) {
+            pdu_free(pdu);
+            break;
+        }
+
+        BUG_ON(elem->out_num == 0 || elem->in_num == 0);
         QEMU_BUILD_BUG_ON(sizeof out != 7);
 
-        len = iov_to_buf(pdu->elem.out_sg, pdu->elem.out_num, 0,
+        len = iov_to_buf(elem->out_sg, elem->out_num, 0,
                          &out, sizeof out);
         BUG_ON(len != sizeof out);
 
@@ -62,7 +70,6 @@ static void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq)
         qemu_co_queue_init(&pdu->complete);
         pdu_submit(pdu);
     }
-    pdu_free(pdu);
 }
 
 static uint64_t virtio_9p_get_features(VirtIODevice *vdev, uint64_t features,
@@ -76,14 +83,15 @@ static void virtio_9p_get_config(VirtIODevice *vdev, uint8_t *config)
 {
     int len;
     struct virtio_9p_config *cfg;
-    V9fsState *s = VIRTIO_9P(vdev);
+    V9fsVirtioState *v = VIRTIO_9P(vdev);
+    V9fsState *s = &v->state;
 
     len = strlen(s->tag);
     cfg = g_malloc0(sizeof(struct virtio_9p_config) + len);
     virtio_stw_p(vdev, &cfg->tag_len, len);
     /* We don't copy the terminating null to config space */
     memcpy(cfg->tag, s->tag, len);
-    memcpy(config, cfg, s->config_size);
+    memcpy(config, cfg, v->config_size);
     g_free(cfg);
 }
 
@@ -100,16 +108,17 @@ static int virtio_9p_load(QEMUFile *f, void *opaque, int version_id)
 static void virtio_9p_device_realize(DeviceState *dev, Error **errp)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
-    V9fsState *s = VIRTIO_9P(dev);
+    V9fsVirtioState *v = VIRTIO_9P(dev);
+    V9fsState *s = &v->state;
 
     if (v9fs_device_realize_common(s, errp)) {
         goto out;
     }
 
-    s->config_size = sizeof(struct virtio_9p_config) + strlen(s->fsconf.tag);
-    virtio_init(vdev, "virtio-9p", VIRTIO_ID_9P, s->config_size);
-    s->vq = virtio_add_queue(vdev, MAX_REQ, handle_9p_output);
-    register_savevm(dev, "virtio-9p", -1, 1, virtio_9p_save, virtio_9p_load, s);
+    v->config_size = sizeof(struct virtio_9p_config) + strlen(s->fsconf.tag);
+    virtio_init(vdev, "virtio-9p", VIRTIO_ID_9P, v->config_size);
+    v->vq = virtio_add_queue(vdev, MAX_REQ, handle_9p_output);
+    register_savevm(dev, "virtio-9p", -1, 1, virtio_9p_save, virtio_9p_load, v);
 
 out:
     return;
@@ -118,44 +127,55 @@ out:
 static void virtio_9p_device_unrealize(DeviceState *dev, Error **errp)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
-    V9fsState *s = VIRTIO_9P(dev);
+    V9fsVirtioState *v = VIRTIO_9P(dev);
+    V9fsState *s = &v->state;
 
     virtio_cleanup(vdev);
-    unregister_savevm(dev, "virtio-9p", s);
+    unregister_savevm(dev, "virtio-9p", v);
     v9fs_device_unrealize_common(s, errp);
 }
 
 ssize_t virtio_pdu_vmarshal(V9fsPDU *pdu, size_t offset,
                             const char *fmt, va_list ap)
 {
-    return v9fs_iov_vmarshal(pdu->elem.in_sg, pdu->elem.in_num,
-                             offset, 1, fmt, ap);
+    V9fsState *s = pdu->s;
+    V9fsVirtioState *v = container_of(s, V9fsVirtioState, state);
+    VirtQueueElement *elem = &v->elems[pdu->idx];
+
+    return v9fs_iov_vmarshal(elem->in_sg, elem->in_num, offset, 1, fmt, ap);
 }
 
 ssize_t virtio_pdu_vunmarshal(V9fsPDU *pdu, size_t offset,
                               const char *fmt, va_list ap)
 {
-    return v9fs_iov_vunmarshal(pdu->elem.out_sg, pdu->elem.out_num,
-                               offset, 1, fmt, ap);
+    V9fsState *s = pdu->s;
+    V9fsVirtioState *v = container_of(s, V9fsVirtioState, state);
+    VirtQueueElement *elem = &v->elems[pdu->idx];
+
+    return v9fs_iov_vunmarshal(elem->out_sg, elem->out_num, offset, 1, fmt, ap);
 }
 
 void virtio_init_iov_from_pdu(V9fsPDU *pdu, struct iovec **piov,
                               unsigned int *pniov, bool is_write)
 {
+    V9fsState *s = pdu->s;
+    V9fsVirtioState *v = container_of(s, V9fsVirtioState, state);
+    VirtQueueElement *elem = &v->elems[pdu->idx];
+
     if (is_write) {
-        *piov = pdu->elem.out_sg;
-        *pniov = pdu->elem.out_num;
+        *piov = elem->out_sg;
+        *pniov = elem->out_num;
     } else {
-        *piov = pdu->elem.in_sg;
-        *pniov = pdu->elem.in_num;
+        *piov = elem->in_sg;
+        *pniov = elem->in_num;
     }
 }
 
 /* virtio-9p device */
 
 static Property virtio_9p_properties[] = {
-    DEFINE_PROP_STRING("mount_tag", V9fsState, fsconf.tag),
-    DEFINE_PROP_STRING("fsdev", V9fsState, fsconf.fsdev_id),
+    DEFINE_PROP_STRING("mount_tag", V9fsVirtioState, state.fsconf.tag),
+    DEFINE_PROP_STRING("fsdev", V9fsVirtioState, state.fsconf.fsdev_id),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -175,7 +195,7 @@ static void virtio_9p_class_init(ObjectClass *klass, void *data)
 static const TypeInfo virtio_device_info = {
     .name = TYPE_VIRTIO_9P,
     .parent = TYPE_VIRTIO_DEVICE,
-    .instance_size = sizeof(V9fsState),
+    .instance_size = sizeof(V9fsVirtioState),
     .class_init = virtio_9p_class_init,
 };
 
diff --git a/hw/9pfs/virtio-9p.h b/hw/9pfs/virtio-9p.h
index 474ab94..1cdf0a2 100644
--- a/hw/9pfs/virtio-9p.h
+++ b/hw/9pfs/virtio-9p.h
@@ -5,6 +5,16 @@
 #include "hw/virtio/virtio.h"
 #include "9p.h"
 
+typedef struct V9fsVirtioState
+{
+    VirtIODevice parent_obj;
+    VirtQueue *vq;
+    size_t config_size;
+    V9fsPDU pdus[MAX_REQ];
+    VirtQueueElement elems[MAX_REQ];
+    V9fsState state;
+} V9fsVirtioState;
+
 extern void virtio_9p_push_and_notify(V9fsPDU *pdu);
 
 ssize_t virtio_pdu_vmarshal(V9fsPDU *pdu, size_t offset,
@@ -16,6 +26,6 @@ void virtio_init_iov_from_pdu(V9fsPDU *pdu, struct iovec **piov,
 
 #define TYPE_VIRTIO_9P "virtio-9p-device"
 #define VIRTIO_9P(obj) \
-        OBJECT_CHECK(V9fsState, (obj), TYPE_VIRTIO_9P)
+        OBJECT_CHECK(V9fsVirtioState, (obj), TYPE_VIRTIO_9P)
 
 #endif
diff --git a/hw/s390x/virtio-ccw.h b/hw/s390x/virtio-ccw.h
index 7ab8367..a526d2f 100644
--- a/hw/s390x/virtio-ccw.h
+++ b/hw/s390x/virtio-ccw.h
@@ -210,7 +210,7 @@ VirtIODevice *virtio_ccw_get_vdev(SubchDev *sch);
 
 typedef struct V9fsCCWState {
     VirtioCcwDevice parent_obj;
-    V9fsState vdev;
+    V9fsVirtioState vdev;
 } V9fsCCWState;
 
 #endif /* CONFIG_VIRTFS */
diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
index 7cf5974..e096e98 100644
--- a/hw/virtio/virtio-pci.h
+++ b/hw/virtio/virtio-pci.h
@@ -242,7 +242,7 @@ struct VirtIONetPCI {
 
 typedef struct V9fsPCIState {
     VirtIOPCIProxy parent_obj;
-    V9fsState vdev;
+    V9fsVirtioState vdev;
 } V9fsPCIState;
 
 #endif
-- 
2.1.4

Re: [Qemu-devel] [PATCH v3 0/3] 9pfs: disentangling virtio and generic code

[Wei Liu]

I forgot to mention -- this series can be pulled from:

  git://xenbits.xen.org/people/liuw/qemu.git wip.9pfs-refactor-v3

Wei.

Re: [Qemu-devel] [PATCH v3 2/3] 9pfs: use V9fsBlob to transmit xattr

[Aneesh Kumar K.V]

Wei Liu <wei.liu2@citrix.com> writes:

> And make v9fs_pack static function. Now we only need to export
> v9fs_{,un}marshal to device.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
> v3: fix bug discovered by Aneesh
> ---
>  fsdev/9p-iov-marshal.c |  4 ++--
>  fsdev/9p-iov-marshal.h |  3 ---
>  hw/9pfs/9p.c           | 21 +++++++++++++--------
>  3 files changed, 15 insertions(+), 13 deletions(-)
>
> diff --git a/fsdev/9p-iov-marshal.c b/fsdev/9p-iov-marshal.c
> index 1f9edf3..5c911c8 100644
> --- a/fsdev/9p-iov-marshal.c
> +++ b/fsdev/9p-iov-marshal.c
> @@ -70,8 +70,8 @@ static ssize_t v9fs_unpack(void *dst, struct iovec *out_sg, int out_num,
>      return v9fs_packunpack(dst, out_sg, out_num, offset, size, 0);
>  }
>
> -ssize_t v9fs_pack(struct iovec *in_sg, int in_num, size_t offset,
> -                  const void *src, size_t size)
> +static ssize_t v9fs_pack(struct iovec *in_sg, int in_num, size_t offset,
> +                         const void *src, size_t size)
>  {
>      return v9fs_packunpack((void *)src, in_sg, in_num, offset, size, 1);
>  }
> diff --git a/fsdev/9p-iov-marshal.h b/fsdev/9p-iov-marshal.h
> index 6bccbfb..410a1ea 100644
> --- a/fsdev/9p-iov-marshal.h
> +++ b/fsdev/9p-iov-marshal.h
> @@ -3,9 +3,6 @@
>
>  #include "9p-marshal.h"
>
> -
> -ssize_t v9fs_pack(struct iovec *in_sg, int in_num, size_t offset,
> -                  const void *src, size_t size);
>  ssize_t v9fs_iov_unmarshal(struct iovec *out_sg, int out_num, size_t offset,
>                             int bswap, const char *fmt, ...);
>  ssize_t v9fs_iov_marshal(struct iovec *in_sg, int in_num, size_t offset,
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index a904403..84cb1d9 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -1585,6 +1585,7 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp,
>      size_t offset = 7;
>      int read_count;
>      int64_t xattr_len;
> +    V9fsBlob blob;
>
>      xattr_len = fidp->fs.xattr.len;
>      read_count = xattr_len - off;
> @@ -1596,14 +1597,18 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp,
>           */
>          read_count = 0;
>      }
> -    err = pdu_marshal(pdu, offset, "d", read_count);
> -    if (err < 0) {
> -        return err;
> -    }
> -    offset += err;
> -    err = v9fs_pack(pdu->elem.in_sg, pdu->elem.in_num, offset,
> -                    ((char *)fidp->fs.xattr.value) + off,
> -                    read_count);
> +
> +    v9fs_blob_init(&blob);
> +
> +    blob.data = g_malloc(read_count);
> +    memcpy(blob.data, ((char *)fidp->fs.xattr.value) + off,
> +           read_count);

Can we do this without the malloc and memcpy ? . I am sure you need this
for Xen abstraction. But for now i am inclined to drop this from the
series and add this later with Xen transport.  v9fs_xattr_read is
essentially T_READ on a xattr fid and we don't use blob ("B") in other
code path. We also want to avoid that extra malloc and memcpy.



> +    blob.size = read_count;
> +
> +    err = pdu_marshal(pdu, offset, "B", &blob);
> +
> +    v9fs_blob_free(&blob);
> +
>      if (err < 0) {
>          return err;
>      }
> -- 
> 2.1.4

-aneesh

Re: [Qemu-devel] [PATCH v3 2/3] 9pfs: use V9fsBlob to transmit xattr

[Wei Liu]

On Mon, Jan 11, 2016 at 07:26:39PM +0530, Aneesh Kumar K.V wrote:
[...]
> >      xattr_len = fidp->fs.xattr.len;
> >      read_count = xattr_len - off;
> > @@ -1596,14 +1597,18 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp,
> >           */
> >          read_count = 0;
> >      }
> > -    err = pdu_marshal(pdu, offset, "d", read_count);
> > -    if (err < 0) {
> > -        return err;
> > -    }
> > -    offset += err;
> > -    err = v9fs_pack(pdu->elem.in_sg, pdu->elem.in_num, offset,
> > -                    ((char *)fidp->fs.xattr.value) + off,
> > -                    read_count);
> > +
> > +    v9fs_blob_init(&blob);
> > +
> > +    blob.data = g_malloc(read_count);
> > +    memcpy(blob.data, ((char *)fidp->fs.xattr.value) + off,
> > +           read_count);
> 
> Can we do this without the malloc and memcpy ? . I am sure you need this
> for Xen abstraction. But for now i am inclined to drop this from the
> series and add this later with Xen transport.  v9fs_xattr_read is
> essentially T_READ on a xattr fid and we don't use blob ("B") in other
> code path. We also want to avoid that extra malloc and memcpy.
> 

That's fine.

Do you want me to resend the whole series or just this one patch
(assuming you don't have other comments on my other patches)?


Wei.

Re: [Qemu-devel] [PATCH v3 2/3] 9pfs: use V9fsBlob to transmit xattr

[Aneesh Kumar K.V]

Wei Liu <wei.liu2@citrix.com> writes:

> On Mon, Jan 11, 2016 at 07:26:39PM +0530, Aneesh Kumar K.V wrote:
> [...]
>> >      xattr_len = fidp->fs.xattr.len;
>> >      read_count = xattr_len - off;
>> > @@ -1596,14 +1597,18 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp,
>> >           */
>> >          read_count = 0;
>> >      }
>> > -    err = pdu_marshal(pdu, offset, "d", read_count);
>> > -    if (err < 0) {
>> > -        return err;
>> > -    }
>> > -    offset += err;
>> > -    err = v9fs_pack(pdu->elem.in_sg, pdu->elem.in_num, offset,
>> > -                    ((char *)fidp->fs.xattr.value) + off,
>> > -                    read_count);
>> > +
>> > +    v9fs_blob_init(&blob);
>> > +
>> > +    blob.data = g_malloc(read_count);
>> > +    memcpy(blob.data, ((char *)fidp->fs.xattr.value) + off,
>> > +           read_count);
>> 
>> Can we do this without the malloc and memcpy ? . I am sure you need this
>> for Xen abstraction. But for now i am inclined to drop this from the
>> series and add this later with Xen transport.  v9fs_xattr_read is
>> essentially T_READ on a xattr fid and we don't use blob ("B") in other
>> code path. We also want to avoid that extra malloc and memcpy.
>> 
>
> That's fine.
>
> Do you want me to resend the whole series or just this one patch
> (assuming you don't have other comments on my other patches)?
>

No. I did this change on top of patch 3

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index a772963fb742..3ff310605cd4 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1585,6 +1585,8 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp,
     size_t offset = 7;
     int read_count;
     int64_t xattr_len;
+    V9fsVirtioState *v = container_of(s, V9fsVirtioState, state);
+    VirtQueueElement *elem = &v->elems[pdu->idx];
 
     xattr_len = fidp->fs.xattr.len;
     read_count = xattr_len - off;
@@ -1601,7 +1603,8 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp,
         return err;
     }
     offset += err;
-    err = v9fs_pack(pdu->elem.in_sg, pdu->elem.in_num, offset,
+
+    err = v9fs_pack(elem->in_sg, elem->in_num, offset,
                     ((char *)fidp->fs.xattr.value) + off,
                     read_count);
     if (err < 0) {

Re: [Qemu-devel] [PATCH v3 2/3] 9pfs: use V9fsBlob to transmit xattr

[Wei Liu]

On Mon, Jan 11, 2016 at 07:56:01PM +0530, Aneesh Kumar K.V wrote:
> Wei Liu <wei.liu2@citrix.com> writes:
> 
> > On Mon, Jan 11, 2016 at 07:26:39PM +0530, Aneesh Kumar K.V wrote:
> > [...]
> >> >      xattr_len = fidp->fs.xattr.len;
> >> >      read_count = xattr_len - off;
> >> > @@ -1596,14 +1597,18 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp,
> >> >           */
> >> >          read_count = 0;
> >> >      }
> >> > -    err = pdu_marshal(pdu, offset, "d", read_count);
> >> > -    if (err < 0) {
> >> > -        return err;
> >> > -    }
> >> > -    offset += err;
> >> > -    err = v9fs_pack(pdu->elem.in_sg, pdu->elem.in_num, offset,
> >> > -                    ((char *)fidp->fs.xattr.value) + off,
> >> > -                    read_count);
> >> > +
> >> > +    v9fs_blob_init(&blob);
> >> > +
> >> > +    blob.data = g_malloc(read_count);
> >> > +    memcpy(blob.data, ((char *)fidp->fs.xattr.value) + off,
> >> > +           read_count);
> >> 
> >> Can we do this without the malloc and memcpy ? . I am sure you need this
> >> for Xen abstraction. But for now i am inclined to drop this from the
> >> series and add this later with Xen transport.  v9fs_xattr_read is
> >> essentially T_READ on a xattr fid and we don't use blob ("B") in other
> >> code path. We also want to avoid that extra malloc and memcpy.
> >> 
> >
> > That's fine.
> >
> > Do you want me to resend the whole series or just this one patch
> > (assuming you don't have other comments on my other patches)?
> >
> 
> No. I did this change on top of patch 3
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index a772963fb742..3ff310605cd4 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -1585,6 +1585,8 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp,
>      size_t offset = 7;
>      int read_count;
>      int64_t xattr_len;
> +    V9fsVirtioState *v = container_of(s, V9fsVirtioState, state);
> +    VirtQueueElement *elem = &v->elems[pdu->idx];
>  
>      xattr_len = fidp->fs.xattr.len;
>      read_count = xattr_len - off;
> @@ -1601,7 +1603,8 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp,
>          return err;
>      }
>      offset += err;
> -    err = v9fs_pack(pdu->elem.in_sg, pdu->elem.in_num, offset,
> +
> +    err = v9fs_pack(elem->in_sg, elem->in_num, offset,
>                      ((char *)fidp->fs.xattr.value) + off,
>                      read_count);
>      if (err < 0) {
> 

Sorry I don't quite follow.

Is there any concrete action I need to take?

Wei.