[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v23 09/12] Implement new driver for block replic
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] [PATCH v23 09/12] Implement new driver for block replication |
Date: |
Tue, 26 Jul 2016 18:17:27 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 |
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.
> + 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
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().
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;
signature.asc
Description: OpenPGP digital signature
- [Qemu-devel] [PATCH v23 12/12] MAINTAINERS: add maintainer for replication, (continued)
- [Qemu-devel] [PATCH v23 12/12] MAINTAINERS: add maintainer for replication, Changlong Xie, 2016/07/26
- [Qemu-devel] [PATCH v23 08/12] Introduce new APIs to do replication operation, Changlong Xie, 2016/07/26
- [Qemu-devel] [PATCH v23 05/12] docs: block replication's description, Changlong Xie, 2016/07/26
- [Qemu-devel] [PATCH v23 07/12] configure: support replication, Changlong Xie, 2016/07/26
- [Qemu-devel] [PATCH v23 11/12] support replication driver in blockdev-add, Changlong Xie, 2016/07/26
- [Qemu-devel] [PATCH v23 09/12] Implement new driver for block replication, Changlong Xie, 2016/07/26
- Re: [Qemu-devel] [PATCH v23 09/12] Implement new driver for block replication,
Max Reitz <=
- [Qemu-devel] [PATCH v23 10/12] tests: add unit test case for replication, Changlong Xie, 2016/07/26