Re: [PATCH v4 10/12] qtest/qmp-cmd-test: Make test build-independent from accelerator

From: Markus Armbruster <armbru@redhat.com>
To: "Philippe Mathieu-Daudé" <philmd@redhat.com>
Cc: "Laurent Vivier" <lvivier@redhat.com>,
	"Peter Maydell" <peter.maydell@linaro.org>,
	"Thomas Huth" <thuth@redhat.com>,
	"Daniel P.Berrangé" <berrange@redhat.com>,
	"Eduardo Habkost" <ehabkost@redhat.com>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	"Juan Quintela" <quintela@redhat.com>,
	"Richard Henderson" <richard.henderson@linaro.org>,
	"Andrew Jones" <drjones@redhat.com>,
	qemu-devel@nongnu.org, qemu-arm@nongnu.org,
	"Claudio Fontana" <cfontana@suse.de>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Igor Mammedov" <imammedo@redhat.com>,
	"Dr. David Alan Gilbert" <dgilbert@redhat.com>
Subject: Re: [PATCH v4 10/12] qtest/qmp-cmd-test: Make test build-independent from accelerator
Date: Sat, 01 May 2021 08:49:05 +0200	[thread overview]
Message-ID: <87fsz7kkym.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <32bc66bc-1ccf-c146-52ac-83c7b4913930@redhat.com> ("Philippe =?utf-8?Q?Mathieu-Daud=C3=A9=22's?= message of "Fri, 30 Apr 2021 17:48:05 +0200")

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 4/30/21 8:13 AM, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>>> On 4/29/21 3:22 PM, Markus Armbruster wrote:
>>>> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>>>>>>> Now than we can probe if the TCG accelerator is available
>>>>>>> at runtime with a QMP command, do it once at the beginning
>>>>>>> and only register the tests we can run.
>>>>>>> We can then replace the #ifdef'ry by a runtime check.
>>>>>>>
>>>>>>> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
>>>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>>>>
>>>>>> Please read the last remark first.  The other ones are detail; feel free
>>>>>> to skip them until we're done with the last one.
>>>>>>
>>>>>>> ---
>>>>>>>  tests/qtest/qmp-cmd-test.c | 18 ++++++++++++++----
>>>>>>>  1 file changed, 14 insertions(+), 4 deletions(-)
>>>>>
>>>>>>> +    tcg_accel_available = qtest_has_accel("tcg");
>>>>>>> +
>>>>>>
>>>>>> When does tcg_accel_available differ from defined(CONFIG_TCG)?
>>>>>
>>>>> qtest_has_accel("tcg") is a runtime check, while defined(CONFIG_TCG)
>>>>> is build-time.
>>>>
>>>> Let me rephrase my question: under what conditions can the values of
>>>> qtest_has_accel("tcg") and defined(CONFIG_TCG) differ?
>>>
>>> They are currently the same, so this patch is a no-op. *But* it
>>> allows us to remove the global dependence on CONFIG_TCG in the
>>> Meson machinery (see the last commit in this series).
>>>
>>> Is that missing part of the commit description?
>>>
>>> "This will allow us to remove the global CONFIG_TCG dependency
>>> in our Meson machinery in a pair of commits."?
>> 
>> Do you mean "in the next two commits"?
>
> Yes.
>
>> Please also note that the probing at run-time always gives the same
>> result as the compile-time check it replaces.
>> 
>> I don't understand what exactly the conversion to probing enables and
>> how, but I believe I don't have to.
>
> This series is removing some limitations in qtests to help Claudio work:
>
> x86: https://www.mail-archive.com/qemu-devel@nongnu.org/msg793885.html
> arm: https://www.mail-archive.com/qemu-devel@nongnu.org/msg799328.html
> s390x: https://www.mail-archive.com/qemu-devel@nongnu.org/msg800254.html
>
> which allow building with different sets of accelerators (and more).
>
> Claudio/Paolo could better explain :)

Will this lead to different values of qtest_has_accel("tcg") and
defined(CONFIG_TCG) within a single build tree?

> What I like from feature tested at runtime is we can run qtests using
> older binaries, binaries built with different configure flags, or the
> binaries installed by the distribution.

Running with binaries build from a different HEAD seems unlikely to be
useful.  Any failures are just as likely to be caused bu the version
mismatch as by actual bugs.  Binaries for the same HEAD built for
another configuration can be made to work (this patch doesn't yet
suffice), but why should I care?

What I don't like is yet another moving part.

I'm not sure this is a good trade.  Quite possibly because I still don't
fully understand what we're trying to accomplish here :)