Re: [Qemu-ppc] [PATCH v7 29/38] libqtest: Merge qtest_init() into qtest_start()

[Thomas Huth]

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