[Qemu-devel] [PATCH 0/8] tests: add 9P functional tests

[Qemu-devel] [PATCH 0/8] tests: add 9P functional tests

[Greg Kurz]

This series brings the ability to test 9P operations. It provides basic tests
of version, attach and walk, which are essential operations in a 9P session.
It also provide two security-oriented tests of the walk operation (for issues
that got fixed in QEMU 2.7).

I could run this on ppc64, ppc64le and i386 hosts. It also passes travis-ci
checks. I plan to push this to 2.9.

---

Greg Kurz (8):
      tests: virtio-9p: rename PCI configuration test
      tests: virtio-9p: code refactoring
      9pfs: fix P9_NOTAG and P9_NOFID macros
      tests: virtio-9p: add version operation test
      tests: virtio-9p: add attach operation test
      tests: virtio-9p: add walk operation test
      tests: virtio-9p: no slash in path elements during walk
      tests: virtio-9p: ".." cannot be used to walk out of the shared directory


 hw/9pfs/9p.h           |    4 
 tests/virtio-9p-test.c |  478 +++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 429 insertions(+), 53 deletions(-)

--
Greg

[Qemu-devel] [PATCH 1/8] tests: virtio-9p: rename PCI configuration test

[Greg Kurz]

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 tests/virtio-9p-test.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/virtio-9p-test.c b/tests/virtio-9p-test.c
index 9c4f6cb40647..f458297f9ae1 100644
--- a/tests/virtio-9p-test.c
+++ b/tests/virtio-9p-test.c
@@ -92,7 +92,7 @@ static void qvirtio_9p_pci_free(QVirtIO9P *v9p)
     g_free(v9p);
 }
 
-static void pci_basic_config(void)
+static void pci_config(void)
 {
     QVirtIO9P *v9p;
     size_t tag_len;
@@ -121,7 +121,7 @@ int main(int argc, char **argv)
 {
     g_test_init(&argc, &argv, NULL);
     qtest_add_func("/virtio/9p/pci/nop", pci_nop);
-    qtest_add_func("/virtio/9p/pci/basic/configuration", pci_basic_config);
+    qtest_add_func("/virtio/9p/pci/config", pci_config);
 
     return g_test_run();
 }

[Qemu-devel] [PATCH 2/8] tests: virtio-9p: code refactoring

[Greg Kurz]

This moves the test_share static and the QOSState into the QVirtIO9P
structure, and put PCI related code in functions with a _pci_ name.

This will avoid code duplication in future tests, and allow to add
support for non-PCI platforms.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 tests/virtio-9p-test.c |  101 ++++++++++++++++++++++++------------------------
 1 file changed, 50 insertions(+), 51 deletions(-)

diff --git a/tests/virtio-9p-test.c b/tests/virtio-9p-test.c
index f458297f9ae1..727316086337 100644
--- a/tests/virtio-9p-test.c
+++ b/tests/virtio-9p-test.c
@@ -18,59 +18,49 @@
 #include "standard-headers/linux/virtio_pci.h"
 
 static const char mount_tag[] = "qtest";
-static char *test_share;
 
+typedef struct {
+    QVirtioDevice *dev;
+    QOSState *qs;
+    QVirtQueue *vq;
+    char *test_share;
+} QVirtIO9P;
 
-static QOSState *qvirtio_9p_start(void)
+static QVirtIO9P *qvirtio_9p_start(const char *driver)
 {
     const char *arch = qtest_get_arch();
     const char *cmd = "-fsdev local,id=fsdev0,security_model=none,path=%s "
-                      "-device virtio-9p-pci,fsdev=fsdev0,mount_tag=%s";
+                      "-device %s,fsdev=fsdev0,mount_tag=%s";
+    QVirtIO9P *v9p = g_new0(QVirtIO9P, 1);
 
-    test_share = g_strdup("/tmp/qtest.XXXXXX");
-    g_assert_nonnull(mkdtemp(test_share));
+    v9p->test_share = g_strdup("/tmp/qtest.XXXXXX");
+    g_assert_nonnull(mkdtemp(v9p->test_share));
 
     if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
-        return qtest_pc_boot(cmd, test_share, mount_tag);
-    }
-    if (strcmp(arch, "ppc64") == 0) {
-        return qtest_spapr_boot(cmd, test_share, mount_tag);
+        v9p->qs = qtest_pc_boot(cmd, v9p->test_share, driver, mount_tag);
+    } else if (strcmp(arch, "ppc64") == 0) {
+        v9p->qs = qtest_spapr_boot(cmd, v9p->test_share, driver, mount_tag);
+    } else {
+        g_printerr("virtio-9p tests are only available on x86 or ppc64\n");
+        exit(EXIT_FAILURE);
     }
 
-    g_printerr("virtio-9p tests are only available on x86 or ppc64\n");
-    exit(EXIT_FAILURE);
-}
-
-static void qvirtio_9p_stop(QOSState *qs)
-{
-    qtest_shutdown(qs);
-    rmdir(test_share);
-    g_free(test_share);
+    return v9p;
 }
 
