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