6:25 p.m.
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
6:25 p.m.
New subject: [PATCH v2 1/6] virsh: domain: refactor cmdSchedinfo()
Signed-off-by: Kristina Hanicova <khanicov(a)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
2:53 a.m.
New subject: [PATCH v2 1/6] virsh: domain: refactor cmdSchedinfo()
On 9/24/21 1:25 AM, Kristina Hanicova wrote:
Signed-off-by: Kristina Hanicova <khanicov(a)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
5:28 a.m.
New subject: [PATCH v2 1/6] virsh: domain: refactor cmdSchedinfo()
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(a)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
6:25 p.m.
New subject: [PATCH v2 2/6] virsh: domain: refactor virshCPUCountCollect()
Signed-off-by: Kristina Hanicova <khanicov(a)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
6:25 p.m.
New subject: [PATCH v2 3/6] virsh: domain: refactor cmdLxcEnterNamespace()
Signed-off-by: Kristina Hanicova <khanicov(a)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
6:25 p.m.
New subject: [PATCH v2 4/6] virsh: host: refactor cmdFreecell()
Signed-off-by: Kristina Hanicova <khanicov(a)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
6:25 p.m.
New subject: [PATCH v2 5/6] virsh: host: refactor cmdNodeCpuStats()
Signed-off-by: Kristina Hanicova <khanicov(a)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
6:25 p.m.
New subject: [PATCH v2 6/6] virsh: volume: refactor cmdVolInfo()
Signed-off-by: Kristina Hanicova <khanicov(a)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
2:53 a.m.
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(a)redhat.com>
I've done the change suggested in 1/6 and pushed.
Michal
1155
days inactive
1156
days old
9 comments
3 participants
participants (3)
-
Ján Tomko -
Kristina Hanicova -
Michal Prívozník