-static void pci_nop(void)
+static void qvirtio_9p_stop(QVirtIO9P *v9p)
 {
-    QOSState *qs;
-
-    qs = qvirtio_9p_start();
-    qvirtio_9p_stop(qs);
+    qtest_shutdown(v9p->qs);
+    rmdir(v9p->test_share);
+    g_free(v9p->test_share);
+    g_free(v9p);
 }
 
-typedef struct {
-    QVirtioDevice *dev;
-    QOSState *qs;
-    QVirtQueue *vq;
-} QVirtIO9P;
-
-static QVirtIO9P *qvirtio_9p_pci_init(QOSState *qs)
+static QVirtIO9P *qvirtio_9p_pci_start(void)
 {
-    QVirtIO9P *v9p;
-    QVirtioPCIDevice *dev;
-
-    v9p = g_new0(QVirtIO9P, 1);
-
-    v9p->qs = qs;
-    dev = qvirtio_pci_device_find(v9p->qs->pcibus, VIRTIO_ID_9P);
+    QVirtIO9P *v9p = qvirtio_9p_start("virtio-9p-pci");
+    QVirtioPCIDevice *dev = qvirtio_pci_device_find(v9p->qs->pcibus,
+                                                    VIRTIO_ID_9P);
     g_assert_nonnull(dev);
     g_assert_cmphex(dev->vdev.device_type, ==, VIRTIO_ID_9P);
     v9p->dev = (QVirtioDevice *) dev;
@@ -84,26 +74,20 @@ static QVirtIO9P *qvirtio_9p_pci_init(QOSState *qs)
     return v9p;
 }
 
-static void qvirtio_9p_pci_free(QVirtIO9P *v9p)
+static void qvirtio_9p_pci_stop(QVirtIO9P *v9p)
 {
     qvirtqueue_cleanup(v9p->dev->bus, v9p->vq, v9p->qs->alloc);
     qvirtio_pci_device_disable(container_of(v9p->dev, QVirtioPCIDevice, vdev));
     g_free(v9p->dev);
-    g_free(v9p);
+    qvirtio_9p_stop(v9p);
 }
 
-static void pci_config(void)
+static void pci_config(QVirtIO9P *v9p)
 {
-    QVirtIO9P *v9p;
-    size_t tag_len;
+    size_t tag_len = qvirtio_config_readw(v9p->dev, 0);
     char *tag;
     int i;
-    QOSState *qs;
-
-    qs = qvirtio_9p_start();
-    v9p = qvirtio_9p_pci_init(qs);
 
-    tag_len = qvirtio_config_readw(v9p->dev, 0);
     g_assert_cmpint(tag_len, ==, strlen(mount_tag));
 
     tag = g_malloc(tag_len);
@@ -112,16 +96,31 @@ static void pci_config(void)
     }
     g_assert_cmpmem(tag, tag_len, mount_tag, tag_len);
     g_free(tag);
+}
+
+typedef void (*v9fs_test_fn)(QVirtIO9P *v9p);
+
+static void v9fs_run_pci_test(gconstpointer data)
+{
+    v9fs_test_fn fn = data;
+    QVirtIO9P *v9p = qvirtio_9p_pci_start();
 
-    qvirtio_9p_pci_free(v9p);
-    qvirtio_9p_stop(qs);
+    if (fn) {
+        fn(v9p);
+    }
+    qvirtio_9p_pci_stop(v9p);
+}
+
+static void v9fs_qtest_pci_add(const char *path, v9fs_test_fn fn)
+{
+    qtest_add_data_func(path, fn, v9fs_run_pci_test);
 }
 
 int main(int argc, char **argv)
 {
     g_test_init(&argc, &argv, NULL);
-    qtest_add_func("/virtio/9p/pci/nop", pci_nop);
-    qtest_add_func("/virtio/9p/pci/config", pci_config);
+    v9fs_qtest_pci_add("/virtio/9p/pci/nop", NULL);
+    v9fs_qtest_pci_add("/virtio/9p/pci/config", pci_config);
 
     return g_test_run();
 }

[Qemu-devel] [PATCH 3/8] 9pfs: fix P9_NOTAG and P9_NOFID macros

[Greg Kurz]

The u16 and u32 types don't exist in QEMU common headers. It never broke
build because these two macros aren't use by the current code, but this
is about to change with the future addition of functional tests for 9P.

This patch convert the types to uintXX_t.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p.h |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index 3976b7fe3dcd..89c904bdb7e7 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -99,8 +99,8 @@ enum p9_proto_version {
     V9FS_PROTO_2000L = 0x02,
 };
 
-#define P9_NOTAG    (u16)(~0)
-#define P9_NOFID    (u32)(~0)
+#define P9_NOTAG    (uint16_t)(~0)
+#define P9_NOFID    (uint32_t)(~0)
 #define P9_MAXWELEM 16
 
 #define FID_REFERENCED          0x1

[Qemu-devel] [PATCH 4/8] tests: virtio-9p: add version operation test

[Greg Kurz]

This patch lays the foundations to be able to test 9P operations and
provides a test for the version operation as a first example.

A 9P request is composed of a T-message sent by the client (guest) to the
server (QEMU), and a R-message sent by the server back to the client.

The following general calls are available to implement requests for any
9P operations:

v9fs_req_init(): allocates the request structure and the guest memory for
                 the T-message

