| Message ID | 20231006123910.17759-4-farosas@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | tests/migration-test: Allow testing older machine types | expand |
Fabiano Rosas <farosas@suse.de> 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> Reviewed-by: Juan Quintela <quintela@redhat.com>
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(-) Reviewed-by: Thomas Huth <thuth@redhat.com>
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? Thomas
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
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.
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; } - qts = qtest_init("-machine none"); + qts = qtest_init_with_env(qemu_var, "-machine none"); response = qtest_qmp(qts, "{ 'execute': 'query-machines' }"); g_assert(response); list = qdict_get_qlist(response, "return"); @@ -1497,7 +1506,7 @@ void qtest_cb_for_every_machine(void (*cb)(const char *machine), struct MachInfo *machines; int i; - machines = qtest_get_machines(); + machines = qtest_get_machines(NULL); for (i = 0; machines[i].name != NULL; i++) { /* Ignore machines that cannot be used for qtests */ @@ -1518,7 +1527,7 @@ bool qtest_has_machine(const char *machine) struct MachInfo *machines; int i; - machines = qtest_get_machines(); + machines = qtest_get_machines(NULL); for (i = 0; machines[i].name != NULL; i++) { if (g_str_equal(machine, machines[i].name) ||
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(-)