Re: [libvirt] [PATCH 2/3] lxc: factorize code for block stats

7 Mar 2018

s/factorize/Refactor/

On 03/05/2018 12:21 PM, Cédric Bosdonnat wrote:
...
lxcDomainBlockStats and lxcDomainBlockStatsFlags were both using very
similar code. This commit factorizes them into a
s/factorsizes/refactors
...
lxcDomainBlockStatsInternal.
---
 src/lxc/lxc_driver.c | 131 ++++++++++++++++++++-------------------------------
 1 file changed, 50 insertions(+), 81 deletions(-)

diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index fa6fc4643..a16dbcc96 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -2348,28 +2348,22 @@ lxcDomainMergeBlkioDevice(virBlkioDevicePtr *dest_array,
     return 0;
 }
Two empty lines between functions
...
-
-static int
-lxcDomainBlockStats(virDomainPtr dom,
-                    const char *path,
-                    virDomainBlockStatsPtr stats)
+static int lxcDomainBlockStatsInternal(virLXCDriverPtr driver,
+                                       virDomainObjPtr vm,
+                                       const char *path,
+                                       long long *rd_bytes,
+                                       long long *wr_bytes,
+                                       long long *rd_req,
+                                       long long *wr_req)
Preference is
static int
lxcDomainBlockStatsInternal(args)

