[PULL 00/16] Block layer patches

[PULL 00/16] Block layer patches

[Kevin Wolf]

The following changes since commit 67b6526cf042f22521feff5ea521a05d3dd2bf8f:

  Merge remote-tracking branch 'remotes/bonzini-gitlab/tags/for-upstream' into staging (2022-01-13 13:59:56 +0000)

are available in the Git repository at:

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

for you to fetch changes up to e5e748739562268ef4063ee77bf53ad7040b25c7:

  iotests/testrunner.py: refactor test_field_width (2022-01-14 12:03:16 +0100)

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

- qemu-storage-daemon: Add vhost-user-blk help
- block-backend: Fix use-after-free for BDS pointers after aio_poll()
- qemu-img: Fix sparseness of output image with unaligned ranges
- vvfat: Fix crashes in read-write mode
- Fix device deletion events with -device JSON syntax
- Code cleanups

----------------------------------------------------------------
Daniel P. Berrangé (1):
      softmmu: fix device deletion events with -device JSON syntax

Emanuele Giuseppe Esposito (3):
      block_int: make bdrv_backing_overridden static
      include/sysemu/blockdev.h: remove drive_mark_claimed_by_board and inline drive_def
      include/sysemu/blockdev.h: remove drive_get_max_devs

Hanna Reitz (2):
      iotests/stream-error-on-reset: New test
      iotests/308: Fix for CAP_DAC_OVERRIDE

Kevin Wolf (3):
      vvfat: Fix size of temporary qcow file
      vvfat: Fix vvfat_write() for writes before the root directory
      iotests: Test qemu-img convert of zeroed data cluster

Philippe Mathieu-Daudé (3):
      docs: Correct 'vhost-user-blk' spelling
      qemu-storage-daemon: Add vhost-user-blk help
      qapi/block: Restrict vhost-user-blk to CONFIG_VHOST_USER_BLK_SERVER

Stefan Hajnoczi (1):
      block-backend: prevent dangling BDS pointers across aio_poll()

Vladimir Sementsov-Ogievskiy (3):
      qemu-img: make is_allocated_sectors() more efficient
      block: drop BLK_PERM_GRAPH_MOD
      iotests/testrunner.py: refactor test_field_width

 qapi/block-core.json                               |   7 +-
 qapi/block-export.json                             |   6 +-
 qapi/qdev.json                                     |   5 +-
 docs/tools/qemu-storage-daemon.rst                 |   2 +-
 include/block/block.h                              |   9 +-
 include/block/block_int.h                          |   3 -
 include/sysemu/blockdev.h                          |   3 -
 block.c                                            |  11 +-
 block/block-backend.c                              |  19 ++-
 block/commit.c                                     |   1 -
 block/mirror.c                                     |  15 +--
 block/monitor/block-hmp-cmds.c                     |   2 +-
 block/vvfat.c                                      |  37 ++++--
 blockdev.c                                         |  24 +---
 hw/block/block.c                                   |   3 +-
 qemu-img.c                                         |  23 +++-
 softmmu/vl.c                                       |   8 +-
 storage-daemon/qemu-storage-daemon.c               |  13 ++
 tests/qtest/device-plug-test.c                     |  19 +++
 scripts/render_block_graph.py                      |   1 -
 tests/qemu-iotests/testrunner.py                   |  21 ++--
 tests/qemu-iotests/122                             |   1 +
 tests/qemu-iotests/122.out                         |   2 +
 tests/qemu-iotests/273.out                         |   4 -
 tests/qemu-iotests/308                             |  25 +++-
 tests/qemu-iotests/308.out                         |   2 +-
 tests/qemu-iotests/tests/stream-error-on-reset     | 140 +++++++++++++++++++++
 tests/qemu-iotests/tests/stream-error-on-reset.out |   5 +
 28 files changed, 307 insertions(+), 104 deletions(-)
 create mode 100755 tests/qemu-iotests/tests/stream-error-on-reset
 create mode 100644 tests/qemu-iotests/tests/stream-error-on-reset.out

[PULL 01/16] block_int: make bdrv_backing_overridden static

[Kevin Wolf]

From: Emanuele Giuseppe Esposito <eesposit@redhat.com>

bdrv_backing_overridden is only used in block.c, so there is
no need to leave it in block_int.h

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <20211215121140.456939-2-eesposit@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/block_int.h | 3 ---
 block.c                   | 4 +++-
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index f4c75e8ba9..27008cfb22 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1122,9 +1122,6 @@ BlockDriver *bdrv_probe_all(const uint8_t *buf, int buf_size,
 void bdrv_parse_filename_strip_prefix(const char *filename, const char *prefix,
                                       QDict *options);
 
-bool bdrv_backing_overridden(BlockDriverState *bs);
-
-
 /**
  * bdrv_add_aio_context_notifier:
  *
diff --git a/block.c b/block.c
index 0ac5b163d2..10346b5011 100644
--- a/block.c
+++ b/block.c
@@ -103,6 +103,8 @@ static int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
 static void bdrv_reopen_commit(BDRVReopenState *reopen_state);
 static void bdrv_reopen_abort(BDRVReopenState *reopen_state);
 
+static bool bdrv_backing_overridden(BlockDriverState *bs);
+
 /* If non-zero, use only whitelisted block drivers */
 static int use_bdrv_whitelist;
 
@@ -7475,7 +7477,7 @@ static bool append_strong_runtime_options(QDict *d, BlockDriverState *bs)
 /* Note: This function may return false positives; it may return true
  * even if opening the backing file specified by bs's image header
  * would result in exactly bs->backing. */
-bool bdrv_backing_overridden(BlockDriverState *bs)
+static bool bdrv_backing_overridden(BlockDriverState *bs)
 {
     if (bs->backing) {
         return strcmp(bs->auto_backing_file,
-- 
2.31.1

[PULL 02/16] include/sysemu/blockdev.h: remove drive_mark_claimed_by_board and inline drive_def

[Kevin Wolf]

From: Emanuele Giuseppe Esposito <eesposit@redhat.com>

drive_def is only a particular use case of
qemu_opts_parse_noisily, so it can be inlined.

Also remove drive_mark_claimed_by_board, as it is only defined
but not implemented (nor used) anywhere.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Message-Id: <20211215121140.456939-3-eesposit@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/sysemu/blockdev.h      | 2 --
 block/monitor/block-hmp-cmds.c | 2 +-
 blockdev.c                     | 7 +------
 softmmu/vl.c                   | 4 +++-
 4 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
index a750f99b79..ea35c42f5c 100644
--- a/include/sysemu/blockdev.h
+++ b/include/sysemu/blockdev.h
@@ -45,13 +45,11 @@ BlockBackend *blk_by_legacy_dinfo(DriveInfo *dinfo);
 void override_max_devs(BlockInterfaceType type, int max_devs);
 
 DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit);
-void drive_mark_claimed_by_board(void);
 void drive_check_orphaned(void);
 DriveInfo *drive_get_by_index(BlockInterfaceType type, int index);
 int drive_get_max_bus(BlockInterfaceType type);
 int drive_get_max_devs(BlockInterfaceType type);
 
-QemuOpts *drive_def(const char *optstr);
 QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file,
                     const char *optstr);
 DriveInfo *drive_new(QemuOpts *arg, BlockInterfaceType block_default_type,
diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index 2ac4aedfff..bfb3c043a0 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -101,7 +101,7 @@ void hmp_drive_add(Monitor *mon, const QDict *qdict)
         return;
     }
 
-    opts = drive_def(optstr);
+    opts = qemu_opts_parse_noisily(qemu_find_opts("drive"), optstr, false);
     if (!opts)
         return;
 
diff --git a/blockdev.c b/blockdev.c
index b5ff9b854e..25b3b202e7 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -197,17 +197,12 @@ static int drive_index_to_unit_id(BlockInterfaceType type, int index)
     return max_devs ? index % max_devs : index;
 }
 
-QemuOpts *drive_def(const char *optstr)
-{
-    return qemu_opts_parse_noisily(qemu_find_opts("drive"), optstr, false);
-}
-
 QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file,
                     const char *optstr)
 {
     QemuOpts *opts;
 
-    opts = drive_def(optstr);
+    opts = qemu_opts_parse_noisily(qemu_find_opts("drive"), optstr, false);
     if (!opts) {
         return NULL;
     }
diff --git a/softmmu/vl.c b/softmmu/vl.c
index a8cad43691..207a9eb8be 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2887,7 +2887,9 @@ void qemu_init(int argc, char **argv, char **envp)
                     break;
                 }
             case QEMU_OPTION_drive:
-                if (drive_def(optarg) == NULL) {
+                opts = qemu_opts_parse_noisily(qemu_find_opts("drive"),
+                                               optarg, false);
+                if (opts == NULL) {
                     exit(1);
                 }
                 break;
-- 
2.31.1

[PULL 03/16] include/sysemu/blockdev.h: remove drive_get_max_devs

[Kevin Wolf]

From: Emanuele Giuseppe Esposito <eesposit@redhat.com>

Remove drive_get_max_devs, as it is not used by anyone.

Last use was removed in commit 8f2d75e81d5
("hw: Drop superfluous special checks for orphaned -drive").

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <20211215121140.456939-4-eesposit@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/sysemu/blockdev.h |  1 -
 blockdev.c                | 17 -----------------
 2 files changed, 18 deletions(-)

diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
index ea35c42f5c..f9fb54d437 100644
--- a/include/sysemu/blockdev.h
+++ b/include/sysemu/blockdev.h
@@ -48,7 +48,6 @@ DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit);
 void drive_check_orphaned(void);
 DriveInfo *drive_get_by_index(BlockInterfaceType type, int index);
 int drive_get_max_bus(BlockInterfaceType type);
-int drive_get_max_devs(BlockInterfaceType type);
 
 QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file,
                     const char *optstr);
diff --git a/blockdev.c b/blockdev.c
index 25b3b202e7..8197165bb5 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -168,23 +168,6 @@ void blockdev_auto_del(BlockBackend *blk)
     }
 }
 
-/**
- * Returns the current mapping of how many units per bus
- * a particular interface can support.
- *
- *  A positive integer indicates n units per bus.
- *  0 implies the mapping has not been established.
- * -1 indicates an invalid BlockInterfaceType was given.
- */
-int drive_get_max_devs(BlockInterfaceType type)
-{
-    if (type >= IF_IDE && type < IF_COUNT) {
-        return if_max_devs[type];
-    }
-
-    return -1;
-}
-
 static int drive_index_to_bus_id(BlockInterfaceType type, int index)
 {
     int max_devs = if_max_devs[type];
-- 
2.31.1

[PULL 04/16] softmmu: fix device deletion events with -device JSON syntax

[Kevin Wolf]

From: Daniel P. Berrangé <berrange@redhat.com>

The -device JSON syntax impl leaks a reference on the created
DeviceState instance. As a result when you hot-unplug the
device, the device_finalize method won't be called and thus
it will fail to emit the required DEVICE_DELETED event.

A 'json-cli' feature was previously added against the
'device_add' QMP command QAPI schema to indicated to mgmt
apps that -device supported JSON syntax. Given the hotplug
bug that feature flag is not usable for its purpose, so
we add a new 'json-cli-hotplug' feature to indicate the
-device supports JSON without breaking hotplug.

Fixes: 5dacda5167560b3af8eadbce5814f60ba44b467e
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/802
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Message-Id: <20220105123847.4047954-2-berrange@redhat.com>
Reviewed-by: Laurent Vivier <lvivier@redhat.com>
Tested-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/qdev.json                 |  5 ++++-
 softmmu/vl.c                   |  4 +++-
 tests/qtest/device-plug-test.c | 19 +++++++++++++++++++
 3 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/qapi/qdev.json b/qapi/qdev.json
index 69656b14df..26cd10106b 100644
--- a/qapi/qdev.json
+++ b/qapi/qdev.json
@@ -44,6 +44,9 @@
 # @json-cli: If present, the "-device" command line option supports JSON
 #            syntax with a structure identical to the arguments of this
 #            command.
+# @json-cli-hotplug: If present, the "-device" command line option supports JSON
+#                    syntax without the reference counting leak that broke
+#                    hot-unplug
 #
 # Notes:
 #
@@ -74,7 +77,7 @@
 { 'command': 'device_add',
   'data': {'driver': 'str', '*bus': 'str', '*id': 'str'},
   'gen': false, # so we can get the additional arguments
-  'features': ['json-cli'] }
+  'features': ['json-cli', 'json-cli-hotplug'] }
 
 ##
 # @device_del:
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 207a9eb8be..5e1b35ba48 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2684,6 +2684,7 @@ static void qemu_create_cli_devices(void)
     qemu_opts_foreach(qemu_find_opts("device"),
                       device_init_func, NULL, &error_fatal);
     QTAILQ_FOREACH(opt, &device_opts, next) {
+        DeviceState *dev;
         loc_push_restore(&opt->loc);
         /*
          * TODO Eventually we should call qmp_device_add() here to make sure it
@@ -2692,7 +2693,8 @@ static void qemu_create_cli_devices(void)
          * from the start, so call qdev_device_add_from_qdict() directly for
          * now.
          */
-        qdev_device_add_from_qdict(opt->opts, true, &error_fatal);
+        dev = qdev_device_add_from_qdict(opt->opts, true, &error_fatal);
+        object_unref(OBJECT(dev));
         loc_pop(&opt->loc);
     }
     rom_reset_order_override();
diff --git a/tests/qtest/device-plug-test.c b/tests/qtest/device-plug-test.c
index 559d47727a..ad79bd4c14 100644
--- a/tests/qtest/device-plug-test.c
+++ b/tests/qtest/device-plug-test.c
@@ -77,6 +77,23 @@ static void test_pci_unplug_request(void)
     qtest_quit(qtest);
 }
 