v9fs_req_send(): allocates the guest memory for the R-message and sends the
                 T-message to QEMU

v9fs_req_recv(): waits for QEMU to answer and does some sanity checks on the
                 returned R-message header

v9fs_req_free(): releases the guest memory and the request structure

Helpers are provided, to be used by each specific 9P operation to copy data
to/from the guest memory.

The version operation is used to negotiate the 9P protocol version to be
used and the maximum buffer size for exchanged data. It is necessarily
the first message of a 9P session. For simplicity, the maximum buffer size
is hardcoded to 4k, which should be enough for functional tests.

The test simply advertises the "9P2000.L" version to QEMU and expects QEMU
to answer it is supported.

References:

http://man.cat-v.org/plan_9/5/intro
http://man.cat-v.org/plan_9/5/version

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 tests/virtio-9p-test.c |  222 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 222 insertions(+)

diff --git a/tests/virtio-9p-test.c b/tests/virtio-9p-test.c
index 727316086337..b3ee956cbcb2 100644
--- a/tests/virtio-9p-test.c
+++ b/tests/virtio-9p-test.c
@@ -16,6 +16,7 @@
 #include "libqos/virtio-pci.h"
 #include "standard-headers/linux/virtio_ids.h"
 #include "standard-headers/linux/virtio_pci.h"
+#include "hw/9pfs/9p.h"
 
 static const char mount_tag[] = "qtest";
 
@@ -98,6 +99,226 @@ static void pci_config(QVirtIO9P *v9p)
     g_free(tag);
 }
 
