mbox series

[v2,00/34] Qtest driver framework

Message ID 20180806143412.27722-1-e.emanuelegiuseppe@gmail.com
Headers show
Series Qtest driver framework | expand

Message

Emanuele Giuseppe Esposito Aug. 6, 2018, 2:33 p.m. UTC
qgraph API for the qtest driver framework

This series of patches introduce a different qtest driver
organization, viewing machines, drivers and tests as node in a
graph, each having one or multiple edges relations.

The idea is to have a framework where each test asks for a specific
driver, and the framework takes care of allocating the proper devices
required and passing the correct command line arguments to QEMU.

A node can be of four types:
- MACHINE:   for example "arm/raspi2"
- DRIVER:    for example "generic-sdhci"
- INTERFACE: for example "sdhci" (interface for all "-sdhci" drivers)
- TEST:      for example "sdhci-test", consumes an interface and tests
             the functions provided

An edge relation between two nodes (drivers or machines) X and Y can be:
- X CONSUMES Y: Y can be plugged into X
- X PRODUCES Y: X provides the interface Y
- X CONTAINS Y: Y is part of X component

Basic framework steps are the following:
- All nodes and edges are created in their respective machine/driver/test files
- The framework starts QEMU and asks for a list of available devices
  and machines
- The framework walks the graph starting from the available machines and
  performs a Depth First Search for tests
- Once a test is found, the path is walked again and all drivers are
  allocated accordingly and the final interface is passed to the test
- The test is executed
- Unused objects are cleaned and the path discovery is continued

Depending on the QEMU binary used, only some drivers/machines will be available
and only test that are reached by them will be executed.

This work is being done as Google Summer of Code 2018 project for QEMU,
my mentors are Paolo Bonzini and Laurent Vivier.
Additional infos on the project can be found at:
https://wiki.qemu.org/Features/qtest_driver_framework

v2:
- added command line arguments caching
- converted all virtio tests
- converted e1000e test
- added two more machines (virt and ppc64)
- introduced separation between edge-name (used by get_driver) and
  node-name (matched with QMP query result)
- edge carries also additional arguments, like the pci address of
  destination device
- test now can execute a function before and after the command line
  is allocated, to allocate various file and sockets needed by command
  line and test
- better documentation with working example in qgraph.h
- removed esplicit creation of interfaces

Signed-off-by: Emanuele Giuseppe Esposito <e.emanuelegiuseppe@gmail.com>

Emanuele Giuseppe Esposito (33):
  tests: qgraph API for the qtest driver framework
  tests/qgraph: rename qpci_init_pc functions
  tests/qgraph: pci-pc driver and interface nodes
  tests/qgraph: x86_64/pc machine node
  tests/qgraph: sdhci driver and interface nodes
  tests/qgraph: sdhci test node
  tests/qgraph: arm/raspi2 machine node
  tests/qgraph: rename qpci_init_spapr functions
  tests/qgraph: pci-spapr driver and interface nodes
  tests/qgraph: ppc64/pseries machine node
  test/qgraph: e1000e driver and interface nodes
  test/qgraph: e1000e-test node
  test/qgraph: virtio_start_device function
  test/qgraph: virtio-pci driver and interface nodes
  tests/qgraph: rename qvirtio_mmio_init_device functions
  test/qgraph: virtio-mmio driver and interface nodes
  test/qgraph: arm/virt machine node
  test/qgraph: virtio-serial driver and interface nodes
  test/qgraph: virtio-console test node
  test/qgraph: virtio-serial test node
  test/qgraph: virtio-9p driver and interface nodes
  test/qgraph: virtio-9p test node
  test/qgraph: virtio-balloon driver and interface nodes
  test/qgraph: virtio-balloon test node
  test/qgraph: virtio-rng driver and interface nodes
  test/qgraph: virtio-rng test node
  test/qgraph: virtio-blk driver and interface nodes
  test/qgraph: virtio-blk test node
  test/qgraph: virtio-net driver and interface nodes
  test/qgraph: virtio-net test node
  test/qgraph: virtio-scsi driver and interface nodes
  test/qgraph: virtio-scsi test node
  test/qgraph: temporarly commented vhost-user-test

