[PATCH 00/12] qtests vs. default devices

[PATCH 00/12] qtests vs. default devices

[Fabiano Rosas]

Most of our tests assume the presence of default devices. When
building --without-default-devices or with individual CONFIGs
disabled, several tests fail.

I went through them one by one and did local changes to skip or avoid
including tests that require devices that are missing. With these
initial changes, 'make check' now passes for the
--without-default-devices build for x86 and arm.

However, the approach of making local changes seems hard to maintain:
every time a CONFIG changes from 'y' to 'n' a test might break and all
new tests need to remember to check before adding devices, which some
tests add several.

So the last patch in the series provides an alternative: Parse the
command line at qtest_init and skip the test if devices are
missing. Individual tests would have to check 'if (!qts)' and
bail. Seems a bit heavy-weight, but it would mean we don't have to put
a qtest_has_device check for every device in every test.

Based on: <20230206140809.26028-1-farosas@suse.de>
[PATCH 00/10] Kconfig vs. default devices
https://lore.kernel.org/r/20230206140809.26028-1-farosas@suse.de

Fabiano Rosas (12):
  tests/qtest: Skip PXE tests for missing devices
  tests/qtest: Do not run lsi53c895a test if device is not present
  tests/qtest: Add dependence on PCIE_ROOT for virtio-net-failover.c
  tests/qtest: Skip virtio-serial-console tests if device not present
  tests/qtest: hd-geo-test: Check for missing devices
  tests/qtest: Skip unplug tests that use missing devices
  tests/qtest: drive_del-test: Skip tests that require missing devices
  tests/qtest: Check for devices in bios-tables-test
  tests/qtest: Do not include hexloader-test if loader device is not
    present
  tests/qemu-iotests: Require virtio-scsi-pci
  tests/qtest: bios-tables-test: Skip if missing configs
  [NOT FOR MERGE] tests/qtest: Introduce qtest_validate_args

 tests/qemu-iotests/186             |   1 +
 tests/qtest/bios-tables-test.c     |  75 +++++++++++++++-
 tests/qtest/device-plug-test.c     |  19 ++++
 tests/qtest/drive_del-test.c       |  70 +++++++++++++++
 tests/qtest/fuzz-lsi53c895a-test.c |   4 +
 tests/qtest/hd-geo-test.c          |  38 +++++---
 tests/qtest/hexloader-test.c       |   5 ++
 tests/qtest/libqtest.c             | 137 ++++++++++++++++++++++++++++-
 tests/qtest/libqtest.h             |  12 +++
 tests/qtest/meson.build            |  11 ++-
 tests/qtest/pxe-test.c             |   4 +
 tests/qtest/virtio-net-failover.c  |   5 ++
 tests/qtest/virtio-serial-test.c   |   4 +
 13 files changed, 363 insertions(+), 22 deletions(-)

-- 
2.35.3

[PATCH 01/12] tests/qtest: Skip PXE tests for missing devices

[Fabiano Rosas]

Check if the devices we're trying to add are present in the QEMU
binary. They could have been removed from the build via Kconfig or the
--without-default-devices option.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/pxe-test.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tests/qtest/pxe-test.c b/tests/qtest/pxe-test.c
index 52f0b5c67c..62b6eef464 100644
--- a/tests/qtest/pxe-test.c
+++ b/tests/qtest/pxe-test.c
@@ -108,6 +108,10 @@ static void test_batch(const testdef_t *tests, bool ipv6)
         const testdef_t *test = &tests[i];
         char *testname;
 
+        if (!qtest_has_device(test->model)) {
+            continue;
+        }
+
         testname = g_strdup_printf("pxe/ipv4/%s/%s",
                                    test->machine, test->model);
         qtest_add_data_func(testname, test, test_pxe_ipv4);
-- 
2.35.3

Re: [PATCH 01/12] tests/qtest: Skip PXE tests for missing devices

[Thomas Huth]

On 06/02/2023 16.04, Fabiano Rosas wrote:
> Check if the devices we're trying to add are present in the QEMU
> binary. They could have been removed from the build via Kconfig or the
> --without-default-devices option.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>   tests/qtest/pxe-test.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/tests/qtest/pxe-test.c b/tests/qtest/pxe-test.c
> index 52f0b5c67c..62b6eef464 100644
> --- a/tests/qtest/pxe-test.c
> +++ b/tests/qtest/pxe-test.c
> @@ -108,6 +108,10 @@ static void test_batch(const testdef_t *tests, bool ipv6)
>           const testdef_t *test = &tests[i];
>           char *testname;
>   
> +        if (!qtest_has_device(test->model)) {
> +            continue;
> +        }

Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH 02/12] tests/qtest: Do not run lsi53c895a test if device is not present

[Fabiano Rosas]