+static void test_pci_unplug_json_request(void)
+{
+    QTestState *qtest = qtest_initf(
+        "-device '{\"driver\": \"virtio-mouse-pci\", \"id\": \"dev0\"}'");
+
+    /*
+     * Request device removal. As the guest is not running, the request won't
+     * be processed. However during system reset, the removal will be
+     * handled, removing the device.
+     */
+    device_del(qtest, "dev0");
+    system_reset(qtest);
+    wait_device_deleted_event(qtest, "dev0");
+
+    qtest_quit(qtest);
+}
+
 static void test_ccw_unplug(void)
 {
     QTestState *qtest = qtest_initf("-device virtio-balloon-ccw,id=dev0");
@@ -145,6 +162,8 @@ int main(int argc, char **argv)
      */
     qtest_add_func("/device-plug/pci-unplug-request",
                    test_pci_unplug_request);
+    qtest_add_func("/device-plug/pci-unplug-json-request",
+                   test_pci_unplug_json_request);
 
     if (!strcmp(arch, "s390x")) {
         qtest_add_func("/device-plug/ccw-unplug",
-- 
2.31.1

[PULL 05/16] docs: Correct 'vhost-user-blk' spelling

[Kevin Wolf]

From: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reported-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Message-Id: <20220107105420.395011-2-f4bug@amsat.org>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 docs/tools/qemu-storage-daemon.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/tools/qemu-storage-daemon.rst b/docs/tools/qemu-storage-daemon.rst
index 3e5a9dc032..9b0eaba6e5 100644
--- a/docs/tools/qemu-storage-daemon.rst
+++ b/docs/tools/qemu-storage-daemon.rst
@@ -201,7 +201,7 @@ Export raw image file ``disk.img`` over NBD UNIX domain socket ``nbd.sock``::
       --nbd-server addr.type=unix,addr.path=nbd.sock \
       --export type=nbd,id=export,node-name=disk,writable=on
 
-Export a qcow2 image file ``disk.qcow2`` as a vhosts-user-blk device over UNIX
+Export a qcow2 image file ``disk.qcow2`` as a vhost-user-blk device over UNIX
 domain socket ``vhost-user-blk.sock``::
 
   $ qemu-storage-daemon \
-- 
2.31.1

[PULL 06/16] qemu-storage-daemon: Add vhost-user-blk help

[Kevin Wolf]

From: Philippe Mathieu-Daudé <philmd@redhat.com>

Add missing vhost-user-blk help:

  $ qemu-storage-daemon -h
  ...
    --export [type=]vhost-user-blk,id=<id>,node-name=<node-name>,
             addr.type=unix,addr.path=<socket-path>[,writable=on|off]
             [,logical-block-size=<block-size>][,num-queues=<num-queues>]
                           export the specified block node as a
                           vhosts-user-blk device over UNIX domain socket
    --export [type=]vhost-user-blk,id=<id>,node-name=<node-name>,
             fd,addr.str=<fd>[,writable=on|off]
             [,logical-block-size=<block-size>][,num-queues=<num-queues>]
                           export the specified block node as a
                           vhosts-user-blk device over file descriptor
  ...

Fixes: 90fc91d50b7 ("convert vhost-user-blk server to block export API")
Reported-by: Qing Wang <qinwang@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Message-Id: <20220107105420.395011-3-f4bug@amsat.org>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 storage-daemon/qemu-storage-daemon.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/storage-daemon/qemu-storage-daemon.c b/storage-daemon/qemu-storage-daemon.c
index 52cf17e8ac..9d76d1114d 100644
--- a/storage-daemon/qemu-storage-daemon.c
+++ b/storage-daemon/qemu-storage-daemon.c
@@ -104,6 +104,19 @@ static void help(void)
 "                         export the specified block node over FUSE\n"
 "\n"
 #endif /* CONFIG_FUSE */
+#ifdef CONFIG_VHOST_USER_BLK_SERVER
+"  --export [type=]vhost-user-blk,id=<id>,node-name=<node-name>,\n"
+"           addr.type=unix,addr.path=<socket-path>[,writable=on|off]\n"
+"           [,logical-block-size=<block-size>][,num-queues=<num-queues>]\n"
+"                         export the specified block node as a\n"
+"                         vhost-user-blk device over UNIX domain socket\n"
+"  --export [type=]vhost-user-blk,id=<id>,node-name=<node-name>,\n"
+"           fd,addr.str=<fd>[,writable=on|off]\n"
+"           [,logical-block-size=<block-size>][,num-queues=<num-queues>]\n"
+"                         export the specified block node as a\n"
+"                         vhost-user-blk device over file descriptor\n"
+"\n"
+#endif /* CONFIG_VHOST_USER_BLK_SERVER */
 "  --monitor [chardev=]name[,mode=control][,pretty[=on|off]]\n"
 "                         configure a QMP monitor\n"
 "\n"
-- 
2.31.1

[PULL 07/16] qapi/block: Restrict vhost-user-blk to CONFIG_VHOST_USER_BLK_SERVER

[Kevin Wolf]

From: Philippe Mathieu-Daudé <philmd@redhat.com>

When building QEMU with --disable-vhost-user and using introspection,
query-qmp-schema lists vhost-user-blk even though it's not actually
available:

  { "execute": "query-qmp-schema" }
  {
      "return": [
          ...
          {
              "name": "312",
              "members": [
                  {
                      "name": "nbd"
                  },
                  {
                      "name": "vhost-user-blk"
                  }
              ],
              "meta-type": "enum",
              "values": [
                  "nbd",
                  "vhost-user-blk"
              ]
          },

Restrict vhost-user-blk in BlockExportType when
CONFIG_VHOST_USER_BLK_SERVER is disabled, so it
doesn't end listed by query-qmp-schema.

Fixes: 90fc91d50b7 ("convert vhost-user-blk server to block export API")
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Message-Id: <20220107105420.395011-4-f4bug@amsat.org>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/block-export.json | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/qapi/block-export.json b/qapi/block-export.json
index c1b92ce1c1..f9ce79a974 100644
--- a/qapi/block-export.json
+++ b/qapi/block-export.json
@@ -277,7 +277,8 @@
 # Since: 4.2
 ##
 { 'enum': 'BlockExportType',
-  'data': [ 'nbd', 'vhost-user-blk',
+  'data': [ 'nbd',
+            { 'name': 'vhost-user-blk', 'if': 'CONFIG_VHOST_USER_BLK_SERVER' },
             { 'name': 'fuse', 'if': 'CONFIG_FUSE' } ] }
 
 ##
@@ -319,7 +320,8 @@
   'discriminator': 'type',
   'data': {
       'nbd': 'BlockExportOptionsNbd',
-      'vhost-user-blk': 'BlockExportOptionsVhostUserBlk',
+      'vhost-user-blk': { 'type': 'BlockExportOptionsVhostUserBlk',
+                          'if': 'CONFIG_VHOST_USER_BLK_SERVER' },
       'fuse': { 'type': 'BlockExportOptionsFuse',
                 'if': 'CONFIG_FUSE' }
    } }
-- 
2.31.1

[PULL 08/16] block-backend: prevent dangling BDS pointers across aio_poll()

[Kevin Wolf]

From: Stefan Hajnoczi <stefanha@redhat.com>

The BlockBackend root child can change when aio_poll() is invoked. This
happens when a temporary filter node is removed upon blockjob
completion, for example.

Functions in block/block-backend.c must be aware of this when using a
blk_bs() pointer across aio_poll() because the BlockDriverState refcnt
may reach 0, resulting in a stale pointer.

One example is scsi_device_purge_requests(), which calls blk_drain() to
wait for in-flight requests to cancel. If the backup blockjob is active,
then the BlockBackend root child is a temporary filter BDS owned by the
blockjob. The blockjob can complete during bdrv_drained_begin() and the
last reference to the BDS is released when the temporary filter node is
removed. This results in a use-after-free when blk_drain() calls
bdrv_drained_end(bs) on the dangling pointer.

Explicitly hold a reference to bs across block APIs that invoke
aio_poll().

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2021778
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2036178
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <20220111153613.25453-2-stefanha@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/block-backend.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 12ef80ea17..23e727199b 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -822,16 +822,22 @@ BlockBackend *blk_by_public(BlockBackendPublic *public)
 void blk_remove_bs(BlockBackend *blk)
 {
     ThrottleGroupMember *tgm = &blk->public.throttle_group_member;
-    BlockDriverState *bs;
     BdrvChild *root;
 
     notifier_list_notify(&blk->remove_bs_notifiers, blk);
     if (tgm->throttle_state) {
-        bs = blk_bs(blk);
+        BlockDriverState *bs = blk_bs(blk);
+
+        /*
+         * Take a ref in case blk_bs() changes across bdrv_drained_begin(), for
+         * example, if a temporary filter node is removed by a blockjob.
+         */
+        bdrv_ref(bs);
         bdrv_drained_begin(bs);
         throttle_group_detach_aio_context(tgm);
         throttle_group_attach_aio_context(tgm, qemu_get_aio_context());
         bdrv_drained_end(bs);
+        bdrv_unref(bs);
     }
 
     blk_update_root_state(blk);
@@ -1705,6 +1711,7 @@ void blk_drain(BlockBackend *blk)
     BlockDriverState *bs = blk_bs(blk);
 
     if (bs) {
+        bdrv_ref(bs);
         bdrv_drained_begin(bs);
     }
 
@@ -1714,6 +1721,7 @@ void blk_drain(BlockBackend *blk)
 
     if (bs) {
         bdrv_drained_end(bs);
+        bdrv_unref(bs);
     }
 }
 
@@ -2044,10 +2052,13 @@ static int blk_do_set_aio_context(BlockBackend *blk, AioContext *new_context,
     int ret;
 
     if (bs) {
+        bdrv_ref(bs);
+
         if (update_root_node) {
             ret = bdrv_child_try_set_aio_context(bs, new_context, blk->root,
                                                  errp);
             if (ret < 0) {
+                bdrv_unref(bs);
                 return ret;
             }
         }
@@ -2057,6 +2068,8 @@ static int blk_do_set_aio_context(BlockBackend *blk, AioContext *new_context,
             throttle_group_attach_aio_context(tgm, new_context);
             bdrv_drained_end(bs);
         }
+
+        bdrv_unref(bs);
     }
 
     blk->ctx = new_context;
@@ -2326,11 +2339,13 @@ void blk_io_limits_disable(BlockBackend *blk)
     ThrottleGroupMember *tgm = &blk->public.throttle_group_member;
     assert(tgm->throttle_state);
     if (bs) {
+        bdrv_ref(bs);
         bdrv_drained_begin(bs);
     }
     throttle_group_unregister_tgm(tgm);
     if (bs) {
         bdrv_drained_end(bs);
+        bdrv_unref(bs);
     }
 }
 
-- 
2.31.1

[PULL 09/16] iotests/stream-error-on-reset: New test

[Kevin Wolf]

From: Hanna Reitz <hreitz@redhat.com>

Test the following scenario:
- Simple stream block in two-layer backing chain (base and top)
- The job is drained via blk_drain(), then an error occurs while the job
  settles the ongoing request
- And so the job completes while in blk_drain()

This was reported as a segfault, but is fixed by "block-backend: prevent
dangling BDS pointers across aio_poll()".

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2036178
Signed-off-by: Hanna Reitz <hreitz@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <20220111153613.25453-3-stefanha@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 .../qemu-iotests/tests/stream-error-on-reset  | 140 ++++++++++++++++++
 .../tests/stream-error-on-reset.out           |   5 +
 2 files changed, 145 insertions(+)
 create mode 100755 tests/qemu-iotests/tests/stream-error-on-reset
 create mode 100644 tests/qemu-iotests/tests/stream-error-on-reset.out

diff --git a/tests/qemu-iotests/tests/stream-error-on-reset b/tests/qemu-iotests/tests/stream-error-on-reset
new file mode 100755
index 0000000000..7eaedb24d7
--- /dev/null
+++ b/tests/qemu-iotests/tests/stream-error-on-reset
@@ -0,0 +1,140 @@
+#!/usr/bin/env python3
+# group: rw quick
+#
+# Test what happens when a stream job completes in a blk_drain().
+#
+# Copyright (C) 2022 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+import os
+import iotests
+from iotests import imgfmt, qemu_img_create, qemu_io_silent, QMPTestCase
+
+
+image_size = 1 * 1024 * 1024
+data_size = 64 * 1024
+base = os.path.join(iotests.test_dir, 'base.img')
+top = os.path.join(iotests.test_dir, 'top.img')
+
+
+# We want to test completing a stream job in a blk_drain().
+#
+# The blk_drain() we are going to use is a virtio-scsi device resetting,
+# which we can trigger by resetting the system.
+#
+# In order to have the block job complete on drain, we (1) throttle its
+# base image so we can start the drain after it has begun, but before it
+# completes, and (2) make it encounter an I/O error on the ensuing write.
+# (If it completes regularly, the completion happens after the drain for
+# some reason.)
+
+class TestStreamErrorOnReset(QMPTestCase):
+    def setUp(self) -> None:
+        """
+        Create two images:
+        - base image {base} with {data_size} bytes allocated
+        - top image {top} without any data allocated
+
+        And the following VM configuration:
+        - base image throttled to {data_size}
+        - top image with a blkdebug configuration so the first write access
+          to it will result in an error
+        - top image is attached to a virtio-scsi device
+        """
+        assert qemu_img_create('-f', imgfmt, base, str(image_size)) == 0
+        assert qemu_io_silent('-c', f'write 0 {data_size}', base) == 0
+        assert qemu_img_create('-f', imgfmt, top, str(image_size)) == 0
+
+        self.vm = iotests.VM()
+        self.vm.add_args('-accel', 'tcg') # Make throttling work properly
+        self.vm.add_object(self.vm.qmp_to_opts({
+            'qom-type': 'throttle-group',
+            'id': 'thrgr',
+            'x-bps-total': str(data_size)
+        }))
+        self.vm.add_blockdev(self.vm.qmp_to_opts({
+            'driver': imgfmt,
+            'node-name': 'base',
+            'file': {
+                'driver': 'throttle',
+                'throttle-group': 'thrgr',
+                'file': {
+                    'driver': 'file',
+                    'filename': base
+                }
+            }
+        }))
+        self.vm.add_blockdev(self.vm.qmp_to_opts({
+            'driver': imgfmt,
+            'node-name': 'top',
+            'file': {
+                'driver': 'blkdebug',
+                'node-name': 'top-blkdebug',
+                'inject-error': [{
+                    'event': 'pwritev',
+                    'immediately': 'true',
+                    'once': 'true'
+                }],
+                'image': {
+                    'driver': 'file',
+                    'filename': top
+                }
+            },
+            'backing': 'base'
+        }))
+        self.vm.add_device(self.vm.qmp_to_opts({
+            'driver': 'virtio-scsi',
+            'id': 'vscsi'
+        }))
+        self.vm.add_device(self.vm.qmp_to_opts({
+            'driver': 'scsi-hd',
+            'bus': 'vscsi.0',
+            'drive': 'top'
+        }))
+        self.vm.launch()
+
+    def tearDown(self) -> None:
+        self.vm.shutdown()
+        os.remove(top)
+        os.remove(base)
+
+    def test_stream_error_on_reset(self) -> None:
+        # Launch a stream job, which will take at least a second to
+        # complete, because the base image is throttled (so we can
+        # get in between it having started and it having completed)
+        res = self.vm.qmp('block-stream', job_id='stream', device='top')
+        self.assert_qmp(res, 'return', {})
+
+        while True:
+            ev = self.vm.event_wait('JOB_STATUS_CHANGE')
+            if ev['data']['status'] == 'running':
+                # Once the stream job is running, reset the system, which
+                # forces the virtio-scsi device to be reset, thus draining
+                # the stream job, and making it complete.  Completing
+                # inside of that drain should not result in a segfault.
+                res = self.vm.qmp('system_reset')
+                self.assert_qmp(res, 'return', {})
+            elif ev['data']['status'] == 'null':
+                # The test is done once the job is gone
+                break
+
+
+if __name__ == '__main__':
+    # Passes with any format with backing file support, but qed and
+    # qcow1 do not seem to exercise the used-to-be problematic code
+    # path, so there is no point in having them in this list
+    iotests.main(supported_fmts=['qcow2', 'vmdk'],
+                 supported_protocols=['file'])
diff --git a/tests/qemu-iotests/tests/stream-error-on-reset.out b/tests/qemu-iotests/tests/stream-error-on-reset.out
new file mode 100644
index 0000000000..ae1213e6f8
--- /dev/null
+++ b/tests/qemu-iotests/tests/stream-error-on-reset.out
@@ -0,0 +1,5 @@
+.
+----------------------------------------------------------------------
+Ran 1 tests
+
+OK
-- 
2.31.1

[PULL 10/16] iotests/308: Fix for CAP_DAC_OVERRIDE

[Kevin Wolf]

From: Hanna Reitz <hreitz@redhat.com>

With CAP_DAC_OVERRIDE (which e.g. root generally has), permission checks
will be bypassed when opening files.

308 in one instance tries to open a read-only file (FUSE export) with
qemu-io as read/write, and expects this to fail.  However, when running
it as root, opening will succeed (thanks to CAP_DAC_OVERRIDE) and only
the actual write operation will fail.

Note this as "Case not run", but have the test pass in either case.

Reported-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Fixes: 2c7dd057aa7bd7a875e9b1a53975c220d6380bc4
       ("export/fuse: Pass default_permissions for mount")
Signed-off-by: Hanna Reitz <hreitz@redhat.com>
Message-Id: <20220103120014.13061-1-hreitz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/308     | 25 +++++++++++++++++++++++--
 tests/qemu-iotests/308.out |  2 +-
 2 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/308 b/tests/qemu-iotests/308
index 2e3f8f4282..bde4aac2fa 100755
--- a/tests/qemu-iotests/308
+++ b/tests/qemu-iotests/308
@@ -230,8 +230,29 @@ echo '=== Writable export ==='
 fuse_export_add 'export-mp' "'mountpoint': '$EXT_MP', 'writable': true"
 
 # Check that writing to the read-only export fails
-$QEMU_IO -f raw -c 'write -P 42 1M 64k' "$TEST_IMG" 2>&1 \
-    | _filter_qemu_io | _filter_testdir | _filter_imgfmt
+output=$($QEMU_IO -f raw -c 'write -P 42 1M 64k' "$TEST_IMG" 2>&1 \
+             | _filter_qemu_io | _filter_testdir | _filter_imgfmt)
+
+# Expected reference output: Opening the file fails because it has no
+# write permission
+reference="Could not open 'TEST_DIR/t.IMGFMT': Permission denied"
+
+if echo "$output" | grep -q "$reference"; then
+    echo "Writing to read-only export failed: OK"
+elif echo "$output" | grep -q "write failed: Permission denied"; then
+    # With CAP_DAC_OVERRIDE (e.g. when running this test as root), the export
+    # can be opened regardless of its file permissions, but writing will then
+    # fail.  This is not the result for which we want to test, so count this as
+    # a SKIP.
+    _casenotrun "Opening RO export as R/W succeeded, perhaps because of" \
+        "CAP_DAC_OVERRIDE"
+
+    # Still, write this to the reference output to make the test pass
+    echo "Writing to read-only export failed: OK"
+else
+    echo "Writing to read-only export failed: ERROR"
+    echo "$output"
+fi
 
 # But here it should work
 $QEMU_IO -f raw -c 'write -P 42 1M 64k' "$EXT_MP" | _filter_qemu_io
diff --git a/tests/qemu-iotests/308.out b/tests/qemu-iotests/308.out
index fc47bb11a2..e4467a10cf 100644
--- a/tests/qemu-iotests/308.out
+++ b/tests/qemu-iotests/308.out
@@ -95,7 +95,7 @@ virtual size: 0 B (0 bytes)
               'mountpoint': 'TEST_DIR/t.IMGFMT.fuse', 'writable': true
           } }
 {"return": {}}
-qemu-io: can't open device TEST_DIR/t.IMGFMT: Could not open 'TEST_DIR/t.IMGFMT': Permission denied
+Writing to read-only export failed: OK
 wrote 65536/65536 bytes at offset 1048576
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 wrote 65536/65536 bytes at offset 1048576
-- 
2.31.1

[PULL 11/16] vvfat: Fix size of temporary qcow file

[Kevin Wolf]

The size of the qcow size was calculated so that only the FAT partition
would fit on it, but not the whole disk. However, offsets relative to
the whole disk are used to access it, so increase its size to be large
enough for that.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-Id: <20211209151815.23495-1-kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/vvfat.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index 5dacc6cfac..36e73d4c64 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1230,6 +1230,7 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
                  dirname, cyls, heads, secs));
 
     s->sector_count = cyls * heads * secs - s->offset_to_bootsector;
+    bs->total_sectors = cyls * heads * secs;
 
     if (qemu_opt_get_bool(opts, "rw", false)) {
         if (!bdrv_is_read_only(bs)) {
@@ -1250,8 +1251,6 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
         }
     }
 
-    bs->total_sectors = cyls * heads * secs;
-
     if (init_directories(s, dirname, heads, secs, errp)) {
         ret = -EIO;
         goto fail;
@@ -3147,8 +3146,8 @@ static int enable_write_target(BlockDriverState *bs, Error **errp)
     }
 
     opts = qemu_opts_create(bdrv_qcow->create_opts, NULL, 0, &error_abort);
-    qemu_opt_set_number(opts, BLOCK_OPT_SIZE, s->sector_count * 512,
-                        &error_abort);
+    qemu_opt_set_number(opts, BLOCK_OPT_SIZE,
+                        bs->total_sectors * BDRV_SECTOR_SIZE, &error_abort);
     qemu_opt_set(opts, BLOCK_OPT_BACKING_FILE, "fat:", &error_abort);
 
     ret = bdrv_create(bdrv_qcow, s->qcow_filename, opts, errp);
-- 
2.31.1

[PULL 12/16] vvfat: Fix vvfat_write() for writes before the root directory

[Kevin Wolf]

The calculation in sector2cluster() is done relative to the offset of
the root directory. Any writes to blocks before the start of the root
directory (in particular, writes to the FAT) result in negative values,
which are not handled correctly in vvfat_write().

This changes sector2cluster() to return a signed value, and makes sure
that vvfat_write() doesn't try to find mappings for negative cluster
number. It clarifies the code in vvfat_write() to make it more obvious
that the cluster numbers can be negative.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-Id: <20211209152231.23756-1-kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/vvfat.c | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index 36e73d4c64..b2b58d93b8 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -882,7 +882,7 @@ static int read_directory(BDRVVVFATState* s, int mapping_index)
     return 0;
 }
 
