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 <[email protected]>
> ---
> 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
