Am 01.04.2022 um 12:08 hat Vladimir Sementsov-Ogievskiy geschrieben:
> We don't need extra bitmap. All we need is to backup the original
> bitmap when we do first merge. So, drop extra temporary bitmap and work
> directly with target and backup.
>
> Still to keep old semantics, that on failure target is unchanged and
> user don't need to restore, we need a local_backup variable and do
> restore ourselves on failure path.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@openvz.org>
> ---
> block/monitor/bitmap-qmp-cmds.c | 39 ++++++++++++++++-----------------
> 1 file changed, 19 insertions(+), 20 deletions(-)
>
> diff --git a/block/monitor/bitmap-qmp-cmds.c b/block/monitor/bitmap-qmp-cmds.c
> index 4db704c015..07d0da323b 100644
> --- a/block/monitor/bitmap-qmp-cmds.c
> +++ b/block/monitor/bitmap-qmp-cmds.c
> @@ -261,8 +261,9 @@ BdrvDirtyBitmap *block_dirty_bitmap_merge(const char
> *node, const char *target,
> HBitmap **backup, Error **errp)
> {
> BlockDriverState *bs;
> - BdrvDirtyBitmap *dst, *src, *anon;
> + BdrvDirtyBitmap *dst, *src;
> BlockDirtyBitmapMergeSourceList *lst;
> + HBitmap *local_backup = NULL;
>
> GLOBAL_STATE_CODE();
>
> @@ -271,12 +272,6 @@ BdrvDirtyBitmap *block_dirty_bitmap_merge(const char
> *node, const char *target,
> return NULL;
> }
>
> - anon = bdrv_create_dirty_bitmap(bs, bdrv_dirty_bitmap_granularity(dst),
> - NULL, errp);
> - if (!anon) {
> - return NULL;
> - }
> -
> for (lst = bms; lst; lst = lst->next) {
> switch (lst->value->type) {
> const char *name, *node;
> @@ -285,8 +280,7 @@ BdrvDirtyBitmap *block_dirty_bitmap_merge(const char
> *node, const char *target,
> src = bdrv_find_dirty_bitmap(bs, name);
> if (!src) {
> error_setg(errp, "Dirty bitmap '%s' not found", name);
> - dst = NULL;
> - goto out;
> + goto fail;
> }
> break;
> case QTYPE_QDICT:
> @@ -294,29 +288,34 @@ BdrvDirtyBitmap *block_dirty_bitmap_merge(const char
> *node, const char *target,
> name = lst->value->u.external.name;
> src = block_dirty_bitmap_lookup(node, name, NULL, errp);
> if (!src) {
> - dst = NULL;
> - goto out;
> + goto fail;
> }
> break;
> default:
> abort();
> }
>
> - if (!bdrv_merge_dirty_bitmap(anon, src, NULL, errp)) {
> - dst = NULL;
> - goto out;
> + /* We do backup only for first merge operation */
> + if (!bdrv_merge_dirty_bitmap(dst, src,
> + local_backup ? NULL : &local_backup,
> + errp))
> + {
> + goto fail;
> }
> }
>
> - /* Merge into dst; dst is unchanged on failure. */
> - if (!bdrv_merge_dirty_bitmap(dst, anon, backup, errp)) {
> - dst = NULL;
> - goto out;
> + if (backup) {
> + *backup = local_backup;
> }
Don't we need something like '... else { hbitmap_free(local_backup); }'
in order to avoid leaking the memory?
>
> - out:
> - bdrv_release_dirty_bitmap(anon);
> return dst;
> +
> +fail:
> + if (local_backup) {
> + bdrv_restore_dirty_bitmap(dst, local_backup);
> + }
> +
> + return NULL;
> }
Kevin