-static inline uint32_t sector2cluster(BDRVVVFATState* s,off_t sector_num)
+static inline int32_t sector2cluster(BDRVVVFATState* s,off_t sector_num)
 {
     return (sector_num - s->offset_to_root_dir) / s->sectors_per_cluster;
 }
@@ -2981,6 +2981,7 @@ static int vvfat_write(BlockDriverState *bs, int64_t sector_num,
 {
     BDRVVVFATState *s = bs->opaque;
     int i, ret;
+    int first_cluster, last_cluster;
 
 DLOG(checkpoint());
 
@@ -2999,9 +3000,20 @@ DLOG(checkpoint());
     if (sector_num < s->offset_to_fat)
         return -1;
 
-    for (i = sector2cluster(s, sector_num);
-            i <= sector2cluster(s, sector_num + nb_sectors - 1);) {
-        mapping_t* mapping = find_mapping_for_cluster(s, i);
+    /*
+     * Values will be negative for writes to the FAT, which is located before
+     * the root directory.
+     */
+    first_cluster = sector2cluster(s, sector_num);
+    last_cluster = sector2cluster(s, sector_num + nb_sectors - 1);
+
+    for (i = first_cluster; i <= last_cluster;) {
+        mapping_t *mapping = NULL;
+
+        if (i >= 0) {
+            mapping = find_mapping_for_cluster(s, i);
+        }
+
         if (mapping) {
             if (mapping->read_only) {
                 fprintf(stderr, "Tried to write to write-protected file %s\n",
@@ -3041,8 +3053,9 @@ DLOG(checkpoint());
                 }
             }
             i = mapping->end;
-        } else
+        } else {
             i++;
+        }
     }
 
     /*
@@ -3056,10 +3069,11 @@ DLOG(fprintf(stderr, "Write to qcow backend: %d + %d\n", (int)sector_num, nb_sec
         return ret;
     }
 
-    for (i = sector2cluster(s, sector_num);
-            i <= sector2cluster(s, sector_num + nb_sectors - 1); i++)
-        if (i >= 0)
+    for (i = first_cluster; i <= last_cluster; i++) {
+        if (i >= 0) {
             s->used_clusters[i] |= USED_ALLOCATED;
+        }
+    }
 
 DLOG(checkpoint());
     /* TODO: add timeout */
-- 
2.31.1

[PULL 13/16] iotests: Test qemu-img convert of zeroed data cluster

[Kevin Wolf]

This demonstrates what happens when the block status changes in
sub-min_sparse granularity, but all of the parts are zeroed out. The
alignment logic in is_allocated_sectors() prevents that the target image
remains fully sparse as expected, but turns it into a data cluster of
explicit zeros.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-Id: <20211217164654.1184218-2-vsementsov@virtuozzo.com>
Tested-by: Peter Lieven <pl@kamp.de>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/122     |  1 +
 tests/qemu-iotests/122.out | 10 ++++++++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/122 b/tests/qemu-iotests/122
index efb260d822..be0f6b79e5 100755
--- a/tests/qemu-iotests/122
+++ b/tests/qemu-iotests/122
@@ -251,6 +251,7 @@ $QEMU_IO -c "write -P 0 0 64k" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_test
 $QEMU_IO -c "write 0 1k" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_testdir
 $QEMU_IO -c "write 8k 1k" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_testdir
 $QEMU_IO -c "write 17k 1k" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_testdir
+$QEMU_IO -c "write -P 0 65k 1k" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_testdir
 
 for min_sparse in 4k 8k; do
     echo
diff --git a/tests/qemu-iotests/122.out b/tests/qemu-iotests/122.out
index 8fbdac2b39..69b8e8b803 100644
--- a/tests/qemu-iotests/122.out
+++ b/tests/qemu-iotests/122.out
@@ -192,6 +192,8 @@ wrote 1024/1024 bytes at offset 8192
 1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 wrote 1024/1024 bytes at offset 17408
 1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 1024/1024 bytes at offset 66560
+1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
 convert -S 4k
 [{ "start": 0, "length": 4096, "depth": 0, "present": true, "zero": false, "data": true, "offset": OFFSET},
@@ -199,7 +201,9 @@ convert -S 4k
 { "start": 8192, "length": 4096, "depth": 0, "present": true, "zero": false, "data": true, "offset": OFFSET},
 { "start": 12288, "length": 4096, "depth": 0, "present": false, "zero": true, "data": false},
 { "start": 16384, "length": 4096, "depth": 0, "present": true, "zero": false, "data": true, "offset": OFFSET},
-{ "start": 20480, "length": 67088384, "depth": 0, "present": false, "zero": true, "data": false}]
+{ "start": 20480, "length": 46080, "depth": 0, "present": false, "zero": true, "data": false},
+{ "start": 66560, "length": 1024, "depth": 0, "present": true, "zero": false, "data": true, "offset": OFFSET},
+{ "start": 67584, "length": 67041280, "depth": 0, "present": false, "zero": true, "data": false}]
 
 convert -c -S 4k
 [{ "start": 0, "length": 1024, "depth": 0, "present": true, "zero": false, "data": true},
@@ -211,7 +215,9 @@ convert -c -S 4k
 
 convert -S 8k
 [{ "start": 0, "length": 24576, "depth": 0, "present": true, "zero": false, "data": true, "offset": OFFSET},
-{ "start": 24576, "length": 67084288, "depth": 0, "present": false, "zero": true, "data": false}]
+{ "start": 24576, "length": 41984, "depth": 0, "present": false, "zero": true, "data": false},
+{ "start": 66560, "length": 1024, "depth": 0, "present": true, "zero": false, "data": true, "offset": OFFSET},
+{ "start": 67584, "length": 67041280, "depth": 0, "present": false, "zero": true, "data": false}]
 
 convert -c -S 8k
 [{ "start": 0, "length": 1024, "depth": 0, "present": true, "zero": false, "data": true},
-- 
2.31.1

[PULL 14/16] qemu-img: make is_allocated_sectors() more efficient

[Kevin Wolf]

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

Consider the case when the whole buffer is zero and end is unaligned.

If i <= tail, we return 1 and do one unaligned WRITE, RMW happens.

If i > tail, we do on aligned WRITE_ZERO (or skip if target is zeroed)
and again one unaligned WRITE, RMW happens.

Let's do better: don't fragment the whole-zero buffer and report it as
ZERO: in case of zeroed target we just do nothing and avoid RMW. If
target is not zeroes, one unaligned WRITE_ZERO should not be much worse
than one unaligned WRITE.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-Id: <20211217164654.1184218-3-vsementsov@virtuozzo.com>
Tested-by: Peter Lieven <pl@kamp.de>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qemu-img.c                 | 23 +++++++++++++++++++----
 tests/qemu-iotests/122.out |  8 ++------
 2 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 21ba1e6800..6fe2466032 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1171,19 +1171,34 @@ static int is_allocated_sectors(const uint8_t *buf, int n, int *pnum,
         }
     }
 
+    if (i == n) {
+        /*
+         * The whole buf is the same.
+         * No reason to split it into chunks, so return now.
+         */
+        *pnum = i;
+        return !is_zero;
+    }
+
     tail = (sector_num + i) & (alignment - 1);
     if (tail) {
         if (is_zero && i <= tail) {
-            /* treat unallocated areas which only consist
-             * of a small tail as allocated. */
+            /*
+             * For sure next sector after i is data, and it will rewrite this
+             * tail anyway due to RMW. So, let's just write data now.
+             */
             is_zero = false;
         }
         if (!is_zero) {
-            /* align up end offset of allocated areas. */
+            /* If possible, align up end offset of allocated areas. */
             i += alignment - tail;
             i = MIN(i, n);
         } else {
-            /* align down end offset of zero areas. */
+            /*
+             * For sure next sector after i is data, and it will rewrite this
+             * tail anyway due to RMW. Better is avoid RMW and write zeroes up
+             * to aligned bound.
+             */
             i -= tail;
         }
     }
diff --git a/tests/qemu-iotests/122.out b/tests/qemu-iotests/122.out
index 69b8e8b803..e18766e167 100644
--- a/tests/qemu-iotests/122.out
+++ b/tests/qemu-iotests/122.out
@@ -201,9 +201,7 @@ convert -S 4k
 { "start": 8192, "length": 4096, "depth": 0, "present": true, "zero": false, "data": true, "offset": OFFSET},
 { "start": 12288, "length": 4096, "depth": 0, "present": false, "zero": true, "data": false},
 { "start": 16384, "length": 4096, "depth": 0, "present": true, "zero": false, "data": true, "offset": OFFSET},
-{ "start": 20480, "length": 46080, "depth": 0, "present": false, "zero": true, "data": false},
-{ "start": 66560, "length": 1024, "depth": 0, "present": true, "zero": false, "data": true, "offset": OFFSET},
-{ "start": 67584, "length": 67041280, "depth": 0, "present": false, "zero": true, "data": false}]
+{ "start": 20480, "length": 67088384, "depth": 0, "present": false, "zero": true, "data": false}]
 
 convert -c -S 4k
 [{ "start": 0, "length": 1024, "depth": 0, "present": true, "zero": false, "data": true},
@@ -215,9 +213,7 @@ convert -c -S 4k
 
 convert -S 8k
 [{ "start": 0, "length": 24576, "depth": 0, "present": true, "zero": false, "data": true, "offset": OFFSET},
-{ "start": 24576, "length": 41984, "depth": 0, "present": false, "zero": true, "data": false},
-{ "start": 66560, "length": 1024, "depth": 0, "present": true, "zero": false, "data": true, "offset": OFFSET},
-{ "start": 67584, "length": 67041280, "depth": 0, "present": false, "zero": true, "data": false}]
+{ "start": 24576, "length": 67084288, "depth": 0, "present": false, "zero": true, "data": false}]
 
 convert -c -S 8k
 [{ "start": 0, "length": 1024, "depth": 0, "present": true, "zero": false, "data": true},
-- 
2.31.1

[PULL 15/16] block: drop BLK_PERM_GRAPH_MOD

[Kevin Wolf]

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

First, this permission never protected a node from being changed, as
generic child-replacing functions don't check it.

Second, it's a strange thing: it presents a permission of parent node
to change its child. But generally, children are replaced by different
mechanisms, like jobs or qmp commands, not by nodes.

Graph-mod permission is hard to understand. All other permissions
describe operations which done by parent node on its child: read,
write, resize. Graph modification operations are something completely
different.

The only place where BLK_PERM_GRAPH_MOD is used as "perm" (not shared
perm) is mirror_start_job, for s->target. Still modern code should use
bdrv_freeze_backing_chain() to protect from graph modification, if we
don't do it somewhere it may be considered as a bug. So, it's a bit
risky to drop GRAPH_MOD, and analyzing of possible loss of protection
is hard. But one day we should do it, let's do it now.

One more bit of information is that locking the corresponding byte in
file-posix doesn't make sense at all.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-Id: <20210902093754.2352-1-vsementsov@virtuozzo.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/block-core.json          |  7 ++-----
 include/block/block.h         |  9 +++++----
 block.c                       |  7 +------
 block/commit.c                |  1 -
 block/mirror.c                | 15 +++------------
 hw/block/block.c              |  3 +--
 scripts/render_block_graph.py |  1 -
 tests/qemu-iotests/273.out    |  4 ----
 8 files changed, 12 insertions(+), 35 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index bd0b285245..9a5a3641d0 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1878,14 +1878,11 @@
 #
 # @resize: This permission is required to change the size of a block node.
 #
-# @graph-mod: This permission is required to change the node that this
-#             BdrvChild points to.
-#
 # Since: 4.0
 ##
 { 'enum': 'BlockPermission',
-  'data': [ 'consistent-read', 'write', 'write-unchanged', 'resize',
-            'graph-mod' ] }
+  'data': [ 'consistent-read', 'write', 'write-unchanged', 'resize' ] }
+
 ##
 # @XDbgBlockGraphEdge:
 #
