Re: -device T,help crashes

[Markus Armbruster <armbru@redhat.com>]

Thomas Huth <thuth@redhat.com> writes:

> On 17/03/2026 08.35, Markus Armbruster wrote:
>> Thomas Huth <thuth@redhat.com> writes:
>> 
>>> On 12/03/2026 09.15, Markus Armbruster wrote:
>>>> Markus Armbruster <armbru@redhat.com> writes:
>>> ...
>>>>> These crashes escape tests/qtest/device-introspect-test, because it
>>>>> covers only HMP "device_add T,help", not CLI "-device T,help".
>>>>
>>>> Anyone wants to cook up a patch to cover -device?
>>>
>>> Quick'n'dirty patch:
>>>
>>> diff --git a/tests/qtest/device-introspect-test.c b/tests/qtest/device-introspect-test.c
>>> --- a/tests/qtest/device-introspect-test.c
>>> +++ b/tests/qtest/device-introspect-test.c
>>> @@ -105,6 +105,7 @@ static void test_one_device(QTestState *qts, const char *type)
>>>       QDict *resp;
>>>       char *help, *escaped;
>>>       GRegex *comma;
>>> +    int ret;
>>>
>>>       g_test_message("Testing device '%s'", type);
>>>
>>> @@ -119,6 +120,17 @@ static void test_one_device(QTestState *qts, const char *type)
>>>
>>>       help = qtest_hmp(qts, "device_add \"%s,help\"", escaped);
>>>       g_free(help);
>>> +
>>> +    if (!g_str_equal(type, "nonexistent") && !g_str_equal(type, "device")) {
>>> +        help = g_strdup_printf("%s -device \"%s,help\" > /dev/null", qtest_qemu_binary(NULL), escaped);
>>> +        ret = system(help);
>> 
>> Not using qtest_init() or similar.  Hmm.
>
> I mentioned it's quick-n-dirty ;-)

Fair, fair :)

>>> +        g_free(help);
>>> +        if (ret) {
>>> +            fprintf(stderr, "Running qemu with -device %s,help failed!\n", escaped);
>>> +            abort();
>>> +        }
>>> +    }
>>> +
>>>       g_free(escaped);
>>>   }
>>>
>>> But I think it needs some more work - we likely don't want to run this for each test run since it slows down the test quite a bit. And the g_test_quick() switch is already used for something else here.
>> 
>> It's used to test with every machine type, not just with "none".
>> 
>> I think we could also use it to additionally test -device just with
>> "none".  Testing it with every machine type would be even slower, and I
>> doubt it's worthwhile.
>
> Agreed.
>
>> Another idea to conserve cycles: use a unit test instead.
>
> Unit tests don't have a way yet to determine which qemu-system-xyz binaries 
> are available, do they? ... so that would need some more logic in the 
> meson.build file there - not sure whether that's worth the effort, I think 
> the device-introspect qtest is likely a better place?

A unit test isn't supposed to run qemu-system-xyz.  Instead, it mocks up
the environment and just runs "the unit".  In this case,
qdev_device_help().

This is less protection than a qtest, but it can also be a lot faster.

> Or maybe add it to scripts/device-crash-test ?

That's another option.

[...]