[Qemu-devel] [PULL 00/14] Block layer patches

[Qemu-devel] [PULL 00/14] Block layer patches

[Kevin Wolf]

The following changes since commit cdfaa2720f4a09e5254868bd1f6e33f3e9eae76f:

  Merge remote-tracking branch 'remotes/armbru/tags/pull-monitor-2019-06-17-v2' into staging (2019-06-18 10:47:00 +0100)

are available in the Git repository at:

  git://repo.or.cz/qemu/kevin.git tags/for-upstream

for you to fetch changes up to 128b05f7e00765d883164631b974a27af5b4b613:

  block/null: Expose read-zeroes option in QAPI schema (2019-06-18 16:41:10 +0200)

----------------------------------------------------------------
Block layer patches:

- block: Remove bs->job
- block: Ignore loosening perm restrictions failures
- block/null: Expose read-zeroes option in QAPI schema
- iotests: Hide timestamps for skipped tests

----------------------------------------------------------------
Kevin Wolf (1):
      block/null: Expose read-zeroes option in QAPI schema

Max Reitz (9):
      iotests: Hide timestamps for skipped tests
      file-posix: Update open_flags in raw_set_perm()
      block: Add bdrv_child_refresh_perms()
      block/mirror: Fix child permissions
      block/commit: Drop bdrv_child_try_set_perm()
      block: Fix order in bdrv_replace_child()
      block: Add *tighten_restrictions to *check*_perm()
      block: Ignore loosening perm restrictions failures
      iotests: Test failure to loosen restrictions

Vladimir Sementsov-Ogievskiy (4):
      block/replication: drop usage of bs->job
      block/block-backend: blk_iostatus_reset: drop usage of bs->job
      blockdev: blockdev_mark_auto_del: drop usage of bs->job
      block: drop bs->job

 qapi/block-core.json       |   4 +-
 include/block/block_int.h  |  30 ++++++---
 include/block/blockjob.h   |   9 +++
 block.c                    | 155 +++++++++++++++++++++++++++++++++++++--------
 block/block-backend.c      |   4 --
 block/commit.c             |   2 -
 block/file-posix.c         |   4 ++
 block/mirror.c             |  70 ++++++++++++--------
 block/replication.c        |  21 +++---
 blockdev.c                 |  19 +++---
 blockjob.c                 |  22 ++++---
 monitor/qmp-cmds.c         |   5 ++
 tests/test-blockjob.c      |   5 +-
 block/trace-events         |   2 +-
 tests/qemu-iotests/182     |  21 ++++++
 tests/qemu-iotests/182.out |   6 ++
 tests/qemu-iotests/check   |   1 +
 17 files changed, 283 insertions(+), 97 deletions(-)

[Qemu-devel] [PULL 01/14] iotests: Hide timestamps for skipped tests

[Kevin Wolf]

From: Max Reitz <mreitz@redhat.com>

Currently, the "thistime" variable is not reinitialized on every loop
iteration.  This leads to tests that do not yield a run time (because
they failed or were skipped) printing the run time of the previous test
that did.  Fix that by reinitializing "thistime" for every test.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/check | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index 44ebf24080..f925606cc5 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -773,6 +773,7 @@ do
     printdiff=false # show diff to reference output?
     status=""       # test result summary
     results=""      # test result details
+    thistime=""     # time the test took
 
     if [ -n "$TESTS_REMAINING_LOG" ] ; then
         sed -e "s/$seq//" -e 's/  / /' -e 's/^ *//' $TESTS_REMAINING_LOG > $TESTS_REMAINING_LOG.tmp
-- 
2.20.1

[Qemu-devel] [PULL 02/14] block/replication: drop usage of bs->job

[Kevin Wolf]

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

