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@xxxxxxxxxx>
>>> ---
>>> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