Paolo Bonzini (1):
  tests: virtio: separate ccw tests from libqos

 configure                            |   2 +-
 include/qemu/module.h                |   2 +
 tests/Makefile.include               |  79 +--
 tests/e1000e-test.c                  | 354 +++----------
 tests/i440fx-test.c                  |   2 +-
 tests/ide-test.c                     |   2 +-
 tests/libqos/ahci.c                  |   2 +-
 tests/libqos/e1000e.c                | 262 ++++++++++
 tests/libqos/e1000e.h                |  53 ++
 tests/libqos/libqos-pc.c             |   2 +-
 tests/libqos/libqos-spapr.c          |   2 +-
 tests/libqos/pci-pc.c                |  41 +-
 tests/libqos/pci-pc.h                |  22 +-
 tests/libqos/pci-spapr.c             |  57 ++-
 tests/libqos/pci-spapr.h             |  26 +-
 tests/libqos/pci.c                   |  48 +-
 tests/libqos/pci.h                   |  15 +
 tests/libqos/ppc64_pseries-machine.c | 111 +++++
 tests/libqos/qgraph.c                | 721 +++++++++++++++++++++++++++
 tests/libqos/qgraph.h                | 514 +++++++++++++++++++
 tests/libqos/qgraph_extra.h          | 263 ++++++++++
 tests/libqos/raspi2-machine.c        |  82 +++
 tests/libqos/sdhci.c                 | 163 ++++++
 tests/libqos/sdhci.h                 |  69 +++
 tests/libqos/virt-machine.c          |  90 ++++
 tests/libqos/virtio-9p.c             | 164 ++++++
 tests/libqos/virtio-9p.h             |  42 ++
 tests/libqos/virtio-balloon.c        | 111 +++++
 tests/libqos/virtio-balloon.h        |  39 ++
 tests/libqos/virtio-blk.c            | 126 +++++
 tests/libqos/virtio-blk.h            |  40 ++
 tests/libqos/virtio-mmio.c           |  72 ++-
 tests/libqos/virtio-mmio.h           |   6 +-
 tests/libqos/virtio-net.c            | 177 +++++++
 tests/libqos/virtio-net.h            |  41 ++
 tests/libqos/virtio-pci.c            |  80 ++-
 tests/libqos/virtio-pci.h            |  12 +
 tests/libqos/virtio-rng.c            | 108 ++++
 tests/libqos/virtio-rng.h            |  39 ++
 tests/libqos/virtio-scsi.c           | 117 +++++
 tests/libqos/virtio-scsi.h           |  39 ++
 tests/libqos/virtio-serial.c         | 109 ++++
 tests/libqos/virtio-serial.h         |  39 ++
 tests/libqos/virtio.c                |   9 +
 tests/libqos/virtio.h                |   1 +
 tests/libqos/x86_64_pc-machine.c     | 110 ++++
 tests/q35-test.c                     |   4 +-
 tests/qos-test.c                     | 480 ++++++++++++++++++
 tests/rtl8139-test.c                 |   2 +-
 tests/sdhci-test.c                   | 222 +++------
 tests/tco-test.c                     |   2 +-
 tests/test-qgraph.c                  | 446 +++++++++++++++++
 tests/usb-hcd-ehci-test.c            |   2 +-
 tests/vhost-user-test.c              |   7 +-
 tests/virtio-9p-test.c               | 221 +++-----
 tests/virtio-balloon-test.c          |  22 +-
 tests/virtio-blk-test.c              | 473 +++++++-----------
 tests/virtio-ccw-test.c              | 121 +++++
 tests/virtio-console-test.c          |  30 +-
 tests/virtio-net-test.c              | 163 ++----
 tests/virtio-rng-test.c              |  25 +-
 tests/virtio-scsi-test.c             | 155 +++---
 tests/virtio-serial-test.c           |  27 +-
 63 files changed, 5624 insertions(+), 1243 deletions(-)
 create mode 100644 tests/libqos/e1000e.c
 create mode 100644 tests/libqos/e1000e.h
 create mode 100644 tests/libqos/ppc64_pseries-machine.c
 create mode 100644 tests/libqos/qgraph.c
 create mode 100644 tests/libqos/qgraph.h
 create mode 100644 tests/libqos/qgraph_extra.h
 create mode 100644 tests/libqos/raspi2-machine.c
 create mode 100644 tests/libqos/sdhci.c
 create mode 100644 tests/libqos/sdhci.h
 create mode 100644 tests/libqos/virt-machine.c
 create mode 100644 tests/libqos/virtio-9p.c
 create mode 100644 tests/libqos/virtio-9p.h
 create mode 100644 tests/libqos/virtio-balloon.c
 create mode 100644 tests/libqos/virtio-balloon.h
 create mode 100644 tests/libqos/virtio-blk.c
 create mode 100644 tests/libqos/virtio-blk.h
 create mode 100644 tests/libqos/virtio-net.c
 create mode 100644 tests/libqos/virtio-net.h
 create mode 100644 tests/libqos/virtio-rng.c
 create mode 100644 tests/libqos/virtio-rng.h
 create mode 100644 tests/libqos/virtio-scsi.c
 create mode 100644 tests/libqos/virtio-scsi.h
 create mode 100644 tests/libqos/virtio-serial.c
 create mode 100644 tests/libqos/virtio-serial.h
 create mode 100644 tests/libqos/x86_64_pc-machine.c
 create mode 100644 tests/qos-test.c
 create mode 100644 tests/test-qgraph.c
 create mode 100644 tests/virtio-ccw-test.c

Comments

