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