[Qemu-block] [PATCH v15 18/21] block: Reuse bs as backing hd for drive-backup sync=none

[Fam Zheng]

Signed-off-by: Fam Zheng 
---
 blockdev.c | 15 ++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index 4927914..4e04dec 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3174,6 +3174,7 @@ static BlockJob *do_drive_backup(DriveBackup *backup, 
BlockJobTxn *txn,
 Error *local_err = NULL;
 int flags;
 int64_t size;
+bool set_backing_hd = false;
 
 if (!backup->has_speed) {
 backup->speed = 0;
@@ -3224,6 +3225,8 @@ static BlockJob *do_drive_backup(DriveBackup *backup, 
BlockJobTxn *txn,
 }
 if (backup->sync == MIRROR_SYNC_MODE_NONE) {
 source = bs;
+flags |= BDRV_O_NO_BACKING;
+set_backing_hd = true;
 }
 
 size = bdrv_getlength(bs);
@@ -3250,7 +3253,9 @@ static BlockJob *do_drive_backup(DriveBackup *backup, 
BlockJobTxn *txn,
 }
 
 if (backup->format) {
-options = qdict_new();
+if (!options) {
+options = qdict_new();
+}
 qdict_put(options, "driver", qstring_from_str(backup->format));
 }
 
@@ -3261,6 +3266,14 @@ static BlockJob *do_drive_backup(DriveBackup *backup, 
BlockJobTxn *txn,
 
 bdrv_set_aio_context(target_bs, aio_context);
 
+if (set_backing_hd) {
+bdrv_set_backing_hd(target_bs, source, &local_err);
+if (local_err) {
+bdrv_unref(target_bs);
+goto out;
+}
+}
+
 if (backup->has_bitmap) {
 bmap = bdrv_find_dirty_bitmap(bs, backup->bitmap);
 if (!bmap) {
-- 
2.9.3

[Qemu-block] [PATCH v15 15/21] tests: Use null-co:// instead of /dev/null as the dummy image

[Fam Zheng]

Signed-off-by: Fam Zheng 
Reviewed-by: Max Reitz 
---
 tests/drive_del-test.c| 2 +-
 tests/nvme-test.c | 2 +-
 tests/usb-hcd-uhci-test.c | 2 +-
 tests/usb-hcd-xhci-test.c | 2 +-
 tests/virtio-blk-test.c   | 2 +-
 tests/virtio-scsi-test.c  | 5 +++--
 6 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/tests/drive_del-test.c b/tests/drive_del-test.c
index 121b9c9..2175139 100644
--- a/tests/drive_del-test.c
+++ b/tests/drive_del-test.c
@@ -92,7 +92,7 @@ static void test_after_failed_device_add(void)
 static void test_drive_del_device_del(void)
 {
 /* Start with a drive used by a device that unplugs instantaneously */
-qtest_start("-drive if=none,id=drive0,file=/dev/null,format=raw"
+qtest_start("-drive if=none,id=drive0,file=null-co://,format=raw"
 " -device virtio-scsi-pci"
 " -device scsi-hd,drive=drive0,id=dev0");
 
diff --git a/tests/nvme-test.c b/tests/nvme-test.c
index c8bece4..7674a44 100644
--- a/tests/nvme-test.c
+++ b/tests/nvme-test.c
@@ -22,7 +22,7 @@ int main(int argc, char **argv)
 g_test_init(&argc, &argv, NULL);
 qtest_add_func("/nvme/nop", nop);
 
-qtest_start("-drive id=drv0,if=none,file=/dev/null,format=raw "
+qtest_start("-drive id=drv0,if=none,file=null-co://,format=raw "
 "-device nvme,drive=drv0,serial=foo");
 ret = g_test_run();
 
diff --git a/tests/usb-hcd-uhci-test.c b/tests/usb-hcd-uhci-test.c
index f25bae5..5b500fe 100644
--- a/tests/usb-hcd-uhci-test.c
+++ b/tests/usb-hcd-uhci-test.c
@@ -79,7 +79,7 @@ int main(int argc, char **argv)
 {
 const char *arch = qtest_get_arch();
 const char *cmd = "-device piix3-usb-uhci,id=uhci,addr=1d.0"
-  " -drive id=drive0,if=none,file=/dev/null,format=raw"
+  " -drive id=drive0,if=none,file=null-co://,format=raw"
   " -device usb-tablet,bus=uhci.0,port=1";
 int ret;
 
diff --git a/tests/usb-hcd-xhci-test.c b/tests/usb-hcd-xhci-test.c
index 22513e9..031764d 100644
--- a/tests/usb-hcd-xhci-test.c
+++ b/tests/usb-hcd-xhci-test.c
@@ -89,7 +89,7 @@ int main(int argc, char **argv)
 qtest_add_func("/xhci/pci/hotplug/usb-uas", test_usb_uas_hotplug);
 
 qtest_start("-device nec-usb-xhci,id=xhci"
-" -drive id=drive0,if=none,file=/dev/null,format=raw");
+" -drive id=drive0,if=none,file=null-co://,format=raw");
 ret = g_test_run();
 qtest_end();
 
diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
index 1eee95d..fd2078c 100644
--- a/tests/virtio-blk-test.c
+++ b/tests/virtio-blk-test.c
@@ -63,7 +63,7 @@ static QOSState *pci_test_start(void)
 const char *arch = qtest_get_arch();
 char *tmp_path;
 const char *cmd = "-drive if=none,id=drive0,file=%s,format=raw "
-  "-drive if=none,id=drive1,file=/dev/null,format=raw "
+  "-drive if=none,id=drive1,file=null-co://,format=raw "
   "-device virtio-blk-pci,id=drv0,drive=drive0,"
   "addr=%x.%x";
 
diff --git a/tests/virtio-scsi-test.c b/tests/virtio-scsi-test.c
index 0eabd56..8b0f77a 100644
--- a/tests/virtio-scsi-test.c
+++ b/tests/virtio-scsi-test.c
@@ -35,7 +35,7 @@ typedef struct {
 static QOSState *qvirtio_scsi_start(const char *extra_opts)
 {
 const char *arch = qtest_get_arch();
-const char *cmd = "-drive id=drv0,if=none,file=/dev/null,format=raw "
+const char *cmd = "-drive id=drv0,if=none,file=null-co://,format=raw "
   "-device virtio-scsi-pci,id=vs0 "
   "-device scsi-hd,bus=vs0.0,drive=drv0 %s";
 
@@ -195,7 +195,8 @@ static void hotplug(void)
 QDict *response;
 QOSState *qs;
 
-qs = qvirtio_scsi_start("-drive 
id=drv1,if=none,file=/dev/null,format=raw");
+qs = qvirtio_scsi_start(
+"-drive id=drv1,if=none,file=null-co://,format=raw");
 response = qmp("{\"execute\": \"device_add\","
" \"arguments\": {"
"   \"driver\": \"scsi-hd\","
-- 
2.9.3

[Qemu-block] [PATCH v15 20/21] file-posix: Add image locking to perm operations

[Fam Zheng]

This extends the permission bits of op blocker API to external using
Linux OFD locks.

Each permission in @perm and @shared_perm is represented by a locked
byte in the image file.  Requesting a permission in @perm is translated
to a shared lock of the corresponding byte; rejecting to share the same
permission is translated to a shared lock of a separate byte. With that,
we use 2x number of bytes of distinct permission types.

virtlockd in libvirt locks the first byte, so we do locking from a
higher offset.

Suggested-by: Kevin Wolf 
Signed-off-by: Fam Zheng 
---
 block/file-posix.c | 267 -
 1 file changed, 264 insertions(+), 3 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 2114720..b92fdc3 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -129,12 +129,28 @@ do { \
 
 #define MAX_BLOCKSIZE  4096
 
+/* Posix file locking bytes. Libvirt takes byte 0, we start from higher bytes,
+ * leaving a few more bytes for its future use. */
+#define RAW_LOCK_PERM_BASE 100
+#define RAW_LOCK_SHARED_BASE   200
+#ifdef F_OFD_SETLK
+#define RAW_LOCK_SUPPORTED 1
+#else
+#define RAW_LOCK_SUPPORTED 0
+#endif
+
 typedef struct BDRVRawState {
 int fd;
+int lock_fd;
+bool use_lock;
 int type;
 int open_flags;
 size_t buf_align;
 
+/* The current permissions. */
+uint64_t perm;
+uint64_t shared_perm;
+
 #ifdef CONFIG_XFS
 bool is_xfs:1;
 #endif
@@ -440,6 +456,8 @@ static int raw_open_common(BlockDriverState *bs, QDict 
*options,
 }
 s->use_linux_aio = (aio == BLOCKDEV_AIO_OPTIONS_NATIVE);
 
+s->use_lock = qemu_opt_get_bool(opts, "locking", true);
+
 s->open_flags = open_flags;
 raw_parse_flags(bdrv_flags, &s->open_flags);
 
@@ -455,6 +473,21 @@ static int raw_open_common(BlockDriverState *bs, QDict 
*options,
 }
 s->fd = fd;
 
+s->lock_fd = -1;
+fd = qemu_open(filename, O_RDONLY);
+if (fd < 0) {
+if (RAW_LOCK_SUPPORTED) {
+ret = -errno;
+error_setg_errno(errp, errno, "Could not open '%s' for locking",
+ filename);
+qemu_close(s->fd);
+goto fail;
+}
+}
+s->lock_fd = fd;
+s->perm = 0;
+s->shared_perm = BLK_PERM_ALL;
+
 #ifdef CONFIG_LINUX_AIO
  /* Currently Linux does AIO only for files opened with O_DIRECT */
 if (s->use_linux_aio && !(s->open_flags & O_DIRECT)) {
@@ -542,6 +575,156 @@ static int raw_open(BlockDriverState *bs, QDict *options, 
int flags,
 return raw_open_common(bs, options, flags, 0, errp);
 }
 
+typedef enum {
+RAW_PL_PREPARE,
+RAW_PL_COMMIT,
+RAW_PL_ABORT,
+} RawPermLockOp;
+
+/* Lock wanted bytes by @perm and ~@shared_perm in the file; if @unlock ==
+ * true, also unlock the unneeded bytes. */
+static int raw_apply_lock_bytes(BDRVRawState *s,
+uint64_t perm_lock_bits,
+uint64_t shared_perm_lock_bits,
+bool unlock, Error **errp)
+{
+int ret;
+int i;
+
+for (i = 0; i < BLK_PERM_MAX; ++i) {
+int off = RAW_LOCK_PERM_BASE + i;
+if (perm_lock_bits & (1ULL << i)) {
+ret = qemu_lock_fd(s->lock_fd, off, 1, false);
+if (ret) {
+error_setg(errp, "Failed to lock byte %d", off);
+return ret;
+}
+} else if (unlock) {
+ret = qemu_unlock_fd(s->lock_fd, off, 1);
+if (ret) {
+error_setg(errp, "Failed to unlock byte %d", off);
+return ret;
+}
+}
+}
+for (i = 0; i < BLK_PERM_MAX; ++i) {
+int off = RAW_LOCK_SHARED_BASE + i;
+if (shared_perm_lock_bits & (1ULL << i)) {
+ret = qemu_lock_fd(s->lock_fd, off, 1, false);
+if (ret) {
+error_setg(errp, "Failed to lock byte %d", off);
+return ret;
+}
+} else if (unlock) {
+ret = qemu_unlock_fd(s->lock_fd, off, 1);
+if (ret) {
+error_setg(errp, "Failed to unlock byte %d", off);
+return ret;
+}
+}
+}
+return 0;
+}
+
+/* Check "unshared" bytes implied by @perm and ~@shared_perm in the file. */
+static int raw_check_lock_bytes(BDRVRawState *s,
+uint64_t perm, uint64_t shared_perm,
+Error **errp)
+{
+int ret;
+int i;
+
+for (i = 0; i < BLK_PERM_MAX; ++i) {
+int off = RAW_LOCK_SHARED_BASE + i;
+uint64_t p = 1ULL << i;
+if (perm & p) {
+ret = qemu_lock_fd_test(s->lock_fd, off, 1, true);
+if (ret) {
+error_setg(errp,
+   "Failed to check byte %d for \"%s\" permission",
+   off, bdrv_perm_names(p));
+error_append_hint(errp,
+ 

[Qemu-block] [PATCH v15 21/21] qemu-iotests: Add test case 153 for image locking

[Fam Zheng]

Signed-off-by: Fam Zheng 
---
 tests/qemu-iotests/153 | 220 +
 tests/qemu-iotests/153.out | 390 +
 tests/qemu-iotests/group   |   1 +
 3 files changed, 611 insertions(+)
 create mode 100755 tests/qemu-iotests/153
 create mode 100644 tests/qemu-iotests/153.out

diff --git a/tests/qemu-iotests/153 b/tests/qemu-iotests/153
new file mode 100755
index 000..7501efa
--- /dev/null
+++ b/tests/qemu-iotests/153
@@ -0,0 +1,220 @@
+#!/bin/bash
+#
+# Test image locking
+#
+# Copyright 2016, 2017 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+# creator
+owner=f...@redhat.com
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+here="$PWD"
+tmp=/tmp/$$
+status=1   # failure is the default!
+
+_cleanup()
+{
+_cleanup_test_img
+rm -f "${TEST_IMG}.base"
+rm -f "${TEST_IMG}.convert"
+rm -f "${TEST_IMG}.a"
+rm -f "${TEST_IMG}.b"
+rm -f "${TEST_IMG}.lnk"
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.qemu
+
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+size=32M
+
+_run_cmd()
+{
+echo
+(echo "$@"; "$@" 2>&1 1>/dev/null) | _filter_testdir
+}
+
+function _do_run_qemu()
+{
+(
+if ! test -t 0; then
+while read cmd; do
+echo $cmd
+done
+fi
+echo quit
+) | $QEMU -nographic -monitor stdio -serial none "$@" 1>/dev/null
+}
+
+function _run_qemu_with_images()
+{
+_do_run_qemu \
+$(for i in $@; do echo "-drive if=none,file=$i"; done) 2>&1 \
+| _filter_testdir | _filter_qemu
+}
+
+echo "== readonly=off,force-share=on should be rejected =="
+_run_qemu_with_images null-co://,readonly=off,force-share=on
+
+for opts1 in "" "read-only=on" "read-only=on,force-share=on"; do
+echo
+echo "== Creating base image =="
+TEST_IMG="${TEST_IMG}.base" _make_test_img $size
+
+echo
+echo "== Creating test image =="
+$QEMU_IMG create -f $IMGFMT "${TEST_IMG}" -b ${TEST_IMG}.base | 
_filter_img_create
+
+echo
+echo "== Launching QEMU, opts: '$opts1' =="
+_launch_qemu -drive file="${TEST_IMG}",if=none,$opts1
+h=$QEMU_HANDLE
+
+for opts2 in "" "read-only=on" "read-only=on,force-share=on"; do
+echo
+echo "== Launching another QEMU, opts: '$opts2' =="
+echo "quit" | \
+$QEMU -nographic -monitor stdio \
+-drive file="${TEST_IMG}",if=none,$opts2 2>&1 1>/dev/null | \
+_filter_testdir | _filter_qemu
+done
+
+for L in "" "-U"; do
+
+echo
+echo "== Running utility commands $L =="
+_run_cmd $QEMU_IO $L -c "read 0 512" "${TEST_IMG}"
+_run_cmd $QEMU_IO $L -r -c "read 0 512" "${TEST_IMG}"
+_run_cmd $QEMU_IO -c "open $L ${TEST_IMG}" -c "read 0 512"
+_run_cmd $QEMU_IO -c "open -r $L ${TEST_IMG}" -c "read 0 512"
+_run_cmd $QEMU_IMG info$L "${TEST_IMG}"
+_run_cmd $QEMU_IMG check   $L "${TEST_IMG}"
+_run_cmd $QEMU_IMG compare $L "${TEST_IMG}" "${TEST_IMG}"
+_run_cmd $QEMU_IMG map $L "${TEST_IMG}"
+_run_cmd $QEMU_IMG amend -o "" $L "${TEST_IMG}"
+_run_cmd $QEMU_IMG commit  $L "${TEST_IMG}"
+_run_cmd $QEMU_IMG resize  $L "${TEST_IMG}" $size
+_run_cmd $QEMU_IMG rebase  $L "${TEST_IMG}" -b "${TEST_IMG}.base"
+_run_cmd $QEMU_IMG snapshot -l $L "${TEST_IMG}"
+_run_cmd $QEMU_IMG convert $L "${TEST_IMG}" "${TEST_IMG}.convert"
+_run_cmd $QEMU_IMG dd  $L if="${TEST_IMG}" 
of="${TEST_IMG}.convert" bs=512 count=1
+_run_cmd $QEMU_IMG bench   $L -c 1 "${TEST_IMG}"
+_run_cmd $QEMU_IMG bench   $L -w -c 1 "${TEST_IMG}"
+done
+_send_qemu_cmd $h "{ 'execute': 'quit', }" ""
+echo
+echo "Round done"
+_cleanup_qemu
+done
+
+for opt1 in $test_opts; do
+for opt2 in $test_opts; do
+echo
+echo "== Two devices with the same image ($opt1 - $opt2) =="
+_run_qemu_with_images "${TEST_IMG},$opt1" "${TEST_IMG},$opt2"
+done
+done
+
+echo "== Creating ${TEST_IMG}.[abc] =="
+(
+$QEMU_IMG create -f qcow2 "${TEST_IMG}.a" -b "${TEST_IMG}"
+$QEMU_IMG create -f qcow2 "${TEST_IMG}.b" -b "${TEST_IMG}"
+$QEMU_IMG

[Qemu-block] [PATCH v15 13/21] iotests: 091: Quit QEMU before checking image

[Fam Zheng]

Signed-off-by: Fam Zheng 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/091 | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/qemu-iotests/091 b/tests/qemu-iotests/091
index 32bbd56..10ac4a8 100755
--- a/tests/qemu-iotests/091
+++ b/tests/qemu-iotests/091
@@ -95,7 +95,9 @@ echo "vm2: qemu process running successfully"
 echo "vm2: flush io, and quit"
 _send_qemu_cmd $h2 'qemu-io disk flush' "(qemu)"
 _send_qemu_cmd $h2 'quit' ""
+_send_qemu_cmd $h1 'quit' ""
 
+wait
 echo "Check image pattern"
 ${QEMU_IO} -c "read -P 0x22 0 4M" "${TEST_IMG}" | _filter_testdir | 
_filter_qemu_io
 
-- 
2.9.3

[Qemu-block] [PATCH v15 19/21] osdep: Add qemu_lock_fd and qemu_unlock_fd

[Fam Zheng]

They are wrappers of POSIX fcntl "file private locking", with a
convenient "try lock" wrapper implemented with F_OFD_GETLK.

Signed-off-by: Fam Zheng 
Reviewed-by: Max Reitz 
---
 include/qemu/osdep.h |  3 +++
 util/osdep.c | 48 
 2 files changed, 51 insertions(+)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 122ff06..1c9f5e2 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -341,6 +341,9 @@ int qemu_close(int fd);
 #ifndef _WIN32
 int qemu_dup(int fd);
 #endif
+int qemu_lock_fd(int fd, int64_t start, int64_t len, bool exclusive);
+int qemu_unlock_fd(int fd, int64_t start, int64_t len);
+int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive);
 
 #if defined(__HAIKU__) && defined(__i386__)
 #define FMT_pid "%ld"
diff --git a/util/osdep.c b/util/osdep.c
index 06fb1cf..3de4a18 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -140,6 +140,54 @@ static int qemu_parse_fdset(const char *param)
 {
 return qemu_parse_fd(param);
 }
+
+static int qemu_lock_fcntl(int fd, int64_t start, int64_t len, int fl_type)
+{
+#ifdef F_OFD_SETLK
+int ret;
+struct flock fl = {
+.l_whence = SEEK_SET,
+.l_start  = start,
+.l_len= len,
+.l_type   = fl_type,
+};
+ret = fcntl(fd, F_OFD_SETLK, &fl);
+return ret == -1 ? -errno : 0;
+#else
+return -ENOTSUP;
+#endif
+}
+
+int qemu_lock_fd(int fd, int64_t start, int64_t len, bool exclusive)
+{
+return qemu_lock_fcntl(fd, start, len, exclusive ? F_WRLCK : F_RDLCK);
+}
+
+int qemu_unlock_fd(int fd, int64_t start, int64_t len)
+{
+return qemu_lock_fcntl(fd, start, len, F_UNLCK);
+}
+
+int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive)
+{
+#ifdef F_OFD_SETLK
+int ret;
+struct flock fl = {
+.l_whence = SEEK_SET,
+.l_start  = start,
+.l_len= len,
+.l_type   = exclusive ? F_WRLCK : F_RDLCK,
+};
+ret = fcntl(fd, F_OFD_GETLK, &fl);
+if (ret == -1) {
+return -errno;
+} else {
+return fl.l_type == F_UNLCK ? 0 : -EAGAIN;
+}
+#else
+return -ENOTSUP;
+#endif
+}
 #endif
 
 /*
-- 
2.9.3

[Qemu-block] [PATCH v15 17/21] tests: Disable image lock in test-replication

[Fam Zheng]

The COLO block replication architecture requires one disk to be shared
between primary and secondary, in the test both processes use posix file
protocol (instead of over NBD) so it is affected by image locking.
Disable the lock.

Signed-off-by: Fam Zheng 
---
 tests/test-replication.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tests/test-replication.c b/tests/test-replication.c
index fac2da3..e1c4235 100644
--- a/tests/test-replication.c
+++ b/tests/test-replication.c
@@ -179,7 +179,8 @@ static BlockBackend *start_primary(void)
 char *cmdline;
 
 cmdline = g_strdup_printf("driver=replication,mode=primary,node-name=xxx,"
-  "file.driver=qcow2,file.file.filename=%s"
+  "file.driver=qcow2,file.file.filename=%s,"
+  "file.file.locking=off"
   , p_local_disk);
 opts = qemu_opts_parse_noisily(&qemu_drive_opts, cmdline, false);
 g_free(cmdline);
@@ -310,7 +311,9 @@ static BlockBackend *start_secondary(void)
 Error *local_err = NULL;
 
 /* add s_local_disk and forge S_LOCAL_DISK_ID */
-cmdline = g_strdup_printf("file.filename=%s,driver=qcow2", s_local_disk);
+cmdline = g_strdup_printf("file.filename=%s,driver=qcow2,"
+  "file.locking=off",
+  s_local_disk);
 opts = qemu_opts_parse_noisily(&qemu_drive_opts, cmdline, false);
 g_free(cmdline);
 
@@ -331,8 +334,10 @@ static BlockBackend *start_secondary(void)
 /* add S_(ACTIVE/HIDDEN)_DISK and forge S_ID */
 cmdline = g_strdup_printf("driver=replication,mode=secondary,top-id=%s,"
   "file.driver=qcow2,file.file.filename=%s,"
+  "file.file.locking=off,"
   "file.backing.driver=qcow2,"
   "file.backing.file.filename=%s,"
+  "file.backing.file.locking=off,"
   "file.backing.backing=%s"
   , S_ID, s_active_disk, s_hidden_disk
   , S_LOCAL_DISK_ID);
-- 
2.9.3

[Qemu-block] [PATCH v15 14/21] iotests: 172: Use separate images for multiple devices

[Fam Zheng]

To avoid image lock failures.

Signed-off-by: Fam Zheng 
---
 tests/qemu-iotests/172 | 55 +-
 tests/qemu-iotests/172.out | 50 +
 2 files changed, 56 insertions(+), 49 deletions(-)

diff --git a/tests/qemu-iotests/172 b/tests/qemu-iotests/172
index 1b7d3a1..826d6fe 100755
--- a/tests/qemu-iotests/172
+++ b/tests/qemu-iotests/172
@@ -30,6 +30,8 @@ status=1  # failure is the default!
 _cleanup()
 {
_cleanup_test_img
+rm -f "$TEST_IMG.2"
+rm -f "$TEST_IMG.3"
 }
 trap "_cleanup; exit \$status" 0 1 2 3 15
 
@@ -86,6 +88,9 @@ size=720k
 
 _make_test_img $size
 
+TEST_IMG="$TEST_IMG.2" _make_test_img $size
+TEST_IMG="$TEST_IMG.3" _make_test_img $size
+
 # Default drive semantics:
 #
 # By default you get a single empty floppy drive. You can override it with
@@ -105,7 +110,7 @@ echo === Using -fda/-fdb options ===
 
 check_floppy_qtree -fda "$TEST_IMG"
 check_floppy_qtree -fdb "$TEST_IMG"
-check_floppy_qtree -fda "$TEST_IMG" -fdb "$TEST_IMG"
+check_floppy_qtree -fda "$TEST_IMG" -fdb "$TEST_IMG.2"
 
 
 echo
@@ -114,7 +119,7 @@ echo === Using -drive options ===
 
 check_floppy_qtree -drive if=floppy,file="$TEST_IMG"
 check_floppy_qtree -drive if=floppy,file="$TEST_IMG",index=1
-check_floppy_qtree -drive if=floppy,file="$TEST_IMG" -drive 
if=floppy,file="$TEST_IMG",index=1
+check_floppy_qtree -drive if=floppy,file="$TEST_IMG" -drive 
if=floppy,file="$TEST_IMG.2",index=1
 
 echo
 echo
@@ -122,7 +127,7 @@ echo === Using -drive if=none and -global ===
 
 check_floppy_qtree -drive if=none,file="$TEST_IMG" -global isa-fdc.driveA=none0
 check_floppy_qtree -drive if=none,file="$TEST_IMG" -global isa-fdc.driveB=none0
-check_floppy_qtree -drive if=none,file="$TEST_IMG" -drive 
if=none,file="$TEST_IMG" \
+check_floppy_qtree -drive if=none,file="$TEST_IMG" -drive 
if=none,file="$TEST_IMG.2" \
-global isa-fdc.driveA=none0 -global isa-fdc.driveB=none1
 
 echo
@@ -131,7 +136,7 @@ echo === Using -drive if=none and -device ===
 
 check_floppy_qtree -drive if=none,file="$TEST_IMG" -device floppy,drive=none0
 check_floppy_qtree -drive if=none,file="$TEST_IMG" -device 
floppy,drive=none0,unit=1
-check_floppy_qtree -drive if=none,file="$TEST_IMG" -drive 
if=none,file="$TEST_IMG" \
+check_floppy_qtree -drive if=none,file="$TEST_IMG" -drive 
if=none,file="$TEST_IMG.2" \
-device floppy,drive=none0 -device floppy,drive=none1,unit=1
 
 echo
@@ -139,58 +144,58 @@ echo
 echo === Mixing -fdX and -global ===
 
 # Working
-check_floppy_qtree -fda "$TEST_IMG" -drive if=none,file="$TEST_IMG" -global 
isa-fdc.driveB=none0
-check_floppy_qtree -fdb "$TEST_IMG" -drive if=none,file="$TEST_IMG" -global 
isa-fdc.driveA=none0
+check_floppy_qtree -fda "$TEST_IMG" -drive if=none,file="$TEST_IMG.2" -global 
isa-fdc.driveB=none0
+check_floppy_qtree -fdb "$TEST_IMG" -drive if=none,file="$TEST_IMG.2" -global 
isa-fdc.driveA=none0
 
 # Conflicting (-fdX wins)
-check_floppy_qtree -fda "$TEST_IMG" -drive if=none,file="$TEST_IMG" -global 
isa-fdc.driveA=none0
-check_floppy_qtree -fdb "$TEST_IMG" -drive if=none,file="$TEST_IMG" -global 
isa-fdc.driveB=none0
+check_floppy_qtree -fda "$TEST_IMG" -drive if=none,file="$TEST_IMG.2" -global 
isa-fdc.driveA=none0
+check_floppy_qtree -fdb "$TEST_IMG" -drive if=none,file="$TEST_IMG.2" -global 
isa-fdc.driveB=none0
 
 echo
 echo
 echo === Mixing -fdX and -device ===
 
 # Working
-check_floppy_qtree -fda "$TEST_IMG" -drive if=none,file="$TEST_IMG" -device 
floppy,drive=none0
-check_floppy_qtree -fda "$TEST_IMG" -drive if=none,file="$TEST_IMG" -device 
floppy,drive=none0,unit=1
+check_floppy_qtree -fda "$TEST_IMG" -drive if=none,file="$TEST_IMG.2" -device 
floppy,drive=none0
+check_floppy_qtree -fda "$TEST_IMG" -drive if=none,file="$TEST_IMG.2" -device 
floppy,drive=none0,unit=1
 
-check_floppy_qtree -fdb "$TEST_IMG" -drive if=none,file="$TEST_IMG" -device 
floppy,drive=none0
-check_floppy_qtree -fdb "$TEST_IMG" -drive if=none,file="$TEST_IMG" -device 
floppy,drive=none0,unit=0
+check_floppy_qtree -fdb "$TEST_IMG" -drive if=none,file="$TEST_IMG.2" -device 
floppy,drive=none0
+check_floppy_qtree -fdb "$TEST_IMG" -drive if=none,file="$TEST_IMG.2" -device 
floppy,drive=none0,unit=0
 
 # Conflicting
-check_floppy_qtree -fda "$TEST_IMG" -drive if=none,file="$TEST_IMG" -device 
floppy,drive=none0,unit=0
-check_floppy_qtree -fdb "$TEST_IMG" -drive if=none,file="$TEST_IMG" -device 
floppy,drive=none0,unit=1
+check_floppy_qtree -fda "$TEST_IMG" -drive if=none,file="$TEST_IMG.2" -device 
floppy,drive=none0,unit=0
+check_floppy_qtree -fdb "$TEST_IMG" -drive if=none,file="$TEST_IMG.2" -device 
floppy,drive=none0,unit=1
 
 echo
 echo
 echo === Mixing -drive and -device ===
 
 # Working
-check_floppy_qtree -drive if=floppy,file="$TEST_IMG" -drive 
if=none,file="$TEST_IMG" -device floppy,drive=none0
-check_floppy_qtree -drive if=floppy,file="$TEST_IMG" -drive 
if=none,file="$TEST_IMG" -device floppy

[Qemu-block] [PATCH v15 09/21] iotests: 046: Prepare for image locking

[Fam Zheng]

The qemu-img info command is executed while VM is running, add -U option
to avoid the image locking error.

Signed-off-by: Fam Zheng 
---
 tests/qemu-iotests/046 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/046 b/tests/qemu-iotests/046
index e528b67..f2ebecf 100755
--- a/tests/qemu-iotests/046
+++ b/tests/qemu-iotests/046
@@ -192,7 +192,7 @@ echo "== Verify image content =="
 
 function verify_io()
 {
-if ($QEMU_IMG info -f "$IMGFMT" "$TEST_IMG" | grep "compat: 0.10" > 
/dev/null); then
+if ($QEMU_IMG info -U -f "$IMGFMT" "$TEST_IMG" | grep "compat: 0.10" > 
/dev/null); then
 # For v2 images, discarded clusters are read from the backing file
 # Keep the variable empty so that the backing file value can be used as
 # the default below
-- 
2.9.3

[Qemu-block] [PATCH v15 10/21] iotests: 055: Don't attach the target image already for drive-backup

[Fam Zheng]

Double attach is not a valid usage of the target image, drive-backup
will open the blockdev itself so skip the add_drive call in this case.

Signed-off-by: Fam Zheng 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/055 | 32 ++--
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055
index aafcd24..ba4da65 100755
--- a/tests/qemu-iotests/055
+++ b/tests/qemu-iotests/055
@@ -458,17 +458,18 @@ class TestDriveCompression(iotests.QMPTestCase):
 except OSError:
 pass
 
-def do_prepare_drives(self, fmt, args):
+def do_prepare_drives(self, fmt, args, attach_target):
 self.vm = iotests.VM().add_drive(test_img)
 
 qemu_img('create', '-f', fmt, blockdev_target_img,
  str(TestDriveCompression.image_len), *args)
-self.vm.add_drive(blockdev_target_img, format=fmt, interface="none")
+if attach_target:
+self.vm.add_drive(blockdev_target_img, format=fmt, 
interface="none")
 
 self.vm.launch()
 
-def do_test_compress_complete(self, cmd, format, **args):
-self.do_prepare_drives(format['type'], format['args'])
+def do_test_compress_complete(self, cmd, format, attach_target, **args):
+self.do_prepare_drives(format['type'], format['args'], attach_target)
 
 self.assert_no_active_block_jobs()
 
@@ -484,15 +485,16 @@ class TestDriveCompression(iotests.QMPTestCase):
 
 def test_complete_compress_drive_backup(self):
 for format in TestDriveCompression.fmt_supports_compression:
-self.do_test_compress_complete('drive-backup', format,
+self.do_test_compress_complete('drive-backup', format, False,
target=blockdev_target_img, 
mode='existing')
 
 def test_complete_compress_blockdev_backup(self):
 for format in TestDriveCompression.fmt_supports_compression:
-self.do_test_compress_complete('blockdev-backup', format, 
target='drive1')
+self.do_test_compress_complete('blockdev-backup', format, True,
+   target='drive1')
 
-def do_test_compress_cancel(self, cmd, format, **args):
-self.do_prepare_drives(format['type'], format['args'])
+def do_test_compress_cancel(self, cmd, format, attach_target, **args):
+self.do_prepare_drives(format['type'], format['args'], attach_target)
 
 self.assert_no_active_block_jobs()
 
@@ -506,15 +508,16 @@ class TestDriveCompression(iotests.QMPTestCase):
 
 def test_compress_cancel_drive_backup(self):
 for format in TestDriveCompression.fmt_supports_compression:
-self.do_test_compress_cancel('drive-backup', format,
+self.do_test_compress_cancel('drive-backup', format, False,
  target=blockdev_target_img, 
mode='existing')
 
 def test_compress_cancel_blockdev_backup(self):
for format in TestDriveCompression.fmt_supports_compression:
-self.do_test_compress_cancel('blockdev-backup', format, 
target='drive1')
+self.do_test_compress_cancel('blockdev-backup', format, True,
+ target='drive1')
 
-def do_test_compress_pause(self, cmd, format, **args):
-self.do_prepare_drives(format['type'], format['args'])
+def do_test_compress_pause(self, cmd, format, attach_target, **args):
+self.do_prepare_drives(format['type'], format['args'], attach_target)
 
 self.assert_no_active_block_jobs()
 
@@ -546,12 +549,13 @@ class TestDriveCompression(iotests.QMPTestCase):
 
 def test_compress_pause_drive_backup(self):
 for format in TestDriveCompression.fmt_supports_compression:
-self.do_test_compress_pause('drive-backup', format,
+self.do_test_compress_pause('drive-backup', format, False,
 target=blockdev_target_img, 
mode='existing')
 
 def test_compress_pause_blockdev_backup(self):
 for format in TestDriveCompression.fmt_supports_compression:
-self.do_test_compress_pause('blockdev-backup', format, 
target='drive1')
+self.do_test_compress_pause('blockdev-backup', format, True,
+target='drive1')
 
 if __name__ == '__main__':
 iotests.main(supported_fmts=['raw', 'qcow2'])
-- 
2.9.3

[Qemu-block] [PATCH v15 12/21] iotests: 087: Don't attach test image twice

[Fam Zheng]

The test scenario doesn't require the same image, instead it focuses on
the duplicated node-name, so use null-co to avoid locking conflict.

Reviewed-by: Max Reitz 
Signed-off-by: Fam Zheng 
---
 tests/qemu-iotests/087 | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087
index 9de57dd..6d52f7d 100755
--- a/tests/qemu-iotests/087
+++ b/tests/qemu-iotests/087
@@ -82,8 +82,7 @@ run_qemu -drive 
driver=$IMGFMT,id=disk,node-name=test-node,file="$TEST_IMG" <

[Qemu-block] [PATCH v15 16/21] file-posix: Add 'locking' option

[Fam Zheng]

Making this option available even before implementing it will let
converting tests easier: in coming patches they can specify the option
already when necessary, before we actually write code to lock the
images.

Signed-off-by: Fam Zheng 
---
 block/file-posix.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/block/file-posix.c b/block/file-posix.c
index ade71db..2114720 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -392,6 +392,11 @@ static QemuOptsList raw_runtime_opts = {
 .type = QEMU_OPT_STRING,
 .help = "host AIO implementation (threads, native)",
 },
+{
+.name = "locking",
+.type = QEMU_OPT_BOOL,
+.help = "lock the file",
+},
 { /* end of list */ }
 },
 };
-- 
2.9.3

[Qemu-block] [PATCH v15 11/21] iotests: 085: Avoid image locking conflict

[Fam Zheng]

In the case where we test the expected error when a blockdev-snapshot
target already has a backing image, the backing chain is opened multiple
times. This will be a problem when we use image locking, so use a
different backing file that is not already open.

Signed-off-by: Fam Zheng 
---
 tests/qemu-iotests/085 | 34 --
 tests/qemu-iotests/085.out |  3 ++-
 2 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/tests/qemu-iotests/085 b/tests/qemu-iotests/085
index c53e97f..cc6efd8 100755
--- a/tests/qemu-iotests/085
+++ b/tests/qemu-iotests/085
@@ -45,7 +45,7 @@ _cleanup()
 rm -f "${TEST_DIR}/${i}-${snapshot_virt0}"
 rm -f "${TEST_DIR}/${i}-${snapshot_virt1}"
 done
-rm -f "${TEST_IMG}.1" "${TEST_IMG}.2"
+rm -f "${TEST_IMG}" "${TEST_IMG}.1" "${TEST_IMG}.2" "${TEST_IMG}.base"
 
 }
 trap "_cleanup; exit \$status" 0 1 2 3 15
@@ -87,24 +87,26 @@ function create_group_snapshot()
 }
 
 # ${1}: unique identifier for the snapshot filename
-# ${2}: true: open backing images; false: don't open them (default)
+# ${2}: extra_params to the blockdev-add command
+# ${3}: filename
+function do_blockdev_add()
+{
+cmd="{ 'execute': 'blockdev-add', 'arguments':
+   { 'driver': 'qcow2', 'node-name': 'snap_${1}', ${2}
+ 'file':
+ { 'driver': 'file', 'filename': '${3}',
+   'node-name': 'file_${1}' } } }"
+_send_qemu_cmd $h "${cmd}" "return"
+}
+
+# ${1}: unique identifier for the snapshot filename
 function add_snapshot_image()
 {
-if [ "${2}" = "true" ]; then
-extra_params=""
-else
-extra_params="'backing': '', "
-fi
 base_image="${TEST_DIR}/$((${1}-1))-${snapshot_virt0}"
 snapshot_file="${TEST_DIR}/${1}-${snapshot_virt0}"
 _make_test_img -b "${base_image}" "$size"
 mv "${TEST_IMG}" "${snapshot_file}"
-cmd="{ 'execute': 'blockdev-add', 'arguments':
-   { 'driver': 'qcow2', 'node-name': 'snap_${1}', ${extra_params}
- 'file':
- { 'driver': 'file', 'filename': '${snapshot_file}',
-   'node-name': 'file_${1}' } } }"
-_send_qemu_cmd $h "${cmd}" "return"
+do_blockdev_add "$1" "'backing': '', " "${snapshot_file}"
 }
 
 # ${1}: unique identifier for the snapshot filename
@@ -222,7 +224,11 @@ echo === Invalid command - snapshot node has a backing 
image ===
 echo
 
 SNAPSHOTS=$((${SNAPSHOTS}+1))
-add_snapshot_image ${SNAPSHOTS} true
+
+_make_test_img "$size"
+mv "${TEST_IMG}" "${TEST_IMG}.base"
+_make_test_img -b "${TEST_IMG}.base" "$size"
+do_blockdev_add ${SNAPSHOTS} "" "${TEST_IMG}"
 blockdev_snapshot ${SNAPSHOTS} error
 
 echo
diff --git a/tests/qemu-iotests/085.out b/tests/qemu-iotests/085.out
index 182acb4..65b0f68 100644
--- a/tests/qemu-iotests/085.out
+++ b/tests/qemu-iotests/085.out
@@ -78,7 +78,8 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 
backing_file=TEST_DIR/
 
 === Invalid command - snapshot node has a backing image ===
 
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 
backing_file=TEST_DIR/12-snapshot-v0.IMGFMT
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 
backing_file=TEST_DIR/t.IMGFMT.base
 {"return": {}}
 {"error": {"class": "GenericError", "desc": "The snapshot already has a 
backing image"}}
 
-- 
2.9.3

[Qemu-block] [PATCH v15 08/21] iotests: 030: Prepare for image locking

[Fam Zheng]

qemu-img and qemu-io commands when guest is running need "-U" option,
add it.

Signed-off-by: Fam Zheng 
---
 tests/qemu-iotests/030 | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index 0d472d5..e00c11b 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -63,8 +63,8 @@ class TestSingleDrive(iotests.QMPTestCase):
 def test_stream_intermediate(self):
 self.assert_no_active_block_jobs()
 
-self.assertNotEqual(qemu_io('-f', 'raw', '-c', 'map', backing_img),
-qemu_io('-f', iotests.imgfmt, '-c', 'map', 
mid_img),
+self.assertNotEqual(qemu_io('-f', 'raw', '-rU', '-c', 'map', 
backing_img),
+qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map', 
mid_img),
 'image file map matches backing file before 
streaming')
 
 result = self.vm.qmp('block-stream', device='mid', job_id='stream-mid')
@@ -114,7 +114,7 @@ class TestSingleDrive(iotests.QMPTestCase):
 self.assert_no_active_block_jobs()
 
 # The image map is empty before the operation
-empty_map = qemu_io('-f', iotests.imgfmt, '-c', 'map', test_img)
+empty_map = qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map', test_img)
 
 # This is a no-op: no data should ever be copied from the base image
 result = self.vm.qmp('block-stream', device='drive0', base=mid_img)
@@ -197,8 +197,8 @@ class TestParallelOps(iotests.QMPTestCase):
 
 # Check that the maps don't match before the streaming operations
 for i in range(2, self.num_imgs, 2):
-self.assertNotEqual(qemu_io('-f', iotests.imgfmt, '-c', 'map', 
self.imgs[i]),
-qemu_io('-f', iotests.imgfmt, '-c', 'map', 
self.imgs[i-1]),
+self.assertNotEqual(qemu_io('-f', iotests.imgfmt, '-rU', '-c', 
'map', self.imgs[i]),
+qemu_io('-f', iotests.imgfmt, '-rU', '-c', 
'map', self.imgs[i-1]),
 'image file map matches backing file before 
streaming')
 
 # Create all streaming jobs
@@ -351,8 +351,8 @@ class TestParallelOps(iotests.QMPTestCase):
 def test_stream_base_node_name(self):
 self.assert_no_active_block_jobs()
 
-self.assertNotEqual(qemu_io('-f', iotests.imgfmt, '-c', 'map', 
self.imgs[4]),
-qemu_io('-f', iotests.imgfmt, '-c', 'map', 
self.imgs[3]),
+self.assertNotEqual(qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map', 
self.imgs[4]),
+qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map', 
self.imgs[3]),
 'image file map matches backing file before 
streaming')
 
 # Error: the base node does not exist
@@ -422,8 +422,8 @@ class TestQuorum(iotests.QMPTestCase):
 if not iotests.supports_quorum():
 return
 
-self.assertNotEqual(qemu_io('-f', iotests.imgfmt, '-c', 'map', 
self.children[0]),
-qemu_io('-f', iotests.imgfmt, '-c', 'map', 
self.backing[0]),
+self.assertNotEqual(qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map', 
self.children[0]),
+qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map', 
self.backing[0]),
 'image file map matches backing file before 
streaming')
 
 self.assert_no_active_block_jobs()
-- 
2.9.3

[Qemu-block] [PATCH v15 06/21] qemu-img: Update documentation for -U

[Fam Zheng]

Signed-off-by: Fam Zheng 
---
 qemu-img-cmds.hx | 36 ++--
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 8ac7822..ae309c0 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -10,15 +10,15 @@ STEXI
 ETEXI
 
 DEF("bench", img_bench,
-"bench [-c count] [-d depth] [-f fmt] [--flush-interval=flush_interval] 
[-n] [--no-drain] [-o offset] [--pattern=pattern] [-q] [-s buffer_size] [-S 
step_size] [-t cache] [-w] filename")
+"bench [-c count] [-d depth] [-f fmt] [--flush-interval=flush_interval] 
[-n] [--no-drain] [-o offset] [--pattern=pattern] [-q] [-s buffer_size] [-S 
step_size] [-t cache] [-w] [-U] filename")
 STEXI
-@item bench [-c @var{count}] [-d @var{depth}] [-f @var{fmt}] 
[--flush-interval=@var{flush_interval}] [-n] [--no-drain] [-o @var{offset}] 
[--pattern=@var{pattern}] [-q] [-s @var{buffer_size}] [-S @var{step_size}] [-t 
@var{cache}] [-w] @var{filename}
+@item bench [-c @var{count}] [-d @var{depth}] [-f @var{fmt}] 
[--flush-interval=@var{flush_interval}] [-n] [--no-drain] [-o @var{offset}] 
[--pattern=@var{pattern}] [-q] [-s @var{buffer_size}] [-S @var{step_size}] [-t 
@var{cache}] [-w] [-U] @var{filename}
 ETEXI
 
 DEF("check", img_check,
-"check [-q] [--object objectdef] [--image-opts] [-f fmt] [--output=ofmt] 
[-r [leaks | all]] [-T src_cache] filename")
+"check [-q] [--object objectdef] [--image-opts] [-f fmt] [--output=ofmt] 
[-r [leaks | all]] [-T src_cache] [-U] filename")
 STEXI
-@item check [--object @var{objectdef}] [--image-opts] [-q] [-f @var{fmt}] 
[--output=@var{ofmt}] [-r [leaks | all]] [-T @var{src_cache}] @var{filename}
+@item check [--object @var{objectdef}] [--image-opts] [-q] [-f @var{fmt}] 
[--output=@var{ofmt}] [-r [leaks | all]] [-T @var{src_cache}] [-U] 
@var{filename}
 ETEXI
 
 DEF("create", img_create,
@@ -34,45 +34,45 @@ STEXI
 ETEXI
 
 DEF("compare", img_compare,
-"compare [--object objectdef] [--image-opts] [-f fmt] [-F fmt] [-T 
src_cache] [-p] [-q] [-s] filename1 filename2")
+"compare [--object objectdef] [--image-opts] [-f fmt] [-F fmt] [-T 
src_cache] [-p] [-q] [-s] [-U] filename1 filename2")
 STEXI
-@item compare [--object @var{objectdef}] [--image-opts] [-f @var{fmt}] [-F 
@var{fmt}] [-T @var{src_cache}] [-p] [-q] [-s] @var{filename1} @var{filename2}
+@item compare [--object @var{objectdef}] [--image-opts] [-f @var{fmt}] [-F 
@var{fmt}] [-T @var{src_cache}] [-p] [-q] [-s] [-U] @var{filename1} 
@var{filename2}
 ETEXI
 
 DEF("convert", img_convert,
-"convert [--object objectdef] [--image-opts] [-c] [-p] [-q] [-n] [-f fmt] 
[-t cache] [-T src_cache] [-O output_fmt] [-o options] [-s snapshot_id_or_name] 
[-l snapshot_param] [-S sparse_size] [-m num_coroutines] [-W] filename 
[filename2 [...]] output_filename")
+"convert [--object objectdef] [--image-opts] [-c] [-p] [-q] [-n] [-f fmt] 
[-t cache] [-T src_cache] [-O output_fmt] [-o options] [-s snapshot_id_or_name] 
[-l snapshot_param] [-S sparse_size] [-m num_coroutines] [-W] [-U] filename 
[filename2 [...]] output_filename")
 STEXI
-@item convert [--object @var{objectdef}] [--image-opts] [-c] [-p] [-q] [-n] 
[-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-O @var{output_fmt}] [-o 
@var{options}] [-s @var{snapshot_id_or_name}] [-l @var{snapshot_param}] [-S 
@var{sparse_size}] [-m @var{num_coroutines}] [-W] @var{filename} 
[@var{filename2} [...]] @var{output_filename}
+@item convert [--object @var{objectdef}] [--image-opts] [-c] [-p] [-q] [-n] 
[-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-O @var{output_fmt}] [-o 
@var{options}] [-s @var{snapshot_id_or_name}] [-l @var{snapshot_param}] [-S 
@var{sparse_size}] [-m @var{num_coroutines}] [-W] [-U] @var{filename} 
[@var{filename2} [...]] @var{output_filename}
 ETEXI
 
 DEF("dd", img_dd,
-"dd [--image-opts] [-f fmt] [-O output_fmt] [bs=block_size] [count=blocks] 
[skip=blocks] if=input of=output")
+"dd [--image-opts] [-U] [-f fmt] [-O output_fmt] [bs=block_size] 
[count=blocks] [skip=blocks] if=input of=output")
 STEXI
-@item dd [--image-opts] [-f @var{fmt}] [-O @var{output_fmt}] 
[bs=@var{block_size}] [count=@var{blocks}] [skip=@var{blocks}] if=@var{input} 
of=@var{output}
+@item dd [--image-opts] [-U] [-f @var{fmt}] [-O @var{output_fmt}] 
[bs=@var{block_size}] [count=@var{blocks}] [skip=@var{blocks}] if=@var{input} 
of=@var{output}
 ETEXI
 
 DEF("info", img_info,
-"info [--object objectdef] [--image-opts] [-f fmt] [--output=ofmt] 
[--backing-chain] filename")
+"info [--object objectdef] [--image-opts] [-f fmt] [--output=ofmt] 
[--backing-chain] [-U] filename")
 STEXI
-@item info [--object @var{objectdef}] [--image-opts] [-f @var{fmt}] 
[--output=@var{ofmt}] [--backing-chain] @var{filename}
+@item info [--object @var{objectdef}] [--image-opts] [-f @var{fmt}] 
[--output=@var{ofmt}] [--backing-chain] [-U] @var{filename}
 ETEXI
 
 DEF("map", img_map,
-"map [--object objectdef] [--image-opts] [-f fmt] [--output=ofmt] 

[Qemu-block] [PATCH v15 07/21] qemu-io: Add --force-share option

[Fam Zheng]

Add --force-share/-U to program options and -U to open subcommand.

Signed-off-by: Fam Zheng 
---
 qemu-io.c | 42 ++
 1 file changed, 34 insertions(+), 8 deletions(-)

diff --git a/qemu-io.c b/qemu-io.c
index 427cbae..cf4b876 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -20,6 +20,7 @@
 #include "qemu/readline.h"
 #include "qemu/log.h"
 #include "qapi/qmp/qstring.h"
+#include "qapi/qmp/qbool.h"
 #include "qom/object_interfaces.h"
 #include "sysemu/block-backend.h"
 #include "block/block_int.h"
@@ -53,7 +54,8 @@ static const cmdinfo_t close_cmd = {
 .oneline= "close the current open file",
 };
 
-static int openfile(char *name, int flags, bool writethrough, QDict *opts)
+static int openfile(char *name, int flags, bool writethrough, bool force_share,
+QDict *opts)
 {
 Error *local_err = NULL;
 BlockDriverState *bs;
@@ -64,6 +66,18 @@ static int openfile(char *name, int flags, bool 
writethrough, QDict *opts)
 return 1;
 }
 
+if (force_share) {
+if (!opts) {
+opts = qdict_new();
+}
+if (qdict_haskey(opts, BDRV_OPT_FORCE_SHARE)
+&& !qdict_get_bool(opts, BDRV_OPT_FORCE_SHARE)) {
+error_report("-U conflicts with image options");
+QDECREF(opts);
+return 1;
+}
+qdict_put(opts, BDRV_OPT_FORCE_SHARE, qbool_from_bool(true));
+}
 qemuio_blk = blk_new_open(name, NULL, opts, flags, &local_err);
 if (!qemuio_blk) {
 error_reportf_err(local_err, "can't open%s%s: ",
@@ -108,6 +122,7 @@ static void open_help(void)
 " -r, -- open file read-only\n"
 " -s, -- use snapshot file\n"
 " -n, -- disable host cache, short for -t none\n"
+" -U, -- force shared permissions\n"
 " -k, -- use kernel AIO implementation (on Linux only)\n"
 " -t, -- use the given cache mode for the image\n"
 " -d, -- use the given discard mode for the image\n"
@@ -124,7 +139,7 @@ static const cmdinfo_t open_cmd = {
 .argmin = 1,
 .argmax = -1,
 .flags  = CMD_NOFILE_OK,
-.args   = "[-rsnk] [-t cache] [-d discard] [-o options] [path]",
+.args   = "[-rsnkU] [-t cache] [-d discard] [-o options] [path]",
 .oneline= "open the file specified by path",
 .help   = open_help,
 };
@@ -147,8 +162,9 @@ static int open_f(BlockBackend *blk, int argc, char **argv)
 int c;
 QemuOpts *qopts;
 QDict *opts;
+bool force_share = false;
 
-while ((c = getopt(argc, argv, "snro:kt:d:")) != -1) {
+while ((c = getopt(argc, argv, "snro:kt:d:U")) != -1) {
 switch (c) {
 case 's':
 flags |= BDRV_O_SNAPSHOT;
@@ -188,6 +204,9 @@ static int open_f(BlockBackend *blk, int argc, char **argv)
 return 0;
 }
 break;
+case 'U':
+force_share = true;
+break;
 default:
 qemu_opts_reset(&empty_opts);
 return qemuio_command_usage(&open_cmd);
@@ -211,9 +230,9 @@ static int open_f(BlockBackend *blk, int argc, char **argv)
 qemu_opts_reset(&empty_opts);
 
 if (optind == argc - 1) {
-return openfile(argv[optind], flags, writethrough, opts);
+return openfile(argv[optind], flags, writethrough, force_share, opts);
 } else if (optind == argc) {
-return openfile(NULL, flags, writethrough, opts);
+return openfile(NULL, flags, writethrough, force_share, opts);
 } else {
 QDECREF(opts);
 return qemuio_command_usage(&open_cmd);
@@ -257,6 +276,7 @@ static void usage(const char *name)
 "  -T, --trace [[enable=]][,events=][,file=]\n"
 "   specify tracing options\n"
 "   see qemu-img(1) man page for full description\n"
+"  -U, --force-shareforce shared permissions\n"
 "  -h, --help   display this help and exit\n"
 "  -V, --versionoutput version information and exit\n"
 "\n"
@@ -436,7 +456,7 @@ static QemuOptsList file_opts = {
 int main(int argc, char **argv)
 {
 int readonly = 0;
-const char *sopt = "hVc:d:f:rsnmkt:T:";
+const char *sopt = "hVc:d:f:rsnmkt:T:U";
 const struct option lopt[] = {
 { "help", no_argument, NULL, 'h' },
 { "version", no_argument, NULL, 'V' },
@@ -452,6 +472,7 @@ int main(int argc, char **argv)
 { "trace", required_argument, NULL, 'T' },
 { "object", required_argument, NULL, OPTION_OBJECT },
 { "image-opts", no_argument, NULL, OPTION_IMAGE_OPTS },
+{ "force-share", no_argument, 0, 'U'},
 { NULL, 0, NULL, 0 }
 };
 int c;
@@ -462,6 +483,7 @@ int main(int argc, char **argv)
 QDict *opts = NULL;
 const char *format = NULL;
 char *trace_file = NULL;
+bool force_share = false;
 
 #ifdef CONFIG_POSIX
 signal(SIGPIPE, SIG_IGN);
@@ -524,6 +546,9 @@ int main(int argc, char **argv)
 case 'h':
 usage(progname);
 exit(0);
+case 'U':
+ 

[Qemu-block] [PATCH v15 04/21] block: Respect "force-share" in perm propagating

[Fam Zheng]

Signed-off-by: Fam Zheng 
---
 block.c | 32 
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/block.c b/block.c
index 9db39b6..e9f4750 100644
--- a/block.c
+++ b/block.c
@@ -1430,6 +1430,22 @@ static int bdrv_child_check_perm(BdrvChild *c, uint64_t 
perm, uint64_t shared,
 static void bdrv_child_abort_perm_update(BdrvChild *c);
 static void bdrv_child_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared);
 
+static void bdrv_child_perm(BlockDriverState *bs, BlockDriverState *child_bs,
+BdrvChild *c,
+const BdrvChildRole *role,
+uint64_t parent_perm, uint64_t parent_shared,
+uint64_t *nperm, uint64_t *nshared)
+{
+if (bs->drv && bs->drv->bdrv_child_perm) {
+bs->drv->bdrv_child_perm(bs, c, role,
+ parent_perm, parent_shared,
+ nperm, nshared);
+}
+if (child_bs && child_bs->force_share) {
+*nshared = BLK_PERM_ALL;
+}
+}
+
 /*
  * Check whether permissions on this node can be changed in a way that
  * @cumulative_perms and @cumulative_shared_perms are the new cumulative
@@ -1474,9 +1490,9 @@ static int bdrv_check_perm(BlockDriverState *bs, uint64_t 
cumulative_perms,
 /* Check all children */
 QLIST_FOREACH(c, &bs->children, next) {
 uint64_t cur_perm, cur_shared;
-drv->bdrv_child_perm(bs, c, c->role,
- cumulative_perms, cumulative_shared_perms,
- &cur_perm, &cur_shared);
+bdrv_child_perm(bs, c->bs, c, c->role,
+cumulative_perms, cumulative_shared_perms,
+&cur_perm, &cur_shared);
 ret = bdrv_child_check_perm(c, cur_perm, cur_shared, ignore_children,
 errp);
 if (ret < 0) {
@@ -1536,9 +1552,9 @@ static void bdrv_set_perm(BlockDriverState *bs, uint64_t 
cumulative_perms,
 /* Update all children */
 QLIST_FOREACH(c, &bs->children, next) {
 uint64_t cur_perm, cur_shared;
-drv->bdrv_child_perm(bs, c, c->role,
- cumulative_perms, cumulative_shared_perms,
- &cur_perm, &cur_shared);
+bdrv_child_perm(bs, c->bs, c, c->role,
+cumulative_perms, cumulative_shared_perms,
+&cur_perm, &cur_shared);
 bdrv_child_set_perm(c, cur_perm, cur_shared);
 }
 }
@@ -1873,8 +1889,8 @@ BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
 
 assert(parent_bs->drv);
 assert(bdrv_get_aio_context(parent_bs) == bdrv_get_aio_context(child_bs));
-parent_bs->drv->bdrv_child_perm(parent_bs, NULL, child_role,
-perm, shared_perm, &perm, &shared_perm);
+bdrv_child_perm(parent_bs, child_bs, NULL, child_role,
+perm, shared_perm, &perm, &shared_perm);
 
 child = bdrv_root_attach_child(child_bs, child_name, child_role,
perm, shared_perm, parent_bs, errp);
-- 
2.9.3

[Qemu-block] [PATCH v15 02/21] block: Define BLK_PERM_MAX

[Fam Zheng]

This is the order of the largest possible permission.

Signed-off-by: Fam Zheng 
---
 include/block/block.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/block/block.h b/include/block/block.h
index eb0565d..a798f10 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -224,6 +224,8 @@ enum {
 BLK_PERM_ALL= 0x1f,
 };
 
+#define BLK_PERM_MAX (64 - clz64((uint64_t)BLK_PERM_ALL))
+
 char *bdrv_perm_names(uint64_t perm);
 
 /* disk I/O throttling */
-- 
2.9.3

[Qemu-block] [PATCH v15 05/21] qemu-img: Add --force-share option to subcommands

[Fam Zheng]

This will force the opened images to allow sharing all permissions with other
programs.

Signed-off-by: Fam Zheng 
---
 qemu-img.c | 154 ++---
 1 file changed, 118 insertions(+), 36 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index ed24371..c0fb1ce 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -28,6 +28,7 @@
 #include "qapi/qobject-output-visitor.h"
 #include "qapi/qmp/qerror.h"
 #include "qapi/qmp/qjson.h"
+#include "qapi/qmp/qbool.h"
 #include "qemu/cutils.h"
 #include "qemu/config-file.h"
 #include "qemu/option.h"
@@ -283,12 +284,20 @@ static int img_open_password(BlockBackend *blk, const 
char *filename,
 
 static BlockBackend *img_open_opts(const char *optstr,
QemuOpts *opts, int flags, bool 
writethrough,
-   bool quiet)
+   bool quiet, bool force_share)
 {
 QDict *options;
 Error *local_err = NULL;
 BlockBackend *blk;
 options = qemu_opts_to_qdict(opts, NULL);
+if (force_share) {
+if (qdict_haskey(options, BDRV_OPT_FORCE_SHARE)
+&& !qdict_get_bool(options, BDRV_OPT_FORCE_SHARE)) {
+error_report("--force-share/-U conflicts with image options");
+return NULL;
+}
+qdict_put(options, BDRV_OPT_FORCE_SHARE, qbool_from_bool(true));
+}
 blk = blk_new_open(NULL, NULL, options, flags, &local_err);
 if (!blk) {
 error_reportf_err(local_err, "Could not open '%s': ", optstr);
@@ -305,17 +314,20 @@ static BlockBackend *img_open_opts(const char *optstr,
 
 static BlockBackend *img_open_file(const char *filename,
const char *fmt, int flags,
-   bool writethrough, bool quiet)
+   bool writethrough, bool quiet,
+   bool force_share)
 {
 BlockBackend *blk;
 Error *local_err = NULL;
-QDict *options = NULL;
+QDict *options = qdict_new();
 
 if (fmt) {
-options = qdict_new();
 qdict_put(options, "driver", qstring_from_str(fmt));
 }
 
+if (force_share) {
+qdict_put(options, BDRV_OPT_FORCE_SHARE, qbool_from_bool(true));
+}
 blk = blk_new_open(filename, NULL, options, flags, &local_err);
 if (!blk) {
 error_reportf_err(local_err, "Could not open '%s': ", filename);
@@ -334,7 +346,7 @@ static BlockBackend *img_open_file(const char *filename,
 static BlockBackend *img_open(bool image_opts,
   const char *filename,
   const char *fmt, int flags, bool writethrough,
-  bool quiet)
+  bool quiet, bool force_share)
 {
 BlockBackend *blk;
 if (image_opts) {
@@ -348,9 +360,11 @@ static BlockBackend *img_open(bool image_opts,
 if (!opts) {
 return NULL;
 }
-blk = img_open_opts(filename, opts, flags, writethrough, quiet);
+blk = img_open_opts(filename, opts, flags, writethrough, quiet,
+force_share);
 } else {
-blk = img_open_file(filename, fmt, flags, writethrough, quiet);
+blk = img_open_file(filename, fmt, flags, writethrough, quiet,
+force_share);
 }
 return blk;
 }
@@ -650,6 +664,7 @@ static int img_check(int argc, char **argv)
 ImageCheck *check;
 bool quiet = false;
 bool image_opts = false;
+bool force_share = false;
 
 fmt = NULL;
 output = NULL;
@@ -664,9 +679,10 @@ static int img_check(int argc, char **argv)
 {"output", required_argument, 0, OPTION_OUTPUT},
 {"object", required_argument, 0, OPTION_OBJECT},
 {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
+{"force-share", no_argument, 0, 'U'},
 {0, 0, 0, 0}
 };
-c = getopt_long(argc, argv, ":hf:r:T:q",
+c = getopt_long(argc, argv, ":hf:r:T:qU",
 long_options, &option_index);
 if (c == -1) {
 break;
@@ -705,6 +721,9 @@ static int img_check(int argc, char **argv)
 case 'q':
 quiet = true;
 break;
+case 'U':
+force_share = true;
+break;
 case OPTION_OBJECT: {
 QemuOpts *opts;
 opts = qemu_opts_parse_noisily(&qemu_object_opts,
@@ -744,7 +763,8 @@ static int img_check(int argc, char **argv)
 return 1;
 }
 
-blk = img_open(image_opts, filename, fmt, flags, writethrough, quiet);
+blk = img_open(image_opts, filename, fmt, flags, writethrough, quiet,
+   force_share);
 if (!blk) {
 return 1;
 }
@@ -947,7 +967,8 @@ static int img_commit(int argc, char **argv)
 return 1;
 }
 
-blk = img_open(image_opts, filename, fmt, flags, writethrough, quiet);
+blk = img_open(image_o

[Qemu-block] [PATCH v15 03/21] block: Add, parse and store "force-share" option

[Fam Zheng]

Signed-off-by: Fam Zheng 
---
 block.c   | 17 +
 include/block/block.h |  1 +
 include/block/block_int.h |  1 +
 qapi/block-core.json  |  3 +++
 4 files changed, 22 insertions(+)

diff --git a/block.c b/block.c
index fce77bf..9db39b6 100644
--- a/block.c
+++ b/block.c
@@ -763,6 +763,7 @@ static void bdrv_inherited_options(int *child_flags, QDict 
*child_options,
  * the parent. */
 qdict_copy_default(child_options, parent_options, BDRV_OPT_CACHE_DIRECT);
 qdict_copy_default(child_options, parent_options, BDRV_OPT_CACHE_NO_FLUSH);
+qdict_copy_default(child_options, parent_options, BDRV_OPT_FORCE_SHARE);
 
 /* Inherit the read-only option from the parent if it's not set */
 qdict_copy_default(child_options, parent_options, BDRV_OPT_READ_ONLY);
@@ -871,6 +872,7 @@ static void bdrv_backing_options(int *child_flags, QDict 
*child_options,
  * which is only applied on the top level (BlockBackend) */
 qdict_copy_default(child_options, parent_options, BDRV_OPT_CACHE_DIRECT);
 qdict_copy_default(child_options, parent_options, BDRV_OPT_CACHE_NO_FLUSH);
+qdict_copy_default(child_options, parent_options, BDRV_OPT_FORCE_SHARE);
 
 /* backing files always opened read-only */
 qdict_set_default_str(child_options, BDRV_OPT_READ_ONLY, "on");
@@ -1115,6 +1117,11 @@ QemuOptsList bdrv_runtime_opts = {
 .type = QEMU_OPT_STRING,
 .help = "discard operation (ignore/off, unmap/on)",
 },
+{
+.name = BDRV_OPT_FORCE_SHARE,
+.type = QEMU_OPT_BOOL,
+.help = "always accept other writers (default: off)",
+},
 { /* end of list */ }
 },
 };
@@ -1154,6 +1161,16 @@ static int bdrv_open_common(BlockDriverState *bs, 
BlockBackend *file,
 drv = bdrv_find_format(driver_name);
 assert(drv != NULL);
 
+bs->force_share = qemu_opt_get_bool(opts, BDRV_OPT_FORCE_SHARE, false);
+
+if (bs->force_share && (bs->open_flags & BDRV_O_RDWR)) {
+error_setg(errp,
+   BDRV_OPT_FORCE_SHARE
+   "=on can only be used with read-only images");
+ret = -EINVAL;
+goto fail_opts;
+}
+
 if (file != NULL) {
 filename = blk_bs(file)->filename;
 } else {
diff --git a/include/block/block.h b/include/block/block.h
index a798f10..feae379 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -109,6 +109,7 @@ typedef struct HDGeometry {
 #define BDRV_OPT_CACHE_NO_FLUSH "cache.no-flush"
 #define BDRV_OPT_READ_ONLY  "read-only"
 #define BDRV_OPT_DISCARD"discard"
+#define BDRV_OPT_FORCE_SHARE"force-share"
 
 
 #define BDRV_SECTOR_BITS   9
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 59400bd..94ba697 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -518,6 +518,7 @@ struct BlockDriverState {
 bool valid_key; /* if true, a valid encryption key has been set */
 bool sg;/* if true, the device is a /dev/sg* */
 bool probed;/* if true, format was probed rather than specified */
+bool force_share; /* if true, always allow all shared permissions */
 
 BlockDriver *drv; /* NULL means no media */
 void *opaque;
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 033457c..5bc56969 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2880,6 +2880,8 @@
 # (default: false)
 # @detect-zeroes: detect and optimize zero writes (Since 2.1)
 # (default: off)
+# @force-share:   force share all permission on added nodes.
+# Requires read-only=true. (Since 2.10)
 #
 # Remaining options are determined by the block driver.
 #
@@ -2891,6 +2893,7 @@
 '*discard': 'BlockdevDiscardOptions',
 '*cache': 'BlockdevCacheOptions',
 '*read-only': 'bool',
+'*force-share': 'bool',
 '*detect-zeroes': 'BlockdevDetectZeroesOptions' },
   'discriminator': 'driver',
   'data': {
-- 
2.9.3

[Qemu-block] [PATCH v15 00/21] block: Image locking series

[Fam Zheng]

v15: Rework a number of things, especially around what and how lockings are
 done.  [Kevin]
 - Map each permission to a locked byte.
 - Make the new option --force-share-perms, and require read-only=on.
 - Update test case 153 accordingly.
 - Only add -U where necessary in iotest 030.
 - Reject -U if conflicting with image opts in qemu-img/qemu-io.

v14: Replace BDRV_ flag with the "force-shared-write" block option. [Kevin]
 Add bs->force_shared_write.
 Update test case accordingly.
 A few fixes in the locking code spotted by patchew and the new test,
 though the long line error is still there for readability.
 Replace the workaround to drive-backup with a real fix in patch 17.

v13: - Address Max's comments.
 - Add reviewed-by from Max and Eric.
 - Rebase for 2.10:
   * Use op blocker API
   * Add --unsafe-read for qemu-img and qemu-io

Fam Zheng (21):
  block: Make bdrv_perm_names public
  block: Define BLK_PERM_MAX
  block: Add, parse and store "force-share" option
  block: Respect "force-share" in perm propagating
  qemu-img: Add --force-share option to subcommands
  qemu-img: Update documentation for -U
  qemu-io: Add --force-share option
  iotests: 030: Prepare for image locking
  iotests: 046: Prepare for image locking
  iotests: 055: Don't attach the target image already for drive-backup
  iotests: 085: Avoid image locking conflict
  iotests: 087: Don't attach test image twice
  iotests: 091: Quit QEMU before checking image
  iotests: 172: Use separate images for multiple devices
  tests: Use null-co:// instead of /dev/null as the dummy image
  file-posix: Add 'locking' option
  tests: Disable image lock in test-replication
  block: Reuse bs as backing hd for drive-backup sync=none
  osdep: Add qemu_lock_fd and qemu_unlock_fd
  file-posix: Add image locking to perm operations
  qemu-iotests: Add test case 153 for image locking

 block.c|  51 --
 block/file-posix.c | 272 ++-
 blockdev.c |  15 +-
 include/block/block.h  |   5 +
 include/block/block_int.h  |   1 +
 include/qemu/osdep.h   |   3 +
 qapi/block-core.json   |   3 +
 qemu-img-cmds.hx   |  36 ++---
 qemu-img.c | 154 +-
 qemu-io.c  |  42 -
 tests/drive_del-test.c |   2 +-
 tests/nvme-test.c  |   2 +-
 tests/qemu-iotests/030 |  18 +--
 tests/qemu-iotests/046 |   2 +-
 tests/qemu-iotests/055 |  32 ++--
 tests/qemu-iotests/085 |  34 ++--
 tests/qemu-iotests/085.out |   3 +-
 tests/qemu-iotests/087 |   6 +-
 tests/qemu-iotests/091 |   2 +
 tests/qemu-iotests/153 | 220 +
 tests/qemu-iotests/153.out | 390 +
 tests/qemu-iotests/172 |  55 ---
 tests/qemu-iotests/172.out |  50 +++---
 tests/qemu-iotests/group   |   1 +
 tests/test-replication.c   |   9 +-
 tests/usb-hcd-uhci-test.c  |   2 +-
 tests/usb-hcd-xhci-test.c  |   2 +-
 tests/virtio-blk-test.c|   2 +-
 tests/virtio-scsi-test.c   |   5 +-
 util/osdep.c   |  48 ++
 30 files changed, 1291 insertions(+), 176 deletions(-)
 create mode 100755 tests/qemu-iotests/153
 create mode 100644 tests/qemu-iotests/153.out

-- 
2.9.3

[Qemu-block] [PATCH v15 01/21] block: Make bdrv_perm_names public

[Fam Zheng]

It can be used outside of block.c for making user friendly messages.

Signed-off-by: Fam Zheng 
---
 block.c   | 2 +-
 include/block/block.h | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 1e668fb..fce77bf 100644
--- a/block.c
+++ b/block.c
@@ -1551,7 +1551,7 @@ static char *bdrv_child_user_desc(BdrvChild *c)
 return g_strdup("another user");
 }
 
-static char *bdrv_perm_names(uint64_t perm)
+char *bdrv_perm_names(uint64_t perm)
 {
 struct perm_name {
 uint64_t perm;
diff --git a/include/block/block.h b/include/block/block.h
index 5ddc0cf..eb0565d 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -224,6 +224,8 @@ enum {
 BLK_PERM_ALL= 0x1f,
 };
 
+char *bdrv_perm_names(uint64_t perm);
+
 /* disk I/O throttling */
 void bdrv_init(void);
 void bdrv_init_with_whitelist(void);
-- 
2.9.3

[Qemu-block] [PATCH] block: Remove NULL check in bdrv_co_flush

[Fam Zheng]

Reported by Coverity. We already use bs in bdrv_inc_in_flight before
checking for NULL. It is unnecessary as all callers pass non-NULL bs, so
drop it.

Signed-off-by: Fam Zheng 
---
 block/io.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/io.c b/block/io.c
index a7142e0..a039966 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2308,7 +2308,7 @@ int coroutine_fn bdrv_co_flush(BlockDriverState *bs)
 
 bdrv_inc_in_flight(bs);
 
-if (!bs || !bdrv_is_inserted(bs) || bdrv_is_read_only(bs) ||
+if (!bdrv_is_inserted(bs) || bdrv_is_read_only(bs) ||
 bdrv_is_sg(bs)) {
 goto early_exit;
 }
-- 
2.9.3

Re: [Qemu-block] [PATCH for 2.9 v3 10/10] block: Fix bdrv_co_flush early return

[Fam Zheng]

On Tue, 04/25 17:16, Kevin Wolf wrote:
> Am 10.04.2017 um 17:05 hat Fam Zheng geschrieben:
> > bdrv_inc_in_flight and bdrv_dec_in_flight are mandatory for
> > BDRV_POLL_WHILE to work, even for the shortcut case where flush is
> > unnecessary. Move the if block to below bdrv_dec_in_flight, and BTW fix
> > the variable declaration position.
> > 
> > Signed-off-by: Fam Zheng 
> > ---
> >  block/io.c | 16 +---
> >  1 file changed, 9 insertions(+), 7 deletions(-)
> > 
> > diff --git a/block/io.c b/block/io.c
> > index 00e45ca..bae6947 100644
> > --- a/block/io.c
> > +++ b/block/io.c
> > @@ -2278,16 +2278,17 @@ static void coroutine_fn bdrv_flush_co_entry(void 
> > *opaque)
> >  
> >  int coroutine_fn bdrv_co_flush(BlockDriverState *bs)
> >  {
> > -int ret;
> > -
> > -if (!bs || !bdrv_is_inserted(bs) || bdrv_is_read_only(bs) ||
> > -bdrv_is_sg(bs)) {
> > -return 0;
> > -}
> > +int current_gen;
> > +int ret = 0;
> >  
> >  bdrv_inc_in_flight(bs);
> 
> As Coverity points out, we're now using bs...
> 
> > -int current_gen = bs->write_gen;
> > +if (!bs || !bdrv_is_inserted(bs) || bdrv_is_read_only(bs) ||
> 
> ...before doing the NULL check.
> 
> I'm not sure if we even need to have a NULL check here, but we would have
> to check all callers to make sure that it's unnecessary. Before commit
> 29cdb251, it only checked bs->drv and I don't see how that commit
> introduced a NULL caller, but maybe one was added later.
> 
> In any case, bdrv_co_flush() needs a fix, either remove the NULL check
> or do it first.

After auditing the callers and knowing the fact that the above
bdrv_inc_in_flight didn't cause a problem, I think removing the NULL check is
fine.

I'll send a patch.

Thanks.

Fam

> 
> > +bdrv_is_sg(bs)) {
> > +goto early_exit;
> > +}
> 
> Kevin

[Qemu-block] [PATCH v1 7/8] dmg: Refactor dmg_co_preadv() to start reading multiple sectors

[Ashijeet Acharya]

At the moment, dmg_co_preadv() reads one sector at a time. Make it
read multiple sectors at a time depending on the number of sectors
stored in "drs->sectors_read". This does not provide any significant
optimization in the I/O process of DMG but is still a nicer way.

Adjust the 'data' variable depending on our cached access point
situation to align our read requests appropriately.

Signed-off-by: Ashijeet Acharya 
---
 block/dmg.c | 23 +++
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/block/dmg.c b/block/dmg.c
index f643e41..b0f3c84 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -718,7 +718,7 @@ dmg_co_preadv(BlockDriverState *bs, uint64_t offset, 
uint64_t bytes,
 BDRVDMGState *s = bs->opaque;
 uint64_t sector_num = offset >> BDRV_SECTOR_BITS;
 int nb_sectors = bytes >> BDRV_SECTOR_BITS;
-int ret, i;
+int ret, i = 0;
 DMGReadState *drs = s->drs;
 
 assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
@@ -726,8 +726,7 @@ dmg_co_preadv(BlockDriverState *bs, uint64_t offset, 
uint64_t bytes,
 
 qemu_co_mutex_lock(&s->lock);
 
-for (i = 0; i < nb_sectors; i++) {
-uint32_t sector_offset_in_chunk;
+while (i < nb_sectors) {
 void *data;
 
 if (dmg_read_chunk(bs, sector_num + i, drs) != 0) {
@@ -738,12 +737,20 @@ dmg_co_preadv(BlockDriverState *bs, uint64_t offset, 
uint64_t bytes,
  * s->uncompressed_chunk may be too small to cover the large all-zeroes
  * section. dmg_read_chunk is called to find s->current_chunk */
 if (s->types[s->current_chunk] == 2) { /* all zeroes block entry */
-qemu_iovec_memset(qiov, i * 512, 0, 512);
-continue;
+qemu_iovec_memset(qiov, i * 512, 0,
+512 * drs->sectors_read);
+goto increment;
+}
+
+if (drs->saved_next_in == NULL) {
+data = s->uncompressed_chunk + drs->sector_offset_in_chunk * 512;
+} else {
+data = s->uncompressed_chunk;
 }
-sector_offset_in_chunk = sector_num + i - s->sectors[s->current_chunk];
-data = s->uncompressed_chunk + sector_offset_in_chunk * 512;
-qemu_iovec_from_buf(qiov, i * 512, data, 512);
+qemu_iovec_from_buf(qiov, i * 512, data, drs->sectors_read * 512);
+
+increment:
+i += drs->sectors_read;
 }
 
 ret = 0;
-- 
2.6.2

[Qemu-block] [PATCH v1 4/8] dmg: Refactor and prepare dmg_read_chunk() to cache random access points

[Ashijeet Acharya]

Refactor dmg_read_chunk() to start making use of the new DMGReadState
structure and do chunk and sector related calculations based on it.
Add a new argument "DMGReadState *drs" to it.

Signed-off-by: Ashijeet Acharya 
---
 block/dmg.c | 153 
 1 file changed, 91 insertions(+), 62 deletions(-)

diff --git a/block/dmg.c b/block/dmg.c
index 7ae30e3..dc356b0 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -578,78 +578,106 @@ static inline uint32_t search_chunk(BDRVDMGState *s, 
uint64_t sector_num)
 return s->n_chunks; /* error */
 }
 
-static inline int dmg_read_chunk(BlockDriverState *bs, uint64_t sector_num)
+static inline int dmg_read_chunk(BlockDriverState *bs, uint64_t sector_num,
+ DMGReadState *drs)
 {
 BDRVDMGState *s = bs->opaque;
 
+int ret;
+uint32_t sector_offset;
+uint64_t sectors_read;
+uint32_t chunk;
+
 if (!is_sector_in_chunk(s, s->current_chunk, sector_num)) {
-int ret;
-uint32_t chunk = search_chunk(s, sector_num);
+chunk = search_chunk(s, sector_num);
+} else {
+chunk = drs->saved_chunk_type;
+}
 
-if (chunk >= s->n_chunks) {
+if (chunk >= s->n_chunks) {
+return -1;
+}
+
+/* reset our access point cache if we had a change in current chunk */
+if (chunk != drs->saved_chunk_type) {
+cache_access_point(drs, NULL, -1, -1, -1, -1);
+}
+
+sector_offset = sector_num - s->sectors[chunk];
+
+if ((s->sectorcounts[chunk] - sector_offset) > DMG_SECTOR_MAX) {
+sectors_read = DMG_SECTOR_MAX;
+} else {
+sectors_read = s->sectorcounts[chunk] - sector_offset;
+}
+
+/* truncate sectors read if it exceeds the 2MB buffer of qemu-img
+ * convert */
+if ((sector_num % DMG_SECTOR_MAX) + sectors_read > DMG_SECTOR_MAX) {
+sectors_read = DMG_SECTOR_MAX - (sector_num % DMG_SECTOR_MAX);
+}
+
+s->current_chunk = s->n_chunks;
+
+switch (s->types[chunk]) { /* block entry type */
+case 0x8005: { /* zlib compressed */
+/* we need to buffer, because only the chunk as whole can be
+ * inflated. */
+ret = bdrv_pread(bs->file, s->offsets[chunk],
+ s->compressed_chunk, s->lengths[chunk]);
+if (ret != s->lengths[chunk]) {
 return -1;
 }
 
-s->current_chunk = s->n_chunks;
-switch (s->types[chunk]) { /* block entry type */
-case 0x8005: { /* zlib compressed */
-/* we need to buffer, because only the chunk as whole can be
- * inflated. */
-ret = bdrv_pread(bs->file, s->offsets[chunk],
- s->compressed_chunk, s->lengths[chunk]);
-if (ret != s->lengths[chunk]) {
-return -1;
-}
-
-s->zstream.next_in = s->compressed_chunk;
-s->zstream.avail_in = s->lengths[chunk];
-s->zstream.next_out = s->uncompressed_chunk;
-s->zstream.avail_out = 512 * s->sectorcounts[chunk];
-ret = inflateReset(&s->zstream);
-if (ret != Z_OK) {
-return -1;
-}
-ret = inflate(&s->zstream, Z_FINISH);
-if (ret != Z_STREAM_END ||
-s->zstream.total_out != 512 * s->sectorcounts[chunk]) {
-return -1;
-}
-break; }
-case 0x8006: /* bzip2 compressed */
-if (!dmg_uncompress_bz2) {
-break;
-}
-/* we need to buffer, because only the chunk as whole can be
- * inflated. */
-ret = bdrv_pread(bs->file, s->offsets[chunk],
- s->compressed_chunk, s->lengths[chunk]);
-if (ret != s->lengths[chunk]) {
-return -1;
-}
-
-ret = dmg_uncompress_bz2((char *)s->compressed_chunk,
- (unsigned int) s->lengths[chunk],
- (char *)s->uncompressed_chunk,
- (unsigned int)
- (512 * s->sectorcounts[chunk]));
-if (ret < 0) {
-return ret;
-}
-break;
-case 1: /* copy */
-ret = bdrv_pread(bs->file, s->offsets[chunk],
- s->uncompressed_chunk, s->lengths[chunk]);
-if (ret != s->lengths[chunk]) {
-return -1;
-}
-break;
-case 2: /* zero */
-/* see dmg_read, it is treated specially. No buffer needs to be
- * pre-filled, the zeroes can be set directly. */
+s->zstream.next_in = s->compressed_chunk;
+s->zstream.avail_in = s->lengths[chunk];
+s->zstream.next_out = s->uncompressed_chunk;
+s->zstream.avail_out = 512 * s->sectorcounts[chunk];
+

Re: [Qemu-block] [Qemu-devel] [PATCH 1/2] qemu-img: make sure contain the consecutive number of zero bytes

[Max Reitz]

On 25.04.2017 03:50, 858585 jemmy wrote:
> On Mon, Apr 24, 2017 at 10:43 PM, Eric Blake  wrote:
>> On 04/23/2017 09:33 AM, jemmy858...@gmail.com wrote:
>>> From: Lidong Chen 
>>>
>>> is_allocated_sectors_min don't guarantee to contain the
>>> consecutive number of zero bytes. this patch fixes this bug.
>>
>> This message was sent without an 'In-Reply-To' header pointing to a 0/2
>> cover letter.  When sending a series, please always thread things to a
>> cover letter; you may find 'git config format.coverletter auto' to be
>> helpful.
> 
> Thanks for your kind advises.
> 
>>
>>>
>>> Signed-off-by: Lidong Chen 
>>> ---
>>>  qemu-img.c | 11 ++-
>>>  1 file changed, 6 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/qemu-img.c b/qemu-img.c
>>> index b220cf7..df6d165 100644
>>> --- a/qemu-img.c
>>> +++ b/qemu-img.c
>>> @@ -1060,9 +1060,9 @@ static int is_allocated_sectors(const uint8_t *buf, 
>>> int n, int *pnum)
>>>  }
>>>
>>>  /*
>>> - * Like is_allocated_sectors, but if the buffer starts with a used sector,
>>> - * up to 'min' consecutive sectors containing zeros are ignored. This 
>>> avoids
>>> - * breaking up write requests for only small sparse areas.
>>> + * Like is_allocated_sectors, but up to 'min' consecutive sectors
>>> + * containing zeros are ignored. This avoids breaking up write requests
>>> + * for only small sparse areas.
>>>   */
>>>  static int is_allocated_sectors_min(const uint8_t *buf, int n, int *pnum,
>>>  int min)
>>> @@ -1071,11 +1071,12 @@ static int is_allocated_sectors_min(const uint8_t 
>>> *buf, int n, int *pnum,
>>>  int num_checked, num_used;
>>>
>>>  if (n < min) {
>>> -min = n;
>>> +*pnum = n;
>>> +return 1;
>>>  }
>>>
>>>  ret = is_allocated_sectors(buf, n, pnum);
>>> -if (!ret) {
>>> +if (!ret && *pnum >= min) {
>>
>> I seem to recall past attempts to try and patch this function, which
>> were then turned down, although I haven't scrubbed the archives for a
>> quick URL to point to. I'm worried that there are more subtleties here
>> than what you realize.
> 
> Hi Eric:
> Do you mean this URL?
> https://lists.gnu.org/archive/html/qemu-block/2017-01/msg00306.html
> 
> But I think the code is not consistent with qemu-img --help.
> qemu-img --help
>   '-S' indicates the consecutive number of bytes (defaults to 4k) that must
>contain only zeros for qemu-img to create a sparse image during
>conversion. If the number of bytes is 0, the source will not be
> scanned for
>unallocated or zero sectors, and the destination image will always be
>fully allocated.
> 
> another reason:
> if s->has_zero_init is 1(the qcow2 image which have backing_file), the empty
> space at the beginning of the buffer still need write and invoke
> blk_co_pwrite_zeroes.

Right, that is indeed a reason that we may want to behave differently in
these cases. But in other cases this is less efficient.

> and split a single write operation into two just because there is small empty
> space at the beginning.
And then there's the fact that, in my opinion, this is actually a
problem at qcow2 level. Some people are working on improving this (see
e.g. Berto's subcluster RFC, which would allow us to implement
bdrv_co_pwrite_zeroes() below cluster granularity).

All in all, I don't think you can generically circumvent this issue here
for all block drivers. The rule we'd have to implement here is: If some
part of a cluster (to be written) is zero and another is not, we should
write the whole cluster. If a whole cluster is zero, we should issue a
write-zeroes request. But that would mean to check where some buffer
passes a cluster boundary and so on, and on top of this all of it is
qcow2-specific; so there goes the genericity...

Max



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH v1 8/8] dmg: Remove the error messages to allow wild images

[Ashijeet Acharya]

We have refactored the DMG driver to accept and process images
irrespective of their chunk sizes since we now have limit of 2MB on our
output buffer size. Thus QEMU will not allocate huge amounts of memory
no matter what the chunk size is.

Remove the error messages to prevent denial-of-service in cases where
untrusted files are being accessed by the user.

Signed-off-by: Ashijeet Acharya 
---
 block/dmg.c | 22 --
 1 file changed, 22 deletions(-)

diff --git a/block/dmg.c b/block/dmg.c
index b0f3c84..01ec40e 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -209,7 +209,6 @@ static int dmg_read_mish_block(BDRVDMGState *s, 
DmgHeaderState *ds,
uint8_t *buffer, uint32_t count)
 {
 uint32_t type, i;
-int ret;
 size_t new_size;
 uint32_t chunk_count;
 int64_t offset = 0;
@@ -258,16 +257,6 @@ static int dmg_read_mish_block(BDRVDMGState *s, 
DmgHeaderState *ds,
 /* sector count */
 s->sectorcounts[i] = buff_read_uint64(buffer, offset + 0x10);
 
-/* all-zeroes sector (type 2) does not need to be "uncompressed" and 
can
- * therefore be unbounded. */
-if (s->types[i] != 2 && s->sectorcounts[i] > DMG_SECTOR_MAX) {
-error_report("sector count %" PRIu64 " for chunk %" PRIu32
- " is larger than max (%u)",
- s->sectorcounts[i], i, DMG_SECTOR_MAX);
-ret = -EINVAL;
-goto fail;
-}
-
 /* offset in (compressed) data fork */
 s->offsets[i] = buff_read_uint64(buffer, offset + 0x18);
 s->offsets[i] += in_offset;
@@ -275,23 +264,12 @@ static int dmg_read_mish_block(BDRVDMGState *s, 
DmgHeaderState *ds,
 /* length in (compressed) data fork */
 s->lengths[i] = buff_read_uint64(buffer, offset + 0x20);
 
-if (s->lengths[i] > DMG_MAX_OUTPUT) {
-error_report("length %" PRIu64 " for chunk %" PRIu32
- " is larger than max (%u)",
- s->lengths[i], i, DMG_MAX_OUTPUT);
-ret = -EINVAL;
-goto fail;
-}
-
 update_max_chunk_size(s, i, &ds->max_compressed_size,
   &ds->max_sectors_per_chunk);
 offset += 40;
 }
 s->n_chunks += chunk_count;
 return 0;
-
-fail:
-return ret;
 }
 
 static int dmg_read_resource_fork(BlockDriverState *bs, DmgHeaderState *ds,
-- 
2.6.2

[Qemu-block] [PATCH v1 5/8] dmg: Handle zlib compressed chunks

[Ashijeet Acharya]

Set the output buffer size to be equal to the size of number of sectors
stored in @sectors_read. Start inflating to a max output buffer size of
2MB and cache our access point to aid random access later if required.

Signed-off-by: Ashijeet Acharya 
---
 block/dmg.c | 48 ++--
 1 file changed, 34 insertions(+), 14 deletions(-)

diff --git a/block/dmg.c b/block/dmg.c
index dc356b0..749c151 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -621,27 +621,47 @@ static inline int dmg_read_chunk(BlockDriverState *bs, 
uint64_t sector_num,
 
 switch (s->types[chunk]) { /* block entry type */
 case 0x8005: { /* zlib compressed */
-/* we need to buffer, because only the chunk as whole can be
- * inflated. */
-ret = bdrv_pread(bs->file, s->offsets[chunk],
- s->compressed_chunk, s->lengths[chunk]);
-if (ret != s->lengths[chunk]) {
-return -1;
+/* check for cached random access point */
+if (drs->saved_next_in == NULL) {
+/* we need to buffer, because only the chunk as whole can be
+ * inflated. */
+ret = bdrv_pread(bs->file, s->offsets[chunk],
+ s->compressed_chunk, s->lengths[chunk]);
+if (ret != s->lengths[chunk]) {
+return -1;
+}
+
+s->zstream.next_in = s->compressed_chunk;
+s->zstream.avail_in = s->lengths[chunk];
+} else {
+s->zstream.next_in = drs->saved_next_in;
+s->zstream.avail_in = drs->saved_avail_in;
 }
 
-s->zstream.next_in = s->compressed_chunk;
-s->zstream.avail_in = s->lengths[chunk];
 s->zstream.next_out = s->uncompressed_chunk;
-s->zstream.avail_out = 512 * s->sectorcounts[chunk];
-ret = inflateReset(&s->zstream);
-if (ret != Z_OK) {
-return -1;
+
+s->zstream.avail_out = sectors_read * BDRV_SECTOR_SIZE;
+
+if (drs->saved_next_in == NULL) {
+ret = inflateReset(&s->zstream);
+if (ret != Z_OK) {
+return -1;
+}
+}
+/* reset total_out for each successive call */
+s->zstream.total_out = 0;
+ret = inflate(&s->zstream, Z_SYNC_FLUSH);
+if (ret == Z_OK &&
+s->zstream.total_out == 512 * sectors_read) {
+goto update;
 }
-ret = inflate(&s->zstream, Z_FINISH);
 if (ret != Z_STREAM_END ||
-s->zstream.total_out != 512 * s->sectorcounts[chunk]) {
+s->zstream.total_out != 512 * sectors_read) {
 return -1;
 }
+update:
+cache_access_point(drs, s->zstream.next_in, s->zstream.avail_in,
+   chunk, sectors_read, sector_offset);
 break; }
 case 0x8006: /* bzip2 compressed */
 if (!dmg_uncompress_bz2) {
-- 
2.6.2

[Qemu-block] [PATCH v1 3/8] dmg: Limit the output buffer size to a max of 2MB

[Ashijeet Acharya]

The size of the output buffer is limited to a maximum of 2MB so that
QEMU doesn't end up allocating huge amounts of memory while
decompressing compressed input streams.

2MB is an appropriate size because "qemu-img convert" has the same I/O
buffer size and the most important use case for DMG files is to be
compatible with qemu-img convert.

Signed-off-by: Ashijeet Acharya 
---
 block/dmg.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/block/dmg.c b/block/dmg.c
index c6fe8b0..7ae30e3 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -37,8 +37,8 @@ enum {
 /* Limit chunk sizes to prevent unreasonable amounts of memory being used
  * or truncating when converting to 32-bit types
  */
-DMG_LENGTHS_MAX = 64 * 1024 * 1024, /* 64 MB */
-DMG_SECTORCOUNTS_MAX = DMG_LENGTHS_MAX / 512,
+DMG_MAX_OUTPUT = 2 * 1024 * 1024, /* 2 MB */
+DMG_SECTOR_MAX = DMG_MAX_OUTPUT / 512,
 };
 
 static int dmg_probe(const uint8_t *buf, int buf_size, const char *filename)
@@ -260,10 +260,10 @@ static int dmg_read_mish_block(BDRVDMGState *s, 
DmgHeaderState *ds,
 
 /* all-zeroes sector (type 2) does not need to be "uncompressed" and 
can
  * therefore be unbounded. */
-if (s->types[i] != 2 && s->sectorcounts[i] > DMG_SECTORCOUNTS_MAX) {
+if (s->types[i] != 2 && s->sectorcounts[i] > DMG_SECTOR_MAX) {
 error_report("sector count %" PRIu64 " for chunk %" PRIu32
  " is larger than max (%u)",
- s->sectorcounts[i], i, DMG_SECTORCOUNTS_MAX);
+ s->sectorcounts[i], i, DMG_SECTOR_MAX);
 ret = -EINVAL;
 goto fail;
 }
@@ -275,10 +275,10 @@ static int dmg_read_mish_block(BDRVDMGState *s, 
DmgHeaderState *ds,
 /* length in (compressed) data fork */
 s->lengths[i] = buff_read_uint64(buffer, offset + 0x20);
 
-if (s->lengths[i] > DMG_LENGTHS_MAX) {
+if (s->lengths[i] > DMG_MAX_OUTPUT) {
 error_report("length %" PRIu64 " for chunk %" PRIu32
  " is larger than max (%u)",
- s->lengths[i], i, DMG_LENGTHS_MAX);
+ s->lengths[i], i, DMG_MAX_OUTPUT);
 ret = -EINVAL;
 goto fail;
 }
-- 
2.6.2

[Qemu-block] [PATCH v1 6/8] dmg: Handle bz2 compressed/raw/zeroed chunks

[Ashijeet Acharya]

We do not need to cache the access point for these chunks but need to
update our various supporting variables like chunk, sectors_read etc.
to keep maintaining our code structure. Call cache_access_point() after
reading chunks of these types.

Signed-off-by: Ashijeet Acharya 
---
 block/dmg.c | 18 ++
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/block/dmg.c b/block/dmg.c
index 749c151..f643e41 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -680,20 +680,30 @@ update:
  (char *)s->uncompressed_chunk,
  (unsigned int)
 (512 * s->sectorcounts[chunk]));
+
 if (ret < 0) {
 return ret;
 }
+cache_access_point(drs, NULL, -1, chunk, sectors_read,
+   sector_offset);
 break;
 case 1: /* copy */
-ret = bdrv_pread(bs->file, s->offsets[chunk],
- s->uncompressed_chunk, s->lengths[chunk]);
-if (ret != s->lengths[chunk]) {
-return -1;
+if (drs->sectors_read == -1) {
+ret = bdrv_pread(bs->file, s->offsets[chunk],
+ s->uncompressed_chunk, s->lengths[chunk]);
+if (ret != s->lengths[chunk]) {
+return -1;
+}
 }
+cache_access_point(drs, NULL, -1, chunk, sectors_read,
+   sector_offset);
 break;
 case 2: /* zero */
 /* see dmg_read, it is treated specially. No buffer needs to be
  * pre-filled, the zeroes can be set directly. */
+cache_access_point(drs, NULL, -1, chunk, sectors_read,
+   sector_offset);
+
 break;
 }
 s->current_chunk = chunk;
-- 
2.6.2

[Qemu-block] [PATCH v1 0/8] Refactor DMG driver to have chunk size independence

[Ashijeet Acharya]

This series helps to provide chunk size independence for DMG driver to prevent
denial-of-service in cases where untrusted files are being accessed by the user.

This task is mentioned on the public block ToDo
Here -> http://wiki.qemu.org/ToDo/Block/DmgChunkSizeIndependence

Patch 1 introduces a new data structure to aid caching of random access points
within a compressed stream.

Patch 2 is an extension of patch 1 and introduces a new function to
initialize/update/reset our cached random access point.

Patch 3 limits the output buffer size to a max of 2MB to avoid QEMU allocate
huge amounts of memory.

Patch 4 is a simple preparatory patch to aid handling of various types of 
chunks.

Patches 5 & 6 help to handle various types of chunks.

Patch 7 simply refactors dmg_co_preadv() to read multiple sectors at once.

Patch 8 finally removes the error messages QEMU used to throw when an image with
chunk sizes above 64MB were accessed by the user.

->Testing procedure:
Convert a DMG file to raw format using the "qemu-img convert" tool present in
v2.9.0
Next convert the same image again after applying these patches.
Compare the two images using "qemu-img compare" tool to check if they are
identical.

You can pickup any DMG image from the collection present
Here -> https://lists.gnu.org/archive/html/qemu-devel/2014-12/msg03606.html

->Important note:
These patches assume that the terms "chunk" and "block" are synonyms of each 
other
when we talk about bz2 compressed streams. Thus according to the bz2 docs[1],
the max uncompressed size of a chunk/block can reach to 46MB which is less than
the previously allowed size of 64MB, so we can continue decompressing the whole
chunk/block at once instead of partial decompression just like we do now.

This limitation was forced by the fact that bz2 compressed streams do not allow
random access midway through a chunk/block as the BZ2_bzDecompress() API in 
bzlib
seeks for the magic key "BZh" before starting decompression.[2] This magic key 
is
present at the start of every chunk/block only and since our cached random 
access
points need not necessarily point to the start of a chunk/block, 
BZ2_bzDecompress()
fails with an error value BZ_DATA_ERROR_MAGIC[3]

[1] https://en.wikipedia.org/wiki/Bzip2#File_format
[2] https://blastedbio.blogspot.in/2011/11/random-access-to-bzip2.html
[3] http://linux.math.tifr.res.in/manuals/html/manual_3.html#SEC17

Special thanks to Peter Wu for helping me understand and tackle the bz2
compressed chunks.

Ashijeet Acharya (8):
  dmg: Introduce a new struct to cache random access points
  dmg: New function to help us cache random access point
  dmg: Limit the output buffer size to a max of 2MB
  dmg: Refactor and prepare dmg_read_chunk() to cache random access
points
  dmg: Handle zlib compressed chunks
  dmg: Handle bz2 compressed/raw/zeroed chunks
  dmg: Refactor dmg_co_preadv() to start reading multiple sectors
  dmg: Remove the error messages to allow wild images

 block/dmg.c | 214 +++-
 block/dmg.h |  10 +++
 2 files changed, 148 insertions(+), 76 deletions(-)

-- 
2.6.2

[Qemu-block] [PATCH v1 1/8] dmg: Introduce a new struct to cache random access points

[Ashijeet Acharya]

We need to cache the random access point while performing partial
decompression so that we can resume decompression from that point
onwards in our next sequential read request. Introduce a new struct
DMGReadState which will help us do this.

Signed-off-by: Ashijeet Acharya 
---
 block/dmg.h | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/block/dmg.h b/block/dmg.h
index b592d6f..ee67ae1 100644
--- a/block/dmg.h
+++ b/block/dmg.h
@@ -31,6 +31,15 @@
 #include "block/block_int.h"
 #include 
 
+/* used to cache current position in compressed input stream */
+typedef struct DMGReadState {
+uint8_t *saved_next_in;
+int64_t saved_avail_in;
+int32_t saved_chunk_type;
+int64_t sectors_read; /* possible sectors read in each cycle */
+int32_t sector_offset_in_chunk;
+} DMGReadState;
+
 typedef struct BDRVDMGState {
 CoMutex lock;
 /* each chunk contains a certain number of sectors,
@@ -51,6 +60,7 @@ typedef struct BDRVDMGState {
 uint8_t *compressed_chunk;
 uint8_t *uncompressed_chunk;
 z_stream zstream;
+DMGReadState *drs;
 } BDRVDMGState;
 
 extern int (*dmg_uncompress_bz2)(char *next_in, unsigned int avail_in,
-- 
2.6.2

[Qemu-block] [PATCH v1 2/8] dmg: New function to help us cache random access point

[Ashijeet Acharya]

Introduce a new cache_access_point() function which will help us first
cache a random access point inside a compressed stream and then keep
updating it according to our requirement at appropriate times.

Signed-off-by: Ashijeet Acharya 
---
 block/dmg.c | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/block/dmg.c b/block/dmg.c
index a7d25fc..c6fe8b0 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -128,6 +128,18 @@ static void update_max_chunk_size(BDRVDMGState *s, 
uint32_t chunk,
 }
 }
 
+static void cache_access_point(DMGReadState *drs, uint8_t *next_in,
+int64_t avail_in, int32_t chunk,
+int64_t sectors_read, int32_t sector_offset)
+{
+drs->saved_next_in = next_in;
+drs->saved_avail_in = avail_in;
+drs->saved_chunk_type = chunk;
+drs->sectors_read = sectors_read;
+drs->sector_offset_in_chunk = sector_offset;
+return;
+}
+
 static int64_t dmg_find_koly_offset(BdrvChild *file, Error **errp)
 {
 BlockDriverState *file_bs = file->bs;
@@ -507,6 +519,10 @@ static int dmg_open(BlockDriverState *bs, QDict *options, 
int flags,
 goto fail;
 }
 
+s->drs = g_malloc(sizeof(DMGReadState));
+/* initialise our access point cache */
+cache_access_point(s->drs, NULL, -1, -1, -1, -1);
+
 if (inflateInit(&s->zstream) != Z_OK) {
 ret = -EINVAL;
 goto fail;
@@ -523,6 +539,7 @@ fail:
 g_free(s->lengths);
 g_free(s->sectors);
 g_free(s->sectorcounts);
+g_free(s->drs);
 qemu_vfree(s->compressed_chunk);
 qemu_vfree(s->uncompressed_chunk);
 return ret;
@@ -685,6 +702,7 @@ static void dmg_close(BlockDriverState *bs)
 g_free(s->lengths);
 g_free(s->sectors);
 g_free(s->sectorcounts);
+g_free(s->drs);
 qemu_vfree(s->compressed_chunk);
 qemu_vfree(s->uncompressed_chunk);
 
-- 
2.6.2

Re: [Qemu-block] [Qemu-devel] [PATCH 1/2] qemu-img: make sure contain the consecutive number of zero bytes

[Eric Blake]

On 04/24/2017 08:50 PM, 858585 jemmy wrote:
> On Mon, Apr 24, 2017 at 10:43 PM, Eric Blake  wrote:
>> On 04/23/2017 09:33 AM, jemmy858...@gmail.com wrote:
>>> From: Lidong Chen 
>>>
>>> is_allocated_sectors_min don't guarantee to contain the
>>> consecutive number of zero bytes. this patch fixes this bug.
>>
>> This message was sent without an 'In-Reply-To' header pointing to a 0/2
>> cover letter.  When sending a series, please always thread things to a
>> cover letter; you may find 'git config format.coverletter auto' to be
>> helpful.
> 
> Thanks for your kind advises.
> 

>>
>> I seem to recall past attempts to try and patch this function, which
>> were then turned down, although I haven't scrubbed the archives for a
>> quick URL to point to. I'm worried that there are more subtleties here
>> than what you realize.
> 
> Hi Eric:
> Do you mean this URL?
> https://lists.gnu.org/archive/html/qemu-block/2017-01/msg00306.html

Yes, that's probably the one.

> 
> But I think the code is not consistent with qemu-img --help.
> qemu-img --help
>   '-S' indicates the consecutive number of bytes (defaults to 4k) that must
>contain only zeros for qemu-img to create a sparse image during
>conversion. If the number of bytes is 0, the source will not be
> scanned for
>unallocated or zero sectors, and the destination image will always be
>fully allocated.

If you still think this patch is needed, the best way to convince me of
it is accompany your patch by a qemu-iotests enhancement that covers the
change in behavior (running the test pre-patch would show that we are
broken without the patch, and having the test ensures we can't later
regress).  That's a lot more work than the vague two lines of the commit
message body.

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH 2/2] qemu-img: fix some spelling errors

[Max Reitz]

On 24.04.2017 17:53, Eric Blake wrote:
> On 04/24/2017 10:47 AM, Eric Blake wrote:
>> On 04/24/2017 10:37 AM, Philippe Mathieu-Daudé wrote:
>>
>  /*
> - * Returns true iff the first sector pointed to by 'buf' contains at
> least
> - * a non-NUL byte.
> + * Returns true if the first sector pointed to by 'buf' contains at
> least
> + * a non-NULL byte.

 NACK to both changes.  'iff' is an English word that is shorthand for
 "if and only if".  "NUL" means the one-byte character, while "NULL"
 means the 8-byte (or 4-byte, on 32-bit platform) pointer value.
>>>
>>> I agree with Lidong shorthands are not obvious from non-native speaker.
>>>
>>> What about this?
>>>
>>>  * Returns true if (and only if) the first sector pointed to by 'buf'
>>> contains
>>
>> That might be okay.

Might, yes, but we have it all over the code. I'm not particularly avid
to change this, because I am in fact one of the culprits (and I'm a
non-native speaker, but I do like to use LaTeX so I know my \iff).

(By the way, judging from the author's name of this line of code (which
is Thiemo Seufer), I'd wager he's not a native speaker either.)

>>>  * at least a non-null character.
>>
>> But that still doesn't make sense.  The character name is NUL, and
>> non-NULL refers to something that is a pointer, not a character.
> 
> What's more, the NUL character can actually occupy more than one byte
> (think UTF-16, where it is the two-byte 0 value).  Referring to NUL byte
> rather than NUL character (or even the 'zero byte') makes it obvious
> that this function is NOT encoding-sensitive, and doesn't start
> mis-behaving just because the data picks a multi-byte character encoding.

Furthermore, this doesn't have anything to do with being a native
speaker or not: NUL is just the commonly used and probably standardized
abbreviation of a certain ASCII character (in any language). It's OK not
to know this, but I don't think it's OK to change the comment.

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH v6 00/18]Convert QCow[2] to QCryptoBlock & add LUKS support

[no-reply]

Hi,

This series failed automatic build test. Please find the testing commands and
their output below. If you have docker installed, you can probably reproduce it
locally.

Message-id: 20170425153858.25660-1-berra...@redhat.com
Subject: [Qemu-devel] [PATCH v6 00/18]Convert QCow[2] to QCryptoBlock & add 
LUKS support
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
set -e
git submodule update --init dtc
# Let docker tests dump environment info
export SHOW_ENV=1
export J=8
make docker-test-quick@centos6
make docker-test-mingw@fedora
make docker-test-build@min-glib
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
5853c61 block: pass option prefix down to crypto layer
d520ee5 block: remove all encryption handling APIs
21831df block: rip out all traces of password prompting
b8525d8 iotests: enable tests 134 and 158 to work with qcow (v1)
f921b52 qcow2: add iotests to cover LUKS encryption support
24921c2 qcow2: add support for LUKS encryption format
ce3d3ef qcow2: extend specification to cover LUKS encryption
d34f397 qcow2: convert QCow2 to use QCryptoBlock for encryption
b4642d4 qcow2: make qcow2_encrypt_sectors encrypt in place
a7264fa qcow: convert QCow to use QCryptoBlock for encryption
0921b2b qcow: make encrypt_sectors encrypt in place
a29311e block: deprecate "encryption=on" in favour of "encrypt.format=aes"
73d4e25 iotests: skip 048 with qcow which doesn't support resize
85013dc iotests: skip 042 with qcow which dosn't support zero sized images
c2ec616 qcow: require image size to be > 1 for new images
bf1689f qcow: document another weakness of qcow AES encryption
7b1c8e1 block: add ability to set a prefix for opt names
7308d51 block: expose crypto option names / defs to other drivers

=== OUTPUT BEGIN ===
Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc'
Cloning into '/var/tmp/patchew-tester-tmp-8jhc2op1/src/dtc'...
Submodule path 'dtc': checked out '558cd81bdd432769b59bff01240c44f82cfb1a9d'
  BUILD   centos6
make[1]: Entering directory '/var/tmp/patchew-tester-tmp-8jhc2op1/src'
  ARCHIVE qemu.tgz
  ARCHIVE dtc.tgz
  COPYRUNNER
RUN test-quick in qemu:centos6 
Packages installed:
SDL-devel-1.2.14-7.el6_7.1.x86_64
ccache-3.1.6-2.el6.x86_64
epel-release-6-8.noarch
gcc-4.4.7-17.el6.x86_64
git-1.7.1-4.el6_7.1.x86_64
glib2-devel-2.28.8-5.el6.x86_64
libfdt-devel-1.4.0-1.el6.x86_64
make-3.81-23.el6.x86_64
package g++ is not installed
pixman-devel-0.32.8-1.el6.x86_64
tar-1.23-15.el6_8.x86_64
zlib-devel-1.2.3-29.el6.x86_64

Environment variables:
PACKAGES=libfdt-devel ccache tar git make gcc g++ zlib-devel 
glib2-devel SDL-devel pixman-devel epel-release
HOSTNAME=02c8f9307716
TERM=xterm
MAKEFLAGS= -j8
HISTSIZE=1000
J=8
USER=root
CCACHE_DIR=/var/tmp/ccache
EXTRA_CONFIGURE_OPTS=
V=
SHOW_ENV=1
MAIL=/var/spool/mail/root
PATH=/usr/lib/ccache:/usr/lib64/ccache:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
PWD=/
LANG=en_US.UTF-8
TARGET_LIST=
HISTCONTROL=ignoredups
SHLVL=1
HOME=/root
TEST_DIR=/tmp/qemu-test
LOGNAME=root
LESSOPEN=||/usr/bin/lesspipe.sh %s
FEATURES= dtc
DEBUG=
G_BROKEN_FILENAMES=1
CCACHE_HASHDIR=
_=/usr/bin/env

Configure options:
--enable-werror --target-list=x86_64-softmmu,aarch64-softmmu 
--prefix=/var/tmp/qemu-build/install
grep: scripts/tracetool/backend/*.py: No such file or directory
No C++ compiler available; disabling C++ specific optional code
Install prefix/var/tmp/qemu-build/install
BIOS directory/var/tmp/qemu-build/install/share/qemu
binary directory  /var/tmp/qemu-build/install/bin
library directory /var/tmp/qemu-build/install/lib
module directory  /var/tmp/qemu-build/install/lib/qemu
libexec directory /var/tmp/qemu-build/install/libexec
include directory /var/tmp/qemu-build/install/include
config directory  /var/tmp/qemu-build/install/etc
local state directory   /var/tmp/qemu-build/install/var
Manual directory  /var/tmp/qemu-build/install/share/man
ELF interp prefix /usr/gnemul/qemu-%M
Source path   /tmp/qemu-test/src
C compilercc
Host C compiler   cc
C++ compiler  
Objective-C compiler cc
ARFLAGS   rv
CFLAGS-O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -g 
QEMU_CFLAGS   -I/usr/include/pixman-1   -I$(SRC_PATH)/dtc/libfdt -pthread 
-I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include   -fPIE -DPIE -m64 -mcx16 
-D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes 
-Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes 
-fno-strict-aliasing -fno-common -fwrapv  -Wendif-labels 
-Wno-missing-include-dirs -Wempty-body -Wnested-externs -Wformat-security 
-Wformat-y2k -Winit-self -Wignored-qualifiers -Wold-style-declaration 
-Wold-style-definition -Wtype-limits -fstack-protector-all
LDFLAGS   -Wl,--warn-common -Wl,-z,relro -Wl,-z,now -pie -m64 -g 
make  make
install   install
pythonpython -B
smbd  /usr/sbin/smbd
module supportno
host CPU   

Re: [Qemu-block] [Qemu-stable] [PATCH v2 1/1] qemu-img: wait for convert coroutines to complete

[Anton Nefedov]

On 04/25/2017 12:55 PM, Peter Lieven wrote:

Am 24.04.2017 um 22:13 schrieb Anton Nefedov:


On 24/04/2017 21:16, Peter Lieven wrote:




Am 24.04.2017 um 18:27 schrieb Anton Nefedov
:


On 04/21/2017 03:37 PM, Peter Lieven wrote:

Am 21.04.2017 um 14:19 schrieb Anton Nefedov:

On 04/21/2017 01:44 PM, Peter Lieven wrote:

Am 21.04.2017 um 12:04 schrieb Anton Nefedov:
On error path (like i/o error in one of the coroutines), it's
required to
 - wait for coroutines completion before cleaning the common
structures
 - reenter dependent coroutines so they ever finish

Introduced in 2d9187bc65.

Signed-off-by: Anton Nefedov 
---
[..]




And what if we error out in the read path? Wouldn't be something
like this easier?


diff --git a/qemu-img.c b/qemu-img.c
index 22f559a..4ff1085 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1903,6 +1903,16 @@ static int convert_do_copy(ImgConvertState
*s)
main_loop_wait(false);
}

+/* on error path we need to enter all coroutines that are still
+ * running before cleaning up common structures */
+if (s->ret) {
+for (i = 0; i < s->num_coroutines; i++) {
+ if (s->co[i]) {
+ qemu_coroutine_enter(s->co[i]);
+ }
+}
+}
+
if (s->compressed && !s->ret) {
/* signal EOF to align */
ret = blk_pwrite_compressed(s->target, 0, NULL, 0);


Peter



seemed a bit too daring to me to re-enter every coroutine
potentially including the ones that yielded waiting for I/O
completion.
If that's ok - that is for sure easier :)


I think we should enter every coroutine that is still running and
have it terminate correctly. It was my mistake that I have not
done this in the original patch. Can you check if the above fixes
your test cases that triggered the bug?



hi, sorry I'm late with the answer

this segfaults in bdrv_close(). Looks like it tries to finish some
i/o which coroutine we have already entered and terminated?

(gdb) run
Starting program: /vz/anefedov/qemu-build/us/./qemu-img convert -O
[..]



Peter



/Anton



it seems that this is a bit tricky, can you share how your test case
works?

thanks,
peter



how I tested today was basically

diff --git a/qemu-img.c b/qemu-img.c
index 4425aaa..3d2d506 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1788,6 +1788,10 @@ static void coroutine_fn
convert_co_do_copy(void *opaque)

 if (status == BLK_DATA) {
 ret = convert_co_read(s, sector_num, n, buf);
+const uint64_t fsector = 10*1024*1024/512;
+if (sector_num <= fsector && fsector < sector_num+n) {
+ret = -EIO;
+}
 if (ret < 0) {
 error_report("error while reading sector %" PRId64
  ": %s", sector_num, strerror(-ret));



I ended up with sth similar to your approach. Can you check this?


Thanks,

Peter


diff --git a/qemu-img.c b/qemu-img.c
index b94fc11..ed9ce9e 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1761,13 +1761,13 @@ static void coroutine_fn convert_co_do_copy(void
*opaque)
 qemu_co_mutex_lock(&s->lock);
 if (s->ret != -EINPROGRESS || s->sector_num >= s->total_sectors) {
 qemu_co_mutex_unlock(&s->lock);
-goto out;
+break;
 }
 n = convert_iteration_sectors(s, s->sector_num);
 if (n < 0) {
 qemu_co_mutex_unlock(&s->lock);
 s->ret = n;
-goto out;
+break;
 }
 /* save current sector and allocation status to local variables */
 sector_num = s->sector_num;
@@ -1792,7 +1792,6 @@ static void coroutine_fn convert_co_do_copy(void
*opaque)
 error_report("error while reading sector %" PRId64
  ": %s", sector_num, strerror(-ret));
 s->ret = ret;
-goto out;
 }
 } else if (!s->min_sparse && status == BLK_ZERO) {
 status = BLK_DATA;
@@ -1801,22 +1800,20 @@ static void coroutine_fn convert_co_do_copy(void
*opaque)

 if (s->wr_in_order) {
 /* keep writes in order */
-while (s->wr_offs != sector_num) {
-if (s->ret != -EINPROGRESS) {
-goto out;
-}
+while (s->wr_offs != sector_num && s->ret == -EINPROGRESS) {
 s->wait_sector_num[index] = sector_num;
 qemu_coroutine_yield();
 }
 s->wait_sector_num[index] = -1;
 }

-ret = convert_co_write(s, sector_num, n, buf, status);
-if (ret < 0) {
-error_report("error while writing sector %" PRId64
- ": %s", sector_num, strerror(-ret));
-s->ret = ret;
-goto out;
+if (s->ret == -EINPROGRESS) {
+ret = convert_co_write(s, sector_num, n, buf, status);
+if (ret < 0) {
+error_report("error while writing sector %" PRId64
+ 

Re: [Qemu-block] [Qemu-devel] [PATCH v6 00/18]Convert QCow[2] to QCryptoBlock & add LUKS support

[no-reply]

Hi,

This series failed build test on s390x host. Please find the details below.

Type: series
Message-id: 20170425153858.25660-1-berra...@redhat.com
Subject: [Qemu-devel] [PATCH v6 00/18]Convert QCow[2] to QCryptoBlock & add 
LUKS support

=== TEST SCRIPT BEGIN ===
#!/bin/bash
# Testing script will be invoked under the git checkout with
# HEAD pointing to a commit that has the patches applied on top of "base"
# branch
set -e
echo "=== ENV ==="
env
echo "=== PACKAGES ==="
rpm -qa
echo "=== TEST BEGIN ==="
CC=$HOME/bin/cc
INSTALL=$PWD/install
BUILD=$PWD/build
echo -n "Using CC: "
realpath $CC
mkdir -p $BUILD $INSTALL
SRC=$PWD
cd $BUILD
$SRC/configure --cc=$CC --prefix=$INSTALL
make -j4
# XXX: we need reliable clean up
# make check -j4 V=1
make install
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag] patchew/20170425153858.25660-1-berra...@redhat.com -> 
patchew/20170425153858.25660-1-berra...@redhat.com
Switched to a new branch 'test'
5853c61 block: pass option prefix down to crypto layer
d520ee5 block: remove all encryption handling APIs
21831df block: rip out all traces of password prompting
b8525d8 iotests: enable tests 134 and 158 to work with qcow (v1)
f921b52 qcow2: add iotests to cover LUKS encryption support
24921c2 qcow2: add support for LUKS encryption format
ce3d3ef qcow2: extend specification to cover LUKS encryption
d34f397 qcow2: convert QCow2 to use QCryptoBlock for encryption
b4642d4 qcow2: make qcow2_encrypt_sectors encrypt in place
a7264fa qcow: convert QCow to use QCryptoBlock for encryption
0921b2b qcow: make encrypt_sectors encrypt in place
a29311e block: deprecate "encryption=on" in favour of "encrypt.format=aes"
73d4e25 iotests: skip 048 with qcow which doesn't support resize
85013dc iotests: skip 042 with qcow which dosn't support zero sized images
c2ec616 qcow: require image size to be > 1 for new images
bf1689f qcow: document another weakness of qcow AES encryption
7b1c8e1 block: add ability to set a prefix for opt names
7308d51 block: expose crypto option names / defs to other drivers

=== OUTPUT BEGIN ===
=== ENV ===
XDG_SESSION_ID=22279
SHELL=/bin/sh
USER=fam
PATCHEW=/home/fam/patchew/patchew-cli -s http://patchew.org --nodebug
PATH=/usr/bin:/bin
PWD=/var/tmp/patchew-tester-tmp-191ln5i9/src
LANG=en_US.UTF-8
HOME=/home/fam
SHLVL=2
LOGNAME=fam
DBUS_SESSION_BUS_ADDRESS=unix:path=/run/user/1012/bus
XDG_RUNTIME_DIR=/run/user/1012
_=/usr/bin/env
=== PACKAGES ===
gpg-pubkey-873529b8-54e386ff
xz-libs-5.2.2-2.fc24.s390x
libxshmfence-1.2-3.fc24.s390x
giflib-4.1.6-15.fc24.s390x
trousers-lib-0.3.13-6.fc24.s390x
ncurses-base-6.0-6.20160709.fc25.noarch
gmp-6.1.1-1.fc25.s390x
libidn-1.33-1.fc25.s390x
slang-2.3.0-7.fc25.s390x
libsemanage-2.5-8.fc25.s390x
pkgconfig-0.29.1-1.fc25.s390x
alsa-lib-1.1.1-2.fc25.s390x
yum-metadata-parser-1.1.4-17.fc25.s390x
python3-slip-dbus-0.6.4-4.fc25.noarch
python2-cssselect-0.9.2-1.fc25.noarch
python-fedora-0.8.0-2.fc25.noarch
createrepo_c-libs-0.10.0-6.fc25.s390x
initscripts-9.69-1.fc25.s390x
wget-1.18-2.fc25.s390x
dhcp-client-4.3.5-1.fc25.s390x
parted-3.2-21.fc25.s390x
flex-2.6.0-3.fc25.s390x
colord-libs-1.3.4-1.fc25.s390x
python-osbs-client-0.33-3.fc25.noarch
perl-Pod-Simple-3.35-1.fc25.noarch
python2-simplejson-3.10.0-1.fc25.s390x
brltty-5.4-2.fc25.s390x
librados2-10.2.4-2.fc25.s390x
tcp_wrappers-7.6-83.fc25.s390x
libcephfs_jni1-10.2.4-2.fc25.s390x
nettle-devel-3.3-1.fc25.s390x
bzip2-devel-1.0.6-21.fc25.s390x
libuuid-2.28.2-2.fc25.s390x
mesa-libglapi-13.0.3-5.fc25.s390x
pcre-cpp-8.40-5.fc25.s390x
pango-1.40.4-1.fc25.s390x
python3-magic-5.29-3.fc25.noarch
python3-dnf-1.1.10-6.fc25.noarch
cryptsetup-libs-1.7.4-1.fc25.s390x
texlive-kpathsea-doc-svn41139-33.fc25.1.noarch
netpbm-10.77.00-3.fc25.s390x
openssh-7.4p1-4.fc25.s390x
kernel-headers-4.10.5-200.fc25.s390x
texlive-kpathsea-bin-svn40473-33.20160520.fc25.1.s390x
texlive-graphics-svn41015-33.fc25.1.noarch
texlive-dvipdfmx-def-svn40328-33.fc25.1.noarch
texlive-mfware-svn40768-33.fc25.1.noarch
texlive-texlive-scripts-svn41433-33.fc25.1.noarch
texlive-euro-svn22191.1.1-33.fc25.1.noarch
texlive-etex-svn37057.0-33.fc25.1.noarch
texlive-iftex-svn29654.0.2-33.fc25.1.noarch
texlive-palatino-svn31835.0-33.fc25.1.noarch
texlive-texlive-docindex-svn41430-33.fc25.1.noarch
texlive-xunicode-svn30466.0.981-33.fc25.1.noarch
texlive-koma-script-svn41508-33.fc25.1.noarch
texlive-pst-grad-svn15878.1.06-33.fc25.1.noarch
texlive-pst-blur-svn15878.2.0-33.fc25.1.noarch
texlive-jknapltx-svn19440.0-33.fc25.1.noarch
netpbm-progs-10.77.00-3.fc25.s390x
mesa-libgbm-devel-13.0.3-5.fc25.s390x
texinfo-6.1-4.fc25.s390x
openssl-devel-1.0.2k-1.fc25.s390x
python2-sssdconfig-1.15.2-1.fc25.noarch
libaio-0.3.110-6.fc24.s390x
libfontenc-1.1.3-3.fc24.s390x
lzo-2.08-8.fc24.s390x
isl-0.14-5.fc24.s390x
libXau-1.0.8-6.fc24.s390x
linux-atm-libs-2.5.1-14.fc24.s390x
libXext-1.3.3-4.fc24.s390x
libXxf86vm-1.1.4-3.fc24.s390x
bison-3.0.4-4.fc24.s390x
perl-srpm-macros-1-2

Re: [Qemu-block] qemu-io aborts at quit after reopen

[Alberto Garcia]

On Tue, Apr 25, 2017 at 11:13:17PM +0800, Fam Zheng wrote:
> Hi Kevin,
> 
> This happens both on master and on your block-next tree:
> 
> $ qemu-io -f raw null-co:// -c 'reopen -r'
> Unexpected error in bdrv_check_perm() at /stor/work/qemu/block.c:1437:
> Block node is read-only
> Aborted
> 
> It seems bs->read_only and perms go out of sync when bdrv_reopen()
> toggles the former.

It seems that the problem started in 8ee039951dea9a809e4745c42aebb4a7.

I just took a quick look at the code, but one difference is that
after this change bdrv_check_perm(old_bs, ...) is called before
QLIST_REMOVE(child, next_parent), so the returned perms are likely
different.

Berto

[Qemu-block] [PATCH v6 13/18] qcow2: add support for LUKS encryption format

[Daniel P. Berrange]

This adds support for using LUKS as an encryption format
with the qcow2 file, using the new encrypt.format parameter
to request "luks" format. e.g.

  # qemu-img create --object secret,data=123456,id=sec0 \
   -f qcow2 -o encrypt.-format=luks,encrypt.key-secret=sec0 \
   test.qcow2 10G

The legacy "encryption=on" parameter still results in
creation of the old qcow2 AES format (and is equivalent
to the new 'encryption-format=aes'). e.g. the following are
equivalent:

  # qemu-img create --object secret,data=123456,id=sec0 \
   -f qcow2 -o encryption=on,encrypt.key-secret=sec0 \
   test.qcow2 10G

 # qemu-img create --object secret,data=123456,id=sec0 \
   -f qcow2 -o encryption-format=aes,encrypt.key-secret=sec0 \
   test.qcow2 10G

With the LUKS format it is necessary to store the LUKS
partition header and key material in the QCow2 file. This
data can be many MB in size, so cannot go into the QCow2
header region directly. Thus the spec defines a FDE
(Full Disk Encryption) header extension that specifies
the offset of a set of clusters to hold the FDE headers,
as well as the length of that region. The LUKS header is
thus stored in these extra allocated clusters before the
main image payload.

Aside from all the cryptographic differences implied by
use of the LUKS format, there is one further key difference
between the use of legacy AES and LUKS encryption in qcow2.
For LUKS, the initialiazation vectors are generated using
the host physical sector as the input, rather than the
guest virtual sector. This guarantees unique initialization
vectors for all sectors when qcow2 internal snapshots are
used, thus giving stronger protection against watermarking
attacks.

Signed-off-by: Daniel P. Berrange 
---
 block/qcow2-cluster.c  |   4 +-
 block/qcow2-refcount.c |  10 ++
 block/qcow2.c  | 267 ++--
 block/qcow2.h  |   9 ++
 qapi/block-core.json   |   5 +-
 tests/qemu-iotests/082.out | 270 -
 6 files changed, 477 insertions(+), 88 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 59ad087..75f8b3e 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -383,7 +383,9 @@ static int coroutine_fn do_perform_cow(BlockDriverState *bs,
 
 if (bs->encrypted) {
 Error *err = NULL;
-int64_t sector = (src_cluster_offset + offset_in_cluster)
+int64_t sector = (s->crypt_physical_offset ?
+  (cluster_offset + offset_in_cluster) :
+  (src_cluster_offset + offset_in_cluster))
  >> BDRV_SECTOR_BITS;
 assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
 assert((bytes & ~BDRV_SECTOR_MASK) == 0);
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 9e96f64..1e8d33a 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1859,6 +1859,16 @@ static int calculate_refcounts(BlockDriverState *bs, 
BdrvCheckResult *res,
 return ret;
 }
 
+/* encryption */
+if (s->crypto_header.length) {
+ret = inc_refcounts(bs, res, refcount_table, nb_clusters,
+s->crypto_header.offset,
+s->crypto_header.length);
+if (ret < 0) {
+return ret;
+}
+}
+
 return check_refblocks(bs, res, fix, rebuild, refcount_table, nb_clusters);
 }
 
diff --git a/block/qcow2.c b/block/qcow2.c
index 1b9eca3..44a573b 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -66,6 +66,7 @@ typedef struct {
 #define  QCOW2_EXT_MAGIC_END 0
 #define  QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
 #define  QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
+#define  QCOW2_EXT_MAGIC_CRYPTO_HEADER 0x0537be77
 
 static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
 {
@@ -80,6 +81,86 @@ static int qcow2_probe(const uint8_t *buf, int buf_size, 
const char *filename)
 }
 
 
+static ssize_t qcow2_crypto_hdr_read_func(QCryptoBlock *block, size_t offset,
+  uint8_t *buf, size_t buflen,
+  void *opaque, Error **errp)
+{
+BlockDriverState *bs = opaque;
+BDRVQcow2State *s = bs->opaque;
+ssize_t ret;
+
+if ((offset + buflen) > s->crypto_header.length) {
+error_setg(errp, "Request for data outside of extension header");
+return -1;
+}
+
+ret = bdrv_pread(bs->file,
+ s->crypto_header.offset + offset, buf, buflen);
+if (ret < 0) {
+error_setg_errno(errp, -ret, "Could not read encryption header");
+return -1;
+}
+return ret;
+}
+
+
+static ssize_t qcow2_crypto_hdr_init_func(QCryptoBlock *block, size_t 
headerlen,
+  void *opaque, Error **errp)
+{
+BlockDriverState *bs = opaque;
+BDRVQcow2State *s = bs->opaque;
+int64_t ret;
+int64_t clusterlen

[Qemu-block] [PATCH v6 15/18] iotests: enable tests 134 and 158 to work with qcow (v1)

[Daniel P. Berrange]

The 138 and 158 iotests exercise the legacy qcow2 aes encryption
code path and they work fine with qcow v1 too.

Reviewed-by: Alberto Garcia 
Reviewed-by: Max Reitz 
Signed-off-by: Daniel P. Berrange 
---
 tests/qemu-iotests/134 | 2 +-
 tests/qemu-iotests/158 | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/134 b/tests/qemu-iotests/134
index f851d92..9914415 100755
--- a/tests/qemu-iotests/134
+++ b/tests/qemu-iotests/134
@@ -37,7 +37,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 . ./common.rc
 . ./common.filter
 
-_supported_fmt qcow2
+_supported_fmt qcow qcow2
 _supported_proto generic
 _unsupported_proto vxhs
 _supported_os Linux
diff --git a/tests/qemu-iotests/158 b/tests/qemu-iotests/158
index e280b79..823c120 100755
--- a/tests/qemu-iotests/158
+++ b/tests/qemu-iotests/158
@@ -37,7 +37,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 . ./common.rc
 . ./common.filter
 
-_supported_fmt qcow2
+_supported_fmt qcow qcow2
 _supported_proto generic
 _unsupported_proto vxhs
 _supported_os Linux
-- 
2.9.3

[Qemu-block] [PATCH v6 18/18] block: pass option prefix down to crypto layer

[Daniel P. Berrange]

While the crypto layer uses a fixed option name "key-secret",
the upper block layer may have a prefix on the options. e.g.
"luks-key-secret", "aes-key-secret", in order to avoid clashes
between crypto option names & other block option names. To
ensure the crypto layer can report accurate error messages,
we must tell it what option name prefix was used.

Reviewed-by: Alberto Garcia 
Reviewed-by: Max Reitz 
Signed-off-by: Daniel P. Berrange 
---
 block/crypto.c|  4 ++--
 block/qcow.c  |  7 ---
 block/qcow2.c | 11 +++
 crypto/block-luks.c   |  8 ++--
 crypto/block-qcow.c   |  8 ++--
 crypto/block.c|  6 --
 crypto/blockpriv.h|  2 ++
 include/crypto/block.h|  6 +-
 tests/test-crypto-block.c |  8 
 9 files changed, 40 insertions(+), 20 deletions(-)

diff --git a/block/crypto.c b/block/crypto.c
index 045836d..faa5501 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -296,7 +296,7 @@ static int block_crypto_open_generic(QCryptoBlockFormat 
format,
 if (flags & BDRV_O_NO_IO) {
 cflags |= QCRYPTO_BLOCK_OPEN_NO_IO;
 }
-crypto->block = qcrypto_block_open(open_opts,
+crypto->block = qcrypto_block_open(open_opts, NULL,
block_crypto_read_func,
bs,
cflags,
@@ -340,7 +340,7 @@ static int block_crypto_create_generic(QCryptoBlockFormat 
format,
 return -1;
 }
 
-crypto = qcrypto_block_create(create_opts,
+crypto = qcrypto_block_create(create_opts, NULL,
   block_crypto_init_func,
   block_crypto_write_func,
   &data,
diff --git a/block/qcow.c b/block/qcow.c
index 962b941..06f2df0 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -208,8 +208,8 @@ static int qcow_open(BlockDriverState *bs, QDict *options, 
int flags,
 if (flags & BDRV_O_NO_IO) {
 cflags |= QCRYPTO_BLOCK_OPEN_NO_IO;
 }
-s->crypto = qcrypto_block_open(crypto_opts, NULL, NULL,
-   cflags, errp);
+s->crypto = qcrypto_block_open(crypto_opts, "encrypt.",
+   NULL, NULL, cflags, errp);
 if (!s->crypto) {
 ret = -EINVAL;
 goto fail;
@@ -865,7 +865,8 @@ static int qcow_create(const char *filename, QemuOpts 
*opts, Error **errp)
 goto exit;
 }
 
-crypto = qcrypto_block_create(crypto_opts, NULL, NULL, NULL, errp);
+crypto = qcrypto_block_create(crypto_opts, "encrypt.",
+  NULL, NULL, NULL, errp);
 if (!crypto) {
 ret = -EINVAL;
 goto exit;
diff --git a/block/qcow2.c b/block/qcow2.c
index 03997c1..a6e1453 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -279,7 +279,7 @@ static int qcow2_read_extensions(BlockDriverState *bs, 
uint64_t start_offset,
 if (flags & BDRV_O_NO_IO) {
 cflags |= QCRYPTO_BLOCK_OPEN_NO_IO;
 }
-s->crypto = qcrypto_block_open(s->crypto_opts,
+s->crypto = qcrypto_block_open(s->crypto_opts, "encrypt.",
qcow2_crypto_hdr_read_func,
bs, cflags, errp);
 if (!s->crypto) {
@@ -1312,8 +1312,8 @@ static int qcow2_do_open(BlockDriverState *bs, QDict 
*options, int flags,
 if (flags & BDRV_O_NO_IO) {
 cflags |= QCRYPTO_BLOCK_OPEN_NO_IO;
 }
-s->crypto = qcrypto_block_open(s->crypto_opts, NULL, NULL,
-   cflags, errp);
+s->crypto = qcrypto_block_open(s->crypto_opts, "encrypt.",
+   NULL, NULL, cflags, errp);
 if (!s->crypto) {
 ret = -EINVAL;
 goto fail;
@@ -2245,6 +2245,7 @@ static int qcow2_set_up_encryption(BlockDriverState *bs, 
const char *encryptfmt,
 QCryptoBlock *crypto = NULL;
 int ret = -EINVAL;
 QDict *options, *encryptopts;
+const char *optprefix;
 
 options = qemu_opts_to_qdict(opts, NULL);
 qdict_extract_subqdict(options, &encryptopts, "encrypt.");
@@ -2254,10 +2255,12 @@ static int qcow2_set_up_encryption(BlockDriverState 
*bs, const char *encryptfmt,
 
 switch (fmt) {
 case QCOW_CRYPT_LUKS:
+optprefix = "luks-";
 cryptoopts = block_crypto_create_opts_init(
 Q_CRYPTO_BLOCK_FORMAT_LUKS, encryptopts, errp);
 break;
 case QCOW_CRYPT_AES:
+optprefix = "aes-";
 cryptoopts = block_crypto_create_opts_init(
 Q_CRYPTO_BLOCK_FORMAT_QCOW, encryptopts, errp);
 break;
@@ -2271,7 +2274,7 @@ static int qcow2_set_up_encryption(BlockDriverState *bs, 
const char *encryptfm

[Qemu-block] [PATCH v6 16/18] block: rip out all traces of password prompting

[Daniel P. Berrange]

Now that qcow & qcow2 are wired up to get encryption keys
via the QCryptoSecret object, nothing is relying on the
interactive prompting for passwords. All the code related
to password prompting can thus be ripped out.

Reviewed-by: Alberto Garcia 
Reviewed-by: Max Reitz 
Signed-off-by: Daniel P. Berrange 
---
 hmp.c | 31 -
 include/monitor/monitor.h |  7 -
 include/qemu/osdep.h  |  2 --
 monitor.c | 68 ---
 qapi-schema.json  | 10 +--
 qemu-img.c| 31 -
 qemu-io.c | 20 --
 qmp.c | 12 +
 util/oslib-posix.c| 66 -
 util/oslib-win32.c| 24 -
 10 files changed, 2 insertions(+), 269 deletions(-)

diff --git a/hmp.c b/hmp.c
index ab407d6..510d412 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1076,37 +1076,12 @@ void hmp_ringbuf_read(Monitor *mon, const QDict *qdict)
 g_free(data);
 }
 
-static void hmp_cont_cb(void *opaque, int err)
-{
-if (!err) {
-qmp_cont(NULL);
-}
-}
-
-static bool key_is_missing(const BlockInfo *bdev)
-{
-return (bdev->inserted && bdev->inserted->encryption_key_missing);
-}
-
 void hmp_cont(Monitor *mon, const QDict *qdict)
 {
-BlockInfoList *bdev_list, *bdev;
 Error *err = NULL;
 
-bdev_list = qmp_query_block(NULL);
-for (bdev = bdev_list; bdev; bdev = bdev->next) {
-if (key_is_missing(bdev->value)) {
-monitor_read_block_device_key(mon, bdev->value->device,
-  hmp_cont_cb, NULL);
-goto out;
-}
-}
-
 qmp_cont(&err);
 hmp_handle_error(mon, &err);
-
-out:
-qapi_free_BlockInfoList(bdev_list);
 }
 
 void hmp_system_wakeup(Monitor *mon, const QDict *qdict)
@@ -1541,12 +1516,6 @@ void hmp_change(Monitor *mon, const QDict *qdict)
 qmp_blockdev_change_medium(true, device, false, NULL, target,
!!arg, arg, !!read_only, read_only_mode,
&err);
-if (err &&
-error_get_class(err) == ERROR_CLASS_DEVICE_ENCRYPTED) {
-error_free(err);
-monitor_read_block_device_key(mon, device, NULL, NULL);
-return;
-}
 }
 
 hmp_handle_error(mon, &err);
diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index d2b3aaf..83ea4a1 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -23,13 +23,6 @@ void monitor_cleanup(void);
 int monitor_suspend(Monitor *mon);
 void monitor_resume(Monitor *mon);
 
-int monitor_read_bdrv_key_start(Monitor *mon, BlockDriverState *bs,
-BlockCompletionFunc *completion_cb,
-void *opaque);
-int monitor_read_block_device_key(Monitor *mon, const char *device,
-  BlockCompletionFunc *completion_cb,
-  void *opaque);
-
 int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp);
 int monitor_fd_param(Monitor *mon, const char *fdname, Error **errp);
 
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 122ff06..85587b8 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -441,8 +441,6 @@ void qemu_set_tty_echo(int fd, bool echo);
 void os_mem_prealloc(int fd, char *area, size_t sz, int smp_cpus,
  Error **errp);
 
-int qemu_read_password(char *buf, int buf_size);
-
 /**
  * qemu_get_pid_name:
  * @pid: pid of a process
diff --git a/monitor.c b/monitor.c
index be282ec..357383f 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4017,74 +4017,6 @@ void monitor_cleanup(void)
 qemu_mutex_unlock(&monitor_lock);
 }
 
-static void bdrv_password_cb(void *opaque, const char *password,
- void *readline_opaque)
-{
-Monitor *mon = opaque;
-BlockDriverState *bs = readline_opaque;
-int ret = 0;
-Error *local_err = NULL;
-
-bdrv_add_key(bs, password, &local_err);
-if (local_err) {
-error_report_err(local_err);
-ret = -EPERM;
-}
-if (mon->password_completion_cb)
-mon->password_completion_cb(mon->password_opaque, ret);
-
-monitor_read_command(mon, 1);
-}
-
-int monitor_read_bdrv_key_start(Monitor *mon, BlockDriverState *bs,
-BlockCompletionFunc *completion_cb,
-void *opaque)
-{
-int err;
-
-monitor_printf(mon, "%s (%s) is encrypted.\n", bdrv_get_device_name(bs),
-   bdrv_get_encrypted_filename(bs));
-
-mon->password_completion_cb = completion_cb;
-mon->password_opaque = opaque;
-
-err = monitor_read_password(mon, bdrv_password_cb, bs);
-
-if (err && completion_cb)
-completion_cb(opaque, err);
-
-return err;
-}
-
-int monitor_read_block_device_key(Monitor *mon, const 

[Qemu-block] [PATCH v6 09/18] qcow: convert QCow to use QCryptoBlock for encryption

[Daniel P. Berrange]

This converts the qcow driver to make use of the QCryptoBlock
APIs for encrypting image content. This is only wired up to
permit use of the legacy QCow encryption format. Users who wish
to have the strong LUKS format should switch to qcow2 instead.

With this change it is now required to use the QCryptoSecret
object for providing passwords, instead of the current block
password APIs / interactive prompting.

  $QEMU \
-object secret,id=sec0,filename=/home/berrange/encrypted.pw \
-drive file=/home/berrange/encrypted.qcow,encrypt.format=qcow,\
   encrypt.key-secret=sec0

Likewise when creating such images

  qemu-img create -f qcow \
-object secret,id=sec0,filename=/home/berrange/encrypted.pw \
-o encrypt.format=qcow,encrypt.key-secret=sec0 \
/home/berrange/encrypted.qcow

Signed-off-by: Daniel P. Berrange 
---
 block/crypto.c   |  10 +++
 block/crypto.h   |  20 --
 block/qcow.c | 196 +--
 qapi/block-core.json |  37 +-
 4 files changed, 156 insertions(+), 107 deletions(-)

diff --git a/block/crypto.c b/block/crypto.c
index 7edcc49..913723a 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -181,6 +181,11 @@ block_crypto_open_opts_init(QCryptoBlockFormat format,
 v, &ret->u.luks, &local_err);
 break;
 
+case Q_CRYPTO_BLOCK_FORMAT_QCOW:
+visit_type_QCryptoBlockOptionsQCow_members(
+v, &ret->u.qcow, &local_err);
+break;
+
 default:
 error_setg(&local_err, "Unsupported block format %d", format);
 break;
@@ -227,6 +232,11 @@ block_crypto_create_opts_init(QCryptoBlockFormat format,
 v, &ret->u.luks, &local_err);
 break;
 
+case Q_CRYPTO_BLOCK_FORMAT_QCOW:
+visit_type_QCryptoBlockOptionsQCow_members(
+v, &ret->u.qcow, &local_err);
+break;
+
 default:
 error_setg(&local_err, "Unsupported block format %d", format);
 break;
diff --git a/block/crypto.h b/block/crypto.h
index 78c1f50..a32e394 100644
--- a/block/crypto.h
+++ b/block/crypto.h
@@ -21,6 +21,19 @@
 #ifndef BLOCK_CRYPTO_H__
 #define BLOCK_CRYPTO_H__
 
+#define BLOCK_CRYPTO_OPT_DEF_KEY_SECRET(prefix, helpstr)\
+{   \
+.name = prefix BLOCK_CRYPTO_OPT_QCOW_KEY_SECRET,\
+.type = QEMU_OPT_STRING,\
+.help = helpstr,\
+}
+
+#define BLOCK_CRYPTO_OPT_QCOW_KEY_SECRET "key-secret"
+
+#define BLOCK_CRYPTO_OPT_DEF_QCOW_KEY_SECRET(prefix)\
+BLOCK_CRYPTO_OPT_DEF_KEY_SECRET(prefix, \
+"ID of the secret that provides the AES encryption key")
+
 #define BLOCK_CRYPTO_OPT_LUKS_KEY_SECRET "key-secret"
 #define BLOCK_CRYPTO_OPT_LUKS_CIPHER_ALG "cipher-alg"
 #define BLOCK_CRYPTO_OPT_LUKS_CIPHER_MODE "cipher-mode"
@@ -30,11 +43,8 @@
 #define BLOCK_CRYPTO_OPT_LUKS_ITER_TIME "iter-time"
 
 #define BLOCK_CRYPTO_OPT_DEF_LUKS_KEY_SECRET(prefix)\
-{   \
-.name = prefix BLOCK_CRYPTO_OPT_LUKS_KEY_SECRET,\
-.type = QEMU_OPT_STRING,\
-.help = "ID of the secret that provides the keyslot passphrase", \
-}
+BLOCK_CRYPTO_OPT_DEF_KEY_SECRET(prefix, \
+"ID of the secret that provides the keyslot passphrase")
 
 #define BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_ALG(prefix)   \
 {  \
diff --git a/block/qcow.c b/block/qcow.c
index 8b38de2..7bfa0dd 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -31,8 +31,10 @@
 #include "qemu/bswap.h"
 #include 
 #include "qapi/qmp/qerror.h"
-#include "crypto/cipher.h"
+#include "qapi/qmp/qstring.h"
+#include "crypto/block.h"
 #include "migration/migration.h"
+#include "block/crypto.h"
 
 /**/
 /* QEMU COW block driver with compression and encryption support */
@@ -77,7 +79,7 @@ typedef struct BDRVQcowState {
 uint8_t *cluster_cache;
 uint8_t *cluster_data;
 uint64_t cluster_cache_offset;
-QCryptoCipher *cipher; /* NULL if no key yet */
+QCryptoBlock *crypto; /* Disk encryption format driver */
 uint32_t crypt_method_header;
 CoMutex lock;
 Error *migration_blocker;
@@ -97,6 +99,15 @@ static int qcow_probe(const uint8_t *buf, int buf_size, 
const char *filename)
 return 0;
 }
 
+static QemuOptsList qcow_runtime_opts = {
+.name = "qcow",
+.head = QTAILQ_HEAD_INITIALIZER(qcow_runtime_opts.head),
+.desc = {
+BLOCK_CRYPTO_OPT_DEF_QCOW_KEY_SECRET("encrypt."),
+{ /* end of list */ }
+},
+};
+
 static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
 

[Qemu-block] [PATCH v6 07/18] block: deprecate "encryption=on" in favour of "encrypt.format=aes"

[Daniel P. Berrange]

Historically the qcow & qcow2 image formats supported a property
"encryption=on" to enable their built-in AES encryption. We'll
soon be supporting LUKS for qcow2, so need a more general purpose
way to enable encryption, with a choice of formats.

This introduces an "encrypt.format" option, which will later be
joined by a number of other "encrypt.XXX" options. The use of
a "encrypt." prefix instead of "encrypt-" is done to facilitate
mapping to a nested QAPI schema at later date.

e.g. the preferred syntax is now

  qemu-img create -f qcow2 -o encrypt.format=aes demo.qcow2

Signed-off-by: Daniel P. Berrange 
---
 block/qcow.c   | 30 ++---
 block/qcow2.c  | 33 +++
 include/block/block_int.h  |  2 +-
 qemu-img.c |  4 ++-
 tests/qemu-iotests/082.out | 81 ++
 5 files changed, 110 insertions(+), 40 deletions(-)

diff --git a/block/qcow.c b/block/qcow.c
index d6ce46c..e9b7861 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -803,10 +803,10 @@ static int qcow_create(const char *filename, QemuOpts 
*opts, Error **errp)
 uint8_t *tmp;
 int64_t total_size = 0;
 char *backing_file = NULL;
-int flags = 0;
 Error *local_err = NULL;
 int ret;
 BlockBackend *qcow_blk;
+const char *encryptfmt = NULL;
 
 /* Read out options */
 total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
@@ -818,8 +818,16 @@ static int qcow_create(const char *filename, QemuOpts 
*opts, Error **errp)
 }
 
 backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
-if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ENCRYPT, false)) {
-flags |= BLOCK_FLAG_ENCRYPT;
+encryptfmt = qemu_opt_get_del(opts, BLOCK_OPT_ENCRYPT_FORMAT);
+if (encryptfmt) {
+if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ENCRYPT, false)) {
+error_setg(errp, "Options " BLOCK_OPT_ENCRYPT " and "
+   BLOCK_OPT_ENCRYPT_FORMAT " are mutually exclusive");
+ret = -EINVAL;
+goto cleanup;
+}
+} else if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ENCRYPT, false)) {
+encryptfmt = "aes";
 }
 
 ret = bdrv_create_file(filename, opts, &local_err);
@@ -872,7 +880,13 @@ static int qcow_create(const char *filename, QemuOpts 
*opts, Error **errp)
 l1_size = (total_size + (1LL << shift) - 1) >> shift;
 
 header.l1_table_offset = cpu_to_be64(header_size);
-if (flags & BLOCK_FLAG_ENCRYPT) {
+if (encryptfmt) {
+if (!g_str_equal(encryptfmt, "aes")) {
+error_setg(errp, "Unknown encryption format '%s', expected 'aes'",
+   encryptfmt);
+ret = -EINVAL;
+goto exit;
+}
 header.crypt_method = cpu_to_be32(QCOW_CRYPT_AES);
 } else {
 header.crypt_method = cpu_to_be32(QCOW_CRYPT_NONE);
@@ -1046,9 +1060,15 @@ static QemuOptsList qcow_create_opts = {
 {
 .name = BLOCK_OPT_ENCRYPT,
 .type = QEMU_OPT_BOOL,
-.help = "Encrypt the image",
+.help = "Encrypt the image with format 'aes'. (Deprecated "
+"in favour of " BLOCK_OPT_ENCRYPT_FORMAT "=aes)",
 .def_value_str = "off"
 },
+{
+.name = BLOCK_OPT_ENCRYPT_FORMAT,
+.type = QEMU_OPT_STRING,
+.help = "Encrypt the image, format choices: 'aes'",
+},
 { /* end of list */ }
 }
 };
diff --git a/block/qcow2.c b/block/qcow2.c
index 6a92d2e..98ca6ed 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2099,7 +2099,7 @@ static int qcow2_create2(const char *filename, int64_t 
total_size,
  const char *backing_file, const char *backing_format,
  int flags, size_t cluster_size, PreallocMode prealloc,
  QemuOpts *opts, int version, int refcount_order,
- Error **errp)
+ const char *encryptfmt, Error **errp)
 {
 int cluster_bits;
 QDict *options;
@@ -2227,7 +2227,13 @@ static int qcow2_create2(const char *filename, int64_t 
total_size,
 .header_length  = cpu_to_be32(sizeof(*header)),
 };
 
-if (flags & BLOCK_FLAG_ENCRYPT) {
+if (encryptfmt) {
+if (!g_str_equal(encryptfmt, "aes")) {
+error_setg(errp, "Unknown encryption format '%s', expected 'aes'",
+   encryptfmt);
+ret = -EINVAL;
+goto out;
+}
 header->crypt_method = cpu_to_be32(QCOW_CRYPT_AES);
 } else {
 header->crypt_method = cpu_to_be32(QCOW_CRYPT_NONE);
@@ -2356,6 +2362,7 @@ static int qcow2_create(const char *filename, QemuOpts 
*opts, Error **errp)
 int version = 3;
 uint64_t refcount_bits = 16;
 int refcount_order;
+const char *encryptfmt = NULL;
 Error *local_err = NULL;
 int ret;
 
@@ -2364,8 +2371,16 @@ static int qcow2_crea

[Qemu-block] [PATCH v6 14/18] qcow2: add iotests to cover LUKS encryption support

[Daniel P. Berrange]

This extends the 087 iotest to cover LUKS encryption when doing
blockdev-add.

Two further tests are added to validate read/write of LUKS
encrypted images with a single file and with a backing file.

Reviewed-by: Alberto Garcia 
Reviewed-by: Max Reitz 
Signed-off-by: Daniel P. Berrange 
---
 tests/qemu-iotests/087 | 35 ++-
 tests/qemu-iotests/087.out | 14 +++-
 tests/qemu-iotests/177 | 76 
 tests/qemu-iotests/177.out | 18 ++
 tests/qemu-iotests/178 | 86 ++
 tests/qemu-iotests/178.out | 26 ++
 tests/qemu-iotests/group   |  2 ++
 7 files changed, 255 insertions(+), 2 deletions(-)
 create mode 100755 tests/qemu-iotests/177
 create mode 100644 tests/qemu-iotests/177.out
 create mode 100755 tests/qemu-iotests/178
 create mode 100644 tests/qemu-iotests/178.out

diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087
index 730270e..153dad0 100755
--- a/tests/qemu-iotests/087
+++ b/tests/qemu-iotests/087
@@ -121,7 +121,7 @@ run_qemu .
+#
+
+# creator
+owner=berra...@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1   # failure is the default!
+
+_cleanup()
+{
+   _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt qcow2
+_supported_proto generic
+_supported_os Linux
+
+
+size=16M
+
+SECRET="secret,id=sec0,data=astrochicken"
+SECRETALT="secret,id=sec0,data=platypus"
+
+_make_test_img --object $SECRET -o 
"encrypt.format=luks,encrypt.key-secret=sec0,encrypt.iter-time=10" $size
+
+IMGSPEC="driver=$IMGFMT,file.filename=$TEST_IMG,encrypt.key-secret=sec0"
+
+QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT
+
+echo
+echo "== reading whole image =="
+$QEMU_IO --object $SECRET -c "read -P 0 0 $size" --image-opts $IMGSPEC | 
_filter_qemu_io | _filter_testdir
+
+echo
+echo "== rewriting whole image =="
+$QEMU_IO --object $SECRET -c "write -P 0xa 0 $size" --image-opts $IMGSPEC | 
_filter_qemu_io | _filter_testdir
+
+echo
+echo "== verify pattern =="
+$QEMU_IO --object $SECRET -c "read -P 0xa 0 $size"  --image-opts $IMGSPEC | 
_filter_qemu_io | _filter_testdir
+
+echo
+echo "== verify open failure with wrong password =="
+$QEMU_IO --object $SECRETALT -c "read -P 0xa 0 $size" --image-opts $IMGSPEC | 
_filter_qemu_io | _filter_testdir
+
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/177.out b/tests/qemu-iotests/177.out
new file mode 100644
index 000..95d558b
--- /dev/null
+++ b/tests/qemu-iotests/177.out
@@ -0,0 +1,18 @@
+QA output created by 177
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=16777216 encrypt.format=luks 
encrypt.key-secret=sec0 encrypt.iter-time=10
+
+== reading whole image ==
+read 16777216/16777216 bytes at offset 0
+16 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== rewriting whole image ==
+wrote 16777216/16777216 bytes at offset 0
+16 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== verify pattern ==
+read 16777216/16777216 bytes at offset 0
+16 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== verify open failure with wrong password ==
+can't open: Invalid password, cannot unlock any keyslot
+*** done
diff --git a/tests/qemu-iotests/178 b/tests/qemu-iotests/178
new file mode 100755
index 000..54ad980
--- /dev/null
+++ b/tests/qemu-iotests/178
@@ -0,0 +1,86 @@
+#!/bin/bash
+#
+# Test encrypted read/write using backing files
+#
+# Copyright (C) 2017 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+# creator
+owner=berra...@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1   # failure is the default!
+
+_cleanup()
+{
+   _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt qcow2
+_supported_proto generic
+_supported_os Linux
+
+
+size=16M
+TEST_IMG_BASE=$TEST_IMG.base
+SECRET0="secret,id=sec0,data=astrochicken"
+SECRET1="secret,id=sec1,data=furby"
+
+TEST_IMG_SAVE=$TEST_IMG
+TEST_IMG=$TEST_IMG_BASE
+echo "== create base =="
+_make_test_img --object $SECRET0 -o 
"encrypt.format=luks,encrypt.key-secret=sec0,encrypt.iter-time=10" $size
+

[Qemu-block] [PATCH v6 11/18] qcow2: convert QCow2 to use QCryptoBlock for encryption

[Daniel P. Berrange]

This converts the qcow2 driver to make use of the QCryptoBlock
APIs for encrypting image content, using the legacyy QCow2 AES
scheme.

With this change it is now required to use the QCryptoSecret
object for providing passwords, instead of the current block
password APIs / interactive prompting.

  $QEMU \
-object secret,id=sec0,filename=/home/berrange/encrypted.pw \
-drive file=/home/berrange/encrypted.qcow2,encrypt.key-secret=sec0

The test 087 could be simplified since there is no longer a
difference in behaviour when using blockdev_add with encrypted
images for the running vs stopped CPU state.

Signed-off-by: Daniel P. Berrange 
---
 block/qcow2-cluster.c  |  47 +-
 block/qcow2.c  | 225 ++---
 block/qcow2.h  |   5 +-
 qapi/block-core.json   |  27 +-
 tests/qemu-iotests/049 |   2 +-
 tests/qemu-iotests/049.out |   4 +-
 tests/qemu-iotests/082.out |  27 ++
 tests/qemu-iotests/087 |  28 +++---
 tests/qemu-iotests/087.out |  12 +--
 tests/qemu-iotests/134 |  18 +++-
 tests/qemu-iotests/134.out |  10 +-
 tests/qemu-iotests/158 |  19 ++--
 tests/qemu-iotests/158.out |  14 +--
 tests/qemu-iotests/common  |  10 +-
 14 files changed, 262 insertions(+), 186 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 0a27399..59ad087 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -345,47 +345,6 @@ static int count_contiguous_clusters_by_type(int 
nb_clusters,
 return i;
 }
 
-/* The crypt function is compatible with the linux cryptoloop
-   algorithm for < 4 GB images. */
-int qcow2_encrypt_sectors(BDRVQcow2State *s, int64_t sector_num,
-  uint8_t *buf, int nb_sectors, bool enc,
-  Error **errp)
-{
-union {
-uint64_t ll[2];
-uint8_t b[16];
-} ivec;
-int i;
-int ret;
-
-for(i = 0; i < nb_sectors; i++) {
-ivec.ll[0] = cpu_to_le64(sector_num);
-ivec.ll[1] = 0;
-if (qcrypto_cipher_setiv(s->cipher,
- ivec.b, G_N_ELEMENTS(ivec.b),
- errp) < 0) {
-return -1;
-}
-if (enc) {
-ret = qcrypto_cipher_encrypt(s->cipher,
- buf, buf,
- 512,
- errp);
-} else {
-ret = qcrypto_cipher_decrypt(s->cipher,
- buf, buf,
- 512,
- errp);
-}
-if (ret < 0) {
-return -1;
-}
-sector_num++;
-buf += 512;
-}
-return 0;
-}
-
 static int coroutine_fn do_perform_cow(BlockDriverState *bs,
uint64_t src_cluster_offset,
uint64_t cluster_offset,
@@ -426,11 +385,11 @@ static int coroutine_fn do_perform_cow(BlockDriverState 
*bs,
 Error *err = NULL;
 int64_t sector = (src_cluster_offset + offset_in_cluster)
  >> BDRV_SECTOR_BITS;
-assert(s->cipher);
 assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
 assert((bytes & ~BDRV_SECTOR_MASK) == 0);
-if (qcow2_encrypt_sectors(s, sector, iov.iov_base,
-  bytes >> BDRV_SECTOR_BITS, true, &err) < 0) {
+assert(s->crypto);
+if (qcrypto_block_encrypt(s->crypto, sector, iov.iov_base,
+  bytes, &err) < 0) {
 ret = -EIO;
 error_free(err);
 goto out;
diff --git a/block/qcow2.c b/block/qcow2.c
index 8c28e9c..1b9eca3 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -37,6 +37,9 @@
 #include "qemu/option_int.h"
 #include "qemu/cutils.h"
 #include "qemu/bswap.h"
+#include "qapi/opts-visitor.h"
+#include "qapi-visit.h"
+#include "block/crypto.h"
 
 /*
   Differences with QCOW:
@@ -461,6 +464,7 @@ static QemuOptsList qcow2_runtime_opts = {
 .type = QEMU_OPT_NUMBER,
 .help = "Clean unused cache entries after this time (in seconds)",
 },
+BLOCK_CRYPTO_OPT_DEF_QCOW_KEY_SECRET("encrypt."),
 { /* end of list */ }
 },
 };
@@ -585,6 +589,7 @@ typedef struct Qcow2ReopenState {
 int overlap_check;
 bool discard_passthrough[QCOW2_DISCARD_MAX];
 uint64_t cache_clean_interval;
+QCryptoBlockOpenOptions *crypto_opts; /* Disk encryption runtime options */
 } Qcow2ReopenState;
 
 static int qcow2_update_options_prepare(BlockDriverState *bs,
@@ -598,9 +603,14 @@ static int qcow2_update_options_prepare(BlockDriverState 
*bs,
 int overlap_check_template = 0;
 uint64_t l2_cache_size, refcount_cache_size;
 int i;
+const char *encryptfmt;
+QDict *encryptopts = NULL;
 Error *local_err = NULL;
 int ret;
 
+qdict_ext

[Qemu-block] [PATCH v6 08/18] qcow: make encrypt_sectors encrypt in place

[Daniel P. Berrange]

Instead of requiring separate input/output buffers for
encrypting data, change encrypt_sectors() to assume
use of a single buffer, encrypting in place. One current
caller uses the same buffer for input/output already
and the other two callers are easily converted to do so.

Reviewed-by: Alberto Garcia 
Reviewed-by: Eric Blake 
Reviewed-by: Max Reitz 
Reviewed-by: Kevin Wolf 
Signed-off-by: Daniel P. Berrange 
---
 block/qcow.c | 45 +++--
 1 file changed, 15 insertions(+), 30 deletions(-)

diff --git a/block/qcow.c b/block/qcow.c
index e9b7861..8b38de2 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -322,11 +322,10 @@ static int qcow_set_key(BlockDriverState *bs, const char 
*key)
 }
 
 /* The crypt function is compatible with the linux cryptoloop
-   algorithm for < 4 GB images. NOTE: out_buf == in_buf is
-   supported */
+   algorithm for < 4 GB images. */
 static int encrypt_sectors(BDRVQcowState *s, int64_t sector_num,
-   uint8_t *out_buf, const uint8_t *in_buf,
-   int nb_sectors, bool enc, Error **errp)
+   uint8_t *buf, int nb_sectors, bool enc,
+   Error **errp)
 {
 union {
 uint64_t ll[2];
@@ -345,14 +344,12 @@ static int encrypt_sectors(BDRVQcowState *s, int64_t 
sector_num,
 }
 if (enc) {
 ret = qcrypto_cipher_encrypt(s->cipher,
- in_buf,
- out_buf,
+ buf, buf,
  512,
  errp);
 } else {
 ret = qcrypto_cipher_decrypt(s->cipher,
- in_buf,
- out_buf,
+ buf, buf,
  512,
  errp);
 }
@@ -360,8 +357,7 @@ static int encrypt_sectors(BDRVQcowState *s, int64_t 
sector_num,
 return -1;
 }
 sector_num++;
-in_buf += 512;
-out_buf += 512;
+buf += 512;
 }
 return 0;
 }
@@ -481,13 +477,12 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
 uint64_t start_sect;
 assert(s->cipher);
 start_sect = (offset & ~(s->cluster_size - 1)) >> 9;
-memset(s->cluster_data + 512, 0x00, 512);
 for(i = 0; i < s->cluster_sectors; i++) {
 if (i < n_start || i >= n_end) {
 Error *err = NULL;
+memset(s->cluster_data, 0x00, 512);
 if (encrypt_sectors(s, start_sect + i,
-s->cluster_data,
-s->cluster_data + 512, 1,
+s->cluster_data, 1,
 true, &err) < 0) {
 error_free(err);
 errno = EIO;
@@ -665,7 +660,7 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, 
int64_t sector_num,
 }
 if (bs->encrypted) {
 assert(s->cipher);
-if (encrypt_sectors(s, sector_num, buf, buf,
+if (encrypt_sectors(s, sector_num, buf,
 n, false, &err) < 0) {
 goto fail;
 }
@@ -700,9 +695,7 @@ static coroutine_fn int qcow_co_writev(BlockDriverState 
*bs, int64_t sector_num,
 BDRVQcowState *s = bs->opaque;
 int index_in_cluster;
 uint64_t cluster_offset;
-const uint8_t *src_buf;
 int ret = 0, n;
-uint8_t *cluster_data = NULL;
 struct iovec hd_iov;
 QEMUIOVector hd_qiov;
 uint8_t *buf;
@@ -710,7 +703,9 @@ static coroutine_fn int qcow_co_writev(BlockDriverState 
*bs, int64_t sector_num,
 
 s->cluster_cache_offset = -1; /* disable compressed cache */
 
-if (qiov->niov > 1) {
+/* We must always copy the iov when encrypting, so we
+ * don't modify the original data buffer during encryption */
+if (bs->encrypted || qiov->niov > 1) {
 buf = orig_buf = qemu_try_blockalign(bs, qiov->size);
 if (buf == NULL) {
 return -ENOMEM;
@@ -740,21 +735,14 @@ static coroutine_fn int qcow_co_writev(BlockDriverState 
*bs, int64_t sector_num,
 if (bs->encrypted) {
 Error *err = NULL;
 assert(s->cipher);
-if (!cluster_data) {
-cluster_data = g_malloc0(s->cluster_size);
-}
-if (encrypt_sectors(s, sector_num, cluster_data, buf,
-n, true, &err) < 0) {
+if (encrypt_sectors(s, sector_num, buf, n, true, &err) < 0) {
 

[Qemu-block] [PATCH v6 17/18] block: remove all encryption handling APIs

[Daniel P. Berrange]

Now that all encryption keys must be provided upfront via
the QCryptoSecret API and associated block driver properties
there is no need for any explicit encryption handling APIs
in the block layer. Encryption can be handled transparently
within the block driver. We only retain an API for querying
whether an image is encrypted or not, since that is a
potentially useful piece of metadata to report to the user.

Reviewed-by: Max Reitz 
Signed-off-by: Daniel P. Berrange 
---
 block.c   | 77 +--
 block/crypto.c|  1 -
 block/qapi.c  |  2 +-
 block/qcow.c  |  7 -
 block/qcow2.c |  1 -
 blockdev.c| 37 ++-
 hmp-commands.hx   |  2 ++
 include/block/block.h |  3 --
 include/block/block_int.h |  1 -
 include/qapi/error.h  |  1 -
 qapi/block-core.json  | 37 ++-
 qapi/common.json  |  5 +--
 12 files changed, 15 insertions(+), 159 deletions(-)

diff --git a/block.c b/block.c
index 5db266b..a3de487 100644
--- a/block.c
+++ b/block.c
@@ -2482,15 +2482,7 @@ static BlockDriverState *bdrv_open_inherit(const char 
*filename,
 goto close_and_fail;
 }
 
-if (!bdrv_key_required(bs)) {
-bdrv_parent_cb_change_media(bs, true);
-} else if (!runstate_check(RUN_STATE_PRELAUNCH)
-   && !runstate_check(RUN_STATE_INMIGRATE)
-   && !runstate_check(RUN_STATE_PAUSED)) { /* HACK */
-error_setg(errp,
-   "Guest must be stopped for opening of encrypted image");
-goto close_and_fail;
-}
+bdrv_parent_cb_change_media(bs, true);
 
 QDECREF(options);
 
@@ -2981,7 +2973,6 @@ static void bdrv_close(BlockDriverState *bs)
 bs->backing_format[0] = '\0';
 bs->total_sectors = 0;
 bs->encrypted = false;
-bs->valid_key = false;
 bs->sg = false;
 QDECREF(bs->options);
 QDECREF(bs->explicit_options);
@@ -3407,72 +3398,6 @@ bool bdrv_is_encrypted(BlockDriverState *bs)
 return bs->encrypted;
 }
 
-bool bdrv_key_required(BlockDriverState *bs)
-{
-BdrvChild *backing = bs->backing;
-
-if (backing && backing->bs->encrypted && !backing->bs->valid_key) {
-return true;
-}
-return (bs->encrypted && !bs->valid_key);
-}
-
-int bdrv_set_key(BlockDriverState *bs, const char *key)
-{
-int ret;
-if (bs->backing && bs->backing->bs->encrypted) {
-ret = bdrv_set_key(bs->backing->bs, key);
-if (ret < 0)
-return ret;
-if (!bs->encrypted)
-return 0;
-}
-if (!bs->encrypted) {
-return -EINVAL;
-} else if (!bs->drv || !bs->drv->bdrv_set_key) {
-return -ENOMEDIUM;
-}
-ret = bs->drv->bdrv_set_key(bs, key);
-if (ret < 0) {
-bs->valid_key = false;
-} else if (!bs->valid_key) {
-/* call the change callback now, we skipped it on open */
-bs->valid_key = true;
-bdrv_parent_cb_change_media(bs, true);
-}
-return ret;
-}
-
-/*
- * Provide an encryption key for @bs.
- * If @key is non-null:
- * If @bs is not encrypted, fail.
- * Else if the key is invalid, fail.
- * Else set @bs's key to @key, replacing the existing key, if any.
- * If @key is null:
- * If @bs is encrypted and still lacks a key, fail.
- * Else do nothing.
- * On failure, store an error object through @errp if non-null.
- */
-void bdrv_add_key(BlockDriverState *bs, const char *key, Error **errp)
-{
-if (key) {
-if (!bdrv_is_encrypted(bs)) {
-error_setg(errp, "Node '%s' is not encrypted",
-  bdrv_get_device_or_node_name(bs));
-} else if (bdrv_set_key(bs, key) < 0) {
-error_setg(errp, QERR_INVALID_PASSWORD);
-}
-} else {
-if (bdrv_key_required(bs)) {
-error_set(errp, ERROR_CLASS_DEVICE_ENCRYPTED,
-  "'%s' (%s) is encrypted",
-  bdrv_get_device_or_node_name(bs),
-  bdrv_get_encrypted_filename(bs));
-}
-}
-}
-
 const char *bdrv_get_format_name(BlockDriverState *bs)
 {
 return bs->drv ? bs->drv->format_name : NULL;
diff --git a/block/crypto.c b/block/crypto.c
index 913723a..045836d 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -308,7 +308,6 @@ static int block_crypto_open_generic(QCryptoBlockFormat 
format,
 }
 
 bs->encrypted = true;
-bs->valid_key = true;
 
 ret = 0;
  cleanup:
diff --git a/block/qapi.c b/block/qapi.c
index a40922e..9d724c2 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -45,7 +45,7 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
 info->ro = bs->read_only;
 info->drv= g_strdup(bs->drv->format_name);
 info->encrypted  = bs->encrypted;
-info->encryption_key_missing = bdrv_key_required(bs);
+info->encryption_key_missin

[Qemu-block] [PATCH v6 05/18] iotests: skip 042 with qcow which dosn't support zero sized images

[Daniel P. Berrange]

Test 042 is designed to verify operation with zero sized images.
Such images are not supported with qcow (v1), so this test has
always failed.

Reviewed-by: Max Reitz 
Reviewed-by: Alberto Garcia 
Signed-off-by: Daniel P. Berrange 
---
 tests/qemu-iotests/042 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/042 b/tests/qemu-iotests/042
index 351b283..a53e7cb 100755
--- a/tests/qemu-iotests/042
+++ b/tests/qemu-iotests/042
@@ -37,7 +37,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 . ./common.rc
 . ./common.filter
 
-_supported_fmt qcow2 qcow qed
+_supported_fmt qcow2 qed
 _supported_proto file
 _supported_os Linux
 
-- 
2.9.3

[Qemu-block] [PATCH v6 06/18] iotests: skip 048 with qcow which doesn't support resize

[Daniel P. Berrange]

Test 048 is designed to verify data preservation during an
image resize. The qcow (v1) format impl has never supported
resize so always fails.

Reviewed-by: Max Reitz 
Reviewed-by: Alberto Garcia 
Signed-off-by: Daniel P. Berrange 
---
 tests/qemu-iotests/048 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/048 b/tests/qemu-iotests/048
index 203c04f..9ed04a0 100755
--- a/tests/qemu-iotests/048
+++ b/tests/qemu-iotests/048
@@ -46,7 +46,7 @@ _compare()
 . ./common.filter
 . ./common.pattern
 
-_supported_fmt raw qcow qcow2 qed luks
+_supported_fmt raw qcow2 qed luks
 _supported_proto file
 _supported_os Linux
 
-- 
2.9.3

[Qemu-block] [PATCH v6 04/18] qcow: require image size to be > 1 for new images

[Daniel P. Berrange]

The qcow driver refuses to open images which are less than
2 bytes in size, but will happily create such images. Add
a check in the create path to avoid this discrepancy.

Reviewed-by: Max Reitz 
Reviewed-by: Alberto Garcia 
Reviewed-by: Eric Blake 
Signed-off-by: Daniel P. Berrange 
---
 block/qcow.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/block/qcow.c b/block/qcow.c
index 9d6ac83..d6ce46c 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -811,6 +811,12 @@ static int qcow_create(const char *filename, QemuOpts 
*opts, Error **errp)
 /* Read out options */
 total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
   BDRV_SECTOR_SIZE);
+if (total_size == 0) {
+error_setg(errp, "Image size is too small, cannot be zero length");
+ret = -EINVAL;
+goto cleanup;
+}
+
 backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
 if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ENCRYPT, false)) {
 flags |= BLOCK_FLAG_ENCRYPT;
-- 
2.9.3

[Qemu-block] [PATCH v6 01/18] block: expose crypto option names / defs to other drivers

[Daniel P. Berrange]

The block/crypto.c defines a set of QemuOpts that provide
parameters for encryption. This will also be needed by
the qcow/qcow2 integration, so expose the relevant pieces
in a new block/crypto.h header. Some helper methods taking
QemuOpts are changed to take QDict to simplify usage in
other places.

Reviewed-by: Max Reitz 
Reviewed-by: Eric Blake 
Reviewed-by: Alberto Garcia 
Signed-off-by: Daniel P. Berrange 
---
 block/crypto.c | 82 +---
 block/crypto.h | 91 ++
 2 files changed, 117 insertions(+), 56 deletions(-)
 create mode 100644 block/crypto.h

diff --git a/block/crypto.c b/block/crypto.c
index 2a6a805..8205bd8 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -24,16 +24,10 @@
 #include "sysemu/block-backend.h"
 #include "crypto/block.h"
 #include "qapi/opts-visitor.h"
+#include "qapi/qobject-input-visitor.h"
 #include "qapi-visit.h"
 #include "qapi/error.h"
-
-#define BLOCK_CRYPTO_OPT_LUKS_KEY_SECRET "key-secret"
-#define BLOCK_CRYPTO_OPT_LUKS_CIPHER_ALG "cipher-alg"
-#define BLOCK_CRYPTO_OPT_LUKS_CIPHER_MODE "cipher-mode"
-#define BLOCK_CRYPTO_OPT_LUKS_IVGEN_ALG "ivgen-alg"
-#define BLOCK_CRYPTO_OPT_LUKS_IVGEN_HASH_ALG "ivgen-hash-alg"
-#define BLOCK_CRYPTO_OPT_LUKS_HASH_ALG "hash-alg"
-#define BLOCK_CRYPTO_OPT_LUKS_ITER_TIME "iter-time"
+#include "block/crypto.h"
 
 typedef struct BlockCrypto BlockCrypto;
 
@@ -135,11 +129,7 @@ static QemuOptsList block_crypto_runtime_opts_luks = {
 .name = "crypto",
 .head = QTAILQ_HEAD_INITIALIZER(block_crypto_runtime_opts_luks.head),
 .desc = {
-{
-.name = BLOCK_CRYPTO_OPT_LUKS_KEY_SECRET,
-.type = QEMU_OPT_STRING,
-.help = "ID of the secret that provides the encryption key",
-},
+BLOCK_CRYPTO_OPT_DEF_LUKS_KEY_SECRET,
 { /* end of list */ }
 },
 };
@@ -154,49 +144,21 @@ static QemuOptsList block_crypto_create_opts_luks = {
 .type = QEMU_OPT_SIZE,
 .help = "Virtual disk size"
 },
-{
-.name = BLOCK_CRYPTO_OPT_LUKS_KEY_SECRET,
-.type = QEMU_OPT_STRING,
-.help = "ID of the secret that provides the encryption key",
-},
-{
-.name = BLOCK_CRYPTO_OPT_LUKS_CIPHER_ALG,
-.type = QEMU_OPT_STRING,
-.help = "Name of encryption cipher algorithm",
-},
-{
-.name = BLOCK_CRYPTO_OPT_LUKS_CIPHER_MODE,
-.type = QEMU_OPT_STRING,
-.help = "Name of encryption cipher mode",
-},
-{
-.name = BLOCK_CRYPTO_OPT_LUKS_IVGEN_ALG,
-.type = QEMU_OPT_STRING,
-.help = "Name of IV generator algorithm",
-},
-{
-.name = BLOCK_CRYPTO_OPT_LUKS_IVGEN_HASH_ALG,
-.type = QEMU_OPT_STRING,
-.help = "Name of IV generator hash algorithm",
-},
-{
-.name = BLOCK_CRYPTO_OPT_LUKS_HASH_ALG,
-.type = QEMU_OPT_STRING,
-.help = "Name of encryption hash algorithm",
-},
-{
-.name = BLOCK_CRYPTO_OPT_LUKS_ITER_TIME,
-.type = QEMU_OPT_NUMBER,
-.help = "Time to spend in PBKDF in milliseconds",
-},
+BLOCK_CRYPTO_OPT_DEF_LUKS_KEY_SECRET,
+BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_ALG,
+BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_MODE,
+BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_ALG,
+BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_HASH_ALG,
+BLOCK_CRYPTO_OPT_DEF_LUKS_HASH_ALG,
+BLOCK_CRYPTO_OPT_DEF_LUKS_ITER_TIME,
 { /* end of list */ }
 },
 };
 
 
-static QCryptoBlockOpenOptions *
+QCryptoBlockOpenOptions *
 block_crypto_open_opts_init(QCryptoBlockFormat format,
-QemuOpts *opts,
+QDict *opts,
 Error **errp)
 {
 Visitor *v;
@@ -206,7 +168,7 @@ block_crypto_open_opts_init(QCryptoBlockFormat format,
 ret = g_new0(QCryptoBlockOpenOptions, 1);
 ret->format = format;
 
-v = opts_visitor_new(opts);
+v = qobject_input_visitor_new_keyval(QOBJECT(opts));
 
 visit_start_struct(v, NULL, NULL, 0, &local_err);
 if (local_err) {
@@ -240,9 +202,9 @@ block_crypto_open_opts_init(QCryptoBlockFormat format,
 }
 
 
-static QCryptoBlockCreateOptions *
+QCryptoBlockCreateOptions *
 block_crypto_create_opts_init(QCryptoBlockFormat format,
-  QemuOpts *opts,
+  QDict *opts,
   Error **errp)
 {
 Visitor *v;
@@ -252,7 +214,7 @@ block_crypto_create_opts_init(QCryptoBlockFormat format,
 ret = g_new0(QCryptoBlockCreateOptions, 1);
 ret->format = format;
 
-v = opts_visitor_new(opts);
+v = qobject_input_visitor_new_keyval(QOBJECT(opts));
 
 visit_start_struct(v, NULL, NULL, 0, &local_err);
 if (local_err) {
@@ -299,6 +261,7 @@ static int b

[Qemu-block] [PATCH v6 02/18] block: add ability to set a prefix for opt names

[Daniel P. Berrange]

When integrating the crypto support with qcow/qcow2, we don't
want to use the bare LUKS option names "hash-alg", "key-secret",
etc. We want to namespace them "luks-hash-alg", "luks-key-secret"
so that they don't clash with any general qcow options at a later
date.

Reviewed-by: Max Reitz 
Reviewed-by: Alberto Garcia 
Signed-off-by: Daniel P. Berrange 
---
 block/crypto.c | 16 
 block/crypto.h | 40 
 2 files changed, 28 insertions(+), 28 deletions(-)

diff --git a/block/crypto.c b/block/crypto.c
index 8205bd8..7edcc49 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -129,7 +129,7 @@ static QemuOptsList block_crypto_runtime_opts_luks = {
 .name = "crypto",
 .head = QTAILQ_HEAD_INITIALIZER(block_crypto_runtime_opts_luks.head),
 .desc = {
-BLOCK_CRYPTO_OPT_DEF_LUKS_KEY_SECRET,
+BLOCK_CRYPTO_OPT_DEF_LUKS_KEY_SECRET(""),
 { /* end of list */ }
 },
 };
@@ -144,13 +144,13 @@ static QemuOptsList block_crypto_create_opts_luks = {
 .type = QEMU_OPT_SIZE,
 .help = "Virtual disk size"
 },
-BLOCK_CRYPTO_OPT_DEF_LUKS_KEY_SECRET,
-BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_ALG,
-BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_MODE,
-BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_ALG,
-BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_HASH_ALG,
-BLOCK_CRYPTO_OPT_DEF_LUKS_HASH_ALG,
-BLOCK_CRYPTO_OPT_DEF_LUKS_ITER_TIME,
+BLOCK_CRYPTO_OPT_DEF_LUKS_KEY_SECRET(""),
+BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_ALG(""),
+BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_MODE(""),
+BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_ALG(""),
+BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_HASH_ALG(""),
+BLOCK_CRYPTO_OPT_DEF_LUKS_HASH_ALG(""),
+BLOCK_CRYPTO_OPT_DEF_LUKS_ITER_TIME(""),
 { /* end of list */ }
 },
 };
diff --git a/block/crypto.h b/block/crypto.h
index a4ea28a..78c1f50 100644
--- a/block/crypto.h
+++ b/block/crypto.h
@@ -29,51 +29,51 @@
 #define BLOCK_CRYPTO_OPT_LUKS_HASH_ALG "hash-alg"
 #define BLOCK_CRYPTO_OPT_LUKS_ITER_TIME "iter-time"
 
-#define BLOCK_CRYPTO_OPT_DEF_LUKS_KEY_SECRET\
+#define BLOCK_CRYPTO_OPT_DEF_LUKS_KEY_SECRET(prefix)\
 {   \
-.name = BLOCK_CRYPTO_OPT_LUKS_KEY_SECRET,   \
+.name = prefix BLOCK_CRYPTO_OPT_LUKS_KEY_SECRET,\
 .type = QEMU_OPT_STRING,\
 .help = "ID of the secret that provides the keyslot passphrase", \
 }
 
-#define BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_ALG   \
+#define BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_ALG(prefix)   \
 {  \
-.name = BLOCK_CRYPTO_OPT_LUKS_CIPHER_ALG,  \
+.name = prefix BLOCK_CRYPTO_OPT_LUKS_CIPHER_ALG,   \
 .type = QEMU_OPT_STRING,   \
 .help = "Name of encryption cipher algorithm", \
 }
 
-#define BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_MODE \
-{ \
-.name = BLOCK_CRYPTO_OPT_LUKS_CIPHER_MODE,\
-.type = QEMU_OPT_STRING,  \
-.help = "Name of encryption cipher mode", \
+#define BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_MODE(prefix)  \
+{  \
+.name = prefix BLOCK_CRYPTO_OPT_LUKS_CIPHER_MODE,  \
+.type = QEMU_OPT_STRING,   \
+.help = "Name of encryption cipher mode",  \
 }
 
-#define BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_ALG   \
-{ \
-.name = BLOCK_CRYPTO_OPT_LUKS_IVGEN_ALG,  \
-.type = QEMU_OPT_STRING,  \
-.help = "Name of IV generator algorithm", \
+#define BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_ALG(prefix) \
+{   \
+.name = prefix BLOCK_CRYPTO_OPT_LUKS_IVGEN_ALG, \
+.type = QEMU_OPT_STRING,\
+.help = "Name of IV generator algorithm",   \
 }
 
-#define BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_HASH_ALG\
+#define BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_HASH_ALG(prefix)\
 {   \
-.name = BLOCK_CRYPTO_OPT_LUKS_IVGEN_HASH_ALG,   \
+.name = prefix BLOCK_CRYPTO_OPT_LUKS_IVGEN_HASH_ALG,\
 .type = QEMU_OPT_STRING,\
 .help = "Name of IV generator hash algorithm",  \
 }
 
-#define BLOCK_CRYPTO_OPT_DEF_LUKS_HASH_ALG   \
+#define BLOCK_CRYPTO_OPT_DEF_LUKS_HASH_ALG(prefix)   \
 {\
-.name = BLOCK_CRYPTO_OPT_LUKS_HASH_ALG,  \
+.name

[Qemu-block] [PATCH v6 10/18] qcow2: make qcow2_encrypt_sectors encrypt in place

[Daniel P. Berrange]

Instead of requiring separate input/output buffers for
encrypting data, change qcow2_encrypt_sectors() to assume
use of a single buffer, encrypting in place. The current
callers all used the same buffer for input/output already.

Reviewed-by: Eric Blake 
Reviewed-by: Fam Zheng 
Reviewed-by: Alberto Garcia 
Signed-off-by: Daniel P. Berrange 
---
 block/qcow2-cluster.c | 17 ++---
 block/qcow2.c |  4 ++--
 block/qcow2.h |  3 +--
 3 files changed, 9 insertions(+), 15 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 100398c..0a27399 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -346,11 +346,9 @@ static int count_contiguous_clusters_by_type(int 
nb_clusters,
 }
 
 /* The crypt function is compatible with the linux cryptoloop
-   algorithm for < 4 GB images. NOTE: out_buf == in_buf is
-   supported */
+   algorithm for < 4 GB images. */
 int qcow2_encrypt_sectors(BDRVQcow2State *s, int64_t sector_num,
-  uint8_t *out_buf, const uint8_t *in_buf,
-  int nb_sectors, bool enc,
+  uint8_t *buf, int nb_sectors, bool enc,
   Error **errp)
 {
 union {
@@ -370,14 +368,12 @@ int qcow2_encrypt_sectors(BDRVQcow2State *s, int64_t 
sector_num,
 }
 if (enc) {
 ret = qcrypto_cipher_encrypt(s->cipher,
- in_buf,
- out_buf,
+ buf, buf,
  512,
  errp);
 } else {
 ret = qcrypto_cipher_decrypt(s->cipher,
- in_buf,
- out_buf,
+ buf, buf,
  512,
  errp);
 }
@@ -385,8 +381,7 @@ int qcow2_encrypt_sectors(BDRVQcow2State *s, int64_t 
sector_num,
 return -1;
 }
 sector_num++;
-in_buf += 512;
-out_buf += 512;
+buf += 512;
 }
 return 0;
 }
@@ -434,7 +429,7 @@ static int coroutine_fn do_perform_cow(BlockDriverState *bs,
 assert(s->cipher);
 assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
 assert((bytes & ~BDRV_SECTOR_MASK) == 0);
-if (qcow2_encrypt_sectors(s, sector, iov.iov_base, iov.iov_base,
+if (qcow2_encrypt_sectors(s, sector, iov.iov_base,
   bytes >> BDRV_SECTOR_BITS, true, &err) < 0) {
 ret = -EIO;
 error_free(err);
diff --git a/block/qcow2.c b/block/qcow2.c
index 98ca6ed..8c28e9c 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1542,7 +1542,7 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState 
*bs, uint64_t offset,
 assert((cur_bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
 Error *err = NULL;
 if (qcow2_encrypt_sectors(s, offset >> BDRV_SECTOR_BITS,
-  cluster_data, cluster_data,
+  cluster_data,
   cur_bytes >> BDRV_SECTOR_BITS,
   false, &err) < 0) {
 error_free(err);
@@ -1638,7 +1638,7 @@ static coroutine_fn int qcow2_co_pwritev(BlockDriverState 
*bs, uint64_t offset,
 qemu_iovec_to_buf(&hd_qiov, 0, cluster_data, hd_qiov.size);
 
 if (qcow2_encrypt_sectors(s, offset >> BDRV_SECTOR_BITS,
-  cluster_data, cluster_data,
+  cluster_data,
   cur_bytes >>BDRV_SECTOR_BITS,
   true, &err) < 0) {
 error_free(err);
diff --git a/block/qcow2.h b/block/qcow2.h
index f8aeb08..f3051f1 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -531,8 +531,7 @@ int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t 
min_size,
 int qcow2_write_l1_entry(BlockDriverState *bs, int l1_index);
 int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset);
 int qcow2_encrypt_sectors(BDRVQcow2State *s, int64_t sector_num,
-  uint8_t *out_buf, const uint8_t *in_buf,
-  int nb_sectors, bool enc, Error **errp);
+  uint8_t *buf, int nb_sectors, bool enc, Error 
**errp);
 
 int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
  unsigned int *bytes, uint64_t *cluster_offset);
-- 
2.9.3

[Qemu-block] [PATCH v6 12/18] qcow2: extend specification to cover LUKS encryption

[Daniel P. Berrange]

Update the qcow2 specification to describe how the LUKS header is
placed inside a qcow2 file, when using LUKS encryption for the
qcow2 payload instead of the legacy AES-CBC encryption

Reviewed-by: Alberto Garcia 
Reviewed-by: Max Reitz 
Signed-off-by: Daniel P. Berrange 
---
 docs/specs/qcow2.txt | 96 
 1 file changed, 96 insertions(+)

diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt
index 80cdfd0..6509b91 100644
--- a/docs/specs/qcow2.txt
+++ b/docs/specs/qcow2.txt
@@ -45,6 +45,7 @@ The first cluster of a qcow2 image contains the file header:
  32 - 35:   crypt_method
 0 for no encryption
 1 for AES encryption
+2 for LUKS encryption
 
  36 - 39:   l1_size
 Number of entries in the active L1 table
@@ -135,6 +136,7 @@ be stored. Each extension has a structure like the 
following:
 0xE2792ACA - Backing file format name
 0x6803f857 - Feature name table
 0x23852875 - Bitmaps extension
+0x0537be77 - Full disk encryption header pointer
 other  - Unknown header extension, can be safely
  ignored
 
@@ -207,6 +209,100 @@ The fields of the bitmaps extension are:
Offset into the image file at which the bitmap directory
starts. Must be aligned to a cluster boundary.
 
+== Full disk encryption header pointer ==
+
+The full disk encryption header must be present if, and only if, the
+'crypt_method' header requires metadata. Currently this is only true
+of the 'LUKS' crypt method. The header extension must be absent for
+other methods.
+
+This header provides the offset at which the crypt method can store
+its additional data, as well as the length of such data.
+
+Byte  0 -  7:   Offset into the image file at which the encryption
+header starts in bytes. Must be aligned to a cluster
+boundary.
+Byte  8 - 15:   Length of the written encryption header in bytes.
+Note actual space allocated in the qcow2 file may
+be larger than this value, since it will be rounded
+to the nearest multiple of the cluster size. Any
+unused bytes in the allocated space will be initialized
+to 0.
+
+For the LUKS crypt method, the encryption header works as follows.
+
+The first 592 bytes of the header clusters will contain the LUKS
+partition header. This is then followed by the key material data areas.
+The size of the key material data areas is determined by the number of
+stripes in the key slot and key size. Refer to the LUKS format
+specification ('docs/on-disk-format.pdf' in the cryptsetup source
+package) for details of the LUKS partition header format.
+
+In the LUKS partition header, the "payload-offset" field will be
+calculated as normal for the LUKS spec. ie the size of the LUKS
+header, plus key material regions, plus padding, relative to the
+start of the LUKS header. Its value is never used in the context
+of qcow2, however, since the qcow2 file format itself defines where
+the real payload offset is.
+
+In the LUKS key slots header, the "key-material-offset" is relative
+to the start of the LUKS header clusters in the qcow2 container,
+not the start of the qcow2 file.
+
+Logically the layout looks like
+
+  +-+
+  | QCow2 header|
+  | QCow2 header extension X|
+  | QCow2 header extension FDE  |
+  | QCow2 header extension ...  |
+  | QCow2 header extension Z|
+  +-+
+  | other QCow2 tables  |
+  . .
+  . .
+  +-+
+  | +-+ |
+  | | LUKS partition header   | |
+  | +-+ |
+  | | LUKS key material 1 | |
+  | +-+ |
+  | | LUKS key material 2 | |
+  | +-+ |
+  | | LUKS key material ...   | |
+  | +-+ |
+  | | LUKS key material 8 | |
+  | +-+ |
+  +-+
+  | QCow2 cluster payload   |
+  . .
+  . .
+  . .
+  | |
+  +-+
+
+== Data encryption ==
+
+When an encryption method is requested in the header, the image payload
+data must be encrypted/decrypted on every write/read. The image headers
+and metadata is never encrypted.
+
+The algorithms used for encryption vary depending on the method
+
+ - AES:
+
+   The AES cipher, in CBC mode, with 256 bit keys.
+
+   Initialization vectors generated using plain64 method, with
+   the virtual disk sector as the 

[Qemu-block] [PATCH v6 03/18] qcow: document another weakness of qcow AES encryption

[Daniel P. Berrange]

Document that use of guest virtual sector numbers as the basis for
the initialization vectors is a potential weakness, when combined
with internal snapshots or multiple images using the same passphrase.
This fixes the formatting of the itemized list too.

Reviewed-by: Max Reitz 
Reviewed-by: Alberto Garcia 
Signed-off-by: Daniel P. Berrange 
---
 qemu-img.texi | 19 ---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/qemu-img.texi b/qemu-img.texi
index c81db3e..d6baae0 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -558,16 +558,29 @@ The use of encryption in qcow and qcow2 images is 
considered to be flawed by
 modern cryptography standards, suffering from a number of design problems:
 
 @itemize @minus
-@item The AES-CBC cipher is used with predictable initialization vectors based
+@item
+The AES-CBC cipher is used with predictable initialization vectors based
 on the sector number. This makes it vulnerable to chosen plaintext attacks
 which can reveal the existence of encrypted data.
-@item The user passphrase is directly used as the encryption key. A poorly
+@item
+The user passphrase is directly used as the encryption key. A poorly
 chosen or short passphrase will compromise the security of the encryption.
-@item In the event of the passphrase being compromised there is no way to
+@item
+In the event of the passphrase being compromised there is no way to
 change the passphrase to protect data in any qcow images. The files must
 be cloned, using a different encryption passphrase in the new file. The
 original file must then be securely erased using a program like shred,
 though even this is ineffective with many modern storage technologies.
+@item
+Initialization vectors used to encrypt sectors are based on the
+guest virtual sector number, instead of the host physical sector. When
+a disk image has multiple internal snapshots this means that data in
+multiple physical sectors is encrypted with the same initialization
+vector. With the CBC mode, this opens the possibility of watermarking
+attacks if the attack can collect multiple sectors encrypted with the
+same IV and some predictable data. Having multiple qcow2 images with
+the same passphrase also exposes this weakness since the passphrase
+is directly used as the key.
 @end itemize
 
 Use of qcow / qcow2 encryption is thus strongly discouraged. Users are
-- 
2.9.3

[Qemu-block] [PATCH v6 00/18]Convert QCow[2] to QCryptoBlock & add LUKS support

[Daniel P. Berrange]

Previously posted:

 v1: https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg00201.html
 v2: https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg05147.html
 v3: https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg05671.html
 v4: https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg02293.html
 v5: https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg04653.html

This series is a continuation of previous work to support LUKS in
QEMU. The existing merged code supports LUKS as a standalone
driver which can be layered over/under any other QEMU block device
driver. This works well when using LUKS over protocol drivers (file,
rbd, iscsi, etc, etc), but has some downsides when combined with
format drivers like qcow2.

If you layer LUKS under qcow2 (eg qcow2 -> luks -> file) then you
cannot get any information about the qcow2 file without first
decrypting it, as both the header and payload are encrypted.

If you layer LUKS over qcow2 (eg luks -> qcow2 -> file) then you
cannot distinguish between a qcow2 file where the guest has done
LUKS encryption from a qcow2 file which qemu has done encryption.
More seriously, when encrypting sectors the guest virtual sector
is used as the input for deriving the initialization vectors.
When internal snapshots are used, this means that multiple sectors
in the qcow2 file may be encrypted with the same initialization
vector. This is a security weakness when combined with certain
cryptographic modes.

Integrating LUKS natively into qcow2 allows us to combine the
best aspects of both layering strategies above. In particular
the header remains unecrypted, but initialization vectors are
generated using physical sector numbers preserving security
when snapshots are used. This is a change from previous postings
of this work, where the IVs were (incorrectly) generated based
on the virtual disk sector.

In a previous posting of this work, Fam had suggested that we
do integration by layering luks over qcow2, but having QEMU
block layer automatically create the luks driver above qcow2
based on the qcow2 header crypt_method field. This is not
possible though, because such a scheme would suffer from the
problem of IVs being generated from the virtual disk sector
instead of physical disk sector. So having LUKS specific
code in the qcow2 block driver is unavoidable. In comparison
to the previous posting though, the amount of code in qcow2.c
has been reduced by allowing re-use of code from block/crypto.c
for handling QemuOpts -> QAPI conversion. So extra lines of
code in qcow2 to support LUKS is < 200.

I have also split the changes to qcow2 up into 2 patches. The
first patch simply introduces use of the QCryptoBlock framework
to qcow2 for the existing (deprecated) AES-CBC encryption method.
The second patch wires up the LUKS support for qcow2. This makes
it clearer which parts of the changes are related to plain code
refactoring, vs enabling the new features. Specifically we can
now see that the LUKS enablement in qcow2 has this footprint:

Changed in v6:

 - Changed QAPI / QemuOpts design to use nested struct/union
   for all encryption parameters (Eric/Kevin)
 - Fix cleanup during error conditions (Alberto)

Changed in v5:

 - Remove accidental use of tabs in spec (Alberto)
 - Clarify payload-offset position semantics (Alberto)
 - Fix leak of QemuOpts in qcow v1 (Alberto)
 - Avoid overwriting existing Error ** (Alberto)
 - Reuse string -> enum convertor for qcow2 encryption format (Alberto)
 - Fix iotests to deal with recent changes on git master

Changed in v4:

 - Change qcow size check to == 0, instead of <= 1 (Alberto/Eric)
 - Fix crash if qcow2 header contains unexpected crypt method (Alberto)
 - Formatting tweaks in qcow2 spec additions (Max)
 - Remove badly merged comment in qapi schema (Max)
 - Don't add iotest number 173 (Max)
 - Fix test-qcrypto-block.c (Max)

Changed in v3:

 - Modify qemu-img to check for 'encryption-format' option too
   and reject it when combined with compression
 - Add check to prevent 'qemu-img amend' changing encryption
   format
 - Ensure crypto layer is able to report correct option names
   in errors. ie luks-key-secret rather than just key-secret
 - Use read -P 0 in test case 174

Changed in v2:

 - Split qcow2 LUKS tests into separate patch
 - Split qcow2 LUKS spec addition into separate patch
 - Use strstart instead of g_str_has_prefix + pointer manipulation
 - Use -1 instead of 1 for error condition when iterating over opts
 - Fix formatting of qemu-img manpage for qcow2 AES flaws list
 - Fix writing zeros in qcow when encrypting sector
 - Don't overwrite input data buffer in qcow2 when encrypting data
 - Use TODO instead of XXX markers
 - Rename qcow2_change_encryption to qcow2_set_up_encryption
 - Add missing QEMU_IO_OPTIONS_NO_FMT variable to iotests
 - Explicitly fill crypto header unused space with zeros
 - Fix byte-swapping of crypto header in qcow2
 - Enforce crypto header offset is multiple of cluster size
 - Move setting 

Re: [Qemu-block] [PATCH for 2.9 v3 10/10] block: Fix bdrv_co_flush early return

[Kevin Wolf]

Am 10.04.2017 um 17:05 hat Fam Zheng geschrieben:
> bdrv_inc_in_flight and bdrv_dec_in_flight are mandatory for
> BDRV_POLL_WHILE to work, even for the shortcut case where flush is
> unnecessary. Move the if block to below bdrv_dec_in_flight, and BTW fix
> the variable declaration position.
> 
> Signed-off-by: Fam Zheng 
> ---
>  block/io.c | 16 +---
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index 00e45ca..bae6947 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -2278,16 +2278,17 @@ static void coroutine_fn bdrv_flush_co_entry(void 
> *opaque)
>  
>  int coroutine_fn bdrv_co_flush(BlockDriverState *bs)
>  {
> -int ret;
> -
> -if (!bs || !bdrv_is_inserted(bs) || bdrv_is_read_only(bs) ||
> -bdrv_is_sg(bs)) {
> -return 0;
> -}
> +int current_gen;
> +int ret = 0;
>  
>  bdrv_inc_in_flight(bs);

As Coverity points out, we're now using bs...

> -int current_gen = bs->write_gen;
> +if (!bs || !bdrv_is_inserted(bs) || bdrv_is_read_only(bs) ||

...before doing the NULL check.

I'm not sure if we even need to have a NULL check here, but we would have
to check all callers to make sure that it's unnecessary. Before commit
29cdb251, it only checked bs->drv and I don't see how that commit
introduced a NULL caller, but maybe one was added later.

In any case, bdrv_co_flush() needs a fix, either remove the NULL check
or do it first.

> +bdrv_is_sg(bs)) {
> +goto early_exit;
> +}

Kevin

[Qemu-block] qemu-io aborts at quit after reopen

[Fam Zheng]

Hi Kevin,

This happens both on master and on your block-next tree:

$ qemu-io -f raw null-co:// -c 'reopen -r'
Unexpected error in bdrv_check_perm() at /stor/work/qemu/block.c:1437:
Block node is read-only
Aborted

It seems bs->read_only and perms go out of sync when bdrv_reopen() toggles the
former.

Something is missing here?

Fam

[Qemu-block] ping Re: [PATCH 3/4] savevm: fix savevm after migration

[Vladimir Sementsov-Ogievskiy]

29.03.2017 18:53, Paolo Bonzini wrote:


On 29/03/2017 17:29, Dr. David Alan Gilbert wrote:

'abort' is not very good too I think. migration is completed, nothing to
abort.. (may be successful migration to file for suspend, some kind of
vm cloning, etc)

There is already migrate_cancel.  Does it make sense to make it
reactivate fds if migration is completed?

It's potentially racy to do that.
Imagine if your migration is almost finished and you issue a migrate_cancel,
what happens?
Maybe it cancelled it.
Maybe it just completed in time - and you really better not be accessing
the disks on the source unless you're sure the destination isn't running.

If you want to avoid races, you probably have to use -S anyway on the
destination and do more handshaking between source and destination.  But
yes, just to be safe it'd be better to add a new flag (-f for HMP,
'cancel-completed-migration':true for QMP or something like that).

Paolo


Looks like we haven't final conclusion on this (solve problems, noted in 
patches 2 and 3). Are someone working on this now?


--
Best regards,
Vladimir

Re: [Qemu-block] [Qemu-stable] [PATCH v2 1/1] qemu-img: wait for convert coroutines to complete

[Peter Lieven]

Am 25.04.2017 um 11:55 schrieb Peter Lieven:

Am 24.04.2017 um 22:13 schrieb Anton Nefedov:


On 24/04/2017 21:16, Peter Lieven wrote:




Am 24.04.2017 um 18:27 schrieb Anton Nefedov :


On 04/21/2017 03:37 PM, Peter Lieven wrote:

Am 21.04.2017 um 14:19 schrieb Anton Nefedov:

On 04/21/2017 01:44 PM, Peter Lieven wrote:

Am 21.04.2017 um 12:04 schrieb Anton Nefedov:
On error path (like i/o error in one of the coroutines), it's required to
 - wait for coroutines completion before cleaning the common structures
 - reenter dependent coroutines so they ever finish

Introduced in 2d9187bc65.

Signed-off-by: Anton Nefedov 
---
[..]




And what if we error out in the read path? Wouldn't be something like this 
easier?


diff --git a/qemu-img.c b/qemu-img.c
index 22f559a..4ff1085 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1903,6 +1903,16 @@ static int convert_do_copy(ImgConvertState *s)
main_loop_wait(false);
}

+/* on error path we need to enter all coroutines that are still
+ * running before cleaning up common structures */
+if (s->ret) {
+for (i = 0; i < s->num_coroutines; i++) {
+ if (s->co[i]) {
+ qemu_coroutine_enter(s->co[i]);
+ }
+}
+}
+
if (s->compressed && !s->ret) {
/* signal EOF to align */
ret = blk_pwrite_compressed(s->target, 0, NULL, 0);


Peter



seemed a bit too daring to me to re-enter every coroutine potentially including 
the ones that yielded waiting for I/O completion.
If that's ok - that is for sure easier :)


I think we should enter every coroutine that is still running and have it 
terminate correctly. It was my mistake that I have not
done this in the original patch. Can you check if the above fixes your test 
cases that triggered the bug?



hi, sorry I'm late with the answer

this segfaults in bdrv_close(). Looks like it tries to finish some i/o which 
coroutine we have already entered and terminated?

(gdb) run
Starting program: /vz/anefedov/qemu-build/us/./qemu-img convert -O qcow2 
./harddisk.hdd.c ./harddisk.hdd
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".
[New Thread 0x7fffeac2d700 (LWP 436020)]
[New Thread 0x7fffe4ed6700 (LWP 436021)]
qemu-img: error while reading sector 20480: Input/output error
qemu-img: error while writing sector 19712: Operation now in progress

Program received signal SIGSEGV, Segmentation fault.
aio_co_wake (co=0x0) at /mnt/code/us-qemu/util/async.c:454
454 ctx = atomic_read(&co->ctx);
(gdb) bt
#0  aio_co_wake (co=0x0) at /mnt/code/us-qemu/util/async.c:454
/* [Anton]: thread_pool_co_cb () here */
#1  0x55634629 in thread_pool_completion_bh (opaque=0x55cfe020) at 
/mnt/code/us-qemu/util/thread-pool.c:189
#2  0x55633b31 in aio_bh_call (bh=0x55cfe0f0) at 
/mnt/code/us-qemu/util/async.c:90
#3  aio_bh_poll (ctx=ctx@entry=0x55cee6d0) at 
/mnt/code/us-qemu/util/async.c:118
#4  0x55636f14 in aio_poll (ctx=ctx@entry=0x55cee6d0, 
blocking=) at /mnt/code/us-qemu/util/aio-posix.c:682
#5  0x555c52d4 in bdrv_drain_recurse (bs=bs@entry=0x55d22560) at 
/mnt/code/us-qemu/block/io.c:164
#6  0x555c5aed in bdrv_drained_begin (bs=bs@entry=0x55d22560) at 
/mnt/code/us-qemu/block/io.c:248
#7  0x55581443 in bdrv_close (bs=0x55d22560) at 
/mnt/code/us-qemu/block.c:2909
#8  bdrv_delete (bs=0x55d22560) at /mnt/code/us-qemu/block.c:3100
#9  bdrv_unref (bs=0x55d22560) at /mnt/code/us-qemu/block.c:4087
#10 0x555baf44 in blk_remove_bs (blk=blk@entry=0x55d22380) at 
/mnt/code/us-qemu/block/block-backend.c:552
#11 0x555bb173 in blk_delete (blk=0x55d22380) at 
/mnt/code/us-qemu/block/block-backend.c:238
#12 blk_unref (blk=blk@entry=0x55d22380) at 
/mnt/code/us-qemu/block/block-backend.c:282
#13 0x5557a22c in img_convert (argc=, argv=) at /mnt/code/us-qemu/qemu-img.c:2359
#14 0x55574189 in main (argc=5, argv=0x7fffe4a0) at 
/mnt/code/us-qemu/qemu-img.c:4464



Peter



/Anton



it seems that this is a bit tricky, can you share how your test case works?

thanks,
peter



how I tested today was basically

diff --git a/qemu-img.c b/qemu-img.c
index 4425aaa..3d2d506 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1788,6 +1788,10 @@ static void coroutine_fn convert_co_do_copy(void *opaque)

 if (status == BLK_DATA) {
 ret = convert_co_read(s, sector_num, n, buf);
+const uint64_t fsector = 10*1024*1024/512;
+if (sector_num <= fsector && fsector < sector_num+n) {
+ret = -EIO;
+}
 if (ret < 0) {
 error_report("error while reading sector %" PRId64
  ": %s", sector_num, strerror(-ret));



I ended up with sth similar to your approach. Can you check this?


Thanks,

Peter


diff --git a/qemu-img.c b/qemu-img.c
index b94fc11..ed9ce9e 100644
--- a/qemu-img.c
+++ b/qemu-img

Re: [Qemu-block] [Qemu-devel] [PATCH] Issue a deprecation warning if the user specifies the "-hdachs" option.

[Eric Blake]

On 04/25/2017 03:08 AM, Thomas Huth wrote:
> If the user needs to specify the disk geometry, the corresponding
> parameters of the "-drive" option should be used instead. "-hdachs"
> is considered as deprecated and might be removed soon.
> 
> Signed-off-by: Thomas Huth 
> ---
>  vl.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/vl.c b/vl.c
> index 0b4ed52..0980213 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3230,6 +3230,8 @@ int main(int argc, char **argv, char **envp)
>  }
>  }
>  }
> +error_report("'-hdachs' is deprecated. Please use '-drive"
> + " ...,cyls=c,heads=h,secs=s,trans=t' instead.");

No trailing dot.  With that fixed,
Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PULL v2 00/12] Block patches

[Peter Maydell]

On 24 April 2017 at 20:19, Jeff Cody  wrote:
> The following changes since commit 4c55b1d0bad8a703f0499fe62e3761a0cd288da3:
>
>   Merge remote-tracking branch 'remotes/armbru/tags/pull-error-2017-04-24' 
> into staging (2017-04-24 14:49:48 +0100)
>
> are available in the git repository at:
>
>   git://github.com/codyprime/qemu-kvm-jtc.git tags/block-pull-request
>
> for you to fetch changes up to ecfa185400ade2abc9915efa924cbad1e15a21a4:
>
>   qemu-iotests: _cleanup_qemu must be called on exit (2017-04-24 15:09:33 
> -0400)
>
> 
> Pull v2, with 32-bit errors fixed.  I don't have OS X to test compile on,
> but I think it is safe to assume the cause of the compile error was the same.
> 
>
> Ashish Mittal (2):
>   block/vxhs.c: Add support for a new block device type called "vxhs"
>   block/vxhs.c: Add qemu-iotests for new block device type "vxhs"
>
> Jeff Cody (10):
>   qemu-iotests: exclude vxhs from image creation via protocol
>   block: add bdrv_set_read_only() helper function
>   block: do not set BDS read_only if copy_on_read enabled
>   block: honor BDRV_O_ALLOW_RDWR when clearing bs->read_only
>   block: code movement
>   block: introduce bdrv_can_set_read_only()
>   block: use bdrv_can_set_read_only() during reopen
>   block/rbd - update variable names to more apt names
>   block/rbd: Add support for reopen()
>   qemu-iotests: _cleanup_qemu must be called on exit
>

Applied, thanks.

-- PMM

Re: [Qemu-block] [Qemu-stable] [PATCH v2 1/1] qemu-img: wait for convert coroutines to complete

[Peter Lieven]

Am 24.04.2017 um 22:13 schrieb Anton Nefedov:


On 24/04/2017 21:16, Peter Lieven wrote:




Am 24.04.2017 um 18:27 schrieb Anton Nefedov :


On 04/21/2017 03:37 PM, Peter Lieven wrote:

Am 21.04.2017 um 14:19 schrieb Anton Nefedov:

On 04/21/2017 01:44 PM, Peter Lieven wrote:

Am 21.04.2017 um 12:04 schrieb Anton Nefedov:
On error path (like i/o error in one of the coroutines), it's required to
 - wait for coroutines completion before cleaning the common structures
 - reenter dependent coroutines so they ever finish

Introduced in 2d9187bc65.

Signed-off-by: Anton Nefedov 
---
[..]




And what if we error out in the read path? Wouldn't be something like this 
easier?


diff --git a/qemu-img.c b/qemu-img.c
index 22f559a..4ff1085 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1903,6 +1903,16 @@ static int convert_do_copy(ImgConvertState *s)
main_loop_wait(false);
}

+/* on error path we need to enter all coroutines that are still
+ * running before cleaning up common structures */
+if (s->ret) {
+for (i = 0; i < s->num_coroutines; i++) {
+ if (s->co[i]) {
+ qemu_coroutine_enter(s->co[i]);
+ }
+}
+}
+
if (s->compressed && !s->ret) {
/* signal EOF to align */
ret = blk_pwrite_compressed(s->target, 0, NULL, 0);


Peter



seemed a bit too daring to me to re-enter every coroutine potentially including 
the ones that yielded waiting for I/O completion.
If that's ok - that is for sure easier :)


I think we should enter every coroutine that is still running and have it 
terminate correctly. It was my mistake that I have not
done this in the original patch. Can you check if the above fixes your test 
cases that triggered the bug?



hi, sorry I'm late with the answer

this segfaults in bdrv_close(). Looks like it tries to finish some i/o which 
coroutine we have already entered and terminated?

(gdb) run
Starting program: /vz/anefedov/qemu-build/us/./qemu-img convert -O qcow2 
./harddisk.hdd.c ./harddisk.hdd
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".
[New Thread 0x7fffeac2d700 (LWP 436020)]
[New Thread 0x7fffe4ed6700 (LWP 436021)]
qemu-img: error while reading sector 20480: Input/output error
qemu-img: error while writing sector 19712: Operation now in progress

Program received signal SIGSEGV, Segmentation fault.
aio_co_wake (co=0x0) at /mnt/code/us-qemu/util/async.c:454
454 ctx = atomic_read(&co->ctx);
(gdb) bt
#0  aio_co_wake (co=0x0) at /mnt/code/us-qemu/util/async.c:454
/* [Anton]: thread_pool_co_cb () here */
#1  0x55634629 in thread_pool_completion_bh (opaque=0x55cfe020) at 
/mnt/code/us-qemu/util/thread-pool.c:189
#2  0x55633b31 in aio_bh_call (bh=0x55cfe0f0) at 
/mnt/code/us-qemu/util/async.c:90
#3  aio_bh_poll (ctx=ctx@entry=0x55cee6d0) at 
/mnt/code/us-qemu/util/async.c:118
#4  0x55636f14 in aio_poll (ctx=ctx@entry=0x55cee6d0, 
blocking=) at /mnt/code/us-qemu/util/aio-posix.c:682
#5  0x555c52d4 in bdrv_drain_recurse (bs=bs@entry=0x55d22560) at 
/mnt/code/us-qemu/block/io.c:164
#6  0x555c5aed in bdrv_drained_begin (bs=bs@entry=0x55d22560) at 
/mnt/code/us-qemu/block/io.c:248
#7  0x55581443 in bdrv_close (bs=0x55d22560) at 
/mnt/code/us-qemu/block.c:2909
#8  bdrv_delete (bs=0x55d22560) at /mnt/code/us-qemu/block.c:3100
#9  bdrv_unref (bs=0x55d22560) at /mnt/code/us-qemu/block.c:4087
#10 0x555baf44 in blk_remove_bs (blk=blk@entry=0x55d22380) at 
/mnt/code/us-qemu/block/block-backend.c:552
#11 0x555bb173 in blk_delete (blk=0x55d22380) at 
/mnt/code/us-qemu/block/block-backend.c:238
#12 blk_unref (blk=blk@entry=0x55d22380) at 
/mnt/code/us-qemu/block/block-backend.c:282
#13 0x5557a22c in img_convert (argc=, argv=) at /mnt/code/us-qemu/qemu-img.c:2359
#14 0x55574189 in main (argc=5, argv=0x7fffe4a0) at 
/mnt/code/us-qemu/qemu-img.c:4464



Peter



/Anton



it seems that this is a bit tricky, can you share how your test case works?

thanks,
peter



how I tested today was basically

diff --git a/qemu-img.c b/qemu-img.c
index 4425aaa..3d2d506 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1788,6 +1788,10 @@ static void coroutine_fn convert_co_do_copy(void *opaque)

 if (status == BLK_DATA) {
 ret = convert_co_read(s, sector_num, n, buf);
+const uint64_t fsector = 10*1024*1024/512;
+if (sector_num <= fsector && fsector < sector_num+n) {
+ret = -EIO;
+}
 if (ret < 0) {
 error_report("error while reading sector %" PRId64
  ": %s", sector_num, strerror(-ret));



I ended up with sth similar to your approach. Can you check this?


Thanks,

Peter


diff --git a/qemu-img.c b/qemu-img.c
index b94fc11..ed9ce9e 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1761,13 +1761,13 @@ static void corouti

[Qemu-block] [PATCH] Issue a deprecation warning if the user specifies the "-hdachs" option.

[Thomas Huth]

If the user needs to specify the disk geometry, the corresponding
parameters of the "-drive" option should be used instead. "-hdachs"
is considered as deprecated and might be removed soon.

Signed-off-by: Thomas Huth 
---
 vl.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/vl.c b/vl.c
index 0b4ed52..0980213 100644
--- a/vl.c
+++ b/vl.c
@@ -3230,6 +3230,8 @@ int main(int argc, char **argv, char **envp)
 }
 }
 }
+error_report("'-hdachs' is deprecated. Please use '-drive"
+ " ...,cyls=c,heads=h,secs=s,trans=t' instead.");
 break;
 case QEMU_OPTION_numa:
 opts = qemu_opts_parse_noisily(qemu_find_opts("numa"),
-- 
1.8.3.1

Re: [Qemu-block] [Qemu-devel] [QEMU-2.8] Source QEMU crashes with: "bdrv_co_pwritev: Assertion `!(bs->open_flags & BDRV_O_INACTIVE)' failed"

[Hailiang Zhang]

On 2017/4/24 15:59, Kashyap Chamarthy wrote:

On Sat, Apr 22, 2017 at 05:23:49PM +0800, Hailiang Zhang wrote:

Hi,

Hi Hailiang,


I think the bellow patch can fix your problme.
[PATCH 2/4] qmp-cont: invalidate on RUN_STATE_PRELAUNCH
https://patchwork.kernel.org/patch/9591885/

Hmm, the above patch ("qmp-cont: invalidate on RUN_STATE_PRELAUNCH") is
not merged in Git, as it's stalled on design discussion between Kevin
Wolf and Vladimir.

And the below patch, from you, seems to be not submitted upstream (2.8
stable tree, perhaps).  Do you intend to do so?


Er, since this patch does the same thing with the above patch, I'm not sure if 
i should
send this patch ...


Actually, we encounter the same problem in our test, we fix it with the follow 
patch:

  From 0e4d6d706afd9909b5fd71536b45c58af60892f8 Mon Sep 17 00:00:00 2001
  From: zhanghailiang
  Date: Tue, 21 Mar 2017 09:44:36 +0800
  Subject: [PATCH] migration: Re-activate blocks whenever migration been
   cancelled

  In commit 1d2acc3162d9c7772510c973f446353fbdd1f9a8, we try to fix the bug
  'bdrv_co_do_pwritev: Assertion `!(bs->open_flags & 0x0800)' failed'
  which occured in migration cancelling process.

  But it seems that we didn't cover all the cases, we caught such a case 
which
  slipped from the old fixup in our test: if libvirtd cancelled the 
migration
  process for a shutting down VM, it will send 'system_reset' command first,
  and then 'cont' command behind, after VM resumes to run, it will trigger 
the above
  error reports, because we didn't regain the control of blocks for VM.

  Signed-off-by: zhanghailiang
  Signed-off-by: Hongyang Yang
  ---
   block.c   | 12 +++-
   include/block/block.h |  1 +
   include/migration/migration.h |  3 ---
   migration/migration.c |  7 +--
   qmp.c |  4 +---
   5 files changed, 14 insertions(+), 13 deletions(-)

[...]