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


Yes, that's same problem in the whole series. I will cleanup later.



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

Okay. :-)



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


I still think it should be the work of the client apps (e.g. virsh
here).


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

Okay. Will update when pushing.



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

Okay. Will ... pushing.



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


Yes, we need to consider the existed script. But compared to the old
output, we only add a new field "Persistent" at the end, not breaking
the old field sequense.

>> -    vshPrintExtra(ctl, "%-20s %-10s %s\n", _("Name"), _("State"),
>> -                  _("Autostart"));
>> -    vshPrintExtra(ctl, "-----------------------------------------\n");

So I think it's fine.



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


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