+#define P9_MAX_SIZE 4096 /* Max size of a T-message or R-message */
+
+typedef struct {
+    QVirtIO9P *v9p;
+    uint16_t tag;
+    uint64_t t_msg;
+    uint32_t t_size;
+    uint64_t r_msg;
+    /* No r_size, it is hardcoded to P9_MAX_SIZE */
+    size_t t_off;
+    size_t r_off;
+} P9Req;
+
+static void v9fs_memwrite(P9Req *req, const void *addr, size_t len)
+{
+    memwrite(req->t_msg + req->t_off, addr, len);
+    req->t_off += len;
+}
+
+static void v9fs_memskip(P9Req *req, size_t len)
+{
+    req->r_off += len;
+}
+
+static void v9fs_memrewind(P9Req *req, size_t len)
+{
+    req->r_off -= len;
+}
+
+static void v9fs_memread(P9Req *req, void *addr, size_t len)
+{
+    memread(req->r_msg + req->r_off, addr, len);
+    req->r_off += len;
+}
+
+static void v9fs_uint16_write(P9Req *req, uint16_t val)
+{
+    uint16_t le_val = cpu_to_le16(val);
+
+    v9fs_memwrite(req, &le_val, 2);
+}
+
+static void v9fs_uint16_read(P9Req *req, uint16_t *val)
+{
+    v9fs_memread(req, val, 2);
+    le16_to_cpus(val);
+}
+
+static void v9fs_uint32_write(P9Req *req, uint32_t val)
+{
+    uint32_t le_val = cpu_to_le32(val);
+
+    v9fs_memwrite(req, &le_val, 4);
+}
+
+static void v9fs_uint32_read(P9Req *req, uint32_t *val)
+{
+    v9fs_memread(req, val, 4);
+    le32_to_cpus(val);
+}
+
+/* len[2] string[len] */
+static uint16_t v9fs_string_size(const char *string)
+{
+    size_t len = strlen(string);
+
+    g_assert_cmpint(len, <=, UINT16_MAX);
+
+    return 2 + len;
+}
+
+static void v9fs_string_write(P9Req *req, const char *string)
+{
+    int len = strlen(string);
+
+    g_assert_cmpint(len, <=, UINT16_MAX);
+
+    v9fs_uint16_write(req, (uint16_t) len);
+    v9fs_memwrite(req, string, len);
+}
+
+static void v9fs_string_read(P9Req *req, uint16_t *len, char **string)
+{
+    uint16_t local_len;
+
+    v9fs_uint16_read(req, &local_len);
+    if (len) {
+        *len = local_len;
+    }
+    if (string) {
+        *string = g_malloc(local_len);
+        v9fs_memread(req, *string, local_len);
+    } else {
+        v9fs_memskip(req, local_len);
+    }
+}
+
+ typedef struct {
+    uint32_t size;
+    uint8_t id;
+    uint16_t tag;
+} QEMU_PACKED P9Hdr;
+
+static P9Req *v9fs_req_init(QVirtIO9P *v9p, uint32_t size, uint8_t id,
+                            uint16_t tag)
+{
+    P9Req *req = g_new0(P9Req, 1);
+    uint32_t t_size = 7 + size; /* 9P header has well-known size of 7 bytes */
+    P9Hdr hdr = {
+        .size = cpu_to_le32(t_size),
+        .id = id,
+        .tag = cpu_to_le16(tag)
+    };
+
+    g_assert_cmpint(t_size, <=, P9_MAX_SIZE);
+
+    req->v9p = v9p;
+    req->t_size = t_size;
+    req->t_msg = guest_alloc(v9p->qs->alloc, req->t_size);
+    v9fs_memwrite(req, &hdr, 7);
+    req->tag = tag;
+    return req;
+}
+
+static void v9fs_req_send(P9Req *req)
+{
+    QVirtIO9P *v9p = req->v9p;
+    uint32_t free_head;
+
+    req->r_msg = guest_alloc(v9p->qs->alloc, P9_MAX_SIZE);
+    free_head = qvirtqueue_add(v9p->vq, req->t_msg, req->t_size, false, true);
+    qvirtqueue_add(v9p->vq, req->r_msg, P9_MAX_SIZE, true, false);
+    qvirtqueue_kick(v9p->dev, v9p->vq, free_head);
+    req->t_off = 0;
+}
+
+static void v9fs_req_recv(P9Req *req, uint8_t id)
+{
+    QVirtIO9P *v9p = req->v9p;
+    P9Hdr hdr;
+    int i;
+
+    for (i = 0; i < 10; i++) {
+        qvirtio_wait_queue_isr(v9p->dev, v9p->vq, 1000 * 1000);
+
+        v9fs_memread(req, &hdr, 7);
+        le32_to_cpus(&hdr.size);
+        le16_to_cpus(&hdr.tag);
+        if (hdr.size >= 7) {
+            break;
+        }
+        v9fs_memrewind(req, 7);
+    }
+
+    g_assert_cmpint(hdr.size, >=, 7);
+    g_assert_cmpint(hdr.size, <=, P9_MAX_SIZE);
+    g_assert_cmpint(hdr.tag, ==, req->tag);
+
+    if (hdr.id != id && hdr.id == P9_RLERROR) {
+        uint32_t err;
+        v9fs_uint32_read(req, &err);
+        g_printerr("Received Rlerror (%d) instead of Response %d\n", err, id);
+        g_assert_not_reached();
+    }
+    g_assert_cmpint(hdr.id, ==, id);
+}
+
+static void v9fs_req_free(P9Req *req)
+{
+    QVirtIO9P *v9p = req->v9p;
+
+    guest_free(v9p->qs->alloc, req->t_msg);
+    guest_free(v9p->qs->alloc, req->r_msg);
+    g_free(req);
+}
+
+/* size[4] Tversion tag[2] msize[4] version[s] */
+static P9Req *v9fs_tversion(QVirtIO9P *v9p, uint32_t msize, const char *version)
+{
+    P9Req *req = v9fs_req_init(v9p, 4 + v9fs_string_size(version), P9_TVERSION,
+                               P9_NOTAG);
+
+    v9fs_uint32_write(req, msize);
+    v9fs_string_write(req, version);
+    v9fs_req_send(req);
+    return req;
+}
+
+/* size[4] Rversion tag[2] msize[4] version[s] */
+static void v9fs_rversion(P9Req *req, uint16_t *len, char **version)
+{
+    uint32_t msize;
+
+    v9fs_req_recv(req, P9_RVERSION);
+    v9fs_uint32_read(req, &msize);
+
+    g_assert_cmpint(msize, ==, P9_MAX_SIZE);
+
+    if (len || version) {
+        v9fs_string_read(req, len, version);
+    }
+
+    v9fs_req_free(req);
+}
+
+static void fs_version(QVirtIO9P *v9p)
+{
+    const char *version = "9P2000.L";
+    uint16_t server_len;
+    char *server_version;
+    P9Req *req;
+
+    req = v9fs_tversion(v9p, P9_MAX_SIZE, version);
+    v9fs_rversion(req, &server_len, &server_version);
+
+    g_assert_cmpmem(server_version, server_len, version, strlen(version));
+
+    g_free(server_version);
+}
+
 typedef void (*v9fs_test_fn)(QVirtIO9P *v9p);
 
 static void v9fs_run_pci_test(gconstpointer data)
@@ -121,6 +342,7 @@ int main(int argc, char **argv)
     g_test_init(&argc, &argv, NULL);
     v9fs_qtest_pci_add("/virtio/9p/pci/nop", NULL);
     v9fs_qtest_pci_add("/virtio/9p/pci/config", pci_config);
+    v9fs_qtest_pci_add("/virtio/9p/pci/fs/version/basic", fs_version);
 
     return g_test_run();
 }

[Qemu-devel] [PATCH 5/8] tests: virtio-9p: add attach operation test

[Greg Kurz]

The attach operation is used to establish a connection between the
client and the server. After this, the client is able to access the
underlying filesystem and do I/O.

This test simply ensures the operation succeeds without error.

Reference:

http://man.cat-v.org/plan_9/5/attach

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 tests/virtio-9p-test.c |   40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/tests/virtio-9p-test.c b/tests/virtio-9p-test.c
index b3ee956cbcb2..37a256ef4a59 100644
--- a/tests/virtio-9p-test.c
+++ b/tests/virtio-9p-test.c
@@ -25,6 +25,7 @@ typedef struct {
     QOSState *qs;
     QVirtQueue *vq;
     char *test_share;
+    uint16_t p9_req_tag;
 } QVirtIO9P;
 
 static QVirtIO9P *qvirtio_9p_start(const char *driver)
@@ -304,6 +305,35 @@ static void v9fs_rversion(P9Req *req, uint16_t *len, char **version)
     v9fs_req_free(req);
 }
 