diff --git a/include/block/block.h b/include/block/block.h
index e5dd22b034..9d4050220b 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -269,12 +269,13 @@ enum {
     BLK_PERM_RESIZE             = 0x08,
 
     /**
-     * This permission is required to change the node that this BdrvChild
-     * points to.
+     * There was a now-removed bit BLK_PERM_GRAPH_MOD, with value of 0x10. QEMU
+     * 6.1 and earlier may still lock the corresponding byte in block/file-posix
+     * locking.  So, implementing some new permission should be very careful to
+     * not interfere with this old unused thing.
      */
-    BLK_PERM_GRAPH_MOD          = 0x10,
 
-    BLK_PERM_ALL                = 0x1f,
+    BLK_PERM_ALL                = 0x0f,
 
     DEFAULT_PERM_PASSTHROUGH    = BLK_PERM_CONSISTENT_READ
                                  | BLK_PERM_WRITE
diff --git a/block.c b/block.c
index 10346b5011..7b3ce415d8 100644
--- a/block.c
+++ b/block.c
@@ -2485,7 +2485,6 @@ char *bdrv_perm_names(uint64_t perm)
         { BLK_PERM_WRITE,           "write" },
         { BLK_PERM_WRITE_UNCHANGED, "write unchanged" },
         { BLK_PERM_RESIZE,          "resize" },
-        { BLK_PERM_GRAPH_MOD,       "change children" },
         { 0, NULL }
     };
 
