On 11.09.2017 19:20, Eric Blake wrote: > Remove the trivial wrapper qtest_init(), and change qtest_start() > to no longer implicitly set global_qtest, to make it obvious in the > rest of the testsuite where we are still relying on global_qtest. > Everything now uses qtest_start() (and friends) and qtest_quit(), > and explicitly tracks the QTestState between the two (although in > many cases, this tracking is still done through global_qtest). > Doing this makes it easier to see what remaining cleanups will be > needed if we don't want an implicit dependency on global state. [...] > diff --git a/tests/libqtest.h b/tests/libqtest.h > index 817e3a5580..2a21bf4605 100644 > --- a/tests/libqtest.h > +++ b/tests/libqtest.h > @@ -27,7 +27,7 @@ extern QTestState *global_qtest; > * qtest_start_without_qmp_handshake: > * @extra_args: other arguments to pass to QEMU. > * > - * Returns: #QTestState instance. Does not affect #global_qtest. > + * Returns: #QTestState instance, handshaking not yet completed.
I'd rather add a description a la: * Start QEMU, but do not execute the QMP handshake yet. * * Returns: #QTestState instance > */ > QTestState *qtest_start_without_qmp_handshake(const char *extra_args); > > @@ -35,10 +35,7 @@ QTestState *qtest_start_without_qmp_handshake(const char > *extra_args); > * qtest_start: > * @args: other arguments to pass to QEMU > * > - * Start QEMU and assign the resulting #QTestState to #global_qtest. > - * The global variable is used by "shortcut" functions documented below. > * > - * Returns: #QTestState instance. > + * Returns: #QTestState instance, handshaking completed. I'd rather change the description instead of removing it: * Start QEMU and execute the initial QMP handshake * * Returns: #QTestState instance. > */ > QTestState *qtest_start(const char *args); > > @@ -47,10 +44,7 @@ QTestState *qtest_start(const char *args); > * @fmt...: Format for creating other arguments to pass to QEMU, formatted > * like sprintf(). > * > - * Start QEMU and return the resulting #QTestState (but unlike qtest_start(), > - * #global_qtest is left at NULL). > - * > - * Returns: #QTestState instance. > + * Returns: #QTestState instance, handshaking completed. dito > */ > QTestState *qtest_startf(const char *fmt, ...) GCC_FMT_ATTR(1, 2); > > @@ -60,30 +54,11 @@ QTestState *qtest_startf(const char *fmt, ...) > GCC_FMT_ATTR(1, 2); > * like vsprintf(). > * @ap: Format arguments. > * > - * Start QEMU and return the resulting #QTestState (but unlike qtest_start(), > - * #global_qtest is left at NULL). > - * > - * Returns: #QTestState instance. > + * Returns: #QTestState instance, handshaking completed. dito > */ > QTestState *qtest_vstartf(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0); > > /** > - * qtest_init: > - * @extra_args: other arguments to pass to QEMU. > - * > - * Returns: #QTestState instance. Does not affect #global_qtest. > - */ > -static inline QTestState *qtest_init(const char *extra_args) > -{ > - QTestState *s; > - > - assert(!global_qtest); > - s = qtest_start(extra_args); > - global_qtest = NULL; > - return s; > -} [...] > diff --git a/tests/display-vga-test.c b/tests/display-vga-test.c > index 8667330e3c..d638f15ec3 100644 > --- a/tests/display-vga-test.c > +++ b/tests/display-vga-test.c > @@ -12,39 +12,33 @@ > > static void pci_cirrus(void) > { > - qtest_start("-vga none -device cirrus-vga"); > - qtest_quit(global_qtest); > + qtest_quit(qtest_start("-vga none -device cirrus-vga")); I'd prefer to keep this on separate lines ... but that's again just my personal taste. (also for the othe changes below) > } > > static void pci_stdvga(void) > { > - qtest_start("-vga none -device VGA"); > - qtest_quit(global_qtest); > + qtest_quit(qtest_start("-vga none -device VGA")); > } > > static void pci_secondary(void) > { > - qtest_start("-vga none -device secondary-vga"); > - qtest_quit(global_qtest); > + qtest_quit(qtest_start("-vga none -device secondary-vga")); > } > > static void pci_multihead(void) > { > - qtest_start("-vga none -device VGA -device secondary-vga"); > - qtest_quit(global_qtest); > + qtest_quit(qtest_start("-vga none -device VGA -device secondary-vga")); > } > > static void pci_virtio_gpu(void) > { > - qtest_start("-vga none -device virtio-gpu-pci"); > - qtest_quit(global_qtest); > + qtest_quit(qtest_start("-vga none -device virtio-gpu-pci")); > } > > #ifdef CONFIG_VIRTIO_VGA > static void pci_virtio_vga(void) > { > - qtest_start("-vga none -device virtio-vga"); > - qtest_quit(global_qtest); > + qtest_quit(qtest_start("-vga none -device virtio-vga")); > } > #endif [...] > diff --git a/tests/ne2000-test.c b/tests/ne2000-test.c > index cae83c5c4c..8e6f7b07c6 100644 > --- a/tests/ne2000-test.c > +++ b/tests/ne2000-test.c > @@ -22,7 +22,7 @@ int main(int argc, char **argv) > g_test_init(&argc, &argv, NULL); > qtest_add_func("/ne2000/pci/nop", pci_nop); > > - qtest_start("-device ne2k_pci"); > + global_qtest = qtest_start("-device ne2k_pci"); > ret = g_test_run(); > > qtest_quit(global_qtest); For such very trivial tests, it maybe makes sense to use a local "QTestState *qts" variable here already, so we don't have to touch this code again later? > diff --git a/tests/nvme-test.c b/tests/nvme-test.c > index 3d6c0f39cf..b054ad6fcd 100644 > --- a/tests/nvme-test.c > +++ b/tests/nvme-test.c > @@ -22,8 +22,9 @@ int main(int argc, char **argv) > g_test_init(&argc, &argv, NULL); > qtest_add_func("/nvme/nop", nop); > > - qtest_start("-drive id=drv0,if=none,file=null-co://,format=raw " > - "-device nvme,drive=drv0,serial=foo"); > + global_qtest = qtest_start( > + "-drive id=drv0,if=none,file=null-co://,format=raw " > + "-device nvme,drive=drv0,serial=foo"); > ret = g_test_run(); > > qtest_quit(global_qtest); dito > diff --git a/tests/pcnet-test.c b/tests/pcnet-test.c > index 98246d3504..a58a5fd7bf 100644 > --- a/tests/pcnet-test.c > +++ b/tests/pcnet-test.c > @@ -22,7 +22,7 @@ int main(int argc, char **argv) > g_test_init(&argc, &argv, NULL); > qtest_add_func("/pcnet/pci/nop", pci_nop); > > - qtest_start("-device pcnet"); > + global_qtest = qtest_start("-device pcnet"); > ret = g_test_run(); > > qtest_quit(global_qtest); dito [...] > diff --git a/tests/virtio-balloon-test.c b/tests/virtio-balloon-test.c > index 34ad718601..cca7b0e8fb 100644 > --- a/tests/virtio-balloon-test.c > +++ b/tests/virtio-balloon-test.c > @@ -22,7 +22,7 @@ int main(int argc, char **argv) > g_test_init(&argc, &argv, NULL); > qtest_add_func("/virtio/balloon/pci/nop", pci_nop); > > - qtest_start("-device virtio-balloon-pci"); > + global_qtest = qtest_start("-device virtio-balloon-pci"); > ret = g_test_run(); > > qtest_quit(global_qtest); dito [...] > diff --git a/tests/vmxnet3-test.c b/tests/vmxnet3-test.c > index 631630b4d0..0b4b807616 100644 > --- a/tests/vmxnet3-test.c > +++ b/tests/vmxnet3-test.c > @@ -22,7 +22,7 @@ int main(int argc, char **argv) > g_test_init(&argc, &argv, NULL); > qtest_add_func("/vmxnet3/nop", nop); > > - qtest_start("-device vmxnet3"); > + global_qtest = qtest_start("-device vmxnet3"); > ret = g_test_run(); > > qtest_quit(global_qtest); dito Thomas