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.