Re: [Qemu-block] [Qemu-devel] [PATCH for-2.10 2/5] block: Allow reopen rw without BDRV_O_ALLOW_RDWR

[Eric Blake]

On 08/03/2017 10:21 AM, Eric Blake wrote:
> On 08/03/2017 10:02 AM, Kevin Wolf wrote:
>> BDRV_O_ALLOW_RDWR is a flag that tells whether qemu can internally
>> reopen a node read-write temporarily because the user requested
>> read-write for the top-level image, but qemu decided that read-only is
>> enough for this node (a backing file).
>>
>> bdrv_reopen() is different, it is also used for cases where the user
>> changed their mind and wants to update the options. There is no reason
>> to forbid making a node read-write in that case.
> 
> Hmm, I wonder.  https://bugzilla.redhat.com/show_bug.cgi?id=1465320
> details a failure when starting qemu with a read-write NBD disk, then
> taking several snapshots (nbd <- snap1 <- snap2 <- snap3), then where
> intermediate commit (snap2 into nbd) works but live commit (snap3 into
> nbd) fails with a message that nbd does not support reopening.  I'm
> presuming that your series may help to address that; I'll give it a spin
> and see what happens.

Nope, even with your patches, I'm still getting:

{'execute':'block-commit','arguments':{'device':'drive-image1','top':'bar2'}}
{"return": {}}
{"timestamp": {"seconds": 1501811285, "microseconds": 439748}, "event":
"BLOCK_JOB_COMPLETED", "data": {"device": "drive-image1", "len":
2097152, "offset": 2097152, "speed": 0, "type": "commit"}}

{'execute':'block-commit','arguments':{'device':'drive-image1','top':'bar3'}}
{"error": {"class": "GenericError", "desc": "Block format 'nbd' used by
node '#block048' does not support reopening files"}}

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



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH v4 17/22] libqtest: Add qmp_args() helper

[Eric Blake]

Similar to the previous helper, we can reduce the boilerplate
of most callers by passing the command name separately from
the interpolated arguments.  Adjust the majority of the callers
that can use the new helpers; in the process, fixing a few
places where we would have failed -Wformat-nonliteral.  The
new helper function already uses GCC_FMT_ATTR to match the
underlying use of qobject_from_jsonv().

Signed-off-by: Eric Blake 
---
 tests/libqtest.h   |  20 ++
 tests/libqtest.c   |  29 +
 tests/ahci-test.c  |  19 +++---
 tests/device-introspect-test.c |   4 +-
 tests/drive_del-test.c |  10 +--
 tests/fdc-test.c   |   8 +--
 tests/libqos/libqos.c  |   7 +--
 tests/libqos/pci-pc.c  |   8 +--
 tests/pc-cpu-test.c|   8 +--
 tests/postcopy-test.c  |  30 +++--
 tests/qom-test.c   |   9 +--
 tests/test-netfilter.c | 139 -
 tests/test-x86-cpuid-compat.c  |   6 +-
 tests/tmp105-test.c|   9 +--
 tests/usb-hcd-uhci-test.c  |  14 ++---
 tests/usb-hcd-xhci-test.c  |  25 ++--
 tests/vhost-user-test.c|  12 +---
 tests/virtio-blk-test.c|  12 +---
 tests/virtio-scsi-test.c   |  13 +---
 tests/virtio-serial-test.c |  12 +---
 20 files changed, 167 insertions(+), 227 deletions(-)

diff --git a/tests/libqtest.h b/tests/libqtest.h
index e0d87d035a..86ca7fa581 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -499,6 +499,26 @@ QDict *qmp_cmd(const char *cmd);
 void qmp_cmd_async(const char *cmd);

 /**
+ * qmp_args:
+ * @cmd: QMP command to send to QEMU.
+ * @fmt...: Arguments for the command; formats arguments through
+ * json-lexer.c (only understands '%(PRI[ud]64|(l|ll)?[du]|[ipsf%])').
+ *
+ * Sends a QMP message to QEMU and returns the response.
+ */
+QDict *qmp_args(const char *cmd, const char *fmt, ...) GCC_FMT_ATTR(2, 3);
+
+/**
+ * qmp_args_async:
+ * @cmd: QMP command to send to QEMU.
+ * @fmt...: Arguments for the command; formats arguments through
+ * json-lexer.c (only understands '%(PRI[ud]64|(l|ll)?[du]|[ipsf%])').
+ *
+ * Sends a QMP message to QEMU and leaves the response in the stream.
+ */
+void qmp_args_async(const char *cmd, const char *fmt, ...) GCC_FMT_ATTR(2, 3);
+
+/**
  * qmp_discard_response:
  *
  * Read and discard a QMP response, typically after qmp_async().
diff --git a/tests/libqtest.c b/tests/libqtest.c
index 3926a4d481..49786cf2d7 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -869,6 +869,35 @@ void qmp_cmd_async(const char *cmd)
 qtest_qmp_send(global_qtest, "{'execute':%s}", cmd);
 }

+static void qmp_args_dict_async(const char *cmd, QDict *args)
+{
+assert(args);
+qtest_qmp_send(global_qtest, "{'execute':%s, 'arguments':%p}", cmd, args);
+}
+
+QDict *qmp_args(const char *cmd, const char *fmt, ...)
+{
+va_list ap;
+QObject *obj;
+
+va_start(ap, fmt);
+obj = qobject_from_jsonv(fmt, ap);
+va_end(ap);
+qmp_args_dict_async(cmd, qobject_to_qdict(obj));
+return qtest_qmp_receive(global_qtest);
+}
+
+void qmp_args_async(const char *cmd, const char *fmt, ...)
+{
+va_list ap;
+QObject *obj;
+
+va_start(ap, fmt);
+obj = qobject_from_jsonv(fmt, ap);
+va_end(ap);
+qmp_args_dict_async(cmd, qobject_to_qdict(obj));
+}
+
 void qmp_discard_response(void)
 {
 QDict *response = qtest_qmp_receive(global_qtest);
diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index 63d52edfca..ee8a539cf6 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -1588,14 +1588,12 @@ static void test_atapi_tray(void)
 atapi_wait_tray(false);

 /* Remove media */
-qmp_async("{'execute': 'blockdev-open-tray', "
-   "'arguments': {'device': 'drive0'}}");
+qmp_args_async("blockdev-open-tray", "{'device': 'drive0'}");
 atapi_wait_tray(true);
 rsp = qmp_receive();
 QDECREF(rsp);

-qmp_async("{'execute': 'x-blockdev-remove-medium', "
-  "'arguments': {'device': 'drive0'}}");
+qmp_args_async("x-blockdev-remove-medium", "{'device': 'drive0'}");
 qmp_discard_response();

 /* Test the tray without a medium */
@@ -1606,17 +1604,16 @@ static void test_atapi_tray(void)
 atapi_wait_tray(true);

 /* Re-insert media */
-qmp_async("{'execute': 'blockdev-add', 'arguments': {"
-  "  'node-name': 'node0', 'driver': 'raw', "
-  "  'file': { 'driver': 'file', 'filename': %s }}}", iso);
+qmp_args_async("blockdev-add", "{'node-name': 'node0', "
+   "'driver': 'raw', "
+   "'file': { 'driver': 'file', 'filename': %s }}", iso);
 qmp_discard_response();
-qmp_async("{'execute': 'x-blockdev-insert-medium',"
-  "'arguments': { 'device': 'drive0', 'node-name': 'node0' }}");
+qmp_args_async("x-blockdev-insert-medium",
+   "{ 'device': 'drive0', 'node-name': 'node0' }");
  

[Qemu-block] [PATCH v4 16/22] libqtest: Add qmp_cmd() helper

[Eric Blake]

Now that we've asserted that all of our interpolated QMP commands
include 'execute', we can reduce some of the caller boilerplate
by providing a helpr function to wrap commands with no arguments
(later patches will cover commands with arguments).

Adjust all callers that can use the new helpers; in the process,
fixing a couple of places where we would have failed
-Wformat-nonliteral.  Likewise, libqos.c no longer needs
qmp_execute(), which in turn fixes the fact that it is better
to interpolate JSON strings through qobject_from_json() than
through sprintf().

The current name is long, but temporary: later patches will
remove all other uses of qmp(), and then make the mass rename
of qmp_cmd() down to qmp().

Signed-off-by: Eric Blake 
---
 tests/libqtest.h   | 16 
 tests/libqtest.c   | 13 -
 tests/ahci-test.c  |  4 +---
 tests/boot-order-test.c|  2 +-
 tests/ide-test.c   |  2 +-
 tests/libqos/ahci.c|  4 ++--
 tests/libqos/libqos.c  | 16 ++--
 tests/numa-test.c  |  2 +-
 tests/postcopy-test.c  |  8 
 tests/q35-test.c   |  2 +-
 tests/qmp-test.c   |  8 
 tests/qom-test.c   |  2 +-
 tests/test-filter-mirror.c |  2 +-
 tests/test-filter-redirector.c |  4 ++--
 tests/test-x86-cpuid-compat.c  |  2 +-
 tests/virtio-net-test.c| 13 ++---
 tests/vmgenid-test.c   |  2 +-
 tests/wdt_ib700-test.c |  2 +-
 18 files changed, 58 insertions(+), 46 deletions(-)

diff --git a/tests/libqtest.h b/tests/libqtest.h
index 684cfb3507..e0d87d035a 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -483,6 +483,22 @@ QDict *qmp_raw(const char *msg);
 void qmp_async(const char *fmt, ...);

 /**
+ * qmp_cmd:
+ * @cmd: QMP command, with no arguments.
+ *
+ * Sends a QMP message to QEMU and returns the response.
+ */
+QDict *qmp_cmd(const char *cmd);
+
+/**
+ * qmp_cmd_async:
+ * @cmd: QMP command, with no arguments.
+ *
+ * Sends a QMP message to QEMU and leaves the response in the stream.
+ */
+void qmp_cmd_async(const char *cmd);
+
+/**
  * qmp_discard_response:
  *
  * Read and discard a QMP response, typically after qmp_async().
diff --git a/tests/libqtest.c b/tests/libqtest.c
index 2df01682c0..3926a4d481 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -858,6 +858,17 @@ void qmp_async(const char *fmt, ...)
 va_end(ap);
 }

+QDict *qmp_cmd(const char *cmd)
+{
+qmp_cmd_async(cmd);
+return qtest_qmp_receive(global_qtest);
+}
+
+void qmp_cmd_async(const char *cmd)
+{
+qtest_qmp_send(global_qtest, "{'execute':%s}", cmd);
+}
+
 void qmp_discard_response(void)
 {
 QDict *response = qtest_qmp_receive(global_qtest);
@@ -890,7 +901,7 @@ void qtest_cb_for_every_machine(void (*cb)(const char 
*machine))
 const char *mname;

 qtest_start("-machine none");
-response = qmp("{ 'execute': 'query-machines' }");
+response = qmp_cmd("query-machines");
 g_assert(response);
 list = qdict_get_qlist(response, "return");
 g_assert(list);
diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index 9460843a9f..63d52edfca 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -1350,7 +1350,6 @@ static void test_flush_migrate(void)
 AHCIQState *src, *dst;
 AHCICommand *cmd;
 uint8_t px;
-const char *s;
 char *uri = g_strdup_printf("unix:%s", mig_socket);

 prepare_blkdebug_script(debug_path, "flush_to_disk");
@@ -1386,8 +1385,7 @@ static void test_flush_migrate(void)
 ahci_migrate(src, dst, uri);

 /* Complete the command */
-s = "{'execute':'cont' }";
-qmp_async(s);
+qmp_cmd_async("cont");
 qmp_eventwait("RESUME");
 ahci_command_wait(dst, cmd);
 ahci_command_verify(dst, cmd);
diff --git a/tests/boot-order-test.c b/tests/boot-order-test.c
index 4114720236..0bee6b 100644
--- a/tests/boot-order-test.c
+++ b/tests/boot-order-test.c
@@ -38,7 +38,7 @@ static void test_a_boot_order(const char *machine,
 qtest_start(args);
 actual = read_boot_order();
 g_assert_cmphex(actual, ==, expected_boot);
-qmp_async("{ 'execute': 'system_reset' }");
+qmp_cmd_async("system_reset");
 /*
  * system_reset only requests reset.  We get a RESET event after
  * the actual reset completes.  Need to wait for that.
diff --git a/tests/ide-test.c b/tests/ide-test.c
index 757af7cd1d..56a02b1c7f 100644
--- a/tests/ide-test.c
+++ b/tests/ide-test.c
@@ -651,7 +651,7 @@ static void test_retry_flush(const char *machine)
 qmp_eventwait("STOP");

 /* Complete the command */
-qmp_async("{'execute':'cont' }");
+qmp_cmd_async("cont");
 qmp_discard_response();

 /* Check registers */
diff --git a/tests/libqos/ahci.c b/tests/libqos/ahci.c
index 1ca7f456b5..06b9ce3a13 100644
--- a/tests/libqos/ahci.c
+++ b/tests/libqos/ahci.c
@@ -668,7 +668,7 @@ void ahci_exec(AHCIQState *ahci, uint8_t port,
  

[Qemu-block] [PATCH v4 20/22] tests/libqos/pci: Clean up string interpolation into QMP input

[Eric Blake]

From: Markus Armbruster 

Leaving interpolation into JSON to qmp() is more robust than building
QMP input manually, as explained in previous commits.

The case in qpci_plug_device_test() is a bit complicated: it
interpolates several JSON object members, not just a value.  Clean it
up by passing them in as QDict rather than string, so we can leave
interpolation to qmp() here and to qobject_from_jsonf() in callers.

Signed-off-by: Markus Armbruster 
Message-Id: <1500645206-13559-7-git-send-email-arm...@redhat.com>
[use qmp_args_dict for a slightly smaller diff, fix '%s' typo in ivshmem-test]
Signed-off-by: Eric Blake 
---
 tests/libqos/pci.h  |  2 +-
 tests/ivshmem-test.c| 10 +-
 tests/libqos/pci.c  | 32 +---
 tests/virtio-blk-test.c |  5 -
 4 files changed, 27 insertions(+), 22 deletions(-)

diff --git a/tests/libqos/pci.h b/tests/libqos/pci.h
index ed480614ff..c981061703 100644
--- a/tests/libqos/pci.h
+++ b/tests/libqos/pci.h
@@ -109,6 +109,6 @@ void qpci_iounmap(QPCIDevice *dev, QPCIBar addr);
 QPCIBar qpci_legacy_iomap(QPCIDevice *dev, uint16_t addr);

 void qpci_plug_device_test(const char *driver, const char *id,
-   uint8_t slot, const char *opts);
+   uint8_t slot, QDict *extra_args);
 void qpci_unplug_acpi_device_test(const char *id, uint8_t slot);
 #endif
diff --git a/tests/ivshmem-test.c b/tests/ivshmem-test.c
index 37763425ee..4cb5a030da 100644
--- a/tests/ivshmem-test.c
+++ b/tests/ivshmem-test.c
@@ -14,6 +14,7 @@
 #include "libqos/libqos-pc.h"
 #include "libqos/libqos-spapr.h"
 #include "libqtest.h"
+#include "qapi/qmp/qjson.h"
 #include "qemu-common.h"

 #define TMPSHMSIZE (1 << 20)
@@ -419,19 +420,18 @@ static void test_ivshmem_server_irq(void)
 static void test_ivshmem_hotplug(void)
 {
 const char *arch = qtest_get_arch();
-gchar *opts;
+QObject *extra_args = qobject_from_jsonf("{ 'shm': %s, 'size': '1M' }",
+ tmpshm);

 qtest_start("");

-opts = g_strdup_printf("'shm': '%s', 'size': '1M'", tmpshm);
-
-qpci_plug_device_test("ivshmem", "iv1", PCI_SLOT_HP, opts);
+qpci_plug_device_test("ivshmem", "iv1", PCI_SLOT_HP,
+  qobject_to_qdict(extra_args));
 if (strcmp(arch, "ppc64") != 0) {
 qpci_unplug_acpi_device_test("iv1", PCI_SLOT_HP);
 }

 qtest_end();
-g_free(opts);
 }

 static void test_ivshmem_memdev(void)
diff --git a/tests/libqos/pci.c b/tests/libqos/pci.c
index 2dcdeade2a..68623795b7 100644
--- a/tests/libqos/pci.c
+++ b/tests/libqos/pci.c
@@ -14,6 +14,7 @@
 #include "libqos/pci.h"

 #include "hw/pci/pci_regs.h"
+#include "qapi/qmp/qjson.h"
 #include "qemu/host-utils.h"

 void qpci_device_foreach(QPCIBus *bus, int vendor_id, int device_id,
@@ -392,22 +393,23 @@ QPCIBar qpci_legacy_iomap(QPCIDevice *dev, uint16_t addr)
 }

 void qpci_plug_device_test(const char *driver, const char *id,
-   uint8_t slot, const char *opts)
+   uint8_t slot, QDict *extra_args)
 {
-QDict *response;
-char *cmd;
-
-cmd = g_strdup_printf("{'execute': 'device_add',"
-  " 'arguments': {"
-  "   'driver': '%s',"
-  "   'addr': '%d',"
-  "   %s%s"
-  "   'id': '%s'"
-  "}}", driver, slot,
-  opts ? opts : "", opts ? "," : "",
-  id);
-response = qmp(cmd);
-g_free(cmd);
+char addr[8];
+QDict *args, *response;
+
+sprintf(addr, "%d", slot);
+args = qobject_to_qdict(
+qobject_from_jsonf("{ 'driver': %s, 'addr': %s, 'id': %s}",
+   driver, addr, id));
+
+if (extra_args) {
+qdict_join(args, extra_args, true);
+QDECREF(extra_args);
+}
+
+response = qmp_args_dict("device_add", args);
+
 g_assert(response);
 g_assert(!qdict_haskey(response, "error"));
 QDECREF(response);
diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
index 649ba03c92..78a9ebfb0d 100644
--- a/tests/virtio-blk-test.c
+++ b/tests/virtio-blk-test.c
@@ -16,6 +16,7 @@
 #include "libqos/virtio-pci.h"
 #include "libqos/virtio-mmio.h"
 #include "libqos/malloc-generic.h"
+#include "qapi/qmp/qjson.h"
 #include "qemu/bswap.h"
 #include "standard-headers/linux/virtio_ids.h"
 #include "standard-headers/linux/virtio_config.h"
@@ -656,12 +657,13 @@ static void pci_hotplug(void)
 QVirtioPCIDevice *dev;
 QOSState *qs;
 const char *arch = qtest_get_arch();
+QObject *extra_args = qobject_from_jsonf("{ 'drive': 'drive1' }");

 qs = pci_test_start();

 /* plug secondary disk */
 qpci_plug_device_test("virtio-blk-pci", "drv1", PCI_SLOT_HP,
-  "'drive': 'drive1'");
+  

[Qemu-block] [PATCH v4 22/22] libqtest: Rename qmp_cmd() to qmp()

[Eric Blake]

Now that the previous patch got rid of the old signature of qmp(),
we can go back to using the shortest possible name for the common
action.

Performed mechanically with:
 for f in $(git grep -l qmp_cmd tests/); do
   case $f in *qemu-iotests*) continue;; esac
   sed -i s/qmp_cmd/qmp/ $f; done

Signed-off-by: Eric Blake 
---
 tests/libqtest.h   | 10 +-
 tests/libqtest.c   |  8 
 tests/ahci-test.c  |  2 +-
 tests/boot-order-test.c|  2 +-
 tests/ide-test.c   |  2 +-
 tests/libqos/ahci.c|  4 ++--
 tests/libqos/libqos.c  |  4 ++--
 tests/numa-test.c  |  2 +-
 tests/postcopy-test.c  |  8 
 tests/q35-test.c   |  2 +-
 tests/qmp-test.c   |  8 
 tests/qom-test.c   |  2 +-
 tests/test-filter-mirror.c |  2 +-
 tests/test-filter-redirector.c |  4 ++--
 tests/test-x86-cpuid-compat.c  |  2 +-
 tests/virtio-net-test.c|  6 +++---
 tests/vmgenid-test.c   |  2 +-
 tests/wdt_ib700-test.c |  2 +-
 18 files changed, 36 insertions(+), 36 deletions(-)

diff --git a/tests/libqtest.h b/tests/libqtest.h
index 04b36a7b11..874f0f9c35 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -465,20 +465,20 @@ static inline void qtest_end(void)
 QDict *qmp_raw(const char *msg);

 /**
- * qmp_cmd:
+ * qmp:
  * @cmd: QMP command, with no arguments.
  *
  * Sends a QMP message to QEMU and returns the response.
  */
-QDict *qmp_cmd(const char *cmd);
+QDict *qmp(const char *cmd);

 /**
- * qmp_cmd_async:
+ * qmp_async:
  * @cmd: QMP command, with no arguments.
  *
  * Sends a QMP message to QEMU and leaves the response in the stream.
  */
-void qmp_cmd_async(const char *cmd);
+void qmp_async(const char *cmd);

 /**
  * qmp_args_dict:
@@ -512,7 +512,7 @@ void qmp_args_async(const char *cmd, const char *fmt, ...) 
GCC_FMT_ATTR(2, 3);
 /**
  * qmp_discard_response:
  *
- * Read and discard a QMP response, typically after qmp_cmd_async().
+ * Read and discard a QMP response, typically after qmp_async().
  */
 void qmp_discard_response(void);

diff --git a/tests/libqtest.c b/tests/libqtest.c
index 4597d4ac66..50e6c312a4 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -834,13 +834,13 @@ QDict *qmp_raw(const char *msg)
 return qtest_qmp_receive(global_qtest);
 }

