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