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



  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.

Hope I get the questions cleared up, :-)

I pushed the set, but I'm ready to work if problems/questions
are still existed.

Thanks for the reviewing!

Osier


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