-QDict *qmp_cmd(const char *cmd)
+QDict *qmp(const char *cmd)
 {
-qmp_cmd_async(cmd);
+qmp_async(cmd);
 return qtest_qmp_receive(global_qtest);
 }

-void qmp_cmd_async(const char *cmd)
+void qmp_async(const char *cmd)
 {
 qtest_qmp_send(global_qtest, "{'execute':%s}", cmd);
 }
@@ -912,7 +912,7 @@ void qtest_cb_for_every_machine(void (*cb)(const char 
*machine))
 const char *mname;

 qtest_start("-machine none");
-response = qmp_cmd("query-machines");
+response = qmp("query-machines");
 g_assert(response);
 list = qdict_get_qlist(response, "return");
 g_assert(list);
diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index ee8a539cf6..96a7249157 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -1385,7 +1385,7 @@ static void test_flush_migrate(void)
 ahci_migrate(src, dst, uri);

 /* Complete the command */
-qmp_cmd_async("cont");
+qmp_async("cont");
 qmp_eventwait("RESUME");
 ahci_command_wait(dst, cmd);
 ahci_command_verify(dst, cmd);
diff --git a/tests/boot-order-test.c b/tests/boot-order-test.c
index 0bee6b..cd883c05c7 100644
--- a/tests/boot-order-test.c
+++ b/tests/boot-order-test.c
@@ -38,7 +38,7 @@ static void test_a_boot_order(const char *machine,
 qtest_start(args);
 actual = read_boot_order();
 g_assert_cmphex(actual, ==, expected_boot);
-qmp_cmd_async("system_reset");
+qmp_async("system_reset");
 /*
  * system_reset only requests reset.  We get a RESET event after
  * the actual reset completes.  Need to wait for that.
diff --git a/tests/ide-test.c b/tests/ide-test.c
index 56a02b1c7f..5819c2ed24 100644
--- a/tests/ide-test.c
+++ b/tests/ide-test.c
@@ -651,7 +651,7 @@ static void test_retry_flush(const char *machine)
 qmp_eventwait("STOP");

 /* Complete the command */
-qmp_cmd_async("cont");
+qmp_async("cont");
 qmp_discard_response();

 /* Check registers */
diff --git a/tests/libqos/ahci.c b/tests/libqos/ahci.c
index 06b9ce3a13..8bdf8ef246 100644
--- a/tests/libqos/ahci.c
+++ b/tests/libqos/ahci.c
@@ -668,7 +668,7 @@ void ahci_exec(AHCIQState *ahci, uint8_t port,
 g_assert_cmpint(rc, ==, 0);
 }
 if (opts->error) {
-qmp_cmd_async("cont");
+qmp_async("cont");
 qmp_eventwait("RESUME");
 }

@@ -706,7 +706,7 @@ AHCICommand *ahci_guest_io_halt(AHCIQState *ahci, uint8_t 
port,
 void ahci_guest_io_resume(AHCIQState *ahci, AHCICommand *cmd)
 {
 /* Complete the command */
-qmp_cmd_async("cont");
+qmp_async("cont");
 qmp_eventwait("RESUME");
 ahci_command_wait(ahci, 

[Qemu-block] [PATCH v4 14/22] libqtest: Separate qmp_discard_response() from command

[Eric Blake]

Upcoming patches will be adding new convenience methods for
constructing QMP commands.  But making every variation of sending
support every variation of response handling becomes unwieldy;
it's easier to specify that discarding a JSON response is
unassociated with sending the command, where qmp_async() already
fits the bill for sending a command without tying up a reference
to the response.

Doing this renders qtest_qmp[v]_discard_response() unused.

Bonus: gets rid of a non-literal format string, which is a step
towards compile-time format string checking without triggering
-Wformat-nonliteral.

Signed-off-by: Eric Blake 
---
 tests/libqtest.h   | 27 ++-
 tests/libqtest.c   | 30 ++
 tests/ahci-test.c  | 20 ++--
 tests/boot-order-test.c|  2 +-
 tests/drive_del-test.c |  5 +++--
 tests/fdc-test.c   | 11 ++-
 tests/ide-test.c   |  5 ++---
 tests/postcopy-test.c  |  3 ++-
 tests/test-filter-mirror.c |  3 ++-
 tests/test-filter-redirector.c |  6 --
 tests/virtio-blk-test.c| 21 -
 11 files changed, 50 insertions(+), 83 deletions(-)

diff --git a/tests/libqtest.h b/tests/libqtest.h
index 917ec5cf92..6bae0223aa 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -48,16 +48,6 @@ QTestState *qtest_init_without_qmp_handshake(const char 
*extra_args);
 void qtest_quit(QTestState *s);

 /**
- * qtest_qmp_discard_response:
- * @s: #QTestState instance to operate on.
- * @fmt...: QMP message to send to qemu; formats arguments through
- * json-lexer.c (only understands '%(PRI[ud]64|(l|ll)?[du]|[ipsf%])').
- *
- * Sends a QMP message to QEMU and consumes the response.
- */
-void qtest_qmp_discard_response(QTestState *s, const char *fmt, ...);
-
-/**
  * qtest_qmp:
  * @s: #QTestState instance to operate on.
  * @fmt...: QMP message to send to qemu; formats arguments through
@@ -78,17 +68,6 @@ QDict *qtest_qmp(QTestState *s, const char *fmt, ...);
 void qtest_async_qmp(QTestState *s, const char *fmt, ...);

 /**
- * qtest_qmpv_discard_response:
- * @s: #QTestState instance to operate on.
- * @fmt: QMP message to send to QEMU; formats arguments through
- * json-lexer.c (only understands '%(PRI[ud]64|(l|ll)?[du]|[ipsf%])').
- * @ap: QMP message arguments
- *
- * Sends a QMP message to QEMU and consumes the response.
- */
-void qtest_qmpv_discard_response(QTestState *s, const char *fmt, va_list ap);
-
-/**
  * qtest_qmpv:
  * @s: #QTestState instance to operate on.
  * @fmt: QMP message to send to QEMU; formats arguments through
@@ -568,12 +547,10 @@ void qmp_async(const char *fmt, ...);

 /**
  * qmp_discard_response:
- * @fmt...: QMP message to send to qemu; formats arguments through
- * json-lexer.c (only understands '%(PRI[ud]64|(l|ll)?[du]|[ipsf%])').
  *
- * Sends a QMP message to QEMU and consumes the response.
+ * Read and discard a QMP response, typically after qmp_async().
  */
-void qmp_discard_response(const char *fmt, ...);
+void qmp_discard_response(void);

 /**
  * qmp_receive:
diff --git a/tests/libqtest.c b/tests/libqtest.c
index 3071be2efb..f9781d67f5 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -235,7 +235,8 @@ QTestState *qtest_init(const char *extra_args)
 /* Read the QMP greeting and then do the handshake */
 greeting = qtest_qmp_receive(s);
 QDECREF(greeting);
-qtest_qmp_discard_response(s, "{ 'execute': 'qmp_capabilities' }");
+greeting = qtest_qmp(s, "{ 'execute': 'qmp_capabilities' }");
+QDECREF(greeting);

 return s;
 }
@@ -518,23 +519,6 @@ void qtest_async_qmp(QTestState *s, const char *fmt, ...)
 va_end(ap);
 }

-void qtest_qmpv_discard_response(QTestState *s, const char *fmt, va_list ap)
-{
-QDict *response = qtest_qmpv(s, fmt, ap);
-QDECREF(response);
-}
-
-void qtest_qmp_discard_response(QTestState *s, const char *fmt, ...)
-{
-va_list ap;
-QDict *response;
-
-va_start(ap, fmt);
-response = qtest_qmpv(s, fmt, ap);
-va_end(ap);
-QDECREF(response);
-}
-
 QDict *qtest_qmp_eventwait_ref(QTestState *s, const char *event)
 {
 QDict *response;
@@ -909,14 +893,12 @@ void qmp_async(const char *fmt, ...)
 va_end(ap);
 }

-void qmp_discard_response(const char *fmt, ...)
+void qmp_discard_response(void)
 {
-va_list ap;
-
-va_start(ap, fmt);
-qtest_qmpv_discard_response(global_qtest, fmt, ap);
-va_end(ap);
+QDict *response = qtest_qmp_receive(global_qtest);
+QDECREF(response);
 }
+
 char *hmp(const char *fmt, ...)
 {
 va_list ap;
diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index 999121bb7c..9460843a9f 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -1596,8 +1596,9 @@ static void test_atapi_tray(void)
 rsp = qmp_receive();
 QDECREF(rsp);

-qmp_discard_response("{'execute': 'x-blockdev-remove-medium', "
- "'arguments': {'device': 

Re: [Qemu-block] [PATCH V5 10/10] block/qcow2: add compress info to image specific info

[Peter Lieven]

Am 25.07.2017 um 23:55 schrieb Eric Blake:
> On 07/25/2017 09:41 AM, Peter Lieven wrote:
>> Signed-off-by: Peter Lieven 
>> ---
>>  block/qcow2.c| 7 +++
>>  qapi/block-core.json | 6 +-
>>  2 files changed, 12 insertions(+), 1 deletion(-)
>>
>> +++ b/qapi/block-core.json
>> @@ -68,6 +68,9 @@
>>  # @encrypt: details about encryption parameters; only set if image
>>  #   is encrypted (since 2.10)
>>  #
>> +# @compress: details about parameters for compressed clusters; only set if
>> +#the compress format header extension is present (since 2.11)
> Thinking aloud:
>
> I still think this is better as "only set if the image uses compressed
> headers" (similar to encrypt).  That is, I would like this output to be
> present even when opening legacy deflate/0/12 images.
>
> But thinking more, what I am REALLY asking for is "if any cluster is
> compressed, then this information is present; if nothing is compressed,
> the choice of compression header doesn't matter".  But we don't have a
> quick-and-dirty way to tell if an image has any compressed clusters,
> short of examining every L2 map (including maps inside internal snapshots).
>
> Hmm - we already have 'Dirty bit' and 'Corrupt bit' as
> incompatible_features that can quickly identify certain image
> properties; do we want another bit set to 1 for images known to use
> compressed clusters?  Or even make it an auto-clear bit (or even a pair
> of auto-clear bits: one to designate that this image was written by a
> new-enough qemu that promises to mark the image if any compressed
> clusters exist, and the other is set only if such clusters do exist;
> that way, if both bits are clear, we know we have to walk L2 tables to
> look for compressed clusters, but if the first bit is set, then the
> second bit is reliable)?
>
> So maybe this means we are still debating spec ideas.  Kevin, do you
> want to chime in?
>
Hi all,


I would really like to move this forward and get at least the spec for

the QCOW2 Extension Header fixed. I internally need this feature and would

like not to produce incompatible images.

Maybe we can even decide later if we add further bits for tracking if

there are compressed cluster and keep this indepented of this series.


Thanks,
Peter

[Qemu-block] [PATCH v3 3/4] qga: Give more --version information

[Eric Blake]

Include the package version information (useful for detecting
builds from git or downstream backports), and the copyright notice.

Signed-off-by: Eric Blake 
Reviewed-by: Daniel P. Berrange 
---
 qga/main.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/qga/main.c b/qga/main.c
index 1b381d0bf3..b64c7ac2a2 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -29,6 +29,7 @@
 #include "qemu/help_option.h"
 #include "qemu/sockets.h"
 #include "qemu/systemd.h"
+#include "qemu-version.h"
 #ifdef _WIN32
 #include "qga/service-win32.h"
 #include "qga/vss-win32.h"
@@ -213,7 +214,8 @@ static void usage(const char *cmd)
 {
 printf(
 "Usage: %s [-m  -p ] []\n"
-"QEMU Guest Agent %s\n"
+"QEMU Guest Agent " QEMU_VERSION QEMU_PKGVERSION "\n"
+QEMU_COPYRIGHT "\n"
 "\n"
 "  -m, --method  transport method: one of unix-listen, virtio-serial,\n"
 "isa-serial, or vsock-listen (virtio-serial is the 
default)\n"
@@ -248,7 +250,7 @@ static void usage(const char *cmd)
 "  -h, --helpdisplay this help and exit\n"
 "\n"
 "Report bugs to \n"
-, cmd, QEMU_VERSION, QGA_VIRTIO_PATH_DEFAULT, QGA_SERIAL_PATH_DEFAULT,
+, cmd, QGA_VIRTIO_PATH_DEFAULT, QGA_SERIAL_PATH_DEFAULT,
 dfl_pathnames.pidfile,
 #ifdef CONFIG_FSFREEZE
 QGA_FSFREEZE_HOOK_DEFAULT,
-- 
2.13.3

[Qemu-block] [PATCH v3 2/4] qemu-io: Give more --version information

[Eric Blake]

Include the package version information (useful for detecting
builds from git or downstream backports), and the copyright notice.

Signed-off-by: Eric Blake 
Reviewed-by: Daniel P. Berrange 
Acked-by: Kevin Wolf 
---
 qemu-io.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/qemu-io.c b/qemu-io.c
index 4cfa41c8f9..ec175630a6 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -26,6 +26,7 @@
 #include "block/block_int.h"
 #include "trace/control.h"
 #include "crypto/init.h"
+#include "qemu-version.h"

 #define CMD_NOFILE_OK   0x01

@@ -522,7 +523,8 @@ int main(int argc, char **argv)
 trace_file = trace_opt_parse(optarg);
 break;
 case 'V':
-printf("%s version %s\n", progname, QEMU_VERSION);
+printf("%s version " QEMU_VERSION QEMU_PKGVERSION "\n"
+   QEMU_COPYRIGHT "\n", progname);
 exit(0);
 case 'h':
 usage(progname);
-- 
2.13.3

[Qemu-block] [PATCH v3 for-2.10 0/4] improved --version/--help tweaks

[Eric Blake]

Not sure if this should go through Kevin's block tree, Paolo's
miscellaneous patches, or if I should just do a pull request
myself (since patch 4 includes a change to qemu-nbd)

since v2: add R-b on 2-4; also sort 'qemu-img --help' create text

001/4:[0012] [FC] 'qemu-img: Sort sub-command names in --help'
002/4:[] [--] 'qemu-io: Give more --version information'
003/4:[] [--] 'qga: Give more --version information'
004/4:[] [--] 'maint: Include bug-reporting info in --help output'

Eric Blake (4):
  qemu-img: Sort sub-command names in --help
  qemu-io: Give more --version information
  qga: Give more --version information
  maint: Include bug-reporting info in --help output

 include/qemu-common.h |  5 +
 vl.c  |  4 +++-
 bsd-user/main.c   |  2 ++
 linux-user/main.c |  4 +++-
 qemu-img.c|  2 +-
 qemu-io.c |  9 ++---
 qemu-nbd.c|  2 +-
 qga/main.c|  8 +---
 qemu-img-cmds.hx  | 21 -
 9 files changed, 38 insertions(+), 19 deletions(-)

-- 
2.13.3

[Qemu-block] [PATCH v3 4/4] maint: Include bug-reporting info in --help output

[Eric Blake]

These days, many programs are including a bug-reporting address,
or better yet, a link to the project web site, at the tail of
their --help output.  However, we were not very consistent at
doing so: only qemu-nbd and qemu-qa mentioned anything, with the
latter pointing to an individual person instead of the project.

Add a new #define that sets up a uniform string, mentioning both
bug reporting instructions and overall project details, and which
a downstream vendor could tweak if they want bugs to go to a
downstream database.  Then use it in all of our binaries which
have --help output.

The canned text intentionally references http:// instead of https://
because our https website currently causes certificate errors in
some browsers.  That can be tweaked later once we have resolved the
web site issued.

Signed-off-by: Eric Blake 
Reviewed-by: Daniel P. Berrange 
Reviewed-by: Philippe Mathieu-Daudé 

---
v3: tweak subject line
---
 include/qemu-common.h | 5 +
 vl.c  | 4 +++-
 bsd-user/main.c   | 2 ++
 linux-user/main.c | 4 +++-
 qemu-img.c| 2 +-
 qemu-io.c | 5 +++--
 qemu-nbd.c| 2 +-
 qga/main.c| 2 +-
 8 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/include/qemu-common.h b/include/qemu-common.h
index b5adbfa5e9..d29045631f 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -22,6 +22,11 @@
 #define QEMU_COPYRIGHT "Copyright (c) 2003-2017 " \
 "Fabrice Bellard and the QEMU Project developers"

+/* Bug reporting information for --help arguments, About dialogs, etc */
+#define QEMU_BUGREPORTS \
+"See  for bug reports.\n" \
+"More information on the QEMU project at ."
+
 /* main function, renamed */
 #if defined(CONFIG_COCOA)
 int qemu_main(int argc, char **argv, char **envp);
diff --git a/vl.c b/vl.c
index 99fcfa0442..5ad2506ded 100644
--- a/vl.c
+++ b/vl.c
@@ -1942,7 +1942,9 @@ static void help(int exitcode)
"ctrl-alt-n  switch to virtual console 'n'\n"
"ctrl-alttoggle mouse and keyboard grab\n"
"\n"
-   "When using -nographic, press 'ctrl-a h' to get some help.\n");
+   "When using -nographic, press 'ctrl-a h' to get some help.\n"
+   "\n"
+   QEMU_BUGREPORTS "\n");

 exit(exitcode);
 }
diff --git a/bsd-user/main.c b/bsd-user/main.c
index 501e16f675..4db10cb376 100644
--- a/bsd-user/main.c
+++ b/bsd-user/main.c
@@ -686,6 +686,8 @@ static void usage(void)
"-E var1=val2 -E var2=val2 -U LD_PRELOAD -U LD_DEBUG\n"
"Note that if you provide several changes to single variable\n"
"last change will stay in effect.\n"
+   "\n"
+   QEMU_BUGREPORTS "\n"
,
TARGET_NAME,
interp_prefix,
diff --git a/linux-user/main.c b/linux-user/main.c
index 2b38d39d87..7d6e481277 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -4136,7 +4136,9 @@ static void usage(int exitcode)
"-E var1=val2,var2=val2 -U LD_PRELOAD,LD_DEBUG\n"
"QEMU_SET_ENV=var1=val2,var2=val2 
QEMU_UNSET_ENV=LD_PRELOAD,LD_DEBUG\n"
"Note that if you provide several changes to a single variable\n"
-   "the last change will stay in effect.\n");
+   "the last change will stay in effect.\n"
+   "\n"
+   QEMU_BUGREPORTS "\n");

 exit(exitcode);
 }
