Re: [Qemu-block] [PATCH v3 07/10] block/dirty-bitmaps: unify qmp_locked and user_locked calls

[Vladimir Sementsov-Ogievskiy]

23.02.2019 3:06, John Snow wrote:
> These mean the same thing now. Unify them and rename the merged call
> bdrv_dirty_bitmap_busy to indicate semantically what we are describing,
> as well as help disambiguate from the various _locked and _unlocked
> versions of bitmap helpers that refer to mutex locks.
> 
> Signed-off-by: John Snow <address@hidden>

Hmm, and here, you directly fixed what I've noticed, so my r-b,
of course, still applies.

Ha, but I noticed funny thing:

> ---
>   block/dirty-bitmap.c           | 40 +++++++++++++++-------------------
>   blockdev.c                     | 18 +++++++--------
>   include/block/dirty-bitmap.h   |  5 ++---
>   migration/block-dirty-bitmap.c |  6 ++---
>   nbd/server.c                   |  6 ++---
>   5 files changed, 34 insertions(+), 41 deletions(-)
> 
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index d92a269753..b3627b0d8c 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c

[..]

> @@ -265,9 +259,9 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState 
> *bs,
>       child->disabled = bitmap->disabled;
>       bitmap->disabled = true;
>   
> -    /* Install the successor and lock the parent */
> +    /* Install the successor and mark the parent as busy */

I asked you to fix comment for bdrv_dirty_bitmap_create_successor(), but I 
didn't
noticed this one, I meant the comment to the whole function, placed above it, 
which
now have
  "Requires that the bitmap is not user_locked and has no successor."

So, it should be updated too.

and with it:
Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>

>       bitmap->successor = child;
> -    bitmap->qmp_locked = true;
> +    bitmap->busy = true;
>       return 0;
>   }
>   



-- 
Best regards,
Vladimir