diff mbox series

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

Message ID 265f5d9a0fb10ce5e782455839d1baf678dbac48.1601203436.git.qemu_oss@crudebyte.com
State New
Headers show
Series 9pfs: add tests using local fs driver | expand

Commit Message

Christian Schoenebeck Sept. 27, 2020, 10:40 a.m. UTC
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(-)

Comments

Paolo Bonzini Sept. 28, 2020, 8:36 a.m. UTC | #1
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
Paolo Bonzini Sept. 28, 2020, 8:37 a.m. UTC | #2
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
Christian Schoenebeck Sept. 28, 2020, 11:56 a.m. UTC | #3
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
Paolo Bonzini Sept. 28, 2020, 12:42 p.m. UTC | #4
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
Christian Schoenebeck Sept. 28, 2020, 1:35 p.m. UTC | #5
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
Paolo Bonzini Sept. 28, 2020, 4:38 p.m. UTC | #6
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
Christian Schoenebeck Oct. 1, 2020, 11:34 a.m. UTC | #7
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
Paolo Bonzini Oct. 1, 2020, 11:56 a.m. UTC | #8
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
Christian Schoenebeck Oct. 1, 2020, 12:15 p.m. UTC | #9
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
Paolo Bonzini Oct. 1, 2020, 2:04 p.m. UTC | #10
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
Christian Schoenebeck Oct. 1, 2020, 3:26 p.m. UTC | #11
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
diff mbox series

Patch

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);