Re: [Qemu-devel] [PATCH v2 3/6] qapi: add block-dirty-bitmap-enable/disable

[Vladimir Sementsov-Ogievskiy]

20.01.2018 02:50, John Snow wrote:
> On 01/16/2018 07:54 AM, Vladimir Sementsov-Ogievskiy wrote:
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> > ---
> >   qapi/block-core.json | 42 ++++++++++++++++++++++++++++++++++++++++++
> >   blockdev.c           | 42 ++++++++++++++++++++++++++++++++++++++++++
> >   2 files changed, 84 insertions(+)
> >
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index 827254db22..b3851ee760 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -1672,6 +1672,48 @@
> >     'data': 'BlockDirtyBitmap' }
> >   
> >   ##
> > +# @block-dirty-bitmap-enable:
> > +#
> > +# Enable dirty bitmap, so that it will continue tracking disk changes.
> > +#
> suggest:
> "Enables a dirty bitmap so that it will begin tracking disk changes."
>
> Key item here is "begin" instead of "continue".

agree.

> > +# Returns: nothing on success
> > +#          If @node is not a valid block device, DeviceNotFound
> > +#          If @name is not found, GenericError with an explanation
> > +#
> > +# Since: 2.12
> > +#
> > +# Example:
> > +#
> > +# -> { "execute": "block-dirty-bitmap-enable",
> > +#      "arguments": { "node": "drive0", "name": "bitmap0" } }
> > +# <- { "return": {} }
> > +#
> > +##
> > +  { 'command': 'block-dirty-bitmap-enable',
> > +    'data': 'BlockDirtyBitmap' }
> > +
> > +##
> > +# @block-dirty-bitmap-disable:
> > +#
> > +# Disable dirty bitmap, so that it will stop tracking disk changes.
> > +#
> suggest:
> "Disables a dirty bitmap so that it will stop tracking disk changes."
>
> > +# Returns: nothing on success
> > +#          If @node is not a valid block device, DeviceNotFound
> > +#          If @name is not found, GenericError with an explanation
> > +#
> > +# Since: 2.12
> > +#
> > +# Example:
> > +#
> > +# -> { "execute": "block-dirty-bitmap-disable",
> > +#      "arguments": { "node": "drive0", "name": "bitmap0" } }
> > +# <- { "return": {} }
> > +#
> > +##
> > +    { 'command': 'block-dirty-bitmap-disable',
> > +      'data': 'BlockDirtyBitmap' }
> > +
> > +##
> >   # @BlockDirtyBitmapSha256:
> >   #
> >   # SHA256 hash of dirty bitmap data
> > diff --git a/blockdev.c b/blockdev.c
> > index 8068cbd606..997a6f514c 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -2821,6 +2821,48 @@ void qmp_block_dirty_bitmap_clear(const char *node, 
> > const char *name,
> >       bdrv_clear_dirty_bitmap(bitmap, NULL);
> >   }
> >   
> > +void qmp_block_dirty_bitmap_enable(const char *node, const char *name,
> > +                                   Error **errp)
> > +{
> > +    BlockDriverState *bs;
> > +    BdrvDirtyBitmap *bitmap;
> > +
> > +    bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp);
> > +    if (!bitmap || !bs) {
> > +        return;
> > +    }
> > +
> > +    if (bdrv_dirty_bitmap_frozen(bitmap)) {
> > +        error_setg(errp,
> > +                   "Bitmap '%s' is currently frozen and cannot be enabled",
> > +                   name);
> > +        return;
> > +    }
> > +
> > +    bdrv_enable_dirty_bitmap(bitmap);
> > +}
> > +
> > +void qmp_block_dirty_bitmap_disable(const char *node, const char *name,
> > +                                    Error **errp)
> > +{
> > +    BlockDriverState *bs;
> > +    BdrvDirtyBitmap *bitmap;
> > +
> > +    bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp);
> > +    if (!bitmap || !bs) {
> > +        return;
> > +    }
> > +
> > +    if (bdrv_dirty_bitmap_frozen(bitmap)) {
> > +        error_setg(errp,
> > +                   "Bitmap '%s' is currently frozen and cannot be disabled",
> > +                   name);
> > +        return;
> > +    }
> > +
> > +    bdrv_disable_dirty_bitmap(bitmap);
> > +}
> > +
> >   BlockDirtyBitmapSha256 *qmp_x_debug_block_dirty_bitmap_sha256(const char 
> > *node,
> >                                                                 const char 
> > *name,
> >                                                                 Error **errp)
> I have to admit exposing this interface still makes me nervous, but :)
>
> Mechanically correct, and with suggesting phrasing changes:
>
> Reviewed-by: John Snow <address@hidden>



--
Best regards,
Vladimir