[PATCH v2 0/2] block: Use 'read-zeroes=true' mode by default with 'null-co' driver

[PATCH v2 0/2] block: Use 'read-zeroes=true' mode by default with 'null-co' driver

[Philippe Mathieu-Daudé]

The null-co driver doesn't zeroize buffer in its default config,
because it is designed for testing and tests want to run fast.
However this confuses security researchers (access to uninit
buffers).

A one-line patch supposed which became a painful one, because
there is so many different syntax to express the same usage:

 opt = qdict_new();
 qdict_put_str(opt, "read-zeroes", "off");
 null_bs = bdrv_open("null-co://", NULL, opt, BDRV_O_RDWR | BDRV_O_PROTOCOL,
                     &error_abort);

vm.qmp('blockdev-add', driver='null-co', read_zeroes=False, ...)

vm.add_drive_raw("id=drive0,driver=null-co,read-zeroes=off,if=none")

    blk0 = { 'node-name': 'src',
        'driver': 'null-co',
        'read-zeroes': 'off' }

    'file': {
        'driver': 'null-co',
        'read-zeroes': False,
    }

    "file": {
        "driver": "null-co",
        "read-zeroes": "off"
    }

    { "execute": "blockdev-add",
      "arguments": {
          "driver": "null-co",
          "read-zeroes": false,
          "node-name": "disk0"
        }
    }

opts = {'driver': 'null-co,read-zeroes=off', 'node-name': 'root', 'size': 1024}

qemu -drive driver=null-co,read-zeroes=off

qemu-io ... "json:{'driver': 'null-co', 'read-zeroes': false, 'size': 65536}"

qemu-img null-co://,read-zeroes=off

qemu-img ... -o data_file="json:{'driver':'null-co',,'read-zeroes':false,,'size':'4294967296'}"

There are probably more.

Anyhow, the iotests I am not sure and should be audited are 056, 155
(I don't understand the syntax) and 162.

Please review,

Phil.

Philippe Mathieu-Daud=C3=A9 (2):
  block: Explicit null-co uses 'read-zeroes=3Dfalse'
  block/null: Enable 'read-zeroes' mode by default

 docs/devel/testing.rst                     | 14 +++++++-------
 tests/qtest/fuzz/generic_fuzz_configs.h    | 11 ++++++-----
 block/null.c                               |  2 +-
 tests/test-bdrv-drain.c                    | 10 ++++++++--
 tests/acceptance/virtio_check_params.py    |  2 +-
 tests/perf/block/qcow2/convert-blockstatus |  6 +++---
 tests/qemu-iotests/040                     |  2 +-
 tests/qemu-iotests/041                     | 12 ++++++++----
 tests/qemu-iotests/051                     |  2 +-
 tests/qemu-iotests/051.out                 |  2 +-
 tests/qemu-iotests/051.pc.out              |  4 ++--
 tests/qemu-iotests/087                     |  6 ++++--
 tests/qemu-iotests/118                     |  2 +-
 tests/qemu-iotests/133                     |  2 +-
 tests/qemu-iotests/153                     |  8 ++++----
 tests/qemu-iotests/184                     |  2 ++
 tests/qemu-iotests/184.out                 | 10 +++++-----
 tests/qemu-iotests/218                     |  3 +++
 tests/qemu-iotests/224                     |  3 ++-
 tests/qemu-iotests/224.out                 |  8 ++++----
 tests/qemu-iotests/225                     |  2 +-
 tests/qemu-iotests/227                     |  4 ++--
 tests/qemu-iotests/227.out                 |  4 ++--
 tests/qemu-iotests/228                     |  2 +-
 tests/qemu-iotests/235                     |  1 +
 tests/qemu-iotests/245                     |  2 +-
 tests/qemu-iotests/270                     |  2 +-
 tests/qemu-iotests/283                     |  3 ++-
 tests/qemu-iotests/283.out                 |  4 ++--
 tests/qemu-iotests/299                     |  1 +
 tests/qemu-iotests/299.out                 |  2 +-
 tests/qemu-iotests/300                     |  4 ++--
 32 files changed, 82 insertions(+), 60 deletions(-)

--=20
2.26.2

[PATCH v2 1/2] block: Explicit null-co uses 'read-zeroes=false'

[Philippe Mathieu-Daudé]

We are going to switch the 'null-co' default 'read-zeroes' value
from FALSE to TRUE in the next commit. First explicit the FALSE
value when it is not set.

Suggested-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
- Missing: 056 & 155. I couldn't figure out the proper syntax,
  any help welcomed...
- I'm unsure about 162, this doesn't seem to use the null-co
  driver but rather testing global syntax.
---
 docs/devel/testing.rst                     | 14 +++++++-------
 tests/qtest/fuzz/generic_fuzz_configs.h    | 11 ++++++-----
 tests/test-bdrv-drain.c                    | 10 ++++++++--
 tests/acceptance/virtio_check_params.py    |  2 +-
 tests/perf/block/qcow2/convert-blockstatus |  6 +++---
 tests/qemu-iotests/040                     |  2 +-
 tests/qemu-iotests/041                     | 12 ++++++++----
 tests/qemu-iotests/051                     |  2 +-
 tests/qemu-iotests/051.out                 |  2 +-
 tests/qemu-iotests/051.pc.out              |  4 ++--
 tests/qemu-iotests/087                     |  6 ++++--
 tests/qemu-iotests/118                     |  2 +-
 tests/qemu-iotests/133                     |  2 +-
 tests/qemu-iotests/153                     |  8 ++++----
 tests/qemu-iotests/184                     |  2 ++
 tests/qemu-iotests/184.out                 | 10 +++++-----
 tests/qemu-iotests/218                     |  3 +++
 tests/qemu-iotests/224                     |  3 ++-
 tests/qemu-iotests/224.out                 |  8 ++++----
 tests/qemu-iotests/225                     |  2 +-
 tests/qemu-iotests/227                     |  4 ++--
 tests/qemu-iotests/227.out                 |  4 ++--
 tests/qemu-iotests/228                     |  2 +-
 tests/qemu-iotests/235                     |  1 +
 tests/qemu-iotests/245                     |  2 +-
 tests/qemu-iotests/270                     |  2 +-
 tests/qemu-iotests/283                     |  3 ++-
 tests/qemu-iotests/283.out                 |  4 ++--
 tests/qemu-iotests/299                     |  1 +
 tests/qemu-iotests/299.out                 |  2 +-
 tests/qemu-iotests/300                     |  4 ++--
 31 files changed, 81 insertions(+), 59 deletions(-)

diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
index 209f9d8172f..45f1a674384 100644
--- a/docs/devel/testing.rst
+++ b/docs/devel/testing.rst
@@ -216,13 +216,13 @@ code.
 Both Python and Bash frameworks in iotests provide helpers to manage test
 images. They can be used to create and clean up images under the test
 directory. If no I/O or any protocol specific feature is needed, it is often
-more convenient to use the pseudo block driver, ``null-co://``, as the test
-image, which doesn't require image creation or cleaning up. Avoid system-wide
-devices or files whenever possible, such as ``/dev/null`` or ``/dev/zero``.
-Otherwise, image locking implications have to be considered.  For example,
-another application on the host may have locked the file, possibly leading to a
-test failure.  If using such devices are explicitly desired, consider adding
-``locking=off`` option to disable image locking.
+more convenient to use the pseudo block driver, ``null-co://,read-zeroes=off``,
+as the test image, which doesn't require image creation or cleaning up. Avoid
+system-wide devices or files whenever possible, such as ``/dev/null`` or
+``/dev/zero``. Otherwise, image locking implications have to be considered.
+For example, another application on the host may have locked the file, possibly
+leading to a test failure.  If using such devices are explicitly desired,
+consider adding ``locking=off`` option to disable image locking.
 
 Test case groups
 ----------------
diff --git a/tests/qtest/fuzz/generic_fuzz_configs.h b/tests/qtest/fuzz/generic_fuzz_configs.h
index 5d599765c4b..dd5a7aeff0d 100644
--- a/tests/qtest/fuzz/generic_fuzz_configs.h
+++ b/tests/qtest/fuzz/generic_fuzz_configs.h
@@ -38,13 +38,13 @@ const generic_fuzz_config predefined_configs[] = {
     },{
         .name = "virtio-blk",
         .args = "-machine q35 -device virtio-blk,drive=disk0 "
-        "-drive file=null-co://,id=disk0,if=none,format=raw",
+        "-drive file=null-co://,file.read-zeroes=off,id=disk0,if=none,format=raw",
         .objects = "virtio*",
     },{
         .name = "virtio-scsi",
         .args = "-machine q35 -device virtio-scsi,num_queues=8 "
         "-device scsi-hd,drive=disk0 "
-        "-drive file=null-co://,id=disk0,if=none,format=raw",
+        "-drive file=null-co://,file.read-zeroes=off,id=disk0,if=none,format=raw",
         .objects = "scsi* virtio*",
     },{
         .name = "virtio-gpu",
@@ -119,7 +119,7 @@ const generic_fuzz_config predefined_configs[] = {
     },{
         .name = "ahci-hd",
         .args = "-machine q35 -nodefaults "
-        "-drive file=null-co://,if=none,format=raw,id=disk0 "
+        "-drive file=null-co://,file.read-zeroes=off,if=none,format=raw,id=disk0 "
         "-device ide-hd,drive=disk0",
         .objects = "*ahci*",
     },{
@@ -137,7 +137,7 @@ const generic_fuzz_config predefined_configs[] = {
     },{
         .name = "xhci",
         .args = "-machine q35 -nodefaults "
-        "-drive file=null-co://,if=none,format=raw,id=disk0 "
+        "-drive file=null-co://,file.read-zeroes=off,if=none,format=raw,id=disk0 "
         "-device qemu-xhci,id=xhci -device usb-tablet,bus=xhci.0 "
         "-device usb-bot -device usb-storage,drive=disk0 "
         "-chardev null,id=cd0 -chardev null,id=cd1 "
@@ -182,7 +182,8 @@ const generic_fuzz_config predefined_configs[] = {
         .name = "sdhci-v3",
         .args = "-nodefaults -device sdhci-pci,sd-spec-version=3 "
         "-device sd-card,drive=mydrive "
-        "-drive if=sd,index=0,file=null-co://,format=raw,id=mydrive -nographic",
+        "-drive if=sd,index=0,file=null-co://,file.read-zeroes=off,"
+        "format=raw,id=mydrive -nographic",
         .objects = "sd*"
     },{
         .name = "ehci",
diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c
index 8a29e33e004..a3e7d872d88 100644
--- a/tests/test-bdrv-drain.c
+++ b/tests/test-bdrv-drain.c
@@ -27,6 +27,7 @@
 #include "block/blockjob_int.h"
 #include "sysemu/block-backend.h"
 #include "qapi/error.h"
+#include "qapi/qmp/qdict.h"
 #include "qemu/main-loop.h"
 #include "iothread.h"
 
@@ -1177,13 +1178,16 @@ static void do_test_delete_by_drain(bool detach_instead_of_delete,
     BDRVTestTopState *tts;
     TestCoDeleteByDrainData dbdd;
     Coroutine *co;
+    QDict *opt;
 
     bs = bdrv_new_open_driver(&bdrv_test_top_driver, "top", BDRV_O_RDWR,
                               &error_abort);
     bs->total_sectors = 65536 >> BDRV_SECTOR_BITS;
     tts = bs->opaque;
 
-    null_bs = bdrv_open("null-co://", NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
+    opt = qdict_new();
+    qdict_put_str(opt, "read-zeroes", "off");
+    null_bs = bdrv_open("null-co://", NULL, opt, BDRV_O_RDWR | BDRV_O_PROTOCOL,
                         &error_abort);
     bdrv_attach_child(bs, null_bs, "null-child", &child_of_bds,
                       BDRV_CHILD_DATA, &error_abort);
@@ -1201,7 +1205,9 @@ static void do_test_delete_by_drain(bool detach_instead_of_delete,
 
     /* This child is just there to be deleted
      * (for detach_instead_of_delete == true) */
-    null_bs = bdrv_open("null-co://", NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
+    opt = qdict_new();
+    qdict_put_str(opt, "read-zeroes", "off");
+    null_bs = bdrv_open("null-co://", NULL, opt, BDRV_O_RDWR | BDRV_O_PROTOCOL,
                         &error_abort);
     bdrv_attach_child(bs, null_bs, "null-child", &child_of_bds, BDRV_CHILD_DATA,
                       &error_abort);
diff --git a/tests/acceptance/virtio_check_params.py b/tests/acceptance/virtio_check_params.py
index 87e6c839d14..dee386d26f4 100644
--- a/tests/acceptance/virtio_check_params.py
+++ b/tests/acceptance/virtio_check_params.py
@@ -38,7 +38,7 @@
                  'virtio-blk-pci': ['-device',
                                     'virtio-blk-pci,id=scsi0,drive=drive0',
                                     '-drive',
-                                    'driver=null-co,id=drive0,if=none']}
+                                    'driver=null-co,read-zeroes=off,id=drive0,if=none']}
 
 
 class VirtioMaxSegSettingsCheck(Test):
diff --git a/tests/perf/block/qcow2/convert-blockstatus b/tests/perf/block/qcow2/convert-blockstatus
index a1a3c1ef438..c7449eb2c38 100755
--- a/tests/perf/block/qcow2/convert-blockstatus
+++ b/tests/perf/block/qcow2/convert-blockstatus
@@ -46,7 +46,7 @@ done | $QEMU_IO "$src"
 ) > /dev/null
 
 echo -n "plain: "
-/usr/bin/time -f %e $QEMU_IMG convert -n "$src" null-co://
+/usr/bin/time -f %e $QEMU_IMG convert -n "$src" null-co://,read-zeroes=off
 
 # test-case forward
 
@@ -61,11 +61,11 @@ done | $QEMU_IO "$src"
 ) > /dev/null
 
 echo -n "forward: "
-/usr/bin/time -f %e $QEMU_IMG convert -n "$src" null-co://
+/usr/bin/time -f %e $QEMU_IMG convert -n "$src" null-co://,read-zeroes=off
 
 # test-case prealloc
 
 $QEMU_IMG create -f qcow2 -o preallocation=metadata "$src" $size > /dev/null
 
 echo -n "prealloc: "
-/usr/bin/time -f %e $QEMU_IMG convert -n "$src" null-co://
+/usr/bin/time -f %e $QEMU_IMG convert -n "$src" null-co://,read-zeroes=off
diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index 7ebc9ed8257..3abc653e599 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -220,7 +220,7 @@ class TestSingleDrive(ImageCommitTestCase):
     def test_top_node_in_wrong_chain(self):
         self.assert_no_active_block_jobs()
 
-        result = self.vm.qmp('blockdev-add', driver='null-co', node_name='null')
+        result = self.vm.qmp('blockdev-add', driver='null-co', read_zeroes=False, node_name='null')
         self.assert_qmp(result, 'return', {})
 
         result = self.vm.qmp('block-commit', device='drive0', top_node='null', base_node='base')
diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index 5cc02b24fc7..74d2bec02a8 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -1159,13 +1159,16 @@ class TestRepairQuorum(iotests.QMPTestCase):
 class TestOrphanedSource(iotests.QMPTestCase):
     def setUp(self):
         blk0 = { 'node-name': 'src',
-                 'driver': 'null-co' }
+                 'driver': 'null-co',
+                 'read-zeroes': 'off' }
 
         blk1 = { 'node-name': 'dest',
-                 'driver': 'null-co' }
+                 'driver': 'null-co',
+                 'read-zeroes': 'off' }
 
         blk2 = { 'node-name': 'dest-ro',
                  'driver': 'null-co',
+                 'read-zeroes': 'off',
                  'read-only': 'on' }
 
         self.vm = iotests.VM()
@@ -1272,14 +1275,15 @@ class TestReplaces(iotests.QMPTestCase):
                                      'driver': 'copy-on-read',
                                      'node-name': 'filter1',
                                      'file': {
-                                         'driver': 'null-co'
+                                         'driver': 'null-co',
+                                         'read-zeroes': False,
                                      }
                                  }
                              })
         self.assert_qmp(result, 'return', {})
 
         result = self.vm.qmp('blockdev-add',
-                             node_name='target', driver='null-co')
+                             node_name='target', driver='null-co', read_zeroes=False)
         self.assert_qmp(result, 'return', {})
 
         result = self.vm.qmp('blockdev-mirror', job_id='mirror', device='filter0',
diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
index 7cbd1415ce7..42e69054f9f 100755
--- a/tests/qemu-iotests/051
+++ b/tests/qemu-iotests/051
@@ -384,7 +384,7 @@ if [ "${VALGRIND_QEMU_VM}" == "y" ]; then
     _casenotrun "Valgrind needs a valid TMPDIR for itself"
 fi
 VALGRIND_QEMU_VM= \
-TMPDIR=/nonexistent run_qemu -drive driver=null-co,snapshot=on
+TMPDIR=/nonexistent run_qemu -drive driver=null-co,read-zeroes=off,snapshot=on
 
 # Using snapshot=on together with read-only=on
 echo "info block" |
diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
index de4771bcb36..a4bf0cc4401 100644
--- a/tests/qemu-iotests/051.out
+++ b/tests/qemu-iotests/051.out
@@ -459,7 +459,7 @@ wrote 4096/4096 bytes at offset 0
 read 4096/4096 bytes at offset 0
 4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 Testing: -drive driver=null-co,snapshot=on
-QEMU_PROG: -drive driver=null-co,snapshot=on: Could not get temporary filename: No such file or directory
+QEMU_PROG: -drive driver=null-co,read-zeroes=off,snapshot=on: Could not get temporary filename: No such file or directory
 
 Testing: -drive file=TEST_DIR/t.qcow2,snapshot=on,read-only=on,if=none,id=drive0
 QEMU X.Y.Z monitor - type 'help' for more information
diff --git a/tests/qemu-iotests/051.pc.out b/tests/qemu-iotests/051.pc.out
index f707471fb00..12275015eaf 100644
--- a/tests/qemu-iotests/051.pc.out
+++ b/tests/qemu-iotests/051.pc.out
@@ -558,8 +558,8 @@ wrote 4096/4096 bytes at offset 0
 
 read 4096/4096 bytes at offset 0
 4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-Testing: -drive driver=null-co,snapshot=on
-QEMU_PROG: -drive driver=null-co,snapshot=on: Could not get temporary filename: No such file or directory
+Testing: -drive driver=null-co,read-zeroes=off,snapshot=on
+QEMU_PROG: -drive driver=null-co,read-zeroes=off,snapshot=on: Could not get temporary filename: No such file or directory
 
 Testing: -drive file=TEST_DIR/t.qcow2,snapshot=on,read-only=on,if=none,id=drive0
 QEMU X.Y.Z monitor - type 'help' for more information
diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087
index edd43f1a281..93549524277 100755
--- a/tests/qemu-iotests/087
+++ b/tests/qemu-iotests/087
@@ -89,7 +89,8 @@ run_qemu -drive driver=$IMGFMT,id=disk,node-name=test-node,file="$TEST_IMG" <<EO
       "driver": "$IMGFMT",
       "node-name": "disk",
       "file": {
-          "driver": "null-co"
+          "driver": "null-co",
+          "read-zeroes": "off"
       }
     }
   }
