[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v5 4/5] replication: Assert that children are writable
From: |
Vladimir Sementsov-Ogievskiy |
Subject: |
Re: [PATCH v5 4/5] replication: Assert that children are writable |
Date: |
Thu, 15 Jul 2021 16:17:56 +0300 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 |
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
- [PATCH v5 0/5] replication: Bugfix and properly attach children, Lukas Straub, 2021/07/12
- [PATCH v5 2/5] replication: Reduce usage of s->hidden_disk and s->secondary_disk, Lukas Straub, 2021/07/12
- [PATCH v5 1/5] replication: Remove s->active_disk, Lukas Straub, 2021/07/12
- [PATCH v5 3/5] replication: Properly attach children, Lukas Straub, 2021/07/12
- [PATCH v5 5/5] replication: Remove workaround, Lukas Straub, 2021/07/12
- [PATCH v5 4/5] replication: Assert that children are writable, Lukas Straub, 2021/07/12
- Re: [PATCH v5 4/5] replication: Assert that children are writable,
Vladimir Sementsov-Ogievskiy <=