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