[Top][All Lists]

[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(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");

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

Best regards,

reply via email to

[Prev in Thread] Current Thread [Next in Thread]