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

Michal Privoznik mprivozn at redhat.com
Fri Sep 21 14:30:36 UTC 2018


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 at 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




More information about the libvir-list mailing list