Re: [PATCH v5 4/5] replication: Assert that children are writable

[Vladimir Sementsov-Ogievskiy]

12.07.2021 14:54, Lukas Straub wrote:
> Assert that the children are writable where it's needed.
> While there is no test-case for the
> BLOCK_REPLICATION_FAILOVER_FAILED state, this at least ensures that
> s->secondary_disk is always writable in case replication might go
> into that state.
>
> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> ---
>   block/replication.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
>
> diff --git a/block/replication.c b/block/replication.c
> index b74192f795..772bb63374 100644
> --- a/block/replication.c
> +++ b/block/replication.c
> @@ -261,6 +261,13 @@ static coroutine_fn int 
> replication_co_writev(BlockDriverState *bs,
>       int64_t n;
>
>       assert(!flags);
> +    assert(top->perm & BLK_PERM_WRITE);
> +    if (s->mode == REPLICATION_MODE_SECONDARY &&
> +        s->stage != BLOCK_REPLICATION_NONE &&
> +        s->stage != BLOCK_REPLICATION_DONE) {
> +        assert(base->perm & BLK_PERM_WRITE);
> +    }
> +

write has assertions in generic code so actually we don't need this.

Also using this additional conditions is not obvious. Better is assert about 
base without extra conditiions exactly before while loop.

>       ret = replication_get_io_status(s);
>       if (ret < 0) {
>           goto out;
> @@ -318,6 +325,9 @@ static void secondary_do_checkpoint(BlockDriverState *bs, 
> Error **errp)
>       Error *local_err = NULL;
>       int ret;
>
> +    assert(active_disk->perm & BLK_PERM_WRITE);
> +    assert(s->hidden_disk->perm & BLK_PERM_WRITE);

Oops, bdrv_make_empty also has this assertion inside. It also is satisfied by 
BLK_PERM_WRITE_UNCHANGED, but we don't work with it here anyway. So we don't 
need it.

> +
>       if (!s->backup_job) {
>           error_setg(errp, "Backup job was cancelled unexpectedly");
>           return;
> --
> 2.20.1

Sorry, seems my suggestion to add assertions was bad, as we already have them 
in generic code.

--
Best regards,
Vladimir