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.