diff --git a/qemu-img.c b/qemu-img.c
index f4d5f0d77d..758719e083 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -201,7 +201,7 @@ static void QEMU_NORETURN help(void)

 printf("%s\nSupported formats:", help_msg);
 bdrv_iterate_format(format_print, NULL);
-printf("\n");
+printf("\n\n" QEMU_BUGREPORTS "\n");
 exit(EXIT_SUCCESS);
 }

diff --git a/qemu-io.c b/qemu-io.c
index ec175630a6..b93553a603 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -262,8 +262,9 @@ static void usage(const char *name)
 "  -h, --help   display this help and exit\n"
 "  -V, --versionoutput version information and exit\n"
 "\n"
-"See '%s -c help' for information on available commands."
-"\n",
+"See '%s -c help' for information on available commands.\n"
+"\n"
+QEMU_BUGREPORTS "\n",
 name, name);
 }

diff --git a/qemu-nbd.c b/qemu-nbd.c
index b8666bb575..052eb4d067 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -123,7 +123,7 @@ static void usage(const char *name)
 "  --detect-zeroes=MODE  set detect-zeroes mode (off, on, unmap)\n"
 "  --image-opts  treat FILE as a full set of image options\n"
 "\n"
-"Report bugs to \n"
+QEMU_BUGREPORTS "\n"
 , name, NBD_DEFAULT_PORT, "DEVICE");
 }

diff --git a/qga/main.c b/qga/main.c
index b64c7ac2a2..56d5633c13 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -249,7 +249,7 @@ QEMU_COPYRIGHT "\n"
 "options / command-line parameters to stdout\n"
 "  -h, 

[Qemu-block] [PATCH v3 1/4] qemu-img: Sort sub-command names in --help

[Eric Blake]

'amend' and 'create' were not listed alphabetically; hoist them
earlier.  Separate the @end table block to make it easier to
copy-and-paste the addition of future sub-commands.

Signed-off-by: Eric Blake 

---
v3: also sort 'create' [Kevin]
---
 qemu-img-cmds.hx | 21 -
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 3763f13625..e6ebdbfee4 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -9,6 +9,12 @@ STEXI
 @table @option
 ETEXI

+DEF("amend", img_amend,
+"amend [--object objectdef] [--image-opts] [-p] [-q] [-f fmt] [-t cache] 
-o options filename")
+STEXI
+@item amend [--object @var{objectdef}] [--image-opts] [-p] [-q] [-f @var{fmt}] 
[-t @var{cache}] -o @var{options} @var{filename}
+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] [-U] filename")
 STEXI
@@ -21,12 +27,6 @@ STEXI
 @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,
-"create [-q] [--object objectdef] [-f fmt] [-b backing_file] [-F 
backing_fmt] [-u] [-o options] filename [size]")
-STEXI
-@item create [--object @var{objectdef}] [-q] [-f @var{fmt}] [-b 
@var{backing_file}] [-F @var{backing_fmt}] [-u] [-o @var{options}] 
@var{filename} [@var{size}]
-ETEXI
-
 DEF("commit", img_commit,
 "commit [-q] [--object objectdef] [--image-opts] [-f fmt] [-t cache] [-b 
base] [-d] [-p] filename")
 STEXI
@@ -45,6 +45,12 @@ STEXI
 @item convert [--object @var{objectdef}] [--image-opts] [--target-image-opts] 
[-U] [-c] [-p] [-q] [-n] [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] 
[-O @var{output_fmt}] [-B @var{backing_file}] [-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}
 ETEXI

