[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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.


>  
>  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).


> +}
> +
> +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.


> +
> +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.


> +    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.


> +
> +    /* 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?


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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]