[PATCH 4/5] virsh: domain, host & volume: refactor bigger functions

Thursday, 23 September 2021

This patch refactors a few bigger functions mainly trying to
remove too deep indentation and make them more readable and more
consistent with the rest of the file.

Signed-off-by: Kristina Hanicova <khanicov(a)redhat.com&gt;
---
 tools/virsh-domain.c | 203 ++++++++++++++++++++-----------------------
 tools/virsh-host.c   | 140 ++++++++++++++---------------
 tools/virsh-volume.c |  56 ++++++------
 3 files changed, 190 insertions(+), 209 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index ec427443c4..2881956d78 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -5133,7 +5133,6 @@ cmdSchedinfo(vshControl *ctl, const vshCmd *cmd)
     int nparams = 0;
     int nupdates = 0;
     size_t i;
-    int ret;
     bool ret_val = false;
     unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT;
     bool current = vshCommandOptBool(cmd, "current");
@@ -5161,64 +5160,61 @@ cmdSchedinfo(vshControl *ctl, const vshCmd *cmd)
         goto cleanup;
     }
 
-    if (nparams) {
-        params = g_new0(virTypedParameter, nparams);
+    if (!nparams)
+        goto cleanup;
 
-        memset(params, 0, sizeof(*params) * nparams);
-        if (flags || current) {
-            /* We cannot query both live and config at once, so settle
-               on current in that case.  If we are setting, then the
-               two values should match when we re-query; otherwise, we
-               report the error later.  */
-            ret = virDomainGetSchedulerParametersFlags(dom, params, &nparams,
-                                                       ((live && config) ? 0
-                                                        : flags));
-        } else {
-            ret = virDomainGetSchedulerParameters(dom, params, &nparams);
-        }
-        if (ret == -1)
+    params = g_new0(virTypedParameter, nparams);
+    memset(params, 0, sizeof(*params) * nparams);
+
+    if (flags || current) {
+        /* We cannot query both live and config at once, so settle
+           on current in that case.  If we are setting, then the
+           two values should match when we re-query; otherwise, we
+           report the error later.  */
+        if (virDomainGetSchedulerParametersFlags(dom, params, &nparams,
+                                                 ((live && config) ? 0 : flags))
== -1)
             goto cleanup;
-
-        /* See if any params are being set */
-        if ((nupdates = cmdSchedInfoUpdate(ctl, cmd, params, nparams,
-                                           &updates)) < 0)
+    } else {
+        if (virDomainGetSchedulerParameters(dom, params, &nparams) == -1)
             goto cleanup;
+    }
 
-        /* Update parameters & refresh data */
-        if (nupdates > 0) {
-            if (flags || current)
-                ret = virDomainSetSchedulerParametersFlags(dom, updates,
-                                                           nupdates, flags);
-            else
-                ret = virDomainSetSchedulerParameters(dom, updates, nupdates);
+    /* See if any params are being set */
+    if ((nupdates = cmdSchedInfoUpdate(ctl, cmd, params, nparams,
+                                       &updates)) < 0)
+        goto cleanup;
 
-            if (ret == -1)
+    /* Update parameters & refresh data */
+    if (nupdates > 0) {
+        if (flags || current) {
+            if (virDomainSetSchedulerParametersFlags(dom, updates,
+                                                     nupdates, flags) == -1)
                 goto cleanup;
 
-            if (flags || current)
-                ret = virDomainGetSchedulerParametersFlags(dom, params,
-                                                           &nparams,
-                                                           ((live && config) ? 0
-                                                            : flags));
-            else
-                ret = virDomainGetSchedulerParameters(dom, params, &nparams);
-            if (ret == -1)
+            if (virDomainGetSchedulerParametersFlags(dom, params, &nparams,
+                                                     ((live && config) ? 0 :
flags)) == -1)
                 goto cleanup;
         } else {
-            /* When not doing --set, --live and --config do not mix.  */
-            if (live && config) {
-                vshError(ctl, "%s",
-                         _("cannot query both live and config at once"));
+            if (virDomainSetSchedulerParameters(dom, updates, nupdates) == -1)
                 goto cleanup;
-            }
-        }
 
-        ret_val = true;
-        for (i = 0; i < nparams; i++) {
-            char *str = vshGetTypedParamValue(ctl, &params[i]);
-            vshPrint(ctl, "%-15s: %s\n", params[i].field, str);
-            VIR_FREE(str);
+            if (virDomainGetSchedulerParameters(dom, params, &nparams) == -1)
+                goto cleanup;
         }
+    } else {
+        /* When not doing --set, --live and --config do not mix.  */
+        if (live && config) {
+            vshError(ctl, "%s",
+                     _("cannot query both live and config at once"));
+            goto cleanup;
+        }
+    }
+
+    ret_val = true;
+    for (i = 0; i < nparams; i++) {
+        char *str = vshGetTypedParamValue(ctl, &params[i]);
+        vshPrint(ctl, "%-15s: %s\n", params[i].field, str);
+        VIR_FREE(str);
     }
 
  cleanup:
@@ -6503,7 +6499,6 @@ virshCPUCountCollect(vshControl *ctl,
                      unsigned int flags,
                      bool checkState)
 {
-    int ret = -2;
     virDomainInfo info;
     int count;
     g_autoptr(xmlDoc) xml = NULL;
@@ -6524,11 +6519,11 @@ virshCPUCountCollect(vshControl *ctl,
     /* fallback code */
     if (!(last_error->code == VIR_ERR_NO_SUPPORT ||
           last_error->code == VIR_ERR_INVALID_ARG))
-        goto cleanup;
+        return -2;
 
     if (flags & VIR_DOMAIN_VCPU_GUEST) {
         vshError(ctl, "%s", _("Failed to retrieve vCPU count from the
guest"));
-        goto cleanup;
+        return -2;
     }
 
     if (!(flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG)) &&
@@ -6538,36 +6533,32 @@ virshCPUCountCollect(vshControl *ctl,
     vshResetLibvirtError();
 
     if (flags & VIR_DOMAIN_AFFECT_LIVE) {
-        if (flags & VIR_DOMAIN_VCPU_MAXIMUM) {
-            count = virDomainGetMaxVcpus(dom);
-        } else {
-           if (virDomainGetInfo(dom, &info) < 0)
-               goto cleanup;
+        if (flags & VIR_DOMAIN_VCPU_MAXIMUM)
+            return virDomainGetMaxVcpus(dom);
 
-           count = info.nrVirtCpu;
+       if (virDomainGetInfo(dom, &info) < 0)
+           return -2;
+
+       return info.nrVirtCpu;
+    }
+
+    if (virshDomainGetXMLFromDom(ctl, dom, VIR_DOMAIN_XML_INACTIVE,
+                                 &xml, &ctxt) < 0)
+        return -2;
+
+    if (flags & VIR_DOMAIN_VCPU_MAXIMUM) {
+        if (virXPathInt("string(/domain/vcpu)", ctxt, &count) < 0) {
+            vshError(ctl, "%s", _("Failed to retrieve maximum vcpu
count"));
+            return -2;
         }
     } else {
-        if (virshDomainGetXMLFromDom(ctl, dom, VIR_DOMAIN_XML_INACTIVE,
-                                     &xml, &ctxt) < 0)
-            goto cleanup;
-
-        if (flags & VIR_DOMAIN_VCPU_MAXIMUM) {
-            if (virXPathInt("string(/domain/vcpu)", ctxt, &count) < 0)
{
-                vshError(ctl, "%s", _("Failed to retrieve maximum vcpu
count"));
-                goto cleanup;
-            }
-        } else {
-            if (virXPathInt("string(/domain/vcpu/@current)", ctxt, &count)
< 0) {
-                vshError(ctl, "%s", _("Failed to retrieve current vcpu
count"));
-                goto cleanup;
-            }
+        if (virXPathInt("string(/domain/vcpu/@current)", ctxt, &count) <
0) {
+            vshError(ctl, "%s", _("Failed to retrieve current vcpu
count"));
+            return -2;
         }
     }
 
-    ret = count;
- cleanup:
-
-    return ret;
+    return count;
 }
 
 static bool
@@ -9588,7 +9579,7 @@ cmdQemuMonitorEvent(vshControl *ctl, const vshCmd *cmd)
     vshEventCleanup(ctl);
     if (eventId >= 0 &&
         virConnectDomainQemuMonitorEventDeregister(priv->conn, eventId) < 0)
-        ret = false;
+        return false;
 
     return ret;
 }
@@ -9790,6 +9781,7 @@ cmdLxcEnterNamespace(vshControl *ctl, const vshCmd *cmd)
     int nfdlist;
     int *fdlist;
     size_t i;
+    int status;
     bool setlabel = true;
     g_autofree virSecurityModelPtr secmodel = NULL;
     g_autofree virSecurityLabelPtr seclabel = NULL;
@@ -9828,40 +9820,8 @@ cmdLxcEnterNamespace(vshControl *ctl, const vshCmd *cmd)
      */
     if ((pid = virFork()) < 0)
         return false;
-    if (pid == 0) {
-        int status;
-
-        if (setlabel &&
-            virDomainLxcEnterSecurityLabel(secmodel,
-                                           seclabel,
-                                           NULL,
-                                           0) < 0)
-            _exit(EXIT_CANCELED);
-
-        if (virDomainLxcEnterCGroup(dom, 0) < 0)
-            _exit(EXIT_CANCELED);
-
-        if (virDomainLxcEnterNamespace(dom,
-                                       nfdlist,
-                                       fdlist,
-                                       NULL,
-                                       NULL,
-                                       0) < 0)
-            _exit(EXIT_CANCELED);
-
-        /* Fork a second time because entering the
-         * pid namespace only takes effect after fork
-         */
-        if ((pid = virFork()) < 0)
-            _exit(EXIT_CANCELED);
-        if (pid == 0) {
-            execv(cmdargv[0], cmdargv);
-            _exit(errno == ENOENT ? EXIT_ENOENT : EXIT_CANNOT_INVOKE);
-        }
-        if (virProcessWait(pid, &status, true) < 0)
-            _exit(EXIT_CANNOT_INVOKE);
-        virProcessExitWithStatus(status);
-    } else {
+
+    if (pid != 0) {
         for (i = 0; i < nfdlist; i++)
             VIR_FORCE_CLOSE(fdlist[i]);
         VIR_FREE(fdlist);
@@ -9869,8 +9829,33 @@ cmdLxcEnterNamespace(vshControl *ctl, const vshCmd *cmd)
             vshReportError(ctl);
             return false;
         }
+        return true;
+    }
+
+    if (setlabel &&
+        virDomainLxcEnterSecurityLabel(secmodel, seclabel, NULL, 0) < 0)
+        _exit(EXIT_CANCELED);
+
+    if (virDomainLxcEnterCGroup(dom, 0) < 0)
+        _exit(EXIT_CANCELED);
+
+    if (virDomainLxcEnterNamespace(dom, nfdlist, fdlist, NULL, NULL, 0) < 0)
+        _exit(EXIT_CANCELED);
+
+    /* Fork a second time because entering the
+     * pid namespace only takes effect after fork
+     */
+    if ((pid = virFork()) < 0)
+        _exit(EXIT_CANCELED);
+
+    if (pid == 0) {
+        execv(cmdargv[0], cmdargv);
+        _exit(errno == ENOENT ? EXIT_ENOENT : EXIT_CANNOT_INVOKE);
     }
 
+    if (virProcessWait(pid, &status, true) < 0)
+        _exit(EXIT_CANNOT_INVOKE);
+    virProcessExitWithStatus(status);
     return true;
 }
 
diff --git a/tools/virsh-host.c b/tools/virsh-host.c
index e6ed4a26ce..591746655b 100644
--- a/tools/virsh-host.c
+++ b/tools/virsh-host.c
@@ -162,7 +162,6 @@ cmdFreecell(vshControl *ctl, const vshCmd *cmd)
     bool cellno = vshCommandOptBool(cmd, "cellno");
     size_t i;
     g_autofree char *cap_xml = NULL;
-    g_autoptr(xmlDoc) xml = NULL;
     g_autoptr(xmlXPathContext) ctxt = NULL;
     virshControl *priv = ctl->privData;
 
@@ -171,68 +170,69 @@ cmdFreecell(vshControl *ctl, const vshCmd *cmd)
     if (cellno && vshCommandOptInt(ctl, cmd, "cellno", &cell) <
0)
         return false;
 
-    if (all) {
-        if (!(cap_xml = virConnectGetCapabilities(priv->conn))) {
-            vshError(ctl, "%s", _("unable to get node
capabilities"));
-            return false;
-        }
+    if (!all) {
+        if (cellno) {
+            if (virNodeGetCellsFreeMemory(priv->conn, &memory, cell, 1) != 1)
+                return false;
 
-        xml = virXMLParseStringCtxt(cap_xml, _("(capabilities)"), &ctxt);
-        if (!xml) {
-            vshError(ctl, "%s", _("unable to get node
capabilities"));
-            return false;
+            vshPrint(ctl, "%d: %llu KiB\n", cell, (memory/1024));
+            return true;
         }
 
-        nodes_cnt = virXPathNodeSet("/capabilities/host/topology/cells/cell",
-                                    ctxt, &nodes);
-
-        if (nodes_cnt == -1) {
-            vshError(ctl, "%s", _("could not get information about "
-                                  "NUMA topology"));
+        if ((memory = virNodeGetFreeMemory(priv->conn)) == 0)
             return false;
-        }
 
-        nodes_free = g_new0(unsigned long long, nodes_cnt);
-        nodes_id = g_new0(unsigned long, nodes_cnt);
+        vshPrint(ctl, "%s: %llu KiB\n", _("Total"), (memory/1024));
+        return true;
+    }
 
-        for (i = 0; i < nodes_cnt; i++) {
-            unsigned long id;
-            g_autofree char *val = virXMLPropString(nodes[i], "id");
-            if (virStrToLong_ulp(val, NULL, 10, &id)) {
-                vshError(ctl, "%s", _("conversion from string
failed"));
-                return false;
-            }
-            nodes_id[i] = id;
-            if (virNodeGetCellsFreeMemory(priv->conn, &(nodes_free[i]),
-                                          id, 1) != 1) {
-                vshError(ctl, _("failed to get free memory for NUMA node "
-                                "number: %lu"), id);
-                return false;
-            }
-        }
+    if (!(cap_xml = virConnectGetCapabilities(priv->conn))) {
+        vshError(ctl, "%s", _("unable to get node capabilities"));
+        return false;
+    }
 
-        for (cell = 0; cell < nodes_cnt; cell++) {
-            vshPrint(ctl, "%5lu: %10llu KiB\n", nodes_id[cell],
-                    (nodes_free[cell]/1024));
-            memory += nodes_free[cell];
-        }
+    if (!virXMLParseStringCtxt(cap_xml, _("(capabilities)"), &ctxt)) {
+        vshError(ctl, "%s", _("unable to get node capabilities"));
+        return false;
+    }
 
-        vshPrintExtra(ctl, "--------------------\n");
-        vshPrintExtra(ctl, "%5s: %10llu KiB\n", _("Total"),
memory/1024);
-    } else {
-        if (cellno) {
-            if (virNodeGetCellsFreeMemory(priv->conn, &memory, cell, 1) != 1)
-                return false;
+    nodes_cnt = virXPathNodeSet("/capabilities/host/topology/cells/cell",
+                                ctxt, &nodes);
 
-            vshPrint(ctl, "%d: %llu KiB\n", cell, (memory/1024));
-        } else {
-            if ((memory = virNodeGetFreeMemory(priv->conn)) == 0)
-                return false;
+    if (nodes_cnt == -1) {
+        vshError(ctl, "%s", _("could not get information about "
+                              "NUMA topology"));
+        return false;
+    }
+
+    nodes_free = g_new0(unsigned long long, nodes_cnt);
+    nodes_id = g_new0(unsigned long, nodes_cnt);
 
-            vshPrint(ctl, "%s: %llu KiB\n", _("Total"),
(memory/1024));
+    for (i = 0; i < nodes_cnt; i++) {
+        unsigned long id;
+        g_autofree char *val = virXMLPropString(nodes[i], "id");
+        if (virStrToLong_ulp(val, NULL, 10, &id)) {
+            vshError(ctl, "%s", _("conversion from string failed"));
+            return false;
+        }
+        nodes_id[i] = id;
+        if (virNodeGetCellsFreeMemory(priv->conn, &(nodes_free[i]),
+                                      id, 1) != 1) {
+            vshError(ctl, _("failed to get free memory for NUMA node "
+                            "number: %lu"), id);
+            return false;
         }
     }
 
+    for (cell = 0; cell < nodes_cnt; cell++) {
+        vshPrint(ctl, "%5lu: %10llu KiB\n", nodes_id[cell],
+                (nodes_free[cell]/1024));
+        memory += nodes_free[cell];
+    }
+
+    vshPrintExtra(ctl, "--------------------\n");
+    vshPrintExtra(ctl, "%5s: %10llu KiB\n", _("Total"),
memory/1024);
+
     return true;
 }
 
@@ -804,30 +804,30 @@ cmdNodeCpuStats(vshControl *ctl, const vshCmd *cmd)
                          cpu_stats[i]);
             }
         }
+        return true;
+    }
+
+    if (present[VIRSH_CPU_USAGE]) {
+        vshPrint(ctl, "%-15s %5.1llu%%\n",
+                 _("usage:"), cpu_stats[VIRSH_CPU_USAGE]);
+        vshPrint(ctl, "%-15s %5.1llu%%\n",
+                 _("idle:"), 100 - cpu_stats[VIRSH_CPU_USAGE]);
     } else {
-        if (present[VIRSH_CPU_USAGE]) {
-            vshPrint(ctl, "%-15s %5.1llu%%\n",
-                     _("usage:"), cpu_stats[VIRSH_CPU_USAGE]);
-            vshPrint(ctl, "%-15s %5.1llu%%\n",
-                     _("idle:"), 100 - cpu_stats[VIRSH_CPU_USAGE]);
-        } else {
-            double usage, total_time = 0;
-            for (i = 0; i < VIRSH_CPU_USAGE; i++)
-                total_time += cpu_stats[i];
-
-            usage = (cpu_stats[VIRSH_CPU_USER] + cpu_stats[VIRSH_CPU_SYSTEM])
-                / total_time * 100;
-
-            vshPrint(ctl, "%-15s %5.1lf%%\n", _("usage:"), usage);
-            for (i = 0; i < VIRSH_CPU_USAGE; i++) {
-                if (present[i]) {
-                    vshPrint(ctl, "%-15s %5.1lf%%\n", _(virshCPUOutput[i]),
-                             cpu_stats[i] / total_time * 100);
-                }
+        double usage, total_time = 0;
+        for (i = 0; i < VIRSH_CPU_USAGE; i++)
+            total_time += cpu_stats[i];
+
+        usage = (cpu_stats[VIRSH_CPU_USER] + cpu_stats[VIRSH_CPU_SYSTEM])
+            / total_time * 100;
+
+        vshPrint(ctl, "%-15s %5.1lf%%\n", _("usage:"), usage);
+        for (i = 0; i < VIRSH_CPU_USAGE; i++) {
+            if (present[i]) {
+                vshPrint(ctl, "%-15s %5.1lf%%\n", _(virshCPUOutput[i]),
+                         cpu_stats[i] / total_time * 100);
             }
         }
     }
-
     return true;
 }
 
diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c
index 152f5b0dbe..ef437413df 100644
--- a/tools/virsh-volume.c
+++ b/tools/virsh-volume.c
@@ -1057,7 +1057,6 @@ cmdVolInfo(vshControl *ctl, const vshCmd *cmd)
     virStorageVolPtr vol;
     bool bytes = vshCommandOptBool(cmd, "bytes");
     bool physical = vshCommandOptBool(cmd, "physical");
-    bool ret = true;
     int rc;
     unsigned int flags = 0;
 
@@ -1074,41 +1073,39 @@ cmdVolInfo(vshControl *ctl, const vshCmd *cmd)
     else
         rc = virStorageVolGetInfo(vol, &info);
 
-    if (rc == 0) {
-        double val;
-        const char *unit;
+    if (rc < 0) {
+        virStorageVolFree(vol);
+        return false;
+    }
 
-        vshPrint(ctl, "%-15s %s\n", _("Type:"),
-                 virshVolumeTypeToString(info.type));
+    vshPrint(ctl, "%-15s %s\n", _("Type:"),
+             virshVolumeTypeToString(info.type));
 
-        if (bytes) {
-            vshPrint(ctl, "%-15s %llu %s\n", _("Capacity:"),
-                     info.capacity, _("bytes"));
-        } else {
-            val = vshPrettyCapacity(info.capacity, &unit);
-            vshPrint(ctl, "%-15s %2.2lf %s\n", _("Capacity:"), val,
unit);
-        }
+    if (bytes) {
+        vshPrint(ctl, "%-15s %llu %s\n", _("Capacity:"),
+                 info.capacity, _("bytes"));
 
-        if (bytes) {
-            if (physical)
-                vshPrint(ctl, "%-15s %llu %s\n", _("Physical:"),
-                         info.allocation, _("bytes"));
-            else
-                vshPrint(ctl, "%-15s %llu %s\n", _("Allocation:"),
-                         info.allocation, _("bytes"));
-         } else {
-            val = vshPrettyCapacity(info.allocation, &unit);
-            if (physical)
-                vshPrint(ctl, "%-15s %2.2lf %s\n", _("Physical:"),
val, unit);
-            else
-                vshPrint(ctl, "%-15s %2.2lf %s\n", _("Allocation:"),
val, unit);
-         }
+        if (physical)
+            vshPrint(ctl, "%-15s %llu %s\n", _("Physical:"),
+                     info.allocation, _("bytes"));
+        else
+            vshPrint(ctl, "%-15s %llu %s\n", _("Allocation:"),
+                     info.allocation, _("bytes"));
     } else {
-        ret = false;
+        const char *unit;
+        double val = vshPrettyCapacity(info.capacity, &unit);
+
+        vshPrint(ctl, "%-15s %2.2lf %s\n", _("Capacity:"), val,
unit);
+
+        val = vshPrettyCapacity(info.allocation, &unit);
+        if (physical)
+            vshPrint(ctl, "%-15s %2.2lf %s\n", _("Physical:"), val,
unit);
+        else
+            vshPrint(ctl, "%-15s %2.2lf %s\n", _("Allocation:"), val,
unit);
     }
 
     virStorageVolFree(vol);
-    return ret;
+    return true;
 }
 
 /*
@@ -1203,7 +1200,6 @@ cmdVolResize(vshControl *ctl, const vshCmd *cmd)
                  delta ? _("Failed to change size of volume '%s' by
%s")
                  : _("Failed to change size of volume '%s' to %s"),
                  virStorageVolGetName(vol), capacityStr);
-        ret = false;
     }
 
  cleanup:
-- 
2.31.1


    

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

2007

2006

2005

[PATCH 4/5] virsh: domain, host & volume: refactor bigger functions