@@ -2601,8 +2600,7 @@ static void bdrv_default_perms_for_cow(BlockDriverState *bs, BdrvChild *c,
         shared = 0;
     }
 
-    shared |= BLK_PERM_CONSISTENT_READ | BLK_PERM_GRAPH_MOD |
-              BLK_PERM_WRITE_UNCHANGED;
+    shared |= BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED;
 
     if (bs->open_flags & BDRV_O_INACTIVE) {
         shared |= BLK_PERM_WRITE | BLK_PERM_RESIZE;
@@ -2720,7 +2718,6 @@ uint64_t bdrv_qapi_perm_to_blk_perm(BlockPermission qapi_perm)
         [BLOCK_PERMISSION_WRITE]            = BLK_PERM_WRITE,
         [BLOCK_PERMISSION_WRITE_UNCHANGED]  = BLK_PERM_WRITE_UNCHANGED,
         [BLOCK_PERMISSION_RESIZE]           = BLK_PERM_RESIZE,
-        [BLOCK_PERMISSION_GRAPH_MOD]        = BLK_PERM_GRAPH_MOD,
     };
 
     QEMU_BUILD_BUG_ON(ARRAY_SIZE(permissions) != BLOCK_PERMISSION__MAX);
@@ -5546,8 +5543,6 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
     update_inherits_from = bdrv_inherits_from_recursive(base, explicit_top);
 
     /* success - we can delete the intermediate states, and link top->base */
