[libvirt] [PATCH 7/7 v5] list: Use virConnectListAllNodeDevices in virsh
Peter Krempa
pkrempa at redhat.com
Fri Sep 14 13:55:47 UTC 2012
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
More information about the libvir-list
mailing list