[PATCH 01/12] tests/qtest/qgraph: add qemu_name to QOSGraphNode

[PATCH 01/12] tests/qtest/qgraph: add qemu_name to QOSGraphNode

[Christian Schoenebeck]

Add new member variable 'qemu_name' to struct QOSGraphNode.

This new member may be optionally set in case a different
name for the node (which must always be unique) vs. its
actually associated QEMU (QMP) device name is required.

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 tests/qtest/libqos/qgraph.c          | 1 +
 tests/qtest/libqos/qgraph_internal.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/tests/qtest/libqos/qgraph.c b/tests/qtest/libqos/qgraph.c
index fc49cfa879..e42f3eaafa 100644
--- a/tests/qtest/libqos/qgraph.c
+++ b/tests/qtest/libqos/qgraph.c
@@ -153,6 +153,7 @@ static QOSGraphNode *create_node(const char *name, QOSNodeType type)
 static void destroy_node(void *val)
 {
     QOSGraphNode *node = val;
+    g_free(node->qemu_name);
     g_free(node->command_line);
     g_free(node);
 }
diff --git a/tests/qtest/libqos/qgraph_internal.h b/tests/qtest/libqos/qgraph_internal.h
index 968fa69450..974985dce9 100644
--- a/tests/qtest/libqos/qgraph_internal.h
+++ b/tests/qtest/libqos/qgraph_internal.h
@@ -56,6 +56,7 @@ struct QOSGraphNode {
     bool available;     /* set by QEMU via QMP, used during graph walk */
     bool visited;       /* used during graph walk */
     char *name;         /* used to identify the node */
+    char *qemu_name;    /* optional: see qos_node_create_driver_named() */
     char *command_line; /* used to start QEMU at test execution */
     union {
         struct {
-- 
2.20.1

[PATCH 02/12] tests/qtest/qgraph: add qos_node_create_driver_named()

[Christian Schoenebeck]

So far the qos subsystem of the qtest framework had the limitation
that only one instance of the same official QEMU (QMP) driver name
could be created for qtests. That's because a) the created qos
node names must always be unique, b) the node name must match the
official QEMU driver name being instantiated and c) all nodes are
in a global space shared by all tests.

This patch removes this limitation by introducing a new function
qos_node_create_driver_named() which allows test case authors to
specify a node name being different from the actual associated
QEMU driver name. It fills the new 'qemu_name' field of
QOSGraphNode for that purpose.

build_driver_cmd_line() and qos_graph_node_set_availability() had
to be adjusted to correctly deal with either accessing node name
vs. node's qemu_name correctly.

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 tests/qtest/libqos/qgraph.c | 53 ++++++++++++++++++++++++++++++++++---
 tests/qtest/libqos/qgraph.h | 16 +++++++++++
 2 files changed, 66 insertions(+), 3 deletions(-)

diff --git a/tests/qtest/libqos/qgraph.c b/tests/qtest/libqos/qgraph.c
index e42f3eaafa..61faf6b27d 100644
--- a/tests/qtest/libqos/qgraph.c
+++ b/tests/qtest/libqos/qgraph.c
@@ -287,7 +287,8 @@ static void build_machine_cmd_line(QOSGraphNode *node, const char *args)
  */
 static void build_driver_cmd_line(QOSGraphNode *node)
 {
-    node->command_line = g_strconcat(" -device ", node->name, NULL);
+    const char *name = node->qemu_name ?: node->name;
+    node->command_line = g_strconcat(" -device ", name, NULL);
 }
 
 /* qos_print_cb(): callback prints all path found by the DFS algorithm. */
@@ -632,6 +633,15 @@ void qos_node_create_driver(const char *name, QOSCreateDriverFunc function)
     node->u.driver.constructor = function;
 }
 
+void qos_node_create_driver_named(const char *name, const char *qemu_name,
+                                  QOSCreateDriverFunc function)
+{
+    QOSGraphNode *node = create_node(name, QNODE_DRIVER);
+    node->qemu_name = g_strdup(qemu_name);
+    build_driver_cmd_line(node);
+    node->u.driver.constructor = function;
+}
+
 void qos_node_contains(const char *container, const char *contained,
                        QOSGraphEdgeOptions *opts, ...)
 {
@@ -664,7 +674,7 @@ void qos_node_consumes(const char *consumer, const char *interface,
     add_edge(interface, consumer, QEDGE_CONSUMED_BY, opts);
 }
 
-void qos_graph_node_set_availability(const char *node, bool av)
+static void qos_graph_node_set_availability_explicit(const char *node, bool av)
 {
     QOSGraphEdgeList *elist;
     QOSGraphNode *n = search_node(node);
@@ -679,9 +689,46 @@ void qos_graph_node_set_availability(const char *node, bool av)
     }
     QSLIST_FOREACH_SAFE(e, elist, edge_list, next) {
         if (e->type == QEDGE_CONTAINS || e->type == QEDGE_PRODUCES) {
-            qos_graph_node_set_availability(e->dest, av);
+            qos_graph_node_set_availability_explicit(e->dest, av);
+        }
+    }
+}
+
+/*
+ * Behaves as qos_graph_node_set_availability_explicit(), except that the
+ * former always matches by node name only, whereas this function matches both
+ * by node name and node's optional 'qemu_name' field.
+ */
+void qos_graph_node_set_availability(const char *node, bool av)
+{
+    GList *l;
+    QOSGraphEdgeList *elist;
+    QOSGraphEdge *e, *next;
+    QOSGraphNode *n;
+    GList *keys = g_hash_table_get_keys(node_table);
+
+    for (l = keys; l != NULL; l = l->next) {
+        const gchar *key = l->data;
+        n = g_hash_table_lookup(node_table, key);
+        /*
+         * node's 'qemu_name' is set if there is more than one device with
+         * the same QEMU (QMP) device name
+         */
+        const char *node_name = n->qemu_name ?: n->name;
+        if (g_strcmp0(node_name, node) == 0) {
+            n->available = av;
+            elist = get_edgelist(n->name);
+            if (elist) {
+                QSLIST_FOREACH_SAFE(e, elist, edge_list, next) {
+                    if (e->type == QEDGE_CONTAINS || e->type == QEDGE_PRODUCES)
+                    {
+                        qos_graph_node_set_availability_explicit(e->dest, av);
+                    }
+                }
+            }
         }
     }
+    g_list_free(keys);
 }
 
 void qos_graph_foreach_test_path(QOSTestCallback fn)
