[libvirt] [PATCH 00/13] Implement vsh-table API to virsh and virt-admin
API is implemented in virsh and virt-admin. It fixes problems with table-alignment, makes tables more readable and deals with unicode. Table however could not be implemented in places, which do use simple lists, or table implementation would change content of output, which users might rely on. If you know about other place where it could be implemented, let me know. https://bugzilla.redhat.com/show_bug.cgi?id=1574624 https://bugzilla.redhat.com/show_bug.cgi?id=1584630 Simon Kobyda (13): virsh: Implement vsh-table to iface-list virsh: Implement vshTable API to net-list and net-dhcp-leases virsh: Implement vshTable API to secret-list virsh: Implement vshTable API to nwfilter-list and nwfilterbinding-list virsh: Implement vshTable API to snapshot-list. virsh: Set up cmdDomblkinfo() and cmdDomblkinfoPrint() for vshTable API implementation virsh: Implement vshTable API to domblkinfo virsh: Implement vshTable API to domblklist virsh: Implement vshTable API to domiflist virsh: Implement vshTable API to vcpupin, iothreadinfo, domfsinfo virsh: Implement vshTable API to pool-list virsh: Implement vshTable API to vol-list virt-admin: Implement vshTable API to server-list and client-list tools/virsh-domain-monitor.c | 131 +++++++++++++++------------- tools/virsh-domain.c | 95 +++++++++++++++------ tools/virsh-interface.c | 27 ++++-- tools/virsh-network.c | 55 ++++++++---- tools/virsh-nwfilter.c | 47 +++++++--- tools/virsh-pool.c | 160 +++++++---------------------------- tools/virsh-secret.c | 30 +++++-- tools/virsh-snapshot.c | 33 +++++--- tools/virsh-volume.c | 127 ++++++--------------------- tools/virt-admin.c | 46 +++++++--- 10 files changed, 367 insertions(+), 384 deletions(-) -- 2.17.1
Signed-off-by: Simon Kobyda <skobyda@redhat.com> --- tools/virsh-interface.c | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/tools/virsh-interface.c b/tools/virsh-interface.c index 50518c667b..3234845596 100644 --- a/tools/virsh-interface.c +++ b/tools/virsh-interface.c @@ -48,6 +48,7 @@ #include "virutil.h" #include "virxml.h" #include "virstring.h" +#include "vsh-table.h" virInterfacePtr virshCommandOptInterfaceBy(vshControl *ctl, const vshCmd *cmd, @@ -356,6 +357,8 @@ cmdInterfaceList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) unsigned int flags = VIR_CONNECT_LIST_INTERFACES_ACTIVE; virshInterfaceListPtr list = NULL; size_t i; + bool ret = false; + vshTablePtr table = NULL; if (inactive) flags = VIR_CONNECT_LIST_INTERFACES_INACTIVE; @@ -366,21 +369,29 @@ cmdInterfaceList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) if (!(list = virshInterfaceListCollect(ctl, flags))) return false; - vshPrintExtra(ctl, " %-20s %-10s %s\n", _("Name"), _("State"), - _("MAC Address")); - vshPrintExtra(ctl, "---------------------------------------------------\n"); + table = vshTableNew("Name", "State", "MAC Address", NULL); + if (!table) + goto cleanup; for (i = 0; i < list->nifaces; i++) { virInterfacePtr iface = list->ifaces[i]; - vshPrint(ctl, " %-20s %-10s %s\n", - virInterfaceGetName(iface), - virInterfaceIsActive(iface) ? _("active") : _("inactive"), - virInterfaceGetMACString(iface)); + if (vshTableRowAppend(table, + virInterfaceGetName(iface), + virInterfaceIsActive(iface) ? _("active") + : _("inactive"), + virInterfaceGetMACString(iface), + NULL) < 0) + goto cleanup; } + vshTablePrintToStdout(table, ctl); + + ret = true; + cleanup: + vshTableFree(table); virshInterfaceListFree(list); - return true; + return ret; } /* -- 2.17.1
On 09/18/2018 04:21 PM, Simon Kobyda wrote:
Signed-off-by: Simon Kobyda <skobyda@redhat.com> --- tools/virsh-interface.c | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-)
diff --git a/tools/virsh-interface.c b/tools/virsh-interface.c index 50518c667b..3234845596 100644 --- a/tools/virsh-interface.c +++ b/tools/virsh-interface.c @@ -48,6 +48,7 @@ #include "virutil.h" #include "virxml.h" #include "virstring.h" +#include "vsh-table.h"
virInterfacePtr virshCommandOptInterfaceBy(vshControl *ctl, const vshCmd *cmd, @@ -356,6 +357,8 @@ cmdInterfaceList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) unsigned int flags = VIR_CONNECT_LIST_INTERFACES_ACTIVE; virshInterfaceListPtr list = NULL; size_t i; + bool ret = false; + vshTablePtr table = NULL;
if (inactive) flags = VIR_CONNECT_LIST_INTERFACES_INACTIVE; @@ -366,21 +369,29 @@ cmdInterfaceList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) if (!(list = virshInterfaceListCollect(ctl, flags))) return false;
- vshPrintExtra(ctl, " %-20s %-10s %s\n", _("Name"), _("State"), - _("MAC Address")); - vshPrintExtra(ctl, "---------------------------------------------------\n"); + table = vshTableNew("Name", "State", "MAC Address", NULL);
This is not right. You need to keep the gettext function. If this was merged as is then no translation would be done. You can find more info in the commit I've just pushed: https://www.redhat.com/archives/libvir-list/2018-September/msg00886.html This applies to the rest of the patches too.
+ if (!table) + goto cleanup;
for (i = 0; i < list->nifaces; i++) { virInterfacePtr iface = list->ifaces[i];
- vshPrint(ctl, " %-20s %-10s %s\n", - virInterfaceGetName(iface), - virInterfaceIsActive(iface) ? _("active") : _("inactive"), - virInterfaceGetMACString(iface)); + if (vshTableRowAppend(table, + virInterfaceGetName(iface), + virInterfaceIsActive(iface) ? _("active") + : _("inactive"), + virInterfaceGetMACString(iface), + NULL) < 0) + goto cleanup; }
+ vshTablePrintToStdout(table, ctl); + + ret = true; + cleanup: + vshTableFree(table); virshInterfaceListFree(list); - return true; + return ret; }
/*
ACK with that fixed. Michal
Signed-off-by: Simon Kobyda <skobyda@redhat.com> --- tools/virsh-network.c | 55 +++++++++++++++++++++++++++++-------------- 1 file changed, 37 insertions(+), 18 deletions(-) diff --git a/tools/virsh-network.c b/tools/virsh-network.c index ca07fb568f..0f975b8899 100644 --- a/tools/virsh-network.c +++ b/tools/virsh-network.c @@ -33,6 +33,7 @@ #include "virstring.h" #include "virtime.h" #include "conf/network_conf.h" +#include "vsh-table.h" #define VIRSH_COMMON_OPT_NETWORK(_helpstr, cflags) \ {.name = "network", \ @@ -677,6 +678,7 @@ cmdNetworkList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) bool optUUID = vshCommandOptBool(cmd, "uuid"); char uuid[VIR_UUID_STRING_BUFLEN]; unsigned int flags = VIR_CONNECT_LIST_NETWORKS_ACTIVE; + vshTablePtr table = NULL; if (vshCommandOptBool(cmd, "inactive")) flags = VIR_CONNECT_LIST_NETWORKS_INACTIVE; @@ -705,10 +707,10 @@ cmdNetworkList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) return false; if (optTable) { - vshPrintExtra(ctl, " %-20s %-10s %-13s %s\n", _("Name"), _("State"), - _("Autostart"), _("Persistent")); - vshPrintExtra(ctl, - "----------------------------------------------------------\n"); + table = vshTableNew("Name", "State", "Autostart", + "Persistent", NULL); + if (!table) + goto cleanup; } for (i = 0; i < list->nnets; i++) { @@ -722,11 +724,15 @@ cmdNetworkList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) else autostartStr = is_autostart ? _("yes") : _("no"); - vshPrint(ctl, " %-20s %-10s %-13s %s\n", - virNetworkGetName(network), - virNetworkIsActive(network) ? _("active") : _("inactive"), - autostartStr, - virNetworkIsPersistent(network) ? _("yes") : _("no")); + if (vshTableRowAppend(table, + virNetworkGetName(network), + virNetworkIsActive(network) ? + _("active") : _("inactive"), + autostartStr, + virNetworkIsPersistent(network) ? + _("yes") : _("no"), + NULL) < 0) + goto cleanup; } else if (optUUID) { if (virNetworkGetUUIDString(network, uuid) < 0) { vshError(ctl, "%s", _("Failed to get network's UUID")); @@ -738,8 +744,12 @@ cmdNetworkList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) } } + if (optTable) + vshTablePrintToStdout(table, ctl); + ret = true; cleanup: + vshTableFree(table); virshNetworkListFree(list); return ret; } @@ -1351,6 +1361,7 @@ cmdNetworkDHCPLeases(vshControl *ctl, const vshCmd *cmd) size_t i; unsigned int flags = 0; virNetworkPtr network = NULL; + vshTablePtr table = NULL; if (vshCommandOptStringReq(ctl, cmd, "mac", &mac) < 0) return false; @@ -1366,11 +1377,11 @@ cmdNetworkDHCPLeases(vshControl *ctl, const vshCmd *cmd) /* Sort the list according to MAC Address/IAID */ qsort(leases, nleases, sizeof(*leases), virshNetworkDHCPLeaseSorter); - vshPrintExtra(ctl, " %-20s %-18s %-9s %-25s %-15s %s\n%s%s\n", - _("Expiry Time"), _("MAC address"), _("Protocol"), - _("IP address"), _("Hostname"), _("Client ID or DUID"), - "----------------------------------------------------------", - "---------------------------------------------------------"); + table = vshTableNew("Expiry Time", "MAC address", "Protocol", + "IP address", "Hostname", "Client ID or DUID", + NULL); + if (!table) + goto cleanup; for (i = 0; i < nleases; i++) { const char *typestr = NULL; @@ -1390,17 +1401,25 @@ cmdNetworkDHCPLeases(vshControl *ctl, const vshCmd *cmd) ignore_value(virAsprintf(&cidr_format, "%s/%d", lease->ipaddr, lease->prefix)); - vshPrint(ctl, " %-20s %-18s %-9s %-25s %-15s %s\n", - expirytime, EMPTYSTR(lease->mac), - EMPTYSTR(typestr), cidr_format, - EMPTYSTR(lease->hostname), EMPTYSTR(lease->clientid)); + if (vshTableRowAppend(table, + expirytime, + EMPTYSTR(lease->mac), + EMPTYSTR(typestr), + cidr_format, + EMPTYSTR(lease->hostname), + EMPTYSTR(lease->clientid), + NULL) < 0) + goto cleanup; VIR_FREE(cidr_format); } + vshTablePrintToStdout(table, ctl); + ret = true; cleanup: + vshTableFree(table); if (leases) { for (i = 0; i < nleases; i++) virNetworkDHCPLeaseFree(leases[i]); -- 2.17.1
On 09/18/2018 04:21 PM, Simon Kobyda wrote:
Signed-off-by: Simon Kobyda <skobyda@redhat.com> --- tools/virsh-network.c | 55 +++++++++++++++++++++++++++++-------------- 1 file changed, 37 insertions(+), 18 deletions(-)
diff --git a/tools/virsh-network.c b/tools/virsh-network.c index ca07fb568f..0f975b8899 100644 --- a/tools/virsh-network.c +++ b/tools/virsh-network.c @@ -33,6 +33,7 @@ #include "virstring.h" #include "virtime.h" #include "conf/network_conf.h" +#include "vsh-table.h"
#define VIRSH_COMMON_OPT_NETWORK(_helpstr, cflags) \ {.name = "network", \ @@ -677,6 +678,7 @@ cmdNetworkList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) bool optUUID = vshCommandOptBool(cmd, "uuid"); char uuid[VIR_UUID_STRING_BUFLEN]; unsigned int flags = VIR_CONNECT_LIST_NETWORKS_ACTIVE; + vshTablePtr table = NULL;
if (vshCommandOptBool(cmd, "inactive")) flags = VIR_CONNECT_LIST_NETWORKS_INACTIVE; @@ -705,10 +707,10 @@ cmdNetworkList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) return false;
if (optTable) { - vshPrintExtra(ctl, " %-20s %-10s %-13s %s\n", _("Name"), _("State"), - _("Autostart"), _("Persistent")); - vshPrintExtra(ctl, - "----------------------------------------------------------\n"); + table = vshTableNew("Name", "State", "Autostart", + "Persistent", NULL); + if (!table) + goto cleanup; }
for (i = 0; i < list->nnets; i++) { @@ -722,11 +724,15 @@ cmdNetworkList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) else autostartStr = is_autostart ? _("yes") : _("no");
- vshPrint(ctl, " %-20s %-10s %-13s %s\n", - virNetworkGetName(network), - virNetworkIsActive(network) ? _("active") : _("inactive"), - autostartStr, - virNetworkIsPersistent(network) ? _("yes") : _("no")); + if (vshTableRowAppend(table, + virNetworkGetName(network), + virNetworkIsActive(network) ? + _("active") : _("inactive"), + autostartStr, + virNetworkIsPersistent(network) ? + _("yes") : _("no"), + NULL) < 0) + goto cleanup; } else if (optUUID) { if (virNetworkGetUUIDString(network, uuid) < 0) { vshError(ctl, "%s", _("Failed to get network's UUID")); @@ -738,8 +744,12 @@ cmdNetworkList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) } }
+ if (optTable) + vshTablePrintToStdout(table, ctl); + ret = true; cleanup: + vshTableFree(table); virshNetworkListFree(list); return ret; } @@ -1351,6 +1361,7 @@ cmdNetworkDHCPLeases(vshControl *ctl, const vshCmd *cmd) size_t i; unsigned int flags = 0; virNetworkPtr network = NULL; + vshTablePtr table = NULL;
if (vshCommandOptStringReq(ctl, cmd, "mac", &mac) < 0) return false; @@ -1366,11 +1377,11 @@ cmdNetworkDHCPLeases(vshControl *ctl, const vshCmd *cmd) /* Sort the list according to MAC Address/IAID */ qsort(leases, nleases, sizeof(*leases), virshNetworkDHCPLeaseSorter);
- vshPrintExtra(ctl, " %-20s %-18s %-9s %-25s %-15s %s\n%s%s\n", - _("Expiry Time"), _("MAC address"), _("Protocol"), - _("IP address"), _("Hostname"), _("Client ID or DUID"), - "----------------------------------------------------------", - "---------------------------------------------------------"); + table = vshTableNew("Expiry Time", "MAC address", "Protocol", + "IP address", "Hostname", "Client ID or DUID", + NULL); + if (!table) + goto cleanup;
for (i = 0; i < nleases; i++) { const char *typestr = NULL; @@ -1390,17 +1401,25 @@ cmdNetworkDHCPLeases(vshControl *ctl, const vshCmd *cmd) ignore_value(virAsprintf(&cidr_format, "%s/%d", lease->ipaddr, lease->prefix));
- vshPrint(ctl, " %-20s %-18s %-9s %-25s %-15s %s\n", - expirytime, EMPTYSTR(lease->mac), - EMPTYSTR(typestr), cidr_format, - EMPTYSTR(lease->hostname), EMPTYSTR(lease->clientid)); + if (vshTableRowAppend(table, + expirytime, + EMPTYSTR(lease->mac), + EMPTYSTR(typestr), + cidr_format, + EMPTYSTR(lease->hostname), + EMPTYSTR(lease->clientid), + NULL) < 0) + goto cleanup;
So if this fails, the control jumps to cleanup label and leaks @cidr_format. I wonder if we can use VIR_AUTOFREE() here (although be aware that since variable is not going out of scope between iterations it won't be freed at the end of an iteration). Also, since you're touching this already - should @cidr_format be passed to EMPTYSTR() too like everything else? In case virAsprintf() above fails.
VIR_FREE(cidr_format); }
+ vshTablePrintToStdout(table, ctl); + ret = true;
cleanup: + vshTableFree(table); if (leases) { for (i = 0; i < nleases; i++) virNetworkDHCPLeaseFree(leases[i]);
Michal
On Wed, Sep 19, 2018 at 11:26:27AM +0200, Michal Privoznik wrote:
On 09/18/2018 04:21 PM, Simon Kobyda wrote:
Signed-off-by: Simon Kobyda <skobyda@redhat.com> --- tools/virsh-network.c | 55 +++++++++++++++++++++++++++++-------------- 1 file changed, 37 insertions(+), 18 deletions(-)
diff --git a/tools/virsh-network.c b/tools/virsh-network.c index ca07fb568f..0f975b8899 100644 --- a/tools/virsh-network.c +++ b/tools/virsh-network.c @@ -33,6 +33,7 @@ #include "virstring.h" #include "virtime.h" #include "conf/network_conf.h" +#include "vsh-table.h"
#define VIRSH_COMMON_OPT_NETWORK(_helpstr, cflags) \ {.name = "network", \ @@ -677,6 +678,7 @@ cmdNetworkList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) bool optUUID = vshCommandOptBool(cmd, "uuid"); char uuid[VIR_UUID_STRING_BUFLEN]; unsigned int flags = VIR_CONNECT_LIST_NETWORKS_ACTIVE; + vshTablePtr table = NULL;
if (vshCommandOptBool(cmd, "inactive")) flags = VIR_CONNECT_LIST_NETWORKS_INACTIVE; @@ -705,10 +707,10 @@ cmdNetworkList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) return false;
if (optTable) { - vshPrintExtra(ctl, " %-20s %-10s %-13s %s\n", _("Name"), _("State"), - _("Autostart"), _("Persistent")); - vshPrintExtra(ctl, - "----------------------------------------------------------\n"); + table = vshTableNew("Name", "State", "Autostart", + "Persistent", NULL); + if (!table) + goto cleanup; }
for (i = 0; i < list->nnets; i++) { @@ -722,11 +724,15 @@ cmdNetworkList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) else autostartStr = is_autostart ? _("yes") : _("no");
- vshPrint(ctl, " %-20s %-10s %-13s %s\n", - virNetworkGetName(network), - virNetworkIsActive(network) ? _("active") : _("inactive"), - autostartStr, - virNetworkIsPersistent(network) ? _("yes") : _("no")); + if (vshTableRowAppend(table, + virNetworkGetName(network), + virNetworkIsActive(network) ? + _("active") : _("inactive"), + autostartStr, + virNetworkIsPersistent(network) ? + _("yes") : _("no"), + NULL) < 0) + goto cleanup; } else if (optUUID) { if (virNetworkGetUUIDString(network, uuid) < 0) { vshError(ctl, "%s", _("Failed to get network's UUID")); @@ -738,8 +744,12 @@ cmdNetworkList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) } }
+ if (optTable) + vshTablePrintToStdout(table, ctl); + ret = true; cleanup: + vshTableFree(table); virshNetworkListFree(list); return ret; } @@ -1351,6 +1361,7 @@ cmdNetworkDHCPLeases(vshControl *ctl, const vshCmd *cmd) size_t i; unsigned int flags = 0; virNetworkPtr network = NULL; + vshTablePtr table = NULL;
if (vshCommandOptStringReq(ctl, cmd, "mac", &mac) < 0) return false; @@ -1366,11 +1377,11 @@ cmdNetworkDHCPLeases(vshControl *ctl, const vshCmd *cmd) /* Sort the list according to MAC Address/IAID */ qsort(leases, nleases, sizeof(*leases), virshNetworkDHCPLeaseSorter);
- vshPrintExtra(ctl, " %-20s %-18s %-9s %-25s %-15s %s\n%s%s\n", - _("Expiry Time"), _("MAC address"), _("Protocol"), - _("IP address"), _("Hostname"), _("Client ID or DUID"), - "----------------------------------------------------------", - "---------------------------------------------------------"); + table = vshTableNew("Expiry Time", "MAC address", "Protocol", + "IP address", "Hostname", "Client ID or DUID", + NULL); + if (!table) + goto cleanup;
for (i = 0; i < nleases; i++) { const char *typestr = NULL; @@ -1390,17 +1401,25 @@ cmdNetworkDHCPLeases(vshControl *ctl, const vshCmd *cmd) ignore_value(virAsprintf(&cidr_format, "%s/%d", lease->ipaddr, lease->prefix));
- vshPrint(ctl, " %-20s %-18s %-9s %-25s %-15s %s\n", - expirytime, EMPTYSTR(lease->mac), - EMPTYSTR(typestr), cidr_format, - EMPTYSTR(lease->hostname), EMPTYSTR(lease->clientid)); + if (vshTableRowAppend(table, + expirytime, + EMPTYSTR(lease->mac), + EMPTYSTR(typestr), + cidr_format, + EMPTYSTR(lease->hostname), + EMPTYSTR(lease->clientid), + NULL) < 0) + goto cleanup;
So if this fails, the control jumps to cleanup label and leaks @cidr_format. I wonder if we can use VIR_AUTOFREE() here (although be aware that since variable is not going out of scope between iterations it won't be freed at the end of an iteration).
Not true, IMHO scope is defined vaguely for loops, especially 'for', which has control sequence, but in the end, what really happens is that when the loop reaches its end, you need to evaluate the termination condition and jump back to the label or continue with the next instruction, ergo the fact you're evaluating the termination condition means the scope is over, this could be better illustrated with C++ destructors, but let's do that in C: #include <stdio.h> void destructor(int *j) { printf("DESTRUCTOR: j: %d\n", *j); } int main(void) { for (int i = 0; i < 2; i++) { int j __attribute__((__cleanup__(destructor))); printf("PRE_INIT: i=%d, j=%d\n", i,j); j = 1; printf("POST_INIT: i=%d, j=%d\n", i,j); j++; printf("POST_INCREMENT: i=%d, j=%d\n", i,j); } } Don't compile with -Wall, as j being unitialized is intended here. For the sake of the argument, let's also turn off the optimizations. You'll see that the destructor is called every iteration. FWIW, have a look at the assembly that gcc produced as well. Erik
On 09/21/2018 03:35 PM, Erik Skultety wrote:
On Wed, Sep 19, 2018 at 11:26:27AM +0200, Michal Privoznik wrote:
On 09/18/2018 04:21 PM, Simon Kobyda wrote:
Signed-off-by: Simon Kobyda <skobyda@redhat.com> --- tools/virsh-network.c | 55 +++++++++++++++++++++++++++++-------------- 1 file changed, 37 insertions(+), 18 deletions(-)
diff --git a/tools/virsh-network.c b/tools/virsh-network.c index ca07fb568f..0f975b8899 100644 --- a/tools/virsh-network.c +++ b/tools/virsh-network.c @@ -33,6 +33,7 @@ #include "virstring.h" #include "virtime.h" #include "conf/network_conf.h" +#include "vsh-table.h"
#define VIRSH_COMMON_OPT_NETWORK(_helpstr, cflags) \ {.name = "network", \ @@ -677,6 +678,7 @@ cmdNetworkList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) bool optUUID = vshCommandOptBool(cmd, "uuid"); char uuid[VIR_UUID_STRING_BUFLEN]; unsigned int flags = VIR_CONNECT_LIST_NETWORKS_ACTIVE; + vshTablePtr table = NULL;
if (vshCommandOptBool(cmd, "inactive")) flags = VIR_CONNECT_LIST_NETWORKS_INACTIVE; @@ -705,10 +707,10 @@ cmdNetworkList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) return false;
if (optTable) { - vshPrintExtra(ctl, " %-20s %-10s %-13s %s\n", _("Name"), _("State"), - _("Autostart"), _("Persistent")); - vshPrintExtra(ctl, - "----------------------------------------------------------\n"); + table = vshTableNew("Name", "State", "Autostart", + "Persistent", NULL); + if (!table) + goto cleanup; }
for (i = 0; i < list->nnets; i++) { @@ -722,11 +724,15 @@ cmdNetworkList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) else autostartStr = is_autostart ? _("yes") : _("no");
- vshPrint(ctl, " %-20s %-10s %-13s %s\n", - virNetworkGetName(network), - virNetworkIsActive(network) ? _("active") : _("inactive"), - autostartStr, - virNetworkIsPersistent(network) ? _("yes") : _("no")); + if (vshTableRowAppend(table, + virNetworkGetName(network), + virNetworkIsActive(network) ? + _("active") : _("inactive"), + autostartStr, + virNetworkIsPersistent(network) ? + _("yes") : _("no"), + NULL) < 0) + goto cleanup; } else if (optUUID) { if (virNetworkGetUUIDString(network, uuid) < 0) { vshError(ctl, "%s", _("Failed to get network's UUID")); @@ -738,8 +744,12 @@ cmdNetworkList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) } }
+ if (optTable) + vshTablePrintToStdout(table, ctl); + ret = true; cleanup: + vshTableFree(table); virshNetworkListFree(list); return ret; } @@ -1351,6 +1361,7 @@ cmdNetworkDHCPLeases(vshControl *ctl, const vshCmd *cmd) size_t i; unsigned int flags = 0; virNetworkPtr network = NULL; + vshTablePtr table = NULL;
if (vshCommandOptStringReq(ctl, cmd, "mac", &mac) < 0) return false; @@ -1366,11 +1377,11 @@ cmdNetworkDHCPLeases(vshControl *ctl, const vshCmd *cmd) /* Sort the list according to MAC Address/IAID */ qsort(leases, nleases, sizeof(*leases), virshNetworkDHCPLeaseSorter);
- vshPrintExtra(ctl, " %-20s %-18s %-9s %-25s %-15s %s\n%s%s\n", - _("Expiry Time"), _("MAC address"), _("Protocol"), - _("IP address"), _("Hostname"), _("Client ID or DUID"), - "----------------------------------------------------------", - "---------------------------------------------------------"); + table = vshTableNew("Expiry Time", "MAC address", "Protocol", + "IP address", "Hostname", "Client ID or DUID", + NULL); + if (!table) + goto cleanup;
for (i = 0; i < nleases; i++) { const char *typestr = NULL; @@ -1390,17 +1401,25 @@ cmdNetworkDHCPLeases(vshControl *ctl, const vshCmd *cmd) ignore_value(virAsprintf(&cidr_format, "%s/%d", lease->ipaddr, lease->prefix));
- vshPrint(ctl, " %-20s %-18s %-9s %-25s %-15s %s\n", - expirytime, EMPTYSTR(lease->mac), - EMPTYSTR(typestr), cidr_format, - EMPTYSTR(lease->hostname), EMPTYSTR(lease->clientid)); + if (vshTableRowAppend(table, + expirytime, + EMPTYSTR(lease->mac), + EMPTYSTR(typestr), + cidr_format, + EMPTYSTR(lease->hostname), + EMPTYSTR(lease->clientid), + NULL) < 0) + goto cleanup;
So if this fails, the control jumps to cleanup label and leaks @cidr_format. I wonder if we can use VIR_AUTOFREE() here (although be aware that since variable is not going out of scope between iterations it won't be freed at the end of an iteration).
Not true, IMHO scope is defined vaguely for loops, especially 'for', which has control sequence, but in the end, what really happens is that when the loop reaches its end, you need to evaluate the termination condition and jump back to the label or continue with the next instruction, ergo the fact you're evaluating the termination condition means the scope is over, this could be better illustrated with C++ destructors, but let's do that in C:
#include <stdio.h>
void destructor(int *j) { printf("DESTRUCTOR: j: %d\n", *j); }
int main(void) { for (int i = 0; i < 2; i++) { int j __attribute__((__cleanup__(destructor)));
printf("PRE_INIT: i=%d, j=%d\n", i,j); j = 1; printf("POST_INIT: i=%d, j=%d\n", i,j); j++; printf("POST_INCREMENT: i=%d, j=%d\n", i,j); } }
Don't compile with -Wall, as j being unitialized is intended here. For the sake of the argument, let's also turn off the optimizations. You'll see that the destructor is called every iteration. FWIW, have a look at the assembly that gcc produced as well.
Okay, for some reason I thought it was the opposite. Good to know. So we should use VIR_AUTOFREE() then. Michal
Signed-off-by: Simon Kobyda <skobyda@redhat.com> --- tools/virsh-secret.c | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-) diff --git a/tools/virsh-secret.c b/tools/virsh-secret.c index 670beea706..0ae248b4dd 100644 --- a/tools/virsh-secret.c +++ b/tools/virsh-secret.c @@ -35,6 +35,7 @@ #include "virsecret.h" #include "virstring.h" #include "virtime.h" +#include "vsh-table.h" static virSecretPtr virshCommandOptSecret(vshControl *ctl, const vshCmd *cmd, const char **name) @@ -507,6 +508,7 @@ cmdSecretList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) virshSecretListPtr list = NULL; bool ret = false; unsigned int flags = 0; + vshTablePtr table = NULL; if (vshCommandOptBool(cmd, "ephemeral")) flags |= VIR_CONNECT_LIST_SECRETS_EPHEMERAL; @@ -523,15 +525,17 @@ cmdSecretList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) if (!(list = virshSecretListCollect(ctl, flags))) return false; - vshPrintExtra(ctl, " %-36s %s\n", _("UUID"), _("Usage")); - vshPrintExtra(ctl, "----------------------------------------" - "----------------------------------------\n"); + table = vshTableNew("UUID", "Usage", NULL); + if (!table) + goto cleanup; for (i = 0; i < list->nsecrets; i++) { virSecretPtr sec = list->secrets[i]; int usageType = virSecretGetUsageType(sec); const char *usageStr = virSecretUsageTypeToString(usageType); char uuid[VIR_UUID_STRING_BUFLEN]; + virBuffer buf = VIR_BUFFER_INITIALIZER; + const char *usage; if (virSecretGetUUIDString(sec, uuid) < 0) { vshError(ctl, "%s", _("Failed to get uuid of secret")); @@ -539,18 +543,28 @@ cmdSecretList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) } if (usageType) { - vshPrint(ctl, " %-36s %s %s\n", - uuid, usageStr, - virSecretGetUsageID(sec)); + virBufferStrcat(&buf, usageStr, " ", + virSecretGetUsageID(sec), NULL); + usage = virBufferCurrentContent(&buf); + if (!usage) + goto cleanup; + + if (vshTableRowAppend(table, uuid, usage, NULL) < 0) + goto cleanup; + + virBufferFreeAndReset(&buf); } else { - vshPrint(ctl, " %-36s %s\n", - uuid, _("Unused")); + if (vshTableRowAppend(table, uuid, "Unused", NULL) < 0) + goto cleanup; } } + vshTablePrintToStdout(table, ctl); + ret = true; cleanup: + vshTableFree(table); virshSecretListFree(list); return ret; } -- 2.17.1
On 09/18/2018 04:21 PM, Simon Kobyda wrote:
Signed-off-by: Simon Kobyda <skobyda@redhat.com> --- tools/virsh-secret.c | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-)
diff --git a/tools/virsh-secret.c b/tools/virsh-secret.c index 670beea706..0ae248b4dd 100644 --- a/tools/virsh-secret.c +++ b/tools/virsh-secret.c @@ -35,6 +35,7 @@ #include "virsecret.h" #include "virstring.h" #include "virtime.h" +#include "vsh-table.h"
static virSecretPtr virshCommandOptSecret(vshControl *ctl, const vshCmd *cmd, const char **name) @@ -507,6 +508,7 @@ cmdSecretList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) virshSecretListPtr list = NULL; bool ret = false; unsigned int flags = 0; + vshTablePtr table = NULL;
if (vshCommandOptBool(cmd, "ephemeral")) flags |= VIR_CONNECT_LIST_SECRETS_EPHEMERAL; @@ -523,15 +525,17 @@ cmdSecretList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) if (!(list = virshSecretListCollect(ctl, flags))) return false;
- vshPrintExtra(ctl, " %-36s %s\n", _("UUID"), _("Usage")); - vshPrintExtra(ctl, "----------------------------------------" - "----------------------------------------\n"); + table = vshTableNew("UUID", "Usage", NULL); + if (!table) + goto cleanup;
for (i = 0; i < list->nsecrets; i++) { virSecretPtr sec = list->secrets[i]; int usageType = virSecretGetUsageType(sec); const char *usageStr = virSecretUsageTypeToString(usageType); char uuid[VIR_UUID_STRING_BUFLEN]; + virBuffer buf = VIR_BUFFER_INITIALIZER; + const char *usage;
if (virSecretGetUUIDString(sec, uuid) < 0) { vshError(ctl, "%s", _("Failed to get uuid of secret")); @@ -539,18 +543,28 @@ cmdSecretList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) }
if (usageType) { - vshPrint(ctl, " %-36s %s %s\n", - uuid, usageStr, - virSecretGetUsageID(sec)); + virBufferStrcat(&buf, usageStr, " ", + virSecretGetUsageID(sec), NULL); + usage = virBufferCurrentContent(&buf); + if (!usage) + goto cleanup; + + if (vshTableRowAppend(table, uuid, usage, NULL) < 0) + goto cleanup;
So if this fails the buffer is leaked. Looks like switching from virBufferCurrentContent() to virBufferContentAndReset() will prevent this.
+ + virBufferFreeAndReset(&buf); } else { - vshPrint(ctl, " %-36s %s\n", - uuid, _("Unused")); + if (vshTableRowAppend(table, uuid, "Unused", NULL) < 0)
Again, you need to honour the gettext macro (src/internal.h:52).
+ goto cleanup; } }
+ vshTablePrintToStdout(table, ctl); + ret = true;
cleanup: + vshTableFree(table); virshSecretListFree(list); return ret; }
Otherwise looking good. Michal
Signed-off-by: Simon Kobyda <skobyda@redhat.com> --- tools/virsh-nwfilter.c | 47 +++++++++++++++++++++++++++++------------- 1 file changed, 33 insertions(+), 14 deletions(-) diff --git a/tools/virsh-nwfilter.c b/tools/virsh-nwfilter.c index 1cdbe5053a..91e9e78b8b 100644 --- a/tools/virsh-nwfilter.c +++ b/tools/virsh-nwfilter.c @@ -31,6 +31,7 @@ #include "viralloc.h" #include "virfile.h" #include "virutil.h" +#include "vsh-table.h" virNWFilterPtr virshCommandOptNWFilterBy(vshControl *ctl, const vshCmd *cmd, @@ -359,26 +360,35 @@ cmdNWFilterList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) { size_t i; char uuid[VIR_UUID_STRING_BUFLEN]; + bool ret = false; virshNWFilterListPtr list = NULL; + vshTablePtr table = NULL; if (!(list = virshNWFilterListCollect(ctl, 0))) return false; - vshPrintExtra(ctl, " %-36s %-20s \n", _("UUID"), _("Name")); - vshPrintExtra(ctl, "---------------------------------" - "---------------------------------\n"); + table = vshTableNew("UUID", "Name", NULL); + if (!table) + goto cleanup; for (i = 0; i < list->nfilters; i++) { virNWFilterPtr nwfilter = list->filters[i]; virNWFilterGetUUIDString(nwfilter, uuid); - vshPrint(ctl, " %-36s %-20s\n", - uuid, - virNWFilterGetName(nwfilter)); + if (vshTableRowAppend(table, + uuid, + virNWFilterGetName(nwfilter), + NULL) < 0) + goto cleanup; } + vshTablePrintToStdout(table, ctl); + + ret = true; + cleanup: + vshTableFree(table); virshNWFilterListFree(list); - return true; + return ret; } /* @@ -714,25 +724,34 @@ static bool cmdNWFilterBindingList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) { size_t i; + bool ret = false; virshNWFilterBindingListPtr list = NULL; + vshTablePtr table = NULL; if (!(list = virshNWFilterBindingListCollect(ctl, 0))) return false; - vshPrintExtra(ctl, " %-20s %-20s \n", _("Port Dev"), _("Filter")); - vshPrintExtra(ctl, "---------------------------------" - "---------------------------------\n"); + table = vshTableNew("Port Dev", "Filter", NULL); + if (!table) + goto cleanup; for (i = 0; i < list->nbindings; i++) { virNWFilterBindingPtr binding = list->bindings[i]; - vshPrint(ctl, " %-20s %-20s\n", - virNWFilterBindingGetPortDev(binding), - virNWFilterBindingGetFilterName(binding)); + if (vshTableRowAppend(table, + virNWFilterBindingGetPortDev(binding), + virNWFilterBindingGetFilterName(binding), + NULL) < 0) + goto cleanup; } + vshTablePrintToStdout(table, ctl); + + ret = true; + cleanup: + vshTableFree(table); virshNWFilterBindingListFree(list); - return true; + return ret; } -- 2.17.1
Signed-off-by: Simon Kobyda <skobyda@redhat.com> --- tools/virsh-snapshot.c | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index a4ea959230..5a02d2c786 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -41,6 +41,7 @@ #include "virstring.h" #include "virxml.h" #include "conf/snapshot_conf.h" +#include "vsh-table.h" /* Helper for snapshot-create and snapshot-create-as */ static bool @@ -1487,6 +1488,7 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) char *parent_snap = NULL; virDomainSnapshotPtr start = NULL; virshSnapshotListPtr snaplist = NULL; + vshTablePtr table = NULL; VSH_EXCLUSIVE_OPTIONS_VAR(tree, name); VSH_EXCLUSIVE_OPTIONS_VAR(parent, roots); @@ -1547,15 +1549,12 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) if (!tree && !name) { if (parent) - vshPrintExtra(ctl, " %-20s %-25s %-15s %s", - _("Name"), _("Creation Time"), _("State"), - _("Parent")); + table = vshTableNew("Name", "Creation Time", "State", "Parent", NULL); else - vshPrintExtra(ctl, " %-20s %-25s %s", - _("Name"), _("Creation Time"), _("State")); - vshPrintExtra(ctl, "\n" - "------------------------------" - "------------------------------\n"); + table = vshTableNew("Name", "Creation Time", "State", NULL); + + if (!table) + goto cleanup; } if (tree) { @@ -1614,13 +1613,20 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) strftime(timestr, sizeof(timestr), "%Y-%m-%d %H:%M:%S %z", &time_info); - if (parent) - vshPrint(ctl, " %-20s %-25s %-15s %s\n", - snap_name, timestr, state, parent_snap); - else - vshPrint(ctl, " %-20s %-25s %s\n", snap_name, timestr, state); + if (parent) { + if (vshTableRowAppend(table, snap_name, timestr, state, parent_snap, + NULL) < 0) + continue; + } else { + if (vshTableRowAppend(table, snap_name, timestr, state, + NULL) < 0) + continue; + } } + if (!tree && !name) + vshTablePrintToStdout(table, ctl); + ret = true; cleanup: @@ -1633,6 +1639,7 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) xmlFreeDoc(xml); VIR_FREE(doc); virshDomainFree(dom); + vshTableFree(table); return ret; } -- 2.17.1
On 09/18/2018 04:21 PM, Simon Kobyda wrote:
Signed-off-by: Simon Kobyda <skobyda@redhat.com> --- tools/virsh-snapshot.c | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-)
diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index a4ea959230..5a02d2c786 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -41,6 +41,7 @@ #include "virstring.h" #include "virxml.h" #include "conf/snapshot_conf.h" +#include "vsh-table.h"
/* Helper for snapshot-create and snapshot-create-as */ static bool @@ -1487,6 +1488,7 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) char *parent_snap = NULL; virDomainSnapshotPtr start = NULL; virshSnapshotListPtr snaplist = NULL; + vshTablePtr table = NULL;
VSH_EXCLUSIVE_OPTIONS_VAR(tree, name); VSH_EXCLUSIVE_OPTIONS_VAR(parent, roots); @@ -1547,15 +1549,12 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd)
if (!tree && !name) { if (parent) - vshPrintExtra(ctl, " %-20s %-25s %-15s %s", - _("Name"), _("Creation Time"), _("State"), - _("Parent")); + table = vshTableNew("Name", "Creation Time", "State", "Parent", NULL); else - vshPrintExtra(ctl, " %-20s %-25s %s", - _("Name"), _("Creation Time"), _("State")); - vshPrintExtra(ctl, "\n" - "------------------------------" - "------------------------------\n"); + table = vshTableNew("Name", "Creation Time", "State", NULL); + + if (!table) + goto cleanup; }
if (tree) { @@ -1614,13 +1613,20 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) strftime(timestr, sizeof(timestr), "%Y-%m-%d %H:%M:%S %z", &time_info);
- if (parent) - vshPrint(ctl, " %-20s %-25s %-15s %s\n", - snap_name, timestr, state, parent_snap); - else - vshPrint(ctl, " %-20s %-25s %s\n", snap_name, timestr, state); + if (parent) { + if (vshTableRowAppend(table, snap_name, timestr, state, parent_snap, + NULL) < 0) + continue; + } else { + if (vshTableRowAppend(table, snap_name, timestr, state, + NULL) < 0) + continue;
What is the point of these 'continue'? Shouldn't we jump to cleanup instead?
+ } }
+ if (!tree && !name)
Or simply: if (table)
+ vshTablePrintToStdout(table, ctl); + ret = true;
cleanup: @@ -1633,6 +1639,7 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) xmlFreeDoc(xml); VIR_FREE(doc); virshDomainFree(dom); + vshTableFree(table);
return ret; }
This is where I'm going to stop my review. You get the idea what changes you need to do for v2. Michal
I've moved all the printing from cmdDomblkinfoPrint() to cmdDomblkinfo(), and renamed the cmdDomblkinfoPrint() to cmdDomblkinfoGet(), since nature of that function changed from gathering and printing informations only to gathering information. This I believe simplifies the functions and makes the implementation of vshTable API simpler. Signed-off-by: Simon Kobyda <skobyda@redhat.com> --- tools/virsh-domain-monitor.c | 78 +++++++++++++++++------------------- 1 file changed, 36 insertions(+), 42 deletions(-) diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index adc5bb1a7a..cb48f9a7be 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -406,33 +406,23 @@ static const vshCmdOptDef opts_domblkinfo[] = { {.name = NULL} }; -static void -cmdDomblkinfoPrint(vshControl *ctl, +static bool +cmdDomblkinfoGet(vshControl *ctl, const virDomainBlockInfo *info, - const char *device, - bool human, bool title) + char **cap, + char **alloc, + char **phy, + bool human) { - char *cap = NULL; - char *alloc = NULL; - char *phy = NULL; - - if (title) { - vshPrintExtra(ctl, "%-10s %-15s %-15s %-15s\n", _("Target"), - _("Capacity"), _("Allocation"), _("Physical")); - vshPrintExtra(ctl, "-----------------------------" - "------------------------\n"); - return; - } - if (info->capacity == 0 && info->allocation == 0 && info->physical == 0) { - cap = vshStrdup(ctl, "-"); - alloc = vshStrdup(ctl, "-"); - phy = vshStrdup(ctl, "-"); + *cap = vshStrdup(ctl, "-"); + *alloc = vshStrdup(ctl, "-"); + *phy = vshStrdup(ctl, "-"); } else if (!human) { - if (virAsprintf(&cap, "%llu", info->capacity) < 0 || - virAsprintf(&alloc, "%llu", info->allocation) < 0 || - virAsprintf(&phy, "%llu", info->physical) < 0) - goto cleanup; + if (virAsprintf(cap, "%llu", info->capacity) < 0 || + virAsprintf(alloc, "%llu", info->allocation) < 0 || + virAsprintf(phy, "%llu", info->physical) < 0) + return false; } else { double val_cap, val_alloc, val_phy; const char *unit_cap, *unit_alloc, *unit_phy; @@ -441,24 +431,13 @@ cmdDomblkinfoPrint(vshControl *ctl, val_alloc = vshPrettyCapacity(info->allocation, &unit_alloc); val_phy = vshPrettyCapacity(info->physical, &unit_phy); - if (virAsprintf(&cap, "%.3lf %s", val_cap, unit_cap) < 0 || - virAsprintf(&alloc, "%.3lf %s", val_alloc, unit_alloc) < 0 || - virAsprintf(&phy, "%.3lf %s", val_phy, unit_phy) < 0) - goto cleanup; - } - - if (device) { - vshPrint(ctl, "%-10s %-15s %-15s %-15s\n", device, cap, alloc, phy); - } else { - vshPrint(ctl, "%-15s %s\n", _("Capacity:"), cap); - vshPrint(ctl, "%-15s %s\n", _("Allocation:"), alloc); - vshPrint(ctl, "%-15s %s\n", _("Physical:"), phy); + if (virAsprintf(cap, "%.3lf %s", val_cap, unit_cap) < 0 || + virAsprintf(alloc, "%.3lf %s", val_alloc, unit_alloc) < 0 || + virAsprintf(phy, "%.3lf %s", val_phy, unit_phy) < 0) + return false; } - cleanup: - VIR_FREE(cap); - VIR_FREE(alloc); - VIR_FREE(phy); + return true; } @@ -478,6 +457,9 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd) xmlNodePtr *disks = NULL; char *target = NULL; char *protocol = NULL; + char *cap = NULL; + char *alloc = NULL; + char *phy = NULL; if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) return false; @@ -502,7 +484,10 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd) goto cleanup; /* print the title */ - cmdDomblkinfoPrint(ctl, NULL, NULL, false, true); + vshPrintExtra(ctl, "%-10s %-15s %-15s %-15s\n", _("Target"), + _("Capacity"), _("Allocation"), _("Physical")); + vshPrintExtra(ctl, "-----------------------------" + "------------------------\n"); for (i = 0; i < ndisks; i++) { ctxt->node = disks[i]; @@ -525,7 +510,9 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd) } } - cmdDomblkinfoPrint(ctl, &info, target, human, false); + if (!cmdDomblkinfoGet(ctl, &info, &cap, &alloc, &phy, human)) + goto cleanup; + vshPrint(ctl, "%-10s %-15s %-15s %-15s\n", target, cap, alloc, phy); VIR_FREE(target); VIR_FREE(protocol); @@ -534,12 +521,19 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd) if (virDomainGetBlockInfo(dom, device, &info, 0) < 0) goto cleanup; - cmdDomblkinfoPrint(ctl, &info, NULL, human, false); + if (!cmdDomblkinfoGet(ctl, &info, &cap, &alloc, &phy, human)) + goto cleanup; + vshPrint(ctl, "%-15s %s\n", _("Capacity:"), cap); + vshPrint(ctl, "%-15s %s\n", _("Allocation:"), alloc); + vshPrint(ctl, "%-15s %s\n", _("Physical:"), phy); } ret = true; cleanup: + VIR_FREE(cap); + VIR_FREE(alloc); + VIR_FREE(phy); virshDomainFree(dom); VIR_FREE(target); VIR_FREE(protocol); -- 2.17.1
Signed-off-by: Simon Kobyda <skobyda@redhat.com> --- tools/virsh-domain-monitor.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index cb48f9a7be..05db825716 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -460,6 +460,7 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd) char *cap = NULL; char *alloc = NULL; char *phy = NULL; + vshTablePtr table = NULL; if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) return false; @@ -483,11 +484,10 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd) if (ndisks < 0) goto cleanup; - /* print the title */ - vshPrintExtra(ctl, "%-10s %-15s %-15s %-15s\n", _("Target"), - _("Capacity"), _("Allocation"), _("Physical")); - vshPrintExtra(ctl, "-----------------------------" - "------------------------\n"); + /* title */ + table = vshTableNew("Target", "Capacity", "Allocation", "Physical", NULL); + if (!table) + goto cleanup; for (i = 0; i < ndisks; i++) { ctxt->node = disks[i]; @@ -512,11 +512,15 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd) if (!cmdDomblkinfoGet(ctl, &info, &cap, &alloc, &phy, human)) goto cleanup; - vshPrint(ctl, "%-10s %-15s %-15s %-15s\n", target, cap, alloc, phy); + if (vshTableRowAppend(table, target, cap, alloc, phy, NULL) < 0) + goto cleanup; VIR_FREE(target); VIR_FREE(protocol); } + + vshTablePrintToStdout(table, ctl); + } else { if (virDomainGetBlockInfo(dom, device, &info, 0) < 0) goto cleanup; @@ -531,6 +535,7 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd) ret = true; cleanup: + vshTableFree(table); VIR_FREE(cap); VIR_FREE(alloc); VIR_FREE(phy); -- 2.17.1
Signed-off-by: Simon Kobyda <skobyda@redhat.com> --- tools/virsh-domain-monitor.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 05db825716..b9b6739287 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -589,6 +589,7 @@ cmdDomblklist(vshControl *ctl, const vshCmd *cmd) char *device = NULL; char *target = NULL; char *source = NULL; + vshTablePtr table = NULL; if (vshCommandOptBool(cmd, "inactive")) flags |= VIR_DOMAIN_XML_INACTIVE; @@ -603,12 +604,12 @@ cmdDomblklist(vshControl *ctl, const vshCmd *cmd) goto cleanup; if (details) - vshPrintExtra(ctl, "%-10s %-10s %-10s %s\n", _("Type"), - _("Device"), _("Target"), _("Source")); + table = vshTableNew("Type", "Device", "Target", "Source", NULL); else - vshPrintExtra(ctl, "%-10s %s\n", _("Target"), _("Source")); + table = vshTableNew("Target", "Source", NULL); - vshPrintExtra(ctl, "------------------------------------------------\n"); + if (!table) + goto cleanup; for (i = 0; i < ndisks; i++) { ctxt->node = disks[i]; @@ -633,10 +634,13 @@ cmdDomblklist(vshControl *ctl, const vshCmd *cmd) "|./source/@name" "|./source/@volume)", ctxt); if (details) { - vshPrint(ctl, "%-10s %-10s %-10s %s\n", type, device, - target, source ? source : "-"); + if (vshTableRowAppend(table, type, device, target, + source ? source : "-", NULL) < 0) + goto cleanup; } else { - vshPrint(ctl, "%-10s %s\n", target, source ? source : "-"); + if (vshTableRowAppend(table, target, + source ? source : "-", NULL) < 0) + goto cleanup; } VIR_FREE(source); @@ -645,9 +649,12 @@ cmdDomblklist(vshControl *ctl, const vshCmd *cmd) VIR_FREE(type); } + vshTablePrintToStdout(table, ctl); + ret = true; cleanup: + vshTableFree(table); VIR_FREE(source); VIR_FREE(target); VIR_FREE(device); -- 2.17.1
Signed-off-by: Simon Kobyda <skobyda@redhat.com> --- tools/virsh-domain-monitor.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index b9b6739287..d31f02e5e6 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -693,6 +693,7 @@ cmdDomiflist(vshControl *ctl, const vshCmd *cmd) int ninterfaces; xmlNodePtr *interfaces = NULL; size_t i; + vshTablePtr table = NULL; if (vshCommandOptBool(cmd, "inactive")) flags |= VIR_DOMAIN_XML_INACTIVE; @@ -704,9 +705,10 @@ cmdDomiflist(vshControl *ctl, const vshCmd *cmd) if (ninterfaces < 0) goto cleanup; - vshPrintExtra(ctl, "%-10s %-10s %-10s %-11s %s\n", _("Interface"), - _("Type"), _("Source"), _("Model"), _("MAC")); - vshPrintExtra(ctl, "-------------------------------------------------------\n"); + table = vshTableNew("Interface", "Type", + "Source", "Model", "MAC", NULL); + if (!table) + goto cleanup; for (i = 0; i < ninterfaces; i++) { char *type = NULL; @@ -727,12 +729,14 @@ cmdDomiflist(vshControl *ctl, const vshCmd *cmd) model = virXPathString("string(./model/@type)", ctxt); mac = virXPathString("string(./mac/@address)", ctxt); - vshPrint(ctl, "%-10s %-10s %-10s %-11s %-10s\n", - target ? target : "-", - type, - source ? source : "-", - model ? model : "-", - mac ? mac : "-"); + if (vshTableRowAppend(table, + target ? target : "-", + type, + source ? source : "-", + model ? model : "-", + mac ? mac : "-", + NULL) < 0) + goto cleanup; VIR_FREE(type); VIR_FREE(source); @@ -741,9 +745,12 @@ cmdDomiflist(vshControl *ctl, const vshCmd *cmd) VIR_FREE(mac); } + vshTablePrintToStdout(table, ctl); + ret = true; cleanup: + vshTableFree(table); VIR_FREE(interfaces); xmlFreeDoc(xmldoc); xmlXPathFreeContext(ctxt); -- 2.17.1
Signed-off-by: Simon Kobyda <skobyda@redhat.com> --- tools/virsh-domain.c | 95 +++++++++++++++++++++++++++++++++----------- 1 file changed, 71 insertions(+), 24 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index c1cff9fe2d..c580be998c 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -59,6 +59,7 @@ #include "virxml.h" #include "virsh-nodedev.h" #include "viruri.h" +#include "vsh-table.h" /* Gnulib doesn't guarantee SA_SIGINFO support. */ #ifndef SA_SIGINFO @@ -6905,6 +6906,7 @@ virshVcpuPinQuery(vshControl *ctl, size_t i; int ncpus; bool ret = false; + vshTablePtr table = NULL; if ((ncpus = virshCPUCountCollect(ctl, dom, countFlags, true)) < 0) { if (ncpus == -1) { @@ -6913,7 +6915,7 @@ virshVcpuPinQuery(vshControl *ctl, else vshError(ctl, "%s", _("cannot get vcpupin for transient domain")); } - return false; + goto cleanup; } if (got_vcpu && vcpu >= ncpus) { @@ -6927,28 +6929,42 @@ virshVcpuPinQuery(vshControl *ctl, vshError(ctl, _("vcpu %d is out of range of persistent cpu count %d"), vcpu, ncpus); - return false; + goto cleanup; } cpumaplen = VIR_CPU_MAPLEN(maxcpu); cpumap = vshMalloc(ctl, ncpus * cpumaplen); if ((ncpus = virDomainGetVcpuPinInfo(dom, ncpus, cpumap, cpumaplen, flags)) >= 0) { - vshPrintExtra(ctl, "%s %s\n", _("VCPU:"), _("CPU Affinity")); - vshPrintExtra(ctl, "----------------------------------\n"); + table = vshTableNew("VCPU", "CPU Affinity", NULL); + if (!table) + goto cleanup; + for (i = 0; i < ncpus; i++) { + char *pinInfo = NULL; + char *vcpuStr; if (got_vcpu && i != vcpu) continue; - vshPrint(ctl, "%4zu: ", i); - ret = virshPrintPinInfo(ctl, VIR_GET_CPUMAP(cpumap, cpumaplen, i), - cpumaplen); - vshPrint(ctl, "\n"); - if (!ret) - break; + if (!(pinInfo = virBitmapDataFormat(cpumap, cpumaplen))) + goto cleanup; + + if (virAsprintf(&vcpuStr, "%lu", i) < 0) + goto cleanup; + + if (vshTableRowAppend(table, vcpuStr, pinInfo, NULL) < 0) + goto cleanup; + + VIR_FREE(vcpuStr); + VIR_FREE(pinInfo); } + + vshTablePrintToStdout(table, ctl); } + ret = true; + cleanup: + vshTableFree(table); VIR_FREE(cpumap); return ret; } @@ -7520,6 +7536,7 @@ cmdIOThreadInfo(vshControl *ctl, const vshCmd *cmd) int maxcpu; unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT; virshControlPtr priv = ctl->privData; + vshTablePtr table = NULL; VSH_EXCLUSIVE_OPTIONS_VAR(current, live); VSH_EXCLUSIVE_OPTIONS_VAR(current, config); @@ -7545,19 +7562,32 @@ cmdIOThreadInfo(vshControl *ctl, const vshCmd *cmd) goto cleanup; } - vshPrintExtra(ctl, " %-15s %-15s\n", - _("IOThread ID"), _("CPU Affinity")); - vshPrintExtra(ctl, "---------------------------------------------------\n"); + table = vshTableNew("IOThread ID", "CPU Affinity", NULL); + if (!table) + goto cleanup; + for (i = 0; i < niothreads; i++) { + char *pinInfo = NULL; + char *iothreadIdStr; - vshPrint(ctl, " %-15u ", info[i]->iothread_id); - ignore_value(virshPrintPinInfo(ctl, info[i]->cpumap, info[i]->cpumaplen)); - vshPrint(ctl, "\n"); + if (virAsprintf(&iothreadIdStr, "%u", info[i]->iothread_id) < 0) + goto cleanup; + + ignore_value(pinInfo = virBitmapDataFormat(info[i]->cpumap, info[i]->cpumaplen)); + + if (vshTableRowAppend(table, iothreadIdStr, pinInfo ? pinInfo : "", NULL) < 0) + goto cleanup; + + VIR_FREE(pinInfo); + VIR_FREE(iothreadIdStr); virDomainIOThreadInfoFree(info[i]); } VIR_FREE(info); + vshTablePrintToStdout(table, ctl); + cleanup: + vshTableFree(table); virshDomainFree(dom); return niothreads >= 0; } @@ -13778,6 +13808,7 @@ cmdDomFSInfo(vshControl *ctl, const vshCmd *cmd) int ret = -1; size_t i, j; virDomainFSInfoPtr *info; + vshTablePtr table = NULL; if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) return false; @@ -13793,25 +13824,41 @@ cmdDomFSInfo(vshControl *ctl, const vshCmd *cmd) } if (info) { - vshPrintExtra(ctl, "%-36s %-8s %-8s %s\n", - _("Mountpoint"), _("Name"), _("Type"), _("Target")); - vshPrintExtra(ctl, "-------------------------------------------------------------------\n"); + table = vshTableNew("Mountpoint", "Name", "Type", "Target", NULL); + if (!table) + goto cleanup; + for (i = 0; i < ret; i++) { - vshPrint(ctl, "%-36s %-8s %-8s ", - info[i]->mountpoint, info[i]->name, info[i]->fstype); + virBuffer targetsBuff = VIR_BUFFER_INITIALIZER; + char *targets; + for (j = 0; j < info[i]->ndevAlias; j++) { - vshPrint(ctl, "%s", info[i]->devAlias[j]); + virBufferAdd(&targetsBuff, info[i]->devAlias[j], -1); if (j != info[i]->ndevAlias - 1) - vshPrint(ctl, ","); + virBufferAddChar(&targetsBuff, ','); } - vshPrint(ctl, "\n"); + targets = virBufferContentAndReset(&targetsBuff); + + if (vshTableRowAppend(table, + info[i]->mountpoint, + info[i]->name, + info[i]->fstype, + targets, + NULL) < 0) + goto cleanup; + + VIR_FREE(targets); virDomainFSInfoFree(info[i]); } + + vshTablePrintToStdout(table, ctl); + VIR_FREE(info); } cleanup: + vshTableFree(table); virshDomainFree(dom); return ret >= 0; } -- 2.17.1
Local lengthy unicode-unreliable table formatting was replaced by new API. Great example of how new API saves space and time. Removed a lot of string lenght canculation used by the local table. Signed-off-by: Simon Kobyda <skobyda@redhat.com> --- tools/virsh-pool.c | 160 +++++++++------------------------------------ 1 file changed, 30 insertions(+), 130 deletions(-) diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c index 89206a48f5..724864a935 100644 --- a/tools/virsh-pool.c +++ b/tools/virsh-pool.c @@ -33,6 +33,7 @@ #include "conf/storage_conf.h" #include "virstring.h" #include "virtime.h" +#include "vsh-table.h" #define VIRSH_COMMON_OPT_POOL_FULL(cflags) \ VIRSH_COMMON_OPT_POOL(N_("pool name or uuid"), cflags) @@ -1113,10 +1114,6 @@ cmdPoolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) virStoragePoolInfo info; size_t i; bool ret = false; - size_t stringLength = 0, nameStrLength = 0; - size_t autostartStrLength = 0, persistStrLength = 0; - size_t stateStrLength = 0, capStrLength = 0; - size_t allocStrLength = 0, availStrLength = 0; struct poolInfoText { char *state; char *autostart; @@ -1133,7 +1130,7 @@ cmdPoolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) bool inactive, all; bool uuid = false; bool name = false; - char *outputStr = NULL; + vshTablePtr table = NULL; inactive = vshCommandOptBool(cmd, "inactive"); all = vshCommandOptBool(cmd, "all"); @@ -1260,11 +1257,6 @@ cmdPoolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) else poolInfoTexts[i].persistent = vshStrdup(ctl, persistent ? _("yes") : _("no")); - - /* Keep the length of persistent string if longest so far */ - stringLength = strlen(poolInfoTexts[i].persistent); - if (stringLength > persistStrLength) - persistStrLength = stringLength; } /* Collect further extended information about the pool */ @@ -1310,21 +1302,6 @@ cmdPoolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) poolInfoTexts[i].allocation = vshStrdup(ctl, _("-")); poolInfoTexts[i].available = vshStrdup(ctl, _("-")); } - - /* Keep the length of capacity string if longest so far */ - stringLength = strlen(poolInfoTexts[i].capacity); - if (stringLength > capStrLength) - capStrLength = stringLength; - - /* Keep the length of allocation string if longest so far */ - stringLength = strlen(poolInfoTexts[i].allocation); - if (stringLength > allocStrLength) - allocStrLength = stringLength; - - /* Keep the length of available string if longest so far */ - stringLength = strlen(poolInfoTexts[i].available); - if (stringLength > availStrLength) - availStrLength = stringLength; } else { /* --details option was not specified, only active/inactive * state strings are used */ @@ -1334,21 +1311,6 @@ cmdPoolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) poolInfoTexts[i].state = vshStrdup(ctl, _("inactive")); } } - - /* Keep the length of name string if longest so far */ - stringLength = strlen(virStoragePoolGetName(list->pools[i])); - if (stringLength > nameStrLength) - nameStrLength = stringLength; - - /* Keep the length of state string if longest so far */ - stringLength = strlen(poolInfoTexts[i].state); - if (stringLength > stateStrLength) - stateStrLength = stringLength; - - /* Keep the length of autostart string if longest so far */ - stringLength = strlen(poolInfoTexts[i].autostart); - if (stringLength > autostartStrLength) - autostartStrLength = stringLength; } /* If the --details option wasn't selected, we output the pool @@ -1376,19 +1338,23 @@ cmdPoolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) } /* Output old style header */ - vshPrintExtra(ctl, " %-20s %-10s %-10s\n", _("Name"), _("State"), - _("Autostart")); - vshPrintExtra(ctl, "-------------------------------------------\n"); + table = vshTableNew("Name", "State", "Autostart", NULL); + if (!table) + goto cleanup; /* Output old style pool info */ for (i = 0; i < list->npools; i++) { const char *name_str = virStoragePoolGetName(list->pools[i]); - vshPrint(ctl, " %-20s %-10s %-10s\n", - name_str, - poolInfoTexts[i].state, - poolInfoTexts[i].autostart); + if (vshTableRowAppend(table, + name_str, + poolInfoTexts[i].state, + poolInfoTexts[i].autostart, + NULL) < 0) + goto cleanup; } + vshTablePrintToStdout(table, ctl); + /* Cleanup and return */ ret = true; goto cleanup; @@ -1396,99 +1362,33 @@ cmdPoolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) /* We only get here if the --details option was selected. */ - /* Use the length of name header string if it's longest */ - stringLength = strlen(_("Name")); - if (stringLength > nameStrLength) - nameStrLength = stringLength; - - /* Use the length of state header string if it's longest */ - stringLength = strlen(_("State")); - if (stringLength > stateStrLength) - stateStrLength = stringLength; - - /* Use the length of autostart header string if it's longest */ - stringLength = strlen(_("Autostart")); - if (stringLength > autostartStrLength) - autostartStrLength = stringLength; - - /* Use the length of persistent header string if it's longest */ - stringLength = strlen(_("Persistent")); - if (stringLength > persistStrLength) - persistStrLength = stringLength; - - /* Use the length of capacity header string if it's longest */ - stringLength = strlen(_("Capacity")); - if (stringLength > capStrLength) - capStrLength = stringLength; - - /* Use the length of allocation header string if it's longest */ - stringLength = strlen(_("Allocation")); - if (stringLength > allocStrLength) - allocStrLength = stringLength; - - /* Use the length of available header string if it's longest */ - stringLength = strlen(_("Available")); - if (stringLength > availStrLength) - availStrLength = stringLength; - - /* Display the string lengths for debugging. */ - vshDebug(ctl, VSH_ERR_DEBUG, "Longest name string = %lu chars\n", - (unsigned long) nameStrLength); - vshDebug(ctl, VSH_ERR_DEBUG, "Longest state string = %lu chars\n", - (unsigned long) stateStrLength); - vshDebug(ctl, VSH_ERR_DEBUG, "Longest autostart string = %lu chars\n", - (unsigned long) autostartStrLength); - vshDebug(ctl, VSH_ERR_DEBUG, "Longest persistent string = %lu chars\n", - (unsigned long) persistStrLength); - vshDebug(ctl, VSH_ERR_DEBUG, "Longest capacity string = %lu chars\n", - (unsigned long) capStrLength); - vshDebug(ctl, VSH_ERR_DEBUG, "Longest allocation string = %lu chars\n", - (unsigned long) allocStrLength); - vshDebug(ctl, VSH_ERR_DEBUG, "Longest available string = %lu chars\n", - (unsigned long) availStrLength); - - /* Create the output template. Each column is sized according to - * the longest string. - */ - if (virAsprintf(&outputStr, - " %%-%lus %%-%lus %%-%lus %%-%lus %%%lus %%%lus %%%lus\n", - (unsigned long) nameStrLength, - (unsigned long) stateStrLength, - (unsigned long) autostartStrLength, - (unsigned long) persistStrLength, - (unsigned long) capStrLength, - (unsigned long) allocStrLength, - (unsigned long) availStrLength) < 0) - goto cleanup; - /* Display the header */ - vshPrintExtra(ctl, outputStr, _("Name"), _("State"), _("Autostart"), - _("Persistent"), _("Capacity"), _("Allocation"), - _("Available")); - for (i = nameStrLength + stateStrLength + autostartStrLength - + persistStrLength + capStrLength - + allocStrLength + availStrLength - + 14; i > 0; i--) - vshPrintExtra(ctl, "-"); - vshPrintExtra(ctl, "\n"); + table = vshTableNew("Name", "State", "Autostart", "Persistent", + "Capacity", "Allocation", "Available", NULL); + if (!table) + goto cleanup; /* Display the pool info rows */ for (i = 0; i < list->npools; i++) { - vshPrint(ctl, outputStr, - virStoragePoolGetName(list->pools[i]), - poolInfoTexts[i].state, - poolInfoTexts[i].autostart, - poolInfoTexts[i].persistent, - poolInfoTexts[i].capacity, - poolInfoTexts[i].allocation, - poolInfoTexts[i].available); + if (vshTableRowAppend(table, + virStoragePoolGetName(list->pools[i]), + poolInfoTexts[i].state, + poolInfoTexts[i].autostart, + poolInfoTexts[i].persistent, + poolInfoTexts[i].capacity, + poolInfoTexts[i].allocation, + poolInfoTexts[i].available, + NULL) < 0) + goto cleanup; } + vshTablePrintToStdout(table, ctl); + /* Cleanup and return */ ret = true; cleanup: - VIR_FREE(outputStr); + vshTableFree(table); if (list && list->npools) { for (i = 0; i < list->npools; i++) { VIR_FREE(poolInfoTexts[i].state); -- 2.17.1
Local lengthy unicode-unreliable table formatting was replaced by new API. Great example of how new API saves space and time. Removed a lot of string lenght calculation used by the local table. Signed-off-by: Simon Kobyda <skobyda@redhat.com> --- tools/virsh-volume.c | 127 +++++++++---------------------------------- 1 file changed, 27 insertions(+), 100 deletions(-) diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c index 42d11701ec..071882c628 100644 --- a/tools/virsh-volume.c +++ b/tools/virsh-volume.c @@ -42,6 +42,7 @@ #include "virsh-pool.h" #include "virxml.h" #include "virstring.h" +#include "vsh-table.h" #define VIRSH_COMMON_OPT_POOL_FULL \ VIRSH_COMMON_OPT_POOL(N_("pool name or uuid"), \ @@ -1382,16 +1383,11 @@ cmdVolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) { virStorageVolInfo volumeInfo; virStoragePoolPtr pool; - char *outputStr = NULL; const char *unit; double val; bool details = vshCommandOptBool(cmd, "details"); size_t i; bool ret = false; - int stringLength = 0; - size_t allocStrLength = 0, capStrLength = 0; - size_t nameStrLength = 0, pathStrLength = 0; - size_t typeStrLength = 0; struct volInfoText { char *allocation; char *capacity; @@ -1400,6 +1396,7 @@ cmdVolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) }; struct volInfoText *volInfoTexts = NULL; virshStorageVolListPtr list = NULL; + vshTablePtr table = NULL; /* Look up the pool information given to us by the user */ if (!(pool = virshCommandOptPool(ctl, cmd, "pool", NULL))) @@ -1446,36 +1443,6 @@ cmdVolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) "%.2lf %s", val, unit) < 0) goto cleanup; } - - /* Remember the largest length for each output string. - * This lets us displaying header and volume information rows - * using a single, properly sized, printf style output string. - */ - - /* Keep the length of name string if longest so far */ - stringLength = strlen(virStorageVolGetName(list->vols[i])); - if (stringLength > nameStrLength) - nameStrLength = stringLength; - - /* Keep the length of path string if longest so far */ - stringLength = strlen(volInfoTexts[i].path); - if (stringLength > pathStrLength) - pathStrLength = stringLength; - - /* Keep the length of type string if longest so far */ - stringLength = strlen(volInfoTexts[i].type); - if (stringLength > typeStrLength) - typeStrLength = stringLength; - - /* Keep the length of capacity string if longest so far */ - stringLength = strlen(volInfoTexts[i].capacity); - if (stringLength > capStrLength) - capStrLength = stringLength; - - /* Keep the length of allocation string if longest so far */ - stringLength = strlen(volInfoTexts[i].allocation); - if (stringLength > allocStrLength) - allocStrLength = stringLength; } } @@ -1487,14 +1454,20 @@ cmdVolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) /* Output basic info then return if --details option not selected */ if (!details) { /* The old output format */ - vshPrintExtra(ctl, " %-20s %-40s\n", _("Name"), _("Path")); - vshPrintExtra(ctl, "---------------------------------------" - "---------------------------------------\n"); + table = vshTableNew("Name", "Path", NULL); + if (!table) + goto cleanup; + for (i = 0; i < list->nvols; i++) { - vshPrint(ctl, " %-20s %-40s\n", virStorageVolGetName(list->vols[i]), - volInfoTexts[i].path); + if (vshTableRowAppend(table, + virStorageVolGetName(list->vols[i]), + volInfoTexts[i].path, + NULL) < 0) + goto cleanup; } + vshTablePrintToStdout(table, ctl); + /* Cleanup and return */ ret = true; goto cleanup; @@ -1502,75 +1475,30 @@ cmdVolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) /* We only get here if the --details option was selected. */ - /* Use the length of name header string if it's longest */ - stringLength = strlen(_("Name")); - if (stringLength > nameStrLength) - nameStrLength = stringLength; - - /* Use the length of path header string if it's longest */ - stringLength = strlen(_("Path")); - if (stringLength > pathStrLength) - pathStrLength = stringLength; - - /* Use the length of type header string if it's longest */ - stringLength = strlen(_("Type")); - if (stringLength > typeStrLength) - typeStrLength = stringLength; - - /* Use the length of capacity header string if it's longest */ - stringLength = strlen(_("Capacity")); - if (stringLength > capStrLength) - capStrLength = stringLength; - - /* Use the length of allocation header string if it's longest */ - stringLength = strlen(_("Allocation")); - if (stringLength > allocStrLength) - allocStrLength = stringLength; - - /* Display the string lengths for debugging */ - vshDebug(ctl, VSH_ERR_DEBUG, - "Longest name string = %zu chars\n", nameStrLength); - vshDebug(ctl, VSH_ERR_DEBUG, - "Longest path string = %zu chars\n", pathStrLength); - vshDebug(ctl, VSH_ERR_DEBUG, - "Longest type string = %zu chars\n", typeStrLength); - vshDebug(ctl, VSH_ERR_DEBUG, - "Longest capacity string = %zu chars\n", capStrLength); - vshDebug(ctl, VSH_ERR_DEBUG, - "Longest allocation string = %zu chars\n", allocStrLength); - - if (virAsprintf(&outputStr, - " %%-%lus %%-%lus %%-%lus %%%lus %%%lus\n", - (unsigned long) nameStrLength, - (unsigned long) pathStrLength, - (unsigned long) typeStrLength, - (unsigned long) capStrLength, - (unsigned long) allocStrLength) < 0) - goto cleanup; - /* Display the header */ - vshPrintExtra(ctl, outputStr, _("Name"), _("Path"), _("Type"), - _("Capacity"), _("Allocation")); - for (i = nameStrLength + pathStrLength + typeStrLength - + capStrLength + allocStrLength - + 10; i > 0; i--) - vshPrintExtra(ctl, "-"); - vshPrintExtra(ctl, "\n"); + table = vshTableNew("Name", "Path", "Type", "Capacity", "Allocation", NULL); + if (!table) + goto cleanup; /* Display the volume info rows */ for (i = 0; i < list->nvols; i++) { - vshPrint(ctl, outputStr, - virStorageVolGetName(list->vols[i]), - volInfoTexts[i].path, - volInfoTexts[i].type, - volInfoTexts[i].capacity, - volInfoTexts[i].allocation); + if (vshTableRowAppend(table, + virStorageVolGetName(list->vols[i]), + volInfoTexts[i].path, + volInfoTexts[i].type, + volInfoTexts[i].capacity, + volInfoTexts[i].allocation, + NULL) < 0) + goto cleanup; } + vshTablePrintToStdout(table, ctl); + /* Cleanup and return */ ret = true; cleanup: + vshTableFree(table); /* Safely free the memory allocated in this function */ if (list && list->nvols) { @@ -1584,7 +1512,6 @@ cmdVolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) } /* Cleanup remaining memory */ - VIR_FREE(outputStr); VIR_FREE(volInfoTexts); virStoragePoolFree(pool); virshStorageVolListFree(list); -- 2.17.1
Signed-off-by: Simon Kobyda <skobyda@redhat.com> --- tools/virt-admin.c | 46 ++++++++++++++++++++++++++++++++++++---------- 1 file changed, 36 insertions(+), 10 deletions(-) diff --git a/tools/virt-admin.c b/tools/virt-admin.c index 63822bc13e..ac4c72efec 100644 --- a/tools/virt-admin.c +++ b/tools/virt-admin.c @@ -40,6 +40,7 @@ #include "virgettext.h" #include "virtime.h" #include "virt-admin-completer.h" +#include "vsh-table.h" /* Gnulib doesn't guarantee SA_SIGINFO support. */ #ifndef SA_SIGINFO @@ -382,6 +383,7 @@ cmdSrvList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) char *uri = NULL; virAdmServerPtr *srvs = NULL; vshAdmControlPtr priv = ctl->privData; + vshTablePtr table = NULL; /* Obtain a list of available servers on the daemon */ if ((nsrvs = virAdmConnectListServers(priv->conn, &srvs, 0)) < 0) { @@ -391,13 +393,28 @@ cmdSrvList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) goto cleanup; } - vshPrintExtra(ctl, " %-5s %-15s\n", "Id", "Name"); - vshPrintExtra(ctl, "---------------\n"); - for (i = 0; i < nsrvs; i++) - vshPrint(ctl, " %-5zu %-15s\n", i, virAdmServerGetName(srvs[i])); + table = vshTableNew("Id", "Name", NULL); + if (!table) + goto cleanup; + + for (i = 0; i < nsrvs; i++) { + char *idStr; + if (virAsprintf(&idStr, "%lu", i) < 0) + goto cleanup; + + if (vshTableRowAppend(table, + idStr, + virAdmServerGetName(srvs[i]), + NULL) < 0) + goto cleanup; + VIR_FREE(idStr); + } + + vshTablePrintToStdout(table, ctl); ret = true; cleanup: + vshTableFree(table); if (srvs) { for (i = 0; i < nsrvs; i++) virAdmServerFree(srvs[i]); @@ -617,6 +634,7 @@ cmdSrvClientsList(vshControl *ctl, const vshCmd *cmd) virAdmServerPtr srv = NULL; virAdmClientPtr *clts = NULL; vshAdmControlPtr priv = ctl->privData; + vshTablePtr table = NULL; if (vshCommandOptStringReq(ctl, cmd, "server", &srvname) < 0) return false; @@ -631,12 +649,12 @@ cmdSrvClientsList(vshControl *ctl, const vshCmd *cmd) goto cleanup; } - vshPrintExtra(ctl, " %-5s %-15s %-15s\n%s\n", _("Id"), _("Transport"), - _("Connected since"), - "-------------------------" - "-------------------------"); + table = vshTableNew("Id", "Transport", "Connected sice", NULL); + if (!table) + goto cleanup; for (i = 0; i < nclts; i++) { + char *idStr; virAdmClientPtr client = clts[i]; id = virAdmClientGetID(client); transport = virAdmClientGetTransport(client); @@ -644,14 +662,22 @@ cmdSrvClientsList(vshControl *ctl, const vshCmd *cmd) ×tr) < 0) goto cleanup; - vshPrint(ctl, " %-5llu %-15s %-15s\n", - id, vshAdmClientTransportToString(transport), timestr); + if (virAsprintf(&idStr, "%llu", id) < 0) + goto cleanup; + if (vshTableRowAppend(table, idStr, + vshAdmClientTransportToString(transport), + timestr, NULL) < 0) + goto cleanup; VIR_FREE(timestr); + VIR_FREE(idStr); } + vshTablePrintToStdout(table, ctl); + ret = true; cleanup: + vshTableFree(table); if (clts) { for (i = 0; i < nclts; i++) virAdmClientFree(clts[i]); -- 2.17.1
participants (3)
-
Erik Skultety -
Michal Privoznik -
Simon Kobyda