On 11.09.2017 19:20, Eric Blake wrote: > We have several callers that were formatting the argument strings > themselves; consolidate this effort by adding new convenience > functions directly in libqtest, and update all call-sites that > can benefit from it. [...] > diff --git a/tests/libqtest.c b/tests/libqtest.c > index e8c2e11817..b535d7768f 100644 > --- a/tests/libqtest.c > +++ b/tests/libqtest.c > @@ -245,6 +245,27 @@ QTestState *qtest_start(const char *extra_args) > return global_qtest = s; > } > > +QTestState *qtest_vstartf(const char *fmt, va_list ap) > +{ > + char *args = g_strdup_vprintf(fmt, ap); > + QTestState *s; > + > + s = qtest_start(args); > + global_qtest = NULL;
Don't you need a g_free(args) here? > + return s; > +} > + > +QTestState *qtest_startf(const char *fmt, ...) > +{ > + va_list ap; > + QTestState *s; > + > + va_start(ap, fmt); > + s = qtest_vstartf(fmt, ap); > + va_end(ap); > + return s; > +} [...] > diff --git a/tests/e1000-test.c b/tests/e1000-test.c > index 0c5fcdcc44..12bc526ad6 100644 > --- a/tests/e1000-test.c > +++ b/tests/e1000-test.c > @@ -14,16 +14,8 @@ > static void test_device(gconstpointer data) > { > const char *model = data; > - QTestState *s; > - char *args; > > - args = g_strdup_printf("-device %s", model); > - s = qtest_start(args); > - > - if (s) { > - qtest_quit(s); > - } > - g_free(args); > + qtest_quit(qtest_startf("-device %s", model)); Just my personal taste, but I think I'd be nicer to keep this on separate lines: QTestState *s; s = qtest_startf("-device %s", model); qtest_quit(s); > } [...] > diff --git a/tests/eepro100-test.c b/tests/eepro100-test.c > index bdc8a67d57..fc9ea84d66 100644 > --- a/tests/eepro100-test.c > +++ b/tests/eepro100-test.c > @@ -13,18 +13,9 @@ > static void test_device(gconstpointer data) > { > const char *model = data; > - QTestState *s; > - char *args; > - > - args = g_strdup_printf("-device %s", model); > - s = qtest_start(args); > > /* Tests only initialization so far. TODO: Implement functional tests */ > - > - if (s) { > - qtest_quit(s); > - } > - g_free(args); > + qtest_quit(qtest_startf("-device %s", model)); > } dito [...] > diff --git a/tests/numa-test.c b/tests/numa-test.c > index fa21d26935..e2f6c68be8 100644 > --- a/tests/numa-test.c > +++ b/tests/numa-test.c > @@ -12,20 +12,14 @@ > #include "qemu/osdep.h" > #include "libqtest.h" > > -static char *make_cli(const char *generic_cli, const char *test_cli) > -{ > - return g_strdup_printf("%s %s", generic_cli ? generic_cli : "", > test_cli); > -} > - > static void test_mon_explicit(const void *data) > { > char *s; > - char *cli; > + const char *args = data; > > - cli = make_cli(data, "-smp 8 " > - "-numa node,nodeid=0,cpus=0-3 " > - "-numa node,nodeid=1,cpus=4-7 "); > - qtest_start(cli); > + global_qtest = qtest_startf("%s -smp 8 " > + "-numa node,nodeid=0,cpus=0-3 " > + "-numa node,nodeid=1,cpus=4-7 ", args); > > s = hmp("info numa"); > g_assert(strstr(s, "node 0 cpus: 0 1 2 3")); > @@ -33,16 +27,14 @@ static void test_mon_explicit(const void *data) > g_free(s); > > qtest_quit(global_qtest); > - g_free(cli); > } > > static void test_mon_default(const void *data) > { > char *s; > - char *cli; > + const char *args = data; > > - cli = make_cli(data, "-smp 8 -numa node -numa node"); > - qtest_start(cli); > + global_qtest = qtest_startf("%s -smp 8 -numa node -numa node", args); > > s = hmp("info numa"); > g_assert(strstr(s, "node 0 cpus: 0 2 4 6")); > @@ -50,18 +42,16 @@ static void test_mon_default(const void *data) > g_free(s); > > qtest_quit(global_qtest); > - g_free(cli); > } > > static void test_mon_partial(const void *data) > { > char *s; > - char *cli; > + const char *args = data; > > - cli = make_cli(data, "-smp 8 " > - "-numa node,nodeid=0,cpus=0-1 " > - "-numa node,nodeid=1,cpus=4-5 "); > - qtest_start(cli); > + global_qtest = qtest_startf("%s -smp 8 " > + "-numa node,nodeid=0,cpus=0-1 " > + "-numa node,nodeid=1,cpus=4-5 ", args); Does GCC emit a warning if you'd used data here directly? Otherwise I think you could simply do this without the local args variable... Thomas