Re: [libvirt] [PATCH 02/13] virsh: Implement vshTable API to net-list and net-dhcp-leases

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(a)redhat.com&gt; >> --- >> 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.