+DEF("create", img_create,
+"create [-q] [--object objectdef] [-f fmt] [-b backing_file] [-F 
backing_fmt] [-u] [-o options] filename [size]")
+STEXI
+@item create [--object @var{objectdef}] [-q] [-f @var{fmt}] [-b 
@var{backing_file}] [-F @var{backing_fmt}] [-u] [-o @var{options}] 
@var{filename} [@var{size}]
+ETEXI
+
 DEF("dd", img_dd,
 "dd [--image-opts] [-U] [-f fmt] [-O output_fmt] [bs=block_size] 
[count=blocks] [skip=blocks] if=input of=output")
 STEXI
@@ -87,9 +93,6 @@ STEXI
 @item resize [--object @var{objectdef}] [--image-opts] [-q] @var{filename} [+ 
| -]@var{size}
 ETEXI

-DEF("amend", img_amend,
-"amend [--object objectdef] [--image-opts] [-p] [-q] [-f fmt] [-t cache] 
-o options filename")
 STEXI
-@item amend [--object @var{objectdef}] [--image-opts] [-p] [-q] [-f @var{fmt}] 
[-t @var{cache}] -o @var{options} @var{filename}
 @end table
 ETEXI
-- 
2.13.3

Re: [Qemu-block] [PATCH for-2.10 5/5] qemu-iotests: Test reopen between read-only and read-write

[Jeff Cody]

On Thu, Aug 03, 2017 at 05:03:01PM +0200, Kevin Wolf wrote:
> This serves as a regression test for the bugs that were just fixed for
> bdrv_reopen() between read-only and read-write mode.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  tests/qemu-iotests/187 | 69 
> ++
>  tests/qemu-iotests/187.out | 18 
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 88 insertions(+)
>  create mode 100755 tests/qemu-iotests/187
>  create mode 100644 tests/qemu-iotests/187.out
> 
> diff --git a/tests/qemu-iotests/187 b/tests/qemu-iotests/187
> new file mode 100755
> index 00..7bb783363c
> --- /dev/null
> +++ b/tests/qemu-iotests/187
> @@ -0,0 +1,69 @@
> +#!/bin/bash
> +#
> +# Test switching between read-only and read-write
> +#
> +# 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=kw...@redhat.com
> +
> +seq=`basename $0`
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +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
> +

As Eric noted, I'll need to make sure to add this to my series to strip out
the cleanup.



Reviewed-by: Jeff Cody 


> +# get standard environment, filters and checks
> +. ./common.rc
> +. ./common.filter
> +
> +_supported_fmt qcow2
> +_supported_proto file
> +_supported_os Linux
> +
> +size=64M
> +_make_test_img $size
> +
> +echo
> +echo "Start from read-only"
> +echo
> +
> +$QEMU_IO -r -c 'write 0 64k' $TEST_IMG | _filter_qemu_io
> +$QEMU_IO -r -c 'reopen -w' -c 'write 0 64k' $TEST_IMG | _filter_qemu_io
> +$QEMU_IO -r -c 'reopen -w' -c 'reopen -r' -c 'write 0 64k' $TEST_IMG | 
> _filter_qemu_io
> +
> +echo
> +echo "Start from read-write"
> +echo
> +
> +$QEMU_IO -c 'write 0 64k' $TEST_IMG | _filter_qemu_io
> +$QEMU_IO -c 'reopen -r' -c 'write 0 64k' $TEST_IMG | _filter_qemu_io
> +$QEMU_IO -c 'reopen -r' -c 'reopen -w' -c 'write 0 64k' $TEST_IMG | 
> _filter_qemu_io
> +
> +
> +# success, all done
> +echo "*** done"
> +rm -f $seq.full
> +status=0
> diff --git a/tests/qemu-iotests/187.out b/tests/qemu-iotests/187.out
> new file mode 100644
> index 00..68fb944cd5
> --- /dev/null
> +++ b/tests/qemu-iotests/187.out
> @@ -0,0 +1,18 @@
> +QA output created by 187
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
> +
> +Start from read-only
> +
> +Block node is read-only
> +wrote 65536/65536 bytes at offset 0
> +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +Block node is read-only
> +
> +Start from read-write
> +
> +wrote 65536/65536 bytes at offset 0
> +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +write failed: Operation not permitted
> +wrote 65536/65536 bytes at offset 0
> +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +*** done
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index 823811076d..1848077932 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -182,6 +182,7 @@
>  183 rw auto migration
>  185 rw auto
>  186 rw auto
> +187 rw auto
>  188 rw auto quick
>  189 rw auto
>  190 rw auto quick
> -- 
> 2.13.3
> 
> 

Re: [Qemu-block] [PATCH for-2.10 4/5] qemu-io: Allow reopen read-write

[Jeff Cody]

On Thu, Aug 03, 2017 at 05:03:00PM +0200, Kevin Wolf wrote:
> This allows qemu-iotests to test the switch between read-only and
> read-write mode for block devices.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  qemu-io-cmds.c | 19 +--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
> index 3eb42c6728..2811a89099 100644
> --- a/qemu-io-cmds.c
> +++ b/qemu-io-cmds.c
> @@ -1920,6 +1920,7 @@ static void reopen_help(void)
>  " 'reopen -o lazy-refcounts=on' - activates lazy refcount writeback on a 
> qcow2 image\n"
>  "\n"
>  " -r, -- Reopen the image read-only\n"
> +" -w, -- Reopen the image read-write\n"
>  " -c, -- Change the cache mode to the given value\n"
>  " -o, -- Changes block driver options (cf. 'open' command)\n"
>  "\n");
> @@ -1942,7 +1943,7 @@ static const cmdinfo_t reopen_cmd = {
> .argmin = 0,
> .argmax = -1,
> .cfunc  = reopen_f,
> -   .args   = "[-r] [-c cache] [-o options]",
> +   .args   = "[(-r|-w)] [-c cache] [-o options]",
> .oneline= "reopens an image with new options",
> .help   = reopen_help,
>  };
> @@ -1955,11 +1956,12 @@ static int reopen_f(BlockBackend *blk, int argc, char 
> **argv)
>  int c;
>  int flags = bs->open_flags;
>  bool writethrough = !blk_enable_write_cache(blk);
> +bool has_rw_option = false;
>  
>  BlockReopenQueue *brq;
>  Error *local_err = NULL;
>  
> -while ((c = getopt(argc, argv, "c:o:r")) != -1) {
> +while ((c = getopt(argc, argv, "c:o:rw")) != -1) {
>  switch (c) {
>  case 'c':
>  if (bdrv_parse_cache_mode(optarg, , ) < 0) {
> @@ -1974,7 +1976,20 @@ static int reopen_f(BlockBackend *blk, int argc, char 
> **argv)
>  }
>  break;
>  case 'r':
> +if (has_rw_option) {
> +error_report("Only one -r/-w option may be given");
> +return 0;
> +}
>  flags &= ~BDRV_O_RDWR;
> +has_rw_option = true;
> +break;
> +case 'w':
> +if (has_rw_option) {
> +error_report("Only one -r/-w option may be given");
> +return 0;
> +}
> +flags |= BDRV_O_RDWR;
> +has_rw_option = true;
>  break;
>  default:
>  qemu_opts_reset(_opts);
> -- 
> 2.13.3
> 
> 

Reviewed-by: Jeff Cody 

Re: [Qemu-block] [PATCH for-2.10 3/5] block: Set BDRV_O_ALLOW_RDWR during rw reopen

[Jeff Cody]

On Thu, Aug 03, 2017 at 05:02:59PM +0200, Kevin Wolf wrote:
> Reopening an image should be consistent with opening it, so we should
> set BDRV_O_ALLOW_RDWR for any image that is reopened read-write like in
> bdrv_open_inherit().
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/block.c b/block.c
> index 2711c3dd3b..3615a6809e 100644
> --- a/block.c
> +++ b/block.c
> @@ -2729,8 +2729,11 @@ static BlockReopenQueue 
> *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
>  bdrv_join_options(bs, options, old_options);
>  QDECREF(old_options);
>  
> -/* bdrv_open() masks this flag out */
> +/* bdrv_open_inherit() sets and clears some additional flags internally 
> */
>  flags &= ~BDRV_O_PROTOCOL;
> +if (flags & BDRV_O_RDWR) {
> +flags |= BDRV_O_ALLOW_RDWR;
> +}
>  
>  QLIST_FOREACH(child, >children, next) {
>  QDict *new_child_options;
> -- 
> 2.13.3
> 
> 

Reviewed-by: Jeff Cody 

Re: [Qemu-block] [PATCH for-2.10 2/5] block: Allow reopen rw without BDRV_O_ALLOW_RDWR

[Jeff Cody]

On Thu, Aug 03, 2017 at 05:02:58PM +0200, Kevin Wolf wrote:
> BDRV_O_ALLOW_RDWR is a flag that tells whether qemu can internally
> reopen a node read-write temporarily because the user requested
> read-write for the top-level image, but qemu decided that read-only is
> enough for this node (a backing file).
> 
> bdrv_reopen() is different, it is also used for cases where the user
> changed their mind and wants to update the options. There is no reason
> to forbid making a node read-write in that case.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  include/block/block.h |  3 ++-
>  block.c   | 11 +++
>  2 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/include/block/block.h b/include/block/block.h
> index 34770bb33a..ab80195378 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -436,7 +436,8 @@ int bdrv_is_allocated_above(BlockDriverState *top, 
> BlockDriverState *base,
>  
>  bool bdrv_is_read_only(BlockDriverState *bs);
>  bool bdrv_is_writable(BlockDriverState *bs);
> -int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only, Error 
> **errp);
> +int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only,
> +   bool ignore_allow_rdw, Error **errp);
>  int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
>  bool bdrv_is_sg(BlockDriverState *bs);
>  bool bdrv_is_inserted(BlockDriverState *bs);
> diff --git a/block.c b/block.c
> index ab908cdc50..2711c3dd3b 100644
> --- a/block.c
> +++ b/block.c
> @@ -246,7 +246,8 @@ bool bdrv_is_writable(BlockDriverState *bs)
>  return !bdrv_is_read_only(bs) && !(bs->open_flags & BDRV_O_INACTIVE);
>  }
>  
> -int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only, Error 
> **errp)
> +int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only,
> +   bool ignore_allow_rdw, Error **errp)

I think 'override_allow_rdw' may be a bit clearer, but that is just
bikeshedding.

Reviewed-by: Jeff Cody 


>  {
>  /* Do not set read_only if copy_on_read is enabled */
>  if (bs->copy_on_read && read_only) {
> @@ -256,7 +257,9 @@ int bdrv_can_set_read_only(BlockDriverState *bs, bool 
> read_only, Error **errp)
>  }
>  
>  /* Do not clear read_only if it is prohibited */
> -if (!read_only && !(bs->open_flags & BDRV_O_ALLOW_RDWR)) {
> +if (!read_only && !(bs->open_flags & BDRV_O_ALLOW_RDWR) &&
> +!ignore_allow_rdw)
> +{
>  error_setg(errp, "Node '%s' is read only",
> bdrv_get_device_or_node_name(bs));
>  return -EPERM;
> @@ -269,7 +272,7 @@ int bdrv_set_read_only(BlockDriverState *bs, bool 
> read_only, Error **errp)
>  {
>  int ret = 0;
>  
> -ret = bdrv_can_set_read_only(bs, read_only, errp);
> +ret = bdrv_can_set_read_only(bs, read_only, false, errp);
>  if (ret < 0) {
>  return ret;
>  }
> @@ -2907,7 +2910,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, 
> BlockReopenQueue *queue,
>   * to r/w. Attempting to set to r/w may fail if either BDRV_O_ALLOW_RDWR 
> is
>   * not set, or if the BDS still has copy_on_read enabled */
>  read_only = !(reopen_state->flags & BDRV_O_RDWR);
> -ret = bdrv_can_set_read_only(reopen_state->bs, read_only, _err);
> +ret = bdrv_can_set_read_only(reopen_state->bs, read_only, true, 
> _err);
>  if (local_err) {
>  error_propagate(errp, local_err);
>  goto error;
> -- 
> 2.13.3
> 
> 

Re: [Qemu-block] [Qemu-devel] [PATCH v2 4/4] maint: Include bug-reporting info in --help output.

[Philippe Mathieu-Daudé]

On 07/28/2017 01:47 PM, Eric Blake wrote:

These days, many programs are including a bug-reporting address,
or better yet, a link to the project web site, at the tail of
their --help output.  However, we were not very consistent at
doing so: only qemu-nbd and qemu-qa mentioned anything, with the
latter pointing to an individual person instead of the project.

Add a new #define that sets up a uniform string, mentioning both
bug reporting instructions and overall project details, and which
a downstream vendor could tweak if they want bugs to go to a
downstream database.  Then use it in all of our binaries which
have --help output.

The canned text intentionally references http:// instead of https://
because our https website currently causes certificate errors in
some browsers.  That can be tweaked later once we have resolved the
web site issued.

Signed-off-by: Eric Blake 


Reviewed-by: Philippe Mathieu-Daudé 


---

v2: tweak text to capitalize QEMU and use consistent trailing .

  include/qemu-common.h | 5 +
  vl.c  | 4 +++-
  bsd-user/main.c   | 2 ++
  linux-user/main.c | 4 +++-
  qemu-img.c| 2 +-
  qemu-io.c | 5 +++--
  qemu-nbd.c| 2 +-
  qga/main.c| 2 +-
  8 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/include/qemu-common.h b/include/qemu-common.h
index b5adbfa5e9..d29045631f 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -22,6 +22,11 @@
  #define QEMU_COPYRIGHT "Copyright (c) 2003-2017 " \
  "Fabrice Bellard and the QEMU Project developers"

+/* Bug reporting information for --help arguments, About dialogs, etc */
+#define QEMU_BUGREPORTS \
+"See  for bug reports.\n" \
+"More information on the QEMU project at ."
+
  /* main function, renamed */
  #if defined(CONFIG_COCOA)
  int qemu_main(int argc, char **argv, char **envp);
diff --git a/vl.c b/vl.c
index fb6b2efafa..b824f81f64 100644
--- a/vl.c
+++ b/vl.c
@@ -1942,7 +1942,9 @@ static void help(int exitcode)
 "ctrl-alt-n  switch to virtual console 'n'\n"
 "ctrl-alttoggle mouse and keyboard grab\n"
 "\n"
-   "When using -nographic, press 'ctrl-a h' to get some help.\n");
+   "When using -nographic, press 'ctrl-a h' to get some help.\n"
+   "\n"
+   QEMU_BUGREPORTS "\n");

  exit(exitcode);
  }
diff --git a/bsd-user/main.c b/bsd-user/main.c
index 501e16f675..4db10cb376 100644
--- a/bsd-user/main.c
+++ b/bsd-user/main.c
@@ -686,6 +686,8 @@ static void usage(void)
 "-E var1=val2 -E var2=val2 -U LD_PRELOAD -U LD_DEBUG\n"
 "Note that if you provide several changes to single variable\n"
 "last change will stay in effect.\n"
+   "\n"
+   QEMU_BUGREPORTS "\n"
 ,
 TARGET_NAME,
 interp_prefix,
diff --git a/linux-user/main.c b/linux-user/main.c
index 2b38d39d87..7d6e481277 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -4136,7 +4136,9 @@ static void usage(int exitcode)
 "-E var1=val2,var2=val2 -U LD_PRELOAD,LD_DEBUG\n"
 "QEMU_SET_ENV=var1=val2,var2=val2 
QEMU_UNSET_ENV=LD_PRELOAD,LD_DEBUG\n"
 "Note that if you provide several changes to a single variable\n"
-   "the last change will stay in effect.\n");
+   "the last change will stay in effect.\n"
+   "\n"
+   QEMU_BUGREPORTS "\n");

  exit(exitcode);
  }
