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

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