+/* size[4] Tattach tag[2] fid[4] afid[4] uname[s] aname[s] n_uname[4] */
+static P9Req *v9fs_tattach(QVirtIO9P *v9p, uint32_t fid, uint32_t n_uname)
+{
+    const char *uname = ""; /* ignored by QEMU */
+    const char *aname = ""; /* ignored by QEMU */
+    P9Req *req = v9fs_req_init(v9p, 4 + 4 + 2 + 2 + 4, P9_TATTACH,
+                               ++(v9p->p9_req_tag));
+
+    v9fs_uint32_write(req, fid);
+    v9fs_uint32_write(req, P9_NOFID);
+    v9fs_string_write(req, uname);
+    v9fs_string_write(req, aname);
+    v9fs_uint32_write(req, n_uname);
+    v9fs_req_send(req);
+    return req;
+}
+
+typedef char v9fs_qid[13];
+
+/* size[4] Rattach tag[2] qid[13] */
+static void v9fs_rattach(P9Req *req, v9fs_qid *qid)
+{
+    v9fs_req_recv(req, P9_RATTACH);
+    if (qid) {
+        v9fs_memread(req, qid, 13);
+    }
+    v9fs_req_free(req);
+}
+
 static void fs_version(QVirtIO9P *v9p)
 {
     const char *version = "9P2000.L";
@@ -319,6 +349,15 @@ static void fs_version(QVirtIO9P *v9p)
     g_free(server_version);
 }
 
+static void fs_attach(QVirtIO9P *v9p)
+{
+    P9Req *req;
+
+    fs_version(v9p);
+    req = v9fs_tattach(v9p, 0, getuid());
+    v9fs_rattach(req, NULL);
+}
+
 typedef void (*v9fs_test_fn)(QVirtIO9P *v9p);
 
 static void v9fs_run_pci_test(gconstpointer data)
@@ -343,6 +382,7 @@ int main(int argc, char **argv)
     v9fs_qtest_pci_add("/virtio/9p/pci/nop", NULL);
     v9fs_qtest_pci_add("/virtio/9p/pci/config", pci_config);
     v9fs_qtest_pci_add("/virtio/9p/pci/fs/version/basic", fs_version);
+    v9fs_qtest_pci_add("/virtio/9p/pci/fs/attach/basic", fs_attach);
 
     return g_test_run();
 }

[Qemu-devel] [PATCH 6/8] tests: virtio-9p: add walk operation test

[Greg Kurz]

The walk operation is used to traverse the directory tree and to associate
paths to fids. A single walk can be used to traverse up to P9_MAXWELEM path
elements at the same time.

The test creates a path with P9_MAXWELEM elements on the backend (à la
'mkdir -p') and issues a walk operation. The walk is expected to succeed
without error.

Reference:

http://man.cat-v.org/plan_9/5/walk

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 tests/virtio-9p-test.c |   70 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/tests/virtio-9p-test.c b/tests/virtio-9p-test.c
index 37a256ef4a59..2deb7fbf92f1 100644
--- a/tests/virtio-9p-test.c
+++ b/tests/virtio-9p-test.c
@@ -334,6 +334,45 @@ static void v9fs_rattach(P9Req *req, v9fs_qid *qid)
     v9fs_req_free(req);
 }
 
+/* size[4] Twalk tag[2] fid[4] newfid[4] nwname[2] nwname*(wname[s]) */
+static P9Req *v9fs_twalk(QVirtIO9P *v9p, uint32_t fid, uint32_t newfid,
+                         uint16_t nwname, char *const wnames[])
+{
+    P9Req *req;
+    int i;
+    uint32_t size = 4 + 4 + 2;
+
+    for (i = 0; i < nwname; i++) {
+        size += v9fs_string_size(wnames[i]);
+    }
+    req = v9fs_req_init(v9p,  size, P9_TWALK, ++(v9p->p9_req_tag));
+    v9fs_uint32_write(req, fid);
+    v9fs_uint32_write(req, newfid);
+    v9fs_uint16_write(req, nwname);
+    for (i = 0; i < nwname; i++) {
+        v9fs_string_write(req, wnames[i]);
+    }
+    v9fs_req_send(req);
+    return req;
+}
+
+/* size[4] Rwalk tag[2] nwqid[2] nwqid*(wqid[13]) */
+static void v9fs_rwalk(P9Req *req, uint16_t *nwqid, v9fs_qid **wqid)
+{
+    uint16_t local_nwqid;
+
+    v9fs_req_recv(req, P9_RWALK);
+    v9fs_uint16_read(req, &local_nwqid);
+    if (nwqid) {
+        *nwqid = local_nwqid;
+    }
+    if (wqid) {
+        *wqid = g_malloc(local_nwqid * 13);
+        v9fs_memread(req, *wqid, local_nwqid * 13);
+    }
+    v9fs_req_free(req);
+}
+
 static void fs_version(QVirtIO9P *v9p)
 {
     const char *version = "9P2000.L";
@@ -358,6 +397,36 @@ static void fs_attach(QVirtIO9P *v9p)
     v9fs_rattach(req, NULL);
 }
 