diff --git a/qemu-img.c b/qemu-img.c
index f4d5f0d77d..758719e083 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -201,7 +201,7 @@ static void QEMU_NORETURN help(void)

  printf("%s\nSupported formats:", help_msg);
  bdrv_iterate_format(format_print, NULL);
-printf("\n");
+printf("\n\n" QEMU_BUGREPORTS "\n");
  exit(EXIT_SUCCESS);
  }

diff --git a/qemu-io.c b/qemu-io.c
index ec175630a6..b93553a603 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -262,8 +262,9 @@ static void usage(const char *name)
  "  -h, --help   display this help and exit\n"
  "  -V, --versionoutput version information and exit\n"
  "\n"
-"See '%s -c help' for information on available commands."
-"\n",
+"See '%s -c help' for information on available commands.\n"
+"\n"
+QEMU_BUGREPORTS "\n",
  name, name);
  }

diff --git a/qemu-nbd.c b/qemu-nbd.c
index b8666bb575..052eb4d067 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -123,7 +123,7 @@ static void usage(const char *name)
  "  --detect-zeroes=MODE  set detect-zeroes mode (off, on, unmap)\n"
  "  --image-opts  treat FILE as a full set of image options\n"
  "\n"
-"Report bugs to \n"
+QEMU_BUGREPORTS "\n"
  , name, NBD_DEFAULT_PORT, "DEVICE");
  }

diff --git a/qga/main.c b/qga/main.c
index b64c7ac2a2..56d5633c13 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -249,7 +249,7 @@ QEMU_COPYRIGHT "\n"
  "  

Re: [Qemu-block] [PATCH for-2.10 1/5] block: Fix order in bdrv_replace_child()

[Jeff Cody]

