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

[Fabiano Rosas <farosas@suse.de>]

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.

Thanks