+static void fs_walk(QVirtIO9P *v9p)
+{
+    char *wnames[P9_MAXWELEM], *paths[P9_MAXWELEM];
+    char *last_path = v9p->test_share;
+    uint16_t nwqid;
+    v9fs_qid *wqid;
+    int i;
+    P9Req *req;
+
+    for (i = 0; i < P9_MAXWELEM; i++) {
+        wnames[i] = g_strdup_printf("%s%d", __func__, i);
+        last_path = paths[i] = g_strdup_printf("%s/%s", last_path, wnames[i]);
+        g_assert(!mkdir(paths[i], 0700));
+    }
+
+    fs_attach(v9p);
+    req = v9fs_twalk(v9p, 0, 1, P9_MAXWELEM, wnames);
+    v9fs_rwalk(req, &nwqid, &wqid);
+
+    g_assert_cmpint(nwqid, ==, P9_MAXWELEM);
+
+    for (i = 0; i < P9_MAXWELEM; i++) {
+        rmdir(paths[P9_MAXWELEM - i - 1]);
+        g_free(paths[P9_MAXWELEM - i - 1]);
+        g_free(wnames[i]);
+    }
+
+    g_free(wqid);
+}
+
 typedef void (*v9fs_test_fn)(QVirtIO9P *v9p);
 
 static void v9fs_run_pci_test(gconstpointer data)
@@ -383,6 +452,7 @@ int main(int argc, char **argv)
     v9fs_qtest_pci_add("/virtio/9p/pci/config", pci_config);
     v9fs_qtest_pci_add("/virtio/9p/pci/fs/version/basic", fs_version);
     v9fs_qtest_pci_add("/virtio/9p/pci/fs/attach/basic", fs_attach);
+    v9fs_qtest_pci_add("/virtio/9p/pci/fs/walk/basic", fs_walk);
 
     return g_test_run();
 }

[Qemu-devel] [PATCH 7/8] tests: virtio-9p: no slash in path elements during walk

[Greg Kurz]

The walk operation is expected to fail and to return ENOENT.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 tests/virtio-9p-test.c |   24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/tests/virtio-9p-test.c b/tests/virtio-9p-test.c
index 2deb7fbf92f1..83cb23b3afc7 100644
--- a/tests/virtio-9p-test.c
+++ b/tests/virtio-9p-test.c
@@ -276,6 +276,14 @@ static void v9fs_req_free(P9Req *req)
     g_free(req);
 }
 
+/* size[4] Rlerror tag[2] ecode[4] */
+static void v9fs_rlerror(P9Req *req, uint32_t *err)
+{
+    v9fs_req_recv(req, P9_RLERROR);
+    v9fs_uint32_read(req, err);
+    v9fs_req_free(req);
+}
+
 /* size[4] Tversion tag[2] msize[4] version[s] */
 static P9Req *v9fs_tversion(QVirtIO9P *v9p, uint32_t msize, const char *version)
 {
@@ -427,6 +435,21 @@ static void fs_walk(QVirtIO9P *v9p)
     g_free(wqid);
 }
 
+static void fs_walk_no_slash(QVirtIO9P *v9p)
+{
+    char *const wnames[] = { g_strdup(" /") };
+    P9Req *req;
+    uint32_t err;
+
+    fs_attach(v9p);
+    req = v9fs_twalk(v9p, 0, 1, 1, wnames);
+    v9fs_rlerror(req, &err);
+
+    g_assert_cmpint(err, ==, ENOENT);
+
+    g_free(wnames[0]);
+}
+
 typedef void (*v9fs_test_fn)(QVirtIO9P *v9p);
 
 static void v9fs_run_pci_test(gconstpointer data)
@@ -453,6 +476,7 @@ int main(int argc, char **argv)
     v9fs_qtest_pci_add("/virtio/9p/pci/fs/version/basic", fs_version);
     v9fs_qtest_pci_add("/virtio/9p/pci/fs/attach/basic", fs_attach);
     v9fs_qtest_pci_add("/virtio/9p/pci/fs/walk/basic", fs_walk);
+    v9fs_qtest_pci_add("/virtio/9p/pci/fs/walk/no_slash", fs_walk_no_slash);
 
     return g_test_run();
 }

[Qemu-devel] [PATCH 8/8] tests: virtio-9p: ".." cannot be used to walk out of the shared directory

[Greg Kurz]

According to the 9P spec at http://man.cat-v.org/plan_9/5/intro, the
parent directory of the root directory of a server's tree is itself.
This test hence checks that the qid of the root directory as returned by
attach is the same as the qid of ".." when walking from the root directory.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 tests/virtio-9p-test.c |   21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/tests/virtio-9p-test.c b/tests/virtio-9p-test.c
index 83cb23b3afc7..060407b20e39 100644
--- a/tests/virtio-9p-test.c
+++ b/tests/virtio-9p-test.c
@@ -450,6 +450,25 @@ static void fs_walk_no_slash(QVirtIO9P *v9p)
     g_free(wnames[0]);
 }
 
