Re: [PATCH v6 14/14] block/dirty-bitmap: convert coroutine-only functions to co_wrapper

[Vladimir Sementsov-Ogievskiy]

On 11/25/22 16:35, Emanuele Giuseppe Esposito wrote:
> bdrv_can_store_new_dirty_bitmap and bdrv_remove_persistent_dirty_bitmap
> check if they are running in a coroutine, directly calling the
> coroutine callback if it's the case.
> Except that no coroutine calls such functions, therefore that check
> can be removed, and function creation can be offloaded to
> c_w.
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

> ---
>   block/dirty-bitmap.c         | 88 +-----------------------------------
>   block/meson.build            |  1 +
>   include/block/block-common.h |  5 +-
>   include/block/block-io.h     | 10 +++-
>   include/block/dirty-bitmap.h | 10 +++-
>   5 files changed, 22 insertions(+), 92 deletions(-)
>
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index bf3dc0512a..21cf592889 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c

Please do

  git config diff.orderFile scripts/git.orderfile

so that .h files appear first.

> @@ -388,7 +388,7 @@ void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs)
>    * not fail.
>    * This function doesn't release corresponding BdrvDirtyBitmap.
>    */
> -static int coroutine_fn
> +int coroutine_fn
>   bdrv_co_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
>                                          Error **errp)
>   {

[..]

> --- a/include/block/block-common.h
> +++ b/include/block/block-common.h
> @@ -29,8 +29,6 @@
>   #include "qemu/iov.h"
>   #include "qemu/coroutine.h"
>   #include "block/accounting.h"
> -#include "block/dirty-bitmap.h"
> -#include "block/blockjob.h"
>   #include "qemu/hbitmap.h"
>   #include "qemu/transactions.h"
>   
> @@ -51,6 +49,9 @@
>   #define co_wrapper
>   #define co_wrapper_mixed
>   
> +#include "block/dirty-bitmap.h"
> +#include "block/blockjob.h"
> +

that's not good.. we have unclean dependency on include order and these wrapper 
definitions.

Actually that means that we want separate header file for co_wrapper and 
co_wrapper_mixed.. But OK, let's keep it as is for now, not hard to fix it 
later if really needed.


--
Best regards,
Vladimir