[libvirt] [PATCH 6/7] list: Use virConnectListAllNetworks in virsh

Osier Yang jyang at redhat.com
Tue Sep 11 10:02:03 UTC 2012


On 2012年09月11日 00:29, Laine Stump wrote:
> On 09/04/2012 11:55 AM, Osier Yang wrote:
>> tools/virsh-network.c:
>>    * vshNetworkSorter to sort networks by name
>>
>>    * vshNetworkListFree to free the network objects list.
>>
>>    * vshNetworkListCollect to collect the network objects, trying
>>      to use new API first, fall back to older APIs if it's not supported.
>>
>>    * New options --persistent, --transient, --autostart, --no-autostart,
>>      for net-list, and new field 'Persistent' for its output.
>>
>> tools/virsh.pod:
>>    * Add documents for the new options.
>> ---
>>   tools/virsh-network.c |  352 ++++++++++++++++++++++++++++++++++++------------
>>   tools/virsh.pod       |   12 ++-
>>   2 files changed, 275 insertions(+), 89 deletions(-)
>>
>> diff --git a/tools/virsh-network.c b/tools/virsh-network.c
>> index db204af..f6623ff 100644
>> --- a/tools/virsh-network.c
>> +++ b/tools/virsh-network.c
>> @@ -36,6 +36,7 @@
>>   #include "memory.h"
>>   #include "util.h"
>>   #include "xml.h"
>> +#include "conf/network_conf.h"
>
>
> I've gotta say that (as discussed before) I don't like including
> something from the conf directory here. I think it's the case that this
> is only being done so that virsh can provide the new functionality even
> when only the old API is available, but I think it should be done in a
> self-contained manner, at least partly because people will look to virsh
> as an example of how to use the libvirt API. I guess I'm okay with
> leaving it this way for now, but I think it really needs to be cleaned up.
>

Yes, that's same problem in the whole series. I will cleanup later.

>
>>
>>   virNetworkPtr
>>   vshCommandOptNetworkBy(vshControl *ctl, const vshCmd *cmd,
>> @@ -342,6 +343,225 @@ cmdNetworkInfo(vshControl *ctl, const vshCmd *cmd)
>>       return true;
>>   }
>>
>> +static int
>> +vshNetworkSorter(const void *a, const void *b)
>> +{
>> +    virNetworkPtr *na = (virNetworkPtr *) a;
>> +    virNetworkPtr *nb = (virNetworkPtr *) b;
>> +
>> +    if (*na&&  !*nb)
>> +        return -1;
>> +
>> +    if (!*na)
>> +        return *nb != NULL;
>> +
>> +    return vshStrcasecmp(virNetworkGetName(*na),
>> +                      virNetworkGetName(*nb));
>
>
> This use of strcasecmp mmade me go back to verify that case *is*
> significant in the rest of the network_conf code (so, for example, you
> can have both a network named "ABC" and one named "Abc"). It's still
> useful to have strcasecmp here, though, as it makes the ordering ignore
> case, which is a "good thing"(tm).

Okay. :-)

