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

[John Snow]

On 11/07/2014 10:16 AM, Vladimir Sementsov-Ogievskiy wrote:
> 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}.

In qmp_block_dirty_bitmap_{enable,disable} I don't think it's a problem 
if we don't check the name here -- it just means that we're not going to 
be able to find the name (since it's invalid) and the function will 
error out anyway.

I don't think a change is warranted, necessarily, unless we factor out 
the error checking: see below.

> 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;
> }

I think normally I would agree, but since qmp_block_dirty_bitmap_remove 
needs to use the BlockDriverState anyway, part of this error-checking 
routine gets duplicated regardless, unless you add another return 
parameter to give you the BlockDriverState back as well, and you lose 
the single purpose nature of factoring it out.

I will keep the redundancy of the error-checking of this patch in mind 
as I clean it up for v7...

> --
> Best regards,
> Vladimir