Fabiano Rosas <faro...@suse.de> writes:
> Thomas Huth <th...@redhat.com> writes:
>
>> On 06/10/2023 14.39, Fabiano Rosas wrote:
>>> We're adding support for using more than one QEMU binary in
>>> tests. Modify qtest_get_machines() to take an environment variable
>>> that contains the QEMU binary path.
>>>
>>> Since the function keeps a cache of the machines list in the form of a
>>> static variable, refresh it any time the environment variable changes.
>>>
>>> Signed-off-by: Fabiano Rosas <faro...@suse.de>
>>> ---
>>> tests/qtest/libqtest.c | 17 +++++++++++++----
>>> 1 file changed, 13 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
>>> index 88b79cb477..47c8b6d46f 100644
>>> --- a/tests/qtest/libqtest.c
>>> +++ b/tests/qtest/libqtest.c
>>> @@ -1441,9 +1441,10 @@ struct MachInfo {
>>> * Returns an array with pointers to the available machine names.
>>> * The terminating entry has the name set to NULL.
>>> */
>>> -static struct MachInfo *qtest_get_machines(void)
>>> +static struct MachInfo *qtest_get_machines(const char *var)
>>> {
>>> static struct MachInfo *machines;
>>> + static char *qemu_var;
>>> QDict *response, *minfo;
>>> QList *list;
>>> const QListEntry *p;
>>> @@ -1452,11 +1453,19 @@ static struct MachInfo *qtest_get_machines(void)
>>> QTestState *qts;
>>> int idx;
>>>
>>> + if (g_strcmp0(qemu_var, var)) {
>>> + qemu_var = g_strdup(var);
>>> +
>>> + /* new qemu, clear the cache */
>>> + g_free(machines);
>>> + machines = NULL;
>>> + }
>>> +
>>> if (machines) {
>>> return machines;
>>> }
>>
>> After sleeping on the topic of the string handling in this patch series a
>> little bit I think it was maybe a bad idea to suggest to remove the
>> g_strdups in the other patches. If you actually clear the cache here, the
>> strings that previously were guaranteed to stay around until the end of the
>> program might now vanish. So instead of returning the pointer to the cache
>> here, it might be better to create a copy of the whole structure here and
>> let the callers decide whether they want to keep it around or free it at the
>> end?
>
> Hm, let me try that out. We could have a 'bool refresh' parameter in the
> top level API then, which would be a clearer interface perhaps.
I'm looking into this right now. I don't think callers ever want to keep
the machines list around. We'd have to cache the list and the binary
name a second time in the callers just to avoid having to copy/free a
few strings.
The caching needs to be centralized at qtest_get_machines(), otherwise
we'd be better off having doing setenv around the function calls, which
is what my hacked first version did.
If you're ok with that I'll just add a cleanup function to free all
strings when clearing the cache and keep strdup'ing where appropriate.
