Re: [RFC PATCH] test-bdrv-drain: keep graph manipulations out of coroutines

[Emanuele Giuseppe Esposito]

Am 02/12/2022 um 14:27 schrieb Paolo Bonzini:
> Changes to the BlockDriverState graph will have to take the
> corresponding lock for writing, and therefore cannot be done
> inside a coroutine.  Move them outside the test body.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  tests/unit/test-bdrv-drain.c | 63 ++++++++++++++++++++++++++----------
>  1 file changed, 46 insertions(+), 17 deletions(-)
> 
> diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c
> index 6ae44116fe79..d85083dd4f9e 100644
> --- a/tests/unit/test-bdrv-drain.c
> +++ b/tests/unit/test-bdrv-drain.c
> @@ -199,25 +199,40 @@ static void do_drain_end_unlocked(enum drain_type 
> drain_type, BlockDriverState *
>      }
>  }
>  
> +static BlockBackend *blk;
> +static BlockDriverState *bs, *backing;
> +
> +static void test_drv_cb_init(void)
> +{
> +    blk = blk_new(qemu_get_aio_context(), BLK_PERM_ALL, BLK_PERM_ALL);
> +    bs = bdrv_new_open_driver(&bdrv_test, "test-node", BDRV_O_RDWR,
> +                              &error_abort);
> +    blk_insert_bs(blk, bs, &error_abort);
> +
> +    backing = bdrv_new_open_driver(&bdrv_test, "backing", 0, &error_abort);
> +    bdrv_set_backing_hd(bs, backing, &error_abort);
> +}
> +
> +static void test_drv_cb_fini(void)

fini stands for "finito"? :)

Anyways, an alternative solution for this is also here (probably coming
from you too):
https://lists.nongnu.org/archive/html/qemu-devel/2022-03/msg03517.html

Thank you,
Emanuele

> +{
> +    bdrv_unref(backing);
> +    bdrv_unref(bs);
> +    blk_unref(blk);
> +    backing = NULL;
> +    bs = NULL;
> +    blk = NULL;
> +}
> +
>  static void test_drv_cb_common(enum drain_type drain_type, bool recursive)
>  {
> -    BlockBackend *blk;
> -    BlockDriverState *bs, *backing;
>      BDRVTestState *s, *backing_s;
>      BlockAIOCB *acb;
>      int aio_ret;
>  
>      QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, NULL, 0);
>  
> -    blk = blk_new(qemu_get_aio_context(), BLK_PERM_ALL, BLK_PERM_ALL);
> -    bs = bdrv_new_open_driver(&bdrv_test, "test-node", BDRV_O_RDWR,
> -                              &error_abort);
>      s = bs->opaque;
> -    blk_insert_bs(blk, bs, &error_abort);
> -
> -    backing = bdrv_new_open_driver(&bdrv_test, "backing", 0, &error_abort);
>      backing_s = backing->opaque;
> -    bdrv_set_backing_hd(bs, backing, &error_abort);
>  
>      /* Simple bdrv_drain_all_begin/end pair, check that CBs are called */
>      g_assert_cmpint(s->drain_count, ==, 0);
> @@ -252,30 +267,44 @@ static void test_drv_cb_common(enum drain_type 
> drain_type, bool recursive)
>  
>      g_assert_cmpint(s->drain_count, ==, 0);
>      g_assert_cmpint(backing_s->drain_count, ==, 0);
> -
> -    bdrv_unref(backing);
> -    bdrv_unref(bs);
> -    blk_unref(blk);
>  }
>  
> -static void test_drv_cb_drain_all(void)
> +static void test_drv_cb_do_drain_all(void)
>  {
>      test_drv_cb_common(BDRV_DRAIN_ALL, true);
>  }
>  
> -static void test_drv_cb_drain(void)
> +static void test_drv_cb_do_drain(void)
>  {
>      test_drv_cb_common(BDRV_DRAIN, false);
>  }
>  
> +static void test_drv_cb_drain_all(void)
> +{
> +    test_drv_cb_init();
> +    test_drv_cb_do_drain_all();
> +    test_drv_cb_fini();
> +}
> +
> +static void test_drv_cb_drain(void)
> +{
> +    test_drv_cb_init();
> +    test_drv_cb_do_drain();
> +    test_drv_cb_fini();
> +}
> +
>  static void test_drv_cb_co_drain_all(void)
>  {
> -    call_in_coroutine(test_drv_cb_drain_all);
> +    test_drv_cb_init();
> +    call_in_coroutine(test_drv_cb_do_drain_all);
> +    test_drv_cb_fini();
>  }
>  
>  static void test_drv_cb_co_drain(void)
>  {
> -    call_in_coroutine(test_drv_cb_drain);
> +    test_drv_cb_init();
> +    call_in_coroutine(test_drv_cb_do_drain);
> +    test_drv_cb_fini();
>  }
>  
>  static void test_quiesce_common(enum drain_type drain_type, bool recursive)
>