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

Re: [libvirt] [PATCH 7/7 v5] list: Use virConnectListAllNodeDevices in virsh



On 09/13/12 08:59, Osier Yang wrote:
tools/virsh-nodedev.c:
   * vshNodeDeviceSorter to sort node devices by name

   * vshNodeDeviceListFree to free the node device objects list.

   * vshNodeDeviceListCollect to collect the node device objects, trying
     to use new API first, fall back to older APIs if it's not supported.

   * Change option --cap to accept multiple capability types.

tools/virsh.pod
   * Update document for --cap

v4 - v5:
   * Desert the change which cause the subtree is listed for --tree.
   * Error out if both --tree and --cap are specified.
   * No second error reset when fallback to old APIs.
   * Version number fix.
   * Some code styles fix.
---
  src/libvirt_private.syms |    1 +
  tools/virsh-nodedev.c    |  302 ++++++++++++++++++++++++++++++++++++++++------
  tools/virsh.pod          |    8 +-
  3 files changed, 271 insertions(+), 40 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 8e0fd47..b25a451 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -852,6 +852,7 @@ virPortGroupFindByName;


  # node_device_conf.h
+virNodeDevCapTypeFromString;

OK, now I understand this change.

  virNodeDevCapTypeToString;
  virNodeDevCapsDefFree;
  virNodeDeviceAssignDef;
diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c
index e784af1..963464f 100644
--- a/tools/virsh-nodedev.c
+++ b/tools/virsh-nodedev.c
@@ -36,6 +36,7 @@
  #include "memory.h"
  #include "util.h"
  #include "xml.h"
+#include "conf/node_device_conf.h"

  /*
   * "nodedev-create" command
@@ -138,6 +139,187 @@ vshNodeListLookup(int devid, bool parent, void *opaque)
      return arrays->names[devid];
  }

+static int
+vshNodeDeviceSorter(const void *a, const void *b)
+{
+    virNodeDevicePtr *na = (virNodeDevicePtr *) a;
+    virNodeDevicePtr *nb = (virNodeDevicePtr *) b;
+
+    if (*na && !*nb)
+        return -1;
+
+    if (!*na)
+        return *nb != NULL;
+
+    return vshStrcasecmp(virNodeDeviceGetName(*na),
+                      virNodeDeviceGetName(*nb));

Still missaligned.

+}
+
+struct vshNodeDeviceList {
+    virNodeDevicePtr *devices;
+    size_t ndevices;
+};
+typedef struct vshNodeDeviceList *vshNodeDeviceListPtr;
+
+static void
+vshNodeDeviceListFree(vshNodeDeviceListPtr list)
+{
+    int i;
+
+    if (list && list->ndevices) {
+        for (i = 0; i < list->ndevices; i++) {
+            if (list->devices[i])
+                virNodeDeviceFree(list->devices[i]);
+        }
+        VIR_FREE(list->devices);
+    }
+    VIR_FREE(list);
+}
+
+static vshNodeDeviceListPtr
+vshNodeDeviceListCollect(vshControl *ctl,
+                         char **capnames,
+                         int ncapnames,
+                         unsigned int flags)
+{
+    vshNodeDeviceListPtr list = vshMalloc(ctl, sizeof(*list));
+    int i;
+    int ret;
+    virNodeDevicePtr device;
+    bool success = false;
+    size_t deleted = 0;
+    int ndevices = 0;
+    char **names = NULL;
+
+    /* try the list with flags support (0.10.2 and later) */
+    if ((ret = virConnectListAllNodeDevices(ctl->conn,
+                                            &list->devices,
+                                            flags)) >= 0) {
+        list->ndevices = ret;
+        goto finished;
+    }
+
+    /* check if the command is actually supported */
+    if (last_error && last_error->code == VIR_ERR_NO_SUPPORT)
+        goto fallback;
+
+    /* there was an error during the call */
+    vshError(ctl, "%s", _("Failed to list node devices"));
+    goto cleanup;
+
+
+fallback:
+    /* fall back to old method (0.10.1 and older) */
+    vshResetLibvirtError();
+
+    ndevices = virNodeNumOfDevices(ctl->conn, NULL, 0);
+    if (ndevices < 0) {
+        vshError(ctl, "%s", _("Failed to count node devices"));
+        goto cleanup;
+    }
+
+    if (ndevices == 0)
+        return list;
+
+    names = vshMalloc(ctl, sizeof(char *) * ndevices);
+
+    ndevices = virNodeListDevices(ctl->conn, NULL, names, ndevices, 0);
+    if (ndevices < 0) {
+        vshError(ctl, "%s", _("Failed to list node devices"));
+        goto cleanup;
+    }
+
+    list->devices = vshMalloc(ctl, sizeof(virNodeDevicePtr) * (ndevices));
+    list->ndevices = 0;
+
+    /* get the node devices */
+    for (i = 0; i < ndevices ; i++) {
+        if (!(device = virNodeDeviceLookupByName(ctl->conn, names[i])))
+            continue;
+        list->devices[list->ndevices++] = device;
+    }
+
+    /* truncate domains that weren't found */
+    deleted = ndevices - list->ndevices;
+
+    if (!capnames)
+        goto finished;
+
+    /* filter the list if the list was acquired by fallback means */
+    for (i = 0; i < list->ndevices; i++) {
+        char **caps = NULL;
+        int ncaps = 0;
+        bool match = false;
+
+        device = list->devices[i];
+
+        if ((ncaps = virNodeDeviceNumOfCaps(device)) < 0) {
+            vshError(ctl, "%s", _("Failed to get capability numbers "
+                                  "of the device"));
+            goto cleanup;
+        }
+
+        caps = vshMalloc(ctl, sizeof(char *) * ncaps);
+
+        if ((ncaps = virNodeDeviceListCaps(device, caps, ncaps)) < 0) {
+            vshError(ctl, "%s", _("Failed to get capability names of the device"));
+            VIR_FREE(caps);
+            goto cleanup;
+        }
+
+        /* Check if the device's capability matches with provied
+         * capabilities.
+         */
+        int j, k;
+        for (j = 0; j < ncaps; j++) {
+            for (k = 0; k < ncapnames; k++) {
+                if (STREQ(caps[j], capnames[k])) {
+                    match = true;
+                    break;
+                }
+            }
+        }
+
+        VIR_FREE(caps);
+
+        if (!match)
+            goto remove_entry;
+
+        /* the device matched all filters, it may stay */
+        continue;
+
+remove_entry:
+        /* the device has to be removed as it failed one of the filters */
+        virNodeDeviceFree(list->devices[i]);
+        list->devices[i] = NULL;
+        deleted++;
+    }
+
+finished:
+    /* sort the list */
+    if (list->devices && list->ndevices)
+        qsort(list->devices, list->ndevices,
+              sizeof(*list->devices), vshNodeDeviceSorter);
+
+    /* truncate the list if filter simulation deleted entries */
+    if (deleted)
+        VIR_SHRINK_N(list->devices, list->ndevices, deleted);
+
+    success = true;
+
+cleanup:
+    for (i = 0; i < ndevices; i++)
+        VIR_FREE(names[i]);
+    VIR_FREE(names);
+
+    if (!success) {
+        vshNodeDeviceListFree(list);
+        list = NULL;
+    }
+
+    return list;
+}
+
  /*
   * "nodedev-list" command
   */
