11:49 a.m.
query-dirty-rate command is used for virsh domstats regardless of qemu version, but this
is available only on qemu >=5.2.0.
So virsh domstats fails if qemu is older.
This patchset fixes the bug.
I added it to NEWS because I thought it is a bug that users want to find, but if it is not
so please simply ignore the last commit.
Hiroki Narukawa (4):
qemu_capabilities: Add QEMU_CAPS_QUERY_DIRTY_RATE capability
qemu_driver: add required capabilities to qemuDomainGetStatsWorkers
qemu_driver: add check for qemu capabilities requirements
NEWS: document bug fix about virsh domstats on qemu < 5.2.0
NEWS.rst | 5 ++
src/qemu/qemu_capabilities.c | 2 +
src/qemu/qemu_capabilities.h | 1 +
src/qemu/qemu_driver.c | 72 +++++++++++++------
.../caps_5.2.0.aarch64.xml | 1 +
.../qemucapabilitiesdata/caps_5.2.0.ppc64.xml | 1 +
.../caps_5.2.0.riscv64.xml | 1 +
.../qemucapabilitiesdata/caps_5.2.0.s390x.xml | 1 +
.../caps_5.2.0.x86_64.xml | 1 +
.../caps_6.0.0.aarch64.xml | 1 +
.../qemucapabilitiesdata/caps_6.0.0.s390x.xml | 1 +
.../caps_6.0.0.x86_64.xml | 1 +
.../caps_6.1.0.x86_64.xml | 1 +
.../caps_6.2.0.x86_64.xml | 1 +
14 files changed, 68 insertions(+), 22 deletions(-)
--
2.17.1
11:49 a.m.
New subject: [PATCH 1/4] qemu_capabilities: Add QEMU_CAPS_QUERY_DIRTY_RATE capability
query-dirty-rate command is used for virsh domstats by default, but this
is available only on qemu >=5.2.0.
In this commit, add capability flag for query-dirty-rate first.
Signed-off-by: Hiroki Narukawa <hnarukaw(a)yahoo-corp.jp>
---
src/qemu/qemu_capabilities.c | 2 ++
src/qemu/qemu_capabilities.h | 1 +
tests/qemucapabilitiesdata/caps_5.2.0.aarch64.xml | 1 +
tests/qemucapabilitiesdata/caps_5.2.0.ppc64.xml | 1 +
tests/qemucapabilitiesdata/caps_5.2.0.riscv64.xml | 1 +
tests/qemucapabilitiesdata/caps_5.2.0.s390x.xml | 1 +
tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_6.0.0.aarch64.xml | 1 +
tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml | 1 +
tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml | 1 +
12 files changed, 13 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 32be7e67f1..9dfddd0129 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -650,6 +650,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
/* 415 */
"chardev.json", /* QEMU_CAPS_CHARDEV_JSON */
"device.json", /* QEMU_CAPS_DEVICE_JSON */
+ "query-dirty-rate", /* QEMU_CAPS_QUERY_DIRTY_RATE */
);
@@ -1194,6 +1195,7 @@ struct virQEMUCapsStringFlags virQEMUCapsCommands[] = {
{ "blockdev-reopen", QEMU_CAPS_BLOCKDEV_REOPEN },
{ "set-numa-node", QEMU_CAPS_NUMA },
{ "set-action", QEMU_CAPS_SET_ACTION },
+ { "query-dirty-rate", QEMU_CAPS_QUERY_DIRTY_RATE },
};
struct virQEMUCapsStringFlags virQEMUCapsMigration[] = {
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 5b5215416f..6a4c5a723d 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -630,6 +630,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check
*/
/* 415 */
QEMU_CAPS_CHARDEV_JSON, /* -chardev accepts JSON */
QEMU_CAPS_DEVICE_JSON, /* -device accepts JSON */
+ QEMU_CAPS_QUERY_DIRTY_RATE, /* accepts query-dirty-rate */
QEMU_CAPS_LAST /* this must always be the last item */
} virQEMUCapsFlags;
diff --git a/tests/qemucapabilitiesdata/caps_5.2.0.aarch64.xml
b/tests/qemucapabilitiesdata/caps_5.2.0.aarch64.xml
index 042060235e..f1b23d7ea4 100644
--- a/tests/qemucapabilitiesdata/caps_5.2.0.aarch64.xml
+++ b/tests/qemucapabilitiesdata/caps_5.2.0.aarch64.xml
@@ -185,6 +185,7 @@
<flag name='input-linux'/>
<flag name='query-display-options'/>
<flag name='virtio-blk.queue-size'/>
+ <flag name='query-dirty-rate'/>
<version>5002000</version>
<kvmVersion>0</kvmVersion>
<microcodeVersion>61700243</microcodeVersion>
diff --git a/tests/qemucapabilitiesdata/caps_5.2.0.ppc64.xml
b/tests/qemucapabilitiesdata/caps_5.2.0.ppc64.xml
index 61328e1987..1c29ebd5d5 100644
--- a/tests/qemucapabilitiesdata/caps_5.2.0.ppc64.xml
+++ b/tests/qemucapabilitiesdata/caps_5.2.0.ppc64.xml
@@ -191,6 +191,7 @@
<flag name='input-linux'/>
<flag name='query-display-options'/>
<flag name='virtio-blk.queue-size'/>
+ <flag name='query-dirty-rate'/>
<version>5002000</version>
<kvmVersion>0</kvmVersion>
<microcodeVersion>42900243</microcodeVersion>
diff --git a/tests/qemucapabilitiesdata/caps_5.2.0.riscv64.xml
b/tests/qemucapabilitiesdata/caps_5.2.0.riscv64.xml
index 5f7b02a396..06f83cf543 100644
--- a/tests/qemucapabilitiesdata/caps_5.2.0.riscv64.xml
+++ b/tests/qemucapabilitiesdata/caps_5.2.0.riscv64.xml
@@ -176,6 +176,7 @@
<flag name='input-linux'/>
<flag name='query-display-options'/>
<flag name='virtio-blk.queue-size'/>
+ <flag name='query-dirty-rate'/>
<version>5002000</version>
<kvmVersion>0</kvmVersion>
<microcodeVersion>0</microcodeVersion>
diff --git a/tests/qemucapabilitiesdata/caps_5.2.0.s390x.xml
b/tests/qemucapabilitiesdata/caps_5.2.0.s390x.xml
index 3c653acbde..d4466bf781 100644
--- a/tests/qemucapabilitiesdata/caps_5.2.0.s390x.xml
+++ b/tests/qemucapabilitiesdata/caps_5.2.0.s390x.xml
@@ -142,6 +142,7 @@
<flag name='input-linux'/>
<flag name='query-display-options'/>
<flag name='virtio-blk.queue-size'/>
+ <flag name='query-dirty-rate'/>
<version>5002000</version>
<kvmVersion>0</kvmVersion>
<microcodeVersion>39100243</microcodeVersion>
diff --git a/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml
index bd2dc77d21..507b2a95a3 100644
--- a/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml
@@ -229,6 +229,7 @@
<flag name='virtio-blk.queue-size'/>
<flag name='virtio-mem-pci'/>
<flag name='piix4.acpi-root-pci-hotplug'/>
+ <flag name='query-dirty-rate'/>
<version>5002000</version>
<kvmVersion>0</kvmVersion>
<microcodeVersion>43100243</microcodeVersion>
diff --git a/tests/qemucapabilitiesdata/caps_6.0.0.aarch64.xml
b/tests/qemucapabilitiesdata/caps_6.0.0.aarch64.xml
index 0c897137a4..0838f42843 100644
--- a/tests/qemucapabilitiesdata/caps_6.0.0.aarch64.xml
+++ b/tests/qemucapabilitiesdata/caps_6.0.0.aarch64.xml
@@ -193,6 +193,7 @@
<flag name='query-display-options'/>
<flag name='set-action'/>
<flag name='virtio-blk.queue-size'/>
+ <flag name='query-dirty-rate'/>
<version>6000000</version>
<kvmVersion>0</kvmVersion>
<microcodeVersion>61700242</microcodeVersion>
diff --git a/tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml
b/tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml
index 5d75b2ef63..0e33c45539 100644
--- a/tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml
+++ b/tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml
@@ -150,6 +150,7 @@
<flag name='s390-pv-guest'/>
<flag name='set-action'/>
<flag name='virtio-blk.queue-size'/>
+ <flag name='query-dirty-rate'/>
<version>6000000</version>
<kvmVersion>0</kvmVersion>
<microcodeVersion>39100242</microcodeVersion>
diff --git a/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml
index dc0c01c25b..9eb948002a 100644
--- a/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml
@@ -237,6 +237,7 @@
<flag name='virtio-blk.queue-size'/>
<flag name='virtio-mem-pci'/>
<flag name='piix4.acpi-root-pci-hotplug'/>
+ <flag name='query-dirty-rate'/>
<version>6000000</version>
<kvmVersion>0</kvmVersion>
<microcodeVersion>43100242</microcodeVersion>
diff --git a/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
index 871e85a2d3..2af1b1ec32 100644
--- a/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
@@ -241,6 +241,7 @@
<flag name='memory-backend-file.reserve'/>
<flag name='piix4.acpi-root-pci-hotplug'/>
<flag name='ich9.acpi-hotplug-bridge'/>
+ <flag name='query-dirty-rate'/>
<version>6001000</version>
<kvmVersion>0</kvmVersion>
<microcodeVersion>43100243</microcodeVersion>
diff --git a/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml
index e5b9bfce0b..25eea8a486 100644
--- a/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml
@@ -241,6 +241,7 @@
<flag name='memory-backend-file.reserve'/>
<flag name='piix4.acpi-root-pci-hotplug'/>
<flag name='ich9.acpi-hotplug-bridge'/>
+ <flag name='query-dirty-rate'/>
<version>6001050</version>
<kvmVersion>0</kvmVersion>
<microcodeVersion>43100244</microcodeVersion>
--
2.17.1
11:49 a.m.
New subject: [PATCH 2/4] qemu_driver: add required capabilities to qemuDomainGetStatsWorkers
One of qemuDomainGetStatsWorkers requires capabilities to run.
This commit adds capability information to qemuDomainGetStatsWorkers.
Signed-off-by: Hiroki Narukawa <hnarukaw(a)yahoo-corp.jp>
---
src/qemu/qemu_driver.c | 32 +++++++++++++++++++++-----------
1 file changed, 21 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 2b759f5719..ac5eaf139e 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18685,20 +18685,30 @@ struct qemuDomainGetStatsWorker {
qemuDomainGetStatsFunc func;
unsigned int stats;
bool monitor;
+ virQEMUCapsFlags* requiredCaps;
+};
+
+static virQEMUCapsFlags noCapsRequired[] = {
+ QEMU_CAPS_LAST,
+};
+
+static virQEMUCapsFlags queryDirtyRateRequired[] = {
+ QEMU_CAPS_QUERY_DIRTY_RATE,
+ QEMU_CAPS_LAST,
};
static struct qemuDomainGetStatsWorker qemuDomainGetStatsWorkers[] = {
- { qemuDomainGetStatsState, VIR_DOMAIN_STATS_STATE, false },
- { qemuDomainGetStatsCpu, VIR_DOMAIN_STATS_CPU_TOTAL, false },
- { qemuDomainGetStatsBalloon, VIR_DOMAIN_STATS_BALLOON, true },
- { qemuDomainGetStatsVcpu, VIR_DOMAIN_STATS_VCPU, true },
- { qemuDomainGetStatsInterface, VIR_DOMAIN_STATS_INTERFACE, false },
- { qemuDomainGetStatsBlock, VIR_DOMAIN_STATS_BLOCK, true },
- { qemuDomainGetStatsPerf, VIR_DOMAIN_STATS_PERF, false },
- { qemuDomainGetStatsIOThread, VIR_DOMAIN_STATS_IOTHREAD, true },
- { qemuDomainGetStatsMemory, VIR_DOMAIN_STATS_MEMORY, false },
- { qemuDomainGetStatsDirtyRate, VIR_DOMAIN_STATS_DIRTYRATE, true },
- { NULL, 0, false }
+ { qemuDomainGetStatsState, VIR_DOMAIN_STATS_STATE, false, noCapsRequired },
+ { qemuDomainGetStatsCpu, VIR_DOMAIN_STATS_CPU_TOTAL, false, noCapsRequired },
+ { qemuDomainGetStatsBalloon, VIR_DOMAIN_STATS_BALLOON, true, noCapsRequired },
+ { qemuDomainGetStatsVcpu, VIR_DOMAIN_STATS_VCPU, true, noCapsRequired },
+ { qemuDomainGetStatsInterface, VIR_DOMAIN_STATS_INTERFACE, false, noCapsRequired },
+ { qemuDomainGetStatsBlock, VIR_DOMAIN_STATS_BLOCK, true, noCapsRequired },
+ { qemuDomainGetStatsPerf, VIR_DOMAIN_STATS_PERF, false, noCapsRequired },
+ { qemuDomainGetStatsIOThread, VIR_DOMAIN_STATS_IOTHREAD, true, noCapsRequired },
+ { qemuDomainGetStatsMemory, VIR_DOMAIN_STATS_MEMORY, false, noCapsRequired },
+ { qemuDomainGetStatsDirtyRate, VIR_DOMAIN_STATS_DIRTYRATE, true,
queryDirtyRateRequired },
+ { NULL, 0, false, NULL }
};
--
2.17.1
3:51 p.m.
New subject: [PATCH 2/4] qemu_driver: add required capabilities to qemuDomainGetStatsWorkers
On 10/15/21 11:49 AM, Hiroki Narukawa wrote:
One of qemuDomainGetStatsWorkers requires capabilities to run.
This commit adds capability information to qemuDomainGetStatsWorkers.
Signed-off-by: Hiroki Narukawa <hnarukaw(a)yahoo-corp.jp>
---
src/qemu/qemu_driver.c | 32 +++++++++++++++++++++-----------
1 file changed, 21 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 2b759f5719..ac5eaf139e 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18685,20 +18685,30 @@ struct qemuDomainGetStatsWorker {
qemuDomainGetStatsFunc func;
unsigned int stats;
bool monitor;
+ virQEMUCapsFlags* requiredCaps;
We prefer the '*' char next to variable, like this:
virQEMUCapsFlags *requiredCaps;
+};
+
+static virQEMUCapsFlags noCapsRequired[] = {
+ QEMU_CAPS_LAST,
+};
+
+static virQEMUCapsFlags queryDirtyRateRequired[] = {
+ QEMU_CAPS_QUERY_DIRTY_RATE,
+ QEMU_CAPS_LAST,
};
static struct qemuDomainGetStatsWorker qemuDomainGetStatsWorkers[] = {
- { qemuDomainGetStatsState, VIR_DOMAIN_STATS_STATE, false },
- { qemuDomainGetStatsCpu, VIR_DOMAIN_STATS_CPU_TOTAL, false },
- { qemuDomainGetStatsBalloon, VIR_DOMAIN_STATS_BALLOON, true },
- { qemuDomainGetStatsVcpu, VIR_DOMAIN_STATS_VCPU, true },
- { qemuDomainGetStatsInterface, VIR_DOMAIN_STATS_INTERFACE, false },
- { qemuDomainGetStatsBlock, VIR_DOMAIN_STATS_BLOCK, true },
- { qemuDomainGetStatsPerf, VIR_DOMAIN_STATS_PERF, false },
- { qemuDomainGetStatsIOThread, VIR_DOMAIN_STATS_IOTHREAD, true },
- { qemuDomainGetStatsMemory, VIR_DOMAIN_STATS_MEMORY, false },
- { qemuDomainGetStatsDirtyRate, VIR_DOMAIN_STATS_DIRTYRATE, true },
- { NULL, 0, false }
+ { qemuDomainGetStatsState, VIR_DOMAIN_STATS_STATE, false, noCapsRequired },
+ { qemuDomainGetStatsCpu, VIR_DOMAIN_STATS_CPU_TOTAL, false, noCapsRequired },
+ { qemuDomainGetStatsBalloon, VIR_DOMAIN_STATS_BALLOON, true, noCapsRequired },
+ { qemuDomainGetStatsVcpu, VIR_DOMAIN_STATS_VCPU, true, noCapsRequired },
+ { qemuDomainGetStatsInterface, VIR_DOMAIN_STATS_INTERFACE, false, noCapsRequired },
+ { qemuDomainGetStatsBlock, VIR_DOMAIN_STATS_BLOCK, true, noCapsRequired },
+ { qemuDomainGetStatsPerf, VIR_DOMAIN_STATS_PERF, false, noCapsRequired },
+ { qemuDomainGetStatsIOThread, VIR_DOMAIN_STATS_IOTHREAD, true, noCapsRequired },
+ { qemuDomainGetStatsMemory, VIR_DOMAIN_STATS_MEMORY, false, noCapsRequired },
+ { qemuDomainGetStatsDirtyRate, VIR_DOMAIN_STATS_DIRTYRATE, true,
queryDirtyRateRequired },
+ { NULL, 0, false, NULL }
How about instead of noCapsRequired a simple NULL would be passed? The
consumer can then check for NULL and skip the loop completely.
Michal
11:49 a.m.
New subject: [PATCH 3/4] qemu_driver: add check for qemu capabilities requirements
query-dirty-rate command is used for virsh domstats by default, but this
is available only on qemu >=5.2.0.
By this commit, qemu domain stats will check capabilities requirements before issuing
actual query.
Signed-off-by: Hiroki Narukawa <hnarukaw(a)yahoo-corp.jp>
---
src/qemu/qemu_driver.c | 40 +++++++++++++++++++++++++++++-----------
1 file changed, 29 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index ac5eaf139e..9cfd0a6ca5 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18714,13 +18714,28 @@ static struct qemuDomainGetStatsWorker
qemuDomainGetStatsWorkers[] = {
static int
qemuDomainGetStatsCheckSupport(unsigned int *stats,
- bool enforce)
+ bool enforce,
+ virDomainObj *vm)
{
+ qemuDomainObjPrivate *priv = vm->privateData;
unsigned int supportedstats = 0;
size_t i;
+ virQEMUCapsFlags* flagCursor;
- for (i = 0; qemuDomainGetStatsWorkers[i].func; i++)
- supportedstats |= qemuDomainGetStatsWorkers[i].stats;
+ for (i = 0; qemuDomainGetStatsWorkers[i].func; i++) {
+ bool supportedByQemu = true;
+
+ for (flagCursor = qemuDomainGetStatsWorkers[i].requiredCaps; *flagCursor !=
QEMU_CAPS_LAST;
+ ++flagCursor) {
+ if (!virQEMUCapsGet(priv->qemuCaps, *flagCursor)) {
+ supportedByQemu = false;
+ break;
+ }
+ }
+ if (supportedByQemu) {
+ supportedstats |= qemuDomainGetStatsWorkers[i].stats;
+ }
+ }
if (*stats == 0) {
*stats = supportedstats;
@@ -18791,7 +18806,7 @@ static int
qemuConnectGetAllDomainStats(virConnectPtr conn,
virDomainPtr *doms,
unsigned int ndoms,
- unsigned int stats,
+ unsigned int requestedStats,
virDomainStatsRecordPtr **retStats,
unsigned int flags)
{
@@ -18805,11 +18820,12 @@ qemuConnectGetAllDomainStats(virConnectPtr conn,
int nstats = 0;
size_t i;
int ret = -1;
- unsigned int privflags = 0;
+ unsigned int privflags;
unsigned int domflags = 0;
unsigned int lflags = flags & (VIR_CONNECT_LIST_DOMAINS_FILTERS_ACTIVE |
VIR_CONNECT_LIST_DOMAINS_FILTERS_PERSISTENT |
VIR_CONNECT_LIST_DOMAINS_FILTERS_STATE);
+ unsigned int stats;
virCheckFlags(VIR_CONNECT_LIST_DOMAINS_FILTERS_ACTIVE |
VIR_CONNECT_LIST_DOMAINS_FILTERS_PERSISTENT |
@@ -18821,9 +18837,6 @@ qemuConnectGetAllDomainStats(virConnectPtr conn,
if (virConnectGetAllDomainStatsEnsureACL(conn) < 0)
return -1;
- if (qemuDomainGetStatsCheckSupport(&stats, enforce) < 0)
- return -1;
-
if (ndoms) {
if (virDomainObjListConvert(driver->domains, conn, doms, ndoms, &vms,
&nvms, virConnectGetAllDomainStatsCheckACL,
@@ -18838,14 +18851,19 @@ qemuConnectGetAllDomainStats(virConnectPtr conn,
tmpstats = g_new0(virDomainStatsRecordPtr, nvms + 1);
- if (qemuDomainGetStatsNeedMonitor(stats))
- privflags |= QEMU_DOMAIN_STATS_HAVE_JOB;
-
for (i = 0; i < nvms; i++) {
virDomainStatsRecordPtr tmp = NULL;
domflags = 0;
+ privflags = 0;
vm = vms[i];
+ stats = requestedStats;
+ if (qemuDomainGetStatsCheckSupport(&stats, enforce, vm) < 0)
+ return -1;
+
+ if (qemuDomainGetStatsNeedMonitor(stats))
+ privflags |= QEMU_DOMAIN_STATS_HAVE_JOB;
+
virObjectLock(vm);
if (HAVE_JOB(privflags)) {
--
2.17.1
3:51 p.m.
New subject: [PATCH 3/4] qemu_driver: add check for qemu capabilities requirements
On 10/15/21 11:49 AM, Hiroki Narukawa wrote:
query-dirty-rate command is used for virsh domstats by default, but
this
is available only on qemu >=5.2.0.
By this commit, qemu domain stats will check capabilities requirements before issuing
actual query.
Signed-off-by: Hiroki Narukawa <hnarukaw(a)yahoo-corp.jp>
---
src/qemu/qemu_driver.c | 40 +++++++++++++++++++++++++++++-----------
1 file changed, 29 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index ac5eaf139e..9cfd0a6ca5 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18714,13 +18714,28 @@ static struct qemuDomainGetStatsWorker
qemuDomainGetStatsWorkers[] = {
static int
qemuDomainGetStatsCheckSupport(unsigned int *stats,
- bool enforce)
+ bool enforce,
+ virDomainObj *vm)
{
+ qemuDomainObjPrivate *priv = vm->privateData;
unsigned int supportedstats = 0;
size_t i;
+ virQEMUCapsFlags* flagCursor;
We like to declare variables inside loops when possible. A variable can
be declared outside if it's immutable (i.e. its value isn't changed
inside loop). So @priv can stay, but @flagCursor should go inside the
for() loop below.
- for (i = 0; qemuDomainGetStatsWorkers[i].func; i++)
- supportedstats |= qemuDomainGetStatsWorkers[i].stats;
+ for (i = 0; qemuDomainGetStatsWorkers[i].func; i++) {
+ bool supportedByQemu = true;
+
+ for (flagCursor = qemuDomainGetStatsWorkers[i].requiredCaps; *flagCursor !=
QEMU_CAPS_LAST;
+ ++flagCursor) {
+ if (!virQEMUCapsGet(priv->qemuCaps, *flagCursor)) {
+ supportedByQemu = false;
+ break;
+ }
+ }
This body will look slightly different if we allow NULL. I suggest:
for (i = 0; qemuDomainGetStatsWorkers[i].func; i++) {
bool supportedByQemu = true;
virQEMUCapsFlags *requiredCaps = qemuDomainGetStatsWorkers[i].requiredCaps;
while (requiredCaps && *requiredCaps != QEMU_CAPS_LAST) {
if (!virQEMUCapsGet(priv->qemuCaps, *requiredCaps)) {
supportedByQemu = false;
break;
}
requiredCaps++;
}
if (supportedByQemu) {
supportedstats |= qemuDomainGetStatsWorkers[i].stats;
}
}
+ if (supportedByQemu) {
+ supportedstats |= qemuDomainGetStatsWorkers[i].stats;
+ }
+ }
if (*stats == 0) {
*stats = supportedstats;
Later in this function there's an error message that deserves to be
updated:
virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED,
_("Stats types bits 0x%x are not supported by this daemon or
QEMU"),
*stats & ~supportedstats);
@@ -18791,7 +18806,7 @@ static int
qemuConnectGetAllDomainStats(virConnectPtr conn,
virDomainPtr *doms,
unsigned int ndoms,
- unsigned int stats,
+ unsigned int requestedStats,
I'd rather not rename this variable (so that its name is the same as in
the public API) and introduce new variable later.
virDomainStatsRecordPtr **retStats,
unsigned int flags)
{
@@ -18805,11 +18820,12 @@ qemuConnectGetAllDomainStats(virConnectPtr conn,
int nstats = 0;
size_t i;
int ret = -1;
- unsigned int privflags = 0;
+ unsigned int privflags;
unsigned int domflags = 0;
unsigned int lflags = flags & (VIR_CONNECT_LIST_DOMAINS_FILTERS_ACTIVE |
VIR_CONNECT_LIST_DOMAINS_FILTERS_PERSISTENT |
VIR_CONNECT_LIST_DOMAINS_FILTERS_STATE);
+ unsigned int stats;
Again, should be moved into the big for() loop below.
virCheckFlags(VIR_CONNECT_LIST_DOMAINS_FILTERS_ACTIVE |
VIR_CONNECT_LIST_DOMAINS_FILTERS_PERSISTENT |
@@ -18821,9 +18837,6 @@ qemuConnectGetAllDomainStats(virConnectPtr conn,
if (virConnectGetAllDomainStatsEnsureACL(conn) < 0)
return -1;
- if (qemuDomainGetStatsCheckSupport(&stats, enforce) < 0)
- return -1;
-
if (ndoms) {
if (virDomainObjListConvert(driver->domains, conn, doms, ndoms, &vms,
&nvms, virConnectGetAllDomainStatsCheckACL,
@@ -18838,14 +18851,19 @@ qemuConnectGetAllDomainStats(virConnectPtr conn,
tmpstats = g_new0(virDomainStatsRecordPtr, nvms + 1);
- if (qemuDomainGetStatsNeedMonitor(stats))
- privflags |= QEMU_DOMAIN_STATS_HAVE_JOB;
-
for (i = 0; i < nvms; i++) {
virDomainStatsRecordPtr tmp = NULL;
domflags = 0;
+ privflags = 0;
vm = vms[i];
+ stats = requestedStats;
+ if (qemuDomainGetStatsCheckSupport(&stats, enforce, vm) < 0)
+ return -1;
+
+ if (qemuDomainGetStatsNeedMonitor(stats))
+ privflags |= QEMU_DOMAIN_STATS_HAVE_JOB;
+
How about the following instead?
for (i = 0; i < nvms; i++) {
virDomainStatsRecordPtr tmp = NULL;
unsigned int privflags = 0;
unsigned int requestedStats = stats;
domflags = 0;
vm = vms[i];
if (qemuDomainGetStatsCheckSupport(&requestedStats, enforce, vm) < 0)
return -1;
if (qemuDomainGetStatsNeedMonitor(requestedStats))
privflags |= QEMU_DOMAIN_STATS_HAVE_JOB;
There's one more line where @stats is used (when calling
qemuDomainGetStats()) and that would also need similar treatment:
if (qemuDomainGetStats(conn, vm, requestedStats, &tmp, domflags) < 0) {
Michal
11:49 a.m.
New subject: [PATCH 4/4] NEWS: document bug fix about virsh domstats on qemu < 5.2.0
I think that virsh domstats problem on qemu < 5.2.0 is what users want
to find which version fixes.
---
NEWS.rst | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/NEWS.rst b/NEWS.rst
index ae678bffc4..2c51628278 100644
--- a/NEWS.rst
+++ b/NEWS.rst
@@ -44,6 +44,11 @@ v7.9.0 (unreleased)
* **Bug fixes**
+ * qemu: Fix problems on ``virsh domstats`` with qemu <5.2.0
+
+ Libvirt v7.2.0 and later called query-dirty-rate, which was introduced in
+ qemu-5.2.0, regardless of qemu version and failed in qemu-5.1.0. This
+ release fixes the bug.
v7.8.0 (2021-10-01)
===================
--
2.17.1
3:51 p.m.
New subject: [PATCH 4/4] NEWS: document bug fix about virsh domstats on qemu < 5.2.0
On 10/15/21 11:49 AM, Hiroki Narukawa wrote:
I think that virsh domstats problem on qemu < 5.2.0 is what users
want
to find which version fixes.
---
NEWS.rst | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/NEWS.rst b/NEWS.rst
index ae678bffc4..2c51628278 100644
--- a/NEWS.rst
+++ b/NEWS.rst
@@ -44,6 +44,11 @@ v7.9.0 (unreleased)
* **Bug fixes**
+ * qemu: Fix problems on ``virsh domstats`` with qemu <5.2.0
+
+ Libvirt v7.2.0 and later called query-dirty-rate, which was introduced in
+ qemu-5.2.0, regardless of qemu version and failed in qemu-5.1.0. This
+ release fixes the bug.
v7.8.0 (2021-10-01)
===================
This is definitely wanted, thank you!
Michal
3:52 p.m.
On 10/15/21 11:49 AM, Hiroki Narukawa wrote:
query-dirty-rate command is used for virsh domstats regardless of
qemu version, but this is available only on qemu >=5.2.0.
So virsh domstats fails if qemu is older.
This patchset fixes the bug.
I added it to NEWS because I thought it is a bug that users want to find, but if it is
not so please simply ignore the last commit.
Hiroki Narukawa (4):
qemu_capabilities: Add QEMU_CAPS_QUERY_DIRTY_RATE capability
qemu_driver: add required capabilities to qemuDomainGetStatsWorkers
qemu_driver: add check for qemu capabilities requirements
NEWS: document bug fix about virsh domstats on qemu < 5.2.0
NEWS.rst | 5 ++
src/qemu/qemu_capabilities.c | 2 +
src/qemu/qemu_capabilities.h | 1 +
src/qemu/qemu_driver.c | 72 +++++++++++++------
.../caps_5.2.0.aarch64.xml | 1 +
.../qemucapabilitiesdata/caps_5.2.0.ppc64.xml | 1 +
.../caps_5.2.0.riscv64.xml | 1 +
.../qemucapabilitiesdata/caps_5.2.0.s390x.xml | 1 +
.../caps_5.2.0.x86_64.xml | 1 +
.../caps_6.0.0.aarch64.xml | 1 +
.../qemucapabilitiesdata/caps_6.0.0.s390x.xml | 1 +
.../caps_6.0.0.x86_64.xml | 1 +
.../caps_6.1.0.x86_64.xml | 1 +
.../caps_6.2.0.x86_64.xml | 1 +
14 files changed, 68 insertions(+), 22 deletions(-)
In general:
Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com>
But please see my comment to individual patches and let me know whether
you agree with my suggestions. I can squash them in and push then.
Michal
4:16 a.m.
Thank you for your review, I checked your suggestions in each patch, and I agree to all
the suggested changes.
Hiroki Narukawa
-----Original Message-----
From: Michal Prívozník <mprivozn(a)redhat.com>
Sent: Friday, October 15, 2021 10:52 PM
To: 成川 弘樹 <hnarukaw(a)yahoo-corp.jp>; libvir-list(a)redhat.com
Cc: 大岩 朗 <aoiwa(a)yahoo-corp.jp>
Subject: Re: [PATCH 0/4] Fix the bug about virsh domstats on qemu <5.2.0
On 10/15/21 11:49 AM, Hiroki Narukawa wrote:
query-dirty-rate command is used for virsh domstats regardless of
qemu version, but this is available only on qemu >=5.2.0.
So virsh domstats fails if qemu is older.
This patchset fixes the bug.
I added it to NEWS because I thought it is a bug that users want to find, but if it is
not so please simply ignore the last commit.
Hiroki Narukawa (4):
qemu_capabilities: Add QEMU_CAPS_QUERY_DIRTY_RATE capability
qemu_driver: add required capabilities to qemuDomainGetStatsWorkers
qemu_driver: add check for qemu capabilities requirements
NEWS: document bug fix about virsh domstats on qemu < 5.2.0
NEWS.rst | 5 ++
src/qemu/qemu_capabilities.c | 2 +
src/qemu/qemu_capabilities.h | 1 +
src/qemu/qemu_driver.c | 72 +++++++++++++------
.../caps_5.2.0.aarch64.xml | 1 +
.../qemucapabilitiesdata/caps_5.2.0.ppc64.xml | 1 +
.../caps_5.2.0.riscv64.xml | 1 +
.../qemucapabilitiesdata/caps_5.2.0.s390x.xml | 1 +
.../caps_5.2.0.x86_64.xml | 1 +
.../caps_6.0.0.aarch64.xml | 1 +
.../qemucapabilitiesdata/caps_6.0.0.s390x.xml | 1 +
.../caps_6.0.0.x86_64.xml | 1 +
.../caps_6.1.0.x86_64.xml | 1 +
.../caps_6.2.0.x86_64.xml | 1 +
14 files changed, 68 insertions(+), 22 deletions(-)
In general:
Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com>
But please see my comment to individual patches and let me know whether you agree with my
suggestions. I can squash them in and push then.
Michal
1069
days inactive
1072
days old
10 comments
2 participants
participants (2)
-
Hiroki Narukawa -
Michal Prívozník