The tests are built once for all the targets, so as long as one QEMU
binary is built with CONFIG_LSI_SCSI_PCI=y, this test will
run. However some binaries might not include the device. So check this
again in runtime.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/fuzz-lsi53c895a-test.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tests/qtest/fuzz-lsi53c895a-test.c b/tests/qtest/fuzz-lsi53c895a-test.c
index 392a7ae7ed..a9254b455d 100644
--- a/tests/qtest/fuzz-lsi53c895a-test.c
+++ b/tests/qtest/fuzz-lsi53c895a-test.c
@@ -112,6 +112,10 @@ static void test_lsi_do_dma_empty_queue(void)
 
 int main(int argc, char **argv)
 {
+    if (!qtest_has_device("lsi53c895a")) {
+        return 0;
+    }
+
     g_test_init(&argc, &argv, NULL);
 
     qtest_add_func("fuzz/lsi53c895a/lsi_do_dma_empty_queue",
-- 
2.35.3

Re: [PATCH 02/12] tests/qtest: Do not run lsi53c895a test if device is not present

[Philippe Mathieu-Daudé]

On 6/2/23 16:04, Fabiano Rosas wrote:
> The tests are built once for all the targets, so as long as one QEMU
> binary is built with CONFIG_LSI_SCSI_PCI=y, this test will
> run. However some binaries might not include the device. So check this
> again in runtime.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>   tests/qtest/fuzz-lsi53c895a-test.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/tests/qtest/fuzz-lsi53c895a-test.c b/tests/qtest/fuzz-lsi53c895a-test.c
> index 392a7ae7ed..a9254b455d 100644
> --- a/tests/qtest/fuzz-lsi53c895a-test.c
> +++ b/tests/qtest/fuzz-lsi53c895a-test.c
> @@ -112,6 +112,10 @@ static void test_lsi_do_dma_empty_queue(void)
>   
>   int main(int argc, char **argv)
>   {
> +    if (!qtest_has_device("lsi53c895a")) {
> +        return 0;
> +    }

Shouldn't we update Kconfig to now add the test unconditionally?
Squashing:

-- >8 --
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index e97616d327..498e49a1b2 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -66,7 +66,6 @@ qtests_i386 = \
    (config_all_devices.has_key('CONFIG_RTL8139_PCI') ? ['rtl8139-test'] 
: []) +              \
    (config_all_devices.has_key('CONFIG_E1000E_PCI_EXPRESS') ? 
['fuzz-e1000e-test'] : []) +   \
    (config_all_devices.has_key('CONFIG_MEGASAS_SCSI_PCI') ? 
['fuzz-megasas-test'] : []) +    \
-  (config_all_devices.has_key('CONFIG_LSI_SCSI_PCI') ? 
['fuzz-lsi53c895a-test'] : []) +     \
    (config_all_devices.has_key('CONFIG_VIRTIO_SCSI') ? 
['fuzz-virtio-scsi-test'] : []) +     \
    (config_all_devices.has_key('CONFIG_SB16') ? ['fuzz-sb16-test'] : 
[]) +                   \
    (config_all_devices.has_key('CONFIG_SDHCI_PCI') ? 
['fuzz-sdcard-test'] : []) +            \
@@ -87,6 +86,7 @@ qtests_i386 = \
     'rtc-test',
     'i440fx-test',
     'fw_cfg-test',
+   'fuzz-lsi53c895a-test',
     'device-plug-test',
     'drive_del-test',
     'tco-test',
---

Re: [PATCH 02/12] tests/qtest: Do not run lsi53c895a test if device is not present

[Fabiano Rosas]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> On 6/2/23 16:04, Fabiano Rosas wrote:
>> The tests are built once for all the targets, so as long as one QEMU
>> binary is built with CONFIG_LSI_SCSI_PCI=y, this test will
>> run. However some binaries might not include the device. So check this
>> again in runtime.
>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>   tests/qtest/fuzz-lsi53c895a-test.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>> 
>> diff --git a/tests/qtest/fuzz-lsi53c895a-test.c b/tests/qtest/fuzz-lsi53c895a-test.c
>> index 392a7ae7ed..a9254b455d 100644
>> --- a/tests/qtest/fuzz-lsi53c895a-test.c
>> +++ b/tests/qtest/fuzz-lsi53c895a-test.c
>> @@ -112,6 +112,10 @@ static void test_lsi_do_dma_empty_queue(void)
>>   
>>   int main(int argc, char **argv)
>>   {
>> +    if (!qtest_has_device("lsi53c895a")) {
>> +        return 0;
>> +    }
>
> Shouldn't we update Kconfig to now add the test unconditionally?
> Squashing:

I think we'd still want to not build this test if nothing selected
CONFIG_LSI_SCSI_PCI.

Re: [PATCH 02/12] tests/qtest: Do not run lsi53c895a test if device is not present

[Philippe Mathieu-Daudé]

On 6/2/23 18:46, Fabiano Rosas wrote:
> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> 
>> On 6/2/23 16:04, Fabiano Rosas wrote:
>>> The tests are built once for all the targets, so as long as one QEMU
>>> binary is built with CONFIG_LSI_SCSI_PCI=y, this test will
>>> run. However some binaries might not include the device. So check this
>>> again in runtime.
>>>
>>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>>> ---
>>>    tests/qtest/fuzz-lsi53c895a-test.c | 4 ++++
>>>    1 file changed, 4 insertions(+)
>>>
>>> diff --git a/tests/qtest/fuzz-lsi53c895a-test.c b/tests/qtest/fuzz-lsi53c895a-test.c
>>> index 392a7ae7ed..a9254b455d 100644
>>> --- a/tests/qtest/fuzz-lsi53c895a-test.c
>>> +++ b/tests/qtest/fuzz-lsi53c895a-test.c
>>> @@ -112,6 +112,10 @@ static void test_lsi_do_dma_empty_queue(void)
>>>    
>>>    int main(int argc, char **argv)
>>>    {
>>> +    if (!qtest_has_device("lsi53c895a")) {
>>> +        return 0;
>>> +    }
>>
>> Shouldn't we update Kconfig to now add the test unconditionally?

(I meant meson.build, not Kconfig).

>> Squashing:
> 
> I think we'd still want to not build this test if nothing selected
> CONFIG_LSI_SCSI_PCI.

Yeah, no need to waste resources building/testing if not available.

OTOH the qtest's meson.build is too complicated.

Re: [PATCH 02/12] tests/qtest: Do not run lsi53c895a test if device is not present

[Thomas Huth]

On 06/02/2023 16.04, Fabiano Rosas wrote:
> The tests are built once for all the targets, so as long as one QEMU
> binary is built with CONFIG_LSI_SCSI_PCI=y, this test will
> run. However some binaries might not include the device. So check this
> again in runtime.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>   tests/qtest/fuzz-lsi53c895a-test.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/tests/qtest/fuzz-lsi53c895a-test.c b/tests/qtest/fuzz-lsi53c895a-test.c
> index 392a7ae7ed..a9254b455d 100644
> --- a/tests/qtest/fuzz-lsi53c895a-test.c
> +++ b/tests/qtest/fuzz-lsi53c895a-test.c
> @@ -112,6 +112,10 @@ static void test_lsi_do_dma_empty_queue(void)
>   
>   int main(int argc, char **argv)
>   {
> +    if (!qtest_has_device("lsi53c895a")) {
> +        return 0;
> +    }

I'm a little bit confused right now ... We're already checking 
CONFIG_LSI_SCSI_PCI in meson.build, and we're only adding the test to 
qtests_i386 there ... so how did you end up in a situation where you needed 
this change?

  Thomas

Re: [PATCH 02/12] tests/qtest: Do not run lsi53c895a test if device is not present

[Fabiano Rosas]

Thomas Huth <thuth@redhat.com> writes:

> On 06/02/2023 16.04, Fabiano Rosas wrote:
>> The tests are built once for all the targets, so as long as one QEMU
>> binary is built with CONFIG_LSI_SCSI_PCI=y, this test will
>> run. However some binaries might not include the device. So check this
>> again in runtime.
>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>   tests/qtest/fuzz-lsi53c895a-test.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>> 
>> diff --git a/tests/qtest/fuzz-lsi53c895a-test.c b/tests/qtest/fuzz-lsi53c895a-test.c
>> index 392a7ae7ed..a9254b455d 100644
>> --- a/tests/qtest/fuzz-lsi53c895a-test.c
>> +++ b/tests/qtest/fuzz-lsi53c895a-test.c
>> @@ -112,6 +112,10 @@ static void test_lsi_do_dma_empty_queue(void)
>>   
>>   int main(int argc, char **argv)
>>   {
>> +    if (!qtest_has_device("lsi53c895a")) {
>> +        return 0;
>> +    }
>
> I'm a little bit confused right now ... We're already checking 
> CONFIG_LSI_SCSI_PCI in meson.build, and we're only adding the test to 
> qtests_i386 there ... so how did you end up in a situation where you needed 
> this change?

I building with
--target-list=i386-softmmu,x86_64-softmmu,aarch64-softmmu,arm-softmmu
and the arm machines are selecting the CONFIG. That goes into
config_all_devices and meson can't tell the difference. In theory that
could be try for any CONFIG out there. Here's the output:

$ (...) qemu/build/tests/qtest/fuzz-lsi53c895a-test --tap -k
# random seed: R02Sfd8259d54ecdb6bdd8187b52e1a921c4
1..2
# Start of i386 tests
# Start of fuzz tests
# Start of lsi53c895a tests

# starting QEMU: exec ./qemu-system-i386 -qtest
  unix:/tmp/qtest-5629.sock -qtest-log /dev/null -chardev
  socket,path=/tmp/qtest-5629.qmp,id=char0 -mon
  chardev=char0,mode=control -display none -M q35 -nographic -monitor
  none -serial none -drive
  if=none,id=drive0,file=null-co://,file.read-zeroes=on,format=raw
  -device lsi53c895a,id=scsi0 -device
  scsi-hd,drive=drive0,bus=scsi0.0,channel=0,scsi-id=0,lun=0 -accel
  qtest

qemu-system-i386: -device lsi53c895a,id=scsi0: 'lsi53c895a' is not a valid device model name
Broken pipe
../tests/qtest/libqtest.c:181: kill_qemu() tried to terminate QEMU process but encountered exit status 1 (expected 0)
Aborted (core dumped)

$ find build -name "*.mak" -exec grep -H LSI_SCSI_PCI {} \;
./aarch64-softmmu-config-devices.mak:CONFIG_LSI_SCSI_PCI=y
./arm-softmmu-config-devices.mak:CONFIG_LSI_SCSI_PCI=y

hw/arm/Kconfig has:

config REALVIEW
    ...
    select LSI_SCSI_PCI

config VERSATILE
    ...
    select LSI_SCSI_PCI

Re: [PATCH 02/12] tests/qtest: Do not run lsi53c895a test if device is not present

[Thomas Huth]

On 07/02/2023 15.02, Fabiano Rosas wrote:
> Thomas Huth <thuth@redhat.com> writes:
> 
>> On 06/02/2023 16.04, Fabiano Rosas wrote:
>>> The tests are built once for all the targets, so as long as one QEMU
>>> binary is built with CONFIG_LSI_SCSI_PCI=y, this test will
>>> run. However some binaries might not include the device. So check this
>>> again in runtime.
>>>
>>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>>> ---
>>>    tests/qtest/fuzz-lsi53c895a-test.c | 4 ++++
>>>    1 file changed, 4 insertions(+)
>>>
>>> diff --git a/tests/qtest/fuzz-lsi53c895a-test.c b/tests/qtest/fuzz-lsi53c895a-test.c
>>> index 392a7ae7ed..a9254b455d 100644
>>> --- a/tests/qtest/fuzz-lsi53c895a-test.c
>>> +++ b/tests/qtest/fuzz-lsi53c895a-test.c
>>> @@ -112,6 +112,10 @@ static void test_lsi_do_dma_empty_queue(void)
>>>    
>>>    int main(int argc, char **argv)
>>>    {
>>> +    if (!qtest_has_device("lsi53c895a")) {
>>> +        return 0;
>>> +    }
>>
>> I'm a little bit confused right now ... We're already checking
>> CONFIG_LSI_SCSI_PCI in meson.build, and we're only adding the test to
>> qtests_i386 there ... so how did you end up in a situation where you needed
>> this change?
> 
> I building with
> --target-list=i386-softmmu,x86_64-softmmu,aarch64-softmmu,arm-softmmu
> and the arm machines are selecting the CONFIG
...
> hw/arm/Kconfig has:
> 
> config REALVIEW
>      ...
>      select LSI_SCSI_PCI

Ah, right, thanks for the explanation, I missed that some machines have this 
device built in this way. Ok, patch makes perfectly sense now:

Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH 03/12] tests/qtest: Add dependence on PCIE_ROOT for virtio-net-failover.c

[Fabiano Rosas]

This test depends on the presence of the pcie-root-port device. Add a
build time dependency and a runtime check.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/meson.build           | 3 ++-
 tests/qtest/virtio-net-failover.c | 5 +++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index e97616d327..431b623df9 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -73,7 +73,8 @@ qtests_i386 = \
   (config_all_devices.has_key('CONFIG_ESP_PCI') ? ['am53c974-test'] : []) +                 \
   (config_host.has_key('CONFIG_POSIX') and                                                  \
    config_all_devices.has_key('CONFIG_ACPI_ERST') ? ['erst-test'] : []) +                   \
-  (config_all_devices.has_key('CONFIG_VIRTIO_NET') and                                      \
+  (config_all_devices.has_key('CONFIG_PCIE_ROOT') and                                       \
+   config_all_devices.has_key('CONFIG_VIRTIO_NET') and                                      \
    config_all_devices.has_key('CONFIG_Q35') and                                             \
    config_all_devices.has_key('CONFIG_VIRTIO_PCI') and                                      \
    slirp.found() ? ['virtio-net-failover'] : []) +                                          \
diff --git a/tests/qtest/virtio-net-failover.c b/tests/qtest/virtio-net-failover.c
index 4a809590bf..9ca52cb026 100644
--- a/tests/qtest/virtio-net-failover.c
+++ b/tests/qtest/virtio-net-failover.c
@@ -1835,6 +1835,11 @@ int main(int argc, char **argv)
     gchar *tmpfile;
     int ret;
 
+    if (!qtest_has_device("pcie-root-port") ||
+        !qtest_has_device("virtio-net")) {
+        return 0;
+    }
+
     g_test_init(&argc, &argv, NULL);
 
     ret = g_file_open_tmp("failover_test_migrate-XXXXXX", &tmpfile, NULL);
-- 
2.35.3

Re: [PATCH 03/12] tests/qtest: Add dependence on PCIE_ROOT for virtio-net-failover.c

[Thomas Huth]

On 06/02/2023 16.04, Fabiano Rosas wrote:
> This test depends on the presence of the pcie-root-port device. Add a
> build time dependency and a runtime check.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>   tests/qtest/meson.build           | 3 ++-
>   tests/qtest/virtio-net-failover.c | 5 +++++
>   2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> index e97616d327..431b623df9 100644
> --- a/tests/qtest/meson.build
> +++ b/tests/qtest/meson.build
> @@ -73,7 +73,8 @@ qtests_i386 = \
>     (config_all_devices.has_key('CONFIG_ESP_PCI') ? ['am53c974-test'] : []) +                 \
>     (config_host.has_key('CONFIG_POSIX') and                                                  \
>      config_all_devices.has_key('CONFIG_ACPI_ERST') ? ['erst-test'] : []) +                   \
> -  (config_all_devices.has_key('CONFIG_VIRTIO_NET') and                                      \
> +  (config_all_devices.has_key('CONFIG_PCIE_ROOT') and                                       \

CONFIG_PCIE_ROOT looks wrong ... hw/pci-bridge/meson.build uses 
CONFIG_PCIE_PORT instead, so I assume this is a typo?

Also, do we really need a run-time check if we already have a build-time check?

  Thomas

Re: [PATCH 03/12] tests/qtest: Add dependence on PCIE_ROOT for virtio-net-failover.c

[Fabiano Rosas]

Thomas Huth <thuth@redhat.com> writes:

> On 06/02/2023 16.04, Fabiano Rosas wrote:
>> This test depends on the presence of the pcie-root-port device. Add a
>> build time dependency and a runtime check.
>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>   tests/qtest/meson.build           | 3 ++-
>>   tests/qtest/virtio-net-failover.c | 5 +++++
>>   2 files changed, 7 insertions(+), 1 deletion(-)
>> 
>> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
>> index e97616d327..431b623df9 100644
>> --- a/tests/qtest/meson.build
>> +++ b/tests/qtest/meson.build
>> @@ -73,7 +73,8 @@ qtests_i386 = \
>>     (config_all_devices.has_key('CONFIG_ESP_PCI') ? ['am53c974-test'] : []) +                 \
>>     (config_host.has_key('CONFIG_POSIX') and                                                  \
>>      config_all_devices.has_key('CONFIG_ACPI_ERST') ? ['erst-test'] : []) +                   \
>> -  (config_all_devices.has_key('CONFIG_VIRTIO_NET') and                                      \
>> +  (config_all_devices.has_key('CONFIG_PCIE_ROOT') and                                       \
>
> CONFIG_PCIE_ROOT looks wrong ... hw/pci-bridge/meson.build uses 
> CONFIG_PCIE_PORT instead, so I assume this is a typo?
>
> Also, do we really need a run-time check if we already have a build-time check?

I guess not. It seems it was the typo that made it necessary. I'll fix
it.

[PATCH 04/12] tests/qtest: Skip virtio-serial-console tests if device not present

[Fabiano Rosas]

The virtconsole device might not be present in the QEMU build that is
being tested. Skip the test if that's the case.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/virtio-serial-test.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tests/qtest/virtio-serial-test.c b/tests/qtest/virtio-serial-test.c
index 2541034822..f4e05e8fdd 100644
--- a/tests/qtest/virtio-serial-test.c
+++ b/tests/qtest/virtio-serial-test.c
@@ -28,6 +28,10 @@ static void register_virtio_serial_test(void)
 {
     QOSGraphTestOptions opts = { };
 
+    if (!qtest_has_device("virtconsole")) {
+        return;
+    }
+
     opts.edge.before_cmd_line = "-device virtconsole,bus=vser0.0";
     qos_add_test("console-nop", "virtio-serial", virtio_serial_nop, &opts);
 
-- 
2.35.3

Re: [PATCH 04/12] tests/qtest: Skip virtio-serial-console tests if device not present

[Thomas Huth]

On 06/02/2023 16.04, Fabiano Rosas wrote:
> The virtconsole device might not be present in the QEMU build that is
> being tested. Skip the test if that's the case.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>   tests/qtest/virtio-serial-test.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/tests/qtest/virtio-serial-test.c b/tests/qtest/virtio-serial-test.c
> index 2541034822..f4e05e8fdd 100644
> --- a/tests/qtest/virtio-serial-test.c
> +++ b/tests/qtest/virtio-serial-test.c
> @@ -28,6 +28,10 @@ static void register_virtio_serial_test(void)
>   {
>       QOSGraphTestOptions opts = { };
>   
> +    if (!qtest_has_device("virtconsole")) {
> +        return;
> +    }
> +
>       opts.edge.before_cmd_line = "-device virtconsole,bus=vser0.0";
>       qos_add_test("console-nop", "virtio-serial", virtio_serial_nop, &opts);
>   

virtio-serial-test.c is a qos test - and this should detect automatically 
already whether a device is available and pluggable or not, I think.

There must be something else wrong here ... Laurent, Paolo, any ideas?

  Thomas

Re: [PATCH 04/12] tests/qtest: Skip virtio-serial-console tests if device not present

[Laurent Vivier]

On 2/7/23 14:25, Thomas Huth wrote:
> On 06/02/2023 16.04, Fabiano Rosas wrote:
>> The virtconsole device might not be present in the QEMU build that is
>> being tested. Skip the test if that's the case.
>>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>   tests/qtest/virtio-serial-test.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/tests/qtest/virtio-serial-test.c b/tests/qtest/virtio-serial-test.c
>> index 2541034822..f4e05e8fdd 100644
>> --- a/tests/qtest/virtio-serial-test.c
>> +++ b/tests/qtest/virtio-serial-test.c
>> @@ -28,6 +28,10 @@ static void register_virtio_serial_test(void)
>>   {
>>       QOSGraphTestOptions opts = { };
>> +    if (!qtest_has_device("virtconsole")) {
>> +        return;
>> +    }
>> +
>>       opts.edge.before_cmd_line = "-device virtconsole,bus=vser0.0";
>>       qos_add_test("console-nop", "virtio-serial", virtio_serial_nop, &opts);
> 
> virtio-serial-test.c is a qos test - and this should detect automatically already whether 
> a device is available and pluggable or not, I think.
> 
> There must be something else wrong here ... Laurent, Paolo, any ideas?

I think the checking of the device should be added where the device is added to the qos 
tree, in libqos/virtio-serial.c

If you don't add the device then the test is not started.

If I remember correctly, there was no autodetection of the devices, all are added statically.

Thanks,
Laurent

Re: [PATCH 04/12] tests/qtest: Skip virtio-serial-console tests if device not present

[Thomas Huth]

On 06/02/2023 16.04, Fabiano Rosas wrote:
> The virtconsole device might not be present in the QEMU build that is
> being tested. Skip the test if that's the case.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>   tests/qtest/virtio-serial-test.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/tests/qtest/virtio-serial-test.c b/tests/qtest/virtio-serial-test.c
> index 2541034822..f4e05e8fdd 100644
> --- a/tests/qtest/virtio-serial-test.c
> +++ b/tests/qtest/virtio-serial-test.c
> @@ -28,6 +28,10 @@ static void register_virtio_serial_test(void)
>   {
>       QOSGraphTestOptions opts = { };
>   
> +    if (!qtest_has_device("virtconsole")) {
> +        return;
> +    }

Maybe it makes more sense to check for CONFIG_VIRTIO_SERIAL in the 
meson.build file first?

  Thomas

[PATCH 05/12] tests/qtest: hd-geo-test: Check for missing devices

[Fabiano Rosas]

Don't include tests that require devices not available in the QEMU
binary.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/hd-geo-test.c | 38 +++++++++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 13 deletions(-)

diff --git a/tests/qtest/hd-geo-test.c b/tests/qtest/hd-geo-test.c
index 4a7628077b..5aa258a2b3 100644
--- a/tests/qtest/hd-geo-test.c
+++ b/tests/qtest/hd-geo-test.c
@@ -1090,30 +1090,42 @@ int main(int argc, char **argv)
         qtest_add_func("hd-geo/override/ide", test_override_ide);
         if (qtest_has_device("lsi53c895a")) {
             qtest_add_func("hd-geo/override/scsi", test_override_scsi);
-            qtest_add_func("hd-geo/override/scsi_2_controllers",
-                           test_override_scsi_2_controllers);
+            if (qtest_has_device("virtio-scsi-pci")) {
+                qtest_add_func("hd-geo/override/scsi_2_controllers",
+                               test_override_scsi_2_controllers);
+            }
         }
-        qtest_add_func("hd-geo/override/virtio_blk", test_override_virtio_blk);
         qtest_add_func("hd-geo/override/zero_chs", test_override_zero_chs);
-        qtest_add_func("hd-geo/override/scsi_hot_unplug",
-                       test_override_scsi_hot_unplug);
-        qtest_add_func("hd-geo/override/virtio_hot_unplug",
-                       test_override_virtio_hot_unplug);
+        if (qtest_has_device("virtio-scsi-pci")) {
+            qtest_add_func("hd-geo/override/scsi_hot_unplug",
+                           test_override_scsi_hot_unplug);
+        }
+        if (qtest_has_device("virtio-blk-pci")) {
+            qtest_add_func("hd-geo/override/virtio_hot_unplug",
+                           test_override_virtio_hot_unplug);
+            qtest_add_func("hd-geo/override/virtio_blk",
+                           test_override_virtio_blk);
+        }
 
         if (qtest_has_machine("q35")) {
             qtest_add_func("hd-geo/override/sata", test_override_sata);
-            qtest_add_func("hd-geo/override/virtio_blk_q35",
-                           test_override_virtio_blk_q35);
             qtest_add_func("hd-geo/override/zero_chs_q35",
                            test_override_zero_chs_q35);
             if (qtest_has_device("lsi53c895a")) {
                 qtest_add_func("hd-geo/override/scsi_q35",
                                test_override_scsi_q35);
             }
-            qtest_add_func("hd-geo/override/scsi_hot_unplug_q35",
-                           test_override_scsi_hot_unplug_q35);
-            qtest_add_func("hd-geo/override/virtio_hot_unplug_q35",
-                           test_override_virtio_hot_unplug_q35);
+            if (qtest_has_device("virtio-scsi-pci")) {
+                qtest_add_func("hd-geo/override/scsi_hot_unplug_q35",
+                               test_override_scsi_hot_unplug_q35);
+            }
+            if (qtest_has_device("virtio-blk-pci")) {
+                qtest_add_func("hd-geo/override/virtio_hot_unplug_q35",
+                               test_override_virtio_hot_unplug_q35);
+                qtest_add_func("hd-geo/override/virtio_blk_q35",
+                               test_override_virtio_blk_q35);
+            }
+
         }
     } else {
         g_test_message("QTEST_QEMU_IMG not set or qemu-img missing; "
-- 
2.35.3

Re: [PATCH 05/12] tests/qtest: hd-geo-test: Check for missing devices

[Thomas Huth]

On 06/02/2023 16.04, Fabiano Rosas wrote:
> Don't include tests that require devices not available in the QEMU
> binary.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>   tests/qtest/hd-geo-test.c | 38 +++++++++++++++++++++++++-------------
>   1 file changed, 25 insertions(+), 13 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH 06/12] tests/qtest: Skip unplug tests that use missing devices

[Fabiano Rosas]

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/device-plug-test.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/tests/qtest/device-plug-test.c b/tests/qtest/device-plug-test.c
index 5a6afa2b57..931acbdf50 100644
--- a/tests/qtest/device-plug-test.c
+++ b/tests/qtest/device-plug-test.c
@@ -67,6 +67,11 @@ static void test_pci_unplug_request(void)
     const char *arch = qtest_get_arch();
     const char *machine_addition = "";
 
+    if (!qtest_has_device("virtio-mouse-pci")) {
+        g_test_skip("Device virtio-mouse-pci not available");
+        return;
+    }
+
     if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
         machine_addition = "-machine pc";
     }
@@ -81,6 +86,10 @@ static void test_pci_unplug_request(void)
 
 static void test_q35_pci_unplug_request(void)
 {
+    if (!qtest_has_device("virtio-mouse-pci")) {
+        g_test_skip("Device virtio-mouse-pci not available");
+        return;
+    }
 
     QTestState *qtest = qtest_initf("-machine q35 "
                                     "-device pcie-root-port,id=p1 "
@@ -97,6 +106,11 @@ static void test_pci_unplug_json_request(void)
     const char *arch = qtest_get_arch();
     const char *machine_addition = "";
 
+    if (!qtest_has_device("virtio-mouse-pci")) {
+        g_test_skip("Device virtio-mouse-pci not available");
+        return;
+    }
+
     if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
         machine_addition = "-machine pc";
     }
@@ -123,6 +137,11 @@ static void test_q35_pci_unplug_json_request(void)
                                     "'bus': 'b1', "
                                     "'id': 'dev0'}\"";
 
+    if (!qtest_has_device("virtio-mouse-pci")) {
+        g_test_skip("Device virtio-mouse-pci not available");
+        return;
+    }
+
     QTestState *qtest = qtest_initf("-machine q35 %s %s %s",
                                     port, bridge, device);
 
-- 
2.35.3

Re: [PATCH 06/12] tests/qtest: Skip unplug tests that use missing devices

[Thomas Huth]

On 06/02/2023 16.04, Fabiano Rosas wrote:
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>   tests/qtest/device-plug-test.c | 19 +++++++++++++++++++
>   1 file changed, 19 insertions(+)
> 
> diff --git a/tests/qtest/device-plug-test.c b/tests/qtest/device-plug-test.c
> index 5a6afa2b57..931acbdf50 100644
> --- a/tests/qtest/device-plug-test.c
> +++ b/tests/qtest/device-plug-test.c
> @@ -67,6 +67,11 @@ static void test_pci_unplug_request(void)
>       const char *arch = qtest_get_arch();
>       const char *machine_addition = "";
>   
> +    if (!qtest_has_device("virtio-mouse-pci")) {
> +        g_test_skip("Device virtio-mouse-pci not available");
> +        return;
> +    }
> +
>       if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
>           machine_addition = "-machine pc";
>       }
> @@ -81,6 +86,10 @@ static void test_pci_unplug_request(void)
>   
>   static void test_q35_pci_unplug_request(void)
>   {
> +    if (!qtest_has_device("virtio-mouse-pci")) {
> +        g_test_skip("Device virtio-mouse-pci not available");
> +        return;
> +    }
>   
>       QTestState *qtest = qtest_initf("-machine q35 "
>                                       "-device pcie-root-port,id=p1 "

This seems to break the QEMU coding style ("Mixed declarations (interleaving 
statements and declarations within blocks) are generally not allowed; 
declarations should be at the beginning
of blocks.") ... could you separate the declaration of qtest from its 
initialization now, please?

  Thomas

Re: [PATCH 06/12] tests/qtest: Skip unplug tests that use missing devices

[Fabiano Rosas]

Thomas Huth <thuth@redhat.com> writes:

> On 06/02/2023 16.04, Fabiano Rosas wrote:
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>   tests/qtest/device-plug-test.c | 19 +++++++++++++++++++
>>   1 file changed, 19 insertions(+)
>> 
>> diff --git a/tests/qtest/device-plug-test.c b/tests/qtest/device-plug-test.c
>> index 5a6afa2b57..931acbdf50 100644
>> --- a/tests/qtest/device-plug-test.c
>> +++ b/tests/qtest/device-plug-test.c
>> @@ -67,6 +67,11 @@ static void test_pci_unplug_request(void)
>>       const char *arch = qtest_get_arch();
>>       const char *machine_addition = "";
>>   
>> +    if (!qtest_has_device("virtio-mouse-pci")) {
>> +        g_test_skip("Device virtio-mouse-pci not available");
>> +        return;
>> +    }
>> +
>>       if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
>>           machine_addition = "-machine pc";
>>       }
>> @@ -81,6 +86,10 @@ static void test_pci_unplug_request(void)
>>   
>>   static void test_q35_pci_unplug_request(void)
>>   {
>> +    if (!qtest_has_device("virtio-mouse-pci")) {
>> +        g_test_skip("Device virtio-mouse-pci not available");
>> +        return;
>> +    }
>>   
>>       QTestState *qtest = qtest_initf("-machine q35 "
>>                                       "-device pcie-root-port,id=p1 "
>
> This seems to break the QEMU coding style ("Mixed declarations (interleaving 
> statements and declarations within blocks) are generally not allowed; 
> declarations should be at the beginning
> of blocks.") ... could you separate the declaration of qtest from its 
> initialization now, please?

Ah well spotted, I got thrown off because some of these tests already
have a:

    if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
        machine_addition = "-machine pc";
    }

    QTestState *qtest = qtest_initf...

I'll fix those as well.

Re: [PATCH 06/12] tests/qtest: Skip unplug tests that use missing devices

[Thomas Huth]

On 07/02/2023 15.17, Fabiano Rosas wrote:
> Thomas Huth <thuth@redhat.com> writes:
> 
>> On 06/02/2023 16.04, Fabiano Rosas wrote:
>>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>>> ---
>>>    tests/qtest/device-plug-test.c | 19 +++++++++++++++++++
>>>    1 file changed, 19 insertions(+)
>>>
>>> diff --git a/tests/qtest/device-plug-test.c b/tests/qtest/device-plug-test.c
>>> index 5a6afa2b57..931acbdf50 100644
>>> --- a/tests/qtest/device-plug-test.c
>>> +++ b/tests/qtest/device-plug-test.c
>>> @@ -67,6 +67,11 @@ static void test_pci_unplug_request(void)
>>>        const char *arch = qtest_get_arch();
>>>        const char *machine_addition = "";
>>>    
>>> +    if (!qtest_has_device("virtio-mouse-pci")) {
>>> +        g_test_skip("Device virtio-mouse-pci not available");
>>> +        return;
>>> +    }
>>> +
>>>        if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
>>>            machine_addition = "-machine pc";
>>>        }
>>> @@ -81,6 +86,10 @@ static void test_pci_unplug_request(void)
>>>    
>>>    static void test_q35_pci_unplug_request(void)
>>>    {
>>> +    if (!qtest_has_device("virtio-mouse-pci")) {
>>> +        g_test_skip("Device virtio-mouse-pci not available");
>>> +        return;
>>> +    }
>>>    
>>>        QTestState *qtest = qtest_initf("-machine q35 "
>>>                                        "-device pcie-root-port,id=p1 "
>>
>> This seems to break the QEMU coding style ("Mixed declarations (interleaving
>> statements and declarations within blocks) are generally not allowed;
>> declarations should be at the beginning
>> of blocks.") ... could you separate the declaration of qtest from its
>> initialization now, please?
> 
> Ah well spotted, I got thrown off because some of these tests already
> have a:
> 
>      if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
>          machine_addition = "-machine pc";
>      }
> 
>      QTestState *qtest = qtest_initf...
> 
> I'll fix those as well.

Yes, please.

Actually, I wonder whether we could remove those "-machine pc" lines again, 
since "pc" is the default machine anyway. I think the original idea here was 
to get rid of the default machine on x86 or to switch it to q35, but that 
never happened, so this code seems superfluous now. Anyway, maybe rather 
something for a separate patch later...

  Thomas

[PATCH 07/12] tests/qtest: drive_del-test: Skip tests that require missing devices

[Fabiano Rosas]

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/drive_del-test.c | 70 ++++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/tests/qtest/drive_del-test.c b/tests/qtest/drive_del-test.c
index 9a750395a9..6fa96fa94a 100644
--- a/tests/qtest/drive_del-test.c
+++ b/tests/qtest/drive_del-test.c
@@ -16,6 +16,21 @@
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qlist.h"
 
+static const char *qvirtio_get_dev_type(void);
+
+/*
+ * This covers the possible absence of a device due to QEMU build
+ * options.
+ */
+static bool look_for_device_builtin(const char *prefix, const char *suffix)
+{
+    gchar *device = g_strdup_printf("%s-%s", prefix, suffix);
+    bool rc = qtest_has_device(device);
+
+    g_free(device);
+    return rc;
+}
+
 static bool look_for_drive0(QTestState *qts, const char *command, const char *key)
 {
     QDict *response;
@@ -40,6 +55,11 @@ static bool look_for_drive0(QTestState *qts, const char *command, const char *ke
     return found;
 }
 
+static bool has_device_builtin(const char *dev)
+{
+    return look_for_device_builtin(dev, qvirtio_get_dev_type());
+}
+
 static bool has_drive(QTestState *qts)
 {
     return look_for_drive0(qts, "query-block", "device");
@@ -208,6 +228,11 @@ static void test_drive_del_device_del(void)
 {
     QTestState *qts;
 
+    if (!has_device_builtin("virtio-scsi")) {
+        g_test_skip(NULL);
+        return;
+    }
+
     /* Start with a drive used by a device that unplugs instantaneously */
     qts = qtest_initf("-drive if=none,id=drive0,file=null-co://,"
                       "file.read-zeroes=on,format=raw"
@@ -232,6 +257,11 @@ static void test_cli_device_del(void)
     const char *arch = qtest_get_arch();
     const char *machine_addition = "";
 
+    if (!has_device_builtin("virtio-blk")) {
+        g_test_skip(NULL);
+        return;
+    }
+
     if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
         machine_addition = "-machine pc";
     }
@@ -256,6 +286,11 @@ static void test_cli_device_del_q35(void)
 {
     QTestState *qts;
 
+    if (!has_device_builtin("virtio-blk")) {
+        g_test_skip(NULL);
+        return;
+    }
+
     /*
      * -drive/-device and device_del.  Start with a drive used by a
      * device that unplugs after reset.
@@ -277,6 +312,11 @@ static void test_empty_device_del(void)
 {
     QTestState *qts;
 
+    if (!has_device_builtin("virtio-scsi")) {
+        g_test_skip(NULL);
+        return;
+    }
+
     /* device_del with no drive plugged.  */
     qts = qtest_initf("-device virtio-scsi-%s -device scsi-cd,id=dev0",
                       qvirtio_get_dev_type());
@@ -291,6 +331,11 @@ static void test_device_add_and_del(void)
     const char *arch = qtest_get_arch();
     const char *machine_addition = "";
 
+    if (!has_device_builtin("virtio-blk")) {
+        g_test_skip(NULL);
+        return;
+    }
+
     if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
         machine_addition = "-machine pc";
     }
@@ -330,6 +375,11 @@ static void test_device_add_and_del_q35(void)
 {
     QTestState *qts;
 
+    if (!has_device_builtin("virtio-blk")) {
+        g_test_skip(NULL);
+        return;
+    }
+
     /*
      * -drive/device_add and device_del.  Start with a drive used by a
      * device that unplugs after reset.
@@ -352,6 +402,11 @@ static void test_drive_add_device_add_and_del(void)
     const char *arch = qtest_get_arch();
     const char *machine_addition = "";
 
+    if (!has_device_builtin("virtio-blk")) {
+        g_test_skip(NULL);
+        return;
+    }
+
     if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
         machine_addition = "-machine pc";
     }
@@ -374,6 +429,11 @@ static void test_drive_add_device_add_and_del_q35(void)
 {
     QTestState *qts;
 
+    if (!has_device_builtin("virtio-blk")) {
+        g_test_skip(NULL);
+        return;
+    }
+
     qts = qtest_init("-machine q35 -device pcie-root-port,id=p1 "
                      "-device pcie-pci-bridge,bus=p1,id=b1");
 
@@ -395,6 +455,11 @@ static void test_blockdev_add_device_add_and_del(void)
     const char *arch = qtest_get_arch();
     const char *machine_addition = "";
 
+    if (!has_device_builtin("virtio-blk")) {
+        g_test_skip(NULL);
+        return;
+    }
+
     if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
         machine_addition = "-machine pc";
     }
@@ -417,6 +482,11 @@ static void test_blockdev_add_device_add_and_del_q35(void)
 {
     QTestState *qts;
 
+    if (!has_device_builtin("virtio-blk")) {
+        g_test_skip(NULL);
+        return;
+    }
+
     qts = qtest_init("-machine q35 -device pcie-root-port,id=p1 "
                      "-device pcie-pci-bridge,bus=p1,id=b1");
 
-- 
2.35.3

Re: [PATCH 07/12] tests/qtest: drive_del-test: Skip tests that require missing devices

[Thomas Huth]

On 06/02/2023 16.04, Fabiano Rosas wrote:
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>   tests/qtest/drive_del-test.c | 70 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 70 insertions(+)
> 
> diff --git a/tests/qtest/drive_del-test.c b/tests/qtest/drive_del-test.c
> index 9a750395a9..6fa96fa94a 100644
> --- a/tests/qtest/drive_del-test.c
> +++ b/tests/qtest/drive_del-test.c
> @@ -16,6 +16,21 @@
>   #include "qapi/qmp/qdict.h"
>   #include "qapi/qmp/qlist.h"
>   
> +static const char *qvirtio_get_dev_type(void);
> +
> +/*
> + * This covers the possible absence of a device due to QEMU build
> + * options.
> + */
> +static bool look_for_device_builtin(const char *prefix, const char *suffix)
> +{
> +    gchar *device = g_strdup_printf("%s-%s", prefix, suffix);
> +    bool rc = qtest_has_device(device);
> +
> +    g_free(device);
> +    return rc;
> +}

I think I'd rather merge the above code into the has_device_builtin() 
function below ... or is there a reason for keeping this separate?

>   static bool look_for_drive0(QTestState *qts, const char *command, const char *key)
>   {
>       QDict *response;
> @@ -40,6 +55,11 @@ static bool look_for_drive0(QTestState *qts, const char *command, const char *ke
>       return found;
>   }
>   
> +static bool has_device_builtin(const char *dev)
> +{
> +    return look_for_device_builtin(dev, qvirtio_get_dev_type());
> +}
> +
>   static bool has_drive(QTestState *qts)
>   {
>       return look_for_drive0(qts, "query-block", "device");
> @@ -208,6 +228,11 @@ static void test_drive_del_device_del(void)
>   {
>       QTestState *qts;
>   
> +    if (!has_device_builtin("virtio-scsi")) {
> +        g_test_skip(NULL);

Having a short message for the skip would be nice.

> +        return;
> +    }

  Thomas

Re: [PATCH 07/12] tests/qtest: drive_del-test: Skip tests that require missing devices

[Fabiano Rosas]

Thomas Huth <thuth@redhat.com> writes:

> On 06/02/2023 16.04, Fabiano Rosas wrote:
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>   tests/qtest/drive_del-test.c | 70 ++++++++++++++++++++++++++++++++++++
>>   1 file changed, 70 insertions(+)
>> 
>> diff --git a/tests/qtest/drive_del-test.c b/tests/qtest/drive_del-test.c
>> index 9a750395a9..6fa96fa94a 100644
>> --- a/tests/qtest/drive_del-test.c
>> +++ b/tests/qtest/drive_del-test.c
>> @@ -16,6 +16,21 @@
>>   #include "qapi/qmp/qdict.h"
>>   #include "qapi/qmp/qlist.h"
>>   
>> +static const char *qvirtio_get_dev_type(void);
>> +
>> +/*
>> + * This covers the possible absence of a device due to QEMU build
>> + * options.
>> + */
>> +static bool look_for_device_builtin(const char *prefix, const char *suffix)
>> +{
>> +    gchar *device = g_strdup_printf("%s-%s", prefix, suffix);
>> +    bool rc = qtest_has_device(device);
>> +
>> +    g_free(device);
>> +    return rc;
>> +}
>
> I think I'd rather merge the above code into the has_device_builtin() 
> function below ... or is there a reason for keeping this separate?

No reason, I'll merge them.

>>   static bool look_for_drive0(QTestState *qts, const char *command, const char *key)
>>   {
>>       QDict *response;
>> @@ -40,6 +55,11 @@ static bool look_for_drive0(QTestState *qts, const char *command, const char *ke
>>       return found;
>>   }
>>   
>> +static bool has_device_builtin(const char *dev)
>> +{
>> +    return look_for_device_builtin(dev, qvirtio_get_dev_type());
>> +}
>> +
>>   static bool has_drive(QTestState *qts)
>>   {
>>       return look_for_drive0(qts, "query-block", "device");
>> @@ -208,6 +228,11 @@ static void test_drive_del_device_del(void)
>>   {
>>       QTestState *qts;
>>   
>> +    if (!has_device_builtin("virtio-scsi")) {
>> +        g_test_skip(NULL);
>
> Having a short message for the skip would be nice.

ok.

[PATCH 08/12] tests/qtest: Check for devices in bios-tables-test

[Fabiano Rosas]

Do not include tests that require devices that are not available in
the QEMU build.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/bios-tables-test.c | 75 ++++++++++++++++++++++++++++++++--
 1 file changed, 71 insertions(+), 4 deletions(-)

diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
index d8c8cda58e..d29a4e47af 100644
--- a/tests/qtest/bios-tables-test.c
+++ b/tests/qtest/bios-tables-test.c
@@ -1008,6 +1008,12 @@ static void test_acpi_q35_multif_bridge(void)
         .machine = MACHINE_Q35,
         .variant = ".multi-bridge",
     };
+
+    if (!qtest_has_device("pcie-root-port")) {
+        g_test_skip("Device pcie-root-port is not available");
+        goto out;
+    }
+
     test_vm_prepare("-S"
         " -device virtio-balloon,id=balloon0,addr=0x4.0x2"
         " -device pcie-root-port,id=rp0,multifunction=on,"
@@ -1043,6 +1049,7 @@ static void test_acpi_q35_multif_bridge(void)
     /* check that reboot/reset doesn't change any ACPI tables  */
     qtest_qmp_send(data.qts, "{'execute':'system_reset' }");
     process_acpi_tables(&data);
+out:
     free_test_data(&data);
 }
 
@@ -1396,6 +1403,11 @@ static void test_acpi_tcg_dimm_pxm(const char *machine)
 {
     test_data data;
 
+    if (!qtest_has_device("nvdimm")) {
+        g_test_skip("Device nvdimm is not available");
+        return;
+    }
+
     memset(&data, 0, sizeof(data));
     data.machine = machine;
     data.variant = ".dimmpxm";
@@ -1444,6 +1456,11 @@ static void test_acpi_virt_tcg_memhp(void)
         .scan_len = 256ULL * 1024 * 1024,
     };
 
+    if (!qtest_has_device("nvdimm")) {
+        g_test_skip("Device nvdimm is not available");
+        goto out;
+    }
+
     data.variant = ".memhp";
     test_acpi_one(" -machine nvdimm=on"
                   " -cpu cortex-a57"
@@ -1457,7 +1474,7 @@ static void test_acpi_virt_tcg_memhp(void)
                   " -device pc-dimm,id=dimm0,memdev=ram2,node=0"
                   " -device nvdimm,id=dimm1,memdev=nvm0,node=1",
                   &data);
-
+out:
     free_test_data(&data);
 
 }
@@ -1475,6 +1492,11 @@ static void test_acpi_microvm_tcg(void)
 {
     test_data data;
 
+    if (!qtest_has_device("virtio-blk-device")) {
+        g_test_skip("Device virtio-blk-device is not available");
+        return;
+    }
+
     test_acpi_microvm_prepare(&data);
     test_acpi_one(" -machine microvm,acpi=on,ioapic2=off,rtc=off",
                   &data);
@@ -1485,6 +1507,11 @@ static void test_acpi_microvm_usb_tcg(void)
 {
     test_data data;
 
+    if (!qtest_has_device("virtio-blk-device")) {
+        g_test_skip("Device virtio-blk-device is not available");
+        return;
+    }
+
     test_acpi_microvm_prepare(&data);
     data.variant = ".usb";
     test_acpi_one(" -machine microvm,acpi=on,ioapic2=off,usb=on,rtc=off",
@@ -1496,6 +1523,11 @@ static void test_acpi_microvm_rtc_tcg(void)
 {
     test_data data;
 
+    if (!qtest_has_device("virtio-blk-device")) {
+        g_test_skip("Device virtio-blk-device is not available");
+        return;
+    }
+
     test_acpi_microvm_prepare(&data);
     data.variant = ".rtc";
     test_acpi_one(" -machine microvm,acpi=on,ioapic2=off,rtc=on",
@@ -1507,6 +1539,11 @@ static void test_acpi_microvm_pcie_tcg(void)
 {
     test_data data;
 
+    if (!qtest_has_device("virtio-blk-device")) {
+        g_test_skip("Device virtio-blk-device is not available");
+        return;
+    }
+
     test_acpi_microvm_prepare(&data);
     data.variant = ".pcie";
     data.tcg_only = true; /* need constant host-phys-bits */
@@ -1519,6 +1556,11 @@ static void test_acpi_microvm_ioapic2_tcg(void)
 {
     test_data data;
 
+    if (!qtest_has_device("virtio-blk-device")) {
+        g_test_skip("Device virtio-blk-device is not available");
+        return;
+    }
+
     test_acpi_microvm_prepare(&data);
     data.variant = ".ioapic2";
     test_acpi_one(" -machine microvm,acpi=on,ioapic2=on,rtc=off",
@@ -1558,6 +1600,12 @@ static void test_acpi_virt_tcg_pxb(void)
         .ram_start = 0x40000000ULL,
         .scan_len = 128ULL * 1024 * 1024,
     };
+
+    if (!qtest_has_device("pcie-root-port")) {
+        g_test_skip("Device pcie-root-port is not available");
+        goto out;
+    }
+
     /*
      * While using -cdrom, the cdrom would auto plugged into pxb-pcie,
      * the reason is the bus of pxb-pcie is also root bus, it would lead
@@ -1576,7 +1624,7 @@ static void test_acpi_virt_tcg_pxb(void)
                   " -cpu cortex-a57"
                   " -device pxb-pcie,bus_nr=128",
                   &data);
-
+out:
     free_test_data(&data);
 }
 
@@ -1764,6 +1812,12 @@ static void test_acpi_microvm_acpi_erst(void)
     gchar *params;
     test_data data;
 
+    if (!qtest_has_device("virtio-blk-device")) {
+        g_test_skip("Device virtio-blk-device is not available");
+        g_free(tmp_path);
+        return;
+    }
+
     test_acpi_microvm_prepare(&data);
     data.variant = ".pcie";
     data.tcg_only = true; /* need constant host-phys-bits */
@@ -1824,6 +1878,11 @@ static void test_acpi_q35_viot(void)
         .variant = ".viot",
     };
 
+    if (!qtest_has_device("virtio-iommu")) {
+        g_test_skip("Device virtio-iommu is not available");
+        goto out;
+    }
+
     /*
      * To keep things interesting, two buses bypass the IOMMU.
      * VIOT should only describes the other two buses.
@@ -1834,6 +1893,7 @@ static void test_acpi_q35_viot(void)
                   "-device pxb-pcie,bus_nr=0x20,id=pcie.200,bus=pcie.0,bypass_iommu=on "
                   "-device pxb-pcie,bus_nr=0x30,id=pcie.300,bus=pcie.0",
                   &data);
+out:
     free_test_data(&data);
 }
 
@@ -1894,8 +1954,10 @@ static void test_acpi_virt_viot(void)
         .scan_len = 128ULL * 1024 * 1024,
     };
 
-    test_acpi_one("-cpu cortex-a57 "
-                  "-device virtio-iommu-pci", &data);
+    if (qtest_has_device("virtio-iommu")) {
+        test_acpi_one("-cpu cortex-a57 "
+                       "-device virtio-iommu-pci", &data);
+    }
     free_test_data(&data);
 }
 
@@ -2004,6 +2066,11 @@ static void test_acpi_microvm_oem_fields(void)
     test_data data;
     char *args;
 
+    if (!qtest_has_device("virtio-blk-device")) {
+        g_test_skip("Device virtio-blk-device is not available");
+        return;
+    }
+
     test_acpi_microvm_prepare(&data);
 
     args = test_acpi_create_args(&data,
-- 
2.35.3

Re: [PATCH 08/12] tests/qtest: Check for devices in bios-tables-test

[Michael S. Tsirkin]

On Mon, Feb 06, 2023 at 12:04:12PM -0300, Fabiano Rosas wrote:
> Do not include tests that require devices that are not available in
> the QEMU build.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

feel free to merge with rest of the patchset.

> ---
>  tests/qtest/bios-tables-test.c | 75 ++++++++++++++++++++++++++++++++--
>  1 file changed, 71 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> index d8c8cda58e..d29a4e47af 100644
> --- a/tests/qtest/bios-tables-test.c
> +++ b/tests/qtest/bios-tables-test.c
> @@ -1008,6 +1008,12 @@ static void test_acpi_q35_multif_bridge(void)
>          .machine = MACHINE_Q35,
>          .variant = ".multi-bridge",
>      };
> +
> +    if (!qtest_has_device("pcie-root-port")) {
> +        g_test_skip("Device pcie-root-port is not available");
> +        goto out;
> +    }
> +
>      test_vm_prepare("-S"
>          " -device virtio-balloon,id=balloon0,addr=0x4.0x2"
>          " -device pcie-root-port,id=rp0,multifunction=on,"
> @@ -1043,6 +1049,7 @@ static void test_acpi_q35_multif_bridge(void)
>      /* check that reboot/reset doesn't change any ACPI tables  */
>      qtest_qmp_send(data.qts, "{'execute':'system_reset' }");
>      process_acpi_tables(&data);
> +out:
>      free_test_data(&data);
>  }
>  
> @@ -1396,6 +1403,11 @@ static void test_acpi_tcg_dimm_pxm(const char *machine)
>  {
>      test_data data;
>  
> +    if (!qtest_has_device("nvdimm")) {
> +        g_test_skip("Device nvdimm is not available");
> +        return;
> +    }
> +
>      memset(&data, 0, sizeof(data));
>      data.machine = machine;
>      data.variant = ".dimmpxm";
> @@ -1444,6 +1456,11 @@ static void test_acpi_virt_tcg_memhp(void)
>          .scan_len = 256ULL * 1024 * 1024,
>      };
>  
> +    if (!qtest_has_device("nvdimm")) {
> +        g_test_skip("Device nvdimm is not available");
> +        goto out;
> +    }
> +
>      data.variant = ".memhp";
>      test_acpi_one(" -machine nvdimm=on"
>                    " -cpu cortex-a57"
> @@ -1457,7 +1474,7 @@ static void test_acpi_virt_tcg_memhp(void)
>                    " -device pc-dimm,id=dimm0,memdev=ram2,node=0"
>                    " -device nvdimm,id=dimm1,memdev=nvm0,node=1",
>                    &data);
> -
> +out:
>      free_test_data(&data);
>  
>  }
> @@ -1475,6 +1492,11 @@ static void test_acpi_microvm_tcg(void)
>  {
>      test_data data;
>  
> +    if (!qtest_has_device("virtio-blk-device")) {
> +        g_test_skip("Device virtio-blk-device is not available");
> +        return;
> +    }
> +
>      test_acpi_microvm_prepare(&data);
>      test_acpi_one(" -machine microvm,acpi=on,ioapic2=off,rtc=off",
>                    &data);
> @@ -1485,6 +1507,11 @@ static void test_acpi_microvm_usb_tcg(void)
>  {
>      test_data data;
>  
> +    if (!qtest_has_device("virtio-blk-device")) {
> +        g_test_skip("Device virtio-blk-device is not available");
> +        return;
> +    }
> +
>      test_acpi_microvm_prepare(&data);
>      data.variant = ".usb";
>      test_acpi_one(" -machine microvm,acpi=on,ioapic2=off,usb=on,rtc=off",
> @@ -1496,6 +1523,11 @@ static void test_acpi_microvm_rtc_tcg(void)
>  {
>      test_data data;
>  
> +    if (!qtest_has_device("virtio-blk-device")) {
> +        g_test_skip("Device virtio-blk-device is not available");
> +        return;
> +    }
> +
>      test_acpi_microvm_prepare(&data);
>      data.variant = ".rtc";
>      test_acpi_one(" -machine microvm,acpi=on,ioapic2=off,rtc=on",
> @@ -1507,6 +1539,11 @@ static void test_acpi_microvm_pcie_tcg(void)
>  {
>      test_data data;
>  
> +    if (!qtest_has_device("virtio-blk-device")) {
> +        g_test_skip("Device virtio-blk-device is not available");
> +        return;
> +    }
> +
>      test_acpi_microvm_prepare(&data);
>      data.variant = ".pcie";
>      data.tcg_only = true; /* need constant host-phys-bits */
> @@ -1519,6 +1556,11 @@ static void test_acpi_microvm_ioapic2_tcg(void)
>  {
>      test_data data;
>  
> +    if (!qtest_has_device("virtio-blk-device")) {
> +        g_test_skip("Device virtio-blk-device is not available");
> +        return;
> +    }
> +
>      test_acpi_microvm_prepare(&data);
>      data.variant = ".ioapic2";
>      test_acpi_one(" -machine microvm,acpi=on,ioapic2=on,rtc=off",
> @@ -1558,6 +1600,12 @@ static void test_acpi_virt_tcg_pxb(void)
>          .ram_start = 0x40000000ULL,
>          .scan_len = 128ULL * 1024 * 1024,
>      };
> +
> +    if (!qtest_has_device("pcie-root-port")) {
> +        g_test_skip("Device pcie-root-port is not available");
> +        goto out;
> +    }
> +
>      /*
>       * While using -cdrom, the cdrom would auto plugged into pxb-pcie,
>       * the reason is the bus of pxb-pcie is also root bus, it would lead
> @@ -1576,7 +1624,7 @@ static void test_acpi_virt_tcg_pxb(void)
>                    " -cpu cortex-a57"
>                    " -device pxb-pcie,bus_nr=128",
>                    &data);
> -
> +out:
>      free_test_data(&data);
>  }
>  
> @@ -1764,6 +1812,12 @@ static void test_acpi_microvm_acpi_erst(void)
>      gchar *params;
>      test_data data;
>  
> +    if (!qtest_has_device("virtio-blk-device")) {
> +        g_test_skip("Device virtio-blk-device is not available");
> +        g_free(tmp_path);
> +        return;
> +    }
> +
>      test_acpi_microvm_prepare(&data);
>      data.variant = ".pcie";
>      data.tcg_only = true; /* need constant host-phys-bits */
> @@ -1824,6 +1878,11 @@ static void test_acpi_q35_viot(void)
>          .variant = ".viot",
>      };
>  
> +    if (!qtest_has_device("virtio-iommu")) {
> +        g_test_skip("Device virtio-iommu is not available");
> +        goto out;
> +    }
> +
>      /*
>       * To keep things interesting, two buses bypass the IOMMU.
>       * VIOT should only describes the other two buses.
> @@ -1834,6 +1893,7 @@ static void test_acpi_q35_viot(void)
>                    "-device pxb-pcie,bus_nr=0x20,id=pcie.200,bus=pcie.0,bypass_iommu=on "
>                    "-device pxb-pcie,bus_nr=0x30,id=pcie.300,bus=pcie.0",
>                    &data);
> +out:
>      free_test_data(&data);
>  }
>  
> @@ -1894,8 +1954,10 @@ static void test_acpi_virt_viot(void)
>          .scan_len = 128ULL * 1024 * 1024,
>      };
>  
> -    test_acpi_one("-cpu cortex-a57 "
> -                  "-device virtio-iommu-pci", &data);
> +    if (qtest_has_device("virtio-iommu")) {
> +        test_acpi_one("-cpu cortex-a57 "
> +                       "-device virtio-iommu-pci", &data);
> +    }
>      free_test_data(&data);
>  }
>  
> @@ -2004,6 +2066,11 @@ static void test_acpi_microvm_oem_fields(void)
>      test_data data;
>      char *args;
>  
> +    if (!qtest_has_device("virtio-blk-device")) {
> +        g_test_skip("Device virtio-blk-device is not available");
> +        return;
> +    }
> +
>      test_acpi_microvm_prepare(&data);
>  
>      args = test_acpi_create_args(&data,
> -- 
> 2.35.3

[PATCH 09/12] tests/qtest: Do not include hexloader-test if loader device is not present

[Fabiano Rosas]

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/hexloader-test.c | 5 +++++
 tests/qtest/meson.build      | 4 ++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/tests/qtest/hexloader-test.c b/tests/qtest/hexloader-test.c
index 3023548041..3ab464f438 100644
--- a/tests/qtest/hexloader-test.c
+++ b/tests/qtest/hexloader-test.c
@@ -22,6 +22,11 @@ static void hex_loader_test(void)
     unsigned int i;
     const unsigned int base_addr = 0x00010000;
 
+    if (!qtest_has_device("loader")) {
+        g_test_skip("Device 'loader' not available");
+        return;
+    }
+
     QTestState *s = qtest_initf(
         "-M vexpress-a9 -device loader,file=tests/data/hex-loader/test.hex");
 
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index 431b623df9..a930706a43 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -197,11 +197,11 @@ qtests_arm = \
   (config_all_devices.has_key('CONFIG_PFLASH_CFI02') ? ['pflash-cfi02-test'] : []) +         \
   (config_all_devices.has_key('CONFIG_ASPEED_SOC') ? qtests_aspeed : []) + \
   (config_all_devices.has_key('CONFIG_NPCM7XX') ? qtests_npcm7xx : []) + \
+  (config_all_devices.has_key('CONFIG_GENERIC_LOADER') ? ['hexloader-test'] : []) + \
   ['arm-cpu-features',
    'microbit-test',
    'test-arm-mptimer',
-   'boot-serial-test',
-   'hexloader-test']
+   'boot-serial-test']
 
 # TODO: once aarch64 TCG is fixed on ARM 32 bit host, make bios-tables-test unconditional
 qtests_aarch64 = \
-- 
2.35.3

Re: [PATCH 09/12] tests/qtest: Do not include hexloader-test if loader device is not present

[Thomas Huth]

On 06/02/2023 16.04, Fabiano Rosas wrote:
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>   tests/qtest/hexloader-test.c | 5 +++++
>   tests/qtest/meson.build      | 4 ++--
>   2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/qtest/hexloader-test.c b/tests/qtest/hexloader-test.c
> index 3023548041..3ab464f438 100644
> --- a/tests/qtest/hexloader-test.c
> +++ b/tests/qtest/hexloader-test.c
> @@ -22,6 +22,11 @@ static void hex_loader_test(void)
>       unsigned int i;
>       const unsigned int base_addr = 0x00010000;
>   
> +    if (!qtest_has_device("loader")) {
> +        g_test_skip("Device 'loader' not available");
> +        return;
> +    }
> +
>       QTestState *s = qtest_initf(
>           "-M vexpress-a9 -device loader,file=tests/data/hex-loader/test.hex");
>   
> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> index 431b623df9..a930706a43 100644
> --- a/tests/qtest/meson.build
> +++ b/tests/qtest/meson.build
> @@ -197,11 +197,11 @@ qtests_arm = \
>     (config_all_devices.has_key('CONFIG_PFLASH_CFI02') ? ['pflash-cfi02-test'] : []) +         \
>     (config_all_devices.has_key('CONFIG_ASPEED_SOC') ? qtests_aspeed : []) + \
>     (config_all_devices.has_key('CONFIG_NPCM7XX') ? qtests_npcm7xx : []) + \
> +  (config_all_devices.has_key('CONFIG_GENERIC_LOADER') ? ['hexloader-test'] : []) + \
>     ['arm-cpu-features',
>      'microbit-test',
>      'test-arm-mptimer',
> -   'boot-serial-test',
> -   'hexloader-test']
> +   'boot-serial-test']

In this case, I think the change to meson.build should be enough, since 
there is no machine that does "select GENERIC_LOADER" in the Kconfig files 
... or do I miss something again?

  Thomas

[PATCH 10/12] tests/qemu-iotests: Require virtio-scsi-pci

[Fabiano Rosas]

Check that virtio-scsi-pci is present in the QEMU build before running
the tests.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qemu-iotests/186 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/qemu-iotests/186 b/tests/qemu-iotests/186
index 072e54e62b..eaf13c7a33 100755
--- a/tests/qemu-iotests/186
+++ b/tests/qemu-iotests/186
@@ -40,6 +40,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 _supported_fmt qcow2
 _supported_proto file fuse
 _require_drivers null-co
+_require_devices virtio-scsi-pci
 
 if [ "$QEMU_DEFAULT_MACHINE" != "pc" ]; then
     _notrun "Requires a PC machine"
-- 
2.35.3

Re: [PATCH 10/12] tests/qemu-iotests: Require virtio-scsi-pci

[Thomas Huth]

On 06/02/2023 16.04, Fabiano Rosas wrote:
> Check that virtio-scsi-pci is present in the QEMU build before running
> the tests.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>   tests/qemu-iotests/186 | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/tests/qemu-iotests/186 b/tests/qemu-iotests/186
> index 072e54e62b..eaf13c7a33 100755
> --- a/tests/qemu-iotests/186
> +++ b/tests/qemu-iotests/186
> @@ -40,6 +40,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>   _supported_fmt qcow2
>   _supported_proto file fuse
>   _require_drivers null-co
> +_require_devices virtio-scsi-pci
>   
>   if [ "$QEMU_DEFAULT_MACHINE" != "pc" ]; then
>       _notrun "Requires a PC machine"

Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH 11/12] tests/qtest: bios-tables-test: Skip if missing configs

[Fabiano Rosas]

If we build with --without-default-devices, CONFIG_HPET and
CONFIG_PARALLEL are set to N, which makes the respective devices go
missing from acpi tables.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
I currently don't see a way of allowing the tests to pass in the
absence of these two configs. As far as I understand, we would need to
have one set of expected table files (tests/data/acpi) for each
combination of machine vs. possible CONFIG that can be toggled.

Any ideas?
---
 tests/qtest/meson.build | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index a930706a43..2829eda2c9 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -78,7 +78,9 @@ qtests_i386 = \
    config_all_devices.has_key('CONFIG_Q35') and                                             \
    config_all_devices.has_key('CONFIG_VIRTIO_PCI') and                                      \
    slirp.found() ? ['virtio-net-failover'] : []) +                                          \
-  (unpack_edk2_blobs ? ['bios-tables-test'] : []) +                                         \
+  (unpack_edk2_blobs and                                                                    \
+   config_all_devices.has_key('CONFIG_HPET') and                                            \
+   config_all_devices.has_key('CONFIG_PARALLEL') ? ['bios-tables-test'] : []) +             \
   qtests_pci +                                                                              \
   qtests_cxl +                                                                              \
   ['fdc-test',
-- 
2.35.3

Re: [PATCH 11/12] tests/qtest: bios-tables-test: Skip if missing configs

[Thomas Huth]

On 06/02/2023 16.04, Fabiano Rosas wrote:
> If we build with --without-default-devices, CONFIG_HPET and
> CONFIG_PARALLEL are set to N, which makes the respective devices go
> missing from acpi tables.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
> I currently don't see a way of allowing the tests to pass in the
> absence of these two configs. As far as I understand, we would need to
> have one set of expected table files (tests/data/acpi) for each
> combination of machine vs. possible CONFIG that can be toggled.

I think you're right ... maintaining tables for each combination does not 
scale. Disabling the test in that case is likely the best we can do here 
right now.

> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> index a930706a43..2829eda2c9 100644
> --- a/tests/qtest/meson.build
> +++ b/tests/qtest/meson.build
> @@ -78,7 +78,9 @@ qtests_i386 = \
>      config_all_devices.has_key('CONFIG_Q35') and                                             \
>      config_all_devices.has_key('CONFIG_VIRTIO_PCI') and                                      \
>      slirp.found() ? ['virtio-net-failover'] : []) +                                          \
> -  (unpack_edk2_blobs ? ['bios-tables-test'] : []) +                                         \
> +  (unpack_edk2_blobs and                                                                    \
> +   config_all_devices.has_key('CONFIG_HPET') and                                            \
> +   config_all_devices.has_key('CONFIG_PARALLEL') ? ['bios-tables-test'] : []) +             \
>     qtests_pci +                                                                              \
>     qtests_cxl +                                                                              \
>     ['fdc-test',

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 11/12] tests/qtest: bios-tables-test: Skip if missing configs

[Michael S. Tsirkin]

On Tue, Feb 07, 2023 at 03:35:56PM +0100, Thomas Huth wrote:
> On 06/02/2023 16.04, Fabiano Rosas wrote:
> > If we build with --without-default-devices, CONFIG_HPET and
> > CONFIG_PARALLEL are set to N, which makes the respective devices go
> > missing from acpi tables.
> > 
> > Signed-off-by: Fabiano Rosas <farosas@suse.de>
> > ---
> > I currently don't see a way of allowing the tests to pass in the
> > absence of these two configs. As far as I understand, we would need to
> > have one set of expected table files (tests/data/acpi) for each
> > combination of machine vs. possible CONFIG that can be toggled.
> 
> I think you're right ... maintaining tables for each combination does not
> scale. Disabling the test in that case is likely the best we can do here
> right now.
> 
> > diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> > index a930706a43..2829eda2c9 100644
> > --- a/tests/qtest/meson.build
> > +++ b/tests/qtest/meson.build
> > @@ -78,7 +78,9 @@ qtests_i386 = \
> >      config_all_devices.has_key('CONFIG_Q35') and                                             \
> >      config_all_devices.has_key('CONFIG_VIRTIO_PCI') and                                      \
> >      slirp.found() ? ['virtio-net-failover'] : []) +                                          \
> > -  (unpack_edk2_blobs ? ['bios-tables-test'] : []) +                                         \
> > +  (unpack_edk2_blobs and                                                                    \
> > +   config_all_devices.has_key('CONFIG_HPET') and                                            \
> > +   config_all_devices.has_key('CONFIG_PARALLEL') ? ['bios-tables-test'] : []) +             \
> >     qtests_pci +                                                                              \
> >     qtests_cxl +                                                                              \
> >     ['fdc-test',
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>


One thing we could do is move this code to an SSDT by itself.  Then
there's two variants of e.g. HPET SSDT: with and without CONFIG_HPET.
Needs ACPI work though. Igor what do you think? Worth it?


-- 
MST

Re: [PATCH 11/12] tests/qtest: bios-tables-test: Skip if missing configs

[Igor Mammedov]

On Tue, 7 Feb 2023 09:42:45 -0500
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Feb 07, 2023 at 03:35:56PM +0100, Thomas Huth wrote:
> > On 06/02/2023 16.04, Fabiano Rosas wrote:  
> > > If we build with --without-default-devices, CONFIG_HPET and
> > > CONFIG_PARALLEL are set to N, which makes the respective devices go
> > > missing from acpi tables.
> > > 
> > > Signed-off-by: Fabiano Rosas <farosas@suse.de>
> > > ---
> > > I currently don't see a way of allowing the tests to pass in the
> > > absence of these two configs. As far as I understand, we would need to
> > > have one set of expected table files (tests/data/acpi) for each
> > > combination of machine vs. possible CONFIG that can be toggled.  
> > 
> > I think you're right ... maintaining tables for each combination does not
> > scale. Disabling the test in that case is likely the best we can do here
> > right now.
> >   
> > > diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> > > index a930706a43..2829eda2c9 100644
> > > --- a/tests/qtest/meson.build
> > > +++ b/tests/qtest/meson.build
> > > @@ -78,7 +78,9 @@ qtests_i386 = \
> > >      config_all_devices.has_key('CONFIG_Q35') and                                             \
> > >      config_all_devices.has_key('CONFIG_VIRTIO_PCI') and                                      \
> > >      slirp.found() ? ['virtio-net-failover'] : []) +                                          \
> > > -  (unpack_edk2_blobs ? ['bios-tables-test'] : []) +                                         \
> > > +  (unpack_edk2_blobs and                                                                    \
> > > +   config_all_devices.has_key('CONFIG_HPET') and                                            \
> > > +   config_all_devices.has_key('CONFIG_PARALLEL') ? ['bios-tables-test'] : []) +             \
> > >     qtests_pci +                                                                              \
> > >     qtests_cxl +                                                                              \
> > >     ['fdc-test',  
> > 
> > Reviewed-by: Thomas Huth <thuth@redhat.com>  
> 
> 
> One thing we could do is move this code to an SSDT by itself.  Then
> there's two variants of e.g. HPET SSDT: with and without CONFIG_HPET.
> Needs ACPI work though. Igor what do you think? Worth it?

I'd go with just disabling test in this case.

having dedicated ACPI tables for each permutation config changes
might cause doesn't look to me as sustainable.

Re: [PATCH 11/12] tests/qtest: bios-tables-test: Skip if missing configs

[Michael S. Tsirkin]

On Wed, Feb 08, 2023 at 11:52:38AM +0100, Igor Mammedov wrote:
> On Tue, 7 Feb 2023 09:42:45 -0500
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, Feb 07, 2023 at 03:35:56PM +0100, Thomas Huth wrote:
> > > On 06/02/2023 16.04, Fabiano Rosas wrote:  
> > > > If we build with --without-default-devices, CONFIG_HPET and
> > > > CONFIG_PARALLEL are set to N, which makes the respective devices go
> > > > missing from acpi tables.
> > > > 
> > > > Signed-off-by: Fabiano Rosas <farosas@suse.de>
> > > > ---
> > > > I currently don't see a way of allowing the tests to pass in the
> > > > absence of these two configs. As far as I understand, we would need to
> > > > have one set of expected table files (tests/data/acpi) for each
> > > > combination of machine vs. possible CONFIG that can be toggled.  
> > > 
> > > I think you're right ... maintaining tables for each combination does not
> > > scale. Disabling the test in that case is likely the best we can do here
> > > right now.
> > >   
> > > > diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> > > > index a930706a43..2829eda2c9 100644
> > > > --- a/tests/qtest/meson.build
> > > > +++ b/tests/qtest/meson.build
> > > > @@ -78,7 +78,9 @@ qtests_i386 = \
> > > >      config_all_devices.has_key('CONFIG_Q35') and                                             \
> > > >      config_all_devices.has_key('CONFIG_VIRTIO_PCI') and                                      \
> > > >      slirp.found() ? ['virtio-net-failover'] : []) +                                          \
> > > > -  (unpack_edk2_blobs ? ['bios-tables-test'] : []) +                                         \
> > > > +  (unpack_edk2_blobs and                                                                    \
> > > > +   config_all_devices.has_key('CONFIG_HPET') and                                            \
> > > > +   config_all_devices.has_key('CONFIG_PARALLEL') ? ['bios-tables-test'] : []) +             \
> > > >     qtests_pci +                                                                              \
> > > >     qtests_cxl +                                                                              \
> > > >     ['fdc-test',  
> > > 
> > > Reviewed-by: Thomas Huth <thuth@redhat.com>  
> > 
> > 
> > One thing we could do is move this code to an SSDT by itself.  Then
> > there's two variants of e.g. HPET SSDT: with and without CONFIG_HPET.
> > Needs ACPI work though. Igor what do you think? Worth it?
> 
> I'd go with just disabling test in this case.

ok for now.

> having dedicated ACPI tables for each permutation config changes
> might cause doesn't look to me as sustainable.
> 

I feel I was unclear. What I am proposing is not that we add SSDT
for each permutation.

What I am saying is basically this: we have build_hpet_aml.  call it
from a separate SSDT. Now with HPET we check this expected SSDT.
Without HPET we don't have this SSDT so nothing to check.


-- 
MST

[PATCH 12/12] [NOT FOR MERGE] tests/qtest: Introduce qtest_validate_args

[Fabiano Rosas]

The QEMU binary can be built with a varied set of features/devices
which are opaque to the tests. Add a centralized point for parsing and
validating the command line.

Tests can now be skipped with the following pattern:

qts = qtest_init(args);
if (!qts) {
    return;
}

For now, the only validation is that the -device options all
correspond to devices that are actually present in the build.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
Would this be better than checking for missing devices in individual
tests?
---
 tests/qtest/libqtest.c | 137 ++++++++++++++++++++++++++++++++++++++++-
 tests/qtest/libqtest.h |  12 ++++
 2 files changed, 148 insertions(+), 1 deletion(-)

diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index d658222a19..7920fd1506 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -479,10 +479,145 @@ QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
     return s;
 }
 
+enum qemu_options {
+    DEVICE = G_TOKEN_LAST + 1,
+    DRIVER = G_TOKEN_LAST + 2,
+};
+
+static void _add_option(GHashTable *ht, const char *key, const char *val)
+{
+    GList *list = g_hash_table_lookup(ht, key);
+
+    if (!list) {
+        list = g_list_append(list, g_strdup(val));
+        g_hash_table_insert(ht, g_strdup(key), list);
+    } else {
+        list = g_list_append(list, g_strdup(val));
+    }
+}
+
+static void _parse_device_json(GHashTable *opts, GScanner *top_scanner)
+{
+    GScanner *scanner = g_scanner_new(top_scanner->config);
+    gchar *text;
+
+    assert(top_scanner->token == G_TOKEN_STRING);
+    text = g_strdup(top_scanner->value.v_string);
+
+    g_scanner_scope_add_symbol(scanner, 0, "driver", GINT_TO_POINTER(DRIVER));
+    g_scanner_input_text(scanner, text, strlen(text));
+
+    do {
+        g_scanner_get_next_token(scanner);
+        switch ((enum qemu_options)scanner->token) {
+        case DRIVER:
+            /* -device "{'driver':'dev' */
+            g_scanner_get_next_token(scanner);
+
+            switch (scanner->token) {
+            case G_TOKEN_IDENTIFIER:
+                _add_option(opts, "devices", scanner->value.v_string);
+                break;
+
+            default: /* invalid */
+                _add_option(opts, "devices", NULL);
+            }
+            break;
+        default:
+            break;
+        }
+        g_scanner_peek_next_token(scanner);
+    } while (scanner->next_token != G_TOKEN_EOF &&
+             scanner->next_token != G_TOKEN_ERROR);
+
+    g_scanner_destroy(scanner);
+    g_free(text);
+}
+
+static void qtest_parse_args(GHashTable *opts, const char *args)
+{
+    GScanner *scanner = g_scanner_new(NULL);
+
+    scanner->input_name = "qtest args";
+    scanner->config->symbol_2_token = 1;
+    scanner->config->scan_float = 0;
+    scanner->config->scan_string_sq = 0;
+    scanner->config->cset_skip_characters = g_strdup(" \t\n':");
+    scanner->config->cset_identifier_first = g_strdup("-" G_CSET_a_2_z
+                                                      G_CSET_A_2_Z
+                                                      G_CSET_DIGITS),
+    scanner->config->cset_identifier_nth = g_strdup("-_." G_CSET_a_2_z
+                                                    G_CSET_A_2_Z G_CSET_DIGITS),
+
+    g_scanner_scope_add_symbol(scanner, 0, "-device", GINT_TO_POINTER(DEVICE));
+
+    g_scanner_input_text(scanner, args, strlen(args));
+
+    do {
+        g_scanner_get_next_token(scanner);
+
+        switch ((enum qemu_options)scanner->token) {
+        case DEVICE:
+            g_scanner_get_next_token(scanner);
+
+            switch (scanner->token) {
+            case G_TOKEN_IDENTIFIER: /* -device dev */
+                _add_option(opts, "devices", scanner->value.v_string);
+                break;
+
+            case G_TOKEN_STRING: /* -device "{'driver':'dev' */
+                _parse_device_json(opts, scanner);
+                break;
+
+            default: /* invalid */
+                _add_option(opts, "devices", NULL);
+            }
+            break;
+        default:
+            break;
+        }
+        g_scanner_peek_next_token(scanner);
+    } while (scanner->next_token != G_TOKEN_EOF &&
+             scanner->next_token != G_TOKEN_ERROR);
+
+    g_scanner_destroy(scanner);
+}
+
+bool qtest_validate_args(const char *args, char **msg)
+{
+    GHashTable *opts = g_hash_table_new(g_str_hash, g_str_equal);
+    GList *l;
+    bool rc = true;
+
+    qtest_parse_args(opts, args);
+
+    for (l = g_hash_table_lookup(opts, "devices"); l != NULL; l = l->next) {
+        if (!l->data || !qtest_has_device(l->data)) {
+            *msg = g_strdup_printf("Device %s is not available",
+                                   (char *)l->data);
+            rc = false;
+            break;
+        }
+    }
+    g_hash_table_unref(opts);
+    return rc;
+}
+
 QTestState *qtest_init(const char *extra_args)
 {
-    QTestState *s = qtest_init_without_qmp_handshake(extra_args);
+    QTestState *s;
     QDict *greeting;
+/*
+ *   char *err_msg;
+ *
+ *    if (!qtest_validate_args(extra_args, &err_msg)) {
+ *        g_test_skip(err_msg);
+ *        g_free(err_msg);
+ *
+ *        return NULL;
+ *    }
+ */
+    s = qtest_init_without_qmp_handshake(extra_args);
 
     /* Read the QMP greeting and then do the handshake */
     greeting = qtest_qmp_receive(s);
diff --git a/tests/qtest/libqtest.h b/tests/qtest/libqtest.h
index fcf1c3c3b3..01a07c448a 100644
--- a/tests/qtest/libqtest.h
+++ b/tests/qtest/libqtest.h
@@ -832,4 +832,16 @@ void qtest_qom_set_bool(QTestState *s, const char *path, const char *property,
  * Returns: Value retrieved from property.
  */
 bool qtest_qom_get_bool(QTestState *s, const char *path, const char *property);
+
+/**
+ * qtest_validate_args:
+ * @args: arguments to validate, exactly as they would be passed
+ * into qtest_init.
+ * @err_msg: String with the reason for the failure, if any.
+ *
+ * Validates the command line (args) for options that are incompatible
+ * with the current QEMU build.
+ */
+bool qtest_validate_args(const char *args, char **err_msg);
+
 #endif
-- 
2.35.3

Re: [PATCH 12/12] [NOT FOR MERGE] tests/qtest: Introduce qtest_validate_args

[Thomas Huth]

On 06/02/2023 16.04, Fabiano Rosas wrote:
> The QEMU binary can be built with a varied set of features/devices
> which are opaque to the tests. Add a centralized point for parsing and
> validating the command line.
> 
> Tests can now be skipped with the following pattern:
> 
> qts = qtest_init(args);
> if (!qts) {
>      return;
> }
> 
> For now, the only validation is that the -device options all
> correspond to devices that are actually present in the build.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
> Would this be better than checking for missing devices in individual
> tests?

This is certainly an interesting idea! ... some things still bug me, though:

- We still need to change all the calling sites (to check for
   !qts) ... so the effort seems to be in a similar ballpark as
   adding qtest_has_device() to the various problematic tests

- This will now call qtest_has_device for each and every device
   in the parameter list, even if it is not necessary. And at
   least the first call to qtest_has_device() is rather expensive
   since it has to fire up a separate QEMU to retrieve the list
   of supported the devices. So adding this to all tests might
   cause a slow-down to the tests...

- It could maybe even hide bugs if you don't look closely, e.g.
   if you have a typo in the device name in a test, the test then
   gets skipped automatically instead of failing ... ok, that's
   unlikely for new tests where you look closely, but still, it
   gives me slightly bad feeling.

So I think I rather tend to go for explicit calls to qtest_has_device() as 
you did in your first 11 patches.

Anyway, I'm interested in what do others think of this? Any other opinions?

  Thomas

Re: [PATCH 12/12] [NOT FOR MERGE] tests/qtest: Introduce qtest_validate_args

[Fabiano Rosas]

Thomas Huth <thuth@redhat.com> writes:

> On 06/02/2023 16.04, Fabiano Rosas wrote:
>> The QEMU binary can be built with a varied set of features/devices
>> which are opaque to the tests. Add a centralized point for parsing and
>> validating the command line.
>> 
>> Tests can now be skipped with the following pattern:
>> 
>> qts = qtest_init(args);
>> if (!qts) {
>>      return;
>> }
>> 
>> For now, the only validation is that the -device options all
>> correspond to devices that are actually present in the build.
>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>> Would this be better than checking for missing devices in individual
>> tests?
>
> This is certainly an interesting idea! ... some things still bug me, though:
>
> - We still need to change all the calling sites (to check for
>    !qts) ... so the effort seems to be in a similar ballpark as
>    adding qtest_has_device() to the various problematic tests

Just notice that this series does not cover _all_ -device uses, only the
ones that refer to devices disabled by --without-default-devices. So we
might need to come back to this when some CONFIG changes, new devices
are added, new tests, etc.

> - This will now call qtest_has_device for each and every device
>    in the parameter list, even if it is not necessary. And at
>    least the first call to qtest_has_device() is rather expensive
>    since it has to fire up a separate QEMU to retrieve the list
>    of supported the devices. So adding this to all tests might
>    cause a slow-down to the tests...

Yes, that was my main concern. We could have something like this patch
but as a helper that tests can call. Initially, I had thought of:

if (qtest_validate_args(args)) {
   qts = qtest_init(args);
}

> - It could maybe even hide bugs if you don't look closely, e.g.
>    if you have a typo in the device name in a test, the test then
>    gets skipped automatically instead of failing ... ok, that's
>    unlikely for new tests where you look closely, but still, it
>    gives me slightly bad feeling.

I agree. In fact I have been looking into making the same change (as
this patch) in the avocado tests, which of course all fail
without-default-devices. There it's considerably simpler (because
Python), but I'm still thinking about how to avoid hiding a legitimate
failure.

> So I think I rather tend to go for explicit calls to qtest_has_device() as 
> you did in your first 11 patches.

Ok, I'll send just them for v2, unless anyone else has something to say.