On Thu, Aug 03, 2017 at 05:02:57PM +0200, Kevin Wolf wrote:
> Commit 8ee03995 refactored the code incorrectly and broke the release of
> permissions on the old BDS. Instead of changing the permissions to the
> new required values after removing the old BDS from the list of
> children, it only re-obtains the permissions it already had.
> 
> Change the order of operations so that the old BDS is removed again
> before calculating the new required permissions.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/block.c b/block.c
> index ce9cce7b3c..ab908cdc50 100644
> --- a/block.c
> +++ b/block.c
> @@ -1933,6 +1933,8 @@ static void bdrv_replace_child(BdrvChild *child, 
> BlockDriverState *new_bs)
>  BlockDriverState *old_bs = child->bs;
>  uint64_t perm, shared_perm;
>  
> +bdrv_replace_child_noperm(child, new_bs);
> +
>  if (old_bs) {
>  /* Update permissions for old node. This is guaranteed to succeed
>   * because we're just taking a parent away, so we're loosening
> @@ -1942,8 +1944,6 @@ static void bdrv_replace_child(BdrvChild *child, 
> BlockDriverState *new_bs)
>  bdrv_set_perm(old_bs, perm, shared_perm);
>  }
>  
> -bdrv_replace_child_noperm(child, new_bs);
> -
>  if (new_bs) {
>  bdrv_get_cumulative_perm(new_bs, , _perm);
>  bdrv_set_perm(new_bs, perm, shared_perm);
> -- 
> 2.13.3
> 
> 

Reviewed-by: Jeff Cody 

Re: [Qemu-block] [Qemu-devel] [PATCH for-2.10 5/5] qemu-iotests: Test reopen between read-only and read-write

[Eric Blake]

On 08/03/2017 10:03 AM, Kevin Wolf wrote:
> This serves as a regression test for the bugs that were just fixed for
> bdrv_reopen() between read-only and read-write mode.

If I'm right that this also fixes the difference between intermediate
vs. live commit to an initial read-write image that can't be reopened,
can we add that to this test?

> 
> Signed-off-by: Kevin Wolf 
> ---
>  tests/qemu-iotests/187 | 69 
> ++
>  tests/qemu-iotests/187.out | 18 
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 88 insertions(+)
>  create mode 100755 tests/qemu-iotests/187
>  create mode 100644 tests/qemu-iotests/187.out
> 

> +
> +_cleanup()
> +{
> + _cleanup_test_img
> +rm -f "$TEST_IMG.2"
> +rm -f "$TEST_IMG.3"
> +}
> +trap "_cleanup; exit \$status" 0 1 2 3 15

There are pending patches for 2.11 that will need to tweak this. But for
2.10, this is fine.


> +++ b/tests/qemu-iotests/187.out
> @@ -0,0 +1,18 @@
> +QA output created by 187
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
> +
> +Start from read-only
> +
> +Block node is read-only
> +wrote 65536/65536 bytes at offset 0
> +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +Block node is read-only
> +

Why the difference in error messages, when starting read-only,

> +Start from read-write
> +
> +wrote 65536/65536 bytes at offset 0
> +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +write failed: Operation not permitted

vs. when reopened read-only?

I don't see it as a flaw in the test, so much as another odd difference
in code paths that we may later want to improve.

So, as written,
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] [Qemu-devel] [PATCH for-2.10 4/5] qemu-io: Allow reopen read-write

[Eric Blake]

On 08/03/2017 10:03 AM, Kevin Wolf wrote:
> This allows qemu-iotests to test the switch between read-only and
> read-write mode for block devices.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  qemu-io-cmds.c | 19 +--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 

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] [PATCH v2 1/4] qemu-img: Sort sub-command names in --help

[Eric Blake]

On 08/03/2017 10:14 AM, Kevin Wolf wrote:
> Am 28.07.2017 um 18:47 hat Eric Blake geschrieben:
>> 'amend' was the only sub-command not listed alphabetically;
> 
> Not completel true: create is the second one that is in the wrong place,
> it should come after commit/compare/convert. Do you want to fix that
> one, too?

Indeed.  I wonder if part of the issue is due to an 80-columen window
displaying qemu-img --help as a wall-o-text and therefore I missed
command names; maybe some well-place newlines and tabs would aid
legibility, as in:

  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
  check [-q] [--object objectdef] [--image-opts] [-f fmt]
[--output=ofmt] [-r [leaks | all]] [-T src_cache] [-U] filename
...

But for this patch, I'll just fix the sorting of 'create'.

> 
>> hoist it earlier, and separate the @end table block to make it easier
>> to copy-and-paste the addition of future sub-commands.
>>
>> Signed-off-by: Eric Blake 
> 
> Kevin
> 

-- 
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 for-2.10 3/5] block: Set BDRV_O_ALLOW_RDWR during rw reopen

[Eric Blake]

On 08/03/2017 10:02 AM, Kevin Wolf wrote:
> Reopening an image should be consistent with opening it, so we should
> set BDRV_O_ALLOW_RDWR for any image that is reopened read-write like in
> bdrv_open_inherit().
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 

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] [Qemu-devel] [PATCH for-2.10 2/5] block: Allow reopen rw without BDRV_O_ALLOW_RDWR

[Eric Blake]

On 08/03/2017 10:02 AM, Kevin Wolf wrote:
> BDRV_O_ALLOW_RDWR is a flag that tells whether qemu can internally
> reopen a node read-write temporarily because the user requested
> read-write for the top-level image, but qemu decided that read-only is
> enough for this node (a backing file).
> 
> bdrv_reopen() is different, it is also used for cases where the user
> changed their mind and wants to update the options. There is no reason
> to forbid making a node read-write in that case.

Hmm, I wonder.  https://bugzilla.redhat.com/show_bug.cgi?id=1465320
details a failure when starting qemu with a read-write NBD disk, then
taking several snapshots (nbd <- snap1 <- snap2 <- snap3), then where
intermediate commit (snap2 into nbd) works but live commit (snap3 into
nbd) fails with a message that nbd does not support reopening.  I'm
presuming that your series may help to address that; I'll give it a spin
and see what happens.  But first, the code review:

> 
> Signed-off-by: Kevin Wolf 
> ---
>  include/block/block.h |  3 ++-
>  block.c   | 11 +++
>  2 files changed, 9 insertions(+), 5 deletions(-)
> 

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] [PATCH v2 2/4] qemu-io: Give more --version information

[Kevin Wolf]

Am 28.07.2017 um 18:47 hat Eric Blake geschrieben:
> Include the package version information (useful for detecting
> builds from git or downstream backports), and the copyright notice.
> 
> Signed-off-by: Eric Blake 

Acked-by: Kevin Wolf 

Re: [Qemu-block] [PATCH v2 1/4] qemu-img: Sort sub-command names in --help

[Kevin Wolf]

Am 28.07.2017 um 18:47 hat Eric Blake geschrieben:
> 'amend' was the only sub-command not listed alphabetically;

Not completel true: create is the second one that is in the wrong place,
it should come after commit/compare/convert. Do you want to fix that
one, too?

> hoist it earlier, and separate the @end table block to make it easier
> to copy-and-paste the addition of future sub-commands.
> 
> Signed-off-by: Eric Blake 

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH for-2.10 1/5] block: Fix order in bdrv_replace_child()

[Eric Blake]

On 08/03/2017 10:02 AM, Kevin Wolf wrote:
> Commit 8ee03995 refactored the code incorrectly and broke the release of
> permissions on the old BDS. Instead of changing the permissions to the
> new required values after removing the old BDS from the list of
> children, it only re-obtains the permissions it already had.
> 
> Change the order of operations so that the old BDS is removed again
> before calculating the new required permissions.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 

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

[Qemu-block] [PATCH for-2.10 3/5] block: Set BDRV_O_ALLOW_RDWR during rw reopen

[Kevin Wolf]

Reopening an image should be consistent with opening it, so we should
set BDRV_O_ALLOW_RDWR for any image that is reopened read-write like in
bdrv_open_inherit().

Signed-off-by: Kevin Wolf 
---
 block.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 2711c3dd3b..3615a6809e 100644
--- a/block.c
+++ b/block.c
@@ -2729,8 +2729,11 @@ static BlockReopenQueue 
*bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
 bdrv_join_options(bs, options, old_options);
 QDECREF(old_options);
 
-/* bdrv_open() masks this flag out */
+/* bdrv_open_inherit() sets and clears some additional flags internally */
 flags &= ~BDRV_O_PROTOCOL;
+if (flags & BDRV_O_RDWR) {
+flags |= BDRV_O_ALLOW_RDWR;
+}
 
 QLIST_FOREACH(child, >children, next) {
 QDict *new_child_options;
-- 
2.13.3

[Qemu-block] [PATCH for-2.10 5/5] qemu-iotests: Test reopen between read-only and read-write

[Kevin Wolf]

This serves as a regression test for the bugs that were just fixed for
bdrv_reopen() between read-only and read-write mode.

Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/187 | 69 ++
 tests/qemu-iotests/187.out | 18 
 tests/qemu-iotests/group   |  1 +
 3 files changed, 88 insertions(+)
 create mode 100755 tests/qemu-iotests/187
 create mode 100644 tests/qemu-iotests/187.out

diff --git a/tests/qemu-iotests/187 b/tests/qemu-iotests/187
new file mode 100755
index 00..7bb783363c
--- /dev/null
+++ b/tests/qemu-iotests/187
@@ -0,0 +1,69 @@
+#!/bin/bash
+#
+# Test switching between read-only and read-write
+#
+# 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=kw...@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+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
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+size=64M
+_make_test_img $size
+
+echo
+echo "Start from read-only"
+echo
+
+$QEMU_IO -r -c 'write 0 64k' $TEST_IMG | _filter_qemu_io
+$QEMU_IO -r -c 'reopen -w' -c 'write 0 64k' $TEST_IMG | _filter_qemu_io
+$QEMU_IO -r -c 'reopen -w' -c 'reopen -r' -c 'write 0 64k' $TEST_IMG | 
_filter_qemu_io
+
+echo
+echo "Start from read-write"
+echo
+
+$QEMU_IO -c 'write 0 64k' $TEST_IMG | _filter_qemu_io
+$QEMU_IO -c 'reopen -r' -c 'write 0 64k' $TEST_IMG | _filter_qemu_io
+$QEMU_IO -c 'reopen -r' -c 'reopen -w' -c 'write 0 64k' $TEST_IMG | 
_filter_qemu_io
+
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/187.out b/tests/qemu-iotests/187.out
new file mode 100644
index 00..68fb944cd5
--- /dev/null
+++ b/tests/qemu-iotests/187.out
@@ -0,0 +1,18 @@
+QA output created by 187
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+
+Start from read-only
+
+Block node is read-only
+wrote 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Block node is read-only
+
+Start from read-write
+
+wrote 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+write failed: Operation not permitted
+wrote 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 823811076d..1848077932 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -182,6 +182,7 @@
 183 rw auto migration
 185 rw auto
 186 rw auto
+187 rw auto
 188 rw auto quick
 189 rw auto
 190 rw auto quick
-- 
2.13.3

[Qemu-block] [PATCH for-2.10 2/5] block: Allow reopen rw without BDRV_O_ALLOW_RDWR

[Kevin Wolf]

BDRV_O_ALLOW_RDWR is a flag that tells whether qemu can internally
reopen a node read-write temporarily because the user requested
read-write for the top-level image, but qemu decided that read-only is
enough for this node (a backing file).

bdrv_reopen() is different, it is also used for cases where the user
changed their mind and wants to update the options. There is no reason
to forbid making a node read-write in that case.

Signed-off-by: Kevin Wolf 
---
 include/block/block.h |  3 ++-
 block.c   | 11 +++
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index 34770bb33a..ab80195378 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -436,7 +436,8 @@ int bdrv_is_allocated_above(BlockDriverState *top, 
BlockDriverState *base,
 
 bool bdrv_is_read_only(BlockDriverState *bs);
 bool bdrv_is_writable(BlockDriverState *bs);
-int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
+int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only,
+   bool ignore_allow_rdw, Error **errp);
 int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
 bool bdrv_is_sg(BlockDriverState *bs);
 bool bdrv_is_inserted(BlockDriverState *bs);
diff --git a/block.c b/block.c
index ab908cdc50..2711c3dd3b 100644
--- a/block.c
+++ b/block.c
@@ -246,7 +246,8 @@ bool bdrv_is_writable(BlockDriverState *bs)
 return !bdrv_is_read_only(bs) && !(bs->open_flags & BDRV_O_INACTIVE);
 }
 
-int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
+int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only,
+   bool ignore_allow_rdw, Error **errp)
 {
 /* Do not set read_only if copy_on_read is enabled */
 if (bs->copy_on_read && read_only) {
@@ -256,7 +257,9 @@ int bdrv_can_set_read_only(BlockDriverState *bs, bool 
read_only, Error **errp)
 }
 
 /* Do not clear read_only if it is prohibited */
-if (!read_only && !(bs->open_flags & BDRV_O_ALLOW_RDWR)) {
+if (!read_only && !(bs->open_flags & BDRV_O_ALLOW_RDWR) &&
+!ignore_allow_rdw)
+{
 error_setg(errp, "Node '%s' is read only",
bdrv_get_device_or_node_name(bs));
 return -EPERM;
@@ -269,7 +272,7 @@ int bdrv_set_read_only(BlockDriverState *bs, bool 
read_only, Error **errp)
 {
 int ret = 0;
 
-ret = bdrv_can_set_read_only(bs, read_only, errp);
+ret = bdrv_can_set_read_only(bs, read_only, false, errp);
 if (ret < 0) {
 return ret;
 }
@@ -2907,7 +2910,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, 
BlockReopenQueue *queue,
  * to r/w. Attempting to set to r/w may fail if either BDRV_O_ALLOW_RDWR is
  * not set, or if the BDS still has copy_on_read enabled */
 read_only = !(reopen_state->flags & BDRV_O_RDWR);
-ret = bdrv_can_set_read_only(reopen_state->bs, read_only, _err);
+ret = bdrv_can_set_read_only(reopen_state->bs, read_only, true, 
_err);
 if (local_err) {
 error_propagate(errp, local_err);
 goto error;
-- 
2.13.3

[Qemu-block] [PATCH for-2.10 4/5] qemu-io: Allow reopen read-write

[Kevin Wolf]

This allows qemu-iotests to test the switch between read-only and
read-write mode for block devices.

Signed-off-by: Kevin Wolf 
---
 qemu-io-cmds.c | 19 +--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 3eb42c6728..2811a89099 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -1920,6 +1920,7 @@ static void reopen_help(void)
 " 'reopen -o lazy-refcounts=on' - activates lazy refcount writeback on a qcow2 
image\n"
 "\n"
 " -r, -- Reopen the image read-only\n"
+" -w, -- Reopen the image read-write\n"
 " -c, -- Change the cache mode to the given value\n"
 " -o, -- Changes block driver options (cf. 'open' command)\n"
 "\n");
@@ -1942,7 +1943,7 @@ static const cmdinfo_t reopen_cmd = {
.argmin = 0,
.argmax = -1,
.cfunc  = reopen_f,
-   .args   = "[-r] [-c cache] [-o options]",
+   .args   = "[(-r|-w)] [-c cache] [-o options]",
.oneline= "reopens an image with new options",
.help   = reopen_help,
 };
@@ -1955,11 +1956,12 @@ static int reopen_f(BlockBackend *blk, int argc, char 
**argv)
 int c;
 int flags = bs->open_flags;
 bool writethrough = !blk_enable_write_cache(blk);
+bool has_rw_option = false;
 
 BlockReopenQueue *brq;
 Error *local_err = NULL;
 
-while ((c = getopt(argc, argv, "c:o:r")) != -1) {
+while ((c = getopt(argc, argv, "c:o:rw")) != -1) {
 switch (c) {
 case 'c':
 if (bdrv_parse_cache_mode(optarg, , ) < 0) {
@@ -1974,7 +1976,20 @@ static int reopen_f(BlockBackend *blk, int argc, char 
**argv)
 }
 break;
 case 'r':
+if (has_rw_option) {
+error_report("Only one -r/-w option may be given");
+return 0;
+}
 flags &= ~BDRV_O_RDWR;
+has_rw_option = true;
+break;
+case 'w':
+if (has_rw_option) {
+error_report("Only one -r/-w option may be given");
+return 0;
+}
+flags |= BDRV_O_RDWR;
+has_rw_option = true;
 break;
 default:
 qemu_opts_reset(_opts);
-- 
2.13.3

[Qemu-block] [PATCH for-2.10 0/5] block: bdrv_reopen() fixes

[Kevin Wolf]

This is the first part of some fixes to bdrv_reopen(), which seems
reasonable enough to merge for 2.10.

There is much more wrong with bdrv_reopen() currently, especially with
respect to op blocker permissions (basically the required permissions
can change based on the options used in bdrv_reopen(), and both things
affect the whole tree and aren't integrated with each other at all), but
I have little hope to get this fixed before 2.10, so let's start with
what is ready now.

Kevin Wolf (5):
  block: Fix order in bdrv_replace_child()
  block: Allow reopen rw without BDRV_O_ALLOW_RDWR
  block: Set BDRV_O_ALLOW_RDWR during rw reopen
  qemu-io: Allow reopen read-write
  qemu-iotests: Test reopen between read-only and read-write

 include/block/block.h  |  3 +-
 block.c| 20 +-
 qemu-io-cmds.c | 19 +++--
 tests/qemu-iotests/187 | 69 ++
 tests/qemu-iotests/187.out | 18 
 tests/qemu-iotests/group   |  1 +
 6 files changed, 120 insertions(+), 10 deletions(-)
 create mode 100755 tests/qemu-iotests/187
 create mode 100644 tests/qemu-iotests/187.out

-- 
2.13.3

Re: [Qemu-block] [PATCH v3 7/7] block: add throttle block filter driver interface tests

[Manos Pitsidianakis]

On Thu, Aug 03, 2017 at 03:32:58PM +0200, Kevin Wolf wrote:

Am 03.08.2017 um 15:24 hat Manos Pitsidianakis geschrieben:

On Thu, Aug 03, 2017 at 10:07:50AM +0200, Kevin Wolf wrote:
> Am 31.07.2017 um 11:54 hat Manos Pitsidianakis geschrieben:
> > Signed-off-by: Manos Pitsidianakis 
>
> I would add at least two more cases:
>
> * Both limits and throttle-group are given in blockdev-add
This exists in the "property changes in ThrottleGroup" section,


You're right, I missed this. The test result shows that this command
succeeds. Do we really want to allow other nodes to be affected with a
blockdev-add? Wouldn't it be cleaner to just forbid the combination of
limits and throtte-group?


So basically only anonymous, immutable groups can be created through the 
driver then. All other shared group configurations must be explicitly 
created with an -object / object-add syntax. I think this is a neat 
separation and compromise if we allow anonymous groups. If not, we can 
ignore limits on the throttle driver.




> * limits and throttle-group are both missing
this creates an anonymous group with no limits. Should we fail at this case?


I'm not sure, you could argue either way. But there should be a test to
check that the semantics won't change.

If we're going with Stefan's suggestion that anonymous groups shouldn't
exist, then the question is moot anyway.


> It would also be nice to test that query-block reflects the new throttle
> group limits correctly when they are changed after the fact.

This belongs to the remove legacy patch, since query-block displays the
legacy limits.


Ok, fair enough.

Kevin





signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH v3 7/7] block: add throttle block filter driver interface tests

[Kevin Wolf]

Am 03.08.2017 um 15:24 hat Manos Pitsidianakis geschrieben:
> On Thu, Aug 03, 2017 at 10:07:50AM +0200, Kevin Wolf wrote:
> > Am 31.07.2017 um 11:54 hat Manos Pitsidianakis geschrieben:
> > > Signed-off-by: Manos Pitsidianakis 
> > 
> > I would add at least two more cases:
> > 
> > * Both limits and throttle-group are given in blockdev-add
> This exists in the "property changes in ThrottleGroup" section,

You're right, I missed this. The test result shows that this command
succeeds. Do we really want to allow other nodes to be affected with a
blockdev-add? Wouldn't it be cleaner to just forbid the combination of
limits and throtte-group?

> > * limits and throttle-group are both missing
> this creates an anonymous group with no limits. Should we fail at this case?

I'm not sure, you could argue either way. But there should be a test to
check that the semantics won't change.

If we're going with Stefan's suggestion that anonymous groups shouldn't
exist, then the question is moot anyway.

> > It would also be nice to test that query-block reflects the new throttle
> > group limits correctly when they are changed after the fact.
> 
> This belongs to the remove legacy patch, since query-block displays the
> legacy limits.

Ok, fair enough.

Kevin


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH v3 7/7] block: add throttle block filter driver interface tests

[Manos Pitsidianakis]

On Thu, Aug 03, 2017 at 10:07:50AM +0200, Kevin Wolf wrote:

Am 31.07.2017 um 11:54 hat Manos Pitsidianakis geschrieben:

Signed-off-by: Manos Pitsidianakis 


I would add at least two more cases:

* Both limits and throttle-group are given in blockdev-add

This exists in the "property changes in ThrottleGroup" section,


* limits and throttle-group are both missing
this creates an anonymous group with no limits. Should we fail at this 
case? 


It would also be nice to test that query-block reflects the new throttle
group limits correctly when they are changed after the fact.


This belongs to the remove legacy patch, since query-block displays the 
legacy limits.


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH v3 4/7] block: convert ThrottleGroup to object with QOM

[Manos Pitsidianakis]

On Thu, Aug 03, 2017 at 01:17:01PM +0200, Kevin Wolf wrote:

Am 03.08.2017 um 12:53 hat Stefan Hajnoczi geschrieben:

On Thu, Aug 03, 2017 at 10:08:01AM +0200, Kevin Wolf wrote:
> Am 02.08.2017 um 12:57 hat Manos Pitsidianakis geschrieben:
> > On Wed, Aug 02, 2017 at 11:39:22AM +0100, Stefan Hajnoczi wrote:
> > > On Tue, Aug 01, 2017 at 07:49:33PM +0300, Manos Pitsidianakis wrote:
> > > > On Tue, Aug 01, 2017 at 04:47:03PM +0100, Stefan Hajnoczi wrote:
> > > > > On Mon, Jul 31, 2017 at 12:54:40PM +0300, Manos Pitsidianakis wrote:
> > > > > > ThrottleGroup is converted to an object. This will allow the future
> > > > > > throttle block filter drive easy creation and configuration of 
throttle
> > > > > > groups in QMP and cli.
> > > > > >
> > > > > > A new QAPI struct, ThrottleLimits, is introduced to provide a shared
> > > > > > struct for all throttle configuration needs in QMP.
> > > > > >
> > > > > > ThrottleGroups can be created via CLI as
> > > > > > -object throttle-group,id=foo,x-iops-total=100,x-..
> > > > > > where x-* are individual limit properties. Since we can't add 
non-scalar
> > > > > > properties in -object this interface must be used instead. However,
> > > > > > setting these properties must be disabled after initialization 
because
> > > > > > certain combinations of limits are forbidden and thus configuration
> > > > > > changes should be done in one transaction. The individual properties
> > > > > > will go away when support for non-scalar values in CLI is 
implemented
> > > > > > and thus are marked as experimental.
> > > > > >
> > > > > > ThrottleGroup also has a `limits` property that uses the 
ThrottleLimits
> > > > > > struct.  It can be used to create ThrottleGroups or set the
> > > > > > configuration in existing groups as follows:
> > > > > >
> > > > > > { "execute": "object-add",
> > > > > >   "arguments": {
> > > > > > "qom-type": "throttle-group",
> > > > > > "id": "foo",
> > > > > > "props" : {
> > > > > >   "limits": {
> > > > > >   "iops-total": 100
> > > > > >   }
> > > > > > }
> > > > > >   }
> > > > > > }
> > > > > > { "execute" : "qom-set",
> > > > > > "arguments" : {
> > > > > > "path" : "foo",
> > > > > > "property" : "limits",
> > > > > > "value" : {
> > > > > > "iops-total" : 99
> > > > > > }
> > > > > > }
> > > > > > }
> > > > > >
> > > > > > This also means a group's configuration can be fetched with qom-get.
> > > > > >
> > > > > > ThrottleGroups can be anonymous which means they can't get accessed 
by
> > > > > > other users ie they will always be units instead of group (Because 
they
> > > > > > have one ThrottleGroupMember).
> > > > >
> > > > > blockdev.c automatically assigns -drive id= to the group name if
> > > > > throttling.group= wasn't given.  So who will use anonymous 
single-drive
> > > > > mode?
> > > >
> > > > Manual filter nodes. It's possible to not pass a group name value and 
the
> > > > resulting group will be anonymous. Are you suggesting to move this 
change to
> > > > the throttle filter patch?
> > >
> > > What is the use case?  Human users will stick to the legacy syntax
> > > because it's convenient.  Management tools will use the filter
> > > explicitly in the future, and it's easy for them to choose a name.
> > >
> > > Unless there is a need for this case I'd prefer to make the group name
> > > mandatory.  That way there are less code paths to worry about.
> >
> > I think Kevin requested this though I don't really remember the use case.
>
> There is no legacy syntax for putting a throttle node anywhere but at
> the root of a BlockBackend. If you want to throttle e.g. only a specific
> backing file, you need to manually create a throttle node.
>
> (We tend to forget this occasionally, but the work you're doing is not
> only cleanup just for fun, but it's actually new features that enable
> new use cases by making everything more flexible.)

It's not clear to me from your reply whether you support anonymous
throttle groups or not.  It is possible to throttle arbitrary nodes in
the graph either way.


I think it would be nice for human users to have them, but on second
thought you're right that it's just syntactic sugar for an explicit
-object, so if you think there is any difficulty with supporting
anonymous groups, feel free to drop them.

Hm, actually... Does this mean that then the whole 'limits' option in
the throttle driver could go away, with all of the parsing code, and the
group name becomes mandatory? That certainly does look tempting.



Anonymous groups don't pose any difficulty. The only problem with groups 
not created with -object or object-add in general is that their limits 
cannot be set with qom-set afterwards; the throttle node will require a 
reopen or replacement with a new one. This is a good argument against 
doing throttle group manipulation in the driver. I don't know if that's 
very user friendly though.


signature.asc

Re: [Qemu-block] [Qemu-devel] [PATCH v3 5/7] block: add throttle block filter driver

[Eric Blake]

On 08/03/2017 03:07 AM, Kevin Wolf wrote:
> Am 31.07.2017 um 11:54 hat Manos Pitsidianakis geschrieben:
>> block/throttle.c uses existing I/O throttle infrastructure inside a
>> block filter driver. I/O operations are intercepted in the filter's
>> read/write coroutines, and referred to block/throttle-groups.c
>>
>> The driver can be used with the syntax
>> -drive driver=throttle,file.filename=foo.qcow2, \
>> limits.iops-total=...,throttle-group=bar
>>
>> The configuration flags and their semantics are identical to the
>> hardcoded throttling ones.
>>
>> A node can be created referring to an existing group, and will overwrite
>> its limits if any are specified, otherwise they are retained.
>>
>> Signed-off-by: Manos Pitsidianakis 
>> ---

>> +
>> +.is_filter  =   true,
>> +};
> 
> What about .bdrv_co_get_block_status?

And if so, do you want my byte-based block status to go in first?  (Our
two series conflict, so we need to pick who needs to rebase on top of
the other).

-- 
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] [PATCH v3 5/7] block: add throttle block filter driver

[Manos Pitsidianakis]

On Thu, Aug 03, 2017 at 10:07:41AM +0200, Kevin Wolf wrote:

Am 31.07.2017 um 11:54 hat Manos Pitsidianakis geschrieben:

+/* Extract ThrottleConfig options. Assumes cfg is initialized and will be
+ * checked for validity.
+ */
+static int throttle_extract_options(QemuOpts *opts, ThrottleConfig *cfg,
+ Error **errp)
+{
+if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_BPS_TOTAL)) {
+cfg->buckets[THROTTLE_BPS_TOTAL].avg =
+qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX QEMU_OPT_BPS_TOTAL,
+0);
+}
+if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_BPS_READ)) {
+cfg->buckets[THROTTLE_BPS_READ].avg  =
+qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX QEMU_OPT_BPS_READ,
+0);
+}
+if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_BPS_WRITE)) {
+cfg->buckets[THROTTLE_BPS_WRITE].avg =
+qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX QEMU_OPT_BPS_WRITE,
+0);
+}
+if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_IOPS_TOTAL)) {
+cfg->buckets[THROTTLE_OPS_TOTAL].avg =
+qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX QEMU_OPT_IOPS_TOTAL,
+0);
+}
+if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_IOPS_READ)) {
+cfg->buckets[THROTTLE_OPS_READ].avg =
+qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX QEMU_OPT_IOPS_READ,
+0);
+}
+if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_IOPS_WRITE)) {
+cfg->buckets[THROTTLE_OPS_WRITE].avg =
+qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX QEMU_OPT_IOPS_WRITE,
+0);
+}
+if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_BPS_TOTAL_MAX)) {
+cfg->buckets[THROTTLE_BPS_TOTAL].max =
+qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX
+QEMU_OPT_BPS_TOTAL_MAX, 0);
+}
+if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_BPS_READ_MAX)) {
+cfg->buckets[THROTTLE_BPS_READ].max  =
+qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX
+QEMU_OPT_BPS_READ_MAX, 0);
+}
+if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_BPS_WRITE_MAX)) {
+cfg->buckets[THROTTLE_BPS_WRITE].max =
+qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX
+QEMU_OPT_BPS_WRITE_MAX, 0);
+}
+if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_IOPS_TOTAL_MAX)) {
+cfg->buckets[THROTTLE_OPS_TOTAL].max =
+qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX
+QEMU_OPT_IOPS_TOTAL_MAX, 0);
+}
+if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_IOPS_READ_MAX)) {
+cfg->buckets[THROTTLE_OPS_READ].max =
+qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX
+QEMU_OPT_IOPS_READ_MAX, 0);
+}
+if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_IOPS_WRITE_MAX)) {
+cfg->buckets[THROTTLE_OPS_WRITE].max =
+qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX
+QEMU_OPT_IOPS_WRITE_MAX, 0);
+}
+if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_BPS_TOTAL_MAX_LENGTH)) 
{
+if (qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX
+QEMU_OPT_BPS_TOTAL_MAX_LENGTH, 1) > UINT_MAX) {
+error_setg(errp, "%s value must be in the range [0, %u]",
+   THROTTLE_OPT_PREFIX QEMU_OPT_BPS_TOTAL_MAX_LENGTH,
+   UINT_MAX);
+return -1;
+}
+cfg->buckets[THROTTLE_BPS_TOTAL].burst_length =
+qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX
+QEMU_OPT_BPS_TOTAL_MAX_LENGTH, 1);
+}
+if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_BPS_READ_MAX_LENGTH)) {
+if (qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX
+QEMU_OPT_BPS_READ_MAX_LENGTH, 1) > UINT_MAX) {
+error_setg(errp, "%s must be in the range [0, %u]",
+   THROTTLE_OPT_PREFIX QEMU_OPT_BPS_READ_MAX_LENGTH,
+   UINT_MAX);
+return -1;
+}
+cfg->buckets[THROTTLE_BPS_READ].burst_length  =
+qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX
+QEMU_OPT_BPS_READ_MAX_LENGTH, 1);
+}
+if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_BPS_WRITE_MAX_LENGTH)) 
{
+if (qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX
+QEMU_OPT_BPS_WRITE_MAX_LENGTH, 1) > UINT_MAX) {
+error_setg(errp, "%s must be in the range [0, %u]",
+   THROTTLE_OPT_PREFIX QEMU_OPT_BPS_WRITE_MAX_LENGTH,
+   UINT_MAX);
+return -1;
+}
+

