[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [PATCH v23 09/12] Implement new driver for block replic

From: Changlong Xie
Subject: Re: [Qemu-block] [PATCH v23 09/12] Implement new driver for block replication
Date: Wed, 27 Jul 2016 10:16:17 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0

On 07/27/2016 12:17 AM, Max Reitz wrote:
On 26.07.2016 10:15, Changlong Xie wrote:
From: Wen Congyang <address@hidden>

Signed-off-by: Wen Congyang <address@hidden>
Signed-off-by: Changlong Xie <address@hidden>
Signed-off-by: Wang WeiWei <address@hidden>
Signed-off-by: zhanghailiang <address@hidden>
Signed-off-by: Gonglei <address@hidden>
  block/Makefile.objs |   1 +
  block/replication.c | 658 ++++++++++++++++++++++++++++++++++++++++++++++++++++
  2 files changed, 659 insertions(+)
  create mode 100644 block/replication.c


diff --git a/block/replication.c b/block/replication.c
new file mode 100644
index 0000000..ec35348
--- /dev/null
+++ b/block/replication.c
@@ -0,0 +1,658 @@


+static void replication_start(ReplicationState *rs, ReplicationMode mode,
+                              Error **errp)


+        /* start backup job now */
+        error_setg(&s->blocker,
+                   "Block device is in use by internal backup job");
+        top_bs = bdrv_lookup_bs(s->top_id, s->top_id, errp);

I think you should pass NULL instead of errp...

+        if (!top_bs || !check_top_bs(top_bs, bs)) {
+            error_setg(errp, "No top_bs or it is invalid");

...or if you don't, then you should not call this function if top_bs is
NULL. Otherwise you'll probably get a failed assertion in error_setv()
because *errp is not NULL.

Thanks for pointing it out. if top_is is NULL, *errp will be set in bdrv_lookup_bs(). Then we'll get failed assertion in error_setv(). Will
fix it.

+            reopen_backing_file(s, false, NULL);
+            aio_context_release(aio_context);
+            return;
+        }
+        bdrv_op_block_all(top_bs, s->blocker);
+        bdrv_op_unblock(top_bs, BLOCK_OP_TYPE_DATAPLANE, s->blocker);

Shouldn't you make sure that top_bs is a root node? The first patch in

Indeed, it should be a root node

Kevin's "block: Accept node-name in all node level QMP commands" series
introduces the bdrv_is_root_node() function for that purpose.

Maybe that check should be put into check_top_bs().

I think we just need check top_bs is a root node or not one time before stepping in check_top_bs().

if (!top_bs || !bdrv_is_root_node(top_bs) ||
    !check_top_bs(top_bs, bs)) {


+        backup_start("replication-backup", s->secondary_disk->bs,
+                     s->hidden_disk->bs, 0, MIRROR_SYNC_MODE_NONE, NULL,
+                     backup_job_completed, s, NULL, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            backup_job_cleanup(s);
+            aio_context_release(aio_context);
+            return;
+        }
+        break;

reply via email to

[Prev in Thread] Current Thread [Next in Thread]