+static void fs_walk_dotdot(QVirtIO9P *v9p)
+{
+    char *const wnames[] = { g_strdup("..") };
+    v9fs_qid root_qid, *wqid;
+    P9Req *req;
+
+    fs_version(v9p);
+    req = v9fs_tattach(v9p, 0, getuid());
+    v9fs_rattach(req, &root_qid);
+
+    req = v9fs_twalk(v9p, 0, 1, 1, wnames);
+    v9fs_rwalk(req, NULL, &wqid); /* We now we'll get one qid */
+
+    g_assert_cmpmem(&root_qid, 13, wqid[0], 13);
+
+    g_free(wqid);
+    g_free(wnames[0]);
+}
+
 typedef void (*v9fs_test_fn)(QVirtIO9P *v9p);
 
 static void v9fs_run_pci_test(gconstpointer data)
@@ -477,6 +496,8 @@ int main(int argc, char **argv)
     v9fs_qtest_pci_add("/virtio/9p/pci/fs/attach/basic", fs_attach);
     v9fs_qtest_pci_add("/virtio/9p/pci/fs/walk/basic", fs_walk);
     v9fs_qtest_pci_add("/virtio/9p/pci/fs/walk/no_slash", fs_walk_no_slash);
+    v9fs_qtest_pci_add("/virtio/9p/pci/fs/walk/dotdot_from_root",
+                       fs_walk_dotdot);
 
     return g_test_run();
 }

Re: [Qemu-devel] [PATCH 3/8] 9pfs: fix P9_NOTAG and P9_NOFID macros

[Eric Blake]

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

On 12/09/2016 03:28 AM, Greg Kurz wrote:
> The u16 and u32 types don't exist in QEMU common headers. It never broke
> build because these two macros aren't use by the current code, but this
> is about to change with the future addition of functional tests for 9P.
> 
> This patch convert the types to uintXX_t.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/9pfs/9p.h |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
> index 3976b7fe3dcd..89c904bdb7e7 100644
> --- a/hw/9pfs/9p.h
> +++ b/hw/9pfs/9p.h
> @@ -99,8 +99,8 @@ enum p9_proto_version {
>      V9FS_PROTO_2000L = 0x02,
>  };
>  
> -#define P9_NOTAG    (u16)(~0)
> -#define P9_NOFID    (u32)(~0)
> +#define P9_NOTAG    (uint16_t)(~0)
> +#define P9_NOFID    (uint32_t)(~0)

Don't you want to write ((uint16_t)(~0)), to ensure that this expression
can be used as a drop-in in any other syntactical situation?

Or even write it as UINT16_C(~0) (using <stdint.h>), or as UINT16_MAX.
(Be aware: the type of (uint16_t)(~0) is uint16_t, while the type of
UINT16_MAX is int, due to the rules of integer promotion, if that matters)

-- 
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 3/8] 9pfs: fix P9_NOTAG and P9_NOFID macros

[Greg Kurz]

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

On Fri, 9 Dec 2016 15:25:55 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 12/09/2016 03:28 AM, Greg Kurz wrote:
> > The u16 and u32 types don't exist in QEMU common headers. It never broke
> > build because these two macros aren't use by the current code, but this
> > is about to change with the future addition of functional tests for 9P.
> > 
> > This patch convert the types to uintXX_t.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  hw/9pfs/9p.h |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
> > index 3976b7fe3dcd..89c904bdb7e7 100644
> > --- a/hw/9pfs/9p.h
> > +++ b/hw/9pfs/9p.h
> > @@ -99,8 +99,8 @@ enum p9_proto_version {
> >      V9FS_PROTO_2000L = 0x02,
> >  };
> >  
> > -#define P9_NOTAG    (u16)(~0)
> > -#define P9_NOFID    (u32)(~0)
> > +#define P9_NOTAG    (uint16_t)(~0)
> > +#define P9_NOFID    (uint32_t)(~0)  
> 
> Don't you want to write ((uint16_t)(~0)), to ensure that this expression
> can be used as a drop-in in any other syntactical situation?
> 

These defines come from the linux kernel sources and I must admit it
didn't cross my mind... can you share a case where this would cause
troubles ?

> Or even write it as UINT16_C(~0) (using <stdint.h>), or as UINT16_MAX.
> (Be aware: the type of (uint16_t)(~0) is uint16_t, while the type of
> UINT16_MAX is int, due to the rules of integer promotion, if that matters)
> 

UINT16_C(~0) expands to ~0 and UINT16_MAX expands to (65535), at least on
my laptop (glibc-headers-2.23.1-11.fc24.x86_64)... doesn't that mean the
type of UINT16_C(~0) is also int ? Please enlighten me.

The 9P spec at http://man.cat-v.org/plan_9/5/version says "(ushort)~0". My
understanding is 16 bits all ones. I guess I'd rather then go for
((uint16_t)(~0)).

Thanks!

--
Greg

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

Re: [Qemu-devel] [PATCH 3/8] 9pfs: fix P9_NOTAG and P9_NOFID macros

[Eric Blake]

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

On 12/10/2016 07:57 AM, Greg Kurz wrote:

>>> -#define P9_NOTAG    (u16)(~0)
>>> -#define P9_NOFID    (u32)(~0)
>>> +#define P9_NOTAG    (uint16_t)(~0)
>>> +#define P9_NOFID    (uint32_t)(~0)  
>>
>> Don't you want to write ((uint16_t)(~0)), to ensure that this expression
>> can be used as a drop-in in any other syntactical situation?
>>
> 
> These defines come from the linux kernel sources and I must admit it
> didn't cross my mind... can you share a case where this would cause
> troubles ?