Re: [Qemu-block] [PATCH v3 4/7] block: convert ThrottleGroup to object with QOM

[Kevin Wolf]

Am 03.08.2017 um 12:53 hat Stefan Hajnoczi geschrieben:
> On Thu, Aug 03, 2017 at 10:08:01AM +0200, Kevin Wolf wrote:
> > Am 02.08.2017 um 12:57 hat Manos Pitsidianakis geschrieben:
> > > On Wed, Aug 02, 2017 at 11:39:22AM +0100, Stefan Hajnoczi wrote:
> > > > On Tue, Aug 01, 2017 at 07:49:33PM +0300, Manos Pitsidianakis wrote:
> > > > > On Tue, Aug 01, 2017 at 04:47:03PM +0100, Stefan Hajnoczi wrote:
> > > > > > On Mon, Jul 31, 2017 at 12:54:40PM +0300, Manos Pitsidianakis wrote:
> > > > > > > ThrottleGroup is converted to an object. This will allow the 
> > > > > > > future
> > > > > > > throttle block filter drive easy creation and configuration of 
> > > > > > > throttle
> > > > > > > groups in QMP and cli.
> > > > > > >
> > > > > > > A new QAPI struct, ThrottleLimits, is introduced to provide a 
> > > > > > > shared
> > > > > > > struct for all throttle configuration needs in QMP.
> > > > > > >
> > > > > > > ThrottleGroups can be created via CLI as
> > > > > > > -object throttle-group,id=foo,x-iops-total=100,x-..
> > > > > > > where x-* are individual limit properties. Since we can't add 
> > > > > > > non-scalar
> > > > > > > properties in -object this interface must be used instead. 
> > > > > > > However,
> > > > > > > setting these properties must be disabled after initialization 
> > > > > > > because
> > > > > > > certain combinations of limits are forbidden and thus 
> > > > > > > configuration
> > > > > > > changes should be done in one transaction. The individual 
> > > > > > > properties
> > > > > > > will go away when support for non-scalar values in CLI is 
> > > > > > > implemented
> > > > > > > and thus are marked as experimental.
> > > > > > >
> > > > > > > ThrottleGroup also has a `limits` property that uses the 
> > > > > > > ThrottleLimits
> > > > > > > struct.  It can be used to create ThrottleGroups or set the
> > > > > > > configuration in existing groups as follows:
> > > > > > >
> > > > > > > { "execute": "object-add",
> > > > > > >   "arguments": {
> > > > > > > "qom-type": "throttle-group",
> > > > > > > "id": "foo",
> > > > > > > "props" : {
> > > > > > >   "limits": {
> > > > > > >   "iops-total": 100
> > > > > > >   }
> > > > > > > }
> > > > > > >   }
> > > > > > > }
> > > > > > > { "execute" : "qom-set",
> > > > > > > "arguments" : {
> > > > > > > "path" : "foo",
> > > > > > > "property" : "limits",
> > > > > > > "value" : {
> > > > > > > "iops-total" : 99
> > > > > > > }
> > > > > > > }
> > > > > > > }
> > > > > > >
> > > > > > > This also means a group's configuration can be fetched with 
> > > > > > > qom-get.
> > > > > > >
> > > > > > > ThrottleGroups can be anonymous which means they can't get 
> > > > > > > accessed by
> > > > > > > other users ie they will always be units instead of group 
> > > > > > > (Because they
> > > > > > > have one ThrottleGroupMember).
> > > > > >
> > > > > > blockdev.c automatically assigns -drive id= to the group name if
> > > > > > throttling.group= wasn't given.  So who will use anonymous 
> > > > > > single-drive
> > > > > > mode?
> > > > > 
> > > > > Manual filter nodes. It's possible to not pass a group name value and 
> > > > > the
> > > > > resulting group will be anonymous. Are you suggesting to move this 
> > > > > change to
> > > > > the throttle filter patch?
> > > > 
> > > > What is the use case?  Human users will stick to the legacy syntax
> > > > because it's convenient.  Management tools will use the filter
> > > > explicitly in the future, and it's easy for them to choose a name.
> > > > 
> > > > Unless there is a need for this case I'd prefer to make the group name
> > > > mandatory.  That way there are less code paths to worry about.
> > > 
> > > I think Kevin requested this though I don't really remember the use case.
> > 
> > There is no legacy syntax for putting a throttle node anywhere but at
> > the root of a BlockBackend. If you want to throttle e.g. only a specific
> > backing file, you need to manually create a throttle node.
> > 
> > (We tend to forget this occasionally, but the work you're doing is not
> > only cleanup just for fun, but it's actually new features that enable
> > new use cases by making everything more flexible.)
> 
> It's not clear to me from your reply whether you support anonymous
> throttle groups or not.  It is possible to throttle arbitrary nodes in
> the graph either way.

I think it would be nice for human users to have them, but on second
thought you're right that it's just syntactic sugar for an explicit
-object, so if you think there is any difficulty with supporting
anonymous groups, feel free to drop them.

Hm, actually... Does this mean that then the whole 'limits' option in
the throttle driver could go away, with all of the parsing code, and the
group name becomes mandatory? That certainly does look tempting.

Kevin


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH 3/3] block: remove legacy I/O throttling

[Kevin Wolf]

Am 01.08.2017 um 15:49 hat Manos Pitsidianakis geschrieben:
> This commit removes all I/O throttling from block/block-backend.c. In
> order to support the existing interface, it is changed to use the
> block/throttle.c filter driver.
> 
> The throttle filter node that is created by the legacy interface is
> stored in a 'throttle_node' field in the BlockBackendPublic of the
> device. The legacy throttle node is managed by the legacy interface
> completely. More advanced configurations with the filter drive are
> possible using the QMP API, but these will be ignored by the legacy
> interface.
> 
> Signed-off-by: Manos Pitsidianakis 

