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@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

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@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

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