Paolo Bonzini Aug. 9, 2018, 8:57 a.m. UTC | #1
On 06/08/2018 16:33, Emanuele Giuseppe Esposito wrote:
> qgraph API for the qtest driver framework
> 
> This series of patches introduce a different qtest driver
> organization, viewing machines, drivers and tests as node in a
> graph, each having one or multiple edges relations.
> 
> The idea is to have a framework where each test asks for a specific
> driver, and the framework takes care of allocating the proper devices
> required and passing the correct command line arguments to QEMU.
> 
> A node can be of four types:
> - MACHINE:   for example "arm/raspi2"
> - DRIVER:    for example "generic-sdhci"
> - INTERFACE: for example "sdhci" (interface for all "-sdhci" drivers)
> - TEST:      for example "sdhci-test", consumes an interface and tests
>              the functions provided
> 
> An edge relation between two nodes (drivers or machines) X and Y can be:
> - X CONSUMES Y: Y can be plugged into X
> - X PRODUCES Y: X provides the interface Y
> - X CONTAINS Y: Y is part of X component
> 
> Basic framework steps are the following:
> - All nodes and edges are created in their respective machine/driver/test files
> - The framework starts QEMU and asks for a list of available devices
>   and machines
> - The framework walks the graph starting from the available machines and
>   performs a Depth First Search for tests
> - Once a test is found, the path is walked again and all drivers are
>   allocated accordingly and the final interface is passed to the test
> - The test is executed
> - Unused objects are cleaned and the path discovery is continued
> 
> Depending on the QEMU binary used, only some drivers/machines will be available
> and only test that are reached by them will be executed.
> 
> This work is being done as Google Summer of Code 2018 project for QEMU,
> my mentors are Paolo Bonzini and Laurent Vivier.
> Additional infos on the project can be found at:
> https://wiki.qemu.org/Features/qtest_driver_framework

Tests are not cleaning up properly.  There are two bugs, both related to
qtest_end not being called (from main in one case, after 
qos_invalidate_command_line in the other).  I tried this fix but then
the e1000e tests start failing:

diff --git a/tests/libqtest.h b/tests/libqtest.h
index ac52872..7a6fa6e 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -555,6 +555,9 @@ static inline QTestState *qtest_start(const char *args)
  */
 static inline void qtest_end(void)
 {
+    if (!global_qtest) {
+        return;
+    }
     qtest_quit(global_qtest);
     global_qtest = NULL;
 }