>
>
>> +}
>> +
>> +struct vshNetworkList {
>> +    virNetworkPtr *nets;
>> +    size_t nnets;
>> +};
>> +typedef struct vshNetworkList *vshNetworkListPtr;
>
> The fact that you're doing this leaves me wondering if maybe
> virNetworkList should be a part of the API (along with a
> "virNetworkListFree()" as I've previously mentioned). After all, the
> fact that this first example of using the new API is doing this is a
> reasonably good indicator that future users will be copying the same code.
>

I still think it should be the work of the client apps (e.g. virsh
here).

>
>> +
>> +static void
>> +vshNetworkListFree(vshNetworkListPtr list)
>> +{
>> +    int i;
>> +
>> +    if (list&&  list->nnets) {
>> +        for (i = 0; i<  list->nnets; i++) {
>> +            if (list->nets[i])
>> +                virNetworkFree(list->nets[i]);
>> +        }
>> +        VIR_FREE(list->nets);
>> +    }
>> +    VIR_FREE(list);
>> +}
>> +
>> +static vshNetworkListPtr
>> +vshNetworkListCollect(vshControl *ctl,
>> +                      unsigned int flags)
>> +{
>> +    vshNetworkListPtr list = vshMalloc(ctl, sizeof(*list));
>> +    int i;
>> +    int ret;
>> +    char **names = NULL;
>> +    virNetworkPtr net;
>> +    bool success = false;
>> +    size_t deleted = 0;
>> +    int persistent;
>> +    int autostart;
>> +    int nActiveNets = 0;
>> +    int nInactiveNets = 0;
>> +    int nAllNets = 0;
>> +
>> +    /* try the list with flags support (0.10.0 and later) */
>> +    if ((ret = virConnectListAllNetworks(ctl->conn,
>> +&list->nets,
>> +                                         flags))>= 0) {
>> +        list->nnets = ret;
>> +        goto finished;
>> +    }
>> +
>> +    /* check if the command is actually supported */
>> +    if (last_error&&  last_error->code == VIR_ERR_NO_SUPPORT) {
>> +        vshResetLibvirtError();
>> +        goto fallback;
>> +    }
>> +
>> +    if (last_error&&  last_error->code ==  VIR_ERR_INVALID_ARG) {
>> +        /* try the new API again but mask non-guaranteed flags */
>> +        unsigned int newflags = flags&  (VIR_CONNECT_LIST_NETWORKS_ACTIVE |
>> +                                         VIR_CONNECT_LIST_NETWORKS_INACTIVE);
>> +
>> +        vshResetLibvirtError();
>> +        if ((ret = virConnectListAllNetworks(ctl->conn,&list->nets,
>> +                                             newflags))>= 0) {
>> +            list->nnets = ret;
>> +            goto filter;
>> +        }
>> +    }
>> +
>> +    /* there was an error during the first or second call */
>> +    vshError(ctl, "%s", _("Failed to list networks"));
>> +    goto cleanup;
>> +
>> +
>> +fallback:
>> +    /* fall back to old method (0.9.13 and older) */
>
>
> Well, okay, I'll mention this incorrect version number, because it's a
> different number than the others.

Okay. Will update when pushing.

>
>
>> +    vshResetLibvirtError();
>
>
> As was pointed out in the nwfilter patches, this 2nd
> vshResetLibvirtError() is redundant; you only need it in one place or
> the other, but not both.

Okay. Will ... pushing.

>
>
>> +
>> +    /* Get the number of active networks */
>> +    if (!MATCH(VIR_CONNECT_LIST_NETWORKS_FILTERS_ACTIVE) ||
>> +        MATCH(VIR_CONNECT_LIST_NETWORKS_ACTIVE)) {
>> +        if ((nActiveNets = virConnectNumOfNetworks(ctl->conn))<  0) {
>> +            vshError(ctl, "%s", _("Failed to get the number of active networks"));
>> +            goto cleanup;
>> +        }
>> +    }
>> +
>> +    /* Get the number of inactive networks */
>> +    if (!MATCH(VIR_CONNECT_LIST_NETWORKS_FILTERS_ACTIVE) ||
>> +        MATCH(VIR_CONNECT_LIST_NETWORKS_INACTIVE)) {
>> +        if ((nInactiveNets = virConnectNumOfDefinedNetworks(ctl->conn))<  0) {
>> +            vshError(ctl, "%s", _("Failed to get the number of inactive networks"));
>> +            goto cleanup;
>> +        }
>> +    }
>> +
>> +    nAllNets = nActiveNets + nInactiveNets;
>> +
>> +    if (nAllNets == 0)
>> +         return list;
>> +
>> +    names = vshMalloc(ctl, sizeof(char *) * nAllNets);
>> +
>> +    /* Retrieve a list of active network names */
>> +    if (!MATCH(VIR_CONNECT_LIST_NETWORKS_FILTERS_ACTIVE) ||
>> +        MATCH(VIR_CONNECT_LIST_NETWORKS_ACTIVE)) {
>> +        if (virConnectListNetworks(ctl->conn,
>> +                                   names, nActiveNets)<  0) {
>> +            vshError(ctl, "%s", _("Failed to list active networks"));
>> +            goto cleanup;
>> +        }
>> +    }
>> +
>> +    /* Add the inactive networks to the end of the name list */
>> +    if (!MATCH(VIR_CONNECT_LIST_NETWORKS_FILTERS_ACTIVE) ||
>> +        MATCH(VIR_CONNECT_LIST_NETWORKS_ACTIVE)) {
>> +        if (virConnectListDefinedNetworks(ctl->conn,
>> +&names[nActiveNets],
>> +                                          nInactiveNets)<  0) {
>> +            vshError(ctl, "%s", _("Failed to list inactive networks"));
>> +            goto cleanup;
>> +        }
>> +    }
>> +
>> +    list->nets = vshMalloc(ctl, sizeof(virNetworkPtr) * (nAllNets));
>> +    list->nnets = 0;
>> +
>> +    /* get active networks */
>> +    for (i = 0; i<  nActiveNets; i++) {
>> +        if (!(net = virNetworkLookupByName(ctl->conn, names[i])))
>> +            continue;
>> +        list->nets[list->nnets++] = net;
>> +    }
>> +
>> +    /* get inactive networks */
>> +    for (i = 0; i<  nInactiveNets; i++) {
>> +        if (!(net = virNetworkLookupByName(ctl->conn, names[i])))
>> +            continue;
>> +        list->nets[list->nnets++] = net;
>> +    }
>> +
>> +    /* truncate networks that weren't found */
>> +    deleted = nAllNets - list->nnets;
>> +
>> +filter:
>> +    /* filter list the list if the list was acquired by fallback means */
>> +    for (i = 0; i<  list->nnets; i++) {
>> +        net = list->nets[i];
>> +
>> +        /* persistence filter */
>> +        if (MATCH(VIR_CONNECT_LIST_NETWORKS_FILTERS_PERSISTENT)) {
>> +            if ((persistent = virNetworkIsPersistent(net))<  0) {
>> +                vshError(ctl, "%s", _("Failed to get network persistence info"));
>> +                goto cleanup;
>> +            }
>> +
>> +            if (!((MATCH(VIR_CONNECT_LIST_NETWORKS_PERSISTENT)&&  persistent) ||
>> +                  (MATCH(VIR_CONNECT_LIST_NETWORKS_TRANSIENT)&&  !persistent)))
>> +                goto remove_entry;
>> +        }
>> +
>> +        /* autostart filter */
>> +        if (MATCH(VIR_CONNECT_LIST_NETWORKS_FILTERS_AUTOSTART)) {
>> +            if (virNetworkGetAutostart(net,&autostart)<  0) {
>> +                vshError(ctl, "%s", _("Failed to get network autostart state"));
>> +                goto cleanup;
>> +            }
>> +
>> +            if (!((MATCH(VIR_CONNECT_LIST_NETWORKS_AUTOSTART)&&  autostart) ||
>> +                  (MATCH(VIR_CONNECT_LIST_NETWORKS_NO_AUTOSTART)&&  !autostart)))
>> +                goto remove_entry;
>> +        }
>> +        /* the pool matched all filters, it may stay */
>> +        continue;
>> +
>> +remove_entry:
>> +        /* the pool has to be removed as it failed one of the filters */
>> +        virNetworkFree(list->nets[i]);
>> +        list->nets[i] = NULL;
>> +        deleted++;
>> +    }
>> +
>> +finished:
>> +    /* sort the list */
>> +    if (list->nets&&  list->nnets)
>> +        qsort(list->nets, list->nnets,
>> +              sizeof(*list->nets), vshNetworkSorter);
>> +
>> +    /* truncate the list if filter simulation deleted entries */
>> +    if (deleted)
>> +        VIR_SHRINK_N(list->nets, list->nnets, deleted);
>> +
>> +    success = true;
>> +
>> +cleanup:
>> +    for (i = 0; i<  nAllNets; i++)
>> +        VIR_FREE(names[i]);
>> +    VIR_FREE(names);
>> +
>> +    if (!success) {
>> +        vshNetworkListFree(list);
>> +        list = NULL;
>> +    }
>> +
>> +    return list;
>> +}
>> +
>>   /*
>>    * "net-list" command
>>    */
>> @@ -354,114 +574,70 @@ static const vshCmdInfo info_network_list[] = {
>>   static const vshCmdOptDef opts_network_list[] = {
>>       {"inactive", VSH_OT_BOOL, 0, N_("list inactive networks")},
>>       {"all", VSH_OT_BOOL, 0, N_("list inactive&  active networks")},
>> +    {"persistent", VSH_OT_BOOL, 0, N_("list persistent networks")},
>> +    {"transient", VSH_OT_BOOL, 0, N_("list transient networks")},
>> +    {"autostart", VSH_OT_BOOL, 0, N_("list networks with autostart enabled")},
>> +    {"no-autostart", VSH_OT_BOOL, 0, N_("list networks with autostart disabled")},
>>       {NULL, 0, 0, NULL}
>>   };
>>
>>   static bool
>>   cmdNetworkList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED)
>>   {
>> +    vshNetworkListPtr list = NULL;
>> +    int i;
>>       bool inactive = vshCommandOptBool(cmd, "inactive");
>>       bool all = vshCommandOptBool(cmd, "all");
>> -    bool active = !inactive || all;
>> -    int maxactive = 0, maxinactive = 0, i;
>> -    char **activeNames = NULL, **inactiveNames = NULL;
>> -    inactive |= all;
>> -
>> -    if (active) {
>> -        maxactive = virConnectNumOfNetworks(ctl->conn);
>> -        if (maxactive<  0) {
>> -            vshError(ctl, "%s", _("Failed to list active networks"));
>> -            return false;
>> -        }
>> -        if (maxactive) {
>> -            activeNames = vshMalloc(ctl, sizeof(char *) * maxactive);
>> -
>> -            if ((maxactive = virConnectListNetworks(ctl->conn, activeNames,
>> -                                                    maxactive))<  0) {
>> -                vshError(ctl, "%s", _("Failed to list active networks"));
>> -                VIR_FREE(activeNames);
>> -                return false;
>> -            }
>> +    bool persistent = vshCommandOptBool(cmd, "persistent");
>> +    bool transient = vshCommandOptBool(cmd, "transient");
>> +    bool autostart = vshCommandOptBool(cmd, "autostart");
>> +    bool no_autostart = vshCommandOptBool(cmd, "no-autostart");
>> +    unsigned int flags = VIR_CONNECT_LIST_NETWORKS_ACTIVE;
>>
>> -            qsort(&activeNames[0], maxactive, sizeof(char *), vshNameSorter);
>> -        }
>> -    }
>> -    if (inactive) {
>> -        maxinactive = virConnectNumOfDefinedNetworks(ctl->conn);
>> -        if (maxinactive<  0) {
>> -            vshError(ctl, "%s", _("Failed to list inactive networks"));
>> -            VIR_FREE(activeNames);
>> -            return false;
>> -        }
>> -        if (maxinactive) {
>> -            inactiveNames = vshMalloc(ctl, sizeof(char *) * maxinactive);
>> -
>> -            if ((maxinactive =
>> -                     virConnectListDefinedNetworks(ctl->conn, inactiveNames,
>> -                                                   maxinactive))<  0) {
>> -                vshError(ctl, "%s", _("Failed to list inactive networks"));
>> -                VIR_FREE(activeNames);
>> -                VIR_FREE(inactiveNames);
>> -                return false;
>> -            }
>> +    if (inactive)
>> +        flags = VIR_CONNECT_LIST_NETWORKS_INACTIVE;
>>
>> -            qsort(&inactiveNames[0], maxinactive, sizeof(char*), vshNameSorter);
>> -        }
>> -    }
>> -    vshPrintExtra(ctl, "%-20s %-10s %s\n", _("Name"), _("State"),
>> -                  _("Autostart"));
>> -    vshPrintExtra(ctl, "-----------------------------------------\n");
>> +    if (all)
>> +        flags = VIR_CONNECT_LIST_NETWORKS_ACTIVE |
>> +                VIR_CONNECT_LIST_NETWORKS_INACTIVE;
>>
>> -    for (i = 0; i<  maxactive; i++) {
>> -        virNetworkPtr network =
>> -            virNetworkLookupByName(ctl->conn, activeNames[i]);
>> -        const char *autostartStr;
>> -        int autostart = 0;
>> +    if (persistent)
>> +         flags |= VIR_CONNECT_LIST_NETWORKS_PERSISTENT;
>>
>> -        /* this kind of work with networks is not atomic operation */
>> -        if (!network) {
>> -            VIR_FREE(activeNames[i]);
>> -            continue;
>> -        }
>> +    if (transient)
>> +         flags |= VIR_CONNECT_LIST_NETWORKS_TRANSIENT;
>>
>> -        if (virNetworkGetAutostart(network,&autostart)<  0)
>> -            autostartStr = _("no autostart");
>> -        else
>> -            autostartStr = autostart ? _("yes") : _("no");
>> +    if (autostart)
>> +         flags |= VIR_CONNECT_LIST_NETWORKS_AUTOSTART;
>>
>> -        vshPrint(ctl, "%-20s %-10s %-10s\n",
>> -                 virNetworkGetName(network),
>> -                 _("active"),
>> -                 autostartStr);
>> -        virNetworkFree(network);
>> -        VIR_FREE(activeNames[i]);
>> -    }
>> -    for (i = 0; i<  maxinactive; i++) {
>> -        virNetworkPtr network = virNetworkLookupByName(ctl->conn, inactiveNames[i]);
>> -        const char *autostartStr;
>> -        int autostart = 0;
>> +    if (no_autostart)
>> +         flags |= VIR_CONNECT_LIST_NETWORKS_NO_AUTOSTART;
>>
>> -        /* this kind of work with networks is not atomic operation */
>> -        if (!network) {
>> -            VIR_FREE(inactiveNames[i]);
>> -            continue;
>> -        }
>> +    if (!(list = vshNetworkListCollect(ctl, flags)))
>> +        return false;
>> +
>> +    vshPrintExtra(ctl, "%-20s %-10s %-13s %s\n", _("Name"), _("State"),
>> +                  _("Autostart"), _("Persistent"));
>> +    vshPrintExtra(ctl, "--------------------------------------------------\n");
>
>
> Do you think we need to worry about adding a new column onto the output
> having an adverse affect on existing scripts that parse the output?
>

Yes, we need to consider the existed script. But compared to the old
output, we only add a new field "Persistent" at the end, not breaking
the old field sequense.

 >> -    vshPrintExtra(ctl, "%-20s %-10s %s\n", _("Name"), _("State"),
 >> -                  _("Autostart"));
 >> -    vshPrintExtra(ctl, "-----------------------------------------\n");

So I think it's fine.

>
>>
>> -        if (virNetworkGetAutostart(network,&autostart)<  0)
>> +    for (i = 0; i<  list->nnets; i++) {
>> +        virNetworkPtr network = list->nets[i];
>> +        const char *autostartStr;
>> +        int is_autostart = 0;
>> +
>> +        if (virNetworkGetAutostart(network,&is_autostart)<  0)
>>               autostartStr = _("no autostart");
>>           else
>> -            autostartStr = autostart ? _("yes") : _("no");
>> -
>> -        vshPrint(ctl, "%-20s %-10s %-10s\n",
>> -                 inactiveNames[i],
>> -                 _("inactive"),
>> -                 autostartStr);
>> +            autostartStr = is_autostart ? _("yes") : _("no");
>>
>> -        virNetworkFree(network);
>> -        VIR_FREE(inactiveNames[i]);
>> +        vshPrint(ctl, "%-20s %-10s %-13s %s\n",
>> +                 virNetworkGetName(network),
>> +                 virNetworkIsActive(network) ? _("active") : _("inactive"),
>> +                 autostartStr,
>> +                 virNetworkIsPersistent(network) ? _("yes") : _("no"));
>>       }
>> -    VIR_FREE(activeNames);
>> -    VIR_FREE(inactiveNames);
>> +
>> +    vshNetworkListFree(list);
>>       return true;
>>   }
>>
>> diff --git a/tools/virsh.pod b/tools/virsh.pod
>> index 9aeb47e..c806335 100644
>> --- a/tools/virsh.pod
>> +++ b/tools/virsh.pod
>> @@ -1933,10 +1933,20 @@ variables, and defaults to C<vi>.
>>   Returns basic information about the I<network>  object.
>>
>>   =item B<net-list>  [I<--inactive>  | I<--all>]
>> +                  [I<--persistent>] [<--transient>]
>> +                  [I<--autostart>] [<--no-autostart>]
>>
>>   Returns the list of active networks, if I<--all>  is specified this will also
>>   include defined but inactive networks, if I<--inactive>  is specified only the
>> -inactive ones will be listed.
>> +inactive ones will be listed. You may also want to filter the returned networks
>> +by I<--persistent>  to list the persitent ones, I<--transient>  to list the
>> +transient ones, I<--autostart>  to list the ones with autostart enabled, and
>> +I<--no-autostart>  to list the ones with autostart disabled.
>> +
>> +NOTE: When talking to older servers, this command is forced to use a series of
>> +API calls with an inherent race, where a pool might not be listed or might appear
>> +more than once if it changed state between calls while the list was being
>> +collected.  Newer servers do not have this problem.
>>
>>   =item B<net-name>  I<network-UUID>
>>
>
> In the interest of having the API in the release, I would be okay with
> pushing as-is, but would rather have the questions above cleared up first.
>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list




More information about the libvir-list mailing list