Re: [Qemu-devel] [PATCH v6 05/10] block: Add bdrv_copy_dirty_bitmap and bdrv_reset_dirty_bitmap

[Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com>]

[-- Attachment #1: Type: text/plain, Size: 3032 bytes --]

from [PATCH v6 02/10]
> +void qmp_block_dirty_bitmap_remove(const char *device, const char *name,
> +                                   Error **errp)
> +{
> +    BlockDriverState *bs;
> +    BdrvDirtyBitmap *bitmap;
> +
> +    bs = bdrv_find(device);
> +    if (!bs) {
> +        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> +        return;
> +    }
> +
> +    if (!name || name[0] == '\0') {
> +        error_setg(errp, "Bitmap name cannot be empty");
> +        return;
> +    }
> +    bitmap = bdrv_find_dirty_bitmap(bs, name);
> +    if (!bitmap) {
> +        error_setg(errp, "Dirty bitmap not found: %s", name);
> +        return;
> +    }
> +
> +    bdrv_dirty_bitmap_make_anon(bs, bitmap);
> +    bdrv_release_dirty_bitmap(bs, bitmap);
> +}

from [PATCH v6 05/10]:
> +void qmp_block_dirty_bitmap_enable(const char *device, const char *name,
> +                                   Error **errp)
> +{
> +    BlockDriverState *bs;
> +    BdrvDirtyBitmap *bitmap;
> +
> +    bs = bdrv_find(device);
> +    if (!bs) {
> +        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> +        return;
> +    }
> +
> +    bitmap = bdrv_find_dirty_bitmap(bs, name);
> +    if (!bitmap) {
> +        error_setg(errp, "Dirty bitmap not found: %s", name);
> +        return;
> +    }
> +
> +    bdrv_enable_dirty_bitmap(bs, bitmap);
> +}
> +
> +void qmp_block_dirty_bitmap_disable(const char *device, const char *name,
> +                                    Error **errp)
> +{
> +    BlockDriverState *bs;
> +    BdrvDirtyBitmap *bitmap;
> +
> +    bs = bdrv_find(device);
> +    if (!bs) {
> +        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> +        return;
> +    }
> +
> +    bitmap = bdrv_find_dirty_bitmap(bs, name);
> +    if (!bitmap) {
> +        error_setg(errp, "Dirty bitmap not found: %s", name);
> +        return;
> +    }
> +
> +    bdrv_disable_dirty_bitmap(bs, bitmap);
> +}
> +

there is one inconsistence:

you have check
> +    if (!name || name[0] == '\0') {
> +        error_setg(errp, "Bitmap name cannot be empty");
> +        return;
> +    }
when accessing bitmap in qmp_block_dirty_bitmap_remove, but not in 
qmp_block_dirty_bitmap_{enable,disable}.

Also, I think it'll be better to put similar part of these three 
functions into one separate function to avoid duplicates, like

static BdrvDirtyBitmap *bitmap find_dirty_bitmap(const char *device, const char *name,
                                    Error **errp)
{
     BlockDriverState *bs;
     BdrvDirtyBitmap *bitmap;

     // most simple error condition earlier
     if (!name || name[0] == '\0') {
         error_setg(errp, "Bitmap name cannot be empty");
         return NULL;
     }

     bs = bdrv_find(device);
     if (!bs) {
         error_set(errp, QERR_DEVICE_NOT_FOUND, device);
         return NULL;
     }

     bitmap = bdrv_find_dirty_bitmap(bs, name);
     if (!bitmap) {
         error_setg(errp, "Dirty bitmap not found: %s", name);
         return NULL;
     }
     
     return bitmap;
}


-- 
Best regards,
Vladimir


[-- Attachment #2: Type: text/html, Size: 4637 bytes --]