@@ -98,7 +99,8 @@ run_qemu -drive driver=$IMGFMT,id=disk,node-name=test-node,file="$TEST_IMG" <<EO
       "driver": "$IMGFMT",
       "node-name": "test-node",
       "file": {
-          "driver": "null-co"
+          "driver": "null-co",
+          "read-zeroes": "off"
       }
     }
   }
diff --git a/tests/qemu-iotests/118 b/tests/qemu-iotests/118
index cae52ffa5ea..2795d7b9edd 100755
--- a/tests/qemu-iotests/118
+++ b/tests/qemu-iotests/118
@@ -629,7 +629,7 @@ class TestBlockJobsAfterCycle(ChangeBaseClass):
         qemu_img('create', '-f', iotests.imgfmt, old_img, '1440K')
 
         self.vm = iotests.VM()
-        self.vm.add_drive_raw("id=drive0,driver=null-co,if=none")
+        self.vm.add_drive_raw("id=drive0,driver=null-co,read-zeroes=off,if=none")
         self.vm.add_device('floppy,drive=drive0,id=%s' % self.device_name)
         self.vm.launch()
 
diff --git a/tests/qemu-iotests/133 b/tests/qemu-iotests/133
index d997db16855..fde80d5ac6c 100755
--- a/tests/qemu-iotests/133
+++ b/tests/qemu-iotests/133
@@ -89,7 +89,7 @@ echo
 # Using the json: pseudo-protocol we can create non-string options
 # (Invoke 'info' just so we get some output afterwards)
 IMGOPTSSYNTAX=false $QEMU_IO -f null-co -c 'reopen' -c 'info' \
-    "json:{'driver': 'null-co', 'size': 65536}"
+    "json:{'driver': 'null-co', 'read-zeroes': false, 'size': 65536}"
 
 echo
 echo "=== Check that mixing -c/-r/-w and their corresponding options is forbidden ==="
diff --git a/tests/qemu-iotests/153 b/tests/qemu-iotests/153
index 607af590918..10299c0ab44 100755
--- a/tests/qemu-iotests/153
+++ b/tests/qemu-iotests/153
@@ -261,17 +261,17 @@ echo "== Detecting -U and force-share conflicts =="
 
 echo
 echo 'No conflict:'
-$QEMU_IMG info -U --image-opts driver=null-co,force-share=on
+$QEMU_IMG info -U --image-opts driver=null-co,read-zeroes=off,force-share=on
 echo
 echo 'Conflict:'
-$QEMU_IMG info -U --image-opts driver=null-co,force-share=off
+$QEMU_IMG info -U --image-opts driver=null-co,read-zeroes=off,force-share=off
 
 echo
 echo 'No conflict:'
-$QEMU_IO -c 'open -r -U -o driver=null-co,force-share=on'
+$QEMU_IO -c 'open -r -U -o driver=null-co,read-zeroes=off,force-share=on'
 echo
 echo 'Conflict:'
-$QEMU_IO -c 'open -r -U -o driver=null-co,force-share=off'
+$QEMU_IO -c 'open -r -U -o driver=null-co,read-zeroes=off,force-share=off'
 
 # success, all done
 echo "*** done"
diff --git a/tests/qemu-iotests/184 b/tests/qemu-iotests/184
index 513d167098e..1a29e2430e2 100755
--- a/tests/qemu-iotests/184
+++ b/tests/qemu-iotests/184
@@ -60,6 +60,7 @@ run_qemu <<EOF
 { "execute": "blockdev-add",
   "arguments": {
     "driver": "null-co",
+    "read-zeroes": false,
     "node-name": "disk0"
   }
 }
@@ -171,6 +172,7 @@ run_qemu <<EOF
 { "execute": "blockdev-add",
   "arguments": {
     "driver": "null-co",
+    "read-zeroes": false,
     "node-name": "disk0"
   }
 }
diff --git a/tests/qemu-iotests/184.out b/tests/qemu-iotests/184.out
index 87c73070e36..ad1762f00f6 100644
--- a/tests/qemu-iotests/184.out
+++ b/tests/qemu-iotests/184.out
@@ -29,12 +29,12 @@ Testing:
             "image": {
                 "backing-image": {
                     "virtual-size": 1073741824,
-                    "filename": "null-co://",
+                    "filename": "json:{\"read-zeroes\": false, \"driver\": \"null-co\"}",
                     "format": "null-co",
                     "actual-size": 0
                 },
                 "virtual-size": 1073741824,
-                "filename": "json:{\"throttle-group\": \"group0\", \"driver\": \"throttle\", \"file\": {\"driver\": \"null-co\"}}",
+                "filename": "json:{\"throttle-group\": \"group0\", \"driver\": \"throttle\", \"file\": {\"read-zeroes\": false, \"driver\": \"null-co\"}}",
                 "format": "throttle",
                 "actual-size": 0
             },
