[PATCH v2 0/6] virsh: refactor some bigger functions
This is v2 of patch: https://listman.redhat.com/archives/libvir-list/2021-September/msg00730.html Diff to v1: * split the previous patch into more smaller ones to make the code review easier * small code quality alternations I did not notice were possible before This series 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 files. Kristina Hanicova (6): virsh: domain: refactor cmdSchedinfo() virsh: domain: refactor virshCPUCountCollect() virsh: domain: refactor cmdLxcEnterNamespace() virsh: host: refactor cmdFreecell() virsh: host: refactor cmdNodeCpuStats() virsh: volume: refactor cmdVolInfo() tools/virsh-domain.c | 210 ++++++++++++++++++++----------------------- tools/virsh-host.c | 139 ++++++++++++++-------------- tools/virsh-volume.c | 52 +++++------ 3 files changed, 189 insertions(+), 212 deletions(-) -- 2.31.1
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- tools/virsh-domain.c | 99 +++++++++++++++++++++----------------------- 1 file changed, 47 insertions(+), 52 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index f876f30cc5..b64df640ba 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -5157,7 +5157,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"); @@ -5176,73 +5175,69 @@ cmdSchedinfo(vshControl *ctl, const vshCmd *cmd) return false; /* Print SchedulerType */ - schedulertype = virDomainGetSchedulerType(dom, &nparams); - if (schedulertype != NULL) { - vshPrint(ctl, "%-15s: %s\n", _("Scheduler"), schedulertype); - VIR_FREE(schedulertype); - } else { + if (!(schedulertype = virDomainGetSchedulerType(dom, &nparams))) { vshPrint(ctl, "%-15s: %s\n", _("Scheduler"), _("Unknown")); goto cleanup; } - if (nparams) { - params = g_new0(virTypedParameter, nparams); + vshPrint(ctl, "%-15s: %s\n", _("Scheduler"), schedulertype); + VIR_FREE(schedulertype); - 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) - goto cleanup; + if (!nparams) + goto cleanup; - /* See if any params are being set */ - if ((nupdates = cmdSchedInfoUpdate(ctl, cmd, params, nparams, - &updates)) < 0) + 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; + } 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, ¶ms[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, ¶ms[i]); + vshPrint(ctl, "%-15s: %s\n", params[i].field, str); + VIR_FREE(str); } cleanup: -- 2.31.1
On 9/24/21 1:25 AM, Kristina Hanicova wrote:
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- tools/virsh-domain.c | 99 +++++++++++++++++++++----------------------- 1 file changed, 47 insertions(+), 52 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index f876f30cc5..b64df640ba 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -5157,7 +5157,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"); @@ -5176,73 +5175,69 @@ cmdSchedinfo(vshControl *ctl, const vshCmd *cmd) return false;
/* Print SchedulerType */ - schedulertype = virDomainGetSchedulerType(dom, &nparams); - if (schedulertype != NULL) { - vshPrint(ctl, "%-15s: %s\n", _("Scheduler"), schedulertype); - VIR_FREE(schedulertype); - } else { + if (!(schedulertype = virDomainGetSchedulerType(dom, &nparams))) { vshPrint(ctl, "%-15s: %s\n", _("Scheduler"), _("Unknown")); goto cleanup; }
- if (nparams) { - params = g_new0(virTypedParameter, nparams); + vshPrint(ctl, "%-15s: %s\n", _("Scheduler"), schedulertype); + VIR_FREE(schedulertype);
I think instead of adding this VIR_FREE() you can just mark the variable as g_autofree. It's a negligible change compared how much lines are being changed.
- 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) - goto cleanup; + if (!nparams) + goto cleanup;
- /* See if any params are being set */ - if ((nupdates = cmdSchedInfoUpdate(ctl, cmd, params, nparams, - &updates)) < 0) + 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; + } 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, ¶ms[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, ¶ms[i]); + vshPrint(ctl, "%-15s: %s\n", params[i].field, str); + VIR_FREE(str);
Same here. Michal
On a Friday in 2021, Michal Prívozník wrote:
On 9/24/21 1:25 AM, Kristina Hanicova wrote:
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- tools/virsh-domain.c | 99 +++++++++++++++++++++----------------------- 1 file changed, 47 insertions(+), 52 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index f876f30cc5..b64df640ba 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -5157,7 +5157,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"); @@ -5176,73 +5175,69 @@ cmdSchedinfo(vshControl *ctl, const vshCmd *cmd) return false;
/* Print SchedulerType */ - schedulertype = virDomainGetSchedulerType(dom, &nparams); - if (schedulertype != NULL) { - vshPrint(ctl, "%-15s: %s\n", _("Scheduler"), schedulertype); - VIR_FREE(schedulertype); - } else { + if (!(schedulertype = virDomainGetSchedulerType(dom, &nparams))) { vshPrint(ctl, "%-15s: %s\n", _("Scheduler"), _("Unknown")); goto cleanup; }
- if (nparams) { - params = g_new0(virTypedParameter, nparams); + vshPrint(ctl, "%-15s: %s\n", _("Scheduler"), schedulertype); + VIR_FREE(schedulertype);
I think instead of adding this VIR_FREE() you can just mark the variable as g_autofree. It's a negligible change compared how much lines are being changed.
Which sounds like a good reason not to mix it with the code movement. Jano
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- tools/virsh-domain.c | 49 ++++++++++++++++++++------------------------ 1 file changed, 22 insertions(+), 27 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index b64df640ba..3dc5fb046e 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6533,7 +6533,6 @@ virshCPUCountCollect(vshControl *ctl, unsigned int flags, bool checkState) { - int ret = -2; virDomainInfo info; int count; g_autoptr(xmlDoc) xml = NULL; @@ -6554,11 +6553,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)) && @@ -6568,36 +6567,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 -- 2.31.1
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- tools/virsh-domain.c | 62 ++++++++++++++++++++------------------------ 1 file changed, 28 insertions(+), 34 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 3dc5fb046e..6f2249ddde 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -9858,6 +9858,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; @@ -9896,40 +9897,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); @@ -9937,8 +9906,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; } -- 2.31.1
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- tools/virsh-host.c | 99 +++++++++++++++++++++++----------------------- 1 file changed, 49 insertions(+), 50 deletions(-) diff --git a/tools/virsh-host.c b/tools/virsh-host.c index e6ed4a26ce..be9cbc2096 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,68 @@ 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; } -- 2.31.1
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- tools/virsh-host.c | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/tools/virsh-host.c b/tools/virsh-host.c index be9cbc2096..bcd44cefe1 100644 --- a/tools/virsh-host.c +++ b/tools/virsh-host.c @@ -803,30 +803,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; } -- 2.31.1
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- tools/virsh-volume.c | 52 ++++++++++++++++++++------------------------ 1 file changed, 23 insertions(+), 29 deletions(-) diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c index 152f5b0dbe..38bb62a48f 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,36 @@ 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; } /* -- 2.31.1
On 9/24/21 1:25 AM, Kristina Hanicova wrote:
This is v2 of patch: https://listman.redhat.com/archives/libvir-list/2021-September/msg00730.html
Diff to v1: * split the previous patch into more smaller ones to make the code review easier * small code quality alternations I did not notice were possible before
This series 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 files.
Kristina Hanicova (6): virsh: domain: refactor cmdSchedinfo() virsh: domain: refactor virshCPUCountCollect() virsh: domain: refactor cmdLxcEnterNamespace() virsh: host: refactor cmdFreecell() virsh: host: refactor cmdNodeCpuStats() virsh: volume: refactor cmdVolInfo()
tools/virsh-domain.c | 210 ++++++++++++++++++++----------------------- tools/virsh-host.c | 139 ++++++++++++++-------------- tools/virsh-volume.c | 52 +++++------ 3 files changed, 189 insertions(+), 212 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> I've done the change suggested in 1/6 and pushed. Michal
participants (3)
-
Ján Tomko -
Kristina Hanicova -
Michal Prívozník