Re: [Qemu-devel] [PATCH v3 3/7] block/dirty-bitmaps: add block_dirty_bitmap_check function

[Eric Blake <eblake@redhat.com>]

On 3/1/19 1:15 PM, John Snow wrote:
> Instead of checking against busy, inconsistent, or read only directly,
> use a check function with permissions bits that let us streamline the
> checks without reproducing them in many places.
> 
> Included in this patch are permissions changes that simply add the
> inconsistent check to existing permissions call spots, without
> addressing existing bugs.
> 
> In general, this means that busy+readonly checks become BDRV_BITMAP_DEFAULT,
> which checks against all three conditions. busy-only checks become
> BDRV_BITMAP_ALLOW_RO.
> 
> Notably, remove allows inconsistent bitmaps, so it doesn't follow the pattern.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  include/block/dirty-bitmap.h   | 13 ++++++++-
>  block/dirty-bitmap.c           | 38 +++++++++++++++++++-------
>  blockdev.c                     | 49 +++++++---------------------------
>  migration/block-dirty-bitmap.c | 12 +++------
>  nbd/server.c                   |  3 +--
>  5 files changed, 54 insertions(+), 61 deletions(-)
> 

Diffstat proves its a win, even with the extra documentation for the new
function. Nice.

> +int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags,
> +                            Error **errp)
> +{
> +    if ((flags & BDRV_BITMAP_BUSY) && bdrv_dirty_bitmap_busy(bitmap)) {
> +        error_setg(errp, "Bitmap '%s' is currently in use by another"
> +                   " operation and cannot be used", bitmap->name);

Split before space,

> +        return -1;
> +    }
> +
> +    if ((flags & BDRV_BITMAP_RO) && bdrv_dirty_bitmap_readonly(bitmap)) {
> +        error_setg(errp, "Bitmap '%s' is readonly and cannot be modified",
> +                   bitmap->name);
> +        return -1;
> +    }
> +
> +    if ((flags & BDRV_BITMAP_INCONSISTENT) &&
> +        bdrv_dirty_bitmap_inconsistent(bitmap)) {
> +        error_setg(errp, "Bitmap '%s' is inconsistent and cannot be used",
> +                   bitmap->name);
> +        error_append_hint(errp, "Try block-dirty-bitmap-remove to delete "
> +                          "this bitmap from disk");

split after space.  Looks inconsistent within a single function (pardon
the pun :)

That's minor,
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org