@@ -54,7 +54,7 @@ Testing:
                 "direct": false,
                 "writeback": true
             },
-            "file": "json:{\"throttle-group\": \"group0\", \"driver\": \"throttle\", \"file\": {\"driver\": \"null-co\"}}",
+            "file": "json:{\"throttle-group\": \"group0\", \"driver\": \"throttle\", \"file\": {\"read-zeroes\": false, \"driver\": \"null-co\"}}",
             "encryption_key_missing": false
         },
         {
@@ -62,7 +62,7 @@ Testing:
             "detect_zeroes": "off",
             "image": {
                 "virtual-size": 1073741824,
-                "filename": "null-co://",
+                "filename": "json:{\"read-zeroes\": false, \"driver\": \"null-co\"}",
                 "format": "null-co",
                 "actual-size": 0
             },
@@ -82,7 +82,7 @@ Testing:
                 "direct": false,
                 "writeback": true
             },
-            "file": "null-co://",
+            "file": "json:{\"read-zeroes\": false, \"driver\": \"null-co\"}",
             "encryption_key_missing": false
         }
     ]
diff --git a/tests/qemu-iotests/218 b/tests/qemu-iotests/218
index ae7c4fb187e..23f8dcc7813 100755
--- a/tests/qemu-iotests/218
+++ b/tests/qemu-iotests/218
@@ -44,12 +44,14 @@ def start_mirror(vm, speed=None, buf_size=None):
     ret = vm.qmp('blockdev-add',
                      node_name='source',
                      driver='null-co',
+                     read_zeroes=False,
                      size=1048576)
     assert ret['return'] == {}
 
     ret = vm.qmp('blockdev-add',
                      node_name='target',
                      driver='null-co',
+                     read_zeroes=False,
                      size=1048576)
     assert ret['return'] == {}
 
@@ -174,6 +176,7 @@ with iotests.VM() as vm, \
     ret = vm.qmp('blockdev-add',
                  node_name='target',
                  driver='null-co',
+                 read_zeroes=False,
                  size=(64 * 1048576))
     assert ret['return'] == {}
 
diff --git a/tests/qemu-iotests/224 b/tests/qemu-iotests/224
index 38dd1536254..4ff8a8e05e6 100755
--- a/tests/qemu-iotests/224
+++ b/tests/qemu-iotests/224
@@ -83,7 +83,8 @@ for filter_node_name in False, True:
                                 'filename': base_img_path
                             },
                             'backing': {
-                                'driver': 'null-co'
+                                'driver': 'null-co',
+                                'read-zeroes': False
                             }
                         }
                     },
diff --git a/tests/qemu-iotests/224.out b/tests/qemu-iotests/224.out
index 23374a1d295..89e972e30ec 100644
--- a/tests/qemu-iotests/224.out
+++ b/tests/qemu-iotests/224.out
@@ -1,18 +1,18 @@
 
 --- filter_node_name: False ---
 
