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