diff --git a/tests/qtest/libqos/qgraph.h b/tests/qtest/libqos/qgraph.h
index 5f63d352ca..f472949f68 100644
--- a/tests/qtest/libqos/qgraph.h
+++ b/tests/qtest/libqos/qgraph.h
@@ -452,6 +452,22 @@ void qos_node_create_machine_args(const char *name,
  */
 void qos_node_create_driver(const char *name, QOSCreateDriverFunc function);
 
+/**
+ * Behaves as qos_node_create_driver() with the extension of allowing to
+ * specify a different node name vs. associated QEMU device name.
+ *
+ * Use this function instead of qos_node_create_driver() if you need to create
+ * several instances of the same QEMU device. You are free to choose a custom
+ * node name, however the chosen node name must always be unique.
+ *
+ * @param name: custom, unique name of the node to be created
+ * @param qemu_name: actual (official) QEMU driver name the node shall be
+ *                   associated with
+ * @param function: driver constructor
+ */
+void qos_node_create_driver_named(const char *name, const char *qemu_name,
+                                  QOSCreateDriverFunc function);
+
 /**
  * qos_node_contains(): creates one or more edges of type QEDGE_CONTAINS
  * and adds them to the edge list mapped to @container in the
-- 
2.20.1

[PATCH 03/12] tests/qtest/qos: add qos_dump_graph()

[Christian Schoenebeck]

This new function is purely for debugging purposes. It prints the
current qos graph to stdout and allows to identify problems in the
created qos graph e.g. when writing new qos tests.

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 tests/qtest/libqos/qgraph.c | 54 +++++++++++++++++++++++++++++++++++++
 tests/qtest/libqos/qgraph.h | 20 ++++++++++++++
 2 files changed, 74 insertions(+)

diff --git a/tests/qtest/libqos/qgraph.c b/tests/qtest/libqos/qgraph.c
index 61faf6b27d..e70635750e 100644
--- a/tests/qtest/libqos/qgraph.c
+++ b/tests/qtest/libqos/qgraph.c
@@ -805,3 +805,57 @@ void qos_delete_cmd_line(const char *name)
         node->command_line = NULL;
     }
 }
+
+#define RED(txt) (    \
+    "\033[0;91m" txt  \
+    "\033[0m"         \
+)
+
+#define GREEN(txt) (    \
+    "\033[0;92m" txt  \
+    "\033[0m"         \
+)
+
+void qos_dump_graph(void)
+{
+    GList *keys;
+    GList *l;
+    QOSGraphEdgeList *list;
+    QOSGraphEdge *e, *next;
+    QOSGraphNode *dest_node, *node;
+
+    printf("ALL QGRAPH EDGES: {\n");
+    keys = g_hash_table_get_keys(edge_table);
+    for (l = keys; l != NULL; l = l->next) {
+        const gchar *key = l->data;
+        printf("\t src='%s'\n", key);
+        list = get_edgelist(key);
+        QSLIST_FOREACH_SAFE(e, list, edge_list, next) {
+            dest_node = g_hash_table_lookup(node_table, e->dest);
+            printf("\t\t|-> dest='%s' type=%d (node=%p)",
+                   e->dest, e->type, dest_node);
+            if (!dest_node) {
+                printf(RED(" <------- ERROR !"));
+            }
+            printf("\n");
+        }
+    }
+    g_list_free(keys);
+    printf("}\n");
+
+    printf("ALL QGRAPH NODES: {\n");
+    keys = g_hash_table_get_keys(node_table);
+    for (l = keys; l != NULL; l = l->next) {
+        const gchar *key = l->data;
+        node = g_hash_table_lookup(node_table, key);
+        printf("\t name='%s' ", key);
+        if (node->qemu_name) {
+            printf("qemu_name='%s' ", node->qemu_name);
+        }
+        printf("type=%d cmd_line='%s' [%s]\n",
+               node->type, node->command_line,
+               node->available ? GREEN("available") : RED("UNAVAILBLE"));
+    }
+    g_list_free(keys);
+    printf("}\n");
+}
diff --git a/tests/qtest/libqos/qgraph.h b/tests/qtest/libqos/qgraph.h
index f472949f68..07a32535f1 100644
--- a/tests/qtest/libqos/qgraph.h
+++ b/tests/qtest/libqos/qgraph.h
@@ -586,5 +586,25 @@ QOSGraphObject *qos_machine_new(QOSGraphNode *node, QTestState *qts);
 QOSGraphObject *qos_driver_new(QOSGraphNode *node, QOSGraphObject *parent,
                                QGuestAllocator *alloc, void *arg);
 
+/**
+ * Just for debugging purpose: prints all currently existing nodes and
+ * edges to stdout.
+ *
+ * All qtests add themselves to the overall qos graph by calling qgraph
+ * functions that add device nodes and edges between the individual graph
+ * nodes for tests. As the actual graph is assmbled at runtime by the qos
+ * subsystem, it is sometimes not obvious how the overall graph looks like.
+ * E.g. when writing new tests it may happen that those new tests are simply
+ * ignored by the qtest framework.
+ *
+ * This function allows to identify problems in the created qgraph. Keep in
+ * mind: only tests with a path down from the actual test case node (leaf) up
+ * to the graph's root node are actually executed by the qtest framework. And
+ * the qtest framework uses QMP to automatically check which QEMU drivers are
+ * actually currently available, and accordingly qos marks certain pathes as
+ * 'unavailable' in such cases (e.g. when QEMU was compiled without support for
+ * a certain feature).
+ */
+void qos_dump_graph(void);
 
 #endif
-- 
2.20.1

[PATCH 04/12] tests/qtest/qos-test: new QTEST_DUMP_GRAPH environment variable

[Christian Schoenebeck]

Setting this new QTEST_DUMP_GRAPH environment variable causes qos-test
to dump the created qos graph (all nodes and edges, along with their
current individual availability status) to stdout.

See API doc comment on function qos_dump_graph() for details.

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 tests/qtest/qos-test.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/qtest/qos-test.c b/tests/qtest/qos-test.c
index 8fdf87b183..db667e16da 100644
--- a/tests/qtest/qos-test.c
+++ b/tests/qtest/qos-test.c
@@ -322,6 +322,9 @@ int main(int argc, char **argv)
     qos_set_machines_devices_available();
 
     qos_graph_foreach_test_path(walk_path);
+    if (getenv("QTEST_DUMP_GRAPH") != NULL) {
+        qos_dump_graph();
+    }
     g_test_run();
     qtest_end();
     qos_graph_destroy();
-- 
2.20.1

[PATCH 05/12] tests/qtest/qos-test: add QTEST_DUMP_ENV environment variable

[Christian Schoenebeck]

If this new QTEST_DUMP_ENV is set, it will cause all environment
variables to be dumped to stdout.

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 tests/qtest/qos-test.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/tests/qtest/qos-test.c b/tests/qtest/qos-test.c
index db667e16da..4b1a1922fc 100644
--- a/tests/qtest/qos-test.c
+++ b/tests/qtest/qos-test.c
@@ -313,8 +313,15 @@ static void walk_path(QOSGraphNode *orig_path, int len)
  *   machine/drivers/test objects
  * - Cleans up everything
  */
-int main(int argc, char **argv)
+int main(int argc, char **argv, char** envp)
 {
+    if (getenv("QTEST_DUMP_ENV") != NULL) {
+        printf("ENVIRONMENT VARIABLES: {\n");
+        for (char **env = envp; *env != 0; env++) {
+            printf("\t%s\n", *env);
+        }
+        printf("}\n");
+    }
     g_test_init(&argc, &argv, NULL);
     qos_graph_init();
     module_call_init(MODULE_INIT_QOM);
-- 
2.20.1

[PATCH 06/12] tests/qtest/qos-test: add environment variable QTEST_DEBUG

[Christian Schoenebeck]

For now this new environment variable QTEST_DEBUG will cause the
assembled qemu command line to be printed before running each test.

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 tests/qtest/qos-test.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/qtest/qos-test.c b/tests/qtest/qos-test.c
index 4b1a1922fc..571d1e140a 100644
--- a/tests/qtest/qos-test.c
+++ b/tests/qtest/qos-test.c
@@ -89,6 +89,9 @@ static void qos_set_machines_devices_available(void)
 
 static void restart_qemu_or_continue(char *path)
 {
+    if (getenv("QTEST_DEBUG") != NULL) {
+        printf("run QEMU with: '%s'\n", path);
+    }
     /* compares the current command line with the
      * one previously executed: if they are the same,
      * don't restart QEMU, if they differ, stop previous
-- 
2.20.1

[PATCH 07/12] test/9pfs: change export tag name to qtest-synth

[Christian Schoenebeck]

All existing 9pfs test cases are using the 'synth' fs driver so far, which
means they are not accessing real files, but a purely simulated (in RAM
only) file system.

Let's start to make this clear with this and the following patch to pave
the way for upcoming new tests going to use the 9pfs 'local' driver
instead.

This patch starts by changing the 9p tag name (which identifies one
particular 9p exported file tree) from 'qtest' to 'qtest-synth' and also
change the preprocessor macro used for this from TAG_NAME to TAG_NAME_SYNTH.

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 tests/qtest/libqos/virtio-9p.c | 4 ++--
 tests/qtest/libqos/virtio-9p.h | 2 +-
 tests/qtest/virtio-9p-test.c   | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/tests/qtest/libqos/virtio-9p.c b/tests/qtest/libqos/virtio-9p.c
index 2e300063e3..9f099737f9 100644
--- a/tests/qtest/libqos/virtio-9p.c
+++ b/tests/qtest/libqos/virtio-9p.c
@@ -148,8 +148,8 @@ static void *virtio_9p_pci_create(void *pci_bus, QGuestAllocator *t_alloc,
 
 static void virtio_9p_register_nodes(void)
 {
-    const char *str_simple = "fsdev=fsdev0,mount_tag=" MOUNT_TAG;
-    const char *str_addr = "fsdev=fsdev0,addr=04.0,mount_tag=" MOUNT_TAG;
+    const char *str_simple = "fsdev=fsdev0,mount_tag=" MOUNT_TAG_SYNTH;
+    const char *str_addr = "fsdev=fsdev0,addr=04.0,mount_tag=" MOUNT_TAG_SYNTH;
 
     QPCIAddress addr = {
         .devfn = QPCI_DEVFN(4, 0),
diff --git a/tests/qtest/libqos/virtio-9p.h b/tests/qtest/libqos/virtio-9p.h
index b1e6badc4a..d9a815083f 100644
--- a/tests/qtest/libqos/virtio-9p.h
+++ b/tests/qtest/libqos/virtio-9p.h
@@ -27,7 +27,7 @@ typedef struct QVirtio9P QVirtio9P;
 typedef struct QVirtio9PPCI QVirtio9PPCI;
 typedef struct QVirtio9PDevice QVirtio9PDevice;
 
-#define MOUNT_TAG "qtest"
+#define MOUNT_TAG_SYNTH "qtest-synth"
 
 struct QVirtio9P {
     QVirtioDevice *vdev;
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index de30b717b6..f7505396f3 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -26,13 +26,13 @@ static void pci_config(void *obj, void *data, QGuestAllocator *t_alloc)
     char *tag;
     int i;
 
-    g_assert_cmpint(tag_len, ==, strlen(MOUNT_TAG));
+    g_assert_cmpint(tag_len, ==, strlen(MOUNT_TAG_SYNTH));
 
     tag = g_malloc(tag_len);
     for (i = 0; i < tag_len; i++) {
         tag[i] = qvirtio_config_readb(v9p->vdev, i + 2);
     }
-    g_assert_cmpmem(tag, tag_len, MOUNT_TAG, tag_len);
+    g_assert_cmpmem(tag, tag_len, MOUNT_TAG_SYNTH, tag_len);
     g_free(tag);
 }
 
-- 
2.20.1

[PATCH 08/12] tests/9pfs: refactor test names and test devices

[Christian Schoenebeck]

Rename all 9pfs tests and devices they create for running their tests
from 'virtio*' -> 'virtio*-synth'.

In order for the tests still to work after this renaming, use the
newly added function qos_node_create_driver_named() instead of
qos_node_create_driver(). That new function allows to assign a name
to a device that differs from the actual QEMU driver it's using.

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 tests/qtest/libqos/virtio-9p.c | 29 ++++++++++++++-------------
 tests/qtest/virtio-9p-test.c   | 36 +++++++++++++++++-----------------
 2 files changed, 33 insertions(+), 32 deletions(-)

diff --git a/tests/qtest/libqos/virtio-9p.c b/tests/qtest/libqos/virtio-9p.c
index 9f099737f9..1bda5403ff 100644
--- a/tests/qtest/libqos/virtio-9p.c
+++ b/tests/qtest/libqos/virtio-9p.c
@@ -62,10 +62,10 @@ static void virtio_9p_device_start_hw(QOSGraphObject *obj)
 static void *virtio_9p_get_driver(QVirtio9P *v_9p,
                                          const char *interface)
 {
-    if (!g_strcmp0(interface, "virtio-9p")) {
+    if (!g_strcmp0(interface, "virtio-9p-synth")) {
         return v_9p;
     }
-    if (!g_strcmp0(interface, "virtio")) {
+    if (!g_strcmp0(interface, "virtio-synth")) {
         return v_9p->vdev;
     }
 
@@ -159,22 +159,23 @@ static void virtio_9p_register_nodes(void)
         .before_cmd_line = "-fsdev synth,id=fsdev0",
     };
 
-    /* virtio-9p-device */
+    /* virtio-9p-device-synth */
     opts.extra_device_opts = str_simple,
-    qos_node_create_driver("virtio-9p-device", virtio_9p_device_create);
-    qos_node_consumes("virtio-9p-device", "virtio-bus", &opts);
-    qos_node_produces("virtio-9p-device", "virtio");
-    qos_node_produces("virtio-9p-device", "virtio-9p");
+    qos_node_create_driver_named("virtio-9p-device-synth", "virtio-9p-device",
+                                 virtio_9p_device_create);
+    qos_node_consumes("virtio-9p-device-synth", "virtio-bus", &opts);
+    qos_node_produces("virtio-9p-device-synth", "virtio-synth");
+    qos_node_produces("virtio-9p-device-synth", "virtio-9p-synth");
 
-    /* virtio-9p-pci */
+    /* virtio-9p-pci-synth */
     opts.extra_device_opts = str_addr;
     add_qpci_address(&opts, &addr);
-    qos_node_create_driver("virtio-9p-pci", virtio_9p_pci_create);
-    qos_node_consumes("virtio-9p-pci", "pci-bus", &opts);
-    qos_node_produces("virtio-9p-pci", "pci-device");
-    qos_node_produces("virtio-9p-pci", "virtio");
-    qos_node_produces("virtio-9p-pci", "virtio-9p");
-
+    qos_node_create_driver_named("virtio-9p-pci-synth", "virtio-9p-pci",
+                                 virtio_9p_pci_create);
+    qos_node_consumes("virtio-9p-pci-synth", "pci-bus", &opts);
+    qos_node_produces("virtio-9p-pci-synth", "pci-device");
+    qos_node_produces("virtio-9p-pci-synth", "virtio-synth");
+    qos_node_produces("virtio-9p-pci-synth", "virtio-9p-synth");
 }
 
 libqos_init(virtio_9p_register_nodes);
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index f7505396f3..d46d675309 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -897,27 +897,27 @@ static void fs_readdir_split_512(void *obj, void *data,
 
 static void register_virtio_9p_test(void)
 {
-    qos_add_test("config", "virtio-9p", pci_config, NULL);
-    qos_add_test("fs/version/basic", "virtio-9p", fs_version, NULL);
-    qos_add_test("fs/attach/basic", "virtio-9p", fs_attach, NULL);
-    qos_add_test("fs/walk/basic", "virtio-9p", fs_walk, NULL);
-    qos_add_test("fs/walk/no_slash", "virtio-9p", fs_walk_no_slash,
+    /* selects the 9pfs 'synth' filesystem driver for the respective test */
+    const char *synth_driver = "virtio-9p-synth";
+
+    qos_add_test("config", synth_driver, pci_config, NULL);
+    qos_add_test("fs/version/basic", synth_driver, fs_version, NULL);
+    qos_add_test("fs/attach/basic", synth_driver, fs_attach, NULL);
+    qos_add_test("fs/walk/basic", synth_driver, fs_walk, NULL);
+    qos_add_test("fs/walk/no_slash", synth_driver, fs_walk_no_slash, NULL);
+    qos_add_test("fs/walk/dotdot_from_root", synth_driver, fs_walk_dotdot,
                  NULL);
-    qos_add_test("fs/walk/dotdot_from_root", "virtio-9p",
-                 fs_walk_dotdot, NULL);
-    qos_add_test("fs/lopen/basic", "virtio-9p", fs_lopen, NULL);
-    qos_add_test("fs/write/basic", "virtio-9p", fs_write, NULL);
-    qos_add_test("fs/flush/success", "virtio-9p", fs_flush_success,
+    qos_add_test("fs/lopen/basic", synth_driver, fs_lopen, NULL);
+    qos_add_test("fs/write/basic", synth_driver, fs_write, NULL);
+    qos_add_test("fs/flush/success", synth_driver, fs_flush_success, NULL);
+    qos_add_test("fs/flush/ignored", synth_driver, fs_flush_ignored, NULL);
+    qos_add_test("fs/readdir/basic", synth_driver, fs_readdir, NULL);
+    qos_add_test("fs/readdir/split_512", synth_driver, fs_readdir_split_512,
                  NULL);
-    qos_add_test("fs/flush/ignored", "virtio-9p", fs_flush_ignored,
+    qos_add_test("fs/readdir/split_256", synth_driver, fs_readdir_split_256,
+                 NULL);
+    qos_add_test("fs/readdir/split_128", synth_driver, fs_readdir_split_128,
                  NULL);
-    qos_add_test("fs/readdir/basic", "virtio-9p", fs_readdir, NULL);
-    qos_add_test("fs/readdir/split_512", "virtio-9p",
-                 fs_readdir_split_512, NULL);
-    qos_add_test("fs/readdir/split_256", "virtio-9p",
-                 fs_readdir_split_256, NULL);
-    qos_add_test("fs/readdir/split_128", "virtio-9p",
-                 fs_readdir_split_128, NULL);
 }
 
 libqos_init(register_virtio_9p_test);
-- 
2.20.1

[PATCH 09/12] tests/9pfs: introduce local tests

[Christian Schoenebeck]

This patch introduces 9pfs test cases using the 9pfs 'local'
filesystem driver which reads/writes/creates/deletes real files
and directories.

In this initial version, there are only 2 local tests which actually
only check if the 9pfs 'local' device was created successfully.

Before the 9pfs 'local' tests are run, a test directory 'qtest-9p-local'
is created (with world rwx permissions) under the current working
directory. At this point that test directory is not auto deleted yet.

A different PCI address, fsdev id and mount tag is used for the 'local'
9pfs device. This is not really necessary right now, but might be useful
to avoid potential colissions in future.

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 tests/qtest/libqos/virtio-9p.c | 101 ++++++++++++++++++++++++++++++++-
 tests/qtest/libqos/virtio-9p.h |   1 +
 tests/qtest/virtio-9p-test.c   |  12 +++-
 3 files changed, 110 insertions(+), 4 deletions(-)

diff --git a/tests/qtest/libqos/virtio-9p.c b/tests/qtest/libqos/virtio-9p.c
index 1bda5403ff..400f4b0113 100644
--- a/tests/qtest/libqos/virtio-9p.c
+++ b/tests/qtest/libqos/virtio-9p.c
@@ -24,6 +24,61 @@
 #include "qgraph.h"
 
 static QGuestAllocator *alloc;
+static char *local_test_path;
+
+/* Concatenates the passed 2 pathes. Returned result must be freed. */
+static char *concat_path(const char* a, const char* b)
+{
+    const int len = strlen(a) + strlen("/") + strlen(b);
+    char *path = g_malloc0(len + 1);
+    snprintf(path, len + 1, "%s/%s", a, b);
+    g_assert(strlen(path) == len);
+    return path;
+}
+
+/*
+ * Lazy sprintf() implementation which auto allocates buffer. Returned result
+ * must be freed.
+ */
+static char *strpr(const char* format, ...)
+{
+    va_list argp;
+
+    va_start(argp, format);
+    const int sz = vsnprintf(NULL, 0, format, argp) + 1;
+    va_end(argp);
+
+    g_assert(sz > 0);
+    char *s = g_malloc0(sz);
+
+    va_start(argp, format);
+    const int len = vsnprintf(s, sz, format, argp);
+    va_end(argp);
+
+    g_assert(len + 1 == sz);
+    return s;
+}
+
+static void init_local_test_path(void)
+{
+    char *pwd = get_current_dir_name();
+    local_test_path = concat_path(pwd, "qtest-9p-local");
+    free(pwd);
+}
+
+/* Creates the directory for the 9pfs 'local' filesystem driver to access. */
+static void create_local_test_dir(void)
+{
+    struct stat st;
+
+    g_assert(local_test_path != NULL);
+    mkdir(local_test_path, 0777);
+
+    /* ensure test directory exists now ... */
+    g_assert(stat(local_test_path, &st) == 0);
+    /* ... and is actually a directory */
+    g_assert((st.st_mode & S_IFMT) == S_IFDIR);
+}
 
 static void virtio_9p_cleanup(QVirtio9P *interface)
 {
@@ -62,10 +117,14 @@ static void virtio_9p_device_start_hw(QOSGraphObject *obj)
 static void *virtio_9p_get_driver(QVirtio9P *v_9p,
                                          const char *interface)
 {
-    if (!g_strcmp0(interface, "virtio-9p-synth")) {
+    if (!g_strcmp0(interface, "virtio-9p-synth") ||
+        !g_strcmp0(interface, "virtio-9p-local"))
+    {
         return v_9p;
     }
-    if (!g_strcmp0(interface, "virtio-synth")) {
+    if (!g_strcmp0(interface, "virtio-synth") ||
+        !g_strcmp0(interface, "virtio-local"))
+    {
         return v_9p->vdev;
     }
 
@@ -148,6 +207,12 @@ static void *virtio_9p_pci_create(void *pci_bus, QGuestAllocator *t_alloc,
 
 static void virtio_9p_register_nodes(void)
 {
+    /* make sure test dir for the 'local' tests exists and is clean */
+    init_local_test_path();
+    create_local_test_dir();
+
+    /* 9pfs device using the 'synth' fs driver */
+
     const char *str_simple = "fsdev=fsdev0,mount_tag=" MOUNT_TAG_SYNTH;
     const char *str_addr = "fsdev=fsdev0,addr=04.0,mount_tag=" MOUNT_TAG_SYNTH;
 
@@ -176,6 +241,38 @@ static void virtio_9p_register_nodes(void)
     qos_node_produces("virtio-9p-pci-synth", "pci-device");
     qos_node_produces("virtio-9p-pci-synth", "virtio-synth");
     qos_node_produces("virtio-9p-pci-synth", "virtio-9p-synth");
+
+
+    /* 9pfs device using the 'local' fs driver */
+
+    const char *local_str_simple = "fsdev=fsdev1,mount_tag=" MOUNT_TAG_LOCAL;
+    const char *local_str_addr = "fsdev=fsdev1,addr=04.1,mount_tag="
+                                 MOUNT_TAG_LOCAL;
+
+    addr.devfn = QPCI_DEVFN(4, 1),
+
+    opts.before_cmd_line = strpr(
+        "-fsdev local,id=fsdev1,path='%s',security_model=mapped-xattr",
+        local_test_path
+    );
+
+    /* virtio-9p-device-local */
+    opts.extra_device_opts = local_str_simple,
+    qos_node_create_driver_named("virtio-9p-device-local", "virtio-9p-device",
+                                 virtio_9p_device_create);
+    qos_node_consumes("virtio-9p-device-local", "virtio-bus", &opts);
+    qos_node_produces("virtio-9p-device-local", "virtio-local");
+    qos_node_produces("virtio-9p-device-local", "virtio-9p-local");
+
+    /* virtio-9p-pci-local */
+    opts.extra_device_opts = local_str_addr;
+    add_qpci_address(&opts, &addr);
+    qos_node_create_driver_named("virtio-9p-pci-local", "virtio-9p-pci",
+                                 virtio_9p_pci_create);
+    qos_node_consumes("virtio-9p-pci-local", "pci-bus", &opts);
+    qos_node_produces("virtio-9p-pci-local", "pci-device");
+    qos_node_produces("virtio-9p-pci-local", "virtio-local");
+    qos_node_produces("virtio-9p-pci-local", "virtio-9p-local");
 }
 
 libqos_init(virtio_9p_register_nodes);
diff --git a/tests/qtest/libqos/virtio-9p.h b/tests/qtest/libqos/virtio-9p.h
index d9a815083f..20d1fc6270 100644
--- a/tests/qtest/libqos/virtio-9p.h
+++ b/tests/qtest/libqos/virtio-9p.h
@@ -28,6 +28,7 @@ typedef struct QVirtio9PPCI QVirtio9PPCI;
 typedef struct QVirtio9PDevice QVirtio9PDevice;
 
 #define MOUNT_TAG_SYNTH "qtest-synth"
+#define MOUNT_TAG_LOCAL "qtest-local"
 
 struct QVirtio9P {
     QVirtioDevice *vdev;
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index d46d675309..88451f255f 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -26,13 +26,15 @@ static void pci_config(void *obj, void *data, QGuestAllocator *t_alloc)
     char *tag;
     int i;
 
-    g_assert_cmpint(tag_len, ==, strlen(MOUNT_TAG_SYNTH));
+    g_assert(tag_len == strlen(MOUNT_TAG_SYNTH) ||
+             tag_len == strlen(MOUNT_TAG_LOCAL));
 
     tag = g_malloc(tag_len);
     for (i = 0; i < tag_len; i++) {
         tag[i] = qvirtio_config_readb(v9p->vdev, i + 2);
     }
-    g_assert_cmpmem(tag, tag_len, MOUNT_TAG_SYNTH, tag_len);
+    g_assert(strncmp(tag, MOUNT_TAG_SYNTH, tag_len) == 0 ||
+             strncmp(tag, MOUNT_TAG_LOCAL, tag_len) == 0);
     g_free(tag);
 }
 
@@ -918,6 +920,12 @@ static void register_virtio_9p_test(void)
                  NULL);
     qos_add_test("fs/readdir/split_128", synth_driver, fs_readdir_split_128,
                  NULL);
+
+
+    /* selects the 9pfs 'local' filesystem driver for the respective test */
+    const char *local_driver = "virtio-9p-local";
+
+    qos_add_test("config", local_driver, pci_config, NULL);
 }
 
 libqos_init(register_virtio_9p_test);
-- 
2.20.1

[PATCH 10/12] tests/9pfs: wipe local 9pfs test directory

[Christian Schoenebeck]

Before running the first 9pfs test case, make sure the test directory
for running the 9pfs 'local' tests on is entirely empty. For that
reason simply delete the test directory (if any) before (re)creating
it on test suite startup.

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 tests/qtest/libqos/virtio-9p.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/tests/qtest/libqos/virtio-9p.c b/tests/qtest/libqos/virtio-9p.c
index 400f4b0113..b44fc4ad63 100644
--- a/tests/qtest/libqos/virtio-9p.c
+++ b/tests/qtest/libqos/virtio-9p.c
@@ -80,6 +80,18 @@ static void create_local_test_dir(void)
     g_assert((st.st_mode & S_IFMT) == S_IFDIR);
 }
 
+/* Deletes directory previously created by create_local_test_dir(). */
+static void remove_local_test_dir(void)
+{
+    g_assert(local_test_path != NULL);
+    char *cmd = strpr("rm -r '%s'\n", local_test_path);
+    int res = system(cmd);
+    if (res < 0) {
+        /* ignore error, dummy check to prevent compiler error */
+    }
+    g_free(cmd);
+}
+
 static void virtio_9p_cleanup(QVirtio9P *interface)
 {
     qvirtqueue_cleanup(interface->vdev->bus, interface->vq, alloc);
@@ -209,6 +221,7 @@ static void virtio_9p_register_nodes(void)
 {
     /* make sure test dir for the 'local' tests exists and is clean */
     init_local_test_path();
+    remove_local_test_dir();
     create_local_test_dir();
 
     /* 9pfs device using the 'synth' fs driver */
-- 
2.20.1

[PATCH 11/12] tests/9pfs: add virtio_9p_test_path()

[Christian Schoenebeck]

This new public function virtio_9p_test_path() allows 9pfs
'local' tests to translate a path from guest scope to host
scope. For instance by passing an empty string it would
return the root path on host of the exported 9pfs tree.

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 tests/qtest/libqos/virtio-9p.c | 6 ++++++
 tests/qtest/libqos/virtio-9p.h | 5 +++++
 2 files changed, 11 insertions(+)

diff --git a/tests/qtest/libqos/virtio-9p.c b/tests/qtest/libqos/virtio-9p.c
index b44fc4ad63..599b73a9d7 100644
--- a/tests/qtest/libqos/virtio-9p.c
+++ b/tests/qtest/libqos/virtio-9p.c
@@ -92,6 +92,12 @@ static void remove_local_test_dir(void)
     g_free(cmd);
 }
 
+char *virtio_9p_test_path(const char *path)
+{
+    g_assert(local_test_path);
+    return concat_path(local_test_path, path);
+}
+
 static void virtio_9p_cleanup(QVirtio9P *interface)
 {
     qvirtqueue_cleanup(interface->vdev->bus, interface->vq, alloc);
diff --git a/tests/qtest/libqos/virtio-9p.h b/tests/qtest/libqos/virtio-9p.h
index 20d1fc6270..052882ef7c 100644
--- a/tests/qtest/libqos/virtio-9p.h
+++ b/tests/qtest/libqos/virtio-9p.h
@@ -45,4 +45,9 @@ struct QVirtio9PDevice {
     QVirtio9P v9p;
 };
 
+/**
+ * Returns path on host to the passed guest path. Result must be freed.
+ */
+char *virtio_9p_test_path(const char *path);
+
 #endif
-- 
2.20.1

[PATCH 12/12] tests/9pfs: add local Tmkdir test

[Christian Schoenebeck]

This test case uses the 9pfs 'local' driver to create a directory
and then checks if the expected directory was actually created on
host side.

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 tests/qtest/virtio-9p-test.c | 140 +++++++++++++++++++++++++++++++++++
 1 file changed, 140 insertions(+)

diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index 88451f255f..c45c706d4f 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -18,6 +18,62 @@
 #define QVIRTIO_9P_TIMEOUT_US (10 * 1000 * 1000)
 static QGuestAllocator *alloc;
 
+/*
+ * Used to auto generate new fids. Start with arbitrary high value to avoid
+ * collision with hard coded fids in basic test code.
+ */
+static uint32_t fid_generator = 1000;
+
+static uint32_t genfid(void)
+{
+    return fid_generator++;
+}
+
+/**
+ * Splits the @a in string by @a delim into individual (non empty) strings
+ * and outputs them to @a out. The output array @a out is NULL terminated.
+ *
+ * Output array @a out must be freed by calling split_free().
+ *
+ * @returns number of individual elements in output array @a out (without the
+ *          final NULL terminating element)
+ */
+static int split(const char *in, const char *delim, char ***out)
+{
+    int n = 0, i = 0;
+    char *tmp, *p;
+
+    tmp = g_strdup(in);
+    for (p = strtok(tmp, delim); p != NULL; p = strtok(NULL, delim)) {
+        if (strlen(p) > 0) {
+            ++n;
+        }
+    }
+    g_free(tmp);
+
+    *out = g_malloc0(n * sizeof(char *) + 1); /* last element NULL delimiter */
+
+    tmp = g_strdup(in);
+    for (p = strtok(tmp, delim); p != NULL; p = strtok(NULL, delim)) {
+        if (strlen(p) > 0) {
+            (*out)[i++] = g_strdup(p);
+        }
+    }
+    g_free(tmp);
+
+    return n;
+}
+
+static void split_free(char ***out)
+{
+    int i;
+    for (i = 0; (*out)[i]; ++i) {
+        g_free((*out)[i]);
+    }
+    g_free(*out);
+    *out = NULL;
+}
+
 static void pci_config(void *obj, void *data, QGuestAllocator *t_alloc)
 {
     QVirtio9P *v9p = obj;
@@ -203,6 +259,7 @@ static const char *rmessage_name(uint8_t id)
         id == P9_RWALK ? "RWALK" :
         id == P9_RLOPEN ? "RLOPEN" :
         id == P9_RWRITE ? "RWRITE" :
+        id == P9_RMKDIR ? "RMKDIR" :
         id == P9_RFLUSH ? "RFLUSH" :
         id == P9_RREADDIR ? "READDIR" :
         "<unknown>";
@@ -580,6 +637,39 @@ static bool fs_dirents_contain_name(struct V9fsDirent *e, const char* name)
     return false;
 }
 
+/* size[4] Tmkdir tag[2] dfid[4] name[s] mode[4] gid[4] */
+static P9Req *v9fs_tmkdir(QVirtio9P *v9p, uint32_t dfid, const char *name,
+                          uint32_t mode, uint32_t gid, uint16_t tag)
+{
+    P9Req *req;
+
+    uint32_t body_size = 4 + 4 + 4;
+    uint16_t string_size = v9fs_string_size(name);
+
+    g_assert_cmpint(body_size, <=, UINT32_MAX - string_size);
+    body_size += string_size;
+
+    req = v9fs_req_init(v9p, body_size, P9_TMKDIR, tag);
+    v9fs_uint32_write(req, dfid);
+    v9fs_string_write(req, name);
+    v9fs_uint32_write(req, mode);
+    v9fs_uint32_write(req, gid);
+    v9fs_req_send(req);
+    return req;
+}
+
+/* size[4] Rmkdir tag[2] qid[13] */
+static void v9fs_rmkdir(P9Req *req, v9fs_qid *qid)
+{
+    v9fs_req_recv(req, P9_RMKDIR);
+    if (qid) {
+        v9fs_memread(req, qid, 13);
+    } else {
+        v9fs_memskip(req, 13);
+    }
+    v9fs_req_free(req);
+}
+
 /* basic readdir test where reply fits into a single response message */
 static void fs_readdir(void *obj, void *data, QGuestAllocator *t_alloc)
 {
@@ -879,6 +969,30 @@ static void fs_flush_ignored(void *obj, void *data, QGuestAllocator *t_alloc)
     g_free(wnames[0]);
 }
 
+static void fs_mkdir(void *obj, void *data, QGuestAllocator *t_alloc,
+                     const char *path, const char *cname)
+{
+    QVirtio9P *v9p = obj;
+    alloc = t_alloc;
+    char **wnames;
+    char *const name = g_strdup(cname);
+    P9Req *req;
+    const uint32_t fid = genfid();
+
+    int nwnames = split(path, "/", &wnames);
+
+    req = v9fs_twalk(v9p, 0, fid, nwnames, wnames, 0);
+    v9fs_req_wait_for_reply(req, NULL);
+    v9fs_rwalk(req, NULL, NULL);
+
+    req = v9fs_tmkdir(v9p, fid, name, 0750, 0, 0);
+    v9fs_req_wait_for_reply(req, NULL);
+    v9fs_rmkdir(req, NULL);
+
+    g_free(name);
+    split_free(&wnames);
+}
+
 static void fs_readdir_split_128(void *obj, void *data,
                                  QGuestAllocator *t_alloc)
 {
@@ -897,6 +1011,31 @@ static void fs_readdir_split_512(void *obj, void *data,
     fs_readdir_split(obj, data, t_alloc, 512);
 }
 
+
+/* tests using the 9pfs 'local' fs driver */
+
+static void fs_create_dir(void *obj, void *data, QGuestAllocator *t_alloc)
+{
+    QVirtio9P *v9p = obj;
+    struct stat st;
+    char *root_path = virtio_9p_test_path("");
+    char *new_dir = virtio_9p_test_path("01");
+
+    g_assert(root_path != NULL);
+
+    fs_attach(v9p, NULL, t_alloc);
+    fs_mkdir(v9p, data, t_alloc, "/", "01");
+
+    /* check if created directory really exists now ... */
+    g_assert(stat(new_dir, &st) == 0);
+    /* ... and is actually a directory */
+    g_assert((st.st_mode & S_IFMT) == S_IFDIR);
+
+    g_free(new_dir);
+    g_free(root_path);
+}
+
+
 static void register_virtio_9p_test(void)
 {
     /* selects the 9pfs 'synth' filesystem driver for the respective test */
@@ -926,6 +1065,7 @@ static void register_virtio_9p_test(void)
     const char *local_driver = "virtio-9p-local";
 
     qos_add_test("config", local_driver, pci_config, NULL);
+    qos_add_test("fs/create_dir", local_driver, fs_create_dir, NULL);
 }
 
 libqos_init(register_virtio_9p_test);
-- 
2.20.1

[PATCH 00/12] 9pfs: add tests using local fs driver

[Christian Schoenebeck]

The currently existing 9pfs test cases are all solely using the 9pfs 'synth'
fileystem driver, which is a very simple and purely simulated (in RAM only)
filesystem. There are issues though where the 'synth' fs driver is not
sufficient. For example the following two bugs need test cases running the
9pfs 'local' fs driver:

https://bugs.launchpad.net/qemu/+bug/1336794
https://bugs.launchpad.net/qemu/+bug/1877384

This patch set for that reason introduces 9pfs test cases using the 9pfs
'local' filesystem driver along to the already existing tests on 'synth'.
It consists of 3 parts:

1. Mandatory qos patches 1 and 2 lay the ground by removing a limitation of
   the qtest/qos subsystem: support for more than one device using the same
   (official) QEMU device name.

2. Optional qos patches 3 to 6 were used for debugging the qtest framework.
   I found them very useful to get insight how the qos graph is built, how
   the generated QEMU commands looks like, and what environemnt variables are
   already available in qtests. I thought they might helpful for other people
   as well, either in suggested or some modified form. Especially as it's not
   obvious sometimes why certain tests are simply ignored by the qtest
   framework (e.g. because of a missing link in qos path from test node to
   qos root node, or certain devices been auto marked as 'unavailable' after
   QMP negotiation with QEMU). I introduced several new QTEST_* environment
   variables for the individual debugging aspects; maybe they could simply
   use one and the same variable like QTEST_DEBUG instead.
   Note: patch 3 uses coloured output to mark the individual graph nodes as
   either available or unavailable. It does not check for tty color support
   right now. I'm not sure if there is already some QEMU util function that
   could be used for that check.

3. Patches 7 to 12 actually introduce 9pfs 'local' test cases using the qtest
   framework. They only work in conjunction with qos patches 1 and 2. These
   'local' tests are adding a test directory 'qtest-9p-local' inside the
   current working directory (using get_current_dir()), which is typically the
   build directory, before running the tests. That test directory is
   automatically recreated next time the test suite is run again, to ensure
   the 9pfs 'local' tests always run consistently on a clean test directory.
   The test directory is used by the 'local' driver as root of its export
   path. So it will add/read/write/delete real files and directories inside
   that test directory.
   Note: I was adding a bunch of basic util functions like split(),
   concat_path() and strpr(). I am not sure if there are already public QEMU
   util functions that I could instead of them.

Christian Schoenebeck (12):
  tests/qtest/qgraph: add qemu_name to QOSGraphNode
  tests/qtest/qgraph: add qos_node_create_driver_named()
  tests/qtest/qos: add qos_dump_graph()
  tests/qtest/qos-test: new QTEST_DUMP_GRAPH environment variable
  tests/qtest/qos-test: add QTEST_DUMP_ENV environment variable
  tests/qtest/qos-test: add environment variable QTEST_DEBUG
  test/9pfs: change export tag name to qtest-synth
  tests/9pfs: refactor test names and test devices
  tests/9pfs: introduce local tests
  tests/9pfs: wipe local 9pfs test directory
  tests/9pfs: add virtio_9p_test_path()
  tests/9pfs: add local Tmkdir test

 tests/qtest/libqos/qgraph.c          | 108 ++++++++++++++-
 tests/qtest/libqos/qgraph.h          |  36 +++++
 tests/qtest/libqos/qgraph_internal.h |   1 +
 tests/qtest/libqos/virtio-9p.c       | 147 ++++++++++++++++++---
 tests/qtest/libqos/virtio-9p.h       |   8 +-
 tests/qtest/qos-test.c               |  15 ++-
 tests/qtest/virtio-9p-test.c         | 188 ++++++++++++++++++++++++---
 7 files changed, 463 insertions(+), 40 deletions(-)

-- 
2.20.1

Re: [PATCH 06/12] tests/qtest/qos-test: add environment variable QTEST_DEBUG

[Paolo Bonzini]

On 27/09/20 12:40, Christian Schoenebeck wrote:
> For now this new environment variable QTEST_DEBUG will cause the
> assembled qemu command line to be printed before running each test.
> 
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---
>  tests/qtest/qos-test.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tests/qtest/qos-test.c b/tests/qtest/qos-test.c
> index 4b1a1922fc..571d1e140a 100644
> --- a/tests/qtest/qos-test.c
> +++ b/tests/qtest/qos-test.c
> @@ -89,6 +89,9 @@ static void qos_set_machines_devices_available(void)
>  
>  static void restart_qemu_or_continue(char *path)
>  {
> +    if (getenv("QTEST_DEBUG") != NULL) {
> +        printf("run QEMU with: '%s'\n", path);
> +    }
>      /* compares the current command line with the
>       * one previously executed: if they are the same,
>       * don't restart QEMU, if they differ, stop previous
> 

You can just use g_test_message for this and do it unconditionally.

Paolo

Re: [PATCH 08/12] tests/9pfs: refactor test names and test devices

[Paolo Bonzini]

On 27/09/20 12:40, Christian Schoenebeck wrote:
> +    qos_node_produces("virtio-9p-device-synth", "virtio-synth");

This is wrong, since it disables the generic virtio device tests (right
now there is only one in virtio-test.c).

Paolo

Re: [PATCH 08/12] tests/9pfs: refactor test names and test devices

[Paolo Bonzini]

On 27/09/20 12:40, Christian Schoenebeck wrote:
> +    qos_node_consumes("virtio-9p-device-local", "virtio-bus", &opts);
> +    qos_node_produces("virtio-9p-device-local", "virtio-local");

This should produce "virtio", similar to what I remarked in the previous
patch.

> +    qos_node_produces("virtio-9p-device-local", "virtio-9p-local");
> +
> +    /* virtio-9p-pci-local */
> +    opts.extra_device_opts = local_str_addr;
> +    add_qpci_address(&opts, &addr);
> +    qos_node_create_driver_named("virtio-9p-pci-local", "virtio-9p-pci",
> +                                 virtio_9p_pci_create);
> +    qos_node_consumes("virtio-9p-pci-local", "pci-bus", &opts);
> +    qos_node_produces("virtio-9p-pci-local", "pci-device");
> +    qos_node_produces("virtio-9p-pci-local", "virtio-local");
> +    qos_node_produces("virtio-9p-pci-local", "virtio-9p-local");
>  }


The implementation in patches 1 and 2 is reasonable, but what is the
advantage of this as opposed to specifying the fsdev in the edge options
for the test (similar to virtio-net)?  I was expecting both
virtio-9p-device-synth and virtio-9p-device-local to produce virtio-9p,
so that the existing tests would be reused automatically by the qos
graph walk.

As things stand, I don't see any reason to have separate devices for
different backends.

Paolo

Re: [PATCH 08/12] tests/9pfs: refactor test names and test devices

[Christian Schoenebeck]

On Montag, 28. September 2020 10:37:52 CEST Paolo Bonzini wrote:
> On 27/09/20 12:40, Christian Schoenebeck wrote:
> > +    qos_node_consumes("virtio-9p-device-local", "virtio-bus", &opts);
> > +    qos_node_produces("virtio-9p-device-local", "virtio-local");
> 
> This should produce "virtio", similar to what I remarked in the previous
> patch.

You're right, I missed that implied virtio test. That should be in the end:

index 599b73a9d7..1df3da18e9 100644
--- a/tests/qtest/libqos/virtio-9p.c
+++ b/tests/qtest/libqos/virtio-9p.c
@@ -140,8 +140,7 @@ static void *virtio_9p_get_driver(QVirtio9P *v_9p,
     {
         return v_9p;
     }
-    if (!g_strcmp0(interface, "virtio-synth") ||
-        !g_strcmp0(interface, "virtio-local"))
+    if (!g_strcmp0(interface, "virtio"))
     {
         return v_9p->vdev;
     }
@@ -248,7 +247,7 @@ static void virtio_9p_register_nodes(void)
     qos_node_create_driver_named("virtio-9p-device-synth", "virtio-9p-
device",
                                  virtio_9p_device_create);
     qos_node_consumes("virtio-9p-device-synth", "virtio-bus", &opts);
-    qos_node_produces("virtio-9p-device-synth", "virtio-synth");
+    qos_node_produces("virtio-9p-device-synth", "virtio");
     qos_node_produces("virtio-9p-device-synth", "virtio-9p-synth");
 
     /* virtio-9p-pci-synth */
@@ -258,7 +257,7 @@ static void virtio_9p_register_nodes(void)
                                  virtio_9p_pci_create);
     qos_node_consumes("virtio-9p-pci-synth", "pci-bus", &opts);
     qos_node_produces("virtio-9p-pci-synth", "pci-device");
-    qos_node_produces("virtio-9p-pci-synth", "virtio-synth");
+    qos_node_produces("virtio-9p-pci-synth", "virtio");
     qos_node_produces("virtio-9p-pci-synth", "virtio-9p-synth");
 
 
@@ -280,7 +279,7 @@ static void virtio_9p_register_nodes(void)
     qos_node_create_driver_named("virtio-9p-device-local", "virtio-9p-
device",
                                  virtio_9p_device_create);
     qos_node_consumes("virtio-9p-device-local", "virtio-bus", &opts);
-    qos_node_produces("virtio-9p-device-local", "virtio-local");
+    qos_node_produces("virtio-9p-device-local", "virtio");
     qos_node_produces("virtio-9p-device-local", "virtio-9p-local");
 
     /* virtio-9p-pci-local */
@@ -290,7 +289,7 @@ static void virtio_9p_register_nodes(void)
                                  virtio_9p_pci_create);
     qos_node_consumes("virtio-9p-pci-local", "pci-bus", &opts);
     qos_node_produces("virtio-9p-pci-local", "pci-device");
-    qos_node_produces("virtio-9p-pci-local", "virtio-local");
+    qos_node_produces("virtio-9p-pci-local", "virtio");
     qos_node_produces("virtio-9p-pci-local", "virtio-9p-local");
 }

I already tested that. Both the implied "virtio" test and the 9p 'synth' and 
'local' tests work as expected then.

> > +    qos_node_produces("virtio-9p-device-local", "virtio-9p-local");
> > +
> > +    /* virtio-9p-pci-local */
> > +    opts.extra_device_opts = local_str_addr;
> > +    add_qpci_address(&opts, &addr);
> > +    qos_node_create_driver_named("virtio-9p-pci-local", "virtio-9p-pci",
> > +                                 virtio_9p_pci_create);
> > +    qos_node_consumes("virtio-9p-pci-local", "pci-bus", &opts);
> > +    qos_node_produces("virtio-9p-pci-local", "pci-device");
> > +    qos_node_produces("virtio-9p-pci-local", "virtio-local");
> > +    qos_node_produces("virtio-9p-pci-local", "virtio-9p-local");
> > 
> >  }
> 
> The implementation in patches 1 and 2 is reasonable, but what is the
> advantage of this as opposed to specifying the fsdev in the edge options
> for the test (similar to virtio-net)?  I was expecting both
> virtio-9p-device-synth and virtio-9p-device-local to produce virtio-9p,
> so that the existing tests would be reused automatically by the qos
> graph walk.
> 
> As things stand, I don't see any reason to have separate devices for
> different backends.

I thought to fix the problem at its root, by removing that singular device 
limitation in qos. That would also allow to cleanly separate tests suites that 
are not related to each other instead of having to depend on each other, 
taking care about other one's command line skeleton and more.

Plus I just tried what you suggested as alternative:

-Subproject commit 85e5d839847af54efab170f2b1331b2a6421e647
+Subproject commit 88f18909db731a627456f26d779445f84e449536
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index c45c706d4f..7f588a9a92 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -1035,12 +1035,22 @@ static void fs_create_dir(void *obj, void *data, 
QGuestAllocator *t_alloc)
     g_free(root_path);
 }
 
+static void *modifycmdline(GString *cmd_line, void *arg)
+{
+    fprintf(stderr, "\n\nbefore modifycmdline(): '%s'\n\n", cmd_line->str);
+    g_string_append(cmd_line, "MODIFYCMDLINE ");
+    return arg;
+}
 
 static void register_virtio_9p_test(void)
 {
     /* selects the 9pfs 'synth' filesystem driver for the respective test */
     const char *synth_driver = "virtio-9p-synth";
 
+    QOSGraphTestOptions opts = {
+        //.before = modifycmdline,
+    };
+
     qos_add_test("config", synth_driver, pci_config, NULL);
     qos_add_test("fs/version/basic", synth_driver, fs_version, NULL);
     qos_add_test("fs/attach/basic", synth_driver, fs_attach, NULL);
@@ -1064,8 +1074,13 @@ static void register_virtio_9p_test(void)
     /* selects the 9pfs 'local' filesystem driver for the respective test */
     const char *local_driver = "virtio-9p-local";
 
-    qos_add_test("config", local_driver, pci_config, NULL);
-    qos_add_test("fs/create_dir", local_driver, fs_create_dir, NULL);
+    opts.before = modifycmdline,
+    opts.edge.extra_device_opts = "EXTRADEVICEOPTS";
+    opts.edge.before_cmd_line = "BEFORECMDLINE";
+    opts.edge.after_cmd_line = "AFTERCMDLINE";
+
+    qos_add_test("config", local_driver, pci_config, &opts);
+    qos_add_test("fs/create_dir", local_driver, fs_create_dir, &opts);
 }
 

Output:


QTEST_DEBUG=1 tests/qtest/qos-test
...

/x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci-local/virtio-9p-
local/virtio-9p-local-tests/config: 

before modifycmdline(): '-M pc  -fsdev local,id=fsdev1,path='/home/me/git/
qemu/build/qtest-9p-local',security_model=mapped-xattr -device virtio-9p-
pci,fsdev=fsdev1,addr=04.1,mount_tag=qtest-local BEFORECMDLINE,EXTRADEVICEOPTS 
AFTERCMDLINE'

run QEMU with: '-M pc  -fsdev local,id=fsdev1,path='/home/me/git/qemu/build/
qtest-9p-local',security_model=mapped-xattr -device virtio-9p-
pci,fsdev=fsdev1,addr=04.1,mount_tag=qtest-local BEFORECMDLINE,EXTRADEVICEOPTS 
AFTERCMDLINEMODIFYCMDLINE '


So your suggested solution is fine for appending extra arguments past the 
command line. However I would not be able to prepend something (easily) in 
front of '-device virtio-9p-pci'.

So I would be forced to parse the existing command line in modifycmdline() 
callback and then insert the required arguments appropriately. I would not 
find that very clean.

I mean yes, you might do hacks for making this patch set work without qos 
patches 1 and 2, however somewhere in future you are probably back at square 
one when facing this single device limitation again. So I still think this qos 
change (patch 1 & 2) make sense.

Best regards,
Christian Schoenebeck

Re: [PATCH 06/12] tests/qtest/qos-test: add environment variable QTEST_DEBUG

[Christian Schoenebeck]

On Montag, 28. September 2020 10:31:21 CEST Paolo Bonzini wrote:
> On 27/09/20 12:40, Christian Schoenebeck wrote:
> > For now this new environment variable QTEST_DEBUG will cause the
> > assembled qemu command line to be printed before running each test.
> > 
> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > ---
> > 
> >  tests/qtest/qos-test.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/tests/qtest/qos-test.c b/tests/qtest/qos-test.c
> > index 4b1a1922fc..571d1e140a 100644
> > --- a/tests/qtest/qos-test.c
> > +++ b/tests/qtest/qos-test.c
> > @@ -89,6 +89,9 @@ static void qos_set_machines_devices_available(void)
> > 
> >  static void restart_qemu_or_continue(char *path)
> >  {
> > 
> > +    if (getenv("QTEST_DEBUG") != NULL) {
> > +        printf("run QEMU with: '%s'\n", path);
> > +    }
> > 
> >      /* compares the current command line with the
> >      
> >       * one previously executed: if they are the same,
> >       * don't restart QEMU, if they differ, stop previous
> 
> You can just use g_test_message for this and do it unconditionally.

I actually had to read the glib sources to understand how g_test_message()'s 
output is turned on:  by appending '--verbose' to the CL.

Okay then.

Best regards,
Christian Schoenebeck

Re: [PATCH 08/12] tests/9pfs: refactor test names and test devices

[Paolo Bonzini]

On 28/09/20 13:56, Christian Schoenebeck wrote:
>> The implementation in patches 1 and 2 is reasonable, but what is the
>> advantage of this as opposed to specifying the fsdev in the edge options
>> for the test (similar to virtio-net)?  I was expecting both
>> virtio-9p-device-synth and virtio-9p-device-local to produce virtio-9p,
>> so that the existing tests would be reused automatically by the qos
>> graph walk.
>>
>> As things stand, I don't see any reason to have separate devices for
>> different backends.
> 
> I thought to fix the problem at its root, by removing that singular device 
> limitation in qos. That would also allow to cleanly separate tests suites that 
> are not related to each other instead of having to depend on each other, 
> taking care about other one's command line skeleton and more.

As I said, the first two patches make total sense.  They would be useful
for testing both packed and split virtqueues, for example.  However, I
think the (useful) feature is being misused here.

> So your suggested solution is fine for appending extra arguments past the 
> command line. However I would not be able to prepend something (easily) in 
> front of '-device virtio-9p-pci'.
> 
> So I would be forced to parse the existing command line in modifycmdline() 
> callback and then insert the required arguments appropriately. I would not 
> find that very clean.

IIRC -fsdev can be added to the end of the command line and it still
works.  But if you think that is ugly, you can also use g_string_prepend.

Also, looking at future plans for qgraph, adding a generic "plug/socket"
mechanism to QOSGraph was an idea that we couldn't do in time for GSoC.
With that model, virtio-9p would provide a "socket" of type fsdev and
the tests would have to provide a "plug" of the same type.  Likewise
there would be sockets of type disk or network.  QOSGraphEdgeOpts fits
better with that plan, compared to duplicating the devices.

Paolo

Re: [PATCH 08/12] tests/9pfs: refactor test names and test devices

[Christian Schoenebeck]

On Montag, 28. September 2020 14:42:48 CEST Paolo Bonzini wrote:
> On 28/09/20 13:56, Christian Schoenebeck wrote:
> >> The implementation in patches 1 and 2 is reasonable, but what is the
> >> advantage of this as opposed to specifying the fsdev in the edge options
> >> for the test (similar to virtio-net)?  I was expecting both
> >> virtio-9p-device-synth and virtio-9p-device-local to produce virtio-9p,
> >> so that the existing tests would be reused automatically by the qos
> >> graph walk.
> >> 
> >> As things stand, I don't see any reason to have separate devices for
> >> different backends.
> > 
> > I thought to fix the problem at its root, by removing that singular device
> > limitation in qos. That would also allow to cleanly separate tests suites
> > that are not related to each other instead of having to depend on each
> > other, taking care about other one's command line skeleton and more.
> 
> As I said, the first two patches make total sense.  They would be useful
> for testing both packed and split virtqueues, for example.  However, I
> think the (useful) feature is being misused here.

I haven't understood why my suggested mult-device use case imposes a misusage, 
but okay, unless I hear different opinions, I'll prepare a v2 with that (IMO 
hackish) CL fiddling instead in couple days or so.

@Greg: If that's the way to go, then I probably change the test names, e.g.

	"fs/version/basic" -> "synth/version/basic"
	...
	"fs/create_dir" -> "local/create_dir"

to be able to easily distinguish 'synth' driver tests from 'local' driver 
tests, as they would then popup with the same device name in v2, unlike in 
this v1 where they have separate device names.

> > So your suggested solution is fine for appending extra arguments past the
> > command line. However I would not be able to prepend something (easily) in
> > front of '-device virtio-9p-pci'.
> > 
> > So I would be forced to parse the existing command line in modifycmdline()
> > callback and then insert the required arguments appropriately. I would not
> > find that very clean.
> 
> IIRC -fsdev can be added to the end of the command line and it still
> works.  But if you think that is ugly, you can also use g_string_prepend.

Yes, "-fsdev ..." and "-device ..." sequence could theoretically be flipped, 
qemu would accept it that is.

> Also, looking at future plans for qgraph, adding a generic "plug/socket"
> mechanism to QOSGraph was an idea that we couldn't do in time for GSoC.
> With that model, virtio-9p would provide a "socket" of type fsdev and
> the tests would have to provide a "plug" of the same type.  Likewise
> there would be sockets of type disk or network.  QOSGraphEdgeOpts fits
> better with that plan, compared to duplicating the devices.

Sounds like that would require huge changes for all existing qtests on initial 
thought at least.

I find the suggested multi-device approach much simpler, as it does not 
require changes to existing tests, nor people having to get used to a new 
qtest model.

Best regards,
Christian Schoenebeck

Re: [PATCH 08/12] tests/9pfs: refactor test names and test devices

[Paolo Bonzini]

On 28/09/20 15:35, Christian Schoenebeck wrote:
>> As I said, the first two patches make total sense.  They would be useful
>> for testing both packed and split virtqueues, for example.  However, I
>> think the (useful) feature is being misused here.
> 
> I haven't understood why my suggested mult-device use case imposes a misusage, 
> but okay, unless I hear different opinions, I'll prepare a v2 with that (IMO 
> hackish) CL fiddling instead in couple days or so.

Because in my opinion the backend in use is a property of the test
rather than a property of the device.

> @Greg: If that's the way to go, then I probably change the test names, e.g.
> 
> 	"fs/version/basic" -> "synth/version/basic"
> 	...
> 	"fs/create_dir" -> "local/create_dir"
> 
> to be able to easily distinguish 'synth' driver tests from 'local' driver 
> tests, as they would then popup with the same device name in v2, unlike in 
> this v1 where they have separate device names.

Right.

>> Also, looking at future plans for qgraph, adding a generic "plug/socket"
>> mechanism to QOSGraph was an idea that we couldn't do in time for GSoC.
>> With that model, virtio-9p would provide a "socket" of type fsdev and
>> the tests would have to provide a "plug" of the same type.  Likewise
>> there would be sockets of type disk or network.  QOSGraphEdgeOpts fits
>> better with that plan, compared to duplicating the devices.
> 
> Sounds like that would require huge changes for all existing qtests on initial 
> thought at least.

Not huge, but yeah many tests would require changes.

Paolo

Re: [PATCH 08/12] tests/9pfs: refactor test names and test devices

[Christian Schoenebeck]

On Montag, 28. September 2020 18:38:00 CEST Paolo Bonzini wrote:
> On 28/09/20 15:35, Christian Schoenebeck wrote:
> >> As I said, the first two patches make total sense.  They would be useful
> >> for testing both packed and split virtqueues, for example.  However, I
> >> think the (useful) feature is being misused here.
> > 
> > I haven't understood why my suggested mult-device use case imposes a
> > misusage, but okay, unless I hear different opinions, I'll prepare a v2
> > with that (IMO hackish) CL fiddling instead in couple days or so.
> 
> Because in my opinion the backend in use is a property of the test
> rather than a property of the device.

Paolo, I'm back at square one after changing to single-device model as you 
suggested:

GTest: run: /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/pci-
device/pci-device-tests/nop
Run QEMU with: '-M pc  -device virtio-9p-pci'
(MSG: starting QEMU: exec x86_64-softmmu/qemu-system-x86_64 -qtest unix:/tmp/
qtest-18032.sock -qtest-log /dev/null -chardev socket,path=/tmp/
qtest-18032.qmp,id=char0 -mon chardev=char0,mode=control -display none -M pc  
-device virtio-9p-pci -accel qtest)
qemu-system-x86_64: -device virtio-9p-pci: 9pfs device couldn't find fsdev 
with the id = NULL
Broken pipe

This fundamental virtio-9p-pci test obviously needs a complete 9p command 
line, that is either a 'synth' driver one, or a 'local' one. But simply either 
picking one or another is inappropriate here. This test should run once for 
'synth' and once for 'local'.

Still not convinced that the multi-device route is the way to go?

Best regards,
Christian Schoenebeck

Re: [PATCH 08/12] tests/9pfs: refactor test names and test devices

[Paolo Bonzini]

On 01/10/20 13:34, Christian Schoenebeck wrote:
> Paolo, I'm back at square one after changing to single-device model as you 
> suggested:
> 
> GTest: run: /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/pci-
> device/pci-device-tests/nop
> Run QEMU with: '-M pc  -device virtio-9p-pci'
> (MSG: starting QEMU: exec x86_64-softmmu/qemu-system-x86_64 -qtest unix:/tmp/
> qtest-18032.sock -qtest-log /dev/null -chardev socket,path=/tmp/
> qtest-18032.qmp,id=char0 -mon chardev=char0,mode=control -display none -M pc  
> -device virtio-9p-pci -accel qtest)
> qemu-system-x86_64: -device virtio-9p-pci: 9pfs device couldn't find fsdev 
> with the id = NULL
> Broken pipe
> 
> This fundamental virtio-9p-pci test obviously needs a complete 9p command 
> line, that is either a 'synth' driver one, or a 'local' one. But simply either 
> picking one or another is inappropriate here. This test should run once for 
> 'synth' and once for 'local'.

You're right, this is in fact also a problem for virtio-blk and virtio-net:

    /* FIXME: every test using these two nodes needs to setup a
     * -drive,id=drive0 otherwise QEMU is not going to start.
     * Therefore, we do not include "produces" edge for virtio
     * and pci-device yet.
    */

    /* FIXME: every test using these nodes needs to setup a
     * -netdev socket,id=hs0 otherwise QEMU is not going to start.
     * Therefore, we do not include "produces" edge for virtio
     * and pci-device yet.
     */

I still think we should do it like this, because it's closer to the way
that libqos will work long term.

Paolo

Re: [PATCH 08/12] tests/9pfs: refactor test names and test devices

[Christian Schoenebeck]

On Donnerstag, 1. Oktober 2020 13:56:42 CEST Paolo Bonzini wrote:
> On 01/10/20 13:34, Christian Schoenebeck wrote:
> > Paolo, I'm back at square one after changing to single-device model as you
> > suggested:
> > 
> > GTest: run:
> > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/pci-
> > device/pci-device-tests/nop
> > Run QEMU with: '-M pc  -device virtio-9p-pci'
> > (MSG: starting QEMU: exec x86_64-softmmu/qemu-system-x86_64 -qtest
> > unix:/tmp/ qtest-18032.sock -qtest-log /dev/null -chardev
> > socket,path=/tmp/
> > qtest-18032.qmp,id=char0 -mon chardev=char0,mode=control -display none -M
> > pc -device virtio-9p-pci -accel qtest)
> > qemu-system-x86_64: -device virtio-9p-pci: 9pfs device couldn't find fsdev
> > with the id = NULL
> > Broken pipe
> > 
> > This fundamental virtio-9p-pci test obviously needs a complete 9p command
> > line, that is either a 'synth' driver one, or a 'local' one. But simply
> > either picking one or another is inappropriate here. This test should run
> > once for 'synth' and once for 'local'.
> 
> You're right, this is in fact also a problem for virtio-blk and virtio-net:
> 
>     /* FIXME: every test using these two nodes needs to setup a
>      * -drive,id=drive0 otherwise QEMU is not going to start.
>      * Therefore, we do not include "produces" edge for virtio
>      * and pci-device yet.
>     */
> 
>     /* FIXME: every test using these nodes needs to setup a
>      * -netdev socket,id=hs0 otherwise QEMU is not going to start.
>      * Therefore, we do not include "produces" edge for virtio
>      * and pci-device yet.
>      */
> 
> I still think we should do it like this, because it's closer to the way
> that libqos will work long term.

Could you please elaborate why that long term plan bites with the working 
solution I provided? [patches 1 and 2]

I mean, the solution I suggested is simple and working. I don't see a reason 
why that should be incompatible with future plans. IMO it makes sense to use 
the suggested solution instead of dropping tests just because of potential qos 
changes somewhere in far future that might happen or not. It is still a qos 
design limitation after all that must be addressed sooner or later. So also a 
future qos design must deal with this in some way.

Best regards,
Christian Schoenebeck

Re: [PATCH 08/12] tests/9pfs: refactor test names and test devices

[Paolo Bonzini]

On 01/10/20 14:15, Christian Schoenebeck wrote:
> On Donnerstag, 1. Oktober 2020 13:56:42 CEST Paolo Bonzini wrote:
>> On 01/10/20 13:34, Christian Schoenebeck wrote:
>>> Paolo, I'm back at square one after changing to single-device model as you
>>> suggested:
>>>
>>> GTest: run:
>>> /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/pci-
>>> device/pci-device-tests/nop
>>> Run QEMU with: '-M pc  -device virtio-9p-pci'
>>> (MSG: starting QEMU: exec x86_64-softmmu/qemu-system-x86_64 -qtest
>>> unix:/tmp/ qtest-18032.sock -qtest-log /dev/null -chardev
>>> socket,path=/tmp/
>>> qtest-18032.qmp,id=char0 -mon chardev=char0,mode=control -display none -M
>>> pc -device virtio-9p-pci -accel qtest)
>>> qemu-system-x86_64: -device virtio-9p-pci: 9pfs device couldn't find fsdev
>>> with the id = NULL
>>> Broken pipe
>>>
>>> This fundamental virtio-9p-pci test obviously needs a complete 9p command
>>> line, that is either a 'synth' driver one, or a 'local' one. But simply
>>> either picking one or another is inappropriate here. This test should run
>>> once for 'synth' and once for 'local'.
>>
>> You're right, this is in fact also a problem for virtio-blk and virtio-net:
>>
>>     /* FIXME: every test using these two nodes needs to setup a
>>      * -drive,id=drive0 otherwise QEMU is not going to start.
>>      * Therefore, we do not include "produces" edge for virtio
>>      * and pci-device yet.
>>     */
>>
>>     /* FIXME: every test using these nodes needs to setup a
>>      * -netdev socket,id=hs0 otherwise QEMU is not going to start.
>>      * Therefore, we do not include "produces" edge for virtio
>>      * and pci-device yet.
>>      */
>>
>> I still think we should do it like this, because it's closer to the way
>> that libqos will work long term.
> 
> Could you please elaborate why that long term plan bites with the working 
> solution I provided? [patches 1 and 2]

Because the long term plan is to have a socket/plug mechanism for
backends where the device can provide a default backend to plug.

The suggested solution is all good for *a different use case*, namely to
test the same device with different options.  It is just wrong for the
purpose of selecting a frontend.

It occurred to me that you could also add a default backend to the
command line with "-fsdev" (in the libqos driver), and use -set in the
test to override it.  This is ugly (-set is ugly!) but it would let you
keep the tests, so it would probably be the best solution.

Paolo

Re: [PATCH 08/12] tests/9pfs: refactor test names and test devices

[Christian Schoenebeck]

On Donnerstag, 1. Oktober 2020 16:04:39 CEST Paolo Bonzini wrote:
> >> You're right, this is in fact also a problem for virtio-blk and virtio-
net:
> >>     /* FIXME: every test using these two nodes needs to setup a
> >>     
> >>      * -drive,id=drive0 otherwise QEMU is not going to start.
> >>      * Therefore, we do not include "produces" edge for virtio
> >>      * and pci-device yet.
> >>     
> >>     */
> >>     
> >>     /* FIXME: every test using these nodes needs to setup a
> >>     
> >>      * -netdev socket,id=hs0 otherwise QEMU is not going to start.
> >>      * Therefore, we do not include "produces" edge for virtio
> >>      * and pci-device yet.
> >>      */
> >> 
> >> I still think we should do it like this, because it's closer to the way
> >> that libqos will work long term.
> > 
> > Could you please elaborate why that long term plan bites with the working
> > solution I provided? [patches 1 and 2]
> 
> Because the long term plan is to have a socket/plug mechanism for
> backends where the device can provide a default backend to plug.

Ok, obviously I don't know the details of that future socket/plug plan, but 
what I could also imagine for the long-term: allowing to optionally define 
'config' nodes as subnodes of devices. Maybe that's similar to what you had in 
mind with that socket/plug model.

> The suggested solution is all good for *a different use case*, namely to
> test the same device with different options.  It is just wrong for the
> purpose of selecting a frontend.
> 
> It occurred to me that you could also add a default backend to the
> command line with "-fsdev" (in the libqos driver), and use -set in the
> test to override it.  This is ugly (-set is ugly!) but it would let you
> keep the tests, so it would probably be the best solution.

Ok, I'll have a look at that '*-set' alternative tomorrow.

But on doubt: I will simply disable those the implied pci and virtio tests for 
the time being.

I need the 'local' backend tests and have to move on; they are of much more 
value than those 2 implied pci and virtio tests.

Best regards,
Christian Schoenebeck