[PATCH v3 09/25] qemuDomainGetStatsBlock: Don't directly access virTypedParamList

Peter Krempa posted 25 patches 2 years, 9 months ago
[PATCH v3 09/25] qemuDomainGetStatsBlock: Don't directly access virTypedParamList
Posted by Peter Krempa 2 years, 9 months ago
The struct will be made private in upcoming patches. Construct the list
of block entries into a separate list and append them rather than
remember the index of the count element.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 src/qemu/qemu_driver.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 4cf5e8a512..b89bf176bb 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18107,9 +18107,9 @@ qemuDomainGetStatsBlock(virQEMUDriver *driver,
     g_autoptr(GHashTable) stats = NULL;
     qemuDomainObjPrivate *priv = dom->privateData;
     g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
-    int count_index = -1;
     size_t visited = 0;
     bool visitBacking = !!(privflags & QEMU_DOMAIN_STATS_BACKING);
+    g_autoptr(virTypedParamList) blockparams = virTypedParamListNew();

     if (HAVE_JOB(privflags) && virDomainObjIsActive(dom)) {
         qemuDomainObjEnterMonitor(dom);
@@ -18126,21 +18126,17 @@ qemuDomainGetStatsBlock(virQEMUDriver *driver,
             virResetLastError();
     }

-    /* When listing backing chains, it's easier to fix up the count
-     * after the iteration than it is to iterate twice; but we still
-     * want count listed first.  */
-    count_index = params->npar;
-    if (virTypedParamListAddUInt(params, 0, "block.count") < 0)
-        return -1;
-
     for (i = 0; i < dom->def->ndisks; i++) {
         if (qemuDomainGetStatsBlockExportDisk(dom->def->disks[i], stats,
-                                              params, &visited,
+                                              blockparams, &visited,
                                               visitBacking, driver, cfg, dom) < 0)
             return -1;
     }

-    params->par[count_index].value.ui = visited;
+    if (virTypedParamListAddUInt(params, 0, "block.count") < 0)
+        return -1;
+
+    virTypedParamListConcat(params, &blockparams);

     return 0;
 }
-- 
2.39.2
Re: [PATCH v3 09/25] qemuDomainGetStatsBlock: Don't directly access virTypedParamList
Posted by Martin Kletzander 2 years, 9 months ago
On Wed, Apr 19, 2023 at 02:04:26PM +0200, Peter Krempa wrote:
>The struct will be made private in upcoming patches. Construct the list
>of block entries into a separate list and append them rather than
>remember the index of the count element.
>
>Signed-off-by: Peter Krempa <pkrempa@redhat.com>
>---
> src/qemu/qemu_driver.c | 16 ++++++----------
> 1 file changed, 6 insertions(+), 10 deletions(-)
>
>diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>index 4cf5e8a512..b89bf176bb 100644
>--- a/src/qemu/qemu_driver.c
>+++ b/src/qemu/qemu_driver.c
>@@ -18107,9 +18107,9 @@ qemuDomainGetStatsBlock(virQEMUDriver *driver,
>     g_autoptr(GHashTable) stats = NULL;
>     qemuDomainObjPrivate *priv = dom->privateData;
>     g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
>-    int count_index = -1;
>     size_t visited = 0;
>     bool visitBacking = !!(privflags & QEMU_DOMAIN_STATS_BACKING);
>+    g_autoptr(virTypedParamList) blockparams = virTypedParamListNew();
>
>     if (HAVE_JOB(privflags) && virDomainObjIsActive(dom)) {
>         qemuDomainObjEnterMonitor(dom);
>@@ -18126,21 +18126,17 @@ qemuDomainGetStatsBlock(virQEMUDriver *driver,
>             virResetLastError();
>     }
>
>-    /* When listing backing chains, it's easier to fix up the count
>-     * after the iteration than it is to iterate twice; but we still
>-     * want count listed first.  */
>-    count_index = params->npar;
>-    if (virTypedParamListAddUInt(params, 0, "block.count") < 0)
>-        return -1;
>-
>     for (i = 0; i < dom->def->ndisks; i++) {
>         if (qemuDomainGetStatsBlockExportDisk(dom->def->disks[i], stats,
>-                                              params, &visited,
>+                                              blockparams, &visited,
>                                               visitBacking, driver, cfg, dom) < 0)
>             return -1;
>     }
>
>-    params->par[count_index].value.ui = visited;
>+    if (virTypedParamListAddUInt(params, 0, "block.count") < 0)

Shouldn't this be:

     if (virTypedParamListAddUInt(params, visited, "block.count") < 0)

instead?

With that changed:

Reviewed-by: Martin Kletzander <mkletzan@redhat.com>

>+        return -1;
>+
>+    virTypedParamListConcat(params, &blockparams);
>
>     return 0;
> }
>-- 
>2.39.2
>
Re: [PATCH v3 09/25] qemuDomainGetStatsBlock: Don't directly access virTypedParamList
Posted by Ján Tomko 2 years, 9 months ago
On a Wednesday in 2023, Peter Krempa wrote:
>The struct will be made private in upcoming patches. Construct the list
>of block entries into a separate list and append them rather than
>remember the index of the count element.
>
>Signed-off-by: Peter Krempa <pkrempa@redhat.com>
>---
> src/qemu/qemu_driver.c | 16 ++++++----------
> 1 file changed, 6 insertions(+), 10 deletions(-)
>
>diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>index 4cf5e8a512..b89bf176bb 100644
>--- a/src/qemu/qemu_driver.c
>+++ b/src/qemu/qemu_driver.c
>@@ -18107,9 +18107,9 @@ qemuDomainGetStatsBlock(virQEMUDriver *driver,
>     g_autoptr(GHashTable) stats = NULL;
>     qemuDomainObjPrivate *priv = dom->privateData;
>     g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
>-    int count_index = -1;
>     size_t visited = 0;
>     bool visitBacking = !!(privflags & QEMU_DOMAIN_STATS_BACKING);
>+    g_autoptr(virTypedParamList) blockparams = virTypedParamListNew();
>
>     if (HAVE_JOB(privflags) && virDomainObjIsActive(dom)) {
>         qemuDomainObjEnterMonitor(dom);
>@@ -18126,21 +18126,17 @@ qemuDomainGetStatsBlock(virQEMUDriver *driver,
>             virResetLastError();
>     }
>
>-    /* When listing backing chains, it's easier to fix up the count
>-     * after the iteration than it is to iterate twice; but we still
>-     * want count listed first.  */
>-    count_index = params->npar;
>-    if (virTypedParamListAddUInt(params, 0, "block.count") < 0)
>-        return -1;
>-
>     for (i = 0; i < dom->def->ndisks; i++) {
>         if (qemuDomainGetStatsBlockExportDisk(dom->def->disks[i], stats,
>-                                              params, &visited,
>+                                              blockparams, &visited,
>                                               visitBacking, driver, cfg, dom) < 0)
>             return -1;
>     }
>
>-    params->par[count_index].value.ui = visited;
>+    if (virTypedParamListAddUInt(params, 0, "block.count") < 0)

s/0/visited/

Jano

>+        return -1;
>+
>+    virTypedParamListConcat(params, &blockparams);
>
>     return 0;
> }
>-- 
>2.39.2
>