> -void blk_io_limits_disable(BlockBackend *blk)
> +void blk_io_limits_disable(BlockBackend *blk, Error **errp)
>  {
> -assert(blk->public.throttle_group_member.throttle_state);
> -bdrv_drained_begin(blk_bs(blk));
> -throttle_group_unregister_tgm(>public.throttle_group_member);
> -bdrv_drained_end(blk_bs(blk));
> +BlockDriverState *bs, *throttle_node;
> +
> +throttle_node = blk_get_public(blk)->throttle_node;
> +
> +assert(throttle_node && throttle_node->refcnt == 1);

I don't think you can assert that nobody else references the node. Once
it's there, it can be used. That doesn't really have to bother the
BlockBackend, though. Or can you think of a case that would break if the
throttle_node were kept alive by someone else?

> +bs = throttle_node->file->bs;
> +blk_get_public(blk)->throttle_node = NULL;
> +
> +/* ref throttle_node's child bs so that it isn't lost when throttle_node 
> is
> + * destroyed */
> +bdrv_ref(bs);
> +
> +/* this destroys throttle_node */
> +blk_remove_bs(blk);

Without the assertion, it doesn't necessarily destroy the node, but it
gives up our reference.

> +blk_insert_bs(blk, bs, _abort);
> +bdrv_unref(bs);
>  }
>  
>  /* should be called before blk_set_io_limits if a limit is set */
> -void blk_io_limits_enable(BlockBackend *blk, const char *group)
> +void blk_io_limits_enable(BlockBackend *blk, const char *group,  Error 
> **errp)
>  {
> -assert(!blk->public.throttle_group_member.throttle_state);
> -throttle_group_register_tgm(>public.throttle_group_member,
> -group, blk_get_aio_context(blk));
> +BlockDriverState *bs = blk_bs(blk), *throttle_node;
> +QDict *options = qdict_new();
> +Error *local_err = NULL;
> +
> +bdrv_drained_begin(bs);
> +/*
> + * increase bs ref count so it doesn't get deleted when removed
> + * from the BlockBackend's root
> + * */
> +bdrv_ref(bs);
> +blk_remove_bs(blk);
> +
> +qdict_set_default_str(options, "file", bs->node_name);
> +qdict_set_default_str(options, QEMU_OPT_THROTTLE_GROUP_NAME, group);
> +throttle_node = bdrv_new_open_driver(bdrv_find_format("throttle"),
> +NULL, bdrv_get_flags(bs), options, errp);
> +QDECREF(options);
> +if (!throttle_node) {
> +blk_insert_bs(blk, bs, _abort);

Wouldn't it be easier to just do the blk_remove_bs() later?

I think _abort isn't entirely correct because blk_insert_bs() can
fail theoretically if image locking prevents this (some other process
would have to grab the lock between blk_remove_bs() and this, so it's
unlikely, but it's possible). So it would be nice if we could avoid it.

> +bdrv_unref(bs);
> +bdrv_drained_end(bs);
> +return;
> +}
> +throttle_node->implicit = true;
> +/* bs will be throttle_node's child now so unref it*/
> +bdrv_unref(bs);

The bdrv_ref/unref() pair could be avoided if you did blk_remove_bs()
only here when the new reference already exists.

> +blk_insert_bs(blk, throttle_node, _err);
> +if (local_err) {
> +bdrv_ref(bs);
> +bdrv_unref(throttle_node);
> +blk_insert_bs(blk, bs, _abort);
> +bdrv_unref(bs);
> +error_propagate(errp, local_err);
> +return;
> +}
> +bdrv_unref(throttle_node);
> +
> +assert(throttle_node->file->bs == bs);
> +assert(throttle_node->refcnt == 1);
> +
> +bdrv_drained_end(bs);
> +
> +blk_get_public(blk)->throttle_node = throttle_node;
>  }
>  
> -void blk_io_limits_update_group(BlockBackend *blk, const char *group)
> +void blk_io_limits_update_group(BlockBackend *blk, const char *group, Error 
> **errp)
>  {
> +ThrottleGroupMember *tgm;
> +Error *local_err = NULL;
> +
>  /* this BB is not part of any group */
> -if (!blk->public.throttle_group_member.throttle_state) {
> +if (!blk->public.throttle_node) {
>  return;
>  }
>  
> +tgm = throttle_get_tgm(blk->public.throttle_node);
>  /* this BB is a part of the same group than the one we want */
> -if 
> (!g_strcmp0(throttle_group_get_name(>public.throttle_group_member),
> +if (!g_strcmp0(throttle_group_get_name(tgm),
>  group)) {
>  return;
>  }
>  
> -/* need to change the group this bs 

Re: [Qemu-block] [PATCH v3 4/7] block: convert ThrottleGroup to object with QOM

[Stefan Hajnoczi]

On Thu, Aug 03, 2017 at 10:08:01AM +0200, Kevin Wolf wrote:
> Am 02.08.2017 um 12:57 hat Manos Pitsidianakis geschrieben:
> > On Wed, Aug 02, 2017 at 11:39:22AM +0100, Stefan Hajnoczi wrote:
> > > On Tue, Aug 01, 2017 at 07:49:33PM +0300, Manos Pitsidianakis wrote:
> > > > On Tue, Aug 01, 2017 at 04:47:03PM +0100, Stefan Hajnoczi wrote:
> > > > > On Mon, Jul 31, 2017 at 12:54:40PM +0300, Manos Pitsidianakis wrote:
> > > > > > ThrottleGroup is converted to an object. This will allow the future
> > > > > > throttle block filter drive easy creation and configuration of 
> > > > > > throttle
> > > > > > groups in QMP and cli.
> > > > > >
> > > > > > A new QAPI struct, ThrottleLimits, is introduced to provide a shared
> > > > > > struct for all throttle configuration needs in QMP.
> > > > > >
> > > > > > ThrottleGroups can be created via CLI as
> > > > > > -object throttle-group,id=foo,x-iops-total=100,x-..
> > > > > > where x-* are individual limit properties. Since we can't add 
> > > > > > non-scalar
> > > > > > properties in -object this interface must be used instead. However,
> > > > > > setting these properties must be disabled after initialization 
> > > > > > because
> > > > > > certain combinations of limits are forbidden and thus configuration
> > > > > > changes should be done in one transaction. The individual properties
> > > > > > will go away when support for non-scalar values in CLI is 
> > > > > > implemented
> > > > > > and thus are marked as experimental.
> > > > > >
> > > > > > ThrottleGroup also has a `limits` property that uses the 
> > > > > > ThrottleLimits
> > > > > > struct.  It can be used to create ThrottleGroups or set the
> > > > > > configuration in existing groups as follows:
> > > > > >
> > > > > > { "execute": "object-add",
> > > > > >   "arguments": {
> > > > > > "qom-type": "throttle-group",
> > > > > > "id": "foo",
> > > > > > "props" : {
> > > > > >   "limits": {
> > > > > >   "iops-total": 100
> > > > > >   }
> > > > > > }
> > > > > >   }
> > > > > > }
> > > > > > { "execute" : "qom-set",
> > > > > > "arguments" : {
> > > > > > "path" : "foo",
> > > > > > "property" : "limits",
> > > > > > "value" : {
> > > > > > "iops-total" : 99
> > > > > > }
> > > > > > }
> > > > > > }
> > > > > >
> > > > > > This also means a group's configuration can be fetched with qom-get.
> > > > > >
> > > > > > ThrottleGroups can be anonymous which means they can't get accessed 
> > > > > > by
> > > > > > other users ie they will always be units instead of group (Because 
> > > > > > they
> > > > > > have one ThrottleGroupMember).
> > > > >
> > > > > blockdev.c automatically assigns -drive id= to the group name if
> > > > > throttling.group= wasn't given.  So who will use anonymous 
> > > > > single-drive
> > > > > mode?
> > > > 
> > > > Manual filter nodes. It's possible to not pass a group name value and 
> > > > the
> > > > resulting group will be anonymous. Are you suggesting to move this 
> > > > change to
> > > > the throttle filter patch?
> > > 
> > > What is the use case?  Human users will stick to the legacy syntax
> > > because it's convenient.  Management tools will use the filter
> > > explicitly in the future, and it's easy for them to choose a name.
> > > 
> > > Unless there is a need for this case I'd prefer to make the group name
> > > mandatory.  That way there are less code paths to worry about.
> > 
> > I think Kevin requested this though I don't really remember the use case.
> 
> There is no legacy syntax for putting a throttle node anywhere but at
> the root of a BlockBackend. If you want to throttle e.g. only a specific
> backing file, you need to manually create a throttle node.
> 
> (We tend to forget this occasionally, but the work you're doing is not
> only cleanup just for fun, but it's actually new features that enable
> new use cases by making everything more flexible.)

It's not clear to me from your reply whether you support anonymous
throttle groups or not.  It is possible to throttle arbitrary nodes in
the graph either way.

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH 2/3] block: skip implicit nodes in snapshots, blockjobs

[Kevin Wolf]

Am 01.08.2017 um 15:49 hat Manos Pitsidianakis geschrieben:
> Implicit filter nodes added at the top of nodes can interfere with block
> jobs. This is not a problem when they are added by other jobs since
> adding another job will issue a QERR_DEVICE_IN_USE, but it can happen in
> the next commit which introduces an implicitly created throttle filter
> node below BlockBackend, which we want to be skipped during automatic
> operations on the graph since the user does not necessarily know about
> their existence.
> 
> Signed-off-by: Manos Pitsidianakis 
> ---
>  block.c   | 17 +
>  blockdev.c| 12 
>  include/block/block_int.h |  2 ++
>  3 files changed, 31 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 886a457ab0..9ebdba28b0 100644
> --- a/block.c
> +++ b/block.c
> @@ -4947,3 +4947,20 @@ bool bdrv_can_store_new_dirty_bitmap(BlockDriverState 
> *bs, const char *name,
>  
>  return drv->bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp);
>  }
> +
> +/* Get first non-implicit node down a bs chain. */
> +BlockDriverState *bdrv_get_first_non_implicit(BlockDriverState *bs)
> +{
> +if (!bs) {
> +return NULL;
> +}
> +
> +if (!bs->implicit) {
> +return bs;
> +}
> +
> +/* at this point bs is implicit and must have a child */
> +assert(bs->file);
> +
> +return bdrv_get_first_non_implicit(bs->file->bs);
> +}

mirror_top/commit_top use bs->backing instead of bs->file, so this
assertion would fail for them.

It's probably better to assert that the implicit node has only one child
and then use that child, whatever it may be.

I'd also prefer the function to work iteratively rather than recursively
because chains can be rather long. (I know that we recurse in many
places anyway, so it's not a strong argument, but I think it would also
look a bit nicer.)

Kevin

Re: [Qemu-block] [PATCH v3 5/7] block: add throttle block filter driver

[Kevin Wolf]

Am 31.07.2017 um 11:54 hat Manos Pitsidianakis geschrieben:
> block/throttle.c uses existing I/O throttle infrastructure inside a
> block filter driver. I/O operations are intercepted in the filter's
> read/write coroutines, and referred to block/throttle-groups.c
> 
> The driver can be used with the syntax
> -drive driver=throttle,file.filename=foo.qcow2, \
> limits.iops-total=...,throttle-group=bar
> 
> The configuration flags and their semantics are identical to the
> hardcoded throttling ones.
> 
> A node can be created referring to an existing group, and will overwrite
> its limits if any are specified, otherwise they are retained.
> 
> Signed-off-by: Manos Pitsidianakis 
> ---
>  block/Makefile.objs |   1 +
>  block/throttle.c| 395 
> 
>  include/qemu/throttle-options.h |   1 +
>  3 files changed, 397 insertions(+)
>  create mode 100644 block/throttle.c
> 
> diff --git a/block/Makefile.objs b/block/Makefile.objs
> index 2aaede4ae1..6eaf78a046 100644
> --- a/block/Makefile.objs
> +++ b/block/Makefile.objs
> @@ -25,6 +25,7 @@ block-obj-y += accounting.o dirty-bitmap.o
>  block-obj-y += write-threshold.o
>  block-obj-y += backup.o
>  block-obj-$(CONFIG_REPLICATION) += replication.o
> +block-obj-y += throttle.o
>  
>  block-obj-y += crypto.o
>  
> diff --git a/block/throttle.c b/block/throttle.c
> new file mode 100644
> index 00..f3395462fb
> --- /dev/null
> +++ b/block/throttle.c
> @@ -0,0 +1,395 @@
> +/*
> + * QEMU block throttling filter driver infrastructure
> + *
> + * Copyright (c) 2017 Manos Pitsidianakis
> + *
> + * 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 or
> + * (at your option) version 3 of the License.
> + *
> + * 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 .
> + */
> +
> +#include "qemu/osdep.h"
> +#include "block/throttle-groups.h"
> +#include "qemu/throttle-options.h"
> +#include "qapi/error.h"
> +
> +#undef THROTTLE_OPT_PREFIX
> +#define THROTTLE_OPT_PREFIX "limits."
> +static QemuOptsList throttle_opts = {
> +.name = "throttle",
> +.head = QTAILQ_HEAD_INITIALIZER(throttle_opts.head),
> +.desc = {
> +THROTTLE_OPTS,
> +{
> +.name = QEMU_OPT_THROTTLE_GROUP_NAME,
> +.type = QEMU_OPT_STRING,
> +.help = "throttle group name",
> +},
> +{ /* end of list */ }
> +},
> +};
> +
> +/* Extract ThrottleConfig options. Assumes cfg is initialized and will be
> + * checked for validity.
> + */
> +static int throttle_extract_options(QemuOpts *opts, ThrottleConfig *cfg,
> + Error **errp)
> +{
> +if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_BPS_TOTAL)) {
> +cfg->buckets[THROTTLE_BPS_TOTAL].avg =
> +qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX QEMU_OPT_BPS_TOTAL,
> +0);
> +}
> +if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_BPS_READ)) {
> +cfg->buckets[THROTTLE_BPS_READ].avg  =
> +qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX QEMU_OPT_BPS_READ,
> +0);
> +}
> +if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_BPS_WRITE)) {
> +cfg->buckets[THROTTLE_BPS_WRITE].avg =
> +qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX QEMU_OPT_BPS_WRITE,
> +0);
> +}
> +if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_IOPS_TOTAL)) {
> +cfg->buckets[THROTTLE_OPS_TOTAL].avg =
> +qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX 
> QEMU_OPT_IOPS_TOTAL,
> +0);
> +}
> +if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_IOPS_READ)) {
> +cfg->buckets[THROTTLE_OPS_READ].avg =
> +qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX QEMU_OPT_IOPS_READ,
> +0);
> +}
> +if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_IOPS_WRITE)) {
> +cfg->buckets[THROTTLE_OPS_WRITE].avg =
> +qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX 
> QEMU_OPT_IOPS_WRITE,
> +0);
> +}
> +if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_BPS_TOTAL_MAX)) {
> +cfg->buckets[THROTTLE_BPS_TOTAL].max =
> +qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX
> +QEMU_OPT_BPS_TOTAL_MAX, 0);
> +}
> +if (qemu_opt_get(opts, 

Re: [Qemu-block] [PATCH v3 4/7] block: convert ThrottleGroup to object with QOM

[Kevin Wolf]

Am 02.08.2017 um 12:57 hat Manos Pitsidianakis geschrieben:
> On Wed, Aug 02, 2017 at 11:39:22AM +0100, Stefan Hajnoczi wrote:
> > On Tue, Aug 01, 2017 at 07:49:33PM +0300, Manos Pitsidianakis wrote:
> > > On Tue, Aug 01, 2017 at 04:47:03PM +0100, Stefan Hajnoczi wrote:
> > > > On Mon, Jul 31, 2017 at 12:54:40PM +0300, Manos Pitsidianakis wrote:
> > > > > ThrottleGroup is converted to an object. This will allow the future
> > > > > throttle block filter drive easy creation and configuration of 
> > > > > throttle
> > > > > groups in QMP and cli.
> > > > >
> > > > > A new QAPI struct, ThrottleLimits, is introduced to provide a shared
> > > > > struct for all throttle configuration needs in QMP.
> > > > >
> > > > > ThrottleGroups can be created via CLI as
> > > > > -object throttle-group,id=foo,x-iops-total=100,x-..
> > > > > where x-* are individual limit properties. Since we can't add 
> > > > > non-scalar
> > > > > properties in -object this interface must be used instead. However,
> > > > > setting these properties must be disabled after initialization because
> > > > > certain combinations of limits are forbidden and thus configuration
> > > > > changes should be done in one transaction. The individual properties
> > > > > will go away when support for non-scalar values in CLI is implemented
> > > > > and thus are marked as experimental.
> > > > >
> > > > > ThrottleGroup also has a `limits` property that uses the 
> > > > > ThrottleLimits
> > > > > struct.  It can be used to create ThrottleGroups or set the
> > > > > configuration in existing groups as follows:
> > > > >
> > > > > { "execute": "object-add",
> > > > >   "arguments": {
> > > > > "qom-type": "throttle-group",
> > > > > "id": "foo",
> > > > > "props" : {
> > > > >   "limits": {
> > > > >   "iops-total": 100
> > > > >   }
> > > > > }
> > > > >   }
> > > > > }
> > > > > { "execute" : "qom-set",
> > > > > "arguments" : {
> > > > > "path" : "foo",
> > > > > "property" : "limits",
> > > > > "value" : {
> > > > > "iops-total" : 99
> > > > > }
> > > > > }
> > > > > }
> > > > >
> > > > > This also means a group's configuration can be fetched with qom-get.
> > > > >
> > > > > ThrottleGroups can be anonymous which means they can't get accessed by
> > > > > other users ie they will always be units instead of group (Because 
> > > > > they
> > > > > have one ThrottleGroupMember).
> > > >
> > > > blockdev.c automatically assigns -drive id= to the group name if
> > > > throttling.group= wasn't given.  So who will use anonymous single-drive
> > > > mode?
> > > 
> > > Manual filter nodes. It's possible to not pass a group name value and the
> > > resulting group will be anonymous. Are you suggesting to move this change 
> > > to
> > > the throttle filter patch?
> > 
> > What is the use case?  Human users will stick to the legacy syntax
> > because it's convenient.  Management tools will use the filter
> > explicitly in the future, and it's easy for them to choose a name.
> > 
> > Unless there is a need for this case I'd prefer to make the group name
> > mandatory.  That way there are less code paths to worry about.
> 
> I think Kevin requested this though I don't really remember the use case.

There is no legacy syntax for putting a throttle node anywhere but at
the root of a BlockBackend. If you want to throttle e.g. only a specific
backing file, you need to manually create a throttle node.

(We tend to forget this occasionally, but the work you're doing is not
only cleanup just for fun, but it's actually new features that enable
new use cases by making everything more flexible.)

Kevin


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH v3 7/7] block: add throttle block filter driver interface tests

[Kevin Wolf]

Am 31.07.2017 um 11:54 hat Manos Pitsidianakis geschrieben:
> Signed-off-by: Manos Pitsidianakis 

I would add at least two more cases:

* Both limits and throttle-group are given in blockdev-add
* limits and throttle-group are both missing

It would also be nice to test that query-block reflects the new throttle
group limits correctly when they are changed after the fact.

Kevin