@@ -149,71 +331,117 @@ static const vshCmdInfo info_node_list_devices[] = {

  static const vshCmdOptDef opts_node_list_devices[] = {
      {"tree", VSH_OT_BOOL, 0, N_("list devices in a tree")},
-    {"cap", VSH_OT_STRING, VSH_OFLAG_NONE, N_("capability name")},
+    {"cap", VSH_OT_STRING, VSH_OFLAG_NONE, N_("capability names, separated by comma")},
      {NULL, 0, 0, NULL}
  };

  static bool
  cmdNodeListDevices(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED)
  {
-    const char *cap = NULL;
-    char **devices;
-    int num_devices, i;
+    const char *cap_str = NULL;
+    int i;
      bool tree = vshCommandOptBool(cmd, "tree");
      bool ret = true;
+    unsigned int flags = 0;
+    char **caps = NULL;
+    int ncaps = 0;
+    vshNodeDeviceListPtr list = NULL;
+    int cap_type = -1;
+
+    ignore_value(vshCommandOptString(cmd, "cap", &cap_str));
+
+    if (cap_str) {
+        if (tree) {
+            vshError(ctl, "%s", _("Options --tree and --cap are incompatible"));
+            return -1;

The function has type bool, so you should "return false" here.

+        }
+        ncaps = vshStringToArray(cap_str, &caps);
+    }

-    if (vshCommandOptString(cmd, "cap", &cap) <= 0)
-        cap = NULL;
+    for (i = 0; i < ncaps; i++) {

This loop can be included into the if (caps_str) block above as ncaps will never be non-zero if that block didn't fire.

+        if ((cap_type = virNodeDevCapTypeFromString(caps[i])) < 0) {
+            vshError(ctl, "%s", _("Invalid capability type"));
+            VIR_FREE(caps);
+            return false;
+        }

-    num_devices = virNodeNumOfDevices(ctl->conn, cap, 0);
-    if (num_devices < 0) {
-        vshError(ctl, "%s", _("Failed to count node devices"));
-        return false;
-    } else if (num_devices == 0) {
-        return true;
+        switch(cap_type) {
+        case VIR_NODE_DEV_CAP_SYSTEM:
+            flags |= VIR_CONNECT_LIST_NODE_DEVICES_CAP_SYSTEM;
+            break;
+        case VIR_NODE_DEV_CAP_PCI_DEV:
+            flags |= VIR_CONNECT_LIST_NODE_DEVICES_CAP_PCI_DEV;
+            break;
+        case VIR_NODE_DEV_CAP_USB_DEV:
+            flags |= VIR_CONNECT_LIST_NODE_DEVICES_CAP_USB_DEV;
+            break;
+        case VIR_NODE_DEV_CAP_USB_INTERFACE:
+            flags |= VIR_CONNECT_LIST_NODE_DEVICES_CAP_USB_INTERFACE;
+            break;
+        case VIR_NODE_DEV_CAP_NET:
+            flags |= VIR_CONNECT_LIST_NODE_DEVICES_CAP_NET;
+            break;
+        case VIR_NODE_DEV_CAP_SCSI_HOST:
+            flags |= VIR_CONNECT_LIST_NODE_DEVICES_CAP_SCSI_HOST;
+            break;
+        case VIR_NODE_DEV_CAP_SCSI_TARGET:
+            flags |= VIR_CONNECT_LIST_NODE_DEVICES_CAP_SCSI_TARGET;
+            break;
+        case VIR_NODE_DEV_CAP_SCSI:
+            flags |= VIR_CONNECT_LIST_NODE_DEVICES_CAP_SCSI;
+            break;
+        case VIR_NODE_DEV_CAP_STORAGE:
+            flags |= VIR_CONNECT_LIST_NODE_DEVICES_CAP_STORAGE;
+            break;
+        default:
+            break;
+        }
      }

-    devices = vshMalloc(ctl, sizeof(char *) * num_devices);
-    num_devices =
-        virNodeListDevices(ctl->conn, cap, devices, num_devices, 0);
-    if (num_devices < 0) {
-        vshError(ctl, "%s", _("Failed to list node devices"));
-        VIR_FREE(devices);
-        return false;
+    if (!(list = vshNodeDeviceListCollect(ctl, caps, ncaps, flags))) {
+        ret = false;
+        goto cleanup;
      }
-    qsort(&devices[0], num_devices, sizeof(char*), vshNameSorter);
+
      if (tree) {
-        char **parents = vshMalloc(ctl, sizeof(char *) * num_devices);
-        struct vshNodeList arrays = { devices, parents };
+        char **parents = vshMalloc(ctl, sizeof(char *) * list->ndevices);
+        char **names = vshMalloc(ctl, sizeof(char *) * list->ndevices);
+        struct vshNodeList arrays = { names, parents };

-        for (i = 0; i < num_devices; i++) {
-            virNodeDevicePtr dev = virNodeDeviceLookupByName(ctl->conn, devices[i]);
-            if (dev && STRNEQ(devices[i], "computer")) {
+        for (i = 0; i < list->ndevices; i++)
+            names[i] = vshStrdup(ctl, virNodeDeviceGetName(list->devices[i]));
+
+        for (i = 0; i < list->ndevices; i++) {
+            virNodeDevicePtr dev = list->devices[i];
+            if (STRNEQ(names[i], "computer")) {
                  const char *parent = virNodeDeviceGetParent(dev);
                  parents[i] = parent ? vshStrdup(ctl, parent) : NULL;
              } else {
                  parents[i] = NULL;
              }
-            virNodeDeviceFree(dev);
          }
-        for (i = 0 ; i < num_devices ; i++) {
+
+        for (i = 0 ; i < list->ndevices; i++) {
              if (parents[i] == NULL &&
-                vshTreePrint(ctl, vshNodeListLookup, &arrays, num_devices,
-                             i) < 0)
+                vshTreePrint(ctl, vshNodeListLookup, &arrays,
+                             list->ndevices, i) < 0)
                  ret = false;
          }
-        for (i = 0 ; i < num_devices ; i++) {
-            VIR_FREE(devices[i]);
+
+        for (i = 0 ; i < list->ndevices; i++)
              VIR_FREE(parents[i]);
-        }
          VIR_FREE(parents);
+        for (i = 0; i < list->ndevices; i++)
+            VIR_FREE(names[i]);
+        VIR_FREE(names);
      } else {
-        for (i = 0; i < num_devices; i++) {
-            vshPrint(ctl, "%s\n", devices[i]);
-            VIR_FREE(devices[i]);
-        }
+        for (i = 0; i < list->ndevices; i++)
+            vshPrint(ctl, "%s\n", virNodeDeviceGetName(list->devices[i]));
      }
-    VIR_FREE(devices);
+
+cleanup:
+    VIR_FREE(caps);

You'll need to free the "caps" string list here.

+    vshNodeDeviceListFree(list);
      return ret;
  }

diff --git a/tools/virsh.pod b/tools/virsh.pod
index 559e64d..a6390c2 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -1856,9 +1856,11 @@ libvirt (such as whether device reset is supported).
  =item B<nodedev-list> I<cap> I<--tree>

  List all of the devices available on the node that are known by libvirt.
-If I<cap> is used, the list is filtered to show only the nodes that
-include the given capability.  If I<--tree> is used, the output is
-formatted in a tree representing parents of each node.
+I<cap> is used to filter the list by capability types, the types must be
+separated by comma, e.g. --cap pci,scsi, valid capability types include
+'system', 'pci', 'usb_device', 'usb', 'net', 'scsi_host', 'scsi_target',
+'scsi', 'storage'. If I<--tree> is used, the output is formatted in a tree
+representing parents of each node.

Nice addition. You also could state that --tree and --cap are mutually exclusive.


  =item B<nodedev-reattach> I<nodedev>



ACK with the nits addressed.

Peter


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