Unlikely to occur in real code, but:

  int a[] = { -2, -3 };
  int *b = a + 1;
  printf("%d\n", (uint16_t)(~0)[b]); // prints 65534 - let's see why?

  // prints 65534, or the result of b[-1] cast to uint16_t
  printf("%d\n", (uint16_t)((~0)[b]));

  // probably dumps core, as b[65535] is out of bounds
  printf("%d\n", ((uint16_t)(~0))[b]);

that is, since [] has higher precedence than casts, failure to
parenthesize a cast will change the interpretation of P9_NOTAG[pointer].

And yes, if you copied from the kernel, that means the kernel has a bug
(even if it is unlikely to trip up normal code).


> 
>> Or even write it as UINT16_C(~0) (using <stdint.h>), or as UINT16_MAX.
>> (Be aware: the type of (uint16_t)(~0) is uint16_t, while the type of
>> UINT16_MAX is int, due to the rules of integer promotion, if that matters)
>>
> 
> UINT16_C(~0) expands to ~0 and UINT16_MAX expands to (65535), at least on
> my laptop (glibc-headers-2.23.1-11.fc24.x86_64)... doesn't that mean the
> type of UINT16_C(~0) is also int ? Please enlighten me.

Indeed, UINT16_C produces an int constant, not uint16_t (since there is
no such thing as a uint16_t constant).  So the cast is the only way to
force ~0 to be truncated to a 16-bit pattern.  But using UINT16_MAX is
probably just fine, as it is the all-ones value with the correct integer
promotion for use in any other arithmetic.

> 
> The 9P spec at http://man.cat-v.org/plan_9/5/version says "(ushort)~0". My
> understanding is 16 bits all ones. I guess I'd rather then go for
> ((uint16_t)(~0)).

Verbose, but works, as does UINT16_MAX.  But I stand corrected that
UINT16_C(~0) does not work.

-- 
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 3/8] 9pfs: fix P9_NOTAG and P9_NOFID macros

[Greg Kurz]

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

On Sat, 10 Dec 2016 10:24:35 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 12/10/2016 07:57 AM, Greg Kurz wrote:
> 
> >>> -#define P9_NOTAG    (u16)(~0)
> >>> -#define P9_NOFID    (u32)(~0)
> >>> +#define P9_NOTAG    (uint16_t)(~0)
> >>> +#define P9_NOFID    (uint32_t)(~0)    
> >>
> >> Don't you want to write ((uint16_t)(~0)), to ensure that this expression
> >> can be used as a drop-in in any other syntactical situation?
> >>  
> > 
> > These defines come from the linux kernel sources and I must admit it
> > didn't cross my mind... can you share a case where this would cause
> > troubles ?  
> 
> Unlikely to occur in real code, but:
> 
>   int a[] = { -2, -3 };
>   int *b = a + 1;
>   printf("%d\n", (uint16_t)(~0)[b]); // prints 65534 - let's see why?
> 
>   // prints 65534, or the result of b[-1] cast to uint16_t
>   printf("%d\n", (uint16_t)((~0)[b]));
> 
>   // probably dumps core, as b[65535] is out of bounds
>   printf("%d\n", ((uint16_t)(~0))[b]);
> 
> that is, since [] has higher precedence than casts, failure to
> parenthesize a cast will change the interpretation of P9_NOTAG[pointer].
> 

... which is indeed very unlikely to happen even if it is legit. :)

> And yes, if you copied from the kernel, that means the kernel has a bug
> (even if it is unlikely to trip up normal code).
> 

I'll send a patch there too.

> 
> >   
> >> Or even write it as UINT16_C(~0) (using <stdint.h>), or as UINT16_MAX.
> >> (Be aware: the type of (uint16_t)(~0) is uint16_t, while the type of
> >> UINT16_MAX is int, due to the rules of integer promotion, if that matters)
> >>  
> > 
> > UINT16_C(~0) expands to ~0 and UINT16_MAX expands to (65535), at least on
> > my laptop (glibc-headers-2.23.1-11.fc24.x86_64)... doesn't that mean the
> > type of UINT16_C(~0) is also int ? Please enlighten me.  
> 
> Indeed, UINT16_C produces an int constant, not uint16_t (since there is
> no such thing as a uint16_t constant).  So the cast is the only way to
> force ~0 to be truncated to a 16-bit pattern.  But using UINT16_MAX is
> probably just fine, as it is the all-ones value with the correct integer
> promotion for use in any other arithmetic.
> 
> > 
> > The 9P spec at http://man.cat-v.org/plan_9/5/version says "(ushort)~0". My
> > understanding is 16 bits all ones. I guess I'd rather then go for
> > ((uint16_t)(~0)).  
> 
> Verbose, but works, as does UINT16_MAX.  But I stand corrected that
> UINT16_C(~0) does not work.
> 

Ok, I'll go the UINT16_MAX way then.

Thanks for the detailed explanation.

Cheers.

--
Greg

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