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

[Changlong Xie <xiecl.fnst@cn.fujitsu.com>]

On 07/27/2016 12:17 AM, Max Reitz wrote:
> On 26.07.2016 10:15, Changlong Xie wrote:
>> From: Wen Congyang <wency@cn.fujitsu.com>
>>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>> Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
>> Signed-off-by: Wang WeiWei <wangww.fnst@cn.fujitsu.com>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>> ---
>>   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;
>