-{"execute": "blockdev-add", "arguments": {"backing": {"backing": {"backing": {"driver": "null-co"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-base.img"}, "node-name": "base"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-mid.img"}, "node-name": "mid"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-top.img"}, "node-name": "top"}}
+{"execute": "blockdev-add", "arguments": {"backing": {"backing": {"backing": {"driver": "null-co", "read-zeroes": false}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-base.img"}, "node-name": "base"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-mid.img"}, "node-name": "mid"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-top.img"}, "node-name": "top"}}
 {"return": {}}
-{"execute": "block-commit", "arguments": {"base": "json:{\"backing\": {\"driver\": \"null-co\"}, \"driver\": \"IMGFMT\", \"file\": {\"driver\": \"file\", \"filename\": \"TEST_DIR/PID-base.img\"}}", "device": "top", "job-id": "commit", "speed": 1, "top": "json:{\"backing\": {\"backing\": {\"driver\": \"null-co\"}, \"driver\": \"IMGFMT\", \"file\": {\"driver\": \"file\", \"filename\": \"TEST_DIR/PID-base.img\"}}, \"driver\": \"IMGFMT\", \"file\": {\"driver\": \"file\", \"filename\": \"TEST_DIR/PID-mid.img\"}}"}}
+{"execute": "block-commit", "arguments": {"base": "json:{\"backing\": {\"read-zeroes\": false, \"driver\": \"null-co\"}, \"driver\": \"IMGFMT\", \"file\": {\"driver\": \"file\", \"filename\": \"TEST_DIR/PID-base.img\"}}", "device": "top", "job-id": "commit", "speed": 1, "top": "json:{\"backing\": {\"backing\": {\"read-zeroes\": false, \"driver\": \"null-co\"}, \"driver\": \"IMGFMT\", \"file\": {\"driver\": \"file\", \"filename\": \"TEST_DIR/PID-base.img\"}}, \"driver\": \"IMGFMT\", \"file\": {\"driver\": \"file\", \"filename\": \"TEST_DIR/PID-mid.img\"}}"}}
 {"return": {}}
 {"execute": "job-pause", "arguments": {"id": "commit"}}
 {"return": {}}
 
 --- filter_node_name: True ---
 
-{"execute": "blockdev-add", "arguments": {"backing": {"backing": {"backing": {"driver": "null-co"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-base.img"}, "node-name": "base"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-mid.img"}, "node-name": "mid"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-top.img"}, "node-name": "top"}}
+{"execute": "blockdev-add", "arguments": {"backing": {"backing": {"backing": {"driver": "null-co", "read-zeroes": false}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-base.img"}, "node-name": "base"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-mid.img"}, "node-name": "mid"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-top.img"}, "node-name": "top"}}
 {"return": {}}
-{"execute": "block-commit", "arguments": {"base": "json:{\"backing\": {\"driver\": \"null-co\"}, \"driver\": \"IMGFMT\", \"file\": {\"driver\": \"file\", \"filename\": \"TEST_DIR/PID-base.img\"}}", "device": "top", "filter-node-name": "filter_node", "job-id": "commit", "speed": 1, "top": "json:{\"backing\": {\"backing\": {\"driver\": \"null-co\"}, \"driver\": \"IMGFMT\", \"file\": {\"driver\": \"file\", \"filename\": \"TEST_DIR/PID-base.img\"}}, \"driver\": \"IMGFMT\", \"file\": {\"driver\": \"file\", \"filename\": \"TEST_DIR/PID-mid.img\"}}"}}
+{"execute": "block-commit", "arguments": {"base": "json:{\"backing\": {\"read-zeroes\": false, \"driver\": \"null-co\"}, \"driver\": \"IMGFMT\", \"file\": {\"driver\": \"file\", \"filename\": \"TEST_DIR/PID-base.img\"}}", "device": "top", "filter-node-name": "filter_node", "job-id": "commit", "speed": 1, "top": "json:{\"backing\": {\"backing\": {\"read-zeroes\": false, \"driver\": \"null-co\"}, \"driver\": \"IMGFMT\", \"file\": {\"driver\": \"file\", \"filename\": \"TEST_DIR/PID-base.img\"}}, \"driver\": \"IMGFMT\", \"file\": {\"driver\": \"file\", \"filename\": \"TEST_DIR/PID-mid.img\"}}"}}
 {"return": {}}
 {"execute": "job-pause", "arguments": {"id": "commit"}}
 {"return": {}}
diff --git a/tests/qemu-iotests/225 b/tests/qemu-iotests/225
index c0053790db8..f62a1af1ccc 100755
--- a/tests/qemu-iotests/225
+++ b/tests/qemu-iotests/225
@@ -76,7 +76,7 @@ overlay_opts=$(make_opts overlay "$TEST_IMG" backing)
 base_opts=$(make_opts backing "$TEST_IMG.base")
 not_base_opts=$(make_opts backing "$TEST_IMG.not_base")
 
-not_vmdk_opts="{ 'node-name': 'backing', 'driver': 'null-co' }"
+not_vmdk_opts="{ 'node-name': 'backing', 'driver': 'null-co', 'read-zeroes': false }"
 
 echo
 echo '=== Testing fitting VMDK backing image ==='
diff --git a/tests/qemu-iotests/227 b/tests/qemu-iotests/227
index 7e45a47ac61..45955f61855 100755
--- a/tests/qemu-iotests/227
+++ b/tests/qemu-iotests/227
@@ -68,7 +68,7 @@ echo
 echo '=== blockstats with -drive if=none ==='
 echo
 
-run_qemu -drive driver=null-co,if=none <<EOF
+run_qemu -drive driver=null-co,read-zeroes=off,if=none <<EOF
 { "execute": "qmp_capabilities" }
 { "execute": "query-blockstats"}
 { "execute": "quit" }
@@ -78,7 +78,7 @@ echo
 echo '=== blockstats with -blockdev ==='
 echo
 
-run_qemu -blockdev driver=null-co,node-name=null <<EOF
+run_qemu -blockdev driver=null-co,read-zeroes=off,node-name=null <<EOF
 { "execute": "qmp_capabilities" }
 { "execute": "query-blockstats"}
 { "execute": "quit" }
diff --git a/tests/qemu-iotests/227.out b/tests/qemu-iotests/227.out
index 9c09ee3917b..f5283b86ce8 100644
--- a/tests/qemu-iotests/227.out
+++ b/tests/qemu-iotests/227.out
@@ -67,7 +67,7 @@ Testing: -drive driver=null-co,read-zeroes=on,if=virtio
 
 === blockstats with -drive if=none ===
 
-Testing: -drive driver=null-co,if=none
+Testing: -drive driver=null-co,read-zeroes=off,if=none
 {
     QMP_VERSION
 }
@@ -131,7 +131,7 @@ Testing: -drive driver=null-co,if=none
 
 === blockstats with -blockdev ===
 
-Testing: -blockdev driver=null-co,node-name=null
+Testing: -blockdev driver=null-co,read-zeroes=off,node-name=null
 {
     QMP_VERSION
 }
diff --git a/tests/qemu-iotests/228 b/tests/qemu-iotests/228
index a5eda2e149b..7188b1b00ca 100755
--- a/tests/qemu-iotests/228
+++ b/tests/qemu-iotests/228
@@ -177,7 +177,7 @@ with iotests.FilePath('base.img') as base_img_path, \
 
     # You can only reliably override backing options by using a node
     # reference (or by specifying file.filename, but, well...)
-    vm.qmp_log('blockdev-add', node_name='null', driver='null-co')
+    vm.qmp_log('blockdev-add', node_name='null', driver='null-co', read_zeroes=False)
 
     vm.qmp_log('blockdev-add',
                node_name='node0',
diff --git a/tests/qemu-iotests/235 b/tests/qemu-iotests/235
index 20d16dbf38f..6b0b14590b8 100755
--- a/tests/qemu-iotests/235
+++ b/tests/qemu-iotests/235
@@ -65,6 +65,7 @@ log(vm.qmp('blockdev-add',
                'throttle-group': 'tg0',
                'file': {
                    'driver': 'null-co',
+                   'read-zeroes': False,
                    'size': size
                 } }))
 
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index cfdeb902be3..f152a7a4e0a 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -602,7 +602,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
         ##################
         ###### null ######
         ##################
-        opts = {'driver': 'null-co', 'node-name': 'root', 'size': 1024}
+        opts = {'driver': 'null-co,read-zeroes=off', 'node-name': 'root', 'size': 1024}
 
         result = self.vm.qmp('blockdev-add', conv_keys = False, **opts)
         self.assert_qmp(result, 'return', {})
diff --git a/tests/qemu-iotests/270 b/tests/qemu-iotests/270
index 74352342db5..4aeb545a900 100755
--- a/tests/qemu-iotests/270
+++ b/tests/qemu-iotests/270
@@ -60,7 +60,7 @@ _make_test_img -o cluster_size=2M,data_file="$TEST_IMG.orig" \
 # "write" 2G of data without using any space.
 # (qemu-img create does not like it, though, because null-co does not
 # support image creation.)
-$QEMU_IMG amend -o data_file="json:{'driver':'null-co',,'size':'4294967296'}" \
+$QEMU_IMG amend -o data_file="json:{'driver':'null-co',,'read-zeroes':false,,'size':'4294967296'}" \
     "$TEST_IMG"
 
 # This gives us a range of:
diff --git a/tests/qemu-iotests/283 b/tests/qemu-iotests/283
index 79643e375b7..cd235f9ad5f 100755
--- a/tests/qemu-iotests/283
+++ b/tests/qemu-iotests/283
@@ -78,13 +78,14 @@ vm.launch()
 vm.qmp_log('blockdev-add', **{
     'node-name': 'target',
     'driver': 'null-co',
+    'read-zeroes': False,
     'size': size,
 })
 
 vm.qmp_log('blockdev-add', **{
     'node-name': 'source',
     'driver': 'blkdebug',
-    'image': {'node-name': 'base', 'driver': 'null-co', 'size': size}
+    'image': {'node-name': 'base', 'driver': 'null-co', 'read-zeroes': False, 'size': size}
 })
 
 vm.qmp_log('blockdev-add', **{
diff --git a/tests/qemu-iotests/283.out b/tests/qemu-iotests/283.out
index d8cff22cc19..d5f759a4d91 100644
--- a/tests/qemu-iotests/283.out
+++ b/tests/qemu-iotests/283.out
@@ -1,6 +1,6 @@
-{"execute": "blockdev-add", "arguments": {"driver": "null-co", "node-name": "target", "size": 1048576}}
+{"execute": "blockdev-add", "arguments": {"driver": "null-co", "node-name": "target", "read-zeroes": false, "size": 1048576}}
 {"return": {}}
-{"execute": "blockdev-add", "arguments": {"driver": "blkdebug", "image": {"driver": "null-co", "node-name": "base", "size": 1048576}, "node-name": "source"}}
+{"execute": "blockdev-add", "arguments": {"driver": "blkdebug", "image": {"driver": "null-co", "node-name": "base", "read-zeroes": false, "size": 1048576}, "node-name": "source"}}
 {"return": {}}
 {"execute": "blockdev-add", "arguments": {"driver": "blkdebug", "image": "base", "node-name": "other", "take-child-perms": ["write"]}}
 {"return": {}}
diff --git a/tests/qemu-iotests/299 b/tests/qemu-iotests/299
index a7122941fd2..6fd8402e8f9 100755
--- a/tests/qemu-iotests/299
+++ b/tests/qemu-iotests/299
@@ -36,6 +36,7 @@ vm.launch()
 vm.qmp_log('blockdev-add', **{
     'node-name': 'disk',
     'driver': 'null-co',
+    'read-zeroes': False,
     'size': 1024 * 1024,
 })
 
diff --git a/tests/qemu-iotests/299.out b/tests/qemu-iotests/299.out
index bba4252923c..a6f2086e0eb 100644
--- a/tests/qemu-iotests/299.out
+++ b/tests/qemu-iotests/299.out
@@ -1,4 +1,4 @@
-{"execute": "blockdev-add", "arguments": {"driver": "null-co", "node-name": "disk", "size": 1048576}}
+{"execute": "blockdev-add", "arguments": {"driver": "null-co", "node-name": "disk", "read-zeroes": false, "size": 1048576}}
 {"return": {}}
 {"execute": "block-dirty-bitmap-add", "arguments": {"name": "bitmap0", "node": "disk"}}
 {"return": {}}
diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300
index 43264d883d7..5464c3a9ff7 100755
--- a/tests/qemu-iotests/300
+++ b/tests/qemu-iotests/300
@@ -44,12 +44,12 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
     def setUp(self) -> None:
         self.vm_a = iotests.VM(path_suffix='-a')
         self.vm_a.add_blockdev(f'node-name={self.src_node_name},'
-                               'driver=null-co')
+                               'driver=null-co,read-zeroes=off')
         self.vm_a.launch()
 
         self.vm_b = iotests.VM(path_suffix='-b')
         self.vm_b.add_blockdev(f'node-name={self.dst_node_name},'
-                               'driver=null-co')
+                               'driver=null-co,read-zeroes=off')
         self.vm_b.add_incoming(f'unix:{mig_sock}')
         self.vm_b.launch()
 
-- 
2.26.2

Re: [PATCH v2 1/2] block: Explicit null-co uses 'read-zeroes=false'

[Vladimir Sementsov-Ogievskiy]

11.02.2021 17:26, Philippe Mathieu-Daudé wrote:
> We are going to switch the 'null-co' default 'read-zeroes' value
> from FALSE to TRUE in the next commit. First explicit the FALSE
> value when it is not set.
> 
> Suggested-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> - Missing: 056 & 155. I couldn't figure out the proper syntax,
>    any help welcomed...
> - I'm unsure about 162, this doesn't seem to use the null-co
>    driver but rather testing global syntax.
> ---
>   docs/devel/testing.rst                     | 14 +++++++-------
>   tests/qtest/fuzz/generic_fuzz_configs.h    | 11 ++++++-----
>   tests/test-bdrv-drain.c                    | 10 ++++++++--
>   tests/acceptance/virtio_check_params.py    |  2 +-
>   tests/perf/block/qcow2/convert-blockstatus |  6 +++---
>   tests/qemu-iotests/040                     |  2 +-
>   tests/qemu-iotests/041                     | 12 ++++++++----
>   tests/qemu-iotests/051                     |  2 +-
>   tests/qemu-iotests/051.out                 |  2 +-
>   tests/qemu-iotests/051.pc.out              |  4 ++--
>   tests/qemu-iotests/087                     |  6 ++++--
>   tests/qemu-iotests/118                     |  2 +-
>   tests/qemu-iotests/133                     |  2 +-
>   tests/qemu-iotests/153                     |  8 ++++----
>   tests/qemu-iotests/184                     |  2 ++
>   tests/qemu-iotests/184.out                 | 10 +++++-----
>   tests/qemu-iotests/218                     |  3 +++
>   tests/qemu-iotests/224                     |  3 ++-
>   tests/qemu-iotests/224.out                 |  8 ++++----
>   tests/qemu-iotests/225                     |  2 +-
>   tests/qemu-iotests/227                     |  4 ++--
>   tests/qemu-iotests/227.out                 |  4 ++--
>   tests/qemu-iotests/228                     |  2 +-
>   tests/qemu-iotests/235                     |  1 +
>   tests/qemu-iotests/245                     |  2 +-
>   tests/qemu-iotests/270                     |  2 +-
>   tests/qemu-iotests/283                     |  3 ++-
>   tests/qemu-iotests/283.out                 |  4 ++--
>   tests/qemu-iotests/299                     |  1 +
>   tests/qemu-iotests/299.out                 |  2 +-
>   tests/qemu-iotests/300                     |  4 ++--

Why do you think these tests will work bad with new default?

At least everything under tests/qemu-iotests/ and tests/test-bdrv-drain are not about performance

tests/perf/block/qcow2/convert-blockstatus is OK with new default too.


-- 
Best regards,
Vladimir

Re: [PATCH v2 1/2] block: Explicit null-co uses 'read-zeroes=false'

[Philippe Mathieu-Daudé]

Hi Vladimir,

On 2/11/21 5:29 PM, Vladimir Sementsov-Ogievskiy wrote:
> 11.02.2021 17:26, Philippe Mathieu-Daudé wrote:
>> We are going to switch the 'null-co' default 'read-zeroes' value
>> from FALSE to TRUE in the next commit. First explicit the FALSE
>> value when it is not set.
>>
>> Suggested-by: Eric Blake <eblake@redhat.com>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> - Missing: 056 & 155. I couldn't figure out the proper syntax,
>>    any help welcomed...
>> - I'm unsure about 162, this doesn't seem to use the null-co
>>    driver but rather testing global syntax.
>> ---
>>   docs/devel/testing.rst                     | 14 +++++++-------
>>   tests/qtest/fuzz/generic_fuzz_configs.h    | 11 ++++++-----
>>   tests/test-bdrv-drain.c                    | 10 ++++++++--
>>   tests/acceptance/virtio_check_params.py    |  2 +-
>>   tests/perf/block/qcow2/convert-blockstatus |  6 +++---
>>   tests/qemu-iotests/040                     |  2 +-
>>   tests/qemu-iotests/041                     | 12 ++++++++----
>>   tests/qemu-iotests/051                     |  2 +-
>>   tests/qemu-iotests/051.out                 |  2 +-
>>   tests/qemu-iotests/051.pc.out              |  4 ++--
>>   tests/qemu-iotests/087                     |  6 ++++--
>>   tests/qemu-iotests/118                     |  2 +-
>>   tests/qemu-iotests/133                     |  2 +-
>>   tests/qemu-iotests/153                     |  8 ++++----
>>   tests/qemu-iotests/184                     |  2 ++
>>   tests/qemu-iotests/184.out                 | 10 +++++-----
>>   tests/qemu-iotests/218                     |  3 +++
>>   tests/qemu-iotests/224                     |  3 ++-
>>   tests/qemu-iotests/224.out                 |  8 ++++----
>>   tests/qemu-iotests/225                     |  2 +-
>>   tests/qemu-iotests/227                     |  4 ++--
>>   tests/qemu-iotests/227.out                 |  4 ++--
>>   tests/qemu-iotests/228                     |  2 +-
>>   tests/qemu-iotests/235                     |  1 +
>>   tests/qemu-iotests/245                     |  2 +-
>>   tests/qemu-iotests/270                     |  2 +-
>>   tests/qemu-iotests/283                     |  3 ++-
>>   tests/qemu-iotests/283.out                 |  4 ++--
>>   tests/qemu-iotests/299                     |  1 +
>>   tests/qemu-iotests/299.out                 |  2 +-
>>   tests/qemu-iotests/300                     |  4 ++--
> 
> Why do you think these tests will work bad with new default?

As I don't understand the tests, this was the deal with Eric :)

"OK to change default if current default is explicited" then
block maintainers could audit each case and see which one can
safely use read-zeroes=true.

> 
> At least everything under tests/qemu-iotests/ and tests/test-bdrv-drain
> are not about performance
> 
> tests/perf/block/qcow2/convert-blockstatus is OK with new default too.

OK, I'll see who should send these patches on top with Eric & Max.

Thank for your review,

Phil.

Re: [PATCH v2 1/2] block: Explicit null-co uses 'read-zeroes=false'

[Eric Blake]

On 2/11/21 8:26 AM, Philippe Mathieu-Daudé wrote:
> We are going to switch the 'null-co' default 'read-zeroes' value
> from FALSE to TRUE in the next commit. First explicit the FALSE
> value when it is not set.

Grammar suggestion, along with a suggestion for an additional sentence
to make the intent of this commit clearer:

As a first step, request an explicit FALSE value rather than relying on
the defaults.  This is intended to be a purely mechanical adjustment for
no performance behavior in the tests; later patches may then flip or
elide the explicit choice for tests where performance does not matter.

> 
> Suggested-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> - Missing: 056 & 155. I couldn't figure out the proper syntax,
>   any help welcomed...

056 - looks like just one line:
        self.vm =
iotests.VM().add_drive_raw("file=blkdebug::null-co://,id=drive0,align=65536,driver=blkdebug")

the best way to add it here would be rewriting that line to use blockdev
syntax rather than blkdebug: URI syntax.  The other question is whether
it is a noticeable time difference when the default is flipped in 2/2.

155 - looks like several uses such as:

class TestBlockdevMirrorForcedBacking(MirrorBaseClass):
    cmd = 'blockdev-mirror'
    existing = True
    target_backing = None
    target_blockdev_backing = { 'driver': 'null-co' }
    target_real_backing = 'null-co://'


> - I'm unsure about 162, this doesn't seem to use the null-co
>   driver but rather testing global syntax.

Concur; I don't see any reason to worry about this one (but mentioning
it in the commit message can't hurt in case someone asks later)

# blkdebug expects all of its arguments to be strings, but its
# bdrv_refresh_filename() implementation should not assume that they
have been
# passed as strings in the original options QDict.
# So this should emit blkdebug:42:null-co:// as the filename:
touch 42
$QEMU_IMG info 'json:{"driver": "blkdebug", "config": 42,
                      "image.driver": "null-co"}' \


> ---
>  docs/devel/testing.rst                     | 14 +++++++-------
>  tests/qtest/fuzz/generic_fuzz_configs.h    | 11 ++++++-----
>  tests/test-bdrv-drain.c                    | 10 ++++++++--
>  tests/acceptance/virtio_check_params.py    |  2 +-
>  tests/perf/block/qcow2/convert-blockstatus |  6 +++---
>  tests/qemu-iotests/040                     |  2 +-

You did a pretty good hunt for culprits!

>  tests/qemu-iotests/041                     | 12 ++++++++----
>  tests/qemu-iotests/051                     |  2 +-
>  tests/qemu-iotests/051.out                 |  2 +-
>  tests/qemu-iotests/051.pc.out              |  4 ++--

and for the fallout to the iotests.

I did not audit for which tests are easy candidates for dropping the
explicit read-zeroes=false (that is, where the extra time in allowing
the flipped default doesn't penalize the test), but am okay giving this
patch:

Reviewed-by: Eric Blake <eblake@redhat.com>


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 1/2] block: Explicit null-co uses 'read-zeroes=false'

[Philippe Mathieu-Daudé]

On 2/11/21 11:40 PM, Eric Blake wrote:
> On 2/11/21 8:26 AM, Philippe Mathieu-Daudé wrote:
>> We are going to switch the 'null-co' default 'read-zeroes' value
>> from FALSE to TRUE in the next commit. First explicit the FALSE
>> value when it is not set.
> 
> Grammar suggestion, along with a suggestion for an additional sentence
> to make the intent of this commit clearer:
> 
> As a first step, request an explicit FALSE value rather than relying on
> the defaults.  This is intended to be a purely mechanical adjustment for
> no performance behavior in the tests; later patches may then flip or
> elide the explicit choice for tests where performance does not matter.
> 
>>
>> Suggested-by: Eric Blake <eblake@redhat.com>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> - Missing: 056 & 155. I couldn't figure out the proper syntax,
>>   any help welcomed...
> 
> 056 - looks like just one line:
>         self.vm =
> iotests.VM().add_drive_raw("file=blkdebug::null-co://,id=drive0,align=65536,driver=blkdebug")
> 
> the best way to add it here would be rewriting that line to use blockdev
> syntax rather than blkdebug: URI syntax.  The other question is whether
> it is a noticeable time difference when the default is flipped in 2/2.
> 
> 155 - looks like several uses such as:
> 
> class TestBlockdevMirrorForcedBacking(MirrorBaseClass):
>     cmd = 'blockdev-mirror'
>     existing = True
>     target_backing = None
>     target_blockdev_backing = { 'driver': 'null-co' }
>     target_real_backing = 'null-co://'
> 
> 
>> - I'm unsure about 162, this doesn't seem to use the null-co
>>   driver but rather testing global syntax.
> 
> Concur; I don't see any reason to worry about this one (but mentioning
> it in the commit message can't hurt in case someone asks later)
> 
> # blkdebug expects all of its arguments to be strings, but its
> # bdrv_refresh_filename() implementation should not assume that they
> have been
> # passed as strings in the original options QDict.
> # So this should emit blkdebug:42:null-co:// as the filename:
> touch 42
> $QEMU_IMG info 'json:{"driver": "blkdebug", "config": 42,
>                       "image.driver": "null-co"}' \
> 
> 
>> ---
>>  docs/devel/testing.rst                     | 14 +++++++-------
>>  tests/qtest/fuzz/generic_fuzz_configs.h    | 11 ++++++-----
>>  tests/test-bdrv-drain.c                    | 10 ++++++++--
>>  tests/acceptance/virtio_check_params.py    |  2 +-
>>  tests/perf/block/qcow2/convert-blockstatus |  6 +++---
>>  tests/qemu-iotests/040                     |  2 +-
> 
> You did a pretty good hunt for culprits!
> 
>>  tests/qemu-iotests/041                     | 12 ++++++++----
>>  tests/qemu-iotests/051                     |  2 +-
>>  tests/qemu-iotests/051.out                 |  2 +-
>>  tests/qemu-iotests/051.pc.out              |  4 ++--
> 
> and for the fallout to the iotests.
> 
> I did not audit for which tests are easy candidates for dropping the
> explicit read-zeroes=false (that is, where the extra time in allowing
> the flipped default doesn't penalize the test), but am okay giving this
> patch:
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks for your help. I'll address your comments and respin.

Regards,

Phil.

Re: [PATCH v2 1/2] block: Explicit null-co uses 'read-zeroes=false'

[Vladimir Sementsov-Ogievskiy]

12.02.2021 01:40, Eric Blake wrote:
> On 2/11/21 8:26 AM, Philippe Mathieu-Daudé wrote:
>> We are going to switch the 'null-co' default 'read-zeroes' value
>> from FALSE to TRUE in the next commit. First explicit the FALSE
>> value when it is not set.
> 
> Grammar suggestion, along with a suggestion for an additional sentence
> to make the intent of this commit clearer:
> 
> As a first step, request an explicit FALSE value rather than relying on
> the defaults.  This is intended to be a purely mechanical adjustment for
> no performance behavior in the tests; later patches may then flip or
> elide the explicit choice for tests where performance does not matter.
> 
>>
>> Suggested-by: Eric Blake <eblake@redhat.com>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> - Missing: 056 & 155. I couldn't figure out the proper syntax,
>>    any help welcomed...
> 
> 056 - looks like just one line:
>          self.vm =
> iotests.VM().add_drive_raw("file=blkdebug::null-co://,id=drive0,align=65536,driver=blkdebug")
> 
> the best way to add it here would be rewriting that line to use blockdev
> syntax rather than blkdebug: URI syntax.  The other question is whether
> it is a noticeable time difference when the default is flipped in 2/2.
> 
> 155 - looks like several uses such as:
> 
> class TestBlockdevMirrorForcedBacking(MirrorBaseClass):
>      cmd = 'blockdev-mirror'
>      existing = True
>      target_backing = None
>      target_blockdev_backing = { 'driver': 'null-co' }
>      target_real_backing = 'null-co://'
> 
> 
>> - I'm unsure about 162, this doesn't seem to use the null-co
>>    driver but rather testing global syntax.
> 
> Concur; I don't see any reason to worry about this one (but mentioning
> it in the commit message can't hurt in case someone asks later)
> 
> # blkdebug expects all of its arguments to be strings, but its
> # bdrv_refresh_filename() implementation should not assume that they
> have been
> # passed as strings in the original options QDict.
> # So this should emit blkdebug:42:null-co:// as the filename:
> touch 42
> $QEMU_IMG info 'json:{"driver": "blkdebug", "config": 42,
>                        "image.driver": "null-co"}' \
> 
> 
>> ---
>>   docs/devel/testing.rst                     | 14 +++++++-------
>>   tests/qtest/fuzz/generic_fuzz_configs.h    | 11 ++++++-----
>>   tests/test-bdrv-drain.c                    | 10 ++++++++--
>>   tests/acceptance/virtio_check_params.py    |  2 +-
>>   tests/perf/block/qcow2/convert-blockstatus |  6 +++---
>>   tests/qemu-iotests/040                     |  2 +-
> 
> You did a pretty good hunt for culprits!
> 
>>   tests/qemu-iotests/041                     | 12 ++++++++----
>>   tests/qemu-iotests/051                     |  2 +-
>>   tests/qemu-iotests/051.out                 |  2 +-
>>   tests/qemu-iotests/051.pc.out              |  4 ++--
> 
> and for the fallout to the iotests.
> 
> I did not audit for which tests are easy candidates for dropping the
> explicit read-zeroes=false (that is, where the extra time in allowing
> the flipped default doesn't penalize the test), but am okay giving this
> patch:
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> 

I'd keep tests/qemu-iotests as is..

Benefits of keeping old behavior (by adding read-zeroes=false)

  - saving several milliseconds. I doubt that it would be even noticeable. Could you give have much time of "make check-block" it saves, if we update all iotests to use read-zeroes=false, instead of transparently move to new default?

Drawbacks of adding read-zeroes=false:

  - it's generally better to test only default behavior than only non-default behavior
  - this patch creates extra conflicts on any patch porting between upstream/downstream
  - for new tests, nobady will care about read-zeroes and will not add it.. (or will add when copy existing test no thinking about it), so it will be a kind of mess

-- 
Best regards,
Vladimir

Re: [PATCH v2 1/2] block: Explicit null-co uses 'read-zeroes=false'

[Eric Blake]

On 2/11/21 8:26 AM, Philippe Mathieu-Daudé wrote:
> We are going to switch the 'null-co' default 'read-zeroes' value
> from FALSE to TRUE in the next commit. First explicit the FALSE
> value when it is not set.
> 
> Suggested-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---

> +++ b/tests/qemu-iotests/300
> @@ -44,12 +44,12 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
>      def setUp(self) -> None:
>          self.vm_a = iotests.VM(path_suffix='-a')
>          self.vm_a.add_blockdev(f'node-name={self.src_node_name},'
> -                               'driver=null-co')
> +                               'driver=null-co,read-zeroes=off')
>          self.vm_a.launch()
>  
>          self.vm_b = iotests.VM(path_suffix='-b')
>          self.vm_b.add_blockdev(f'node-name={self.dst_node_name},'
> -                               'driver=null-co')
> +                               'driver=null-co,read-zeroes=off')
>          self.vm_b.add_incoming(f'unix:{mig_sock}')
>          self.vm_b.launch()
>  
> 

Incomplete: 300 has a couple more lines that look like:

tests/qemu-iotests/300-        result = self.vm_a.qmp('blockdev-add',
tests/qemu-iotests/300:
node_name='node-b', driver='null-co')
--
tests/qemu-iotests/300-        result = self.vm_b.qmp('blockdev-add',
tests/qemu-iotests/300:
node_name='node-a', driver='null-co')

that could use the same treatment (noticed while reviewing Peter's patch
to add yet more uses in that test).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

[PATCH v2 2/2] block/null: Enable 'read-zeroes' mode by default

[Philippe Mathieu-Daudé]

The null-co driver is meant for (performance) testing.
By default, read operation does nothing, the provided buffer
is not filled with zero values and its content is unchanged.

This performance 'feature' becomes an issue from a security
perspective.  For example, using the default null-co driver,
buf[] is uninitialized, the blk_pread() call succeeds and we
then access uninitialized memory:

  static int guess_disk_lchs(BlockBackend *blk,
                             int *pcylinders, int *pheads,
                             int *psectors)
  {
      uint8_t buf[BDRV_SECTOR_SIZE];
      ...

      if (blk_pread(blk, 0, buf, BDRV_SECTOR_SIZE) < 0) {
          return -1;
      }
      /* test msdos magic */
      if (buf[510] != 0x55 || buf[511] != 0xaa) {
          return -1;
      }

We could audit all the uninitialized buffers and the
bdrv_co_preadv() handlers, but it is simpler to change the
default of this testing driver. Performance tests will have
to adapt and use 'null-co,read-zeroes=off'.

Suggested-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/null.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/null.c b/block/null.c
index cc9b1d4ea72..f9658fd70ac 100644
--- a/block/null.c
+++ b/block/null.c
@@ -93,7 +93,7 @@ static int null_file_open(BlockDriverState *bs, QDict *options, int flags,
         error_setg(errp, "latency-ns is invalid");
         ret = -EINVAL;
     }
-    s->read_zeroes = qemu_opt_get_bool(opts, NULL_OPT_ZEROES, false);
+    s->read_zeroes = qemu_opt_get_bool(opts, NULL_OPT_ZEROES, true);
     qemu_opts_del(opts);
     bs->supported_write_flags = BDRV_REQ_FUA;
     return ret;
-- 
2.26.2

Re: [PATCH v2 2/2] block/null: Enable 'read-zeroes' mode by default

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1414 bytes --]

On Thu, Feb 11, 2021 at 03:26:56PM +0100, Philippe Mathieu-Daudé wrote:
> The null-co driver is meant for (performance) testing.
> By default, read operation does nothing, the provided buffer
> is not filled with zero values and its content is unchanged.
> 
> This performance 'feature' becomes an issue from a security
> perspective.  For example, using the default null-co driver,
> buf[] is uninitialized, the blk_pread() call succeeds and we
> then access uninitialized memory:
> 
>   static int guess_disk_lchs(BlockBackend *blk,
>                              int *pcylinders, int *pheads,
>                              int *psectors)
>   {
>       uint8_t buf[BDRV_SECTOR_SIZE];
>       ...
> 
>       if (blk_pread(blk, 0, buf, BDRV_SECTOR_SIZE) < 0) {
>           return -1;
>       }
>       /* test msdos magic */
>       if (buf[510] != 0x55 || buf[511] != 0xaa) {
>           return -1;
>       }
> 
> We could audit all the uninitialized buffers and the
> bdrv_co_preadv() handlers, but it is simpler to change the
> default of this testing driver. Performance tests will have
> to adapt and use 'null-co,read-zeroes=off'.
> 
> Suggested-by: Max Reitz <mreitz@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/null.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 0/2] block: Use 'read-zeroes=true' mode by default with 'null-co' driver

[Alexander Bulekov]

On 210211 1526, Philippe Mathieu-Daudé wrote:
> The null-co driver doesn't zeroize buffer in its default config,
> because it is designed for testing and tests want to run fast.
> However this confuses security researchers (access to uninit
> buffers).
> 

Interesting.. Is there an example bug report, where it raised alarms
because of an un-zeroed null-co:// buffer?
-Alex

> A one-line patch supposed which became a painful one, because
> there is so many different syntax to express the same usage:
> 
>  opt = qdict_new();
>  qdict_put_str(opt, "read-zeroes", "off");
>  null_bs = bdrv_open("null-co://", NULL, opt, BDRV_O_RDWR | BDRV_O_PROTOCOL,
>                      &error_abort);
> 
> vm.qmp('blockdev-add', driver='null-co', read_zeroes=False, ...)
> 
> vm.add_drive_raw("id=drive0,driver=null-co,read-zeroes=off,if=none")
> 
>     blk0 = { 'node-name': 'src',
>         'driver': 'null-co',
>         'read-zeroes': 'off' }
> 
>     'file': {
>         'driver': 'null-co',
>         'read-zeroes': False,
>     }
> 
>     "file": {
>         "driver": "null-co",
>         "read-zeroes": "off"
>     }
> 
>     { "execute": "blockdev-add",
>       "arguments": {
>           "driver": "null-co",
>           "read-zeroes": false,
>           "node-name": "disk0"
>         }
>     }
> 
> opts = {'driver': 'null-co,read-zeroes=off', 'node-name': 'root', 'size': 1024}
> 
> qemu -drive driver=null-co,read-zeroes=off
> 
> qemu-io ... "json:{'driver': 'null-co', 'read-zeroes': false, 'size': 65536}"
> 
> qemu-img null-co://,read-zeroes=off
> 
> qemu-img ... -o data_file="json:{'driver':'null-co',,'read-zeroes':false,,'size':'4294967296'}"
> 
> There are probably more.
> 
> Anyhow, the iotests I am not sure and should be audited are 056, 155
> (I don't understand the syntax) and 162.
> 
> Please review,
> 
> Phil.
> 
> Philippe Mathieu-Daud=C3=A9 (2):
>   block: Explicit null-co uses 'read-zeroes=3Dfalse'
>   block/null: Enable 'read-zeroes' mode by default
> 
>  docs/devel/testing.rst                     | 14 +++++++-------
>  tests/qtest/fuzz/generic_fuzz_configs.h    | 11 ++++++-----
>  block/null.c                               |  2 +-
>  tests/test-bdrv-drain.c                    | 10 ++++++++--
>  tests/acceptance/virtio_check_params.py    |  2 +-
>  tests/perf/block/qcow2/convert-blockstatus |  6 +++---
>  tests/qemu-iotests/040                     |  2 +-
>  tests/qemu-iotests/041                     | 12 ++++++++----
>  tests/qemu-iotests/051                     |  2 +-
>  tests/qemu-iotests/051.out                 |  2 +-
>  tests/qemu-iotests/051.pc.out              |  4 ++--
>  tests/qemu-iotests/087                     |  6 ++++--
>  tests/qemu-iotests/118                     |  2 +-
>  tests/qemu-iotests/133                     |  2 +-
>  tests/qemu-iotests/153                     |  8 ++++----
>  tests/qemu-iotests/184                     |  2 ++
>  tests/qemu-iotests/184.out                 | 10 +++++-----
>  tests/qemu-iotests/218                     |  3 +++
>  tests/qemu-iotests/224                     |  3 ++-
>  tests/qemu-iotests/224.out                 |  8 ++++----
>  tests/qemu-iotests/225                     |  2 +-
>  tests/qemu-iotests/227                     |  4 ++--
>  tests/qemu-iotests/227.out                 |  4 ++--
>  tests/qemu-iotests/228                     |  2 +-
>  tests/qemu-iotests/235                     |  1 +
>  tests/qemu-iotests/245                     |  2 +-
>  tests/qemu-iotests/270                     |  2 +-
>  tests/qemu-iotests/283                     |  3 ++-
>  tests/qemu-iotests/283.out                 |  4 ++--
>  tests/qemu-iotests/299                     |  1 +
>  tests/qemu-iotests/299.out                 |  2 +-
>  tests/qemu-iotests/300                     |  4 ++--
>  32 files changed, 82 insertions(+), 60 deletions(-)
> 
> --=20
> 2.26.2
> 
> 

Re: [PATCH v2 0/2] block: Use 'read-zeroes=true' mode by default with 'null-co' driver

[Philippe Mathieu-Daudé]

On 2/11/21 4:42 PM, Alexander Bulekov wrote:
> On 210211 1526, Philippe Mathieu-Daudé wrote:
>> The null-co driver doesn't zeroize buffer in its default config,
>> because it is designed for testing and tests want to run fast.
>> However this confuses security researchers (access to uninit
>> buffers).
>>
> 
> Interesting.. Is there an example bug report, where it raised alarms
> because of an un-zeroed null-co:// buffer?

No, but I found a similar mention here:
https://www.mail-archive.com/qemu-block@nongnu.org/msg52045.html

Example:

$ valgrind qemu-system-i386 -S -drive
file=null-co://,format=raw,file.read-zeroes=on

$ valgrind qemu-system-i386 -S -drive
file=null-co://,format=raw,file.read-zeroes=off
==4048219== Conditional jump or move depends on uninitialised value(s)
==4048219==    at 0x4E19CC: guess_disk_lchs (hd-geometry.c:70)
==4048219==    by 0x4E1C72: hd_geometry_guess (hd-geometry.c:131)
==4048219==    by 0x4E0F0F: blkconf_geometry (block.c:183)
==4048219==    by 0x563727: ide_dev_initfn (qdev.c:201)
==4048219==    by 0x563AE4: ide_hd_realize (qdev.c:278)
==4048219==    by 0x563320: ide_qdev_realize (qdev.c:124)
==4048219==    by 0x8F8EAA: device_set_realized (qdev.c:761)
==4048219==    by 0x902347: property_set_bool (object.c:2255)
==4048219==    by 0x900441: object_property_set (object.c:1400)
==4048219==    by 0x904467: object_property_set_qobject (qom-qobject.c:28)
==4048219==    by 0x9007A4: object_property_set_bool (object.c:1470)
==4048219==    by 0x8F7F3B: qdev_realize (qdev.c:389)

Re: [PATCH v2 0/2] block: Use 'read-zeroes=true' mode by default with 'null-co' driver

[Fam Zheng]

On 2021-02-11 15:26, Philippe Mathieu-Daudé wrote:
> The null-co driver doesn't zeroize buffer in its default config,
> because it is designed for testing and tests want to run fast.
> However this confuses security researchers (access to uninit
> buffers).

I'm a little surprised.

Is changing default the only way to fix this? I'm not opposed to
changing the default but I'm not convinced this is the easiest way.
block/nvme.c also doesn't touch the memory, but defers to the device
DMA, why doesn't that confuse the security checker?

Cannot we just somehow annotate it in a way that the checker can
understand (akin to how we provide coverity models) and be done?

Thanks,
Fam

> 
> A one-line patch supposed which became a painful one, because
> there is so many different syntax to express the same usage:
> 
>  opt = qdict_new();
>  qdict_put_str(opt, "read-zeroes", "off");
>  null_bs = bdrv_open("null-co://", NULL, opt, BDRV_O_RDWR | BDRV_O_PROTOCOL,
>                      &error_abort);
> 
> vm.qmp('blockdev-add', driver='null-co', read_zeroes=False, ...)
> 
> vm.add_drive_raw("id=drive0,driver=null-co,read-zeroes=off,if=none")
> 
>     blk0 = { 'node-name': 'src',
>         'driver': 'null-co',
>         'read-zeroes': 'off' }
> 
>     'file': {
>         'driver': 'null-co',
>         'read-zeroes': False,
>     }
> 
>     "file": {
>         "driver": "null-co",
>         "read-zeroes": "off"
>     }
> 
>     { "execute": "blockdev-add",
>       "arguments": {
>           "driver": "null-co",
>           "read-zeroes": false,
>           "node-name": "disk0"
>         }
>     }
> 
> opts = {'driver': 'null-co,read-zeroes=off', 'node-name': 'root', 'size': 1024}
> 
> qemu -drive driver=null-co,read-zeroes=off
> 
> qemu-io ... "json:{'driver': 'null-co', 'read-zeroes': false, 'size': 65536}"
> 
> qemu-img null-co://,read-zeroes=off
> 
> qemu-img ... -o data_file="json:{'driver':'null-co',,'read-zeroes':false,,'size':'4294967296'}"
> 
> There are probably more.
> 
> Anyhow, the iotests I am not sure and should be audited are 056, 155
> (I don't understand the syntax) and 162.
> 
> Please review,
> 
> Phil.
> 
> Philippe Mathieu-Daud=C3=A9 (2):
>   block: Explicit null-co uses 'read-zeroes=3Dfalse'
>   block/null: Enable 'read-zeroes' mode by default
> 
>  docs/devel/testing.rst                     | 14 +++++++-------
>  tests/qtest/fuzz/generic_fuzz_configs.h    | 11 ++++++-----
>  block/null.c                               |  2 +-
>  tests/test-bdrv-drain.c                    | 10 ++++++++--
>  tests/acceptance/virtio_check_params.py    |  2 +-
>  tests/perf/block/qcow2/convert-blockstatus |  6 +++---
>  tests/qemu-iotests/040                     |  2 +-
>  tests/qemu-iotests/041                     | 12 ++++++++----
>  tests/qemu-iotests/051                     |  2 +-
>  tests/qemu-iotests/051.out                 |  2 +-
>  tests/qemu-iotests/051.pc.out              |  4 ++--
>  tests/qemu-iotests/087                     |  6 ++++--
>  tests/qemu-iotests/118                     |  2 +-
>  tests/qemu-iotests/133                     |  2 +-
>  tests/qemu-iotests/153                     |  8 ++++----
>  tests/qemu-iotests/184                     |  2 ++
>  tests/qemu-iotests/184.out                 | 10 +++++-----
>  tests/qemu-iotests/218                     |  3 +++
>  tests/qemu-iotests/224                     |  3 ++-
>  tests/qemu-iotests/224.out                 |  8 ++++----
>  tests/qemu-iotests/225                     |  2 +-
>  tests/qemu-iotests/227                     |  4 ++--
>  tests/qemu-iotests/227.out                 |  4 ++--
>  tests/qemu-iotests/228                     |  2 +-
>  tests/qemu-iotests/235                     |  1 +
>  tests/qemu-iotests/245                     |  2 +-
>  tests/qemu-iotests/270                     |  2 +-
>  tests/qemu-iotests/283                     |  3 ++-
>  tests/qemu-iotests/283.out                 |  4 ++--
>  tests/qemu-iotests/299                     |  1 +
>  tests/qemu-iotests/299.out                 |  2 +-
>  tests/qemu-iotests/300                     |  4 ++--
>  32 files changed, 82 insertions(+), 60 deletions(-)
> 
> --=20
> 2.26.2
> 
> 
> 

Re: [PATCH v2 0/2] block: Use 'read-zeroes=true' mode by default with 'null-co' driver

[Max Reitz]

On 13.02.21 22:54, Fam Zheng wrote:
> On 2021-02-11 15:26, Philippe Mathieu-Daudé wrote:
>> The null-co driver doesn't zeroize buffer in its default config,
>> because it is designed for testing and tests want to run fast.
>> However this confuses security researchers (access to uninit
>> buffers).
> 
> I'm a little surprised.
> 
> Is changing default the only way to fix this? I'm not opposed to
> changing the default but I'm not convinced this is the easiest way.
> block/nvme.c also doesn't touch the memory, but defers to the device
> DMA, why doesn't that confuse the security checker?
> 
> Cannot we just somehow annotate it in a way that the checker can
> understand (akin to how we provide coverity models) and be done?

The question is, why wouldn’t we change the default?  read-zeroes=true 
seems the better default to me.  I consider silencing valgrind warnings 
and the like a nice side effect.

Max

Re: [PATCH v2 0/2] block: Use 'read-zeroes=true' mode by default with 'null-co' driver

[Philippe Mathieu-Daudé]

On 2/19/21 12:07 PM, Max Reitz wrote:
> On 13.02.21 22:54, Fam Zheng wrote:
>> On 2021-02-11 15:26, Philippe Mathieu-Daudé wrote:
>>> The null-co driver doesn't zeroize buffer in its default config,
>>> because it is designed for testing and tests want to run fast.
>>> However this confuses security researchers (access to uninit
>>> buffers).
>>
>> I'm a little surprised.
>>
>> Is changing default the only way to fix this? I'm not opposed to
>> changing the default but I'm not convinced this is the easiest way.
>> block/nvme.c also doesn't touch the memory, but defers to the device
>> DMA, why doesn't that confuse the security checker?

Generally speaking, there is a balance between security and performance.
We try to provide both, but when we can't, my understanding is security
is more important.

Customers expect a secure product. If they prefer performance and
at the price of security, it is also possible by enabling an option
that is not the default.

I'm not sure why you mention block/nvme here. I have the understanding
the null-co driver is only useful for testing. Are there production
cases where null-co is used?

BTW block/nvme is a particular driver where performance matters more
than security (but we have to make sure the users are aware of that).

>> Cannot we just somehow annotate it in a way that the checker can
>> understand (akin to how we provide coverity models) and be done?
> 
> The question is, why wouldn’t we change the default?  read-zeroes=true
> seems the better default to me.  I consider silencing valgrind warnings
> and the like a nice side effect.
> 
> Max
> 

Re: [PATCH v2 0/2] block: Use 'read-zeroes=true' mode by default with 'null-co' driver

[Fam Zheng]

On 2021-02-19 15:09, Philippe Mathieu-Daudé wrote:
> On 2/19/21 12:07 PM, Max Reitz wrote:
> > On 13.02.21 22:54, Fam Zheng wrote:
> >> On 2021-02-11 15:26, Philippe Mathieu-Daudé wrote:
> >>> The null-co driver doesn't zeroize buffer in its default config,
> >>> because it is designed for testing and tests want to run fast.
> >>> However this confuses security researchers (access to uninit
> >>> buffers).
> >>
> >> I'm a little surprised.
> >>
> >> Is changing default the only way to fix this? I'm not opposed to
> >> changing the default but I'm not convinced this is the easiest way.
> >> block/nvme.c also doesn't touch the memory, but defers to the device
> >> DMA, why doesn't that confuse the security checker?
> 
> Generally speaking, there is a balance between security and performance.
> We try to provide both, but when we can't, my understanding is security
> is more important.

Why is hiding the code path behind a non-default more secure? What is
not secure now?

Fam

Re: [PATCH v2 0/2] block: Use 'read-zeroes=true' mode by default with 'null-co' driver

[Philippe Mathieu-Daudé]

On 2/22/21 6:35 PM, Fam Zheng wrote:
> On 2021-02-19 15:09, Philippe Mathieu-Daudé wrote:
>> On 2/19/21 12:07 PM, Max Reitz wrote:
>>> On 13.02.21 22:54, Fam Zheng wrote:
>>>> On 2021-02-11 15:26, Philippe Mathieu-Daudé wrote:
>>>>> The null-co driver doesn't zeroize buffer in its default config,
>>>>> because it is designed for testing and tests want to run fast.
>>>>> However this confuses security researchers (access to uninit
>>>>> buffers).
>>>>
>>>> I'm a little surprised.
>>>>
>>>> Is changing default the only way to fix this? I'm not opposed to
>>>> changing the default but I'm not convinced this is the easiest way.
>>>> block/nvme.c also doesn't touch the memory, but defers to the device
>>>> DMA, why doesn't that confuse the security checker?
>>
>> Generally speaking, there is a balance between security and performance.
>> We try to provide both, but when we can't, my understanding is security
>> is more important.
> 
> Why is hiding the code path behind a non-default more secure? What is
> not secure now?

Se we are back to the problem of having default values.

I'd like to remove the default and have the option explicit,
but qemu_opt_get_bool() expects a 'default' value.

Should we rename qemu_opt_get_bool() -> qemu_opt_get_bool_with_default()
and add a simpler qemu_opt_get_bool()?

Re: [PATCH v2 0/2] block: Use 'read-zeroes=true' mode by default with 'null-co' driver

[Fam Zheng]

On 2021-02-22 18:55, Philippe Mathieu-Daudé wrote:
> On 2/22/21 6:35 PM, Fam Zheng wrote:
> > On 2021-02-19 15:09, Philippe Mathieu-Daudé wrote:
> >> On 2/19/21 12:07 PM, Max Reitz wrote:
> >>> On 13.02.21 22:54, Fam Zheng wrote:
> >>>> On 2021-02-11 15:26, Philippe Mathieu-Daudé wrote:
> >>>>> The null-co driver doesn't zeroize buffer in its default config,
> >>>>> because it is designed for testing and tests want to run fast.
> >>>>> However this confuses security researchers (access to uninit
> >>>>> buffers).
> >>>>
> >>>> I'm a little surprised.
> >>>>
> >>>> Is changing default the only way to fix this? I'm not opposed to
> >>>> changing the default but I'm not convinced this is the easiest way.
> >>>> block/nvme.c also doesn't touch the memory, but defers to the device
> >>>> DMA, why doesn't that confuse the security checker?
> >>
> >> Generally speaking, there is a balance between security and performance.
> >> We try to provide both, but when we can't, my understanding is security
> >> is more important.
> > 
> > Why is hiding the code path behind a non-default more secure? What is
> > not secure now?
> 
> Se we are back to the problem of having default values.
> 
> I'd like to remove the default and have the option explicit,
> but qemu_opt_get_bool() expects a 'default' value.
> 
> Should we rename qemu_opt_get_bool() -> qemu_opt_get_bool_with_default()
> and add a simpler qemu_opt_get_bool()?

My point is I still don't get the full context of the problem this
series is trying to solve. If the problem is tools are confused, I would
like to understand why. What is the thing that matters here, exactly?

But there's always been nullblk.ko in kernel that doesn't lie in the
name. If we change the default we are not very honest any more about
what the driver actually does.

Even if null-co:// and null-aio:// is a bad idea, then let's add
zero-co://co and zero-aio://, and deprecate null-*://.

Fam

Re: [PATCH v2 0/2] block: Use 'read-zeroes=true' mode by default with 'null-co' driver

[Max Reitz]

On 23.02.21 10:21, Fam Zheng wrote:
> On 2021-02-22 18:55, Philippe Mathieu-Daudé wrote:
>> On 2/22/21 6:35 PM, Fam Zheng wrote:
>>> On 2021-02-19 15:09, Philippe Mathieu-Daudé wrote:
>>>> On 2/19/21 12:07 PM, Max Reitz wrote:
>>>>> On 13.02.21 22:54, Fam Zheng wrote:
>>>>>> On 2021-02-11 15:26, Philippe Mathieu-Daudé wrote:
>>>>>>> The null-co driver doesn't zeroize buffer in its default config,
>>>>>>> because it is designed for testing and tests want to run fast.
>>>>>>> However this confuses security researchers (access to uninit
>>>>>>> buffers).
>>>>>>
>>>>>> I'm a little surprised.
>>>>>>
>>>>>> Is changing default the only way to fix this? I'm not opposed to
>>>>>> changing the default but I'm not convinced this is the easiest way.
>>>>>> block/nvme.c also doesn't touch the memory, but defers to the device
>>>>>> DMA, why doesn't that confuse the security checker?
>>>>
>>>> Generally speaking, there is a balance between security and performance.
>>>> We try to provide both, but when we can't, my understanding is security
>>>> is more important.
>>>
>>> Why is hiding the code path behind a non-default more secure? What is
>>> not secure now?
>>
>> Se we are back to the problem of having default values.
>>
>> I'd like to remove the default and have the option explicit,
>> but qemu_opt_get_bool() expects a 'default' value.
>>
>> Should we rename qemu_opt_get_bool() -> qemu_opt_get_bool_with_default()
>> and add a simpler qemu_opt_get_bool()?
> 
> My point is I still don't get the full context of the problem this
> series is trying to solve. If the problem is tools are confused, I would
> like to understand why. What is the thing that matters here, exactly?

My personal opinion is that it isn’t even about tools, it’s just about 
the fact that operating on uninitialized data is something that should 
generally be avoided.  For the null drivers, there is a reason to allow 
it (performance testing), but that should be a special case, not the 
default.

> But there's always been nullblk.ko in kernel that doesn't lie in the
> name. If we change the default we are not very honest any more about
> what the driver actually does.

Then we’re already lying, because if we model it after /dev/null, we 
should probably return -EIO on every read.

If a null device returns data, that data may be arbitrary, so we might 
as well memset() it to 0.  As I wrote in my reply to Daniel, I find it 
perfectly reasonable to make that the default: For functional tests 
(which I think are the majority of null’s users), it doesn’t make a 
difference except that operating on uninitialized data just isn’t a nice 
thing to do.

The only reasons I can see we wouldn’t change the default are (1) 
compatibility, which I don’t think is an issue for a test driver (plus, 
the only thing it might break are performance tests, which naively I 
think wouldn’t be a problem), and (2) it’s an additional gotcha when 
performance testing, but there are usually so many gotchas with 
performance testing, that I don’t see this as a problem either.

> Even if null-co:// and null-aio:// is a bad idea, then let's add
> zero-co://co and zero-aio://, and deprecate null-*://.
I find that too much work simply because it’s more work than just making 
read-zeroes=on the default, and I find doing that reasonable.

(Furthermore, we wouldn’t deprecate null-*, because it’s needed for 
performance testing.  We could add read-zeroes as an option to the new 
zero-* drivers, but I find that silly.)

Max

Re: [PATCH v2 0/2] block: Use 'read-zeroes=true' mode by default with 'null-co' driver

[Fam Zheng]

On 2021-02-23 17:01, Max Reitz wrote:
> On 23.02.21 10:21, Fam Zheng wrote:
> > On 2021-02-22 18:55, Philippe Mathieu-Daudé wrote:
> > > On 2/22/21 6:35 PM, Fam Zheng wrote:
> > > > On 2021-02-19 15:09, Philippe Mathieu-Daudé wrote:
> > > > > On 2/19/21 12:07 PM, Max Reitz wrote:
> > > > > > On 13.02.21 22:54, Fam Zheng wrote:
> > > > > > > On 2021-02-11 15:26, Philippe Mathieu-Daudé wrote:
> > > > > > > > The null-co driver doesn't zeroize buffer in its default config,
> > > > > > > > because it is designed for testing and tests want to run fast.
> > > > > > > > However this confuses security researchers (access to uninit
> > > > > > > > buffers).
> > > > > > > 
> > > > > > > I'm a little surprised.
> > > > > > > 
> > > > > > > Is changing default the only way to fix this? I'm not opposed to
> > > > > > > changing the default but I'm not convinced this is the easiest way.
> > > > > > > block/nvme.c also doesn't touch the memory, but defers to the device
> > > > > > > DMA, why doesn't that confuse the security checker?
> > > > > 
> > > > > Generally speaking, there is a balance between security and performance.
> > > > > We try to provide both, but when we can't, my understanding is security
> > > > > is more important.
> > > > 
> > > > Why is hiding the code path behind a non-default more secure? What is
> > > > not secure now?
> > > 
> > > Se we are back to the problem of having default values.
> > > 
> > > I'd like to remove the default and have the option explicit,
> > > but qemu_opt_get_bool() expects a 'default' value.
> > > 
> > > Should we rename qemu_opt_get_bool() -> qemu_opt_get_bool_with_default()
> > > and add a simpler qemu_opt_get_bool()?
> > 
> > My point is I still don't get the full context of the problem this
> > series is trying to solve. If the problem is tools are confused, I would
> > like to understand why. What is the thing that matters here, exactly?
> 
> My personal opinion is that it isn’t even about tools, it’s just about the
> fact that operating on uninitialized data is something that should generally
> be avoided.  For the null drivers, there is a reason to allow it
> (performance testing), but that should be a special case, not the default.

How do we define uninitialized data? Are buffers passed to a successful
read (2) syscall initialized? We cannot know, because it's up to the
fs/driver/storage. It's the same to bdrv_pread().

In fact block/null.c doesn't operate on uninitialized data, we should
really fix guess_disk_lchs() and similar.

> 
> > But there's always been nullblk.ko in kernel that doesn't lie in the
> > name. If we change the default we are not very honest any more about
> > what the driver actually does.
> 
> Then we’re already lying, because if we model it after /dev/null, we should
> probably return -EIO on every read.

nullblk.ko is not /dev/null, it's /dev/nullb*:

https://www.kernel.org/doc/Documentation/block/null_blk.txt

Fam

Re: [PATCH v2 0/2] block: Use 'read-zeroes=true' mode by default with 'null-co' driver

[Daniel P. Berrangé]

On Fri, Feb 19, 2021 at 03:09:43PM +0100, Philippe Mathieu-Daudé wrote:
> On 2/19/21 12:07 PM, Max Reitz wrote:
> > On 13.02.21 22:54, Fam Zheng wrote:
> >> On 2021-02-11 15:26, Philippe Mathieu-Daudé wrote:
> >>> The null-co driver doesn't zeroize buffer in its default config,
> >>> because it is designed for testing and tests want to run fast.
> >>> However this confuses security researchers (access to uninit
> >>> buffers).
> >>
> >> I'm a little surprised.
> >>
> >> Is changing default the only way to fix this? I'm not opposed to
> >> changing the default but I'm not convinced this is the easiest way.
> >> block/nvme.c also doesn't touch the memory, but defers to the device
> >> DMA, why doesn't that confuse the security checker?
> 
> Generally speaking, there is a balance between security and performance.
> We try to provide both, but when we can't, my understanding is security
> is more important.
> 
> Customers expect a secure product. If they prefer performance and
> at the price of security, it is also possible by enabling an option
> that is not the default.
> 
> I'm not sure why you mention block/nvme here. I have the understanding
> the null-co driver is only useful for testing. Are there production
> cases where null-co is used?

Do we have any real world figures for the performance of null-co
with & without  zero'ing ?  Before worrying about a tradeoff of
security vs performance, it'd be good to know if there is actually
a real world performance problem in the first place. Personally I'd
go for zero'ing by defualt unless the performance hit was really
bad.

> BTW block/nvme is a particular driver where performance matters more
> than security (but we have to make sure the users are aware of that).


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 0/2] block: Use 'read-zeroes=true' mode by default with 'null-co' driver

[Philippe Mathieu-Daudé]

On 2/22/21 7:15 PM, Daniel P. Berrangé wrote:
> On Fri, Feb 19, 2021 at 03:09:43PM +0100, Philippe Mathieu-Daudé wrote:
>> On 2/19/21 12:07 PM, Max Reitz wrote:
>>> On 13.02.21 22:54, Fam Zheng wrote:
>>>> On 2021-02-11 15:26, Philippe Mathieu-Daudé wrote:
>>>>> The null-co driver doesn't zeroize buffer in its default config,
>>>>> because it is designed for testing and tests want to run fast.
>>>>> However this confuses security researchers (access to uninit
>>>>> buffers).
>>>>
>>>> I'm a little surprised.
>>>>
>>>> Is changing default the only way to fix this? I'm not opposed to
>>>> changing the default but I'm not convinced this is the easiest way.
>>>> block/nvme.c also doesn't touch the memory, but defers to the device
>>>> DMA, why doesn't that confuse the security checker?
>>
>> Generally speaking, there is a balance between security and performance.
>> We try to provide both, but when we can't, my understanding is security
>> is more important.
>>
>> Customers expect a secure product. If they prefer performance and
>> at the price of security, it is also possible by enabling an option
>> that is not the default.
>>
>> I'm not sure why you mention block/nvme here. I have the understanding
>> the null-co driver is only useful for testing. Are there production
>> cases where null-co is used?
> 
> Do we have any real world figures for the performance of null-co
> with & without  zero'ing ?  Before worrying about a tradeoff of
> security vs performance, it'd be good to know if there is actually
> a real world performance problem in the first place. Personally I'd
> go for zero'ing by defualt unless the performance hit was really
> bad.

I simply wanted to help the security team by removing the
amount of reports using the null-co driver. This is probably
easier to resolve with one documentation line.
As I am not understanding where this thread is heading, I am
taking a step back with this topic. Please disregard this series.

Regards,

Phil.

Re: [PATCH v2 0/2] block: Use 'read-zeroes=true' mode by default with 'null-co' driver

[Max Reitz]

On 22.02.21 19:15, Daniel P. Berrangé wrote:
> On Fri, Feb 19, 2021 at 03:09:43PM +0100, Philippe Mathieu-Daudé wrote:
>> On 2/19/21 12:07 PM, Max Reitz wrote:
>>> On 13.02.21 22:54, Fam Zheng wrote:
>>>> On 2021-02-11 15:26, Philippe Mathieu-Daudé wrote:
>>>>> The null-co driver doesn't zeroize buffer in its default config,
>>>>> because it is designed for testing and tests want to run fast.
>>>>> However this confuses security researchers (access to uninit
>>>>> buffers).
>>>>
>>>> I'm a little surprised.
>>>>
>>>> Is changing default the only way to fix this? I'm not opposed to
>>>> changing the default but I'm not convinced this is the easiest way.
>>>> block/nvme.c also doesn't touch the memory, but defers to the device
>>>> DMA, why doesn't that confuse the security checker?
>>
>> Generally speaking, there is a balance between security and performance.
>> We try to provide both, but when we can't, my understanding is security
>> is more important.
>>
>> Customers expect a secure product. If they prefer performance and
>> at the price of security, it is also possible by enabling an option
>> that is not the default.
>>
>> I'm not sure why you mention block/nvme here. I have the understanding
>> the null-co driver is only useful for testing. Are there production
>> cases where null-co is used?
> 
> Do we have any real world figures for the performance of null-co
> with & without  zero'ing ?  Before worrying about a tradeoff of
> security vs performance, it'd be good to know if there is actually
> a real world performance problem in the first place. Personally I'd
> go for zero'ing by defualt unless the performance hit was really
> bad.

AFAIU, null-co is only used for testing, be it to just create some block 
nodes in the iotests, or perhaps for performance testing where you want 
to get the minimal roundtrip time through the block layer.  So there is 
no "real world performance problem", because there is no real world use 
of null-co or null-aio.  At least there shouldn’t be.

That begs the question of whether read-zeroes=off even makes sense, and 
I think it absolutely does.

In cases where we have a test that just wants a simple block node that 
doesn’t use disk space, the memset() can’t be noticeable.  But it’s just 
a test, so do we even need the memset()?  Strictly speaking, perhaps 
not, but if someone is to run it via Valgrind or something, they may get 
false positives, so just doing the memset() is the right thing to do.

For performance tests, it must be possible to set read-zeroes=off, 
because even though “that memset() isn’t noticeable in a functional 
test”, in a hard-core performance test, it will be.

So we need a switch.  It should default to memset(), because (1) making 
tools like Valgrind happy seems like a reasonable objective to me, and 
(2) in the majority of cases, the memset() cannot have a noticeable impact.

Max

Re: [PATCH v2 0/2] block: Use 'read-zeroes=true' mode by default with 'null-co' driver

[Daniel P. Berrangé]

On Tue, Feb 23, 2021 at 09:44:51AM +0100, Max Reitz wrote:
> On 22.02.21 19:15, Daniel P. Berrangé wrote:
> > On Fri, Feb 19, 2021 at 03:09:43PM +0100, Philippe Mathieu-Daudé wrote:
> > > On 2/19/21 12:07 PM, Max Reitz wrote:
> > > > On 13.02.21 22:54, Fam Zheng wrote:
> > > > > On 2021-02-11 15:26, Philippe Mathieu-Daudé wrote:
> > > > > > The null-co driver doesn't zeroize buffer in its default config,
> > > > > > because it is designed for testing and tests want to run fast.
> > > > > > However this confuses security researchers (access to uninit
> > > > > > buffers).
> > > > > 
> > > > > I'm a little surprised.
> > > > > 
> > > > > Is changing default the only way to fix this? I'm not opposed to
> > > > > changing the default but I'm not convinced this is the easiest way.
> > > > > block/nvme.c also doesn't touch the memory, but defers to the device
> > > > > DMA, why doesn't that confuse the security checker?
> > > 
> > > Generally speaking, there is a balance between security and performance.
> > > We try to provide both, but when we can't, my understanding is security
> > > is more important.
> > > 
> > > Customers expect a secure product. If they prefer performance and
> > > at the price of security, it is also possible by enabling an option
> > > that is not the default.
> > > 
> > > I'm not sure why you mention block/nvme here. I have the understanding
> > > the null-co driver is only useful for testing. Are there production
> > > cases where null-co is used?
> > 
> > Do we have any real world figures for the performance of null-co
> > with & without  zero'ing ?  Before worrying about a tradeoff of
> > security vs performance, it'd be good to know if there is actually
> > a real world performance problem in the first place. Personally I'd
> > go for zero'ing by defualt unless the performance hit was really
> > bad.
> 
> AFAIU, null-co is only used for testing, be it to just create some block
> nodes in the iotests, or perhaps for performance testing where you want to
> get the minimal roundtrip time through the block layer.  So there is no
> "real world performance problem", because there is no real world use of
> null-co or null-aio.  At least there shouldn’t be.
> 
> That begs the question of whether read-zeroes=off even makes sense, and I
> think it absolutely does.
> 
> In cases where we have a test that just wants a simple block node that
> doesn’t use disk space, the memset() can’t be noticeable.  But it’s just a
> test, so do we even need the memset()?  Strictly speaking, perhaps not, but
> if someone is to run it via Valgrind or something, they may get false
> positives, so just doing the memset() is the right thing to do.
> 
> For performance tests, it must be possible to set read-zeroes=off, because
> even though “that memset() isn’t noticeable in a functional test”, in a
> hard-core performance test, it will be.
> 
> So we need a switch.  It should default to memset(), because (1) making
> tools like Valgrind happy seems like a reasonable objective to me, and (2)
> in the majority of cases, the memset() cannot have a noticeable impact.

Yes, that all makes sense to me.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|