On 12/19/18 12:50 AM, mahaocong wrote:
From: mahaocong <mahaoc...@didichuxing.com>
Signed-off-by: mahaocong <mahaoc...@didichuxing.com>
The subject line tells "what", but you are missing a commit message body
that gives the "why". Details about the new feature, and how it is
useful, go a long way to making the code easier to review. How much of
your cover letter should be here as well?
At a first glance, I'm just going to focus on the interface:
---
block/dirty-bitmap.c | 14 ++++++++++
block/mirror.c | 63 +++++++++++++++++++++++++++++++++++---------
blockdev.c | 36 +++++++++++++++++++++++--
include/block/block_int.h | 3 ++-
include/block/dirty-bitmap.h | 3 +++
include/qemu/hbitmap.h | 2 ++
qapi/block-core.json | 4 ++-
util/hbitmap.c | 28 ++++++++++++++++++++
8 files changed, 136 insertions(+), 17 deletions(-)
+++ b/qapi/block-core.json
@@ -1727,6 +1727,8 @@
# (all the disk, only the sectors allocated in the topmost image, or
# only new I/O).
#
+# @bitmap-name: the bitmap to be used in incremental mode.
Missing a '(since 4.0)' tag. And doesn't really explain what
incremental mode in a mirroring operation actually means.
+#
# @granularity: granularity of the dirty bitmap, default is 64K
# if the image format doesn't have clusters, 4K if the clusters
# are smaller than that, else the cluster size. Must be a
@@ -1768,7 +1770,7 @@
{ 'struct': 'DriveMirror',
'data': { '*job-id': 'str', 'device': 'str', 'target': 'str',
'*format': 'str', '*node-name': 'str', '*replaces': 'str',
- 'sync': 'MirrorSyncMode', '*mode': 'NewImageMode',
+ 'sync': 'MirrorSyncMode', '*bitmap-name': 'str', '*mode':
'NewImageMode',
Can this be named just 'bitmap', instead of the longer 'bitmap-name'?
'*speed': 'int', '*granularity': 'uint32',
'*buf-size': 'int', '*on-source-error': 'BlockdevOnError',
'*on-target-error': 'BlockdevOnError',
diff --git a/util/hbitmap.c b/util/hbitmap.c
index 8d402c5..7ae2fc2 100644
--- a/util/hbitmap.c
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