-    /* TODO Check graph modification op blockers (BLK_PERM_GRAPH_MOD) once
-     * we've figured out how they should work. */
     if (!backing_file_str) {
         bdrv_refresh_filename(base);
         backing_file_str = base->filename;
diff --git a/block/commit.c b/block/commit.c
index 10cc5ff451..b1fc7b908b 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -370,7 +370,6 @@ void commit_start(const char *job_id, BlockDriverState *bs,
     s->base = blk_new(s->common.job.aio_context,
                       base_perms,
                       BLK_PERM_CONSISTENT_READ
-                      | BLK_PERM_GRAPH_MOD
                       | BLK_PERM_WRITE_UNCHANGED);
     ret = blk_insert_bs(s->base, base, errp);
     if (ret < 0) {
diff --git a/block/mirror.c b/block/mirror.c
index 959e3dfbd6..69b2c1c697 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1139,10 +1139,7 @@ static void mirror_complete(Job *job, Error **errp)
         replace_aio_context = bdrv_get_aio_context(s->to_replace);
         aio_context_acquire(replace_aio_context);
 
-        /* TODO Translate this into permission system. Current definition of
-         * GRAPH_MOD would require to request it for the parents; they might
-         * not even be BlockDriverStates, however, so a BdrvChild can't address
-         * them. May need redefinition of GRAPH_MOD. */
+        /* TODO Translate this into child freeze system. */
         error_setg(&s->replace_blocker,
                    "block device is in use by block-job-complete");
         bdrv_op_block_all(s->to_replace, s->replace_blocker);
@@ -1666,7 +1663,7 @@ static BlockJob *mirror_start_job(
     s = block_job_create(job_id, driver, NULL, mirror_top_bs,
                          BLK_PERM_CONSISTENT_READ,
                          BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED |
-                         BLK_PERM_WRITE | BLK_PERM_GRAPH_MOD, speed,
+                         BLK_PERM_WRITE, speed,
                          creation_flags, cb, opaque, errp);
     if (!s) {
         goto fail;
@@ -1710,9 +1707,7 @@ static BlockJob *mirror_start_job(
             target_perms |= BLK_PERM_RESIZE;
         }
 
-        target_shared_perms |= BLK_PERM_CONSISTENT_READ
-                            |  BLK_PERM_WRITE
-                            |  BLK_PERM_GRAPH_MOD;
+        target_shared_perms |= BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE;
     } else if (bdrv_chain_contains(bs, bdrv_skip_filters(target))) {
         /*
          * We may want to allow this in the future, but it would
@@ -1723,10 +1718,6 @@ static BlockJob *mirror_start_job(
         goto fail;
     }
 
-    if (backing_mode != MIRROR_LEAVE_BACKING_CHAIN) {
-        target_perms |= BLK_PERM_GRAPH_MOD;
-    }
-
     s->target = blk_new(s->common.job.aio_context,
                         target_perms, target_shared_perms);
     ret = blk_insert_bs(s->target, target, errp);
diff --git a/hw/block/block.c b/hw/block/block.c
index d47ebf005a..25f45df723 100644
--- a/hw/block/block.c
+++ b/hw/block/block.c
@@ -171,8 +171,7 @@ bool blkconf_apply_backend_options(BlockConf *conf, bool readonly,
         perm |= BLK_PERM_WRITE;
     }
 
-    shared_perm = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED |
-                  BLK_PERM_GRAPH_MOD;
+    shared_perm = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED;
     if (resizable) {
         shared_perm |= BLK_PERM_RESIZE;
     }
diff --git a/scripts/render_block_graph.py b/scripts/render_block_graph.py
index da6acf050d..42288a3cfb 100755
--- a/scripts/render_block_graph.py
+++ b/scripts/render_block_graph.py
@@ -35,7 +35,6 @@ def perm(arr):
     s = 'w' if 'write' in arr else '_'
     s += 'r' if 'consistent-read' in arr else '_'
     s += 'u' if 'write-unchanged' in arr else '_'
-    s += 'g' if 'graph-mod' in arr else '_'
     s += 's' if 'resize' in arr else '_'
     return s
 
diff --git a/tests/qemu-iotests/273.out b/tests/qemu-iotests/273.out
index 4e840b6730..6a74a8138b 100644
--- a/tests/qemu-iotests/273.out
+++ b/tests/qemu-iotests/273.out
@@ -204,7 +204,6 @@ Testing: -blockdev file,node-name=base,filename=TEST_DIR/t.IMGFMT.base -blockdev
                 "name": "file",
                 "parent": 5,
                 "shared-perm": [
-                    "graph-mod",
                     "write-unchanged",
                     "consistent-read"
                 ],
@@ -219,7 +218,6 @@ Testing: -blockdev file,node-name=base,filename=TEST_DIR/t.IMGFMT.base -blockdev
                 "name": "backing",
                 "parent": 5,
                 "shared-perm": [
-                    "graph-mod",
                     "resize",
                     "write-unchanged",
                     "write",
@@ -233,7 +231,6 @@ Testing: -blockdev file,node-name=base,filename=TEST_DIR/t.IMGFMT.base -blockdev
                 "name": "file",
                 "parent": 3,
                 "shared-perm": [
-                    "graph-mod",
                     "write-unchanged",
                     "consistent-read"
                 ],
@@ -246,7 +243,6 @@ Testing: -blockdev file,node-name=base,filename=TEST_DIR/t.IMGFMT.base -blockdev
                 "name": "backing",
                 "parent": 3,
                 "shared-perm": [
-                    "graph-mod",
                     "resize",
                     "write-unchanged",
                     "write",
-- 
2.31.1

[PULL 16/16] iotests/testrunner.py: refactor test_field_width

[Kevin Wolf]

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

A lot of Optional[] types doesn't make code beautiful.
test_field_width defaults to 8, but that is never used in the code.

More over, if we want some default behavior for single call of
test_run(), it should just print the whole test name, not limiting or
expanding its width, so 8 is bad default.

So, just drop the default as unused for now.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-Id: <20211210201450.101576-1-vsementsov@virtuozzo.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/testrunner.py | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py
index 0feaa396d0..15788f919e 100644
--- a/tests/qemu-iotests/testrunner.py
+++ b/tests/qemu-iotests/testrunner.py
@@ -174,19 +174,17 @@ def __enter__(self) -> 'TestRunner':
     def __exit__(self, exc_type: Any, exc_value: Any, traceback: Any) -> None:
         self._stack.close()
 
-    def test_print_one_line(self, test: str, starttime: str,
+    def test_print_one_line(self, test: str,
+                            test_field_width: int,
+                            starttime: str,
                             endtime: Optional[str] = None, status: str = '...',
                             lasttime: Optional[float] = None,
                             thistime: Optional[float] = None,
                             description: str = '',
-                            test_field_width: Optional[int] = None,
                             end: str = '\n') -> None:
         """ Print short test info before/after test run """
         test = os.path.basename(test)
 
-        if test_field_width is None:
-            test_field_width = 8
-
         if self.makecheck and status != '...':
             if status and status != 'pass':
                 status = f' [{status}]'
@@ -328,7 +326,7 @@ def do_run_test(self, test: str, mp: bool) -> TestResult:
                               casenotrun=casenotrun)
 
     def run_test(self, test: str,
-                 test_field_width: Optional[int] = None,
+                 test_field_width: int,
                  mp: bool = False) -> TestResult:
         """
         Run one test and print short status
@@ -347,20 +345,21 @@ def run_test(self, test: str,
 
         if not self.makecheck:
             self.test_print_one_line(test=test,
+                                     test_field_width=test_field_width,
                                      status = 'started' if mp else '...',
                                      starttime=start,
                                      lasttime=last_el,
-                                     end = '\n' if mp else '\r',
-                                     test_field_width=test_field_width)
+                                     end = '\n' if mp else '\r')
 
         res = self.do_run_test(test, mp)
 
         end = datetime.datetime.now().strftime('%H:%M:%S')
-        self.test_print_one_line(test=test, status=res.status,
+        self.test_print_one_line(test=test,
+                                 test_field_width=test_field_width,
+                                 status=res.status,
                                  starttime=start, endtime=end,
                                  lasttime=last_el, thistime=res.elapsed,
-                                 description=res.description,
-                                 test_field_width=test_field_width)
+                                 description=res.description)
 
         if res.casenotrun:
             print(res.casenotrun)
-- 
2.31.1

Re: [PULL 07/16] qapi/block: Restrict vhost-user-blk to CONFIG_VHOST_USER_BLK_SERVER

[Philippe Mathieu-Daudé via]

On 14/1/22 14:52, Kevin Wolf wrote:
> From: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> When building QEMU with --disable-vhost-user and using introspection,
> query-qmp-schema lists vhost-user-blk even though it's not actually
> available:
> 
>    { "execute": "query-qmp-schema" }
>    {
>        "return": [
>            ...
>            {
>                "name": "312",
>                "members": [
>                    {
>                        "name": "nbd"
>                    },
>                    {
>                        "name": "vhost-user-blk"
>                    }
>                ],
>                "meta-type": "enum",
>                "values": [
>                    "nbd",
>                    "vhost-user-blk"
>                ]
>            },
> 
> Restrict vhost-user-blk in BlockExportType when
> CONFIG_VHOST_USER_BLK_SERVER is disabled, so it
> doesn't end listed by query-qmp-schema.
> 
> Fixes: 90fc91d50b7 ("convert vhost-user-blk server to block export API")
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Message-Id: <20220107105420.395011-4-f4bug@amsat.org>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   qapi/block-export.json | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/qapi/block-export.json b/qapi/block-export.json
> index c1b92ce1c1..f9ce79a974 100644
> --- a/qapi/block-export.json
> +++ b/qapi/block-export.json
> @@ -277,7 +277,8 @@
>   # Since: 4.2
>   ##
>   { 'enum': 'BlockExportType',
> -  'data': [ 'nbd', 'vhost-user-blk',
> +  'data': [ 'nbd',
> +            { 'name': 'vhost-user-blk', 'if': 'CONFIG_VHOST_USER_BLK_SERVER' },
>               { 'name': 'fuse', 'if': 'CONFIG_FUSE' } ] }

Markus asked to split this line:
https://lore.kernel.org/qemu-devel/87zgny37s8.fsf@dusky.pond.sub.org/
I will add a cleanup patch, no need to cancel this PR for that ;)

Re: [PULL 00/16] Block layer patches

[Peter Maydell]

On Fri, 14 Jan 2022 at 13:52, Kevin Wolf <kwolf@redhat.com> wrote:
>
> The following changes since commit 67b6526cf042f22521feff5ea521a05d3dd2bf8f:
>
>   Merge remote-tracking branch 'remotes/bonzini-gitlab/tags/for-upstream' into staging (2022-01-13 13:59:56 +0000)
>
> are available in the Git repository at:
>
>   git://repo.or.cz/qemu/kevin.git tags/for-upstream
>
> for you to fetch changes up to e5e748739562268ef4063ee77bf53ad7040b25c7:
>
>   iotests/testrunner.py: refactor test_field_width (2022-01-14 12:03:16 +0100)
>
> ----------------------------------------------------------------
> Block layer patches
>
> - qemu-storage-daemon: Add vhost-user-blk help
> - block-backend: Fix use-after-free for BDS pointers after aio_poll()
> - qemu-img: Fix sparseness of output image with unaligned ranges
> - vvfat: Fix crashes in read-write mode
> - Fix device deletion events with -device JSON syntax
> - Code cleanups

I still get intermittent failures for iotests 040, 041 on NetBSD VM,
but those are a pre-existing thing:
https://lore.kernel.org/qemu-devel/CAFEAcA-UKdcTROB7e3jO1qe=WCbuHRuX5WN7HZF2CcdMsmAt=g@mail.gmail.com/


Applied, thanks.

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

-- PMM