The downside of passing 4 args is that sometime in the future there's 4
more stats to collect, then 4 more... I think you could just pass the
stats pointer here
...
{
-    virLXCDriverPtr driver = dom->conn->privateData;
     int ret = -1;
-    virDomainObjPtr vm;
     virDomainDiskDefPtr disk = NULL;
     virLXCDomainObjPrivatePtr priv;
-    if (!(vm = lxcDomObjFromDomain(dom)))
-        return ret;
-
     priv = vm->privateData;
-    if (virDomainBlockStatsEnsureACL(dom->conn, vm->def) < 0)
-        goto cleanup;
-
    if (virLXCDomainObjBeginJob(driver, vm, LXC_JOB_QUERY) < 0)
-        goto cleanup;
+        return -1;
if (!virDomainObjIsActive(vm)) {
         virReportError(VIR_ERR_OPERATION_INVALID,
@@ -2386,10 +2380,10 @@ lxcDomainBlockStats(virDomainPtr dom,
     if (!*path) {
         /* empty path - return entire domain blkstats instead */
         ret = virCgroupGetBlkioIoServiced(priv->cgroup,
-                                          &stats->rd_bytes,
-                                          &stats->wr_bytes,
-                                          &stats->rd_req,
-                                          &stats->wr_req);
+                                          rd_bytes,
+                                          wr_bytes,
+                                          rd_req,
+                                          wr_req);
Existing, but if ret < 0, then we return with a generic failed for some
unknown reason. You could add the message from lxcDomainBlockStatsFlags
of _("domain stats query failed") if (ret < 0)

Probably would make the following goto stand out or you could use the if
{} else {} like lxcDomainBlockStatsFlags does.  That's your call.
...
goto endjob;
     }
@@ -2407,13 +2401,36 @@ lxcDomainBlockStats(virDomainPtr dom,
ret = virCgroupGetBlkioIoDeviceServiced(priv->cgroup,
                                             disk->info.alias,
-                                            &stats->rd_bytes,
-                                            &stats->wr_bytes,
-                                            &stats->rd_req,
-                                            &stats->wr_req);
+                                            rd_bytes,
+                                            wr_bytes,
+                                            rd_req,
+                                            wr_req);
endjob:
     virLXCDomainObjEndJob(driver, vm);
+    return ret;
+}
+
2 empty lines
...
+static int
+lxcDomainBlockStats(virDomainPtr dom,
+                    const char *path,
+                    virDomainBlockStatsPtr stats)
+{
+    virLXCDriverPtr driver = dom->conn->privateData;
+    virDomainObjPtr vm;
+    int ret = -1;
+
+    if (!(vm = lxcDomObjFromDomain(dom)))
+        return -1;
+
+    if (virDomainBlockStatsEnsureACL(dom->conn, vm->def) < 0)
+        goto cleanup;
+
+    ret = lxcDomainBlockStatsInternal(driver, vm, path,
+                                      &stats->rd_bytes,
+                                      &stats->wr_bytes,
+                                      &stats->rd_req,
+                                      &stats->wr_req);
cleanup:
     virDomainObjEndAPI(&vm);
@@ -2431,8 +2448,6 @@ lxcDomainBlockStatsFlags(virDomainPtr dom,
Could we take the opportunity to "alter" the "const char * path" to be
"const char *path"...  Same for "int * nparams".
...
virLXCDriverPtr driver = dom->conn->privateData;
     int tmp, ret = -1;
     virDomainObjPtr vm;
-    virDomainDiskDefPtr disk = NULL;
-    virLXCDomainObjPrivatePtr priv;
     long long rd_req, rd_bytes, wr_req, wr_bytes;
Change these to be:

    virDomainBlockStats stats = { 0 };

and pass &stats to lxcDomainBlockStatsInternal and use the stats.*
values results that you want to use in the virTypedParameterAssign calls
...
virTypedParameterPtr param;
@@ -2449,60 +2464,17 @@ lxcDomainBlockStatsFlags(virDomainPtr dom,
     if (!(vm = lxcDomObjFromDomain(dom)))
         return ret;
-    priv = vm->privateData;
-
     if (virDomainBlockStatsFlagsEnsureACL(dom->conn, vm->def) < 0)
         goto cleanup;
-    if (virLXCDomainObjBeginJob(driver, vm, LXC_JOB_QUERY) < 0)
+    if (lxcDomainBlockStatsInternal(driver, vm, path,
+                                    &rd_bytes,
+                                    &wr_bytes,
+                                    &rd_req,
+                                    &wr_req) < 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       "%s", _("domain stats query failed"));
This will overwrite what lxcDomainBlockStatsInternal had and return this
message. As noted above, perhaps this message needs to move into the
*Internal function and be displayed when virCgroupGetBlkioIoServiced fails.

John
...
goto cleanup;
-
-    if (!virDomainObjIsActive(vm)) {
-        virReportError(VIR_ERR_OPERATION_INVALID,
-                       "%s", _("domain is not running"));
-        goto endjob;
-    }
-
-    if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_BLKIO)) {
-        virReportError(VIR_ERR_OPERATION_INVALID, "%s",
-                       _("blkio cgroup isn't mounted"));
-        goto endjob;
-    }
-
-    if (!*path) {
-        /* empty path - return entire domain blkstats instead */
-        if (virCgroupGetBlkioIoServiced(priv->cgroup,
-                                        &rd_bytes,
-                                        &wr_bytes,
-                                        &rd_req,
-                                        &wr_req) < 0) {
-            virReportError(VIR_ERR_INTERNAL_ERROR,
-                           "%s", _("domain stats query failed"));
-            goto endjob;
-        }
-    } else {
-        if (!(disk = virDomainDiskByName(vm->def, path, false))) {
-            virReportError(VIR_ERR_INVALID_ARG,
-                           _("invalid path: %s"), path);
-            goto endjob;
-        }
-
-        if (!disk->info.alias) {
-            virReportError(VIR_ERR_INTERNAL_ERROR,
-                           _("missing disk device alias name for %s"), disk->dst);
-            goto endjob;
-        }
-
-        if (virCgroupGetBlkioIoDeviceServiced(priv->cgroup,
-                                              disk->info.alias,
-                                              &rd_bytes,
-                                              &wr_bytes,
-                                              &rd_req,
-                                              &wr_req) < 0) {
-            virReportError(VIR_ERR_INTERNAL_ERROR,
-                           "%s", _("domain stats query failed"));
-            goto endjob;
-        }
     }
tmp = 0;
@@ -2512,7 +2484,7 @@ lxcDomainBlockStatsFlags(virDomainPtr dom,
         param = ¶ms[tmp];
         if (virTypedParameterAssign(param, VIR_DOMAIN_BLOCK_STATS_WRITE_BYTES,
                                     VIR_TYPED_PARAM_LLONG, wr_bytes) < 0)
-            goto endjob;
+            goto cleanup;
         tmp++;
     }
@@ -2520,7 +2492,7 @@ lxcDomainBlockStatsFlags(virDomainPtr dom,
         param = ¶ms[tmp];
         if (virTypedParameterAssign(param, VIR_DOMAIN_BLOCK_STATS_WRITE_REQ,
                                     VIR_TYPED_PARAM_LLONG, wr_req) < 0)
-            goto endjob;
+            goto cleanup;
         tmp++;
     }
@@ -2528,7 +2500,7 @@ lxcDomainBlockStatsFlags(virDomainPtr dom,
         param = ¶ms[tmp];
         if (virTypedParameterAssign(param, VIR_DOMAIN_BLOCK_STATS_READ_BYTES,
                                     VIR_TYPED_PARAM_LLONG, rd_bytes) < 0)
-            goto endjob;
+            goto cleanup;
         tmp++;
     }
@@ -2536,16 +2508,13 @@ lxcDomainBlockStatsFlags(virDomainPtr dom,
         param = ¶ms[tmp];
         if (virTypedParameterAssign(param, VIR_DOMAIN_BLOCK_STATS_READ_REQ,
                                     VIR_TYPED_PARAM_LLONG, rd_req) < 0)
-            goto endjob;
+            goto cleanup;
         tmp++;
     }
ret = 0;
     *nparams = tmp;
- endjob:
-    virLXCDomainObjEndJob(driver, vm);
-
  cleanup:
     virDomainObjEndAPI(&vm);
     return ret;


    

Re: [libvirt] [PATCH 2/3] lxc: factorize code for block stats

John Ferlan