We are going to remove bs->job pointer. Drop it's usage in replication
code. Additionally we have to return job pointer from some mirror APIs.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/block_int.h | 12 ++++++------
 block/mirror.c            | 38 ++++++++++++++++++++++----------------
 block/replication.c       | 21 ++++++++++++---------
 3 files changed, 40 insertions(+), 31 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 06df2bda1b..8bb1cfb80a 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1082,12 +1082,12 @@ void commit_start(const char *job_id, BlockDriverState *bs,
  * @errp: Error object.
  *
  */
-void commit_active_start(const char *job_id, BlockDriverState *bs,
-                         BlockDriverState *base, int creation_flags,
-                         int64_t speed, BlockdevOnError on_error,
-                         const char *filter_node_name,
-                         BlockCompletionFunc *cb, void *opaque,
-                         bool auto_complete, Error **errp);
+BlockJob *commit_active_start(const char *job_id, BlockDriverState *bs,
+                              BlockDriverState *base, int creation_flags,
+                              int64_t speed, BlockdevOnError on_error,
+                              const char *filter_node_name,
+                              BlockCompletionFunc *cb, void *opaque,
+                              bool auto_complete, Error **errp);
 /*
  * mirror_start:
  * @job_id: The id of the newly-created job, or %NULL to use the
diff --git a/block/mirror.c b/block/mirror.c
index f8bdb5b21b..b5878ba574 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1482,7 +1482,8 @@ static BlockDriver bdrv_mirror_top = {
     .bdrv_child_perm            = bdrv_mirror_top_child_perm,
 };
 
-static void mirror_start_job(const char *job_id, BlockDriverState *bs,
+static BlockJob *mirror_start_job(
+                             const char *job_id, BlockDriverState *bs,
                              int creation_flags, BlockDriverState *target,
                              const char *replaces, int64_t speed,
                              uint32_t granularity, int64_t buf_size,
@@ -1514,7 +1515,7 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
 
     if (buf_size < 0) {
         error_setg(errp, "Invalid parameter 'buf-size'");
-        return;
+        return NULL;
     }
 
     if (buf_size == 0) {
@@ -1523,7 +1524,7 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
 
     if (bs == target) {
         error_setg(errp, "Can't mirror node into itself");
-        return;
+        return NULL;
     }
 
     /* In the case of active commit, add dummy driver to provide consistent
@@ -1532,7 +1533,7 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
     mirror_top_bs = bdrv_new_open_driver(&bdrv_mirror_top, filter_node_name,
                                          BDRV_O_RDWR, errp);
     if (mirror_top_bs == NULL) {
-        return;
+        return NULL;
     }
     if (!filter_node_name) {
         mirror_top_bs->implicit = true;
@@ -1554,7 +1555,7 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
     if (local_err) {
         bdrv_unref(mirror_top_bs);
         error_propagate(errp, local_err);
-        return;
+        return NULL;
     }
 
     /* Make sure that the source is not resized while the job is running */
@@ -1662,7 +1663,8 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
 
     trace_mirror_start(bs, s, opaque);
     job_start(&s->common.job);
-    return;
+
+    return &s->common;
 
 fail:
     if (s) {
@@ -1684,6 +1686,8 @@ fail:
     bdrv_replace_node(mirror_top_bs, backing_bs(mirror_top_bs), &error_abort);
 
     bdrv_unref(mirror_top_bs);
+
+    return NULL;
 }
 
 void mirror_start(const char *job_id, BlockDriverState *bs,
@@ -1712,25 +1716,27 @@ void mirror_start(const char *job_id, BlockDriverState *bs,
                      filter_node_name, true, copy_mode, errp);
 }
 
-void commit_active_start(const char *job_id, BlockDriverState *bs,
-                         BlockDriverState *base, int creation_flags,
-                         int64_t speed, BlockdevOnError on_error,
-                         const char *filter_node_name,
-                         BlockCompletionFunc *cb, void *opaque,
-                         bool auto_complete, Error **errp)
+BlockJob *commit_active_start(const char *job_id, BlockDriverState *bs,
+                              BlockDriverState *base, int creation_flags,
+                              int64_t speed, BlockdevOnError on_error,
+                              const char *filter_node_name,
+                              BlockCompletionFunc *cb, void *opaque,
+                              bool auto_complete, Error **errp)
 {
     bool base_read_only;
     Error *local_err = NULL;
+    BlockJob *ret;
 
     base_read_only = bdrv_is_read_only(base);
 
     if (base_read_only) {
         if (bdrv_reopen_set_read_only(base, false, errp) < 0) {
-            return;
+            return NULL;
         }
     }
 
-    mirror_start_job(job_id, bs, creation_flags, base, NULL, speed, 0, 0,
+    ret = mirror_start_job(
+                     job_id, bs, creation_flags, base, NULL, speed, 0, 0,
                      MIRROR_LEAVE_BACKING_CHAIN,
                      on_error, on_error, true, cb, opaque,
                      &commit_active_job_driver, false, base, auto_complete,
@@ -1741,7 +1747,7 @@ void commit_active_start(const char *job_id, BlockDriverState *bs,
         goto error_restore_flags;
     }
 
-    return;
+    return ret;
 
 error_restore_flags:
     /* ignore error and errp for bdrv_reopen, because we want to propagate
@@ -1749,5 +1755,5 @@ error_restore_flags:
     if (base_read_only) {
         bdrv_reopen_set_read_only(base, true, NULL);
     }
-    return;
+    return NULL;
 }
diff --git a/block/replication.c b/block/replication.c
index 5cffb790b3..b41bc507c0 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -36,8 +36,10 @@ typedef struct BDRVReplicationState {
     ReplicationMode mode;
     ReplicationStage stage;
     BdrvChild *active_disk;
+    BlockJob *commit_job;
     BdrvChild *hidden_disk;
     BdrvChild *secondary_disk;
+    BlockJob *backup_job;
     char *top_id;
     ReplicationState *rs;
     Error *blocker;
@@ -147,7 +149,7 @@ static void replication_close(BlockDriverState *bs)
         replication_stop(s->rs, false, NULL);
     }
     if (s->stage == BLOCK_REPLICATION_FAILOVER) {
-        job_cancel_sync(&s->active_disk->bs->job->job);
+        job_cancel_sync(&s->commit_job->job);
     }
 
     if (s->mode == REPLICATION_MODE_SECONDARY) {
@@ -315,12 +317,12 @@ static void secondary_do_checkpoint(BDRVReplicationState *s, Error **errp)
     Error *local_err = NULL;
     int ret;
 
-    if (!s->secondary_disk->bs->job) {
+    if (!s->backup_job) {
         error_setg(errp, "Backup job was cancelled unexpectedly");
         return;
     }
 
-    backup_do_checkpoint(s->secondary_disk->bs->job, &local_err);
+    backup_do_checkpoint(s->backup_job, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return;
@@ -449,7 +451,6 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
     int64_t active_length, hidden_length, disk_length;
     AioContext *aio_context;
     Error *local_err = NULL;
-    BlockJob *job;
 
     aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(aio_context);
@@ -540,7 +541,8 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
         bdrv_op_block_all(top_bs, s->blocker);
         bdrv_op_unblock(top_bs, BLOCK_OP_TYPE_DATAPLANE, s->blocker);
 
-        job = backup_job_create(NULL, s->secondary_disk->bs, s->hidden_disk->bs,
+        s->backup_job = backup_job_create(
+                                NULL, s->secondary_disk->bs, s->hidden_disk->bs,
                                 0, MIRROR_SYNC_MODE_NONE, NULL, false,
                                 BLOCKDEV_ON_ERROR_REPORT,
                                 BLOCKDEV_ON_ERROR_REPORT, JOB_INTERNAL,
@@ -551,7 +553,7 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
             aio_context_release(aio_context);
             return;
         }
-        job_start(&job->job);
+        job_start(&s->backup_job->job);
         break;
     default:
         aio_context_release(aio_context);
@@ -653,8 +655,8 @@ static void replication_stop(ReplicationState *rs, bool failover, Error **errp)
          * before the BDS is closed, because we will access hidden
          * disk, secondary disk in backup_job_completed().
          */
-        if (s->secondary_disk->bs->job) {
-            job_cancel_sync(&s->secondary_disk->bs->job->job);
+        if (s->backup_job) {
+            job_cancel_sync(&s->backup_job->job);
         }
 
         if (!failover) {
@@ -665,7 +667,8 @@ static void replication_stop(ReplicationState *rs, bool failover, Error **errp)
         }
 
         s->stage = BLOCK_REPLICATION_FAILOVER;
-        commit_active_start(NULL, s->active_disk->bs, s->secondary_disk->bs,
+        s->commit_job = commit_active_start(
+                            NULL, s->active_disk->bs, s->secondary_disk->bs,
                             JOB_INTERNAL, 0, BLOCKDEV_ON_ERROR_REPORT,
                             NULL, replication_done, bs, true, errp);
         break;
-- 
2.20.1

[Qemu-devel] [PULL 03/14] block/block-backend: blk_iostatus_reset: drop usage of bs->job

[Kevin Wolf]

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

We are going to remove bs->job pointer. Drop it's usage in
blk_iostatus_reset.

blk_iostatus_reset() has only two callers:

1. blk_attach_dev(). This doesn't have anything to do with jobs and
    attaching a new guest device won't solve any problem the job
    encountered, so no reason to reset the iostatus for the job.

2. qmp_cont(). This resets the iostatus for everything. We can just
    call block_job_iostatus_reset() for all block jobs instead of going
    through BlockBackend.

Suggested-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/block-backend.c | 4 ----
 monitor/qmp-cmds.c    | 5 +++++
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index f5d9407d20..a8d160fd5d 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1073,11 +1073,7 @@ void blk_iostatus_disable(BlockBackend *blk)
 void blk_iostatus_reset(BlockBackend *blk)
 {
     if (blk_iostatus_is_enabled(blk)) {
-        BlockDriverState *bs = blk_bs(blk);
         blk->iostatus = BLOCK_DEVICE_IO_STATUS_OK;
-        if (bs && bs->job) {
-            block_job_iostatus_reset(bs->job);
-        }
     }
 }
 
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index f1b1e4f08b..01ce77e129 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -143,6 +143,7 @@ void qmp_x_exit_preconfig(Error **errp)
 void qmp_cont(Error **errp)
 {
     BlockBackend *blk;
+    BlockJob *job;
     Error *local_err = NULL;
 
     /* if there is a dump in background, we should wait until the dump
@@ -166,6 +167,10 @@ void qmp_cont(Error **errp)
         blk_iostatus_reset(blk);
     }
 
+    for (job = block_job_next(NULL); job; job = block_job_next(job)) {
+        block_job_iostatus_reset(job);
+    }
+
     /* Continuing after completed migration. Images have been inactivated to
      * allow the destination to take control. Need to get control back now.
      *
-- 
2.20.1

[Qemu-devel] [PULL 04/14] blockdev: blockdev_mark_auto_del: drop usage of bs->job

[Kevin Wolf]

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

We are going to remove bs->job pointer. Drop it's usage in
blockdev_mark_auto_del: instead of looking at bs->job let's check all
jobs for references to bs.

Suggested-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/blockjob.h |  9 +++++++++
 blockdev.c               | 17 ++++++++---------
 blockjob.c               | 14 ++++++++++++++
 3 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index ede0bd8dcb..35faa3aa26 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -121,6 +121,15 @@ int block_job_add_bdrv(BlockJob *job, const char *name, BlockDriverState *bs,
  */
 void block_job_remove_all_bdrv(BlockJob *job);
 
+/**
+ * block_job_has_bdrv:
+ * @job: The block job
+ *
+ * Searches for @bs in the list of nodes that are involved in the
+ * job.
+ */
+bool block_job_has_bdrv(BlockJob *job, BlockDriverState *bs);
+
 /**
  * block_job_set_speed:
  * @job: The job to set the speed for.
diff --git a/blockdev.c b/blockdev.c
index b5c0fd3c49..a9dd73eafc 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -140,22 +140,21 @@ void override_max_devs(BlockInterfaceType type, int max_devs)
 void blockdev_mark_auto_del(BlockBackend *blk)
 {
     DriveInfo *dinfo = blk_legacy_dinfo(blk);
-    BlockDriverState *bs = blk_bs(blk);
-    AioContext *aio_context;
+    BlockJob *job;
 
     if (!dinfo) {
         return;
     }
 
-    if (bs) {
-        aio_context = bdrv_get_aio_context(bs);
-        aio_context_acquire(aio_context);
+    for (job = block_job_next(NULL); job; job = block_job_next(job)) {
+        if (block_job_has_bdrv(job, blk_bs(blk))) {
+            AioContext *aio_context = job->job.aio_context;
+            aio_context_acquire(aio_context);
 
-        if (bs->job) {
-            job_cancel(&bs->job->job, false);
-        }
+            job_cancel(&job->job, false);
 
-        aio_context_release(aio_context);
+            aio_context_release(aio_context);
+        }
     }
 
     dinfo->auto_del = 1;
diff --git a/blockjob.c b/blockjob.c
index 1fac6bb8a7..c3620ec544 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -198,6 +198,20 @@ void block_job_remove_all_bdrv(BlockJob *job)
     job->nodes = NULL;
 }
 
+bool block_job_has_bdrv(BlockJob *job, BlockDriverState *bs)
+{
+    GSList *el;
+
+    for (el = job->nodes; el; el = el->next) {
+        BdrvChild *c = el->data;
+        if (c->bs == bs) {
+            return true;
+        }
+    }
+
+    return false;
+}
+
 int block_job_add_bdrv(BlockJob *job, const char *name, BlockDriverState *bs,
                        uint64_t perm, uint64_t shared_perm, Error **errp)
 {
-- 
2.20.1

[Qemu-devel] [PULL 05/14] block: drop bs->job

[Kevin Wolf]

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Drop remaining users of bs->job:
1. assertions actually duplicated by assert(!bs->refcnt)
2. trace-point seems not enough reason to change stream_start to return
   BlockJob pointer
3. Restricting creation of two jobs based on same bs is bad idea, as
   3.1 Some jobs creates filters to be their main node, so, this check
   don't actually prevent creating second job on same real node (which
   will create another filter node) (but I hope it is restricted by
   other mechanisms)
   3.2 Even without bs->job we have two systems of permissions:
   op-blockers and BLK_PERM
   3.3 We may want to run several jobs on one node one day

And finally, drop bs->job pointer itself. Hurrah!

Suggested-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/block_int.h | 3 ---
 block.c                   | 2 --
 blockdev.c                | 2 +-
 blockjob.c                | 8 --------
 tests/test-blockjob.c     | 5 +++--
 block/trace-events        | 2 +-
 6 files changed, 5 insertions(+), 17 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 8bb1cfb80a..a498c2670b 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -812,9 +812,6 @@ struct BlockDriverState {
     /* operation blockers */
     QLIST_HEAD(, BdrvOpBlocker) op_blockers[BLOCK_OP_TYPE_MAX];
 
-    /* long-running background operation */
-    BlockJob *job;
-
     /* The node that this node inherited default options from (and a reopen on
      * which can affect this node by changing these defaults). This is always a
      * parent node of this node. */
diff --git a/block.c b/block.c
index e3e77feee0..ceb2ea23c5 100644
--- a/block.c
+++ b/block.c
@@ -3905,7 +3905,6 @@ static void bdrv_close(BlockDriverState *bs)
     BdrvAioNotifier *ban, *ban_next;
     BdrvChild *child, *next;
 
-    assert(!bs->job);
     assert(!bs->refcnt);
 
     bdrv_drained_begin(bs); /* complete I/O */
@@ -4146,7 +4145,6 @@ out:
 
 static void bdrv_delete(BlockDriverState *bs)
 {
-    assert(!bs->job);
     assert(bdrv_op_blocker_is_empty(bs));
     assert(!bs->refcnt);
 
diff --git a/blockdev.c b/blockdev.c
index a9dd73eafc..5d6a13dea9 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3260,7 +3260,7 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
         goto out;
     }
 
-    trace_qmp_block_stream(bs, bs->job);
+    trace_qmp_block_stream(bs);
 
 out:
     aio_context_release(aio_context);
diff --git a/blockjob.c b/blockjob.c
index c3620ec544..458ae76f51 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -83,9 +83,7 @@ BlockJob *block_job_get(const char *id)
 void block_job_free(Job *job)
 {
     BlockJob *bjob = container_of(job, BlockJob, job);
-    BlockDriverState *bs = blk_bs(bjob->blk);
 
-    bs->job = NULL;
     block_job_remove_all_bdrv(bjob);
     blk_unref(bjob->blk);
     error_free(bjob->blocker);
@@ -402,11 +400,6 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
     BlockJob *job;
     int ret;
 
-    if (bs->job) {
-        error_setg(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(bs));
-        return NULL;
-    }
-
     if (job_id == NULL && !(flags & JOB_INTERNAL)) {
         job_id = bdrv_get_device_name(bs);
     }
@@ -449,7 +442,6 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
     error_setg(&job->blocker, "block device is in use by block job: %s",
                job_type_str(&job->job));
     block_job_add_bdrv(job, "main node", bs, 0, BLK_PERM_ALL, &error_abort);
-    bs->job = job;
 
     bdrv_op_unblock(bs, BLOCK_OP_TYPE_DATAPLANE, job->blocker);
 
diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
index 8c91980c70..b33f899873 100644
--- a/tests/test-blockjob.c
+++ b/tests/test-blockjob.c
@@ -122,8 +122,9 @@ static void test_job_ids(void)
     /* This one is valid */
     job[0] = do_test_id(blk[0], "id0", true);
 
-    /* We cannot have two jobs in the same BDS */
-    do_test_id(blk[0], "id1", false);
+    /* We can have two jobs in the same BDS */
+    job[1] = do_test_id(blk[0], "id1", true);
+    job_early_fail(&job[1]->job);
 
     /* Duplicate job IDs are not allowed */
     job[1] = do_test_id(blk[1], "id0", false);
diff --git a/block/trace-events b/block/trace-events
index f6e43ee023..9ccea755da 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -53,7 +53,7 @@ qmp_block_job_resume(void *job) "job %p"
 qmp_block_job_complete(void *job) "job %p"
 qmp_block_job_finalize(void *job) "job %p"
 qmp_block_job_dismiss(void *job) "job %p"
-qmp_block_stream(void *bs, void *job) "bs %p job %p"
+qmp_block_stream(void *bs) "bs %p"
 
 # file-posix.c
 # file-win32.c
-- 
2.20.1

[Qemu-devel] [PULL 06/14] file-posix: Update open_flags in raw_set_perm()

[Kevin Wolf]

From: Max Reitz <mreitz@redhat.com>

raw_check_perm() + raw_set_perm() can change the flags associated with
the current FD.  If so, we have to update BDRVRawState.open_flags
accordingly.  Otherwise, we may keep reopening the FD even though the
current one already has the correct flags.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/file-posix.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/block/file-posix.c b/block/file-posix.c
index 83ab1b78ef..ab05b51a66 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -146,6 +146,7 @@ typedef struct BDRVRawState {
     uint64_t locked_shared_perm;
 
     int perm_change_fd;
+    int perm_change_flags;
     BDRVReopenState *reopen_state;
 
 #ifdef CONFIG_XFS
@@ -2788,6 +2789,7 @@ static int raw_check_perm(BlockDriverState *bs, uint64_t perm, uint64_t shared,
         assert(s->reopen_state->shared_perm == shared);
         rs = s->reopen_state->opaque;
         s->perm_change_fd = rs->fd;
+        s->perm_change_flags = rs->open_flags;
     } else {
         /* We may need a new fd if auto-read-only switches the mode */
         ret = raw_reconfigure_getfd(bs, bs->open_flags, &open_flags, perm,
@@ -2796,6 +2798,7 @@ static int raw_check_perm(BlockDriverState *bs, uint64_t perm, uint64_t shared,
             return ret;
         } else if (ret != s->fd) {
             s->perm_change_fd = ret;
+            s->perm_change_flags = open_flags;
         }
     }
 
@@ -2834,6 +2837,7 @@ static void raw_set_perm(BlockDriverState *bs, uint64_t perm, uint64_t shared)
     if (s->perm_change_fd && s->fd != s->perm_change_fd) {
         qemu_close(s->fd);
         s->fd = s->perm_change_fd;
+        s->open_flags = s->perm_change_flags;
     }
     s->perm_change_fd = 0;
 
-- 
2.20.1

[Qemu-devel] [PULL 07/14] block: Add bdrv_child_refresh_perms()

[Kevin Wolf]

From: Max Reitz <mreitz@redhat.com>

If a block node uses bdrv_child_try_set_perm() to change the permission
it takes on its child, the result may be very short-lived.  If anything
makes the block layer recalculate the permissions internally, it will
invoke the node driver's .bdrv_child_perm() implementation.  The
permission/shared permissions masks that returns will then override the
values previously passed to bdrv_child_try_set_perm().

If drivers want a child edge to have specific values for the
permissions/shared permissions mask, it must return them in
.bdrv_child_perm().  Consequentially, there is no need for them to pass
the same values to bdrv_child_try_set_perm() then: It is better to have
a function that invokes .bdrv_child_perm() and calls
bdrv_child_try_set_perm() with the result.  This patch adds such a
function under the name of bdrv_child_refresh_perms().

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/block_int.h | 15 +++++++++++++++
 block.c                   | 12 ++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index a498c2670b..d6415b53c1 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1165,9 +1165,24 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
                                   void *opaque, Error **errp);
 void bdrv_root_unref_child(BdrvChild *child);
 
+/**
+ * Sets a BdrvChild's permissions.  Avoid if the parent is a BDS; use
+ * bdrv_child_refresh_perms() instead and make the parent's
+ * .bdrv_child_perm() implementation return the correct values.
+ */
 int bdrv_child_try_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared,
                             Error **errp);
 
+/**
+ * Calls bs->drv->bdrv_child_perm() and updates the child's permission
+ * masks with the result.
+ * Drivers should invoke this function whenever an event occurs that
+ * makes their .bdrv_child_perm() implementation return different
+ * values than before, but which will not result in the block layer
+ * automatically refreshing the permissions.
+ */
+int bdrv_child_refresh_perms(BlockDriverState *bs, BdrvChild *c, Error **errp);
+
 /* Default implementation for BlockDriver.bdrv_child_perm() that can be used by
  * block filters: Forward CONSISTENT_READ, WRITE, WRITE_UNCHANGED and RESIZE to
  * all children */
diff --git a/block.c b/block.c
index ceb2ea23c5..013369851b 100644
--- a/block.c
+++ b/block.c
@@ -2083,6 +2083,18 @@ int bdrv_child_try_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared,
     return 0;
 }
 
+int bdrv_child_refresh_perms(BlockDriverState *bs, BdrvChild *c, Error **errp)
+{
+    uint64_t parent_perms, parent_shared;
+    uint64_t perms, shared;
+
+    bdrv_get_cumulative_perm(bs, &parent_perms, &parent_shared);
+    bdrv_child_perm(bs, c->bs, c, c->role, NULL, parent_perms, parent_shared,
+                    &perms, &shared);
+
+    return bdrv_child_try_set_perm(c, perms, shared, errp);
+}
+
 void bdrv_filter_default_perms(BlockDriverState *bs, BdrvChild *c,
                                const BdrvChildRole *role,
                                BlockReopenQueue *reopen_queue,
-- 
2.20.1

[Qemu-devel] [PULL 08/14] block/mirror: Fix child permissions

[Kevin Wolf]

From: Max Reitz <mreitz@redhat.com>

We cannot use bdrv_child_try_set_perm() to give up all restrictions on
the child edge, and still have bdrv_mirror_top_child_perm() request
BLK_PERM_WRITE.  Fix this by making bdrv_mirror_top_child_perm() return
0/BLK_PERM_ALL when we want to give up all permissions, and replacing
bdrv_child_try_set_perm() by bdrv_child_refresh_perms().

The bdrv_child_try_set_perm() before removing the node with
bdrv_replace_node() is then unnecessary.  No permissions have changed
since the previous invocation of bdrv_child_try_set_perm().

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/mirror.c | 32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index b5878ba574..d17be4cdbc 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -85,6 +85,7 @@ typedef struct MirrorBlockJob {
 
 typedef struct MirrorBDSOpaque {
     MirrorBlockJob *job;
+    bool stop;
 } MirrorBDSOpaque;
 
 struct MirrorOp {
@@ -656,8 +657,9 @@ static int mirror_exit_common(Job *job)
 
     /* We don't access the source any more. Dropping any WRITE/RESIZE is
      * required before it could become a backing file of target_bs. */
-    bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL,
-                            &error_abort);
+    bs_opaque->stop = true;
+    bdrv_child_refresh_perms(mirror_top_bs, mirror_top_bs->backing,
+                             &error_abort);
     if (!abort && s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
         BlockDriverState *backing = s->is_none_mode ? src : s->base;
         if (backing_bs(target_bs) != backing) {
@@ -704,13 +706,12 @@ static int mirror_exit_common(Job *job)
     g_free(s->replaces);
     bdrv_unref(target_bs);
 
-    /* Remove the mirror filter driver from the graph. Before this, get rid of
+    /*
+     * Remove the mirror filter driver from the graph. Before this, get rid of
      * the blockers on the intermediate nodes so that the resulting state is
-     * valid. Also give up permissions on mirror_top_bs->backing, which might
-     * block the removal. */
+     * valid.
+     */
     block_job_remove_all_bdrv(bjob);
-    bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL,
-                            &error_abort);
     bdrv_replace_node(mirror_top_bs, backing_bs(mirror_top_bs), &error_abort);
 
     /* We just changed the BDS the job BB refers to (with either or both of the
@@ -1459,6 +1460,18 @@ static void bdrv_mirror_top_child_perm(BlockDriverState *bs, BdrvChild *c,
                                        uint64_t perm, uint64_t shared,
                                        uint64_t *nperm, uint64_t *nshared)
 {
+    MirrorBDSOpaque *s = bs->opaque;
+
+    if (s->stop) {
+        /*
+         * If the job is to be stopped, we do not need to forward
+         * anything to the real image.
+         */
+        *nperm = 0;
+        *nshared = BLK_PERM_ALL;
+        return;
+    }
+
     /* Must be able to forward guest writes to the real image */
     *nperm = 0;
     if (perm & BLK_PERM_WRITE) {
@@ -1681,8 +1694,9 @@ fail:
         job_early_fail(&s->common.job);
     }
 
-    bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL,
-                            &error_abort);
+    bs_opaque->stop = true;
+    bdrv_child_refresh_perms(mirror_top_bs, mirror_top_bs->backing,
+                             &error_abort);
     bdrv_replace_node(mirror_top_bs, backing_bs(mirror_top_bs), &error_abort);
 
     bdrv_unref(mirror_top_bs);
-- 
2.20.1

[Qemu-devel] [PULL 09/14] block/commit: Drop bdrv_child_try_set_perm()

[Kevin Wolf]

From: Max Reitz <mreitz@redhat.com>

commit_top_bs never requests or unshares any permissions.  There is no
reason to make this so explicit here.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/commit.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/block/commit.c b/block/commit.c
index c815def89a..212c6f639e 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -110,8 +110,6 @@ static void commit_abort(Job *job)
      * XXX Can (or should) we somehow keep 'consistent read' blocked even
      * after the failed/cancelled commit job is gone? If we already wrote
      * something to base, the intermediate images aren't valid any more. */
-    bdrv_child_try_set_perm(s->commit_top_bs->backing, 0, BLK_PERM_ALL,
-                            &error_abort);
     bdrv_replace_node(s->commit_top_bs, backing_bs(s->commit_top_bs),
                       &error_abort);
 
-- 
2.20.1

[Qemu-devel] [PULL 10/14] block: Fix order in bdrv_replace_child()

[Kevin Wolf]

From: Max Reitz <mreitz@redhat.com>

We have to start by applying the permission restrictions to new_bs
before we can loosen them on old_bs.  See the comment for the
explanation.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/block.c b/block.c
index 013369851b..b7d4149c2f 100644
--- a/block.c
+++ b/block.c
@@ -2240,6 +2240,19 @@ static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs)
 
     bdrv_replace_child_noperm(child, new_bs);
 
+    /*
+     * Start with the new node's permissions.  If @new_bs is a (direct
+     * or indirect) child of @old_bs, we must complete the permission
+     * update on @new_bs before we loosen the restrictions on @old_bs.
+     * Otherwise, bdrv_check_perm() on @old_bs would re-initiate
+     * updating the permissions of @new_bs, and thus not purely loosen
+     * restrictions.
+     */
+    if (new_bs) {
+        bdrv_get_cumulative_perm(new_bs, &perm, &shared_perm);
+        bdrv_set_perm(new_bs, perm, shared_perm);
+    }
+
     if (old_bs) {
         /* Update permissions for old node. This is guaranteed to succeed
          * because we're just taking a parent away, so we're loosening
@@ -2252,11 +2265,6 @@ static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs)
          * node moves back to the main AioContext */
         bdrv_try_set_aio_context(old_bs, qemu_get_aio_context(), NULL);
     }
-
-    if (new_bs) {
-        bdrv_get_cumulative_perm(new_bs, &perm, &shared_perm);
-        bdrv_set_perm(new_bs, perm, shared_perm);
-    }
 }
 
 /*
-- 
2.20.1

[Qemu-devel] [PULL 11/14] block: Add *tighten_restrictions to *check*_perm()

[Kevin Wolf]

From: Max Reitz <mreitz@redhat.com>

This patch makes three functions report whether the necessary permission
change tightens restrictions or not.  These functions are:
- bdrv_check_perm()
- bdrv_check_update_perm()
- bdrv_child_check_perm()

Callers can use this result to decide whether a failure is fatal or not
(see the next patch).

Signed-off-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 70 insertions(+), 17 deletions(-)

diff --git a/block.c b/block.c
index b7d4149c2f..1b10a5ce35 100644
--- a/block.c
+++ b/block.c
@@ -1706,7 +1706,8 @@ static int bdrv_fill_options(QDict **options, const char *filename,
 
 static int bdrv_child_check_perm(BdrvChild *c, BlockReopenQueue *q,
                                  uint64_t perm, uint64_t shared,
-                                 GSList *ignore_children, Error **errp);
+                                 GSList *ignore_children,
+                                 bool *tighten_restrictions, Error **errp);
 static void bdrv_child_abort_perm_update(BdrvChild *c);
 static void bdrv_child_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared);
 static void bdrv_get_cumulative_perm(BlockDriverState *bs, uint64_t *perm,
@@ -1781,18 +1782,43 @@ static void bdrv_child_perm(BlockDriverState *bs, BlockDriverState *child_bs,
  * permissions of all its parents. This involves checking whether all necessary
  * permission changes to child nodes can be performed.
  *
+ * Will set *tighten_restrictions to true if and only if new permissions have to
+ * be taken or currently shared permissions are to be unshared.  Otherwise,
+ * errors are not fatal as long as the caller accepts that the restrictions
+ * remain tighter than they need to be.  The caller still has to abort the
+ * transaction.
+ * @tighten_restrictions cannot be used together with @q: When reopening, we may
+ * encounter fatal errors even though no restrictions are to be tightened.  For
+ * example, changing a node from RW to RO will fail if the WRITE permission is
+ * to be kept.
+ *
  * A call to this function must always be followed by a call to bdrv_set_perm()
  * or bdrv_abort_perm_update().
  */
 static int bdrv_check_perm(BlockDriverState *bs, BlockReopenQueue *q,
                            uint64_t cumulative_perms,
                            uint64_t cumulative_shared_perms,
-                           GSList *ignore_children, Error **errp)
+                           GSList *ignore_children,
+                           bool *tighten_restrictions, Error **errp)
 {
     BlockDriver *drv = bs->drv;
     BdrvChild *c;
     int ret;
 
+    assert(!q || !tighten_restrictions);
+
+    if (tighten_restrictions) {
+        uint64_t current_perms, current_shared;
+        uint64_t added_perms, removed_shared_perms;
+
+        bdrv_get_cumulative_perm(bs, &current_perms, &current_shared);
+
+        added_perms = cumulative_perms & ~current_perms;
+        removed_shared_perms = current_shared & ~cumulative_shared_perms;
+
+        *tighten_restrictions = added_perms || removed_shared_perms;
+    }
+
     /* Write permissions never work with read-only images */
     if ((cumulative_perms & (BLK_PERM_WRITE | BLK_PERM_WRITE_UNCHANGED)) &&
         !bdrv_is_writable_after_reopen(bs, q))
@@ -1833,11 +1859,18 @@ static int bdrv_check_perm(BlockDriverState *bs, BlockReopenQueue *q,
     /* Check all children */
     QLIST_FOREACH(c, &bs->children, next) {
         uint64_t cur_perm, cur_shared;
+        bool child_tighten_restr;
+
         bdrv_child_perm(bs, c->bs, c, c->role, q,
                         cumulative_perms, cumulative_shared_perms,
                         &cur_perm, &cur_shared);
-        ret = bdrv_child_check_perm(c, q, cur_perm, cur_shared,
-                                    ignore_children, errp);
+        ret = bdrv_child_check_perm(c, q, cur_perm, cur_shared, ignore_children,
+                                    tighten_restrictions ? &child_tighten_restr
+                                                         : NULL,
+                                    errp);
+        if (tighten_restrictions) {
+            *tighten_restrictions |= child_tighten_restr;
+        }
         if (ret < 0) {
             return ret;
         }
@@ -1961,17 +1994,23 @@ char *bdrv_perm_names(uint64_t perm)
  * set, the BdrvChild objects in this list are ignored in the calculations;
  * this allows checking permission updates for an existing reference.
  *
+ * See bdrv_check_perm() for the semantics of @tighten_restrictions.
+ *
  * Needs to be followed by a call to either bdrv_set_perm() or
  * bdrv_abort_perm_update(). */
 static int bdrv_check_update_perm(BlockDriverState *bs, BlockReopenQueue *q,
                                   uint64_t new_used_perm,
                                   uint64_t new_shared_perm,
-                                  GSList *ignore_children, Error **errp)
+                                  GSList *ignore_children,
+                                  bool *tighten_restrictions,
+                                  Error **errp)
 {
     BdrvChild *c;
     uint64_t cumulative_perms = new_used_perm;
     uint64_t cumulative_shared_perms = new_shared_perm;
 
+    assert(!q || !tighten_restrictions);
+
     /* There is no reason why anyone couldn't tolerate write_unchanged */
     assert(new_shared_perm & BLK_PERM_WRITE_UNCHANGED);
 
@@ -1983,6 +2022,11 @@ static int bdrv_check_update_perm(BlockDriverState *bs, BlockReopenQueue *q,
         if ((new_used_perm & c->shared_perm) != new_used_perm) {
             char *user = bdrv_child_user_desc(c);
             char *perm_names = bdrv_perm_names(new_used_perm & ~c->shared_perm);
+
+            if (tighten_restrictions) {
+                *tighten_restrictions = true;
+            }
+
             error_setg(errp, "Conflicts with use by %s as '%s', which does not "
                              "allow '%s' on %s",
                        user, c->name, perm_names, bdrv_get_node_name(c->bs));
@@ -1994,6 +2038,11 @@ static int bdrv_check_update_perm(BlockDriverState *bs, BlockReopenQueue *q,
         if ((c->perm & new_shared_perm) != c->perm) {
             char *user = bdrv_child_user_desc(c);
             char *perm_names = bdrv_perm_names(c->perm & ~new_shared_perm);
+
+            if (tighten_restrictions) {
+                *tighten_restrictions = true;
+            }
+
             error_setg(errp, "Conflicts with use by %s as '%s', which uses "
                              "'%s' on %s",
                        user, c->name, perm_names, bdrv_get_node_name(c->bs));
@@ -2007,19 +2056,21 @@ static int bdrv_check_update_perm(BlockDriverState *bs, BlockReopenQueue *q,
     }
 
     return bdrv_check_perm(bs, q, cumulative_perms, cumulative_shared_perms,
-                           ignore_children, errp);
+                           ignore_children, tighten_restrictions, errp);
 }
 
 /* Needs to be followed by a call to either bdrv_child_set_perm() or
  * bdrv_child_abort_perm_update(). */
 static int bdrv_child_check_perm(BdrvChild *c, BlockReopenQueue *q,
                                  uint64_t perm, uint64_t shared,
-                                 GSList *ignore_children, Error **errp)
+                                 GSList *ignore_children,
+                                 bool *tighten_restrictions, Error **errp)
 {
     int ret;
 
     ignore_children = g_slist_prepend(g_slist_copy(ignore_children), c);
-    ret = bdrv_check_update_perm(c->bs, q, perm, shared, ignore_children, errp);
+    ret = bdrv_check_update_perm(c->bs, q, perm, shared, ignore_children,
+                                 tighten_restrictions, errp);
     g_slist_free(ignore_children);
 
     if (ret < 0) {
@@ -2072,7 +2123,7 @@ int bdrv_child_try_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared,
 {
     int ret;
 
-    ret = bdrv_child_check_perm(c, NULL, perm, shared, NULL, errp);
+    ret = bdrv_child_check_perm(c, NULL, perm, shared, NULL, NULL, errp);
     if (ret < 0) {
         bdrv_child_abort_perm_update(c);
         return ret;
@@ -2258,7 +2309,8 @@ static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs)
          * because we're just taking a parent away, so we're loosening
          * restrictions. */
         bdrv_get_cumulative_perm(old_bs, &perm, &shared_perm);
-        bdrv_check_perm(old_bs, NULL, perm, shared_perm, NULL, &error_abort);
+        bdrv_check_perm(old_bs, NULL, perm, shared_perm, NULL,
+                        NULL, &error_abort);
         bdrv_set_perm(old_bs, perm, shared_perm);
 
         /* When the parent requiring a non-default AioContext is removed, the
@@ -2288,7 +2340,8 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
     Error *local_err = NULL;
     int ret;
 
-    ret = bdrv_check_update_perm(child_bs, NULL, perm, shared_perm, NULL, errp);
+    ret = bdrv_check_update_perm(child_bs, NULL, perm, shared_perm, NULL, NULL,
+                                 errp);
     if (ret < 0) {
         bdrv_abort_perm_update(child_bs);
         bdrv_unref(child_bs);
@@ -3369,7 +3422,7 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
     QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
         BDRVReopenState *state = &bs_entry->state;
         ret = bdrv_check_perm(state->bs, bs_queue, state->perm,
-                              state->shared_perm, NULL, errp);
+                              state->shared_perm, NULL, NULL, errp);
         if (ret < 0) {
             goto cleanup_perm;
         }
@@ -3381,7 +3434,7 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
                             state->perm, state->shared_perm,
                             &nperm, &nshared);
             ret = bdrv_check_update_perm(state->new_backing_bs, NULL,
-                                         nperm, nshared, NULL, errp);
+                                         nperm, nshared, NULL, NULL, errp);
             if (ret < 0) {
                 goto cleanup_perm;
             }
@@ -4097,7 +4150,7 @@ void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
 
     /* Check whether the required permissions can be granted on @to, ignoring
      * all BdrvChild in @list so that they can't block themselves. */
-    ret = bdrv_check_update_perm(to, NULL, perm, shared, list, errp);
+    ret = bdrv_check_update_perm(to, NULL, perm, shared, list, NULL, errp);
     if (ret < 0) {
         bdrv_abort_perm_update(to);
         goto out;
@@ -4444,7 +4497,7 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
         /* Check whether we are allowed to switch c from top to base */
         GSList *ignore_children = g_slist_prepend(NULL, c);
         ret = bdrv_check_update_perm(base, NULL, c->perm, c->shared_perm,
-                                     ignore_children, &local_err);
+                                     ignore_children, NULL, &local_err);
         g_slist_free(ignore_children);
         if (ret < 0) {
             error_report_err(local_err);
@@ -5219,7 +5272,7 @@ static void coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs,
      */
     bs->open_flags &= ~BDRV_O_INACTIVE;
     bdrv_get_cumulative_perm(bs, &perm, &shared_perm);
-    ret = bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, &local_err);
+    ret = bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, NULL, &local_err);
     if (ret < 0) {
         bs->open_flags |= BDRV_O_INACTIVE;
         error_propagate(errp, local_err);
@@ -5369,7 +5422,7 @@ static int bdrv_inactivate_recurse(BlockDriverState *bs)
 
     /* Update permissions, they may differ for inactive nodes */
     bdrv_get_cumulative_perm(bs, &perm, &shared_perm);
-    bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, &error_abort);
+    bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, NULL, &error_abort);
     bdrv_set_perm(bs, perm, shared_perm);
 
 
-- 
2.20.1

[Qemu-devel] [PULL 12/14] block: Ignore loosening perm restrictions failures

[Kevin Wolf]

From: Max Reitz <mreitz@redhat.com>

We generally assume that loosening permission restrictions can never
fail.  We have seen in the past that this assumption is wrong.  This has
led to crashes because we generally pass &error_abort when loosening
permissions.

However, a failure in such a case should actually be handled in quite
the opposite way: It is very much not fatal, so qemu may report it, but
still consider the operation successful.  The only realistic problem is
that qemu may then retain permissions and thus locks on images it
actually does not require.  But again, that is not fatal.

To implement this behavior, we make all functions that change
permissions and that pass &error_abort to the initiating function
(bdrv_check_perm() or bdrv_child_check_perm()) evaluate the
@loosen_restrictions value introduced in the previous patch.  If it is
true and an error did occur, we abort the permission update, discard the
error, and instead report success to the caller.

bdrv_child_try_set_perm() itself does not pass &error_abort, but it is
the only public function to change permissions.  As such, callers may
pass &error_abort to it, expecting dropping permission restrictions to
never fail.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 44 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 38 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index 1b10a5ce35..c139540f2b 100644
--- a/block.c
+++ b/block.c
@@ -2121,11 +2121,26 @@ static void bdrv_child_abort_perm_update(BdrvChild *c)
 int bdrv_child_try_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared,
                             Error **errp)
 {
+    Error *local_err = NULL;
     int ret;
+    bool tighten_restrictions;
 
-    ret = bdrv_child_check_perm(c, NULL, perm, shared, NULL, NULL, errp);
+    ret = bdrv_child_check_perm(c, NULL, perm, shared, NULL,
+                                &tighten_restrictions, &local_err);
     if (ret < 0) {
         bdrv_child_abort_perm_update(c);
+        if (tighten_restrictions) {
+            error_propagate(errp, local_err);
+        } else {
+            /*
+             * Our caller may intend to only loosen restrictions and
+             * does not expect this function to fail.  Errors are not
+             * fatal in such a case, so we can just hide them from our
+             * caller.
+             */
+            error_free(local_err);
+            ret = 0;
+        }
         return ret;
     }
 
@@ -2308,10 +2323,19 @@ static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs)
         /* Update permissions for old node. This is guaranteed to succeed
          * because we're just taking a parent away, so we're loosening
          * restrictions. */
+        bool tighten_restrictions;
+        int ret;
+
         bdrv_get_cumulative_perm(old_bs, &perm, &shared_perm);
-        bdrv_check_perm(old_bs, NULL, perm, shared_perm, NULL,
-                        NULL, &error_abort);
-        bdrv_set_perm(old_bs, perm, shared_perm);
+        ret = bdrv_check_perm(old_bs, NULL, perm, shared_perm, NULL,
+                              &tighten_restrictions, NULL);
+        assert(tighten_restrictions == false);
+        if (ret < 0) {
+            /* We only tried to loosen restrictions, so errors are not fatal */
+            bdrv_abort_perm_update(old_bs);
+        } else {
+            bdrv_set_perm(old_bs, perm, shared_perm);
+        }
 
         /* When the parent requiring a non-default AioContext is removed, the
          * node moves back to the main AioContext */
@@ -5386,6 +5410,7 @@ static bool bdrv_has_bds_parent(BlockDriverState *bs, bool only_active)
 static int bdrv_inactivate_recurse(BlockDriverState *bs)
 {
     BdrvChild *child, *parent;
+    bool tighten_restrictions;
     uint64_t perm, shared_perm;
     int ret;
 
@@ -5422,8 +5447,15 @@ static int bdrv_inactivate_recurse(BlockDriverState *bs)
 
     /* Update permissions, they may differ for inactive nodes */
     bdrv_get_cumulative_perm(bs, &perm, &shared_perm);
-    bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, NULL, &error_abort);
-    bdrv_set_perm(bs, perm, shared_perm);
+    ret = bdrv_check_perm(bs, NULL, perm, shared_perm, NULL,
+                          &tighten_restrictions, NULL);
+    assert(tighten_restrictions == false);
+    if (ret < 0) {
+        /* We only tried to loosen restrictions, so errors are not fatal */
+        bdrv_abort_perm_update(bs);
+    } else {
+        bdrv_set_perm(bs, perm, shared_perm);
+    }
 
 
     /* Recursively inactivate children */
-- 
2.20.1

[Qemu-devel] [PULL 13/14] iotests: Test failure to loosen restrictions

[Kevin Wolf]

From: Max Reitz <mreitz@redhat.com>

Signed-off-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/182     | 21 +++++++++++++++++++++
 tests/qemu-iotests/182.out |  6 ++++++
 2 files changed, 27 insertions(+)

diff --git a/tests/qemu-iotests/182 b/tests/qemu-iotests/182
index 3a90ebfbfd..7f494eb9bb 100755
--- a/tests/qemu-iotests/182
+++ b/tests/qemu-iotests/182
@@ -152,6 +152,27 @@ success_or_failure=y _send_qemu_cmd $QEMU_HANDLE \
 
 _cleanup_qemu
 
+echo
+echo '=== Testing failure to loosen restrictions ==='
+echo
+
+_launch_qemu -drive file=$TEST_IMG,if=none,file.locking=on
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{'execute': 'qmp_capabilities'}" \
+    'return'
+
+_cleanup_test_img
+
+# When quitting qemu, it will try to drop its locks on the test image.
+# Because that file no longer exists, it will be unable to do so.
+# However, that is not fatal, so it should just move on.
+_send_qemu_cmd $QEMU_HANDLE \
+    "{'execute': 'quit'}" \
+    'return'
+
+wait=1 _cleanup_qemu
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/182.out b/tests/qemu-iotests/182.out
index 33d41eea91..ffef23e32b 100644
--- a/tests/qemu-iotests/182.out
+++ b/tests/qemu-iotests/182.out
@@ -15,4 +15,10 @@ Formatting 'TEST_DIR/t.qcow2.overlay', fmt=qcow2 size=197120 backing_file=TEST_D
 {"return": {}}
 {"return": {}}
 {"return": {}}
+
+=== Testing failure to loosen restrictions ===
+
+{"return": {}}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
 *** done
-- 
2.20.1

[Qemu-devel] [PULL 14/14] block/null: Expose read-zeroes option in QAPI schema

[Kevin Wolf]

Commit cd219eb1e55 added the read-zeroes option for the null-co and
null-aio block driver, but forgot to add them to the QAPI schema.
Therefore, this option wasn't available in -blockdev and blockdev-add
until now.

Add the missing option in the schema to make it available there, too.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 qapi/block-core.json | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 61124431d8..0d43d4f37c 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2890,11 +2890,13 @@
 # @latency-ns: emulated latency (in nanoseconds) in processing
 #              requests. Default to zero which completes requests immediately.
 #              (Since 2.4)
+# @read-zeroes: if true, reads from the device produce zeroes; if false, the
+#               buffer is left unchanged. (default: false; since: 4.1)
 #
 # Since: 2.9
 ##
 { 'struct': 'BlockdevOptionsNull',
-  'data': { '*size': 'int', '*latency-ns': 'uint64' } }
+  'data': { '*size': 'int', '*latency-ns': 'uint64', '*read-zeroes': 'bool' } }
 
 ##
 # @BlockdevOptionsNVMe:
-- 
2.20.1

Re: [Qemu-devel] [PULL 00/14] Block layer patches

[Peter Maydell]

On Tue, 18 Jun 2019 at 16:59, Kevin Wolf <kwolf@redhat.com> wrote:
>
> The following changes since commit cdfaa2720f4a09e5254868bd1f6e33f3e9eae76f:
>
>   Merge remote-tracking branch 'remotes/armbru/tags/pull-monitor-2019-06-17-v2' into staging (2019-06-18 10:47:00 +0100)
>
> are available in the Git repository at:
>
>   git://repo.or.cz/qemu/kevin.git tags/for-upstream
>
> for you to fetch changes up to 128b05f7e00765d883164631b974a27af5b4b613:
>
>   block/null: Expose read-zeroes option in QAPI schema (2019-06-18 16:41:10 +0200)
>
> ----------------------------------------------------------------
> Block layer patches:
>
> - block: Remove bs->job
> - block: Ignore loosening perm restrictions failures
> - block/null: Expose read-zeroes option in QAPI schema
> - iotests: Hide timestamps for skipped tests
>


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/4.1
for any user-visible changes.

-- PMM