From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41643) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XtwAV-0005dk-SV for qemu-devel@nongnu.org; Thu, 27 Nov 2014 05:19:30 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XtwAP-0002ZV-M8 for qemu-devel@nongnu.org; Thu, 27 Nov 2014 05:19:23 -0500 Received: from mx1.redhat.com ([209.132.183.28]:53702) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XtwAP-0002ZP-Ex for qemu-devel@nongnu.org; Thu, 27 Nov 2014 05:19:17 -0500 Message-ID: <5476FAA1.2060608@redhat.com> Date: Thu, 27 Nov 2014 11:19:13 +0100 From: Max Reitz MIME-Version: 1.0 References: <1417023715-18210-1-git-send-email-jsnow@redhat.com> <1417023715-18210-8-git-send-email-jsnow@redhat.com> In-Reply-To: <1417023715-18210-8-git-send-email-jsnow@redhat.com> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v8 07/10] qmp: Add support of "dirty-bitmap" sync mode for drive-backup List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: John Snow , qemu-devel@nongnu.org Cc: kwolf@redhat.com, Fam Zheng , armbru@redhat.com, vsementsov@parallels.com, stefanha@redhat.com, pbonzini@redhat.com On 2014-11-26 at 18:41, John Snow wrote: > From: Fam Zheng > > For "dirty-bitmap" sync mode, the block job will iterate through the > given dirty bitmap to decide if a sector needs backup (backup all the > dirty clusters and skip clean ones), just as allocation conditions of > "top" sync mode. > > There are two bitmap use modes for sync=dirty-bitmap: > > - reset: backup job makes a copy of bitmap and resets the original > one. > - consume: backup job makes the original anonymous (invisible to user) > and releases it after use. > > Signed-off-by: Fam Zheng > Signed-off-by: John Snow > --- > block.c | 5 ++ > block/backup.c | 130 ++++++++++++++++++++++++++++++++++++++-------- > block/mirror.c | 4 ++ > blockdev.c | 18 ++++++- > hmp.c | 4 +- > include/block/block.h | 1 + > include/block/block_int.h | 6 +++ > qapi/block-core.json | 30 +++++++++-- > qmp-commands.hx | 7 +-- > 9 files changed, 175 insertions(+), 30 deletions(-) [snip] > diff --git a/block/backup.c b/block/backup.c > index 792e655..2aab68f 100644 > --- a/block/backup.c > +++ b/block/backup.c > @@ -386,6 +438,36 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target, > return; > } > > + if (sync_mode == MIRROR_SYNC_MODE_DIRTY_BITMAP) { > + if (!sync_bitmap) { > + error_setg(errp, "must provide a valid bitmap name for " > + "\"dirty-bitmap\" sync mode"); > + return; > + } > + > + switch (bitmap_mode) { > + case BITMAP_USE_MODE_RESET: > + original = sync_bitmap; > + sync_bitmap = bdrv_copy_dirty_bitmap(bs, sync_bitmap, NULL); > + bdrv_reset_dirty_bitmap(bs, original); > + break; > + case BITMAP_USE_MODE_CONSUME: > + bdrv_dirty_bitmap_make_anon(bs, sync_bitmap); > + break; > + default: > + error_setg(errp, "Invalid BitmapUseMode (%s) given to backup_start", > + BitmapUseMode_lookup[bitmap_mode]); > + return; Well, this is basically the opposite of what I'd done, but making it difficult by formatting a nice error message for the user in places which should never be reached instead of just aborting is not wrong, in my opinion (I do know that others have other opinions). [snip] > diff --git a/blockdev.c b/blockdev.c > index 276a31b..1a56959 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -2337,7 +2342,18 @@ void qmp_drive_backup(const char *device, const char *target, > > bdrv_set_aio_context(target_bs, aio_context); > > - backup_start(bs, target_bs, speed, sync, on_source_error, on_target_error, > + if (has_bitmap) { > + bmap = bdrv_find_dirty_bitmap(bs, bitmap); > + if (!bmap) { > + error_setg(errp, "A bitmap name was given, but bitmap '%s' could not be found", > + bitmap); Well, I do know I said that I'd translate "has_bitmap was set" to "A bitmap name was specified", but I assumed you'd remove the redundancy yourself. ;-) Since this line needs to be reworked anyway (see Fam's reply), it can be (imho) shortened to "Bitmap '%s' could not be found". The user should know that he/she has specified a bitmap, and that it did have that name; the only thing he/she doesn't know so far is that qemu failed to find that bitmap, so that's the information we need to give. With that line somehow shortened/split to 80 characters (I'd be fine with keeping its content as-is, I just think it'd sound strange to the user): Reviewed-by: Max Reitz