diff --git a/tests/qos-test.c b/tests/qos-test.c
index 1c3a7fc..a22d176 100644
--- a/tests/qos-test.c
+++ b/tests/qos-test.c
@@ -233,10 +233,7 @@ static void restart_qemu_or_continue(char *path)
     * new command line
     */
     if (g_strcmp0(old_path, path)) {
-        if (old_path) {
-            qtest_end();
-            g_free(old_path);
-        }
+        qtest_end();
         old_path = path;
         global_qtest = qtest_start(path);
     } else { /* if cmd line is the same, reset the guest */
@@ -247,7 +244,10 @@ static void restart_qemu_or_continue(char *path)
 
 void qos_invalidate_command_line(void)
 {
-    old_path = NULL;
+    if (old_path) {
+        g_free(old_path);
+        old_path = NULL;
+    }
 }
 
 /**
@@ -475,6 +475,7 @@ int main(int argc, char **argv)
 
     qos_graph_foreach_test_path(walk_path);
     g_test_run();
+    qtest_end();
     qos_graph_destroy();
     return 0;
 }


Also, qos-test can be added to check-qtest-generic-y, since it is not 
PCI-specific.

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 14f19eb..6012f6e 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -182,6 +182,7 @@ gcov-files-generic-y = monitor.c qapi/qmp-dispatch.c
 check-qtest-generic-y += tests/device-introspect-test$(EXESUF)
 gcov-files-generic-y = qdev-monitor.c qmp.c
 check-qtest-generic-y += tests/cdrom-test$(EXESUF)
+check-qtest-generic-y += tests/qos-test$(EXESUF)
 
 gcov-files-ipack-y += hw/ipack/ipack.c
 check-qtest-ipack-y += tests/ipoctal232-test$(EXESUF)
@@ -790,9 +791,6 @@ libqgraph-tests-obj-y += tests/virtio-scsi-test.o
 check-unit-y += tests/test-qgraph$(EXESUF)
 tests/test-qgraph$(EXESUF): tests/test-qgraph.o $(libqgraph-obj-y)
 
-check-qtest-pci-y += tests/qos-test$(EXESUF)
-tests/qos-test$(EXESUF): tests/qos-test.o $(libqgraph-tests-obj-y)
-
 tests/qmp-test$(EXESUF): tests/qmp-test.o
 tests/device-introspect-test$(EXESUF): tests/device-introspect-test.o
 tests/rtc-test$(EXESUF): tests/rtc-test.o

Another small bug is that virtio-net-test should also call 
qos_invalidate_command_line, since it is closing the socket that
is opened in virtio_net_test_setup.

Paolo

> v2:
> - added command line arguments caching
> - converted all virtio tests
> - converted e1000e test
> - added two more machines (virt and ppc64)
> - introduced separation between edge-name (used by get_driver) and
>   node-name (matched with QMP query result)
> - edge carries also additional arguments, like the pci address of
>   destination device
> - test now can execute a function before and after the command line
>   is allocated, to allocate various file and sockets needed by command
>   line and test
> - better documentation with working example in qgraph.h
> - removed esplicit creation of interfaces
> 
> Signed-off-by: Emanuele Giuseppe Esposito <e.emanuelegiuseppe@gmail.com>
> 
> Emanuele Giuseppe Esposito (33):
>   tests: qgraph API for the qtest driver framework
>   tests/qgraph: rename qpci_init_pc functions
>   tests/qgraph: pci-pc driver and interface nodes
>   tests/qgraph: x86_64/pc machine node
>   tests/qgraph: sdhci driver and interface nodes
>   tests/qgraph: sdhci test node
>   tests/qgraph: arm/raspi2 machine node
>   tests/qgraph: rename qpci_init_spapr functions
>   tests/qgraph: pci-spapr driver and interface nodes
>   tests/qgraph: ppc64/pseries machine node
>   test/qgraph: e1000e driver and interface nodes
>   test/qgraph: e1000e-test node
>   test/qgraph: virtio_start_device function
>   test/qgraph: virtio-pci driver and interface nodes
>   tests/qgraph: rename qvirtio_mmio_init_device functions
>   test/qgraph: virtio-mmio driver and interface nodes
>   test/qgraph: arm/virt machine node
>   test/qgraph: virtio-serial driver and interface nodes
>   test/qgraph: virtio-console test node
>   test/qgraph: virtio-serial test node
>   test/qgraph: virtio-9p driver and interface nodes
>   test/qgraph: virtio-9p test node
>   test/qgraph: virtio-balloon driver and interface nodes
>   test/qgraph: virtio-balloon test node
>   test/qgraph: virtio-rng driver and interface nodes
>   test/qgraph: virtio-rng test node
>   test/qgraph: virtio-blk driver and interface nodes
>   test/qgraph: virtio-blk test node
>   test/qgraph: virtio-net driver and interface nodes
>   test/qgraph: virtio-net test node
>   test/qgraph: virtio-scsi driver and interface nodes
>   test/qgraph: virtio-scsi test node
>   test/qgraph: temporarly commented vhost-user-test
> 
> Paolo Bonzini (1):
>   tests: virtio: separate ccw tests from libqos
> 
>  configure                            |   2 +-
>  include/qemu/module.h                |   2 +
>  tests/Makefile.include               |  79 +--
>  tests/e1000e-test.c                  | 354 +++----------
>  tests/i440fx-test.c                  |   2 +-
>  tests/ide-test.c                     |   2 +-
>  tests/libqos/ahci.c                  |   2 +-
>  tests/libqos/e1000e.c                | 262 ++++++++++
>  tests/libqos/e1000e.h                |  53 ++
>  tests/libqos/libqos-pc.c             |   2 +-
>  tests/libqos/libqos-spapr.c          |   2 +-
>  tests/libqos/pci-pc.c                |  41 +-
>  tests/libqos/pci-pc.h                |  22 +-
>  tests/libqos/pci-spapr.c             |  57 ++-
>  tests/libqos/pci-spapr.h             |  26 +-
>  tests/libqos/pci.c                   |  48 +-
>  tests/libqos/pci.h                   |  15 +
>  tests/libqos/ppc64_pseries-machine.c | 111 +++++
>  tests/libqos/qgraph.c                | 721 +++++++++++++++++++++++++++
>  tests/libqos/qgraph.h                | 514 +++++++++++++++++++
>  tests/libqos/qgraph_extra.h          | 263 ++++++++++
>  tests/libqos/raspi2-machine.c        |  82 +++
>  tests/libqos/sdhci.c                 | 163 ++++++
>  tests/libqos/sdhci.h                 |  69 +++
>  tests/libqos/virt-machine.c          |  90 ++++
>  tests/libqos/virtio-9p.c             | 164 ++++++
>  tests/libqos/virtio-9p.h             |  42 ++
>  tests/libqos/virtio-balloon.c        | 111 +++++
>  tests/libqos/virtio-balloon.h        |  39 ++
>  tests/libqos/virtio-blk.c            | 126 +++++
>  tests/libqos/virtio-blk.h            |  40 ++
>  tests/libqos/virtio-mmio.c           |  72 ++-
>  tests/libqos/virtio-mmio.h           |   6 +-
>  tests/libqos/virtio-net.c            | 177 +++++++
>  tests/libqos/virtio-net.h            |  41 ++
>  tests/libqos/virtio-pci.c            |  80 ++-
>  tests/libqos/virtio-pci.h            |  12 +
>  tests/libqos/virtio-rng.c            | 108 ++++
>  tests/libqos/virtio-rng.h            |  39 ++
>  tests/libqos/virtio-scsi.c           | 117 +++++
>  tests/libqos/virtio-scsi.h           |  39 ++
>  tests/libqos/virtio-serial.c         | 109 ++++
>  tests/libqos/virtio-serial.h         |  39 ++
>  tests/libqos/virtio.c                |   9 +
>  tests/libqos/virtio.h                |   1 +
>  tests/libqos/x86_64_pc-machine.c     | 110 ++++
>  tests/q35-test.c                     |   4 +-
>  tests/qos-test.c                     | 480 ++++++++++++++++++
>  tests/rtl8139-test.c                 |   2 +-
>  tests/sdhci-test.c                   | 222 +++------
>  tests/tco-test.c                     |   2 +-
>  tests/test-qgraph.c                  | 446 +++++++++++++++++
>  tests/usb-hcd-ehci-test.c            |   2 +-
>  tests/vhost-user-test.c              |   7 +-
>  tests/virtio-9p-test.c               | 221 +++-----
>  tests/virtio-balloon-test.c          |  22 +-
>  tests/virtio-blk-test.c              | 473 +++++++-----------
>  tests/virtio-ccw-test.c              | 121 +++++
>  tests/virtio-console-test.c          |  30 +-
>  tests/virtio-net-test.c              | 163 ++----
>  tests/virtio-rng-test.c              |  25 +-
>  tests/virtio-scsi-test.c             | 155 +++---
>  tests/virtio-serial-test.c           |  27 +-
>  63 files changed, 5624 insertions(+), 1243 deletions(-)
>  create mode 100644 tests/libqos/e1000e.c
>  create mode 100644 tests/libqos/e1000e.h
>  create mode 100644 tests/libqos/ppc64_pseries-machine.c
>  create mode 100644 tests/libqos/qgraph.c
>  create mode 100644 tests/libqos/qgraph.h
>  create mode 100644 tests/libqos/qgraph_extra.h
>  create mode 100644 tests/libqos/raspi2-machine.c
>  create mode 100644 tests/libqos/sdhci.c
>  create mode 100644 tests/libqos/sdhci.h
>  create mode 100644 tests/libqos/virt-machine.c
>  create mode 100644 tests/libqos/virtio-9p.c
>  create mode 100644 tests/libqos/virtio-9p.h
>  create mode 100644 tests/libqos/virtio-balloon.c
>  create mode 100644 tests/libqos/virtio-balloon.h
>  create mode 100644 tests/libqos/virtio-blk.c
>  create mode 100644 tests/libqos/virtio-blk.h
>  create mode 100644 tests/libqos/virtio-net.c
>  create mode 100644 tests/libqos/virtio-net.h
>  create mode 100644 tests/libqos/virtio-rng.c
>  create mode 100644 tests/libqos/virtio-rng.h
>  create mode 100644 tests/libqos/virtio-scsi.c
>  create mode 100644 tests/libqos/virtio-scsi.h
>  create mode 100644 tests/libqos/virtio-serial.c
>  create mode 100644 tests/libqos/virtio-serial.h
>  create mode 100644 tests/libqos/x86_64_pc-machine.c
>  create mode 100644 tests/qos-test.c
>  create mode 100644 tests/test-qgraph.c
>  create mode 100644 tests/virtio-ccw-test.c
>
Emanuele Giuseppe Esposito Aug. 9, 2018, 9:20 a.m. UTC | #2
On 08/09/2018 10:57 AM, Paolo Bonzini wrote:
> On 06/08/2018 16:33, Emanuele Giuseppe Esposito wrote:
>> qgraph API for the qtest driver framework
>>
>> This series of patches introduce a different qtest driver
>> organization, viewing machines, drivers and tests as node in a
>> graph, each having one or multiple edges relations.
>>
>> The idea is to have a framework where each test asks for a specific
>> driver, and the framework takes care of allocating the proper devices
>> required and passing the correct command line arguments to QEMU.
>>
>> A node can be of four types:
>> - MACHINE:   for example "arm/raspi2"
>> - DRIVER:    for example "generic-sdhci"
>> - INTERFACE: for example "sdhci" (interface for all "-sdhci" drivers)
>> - TEST:      for example "sdhci-test", consumes an interface and tests
>>               the functions provided
>>
>> An edge relation between two nodes (drivers or machines) X and Y can be:
>> - X CONSUMES Y: Y can be plugged into X
>> - X PRODUCES Y: X provides the interface Y
>> - X CONTAINS Y: Y is part of X component
>>
>> Basic framework steps are the following:
>> - All nodes and edges are created in their respective machine/driver/test files
>> - The framework starts QEMU and asks for a list of available devices
>>    and machines
>> - The framework walks the graph starting from the available machines and
>>    performs a Depth First Search for tests
>> - Once a test is found, the path is walked again and all drivers are
>>    allocated accordingly and the final interface is passed to the test
>> - The test is executed
>> - Unused objects are cleaned and the path discovery is continued
>>
>> Depending on the QEMU binary used, only some drivers/machines will be available
>> and only test that are reached by them will be executed.
>>
>> This work is being done as Google Summer of Code 2018 project for QEMU,
>> my mentors are Paolo Bonzini and Laurent Vivier.
>> Additional infos on the project can be found at:
>> https://wiki.qemu.org/Features/qtest_driver_framework
> Tests are not cleaning up properly.  There are two bugs, both related to
> qtest_end not being called (from main in one case, after
> qos_invalidate_command_line in the other).  I tried this fix but then
> the e1000e tests start failing:
>
> diff --git a/tests/libqtest.h b/tests/libqtest.h
> index ac52872..7a6fa6e 100644
> --- a/tests/libqtest.h
> +++ b/tests/libqtest.h
> @@ -555,6 +555,9 @@ static inline QTestState *qtest_start(const char *args)
>    */
>   static inline void qtest_end(void)
>   {
> +    if (!global_qtest) {
> +        return;
> +    }
>       qtest_quit(global_qtest);
>       global_qtest = NULL;
>   }
> diff --git a/tests/qos-test.c b/tests/qos-test.c
> index 1c3a7fc..a22d176 100644
> --- a/tests/qos-test.c
> +++ b/tests/qos-test.c
> @@ -233,10 +233,7 @@ static void restart_qemu_or_continue(char *path)
>       * new command line
>       */
>       if (g_strcmp0(old_path, path)) {
> -        if (old_path) {
> -            qtest_end();
> -            g_free(old_path);
> -        }
> +        qtest_end();
Why this? Shouldn't it be:

if (g_strcmp0(old_path, path)) {
         qtest_end(); /* handles global_qtest = NULL */
         g_free(old_path); /* handles NULL */
         old_path = path;
         global_qtest = qtest_start(path);
} else ....

>           old_path = path;
>           global_qtest = qtest_start(path);
>       } else { /* if cmd line is the same, reset the guest */
> @@ -247,7 +244,10 @@ static void restart_qemu_or_continue(char *path)
>   
>   void qos_invalidate_command_line(void)
>   {
> -    old_path = NULL;
> +    if (old_path) {
> +        g_free(old_path);
> +        old_path = NULL;
> +    }
>   }
Same here, just

{
     g_free(old_path);
     old_path=NULL;
}

  should be enough I think.
>   
>   /**
> @@ -475,6 +475,7 @@ int main(int argc, char **argv)
>   
>       qos_graph_foreach_test_path(walk_path);
>       g_test_run();
> +    qtest_end();
>       qos_graph_destroy();
>       return 0;
>   }
>
>
> Also, qos-test can be added to check-qtest-generic-y, since it is not
> PCI-specific.
>
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index 14f19eb..6012f6e 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -182,6 +182,7 @@ gcov-files-generic-y = monitor.c qapi/qmp-dispatch.c
>   check-qtest-generic-y += tests/device-introspect-test$(EXESUF)
>   gcov-files-generic-y = qdev-monitor.c qmp.c
>   check-qtest-generic-y += tests/cdrom-test$(EXESUF)
> +check-qtest-generic-y += tests/qos-test$(EXESUF)
>   
>   gcov-files-ipack-y += hw/ipack/ipack.c
>   check-qtest-ipack-y += tests/ipoctal232-test$(EXESUF)
> @@ -790,9 +791,6 @@ libqgraph-tests-obj-y += tests/virtio-scsi-test.o
>   check-unit-y += tests/test-qgraph$(EXESUF)
>   tests/test-qgraph$(EXESUF): tests/test-qgraph.o $(libqgraph-obj-y)
>   
> -check-qtest-pci-y += tests/qos-test$(EXESUF)

> -tests/qos-test$(EXESUF): tests/qos-test.o $(libqgraph-tests-obj-y)
Why did you comment this line? To me it does not compile without it.
> -
>   tests/qmp-test$(EXESUF): tests/qmp-test.o
>   tests/device-introspect-test$(EXESUF): tests/device-introspect-test.o
>   tests/rtc-test$(EXESUF): tests/rtc-test.o
>
> Another small bug is that virtio-net-test should also call
> qos_invalidate_command_line, since it is closing the socket that
> is opened in virtio_net_test_setup.
Right, I forgot. Also virtio-blk-test needs to do it.

I tried to apply your changes with my suggested modifications, and 
everything seems to work to me.
>> v2:
>> - added command line arguments caching
>> - converted all virtio tests
>> - converted e1000e test
>> - added two more machines (virt and ppc64)
>> - introduced separation between edge-name (used by get_driver) and
>>    node-name (matched with QMP query result)
>> - edge carries also additional arguments, like the pci address of
>>    destination device
>> - test now can execute a function before and after the command line
>>    is allocated, to allocate various file and sockets needed by command
>>    line and test
>> - better documentation with working example in qgraph.h
>> - removed esplicit creation of interfaces
>>
>> Signed-off-by: Emanuele Giuseppe Esposito <e.emanuelegiuseppe@gmail.com>
>>
>> Emanuele Giuseppe Esposito (33):
>>    tests: qgraph API for the qtest driver framework
>>    tests/qgraph: rename qpci_init_pc functions
>>    tests/qgraph: pci-pc driver and interface nodes
>>    tests/qgraph: x86_64/pc machine node
>>    tests/qgraph: sdhci driver and interface nodes
>>    tests/qgraph: sdhci test node
>>    tests/qgraph: arm/raspi2 machine node
>>    tests/qgraph: rename qpci_init_spapr functions
>>    tests/qgraph: pci-spapr driver and interface nodes
>>    tests/qgraph: ppc64/pseries machine node
>>    test/qgraph: e1000e driver and interface nodes
>>    test/qgraph: e1000e-test node
>>    test/qgraph: virtio_start_device function
>>    test/qgraph: virtio-pci driver and interface nodes
>>    tests/qgraph: rename qvirtio_mmio_init_device functions
>>    test/qgraph: virtio-mmio driver and interface nodes
>>    test/qgraph: arm/virt machine node
>>    test/qgraph: virtio-serial driver and interface nodes
>>    test/qgraph: virtio-console test node
>>    test/qgraph: virtio-serial test node
>>    test/qgraph: virtio-9p driver and interface nodes
>>    test/qgraph: virtio-9p test node
>>    test/qgraph: virtio-balloon driver and interface nodes
>>    test/qgraph: virtio-balloon test node
>>    test/qgraph: virtio-rng driver and interface nodes
>>    test/qgraph: virtio-rng test node
>>    test/qgraph: virtio-blk driver and interface nodes
>>    test/qgraph: virtio-blk test node
>>    test/qgraph: virtio-net driver and interface nodes
>>    test/qgraph: virtio-net test node
>>    test/qgraph: virtio-scsi driver and interface nodes
>>    test/qgraph: virtio-scsi test node
>>    test/qgraph: temporarly commented vhost-user-test
>>
>> Paolo Bonzini (1):
>>    tests: virtio: separate ccw tests from libqos
>>
>>   configure                            |   2 +-
>>   include/qemu/module.h                |   2 +
>>   tests/Makefile.include               |  79 +--
>>   tests/e1000e-test.c                  | 354 +++----------
>>   tests/i440fx-test.c                  |   2 +-
>>   tests/ide-test.c                     |   2 +-
>>   tests/libqos/ahci.c                  |   2 +-
>>   tests/libqos/e1000e.c                | 262 ++++++++++
>>   tests/libqos/e1000e.h                |  53 ++
>>   tests/libqos/libqos-pc.c             |   2 +-
>>   tests/libqos/libqos-spapr.c          |   2 +-
>>   tests/libqos/pci-pc.c                |  41 +-
>>   tests/libqos/pci-pc.h                |  22 +-
>>   tests/libqos/pci-spapr.c             |  57 ++-
>>   tests/libqos/pci-spapr.h             |  26 +-
>>   tests/libqos/pci.c                   |  48 +-
>>   tests/libqos/pci.h                   |  15 +
>>   tests/libqos/ppc64_pseries-machine.c | 111 +++++
>>   tests/libqos/qgraph.c                | 721 +++++++++++++++++++++++++++
>>   tests/libqos/qgraph.h                | 514 +++++++++++++++++++
>>   tests/libqos/qgraph_extra.h          | 263 ++++++++++
>>   tests/libqos/raspi2-machine.c        |  82 +++
>>   tests/libqos/sdhci.c                 | 163 ++++++
>>   tests/libqos/sdhci.h                 |  69 +++
>>   tests/libqos/virt-machine.c          |  90 ++++
>>   tests/libqos/virtio-9p.c             | 164 ++++++
>>   tests/libqos/virtio-9p.h             |  42 ++
>>   tests/libqos/virtio-balloon.c        | 111 +++++
>>   tests/libqos/virtio-balloon.h        |  39 ++
>>   tests/libqos/virtio-blk.c            | 126 +++++
>>   tests/libqos/virtio-blk.h            |  40 ++
>>   tests/libqos/virtio-mmio.c           |  72 ++-
>>   tests/libqos/virtio-mmio.h           |   6 +-
>>   tests/libqos/virtio-net.c            | 177 +++++++
>>   tests/libqos/virtio-net.h            |  41 ++
>>   tests/libqos/virtio-pci.c            |  80 ++-
>>   tests/libqos/virtio-pci.h            |  12 +
>>   tests/libqos/virtio-rng.c            | 108 ++++
>>   tests/libqos/virtio-rng.h            |  39 ++
>>   tests/libqos/virtio-scsi.c           | 117 +++++
>>   tests/libqos/virtio-scsi.h           |  39 ++
>>   tests/libqos/virtio-serial.c         | 109 ++++
>>   tests/libqos/virtio-serial.h         |  39 ++
>>   tests/libqos/virtio.c                |   9 +
>>   tests/libqos/virtio.h                |   1 +
>>   tests/libqos/x86_64_pc-machine.c     | 110 ++++
>>   tests/q35-test.c                     |   4 +-
>>   tests/qos-test.c                     | 480 ++++++++++++++++++
>>   tests/rtl8139-test.c                 |   2 +-
>>   tests/sdhci-test.c                   | 222 +++------
>>   tests/tco-test.c                     |   2 +-
>>   tests/test-qgraph.c                  | 446 +++++++++++++++++
>>   tests/usb-hcd-ehci-test.c            |   2 +-
>>   tests/vhost-user-test.c              |   7 +-
>>   tests/virtio-9p-test.c               | 221 +++-----
>>   tests/virtio-balloon-test.c          |  22 +-
>>   tests/virtio-blk-test.c              | 473 +++++++-----------
>>   tests/virtio-ccw-test.c              | 121 +++++
>>   tests/virtio-console-test.c          |  30 +-
>>   tests/virtio-net-test.c              | 163 ++----
>>   tests/virtio-rng-test.c              |  25 +-
>>   tests/virtio-scsi-test.c             | 155 +++---
>>   tests/virtio-serial-test.c           |  27 +-
>>   63 files changed, 5624 insertions(+), 1243 deletions(-)
>>   create mode 100644 tests/libqos/e1000e.c
>>   create mode 100644 tests/libqos/e1000e.h
>>   create mode 100644 tests/libqos/ppc64_pseries-machine.c
>>   create mode 100644 tests/libqos/qgraph.c
>>   create mode 100644 tests/libqos/qgraph.h
>>   create mode 100644 tests/libqos/qgraph_extra.h
>>   create mode 100644 tests/libqos/raspi2-machine.c
>>   create mode 100644 tests/libqos/sdhci.c
>>   create mode 100644 tests/libqos/sdhci.h
>>   create mode 100644 tests/libqos/virt-machine.c
>>   create mode 100644 tests/libqos/virtio-9p.c
>>   create mode 100644 tests/libqos/virtio-9p.h
>>   create mode 100644 tests/libqos/virtio-balloon.c
>>   create mode 100644 tests/libqos/virtio-balloon.h
>>   create mode 100644 tests/libqos/virtio-blk.c
>>   create mode 100644 tests/libqos/virtio-blk.h
>>   create mode 100644 tests/libqos/virtio-net.c
>>   create mode 100644 tests/libqos/virtio-net.h
>>   create mode 100644 tests/libqos/virtio-rng.c
>>   create mode 100644 tests/libqos/virtio-rng.h
>>   create mode 100644 tests/libqos/virtio-scsi.c
>>   create mode 100644 tests/libqos/virtio-scsi.h
>>   create mode 100644 tests/libqos/virtio-serial.c
>>   create mode 100644 tests/libqos/virtio-serial.h
>>   create mode 100644 tests/libqos/x86_64_pc-machine.c
>>   create mode 100644 tests/qos-test.c
>>   create mode 100644 tests/test-qgraph.c
>>   create mode 100644 tests/virtio-ccw-test.c
>>
Paolo Bonzini Aug. 9, 2018, 9:44 a.m. UTC | #3
On 09/08/2018 11:20, Emanuele wrote:
>>
> Why this? Shouldn't it be:
> 
> if (g_strcmp0(old_path, path)) {
>         qtest_end(); /* handles global_qtest = NULL */
>         g_free(old_path); /* handles NULL */
>         old_path = path;
>         global_qtest = qtest_start(path);
> } else ....

Yes, of course.  Though I'd have thought that my version has "just" a
memory leak.  Even better, this could call qos_invalidate_command_line.

Can you post the fixes to a separate commit on github?

Paolo
Emanuele Giuseppe Esposito Aug. 9, 2018, 10:27 a.m. UTC | #4
On 08/09/2018 11:44 AM, Paolo Bonzini wrote:
> On 09/08/2018 11:20, Emanuele wrote:
>> Why this? Shouldn't it be:
>>
>> if (g_strcmp0(old_path, path)) {
>>          qtest_end(); /* handles global_qtest = NULL */
>>          g_free(old_path); /* handles NULL */
>>          old_path = path;
>>          global_qtest = qtest_start(path);
>> } else ....
> Yes, of course.  Though I'd have thought that my version has "just" a
> memory leak.  Even better, this could call qos_invalidate_command_line.
>
> Can you post the fixes to a separate commit on github?
https://github.com/esposem/qemu/commit/4389e83522c06ce7001382284153d59105692061

I also added g_free(old_path); in qos-test main function, since last 
old_path is not free'd.
This commit is part of qgraph-v3, where I am also applying Laurent's 
suggestions.

Emanuele
Emanuele Giuseppe Esposito Aug. 10, 2018, 10:40 a.m. UTC | #5
On 08/09/2018 11:44 AM, Paolo Bonzini wrote:
> On 09/08/2018 11:20, Emanuele wrote:
>> Why this? Shouldn't it be:
>>
>> if (g_strcmp0(old_path, path)) {
>>          qtest_end(); /* handles global_qtest = NULL */
>>          g_free(old_path); /* handles NULL */
>>          old_path = path;
>>          global_qtest = qtest_start(path);
>> } else ....
> Yes, of course.  Though I'd have thought that my version has "just" a
> memory leak.  Even better, this could call qos_invalidate_command_line.
>
> Can you post the fixes to a separate commit on github?
Paolo, did you have a chance to test the fixes? I tried, it seems to be 
working fine.
About valgrind, it only detects an "invalide read of size 8" in 
blk-hotplug test (tests/virtio-blk-test.c), but I don't know why is it 
doing this.
For the rest, there shouldn't be any memory leak.

Should I rebase them in the proper commits? I am ready to submit patch v3.

Emanuele