From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53167) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bSEJS-0000fO-QW for qemu-devel@nongnu.org; Tue, 26 Jul 2016 22:11:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bSEJQ-0007la-GZ for qemu-devel@nongnu.org; Tue, 26 Jul 2016 22:11:09 -0400 Message-ID: <57981971.5080405@cn.fujitsu.com> Date: Wed, 27 Jul 2016 10:16:17 +0800 From: Changlong Xie MIME-Version: 1.0 References: <1469520941-16283-1-git-send-email-xiecl.fnst@cn.fujitsu.com> <1469520941-16283-10-git-send-email-xiecl.fnst@cn.fujitsu.com> <38640520-a66b-fe88-2b69-9060a3483dab@redhat.com> In-Reply-To: <38640520-a66b-fe88-2b69-9060a3483dab@redhat.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v23 09/12] Implement new driver for block replication List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz , qemu devel , qemu block , Stefan Hajnoczi , Fam Zheng , Kevin Wolf , Jeff Cody Cc: Paolo Bonzini , John Snow , Eric Blake , Markus Armbruster , "Dr. David Alan Gilbert" , Dong Eddie , Jiang Yunhong , zhanghailiang , Gonglei , Wen Congyang , Wang Weiwei On 07/27/2016 12:17 AM, Max Reitz wrote: > On 26.07.2016 10:15, Changlong Xie wrote: >> From: Wen Congyang >> >> Signed-off-by: Wen Congyang >> Signed-off-by: Changlong Xie >> Signed-off-by: Wang WeiWei >> Signed-off-by: zhanghailiang >> Signed-off-by: Gonglei >> --- >> 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)) { > Max > >> + >> + backup_start("replication-backup", s->secondary_disk->bs, >> + s->hidden_disk->bs, 0, MIRROR_SYNC_MODE_NONE, NULL, >> + BLOCKDEV_ON_ERROR_REPORT, BLOCKDEV_ON_ERROR_REPORT, >> + 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; >