[PATCH 0/2] virsh: Simplify vshTableRowAppend() calling in cmdList()
*** BLURB HERE *** Michal Prívozník (2): virsh: Simplify vshTableRowAppend() calling in cmdList(), part one virsh: Simplify vshTableRowAppend() calling in cmdList(), part two tools/virsh-domain-monitor.c | 52 +++++++++++++++--------------------- 1 file changed, 21 insertions(+), 31 deletions(-) -- 2.44.2
All calls to vshTableRowAppend() inside of cmdList() share couple of same arguments: domain ID, domain name and domain state. While the first one is stored in a variable and then passed to all vshTableRowAppend() calls, the others are passed as a function call. Switch the latter to variables too. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh-domain-monitor.c | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 37184baa69..80b6857fad 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -1889,6 +1889,9 @@ cmdList(vshControl *ctl, const vshCmd *cmd) ignore_value(virStrcpyStatic(id_buf, "-")); if (optTable) { + const char *domName = virDomainGetName(dom); + const char *stateStr = NULL; + state = virshDomainState(ctl, dom, NULL); /* Domain could've been removed in the meantime */ @@ -1896,8 +1899,11 @@ cmdList(vshControl *ctl, const vshCmd *cmd) continue; if (managed && state == VIR_DOMAIN_SHUTOFF && - virDomainHasManagedSaveImage(dom, 0) > 0) - state = -2; + virDomainHasManagedSaveImage(dom, 0) > 0) { + stateStr = _("saved"); + } else { + stateStr = virshDomainStateToString(state); + } if (optTitle && !optUUID) { g_autofree char *title = NULL; @@ -1905,9 +1911,7 @@ cmdList(vshControl *ctl, const vshCmd *cmd) if (!(title = virshGetDomainDescription(ctl, dom, true, 0))) goto cleanup; if (vshTableRowAppend(table, id_buf, - virDomainGetName(dom), - state == -2 ? _("saved") - : virshDomainStateToString(state), + domName, stateStr, title, NULL) < 0) goto cleanup; } else if (optUUID && !optTitle) { @@ -1916,9 +1920,7 @@ cmdList(vshControl *ctl, const vshCmd *cmd) goto cleanup; } if (vshTableRowAppend(table, id_buf, - virDomainGetName(dom), - state == -2 ? _("saved") - : virshDomainStateToString(state), + domName, stateStr, uuid, NULL) < 0) goto cleanup; } else if (optUUID && optTitle) { @@ -1931,16 +1933,12 @@ cmdList(vshControl *ctl, const vshCmd *cmd) goto cleanup; } if (vshTableRowAppend(table, id_buf, - virDomainGetName(dom), - state == -2 ? _("saved") - : virshDomainStateToString(state), + domName, stateStr, title, uuid, NULL) < 0) goto cleanup; } else { if (vshTableRowAppend(table, id_buf, - virDomainGetName(dom), - state == -2 ? _("saved") - : virshDomainStateToString(state), + domName, stateStr, NULL) < 0) goto cleanup; } -- 2.44.2
On Mon, Aug 19, 2024 at 16:38:27 +0200, Michal Privoznik wrote:
All calls to vshTableRowAppend() inside of cmdList() share couple of same arguments: domain ID, domain name and domain state. While the first one is stored in a variable and then passed to all vshTableRowAppend() calls, the others are passed as a function call. Switch the latter to variables too.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh-domain-monitor.c | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Instead of having many if-else statements, each with its own vshTableRowAppend() call, we can use a simple trick - have an array of string pointers, set array members in the if bodies and then call vshTableRowAppend() once. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh-domain-monitor.c | 34 +++++++++++++--------------------- 1 file changed, 13 insertions(+), 21 deletions(-) diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 80b6857fad..87efd86a69 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -1891,6 +1891,8 @@ cmdList(vshControl *ctl, const vshCmd *cmd) if (optTable) { const char *domName = virDomainGetName(dom); const char *stateStr = NULL; + g_autofree char *title = NULL; + const char *arg[2] = {}; state = virshDomainState(ctl, dom, NULL); @@ -1906,43 +1908,33 @@ cmdList(vshControl *ctl, const vshCmd *cmd) } if (optTitle && !optUUID) { - g_autofree char *title = NULL; - if (!(title = virshGetDomainDescription(ctl, dom, true, 0))) goto cleanup; - if (vshTableRowAppend(table, id_buf, - domName, stateStr, - title, NULL) < 0) - goto cleanup; + + arg[0] = title; } else if (optUUID && !optTitle) { if (virDomainGetUUIDString(dom, uuid) < 0) { vshError(ctl, "%s", _("Failed to get domain's UUID")); goto cleanup; } - if (vshTableRowAppend(table, id_buf, - domName, stateStr, - uuid, NULL) < 0) - goto cleanup; + + arg[0] = uuid; } else if (optUUID && optTitle) { - g_autofree char *title = NULL; - if (!(title = virshGetDomainDescription(ctl, dom, true, 0))) goto cleanup; if (virDomainGetUUIDString(dom, uuid) < 0) { vshError(ctl, "%s", _("Failed to get domain's UUID")); goto cleanup; } - if (vshTableRowAppend(table, id_buf, - domName, stateStr, - title, uuid, NULL) < 0) - goto cleanup; - } else { - if (vshTableRowAppend(table, id_buf, - domName, stateStr, - NULL) < 0) - goto cleanup; + + arg[0] = title; + arg[1] = uuid; } + if (vshTableRowAppend(table, id_buf, + domName, stateStr, + arg[0], arg[1], NULL) < 0) + goto cleanup; } else { if (optUUID) { if (virDomainGetUUIDString(dom, uuid) < 0) { -- 2.44.2
On Mon, Aug 19, 2024 at 16:38:28 +0200, Michal Privoznik wrote:
Instead of having many if-else statements, each with its own vshTableRowAppend() call, we can use a simple trick - have an array of string pointers, set array members in the if bodies and then call vshTableRowAppend() once.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh-domain-monitor.c | 34 +++++++++++++--------------------- 1 file changed, 13 insertions(+), 21 deletions(-)
[...]
+ if (vshTableRowAppend(table, id_buf, + domName, stateStr, + arg[0], arg[1], NULL) < 0) + goto cleanup;
While this works for this case when the arguments are at the end it will not work in others. When I've recently looked at that patch that motivated you to do this I thought it would be cool if vshTable would have a visibility flag for each column. That way you could always fill all of them and don't have to deal with such scenarios.
} else { if (optUUID) { if (virDomainGetUUIDString(dom, uuid) < 0) {
anyways ... Reviewed-by: Peter Krempa <pkrempa@redhat.com>
participants (2)
-
Michal Privoznik -
Peter Krempa