Re: [Qemu-block] [Qemu-devel] block/dirty-bitmap: Useless bitmaps in image can be removed

[John Snow]

On 06/24/2018 11:36 PM, address@hidden wrote:
> If qemu-kvm quit without saving bitmaps to image(coredump, host kernel panic,
> or host pooweroff), bitmaps in image can not be safely used anymore, and also
> can not be removed. Useless bitmaps should be removed.
> 
> Signed-off-by: yaoxu <address@hidden>

I count at least ten copies of this same patch in my inbox. Please send
just ONE copy, and make sure it has [PATCH] in the title, like other
patches do.

> ---
> diff --git a/blockdev.c b/blockdev.c
> index 58d7570932..c85056a74b 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2837,31 +2837,35 @@ void qmp_block_dirty_bitmap_remove(const char *node, 
> const char *name,
>      Error *local_err = NULL;
>  
>      bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp);
> -    if (!bitmap || !bs) {
> +    if (!bs) {

In what cases are we going to fail to find a bitmap but manage to return
a BlockDriverState?

>          return;
>      }
>  
> -    if (bdrv_dirty_bitmap_frozen(bitmap)) {
> -        error_setg(errp,
> -                   "Bitmap '%s' is currently frozen and cannot be removed",
> -                   name);
> -        return;
> -    } else if (bdrv_dirty_bitmap_qmp_locked(bitmap)) {
> -        error_setg(errp,
> -                   "Bitmap '%s' is currently locked and cannot be removed",
> -                   name);
> +    if (bitmap) {
> +        if (bdrv_dirty_bitmap_frozen(bitmap)) {
> +            error_setg(errp,
> +                       "Bitmap '%s' is currently frozen and cannot be 
> removed",
> +                       name);
> +            return;
> +        } else if (bdrv_dirty_bitmap_qmp_locked(bitmap)) {
> +            error_setg(errp,
> +                       "Bitmap '%s' is currently locked and cannot be 
> removed",
> +                       name);
> +            return;
> +        }
> +    }
> +
> +    bdrv_remove_persistent_dirty_bitmap(bs, name, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
>          return;
>      }
>  
> -    if (bdrv_dirty_bitmap_get_persistance(bitmap)) {
> -        bdrv_remove_persistent_dirty_bitmap(bs, name, &local_err);
> -        if (local_err != NULL) {
> -            error_propagate(errp, local_err);
> -            return;
> -        }
> +    if (bitmap) {
> +        bdrv_release_dirty_bitmap(bs, bitmap);
>      }
>  
> -    bdrv_release_dirty_bitmap(bs, bitmap);
> +    if (*errp) {
> +        error_free(*errp);
> +        *errp = NULL;
> +    }
>  }
>  
>  /**
> 

Even if bitmap was null and bs was set, this is going to forward
requests for any bitmap, whether it exists or not, straight along to the
persistence layer, which does not give you an error if the bitmap didn't
exist.

We also can't open images with corrupted bitmaps in them for editing, so
this won't work there, either.

We are otherwise aware of the problem and intend to address it via
`qemu-img check -r`.

--js