Re: [PATCH v2 3/9] tests/qtest: Allow qtest_get_machines to use an alternate QEMU binary

[Fabiano Rosas]

Fabiano Rosas <farosas@suse.de> writes:

> Thomas Huth <thuth@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 <farosas@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.