On 09/05/12 07:34, Osier Yang wrote:

This is to list the node device objects, supports to filter the results by capability types.

include/libvirt/libvirt.h.in: Declare enum virConnectListAllNodeDeviceFlags and virConnectListAllNodeDevices. python/generator.py: Skip auto-generating src/driver.h: (virDrvConnectListAllNodeDevices) src/libvirt.c: Implement the public API src/libvirt_public.syms: Export the symbol to public --- include/libvirt/libvirt.h.in | 25 +++++++++++++++++ python/generator.py | 1 + src/driver.h | 4 +++ src/libvirt.c | 62 ++++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 1 + 5 files changed, 93 insertions(+), 0 deletions(-)

diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 9226f3e..96d0760 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2834,6 +2834,31 @@ int virNodeListDevices (virConnectPtr conn, char **const names, int maxnames, unsigned int flags); +/* + * virConnectListAllNodeDevices: + * + * Flags used to filter the returned node devices. Flags in each group + * are exclusive. + */ +typedef enum { + /* Reserved the first 6 bits for the possibility of persistent + * node device support in future. + */

Huh, I think reserving some bits doesn't make sense. If we add something, we might just add it at the end and nothing will happen. And if you will need to add more than 6 bits of filters, than you'll need to split it.

+ + VIR_CONNECT_LIST_NODE_DEVICES_CAP_SYSTEM = 1 << 6, + VIR_CONNECT_LIST_NODE_DEVICES_CAP_PCI_DEV = 1 << 7, + VIR_CONNECT_LIST_NODE_DEVICES_CAP_USB_DEV = 1 << 8, + VIR_CONNECT_LIST_NODE_DEVICES_CAP_USB_INTERFACE = 1 << 9, + VIR_CONNECT_LIST_NODE_DEVICES_CAP_NET = 1 << 10, + VIR_CONNECT_LIST_NODE_DEVICES_CAP_SCSI_HOST = 1 << 11, + VIR_CONNECT_LIST_NODE_DEVICES_CAP_SCSI_TARGET = 1 << 12, + VIR_CONNECT_LIST_NODE_DEVICES_CAP_SCSI = 1 << 13, + VIR_CONNECT_LIST_NODE_DEVICES_CAP_STORAGE = 1 << 14, +} virConnectListAllNodeDeviceFlags;

Also it probably would be better to add a comment to (at least some of those) flags. For example I don't know the difference between VIR_CONNECT_LIST_NODE_DEVICES_CAP_SCSI and the other two SCSI filters.

+ +int virConnectListAllNodeDevices (virConnectPtr conn, + virNodeDevicePtr **devices, + unsigned int flags);

virNodeDevicePtr virNodeDeviceLookupByName (virConnectPtr conn, const char *name); diff --git a/python/generator.py b/python/generator.py index 8f6e455..a8e4ec6 100755 --- a/python/generator.py +++ b/python/generator.py @@ -464,6 +464,7 @@ skip_function = ( 'virStoragePoolListAllVolumes', # overridden in virStoragePool.py 'virConnectListAllNetworks', # overridden in virConnect.py 'virConnectListAllInterfaces', # overridden in virConnect.py + 'virConnectListAllNodeDevices', # overridden in virConnect.py

'virStreamRecvAll', # Pure python libvirt-override-virStream.py 'virStreamSendAll', # Pure python libvirt-override-virStream.py diff --git a/src/driver.h b/src/driver.h index 518e9d4..34a94af 100644 --- a/src/driver.h +++ b/src/driver.h @@ -1483,6 +1483,9 @@ typedef int (*virDevMonListDevices)(virConnectPtr conn, char **const names, int maxnames, unsigned int flags); +typedef int (*virDevMonListAllNodeDevices)(virConnectPtr conn, + virNodeDevicePtr **devices, + unsigned int flags);

typedef virNodeDevicePtr (*virDevMonDeviceLookupByName)(virConnectPtr conn, const char *name); @@ -1516,6 +1519,7 @@ struct _virDeviceMonitor { virDrvClose close; virDevMonNumOfDevices numOfDevices; virDevMonListDevices listDevices; + virDevMonListAllNodeDevices listAllNodeDevices;

In case of this API the old one actually does list all devices as this one does and has no races, but It's horrible to use.

virDevMonDeviceLookupByName deviceLookupByName; virDevMonDeviceGetXMLDesc deviceGetXMLDesc; virDevMonDeviceGetParent deviceGetParent; diff --git a/src/libvirt.c b/src/libvirt.c index 90ed7ff..b8d0bec 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -13793,6 +13793,66 @@ error: return -1; }

+/** + * virConnectListAllNodeDevices: + * @conn: Pointer to the hypervisor connection. + * @devices: Pointer to a variable to store the array containing the node + * device objects or NULL if the list is not required (just returns + * number of node devices). + * @flags: bitwise-OR of virConnectListAllNodeDevices. + * + * Collect the list of node devices, and allocate an array to store those + * objects. + * + * Normally, all node devices are returned; however, @flags can be used to + * filter the results for a smaller list of targeted node devices. The valid + * flags are divided into groups, where each group contains bits that + * describe mutually exclusive attributes of a node device, and where all bits + * within a group describe all possible node devices. + * + * Only one group of the @flags is supported. It supports to filter the node + * devices by capability type.

I'd list the flags here even if there's just the one group.

+ * + * Returns the number of node devices found or -1 and sets @devices to NULL in + * case of error. On success, the array stored into @devices is guaranteed to + * have an extra allocated element set to NULL but not included in the return + * count, to make iteration easier. The caller is responsible for calling + * virNodeDeviceFree() on each array element, then calling free() on + * @devices. + */ +int +virConnectListAllNodeDevices(virConnectPtr conn, + virNodeDevicePtr **devices, + unsigned int flags) +{ + VIR_DEBUG("conn=%p, devices=%p, flags=%x", conn, devices, flags); + + virResetLastError(); + + if (devices) + *devices = NULL; + + if (!VIR_IS_CONNECT(conn)) { + virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__); + virDispatchError(NULL); + return -1; + } + + if (conn->deviceMonitor && + conn->deviceMonitor->listAllNodeDevices) { + int ret; + ret = conn->deviceMonitor->listAllNodeDevices(conn, devices, flags); + if (ret < 0) + goto error; + return ret; + } + + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: + virDispatchError(conn); + return -1; +}

/** * virNodeListDevices: @@ -13804,6 +13864,8 @@ error: * * Collect the list of node devices, and store their names in @names * + * For more control over the results, see virConnectListAllNodeDevices(). + * * If the optional 'cap' argument is non-NULL, then the count * will be restricted to devices with the specified capability * diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 8dda48b..5a4451b 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -558,6 +558,7 @@ LIBVIRT_0.10.2 { global: virConnectListAllInterfaces; virConnectListAllNetworks; + virConnectListAllNodeDevices; virConnectListAllStoragePools; virStoragePoolListAllVolumes; } LIBVIRT_0.10.2;

On 09/13/12 08:54, Osier Yang wrote:

This is to list the node device objects, supports to filter the results by capability types.

include/libvirt/libvirt.h.in: Declare enum virConnectListAllNodeDeviceFlags and virConnectListAllNodeDevices. python/generator.py: Skip auto-generating src/driver.h: (virDrvConnectListAllNodeDevices) src/libvirt.c: Implement the public API src/libvirt_public.syms: Export the symbol to public --- include/libvirt/libvirt.h.in | 39 +++++++++++++++++++++++ python/generator.py | 1 + src/driver.h | 4 ++ src/libvirt.c | 71 ++++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 1 + 5 files changed, 116 insertions(+), 0 deletions(-)

diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index ca04f6c..2bb39be 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2834,6 +2834,45 @@ int virNodeListDevices (virConnectPtr conn, char **const names, int maxnames, unsigned int flags); +/* + * virConnectListAllNodeDevices: + * + * Flags used to filter the returned node devices. Flags in each group + * are exclusive. Currently only one group to filter the devices by cap + * type. + */ +typedef enum { + /* System capability */ + VIR_CONNECT_LIST_NODE_DEVICES_CAP_SYSTEM = 1 << 0,

You unfortunately need to specify the comment _after_ the value, otherwise the docs generator generates the comments to wrong lines.

+ + /* PCI device */ + VIR_CONNECT_LIST_NODE_DEVICES_CAP_PCI_DEV = 1 << 1, + + /* USB device */ + VIR_CONNECT_LIST_NODE_DEVICES_CAP_USB_DEV = 1 << 2, + + /* USB interface */ + VIR_CONNECT_LIST_NODE_DEVICES_CAP_USB_INTERFACE = 1 << 3, + + /* Network device */ + VIR_CONNECT_LIST_NODE_DEVICES_CAP_NET = 1 << 4, + + /* SCSI Host Bus Adapter */ + VIR_CONNECT_LIST_NODE_DEVICES_CAP_SCSI_HOST = 1 << 5, + + /* SCSI Target */ + VIR_CONNECT_LIST_NODE_DEVICES_CAP_SCSI_TARGET = 1 << 6, + + /* SCSI device */ + VIR_CONNECT_LIST_NODE_DEVICES_CAP_SCSI = 1 << 7, + + /* Storage device */ + VIR_CONNECT_LIST_NODE_DEVICES_CAP_STORAGE = 1 << 8, +} virConnectListAllNodeDeviceFlags; + +int virConnectListAllNodeDevices (virConnectPtr conn, + virNodeDevicePtr **devices, + unsigned int flags);

virNodeDevicePtr virNodeDeviceLookupByName (virConnectPtr conn, const char *name);

Otherwise ACK as you addressed all my comments. Peter

On 09/05/12 07:34, Osier Yang wrote:

src/conf/node_device_conf.h: * New macro VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_CAP * Declare virNodeDeviceList

src/conf/node_device_conf.c: * New helpers virNodeDeviceCapMatch, virNodeDeviceMatch. virNodeDeviceCapMatch looks up the list of all the caps the device support, to see if the device support the cap type. * Implement virNodeDeviceList

src/libvirt_private.syms: * Export virNodeDeviceList * Export virNodeDevCapTypeFromString --- src/conf/node_device_conf.c | 103 +++++++++++++++++++++++++++++++++++++++++++ src/conf/node_device_conf.h | 16 +++++++ src/libvirt_private.syms | 2 + 3 files changed, 121 insertions(+), 0 deletions(-)

diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 048c70c..60462b8 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -1432,3 +1432,106 @@ void virNodeDeviceObjUnlock(virNodeDeviceObjPtr obj) { virMutexUnlock(&obj->lock); } + +static bool +virNodeDeviceCapMatch(virNodeDeviceObjPtr devobj, + int type) +{ + virNodeDevCapsDefPtr cap = NULL; + + for (cap = devobj->def->caps; cap; cap = cap->next) { + if (type == cap->type) + return true; + } + + return false; +} + +#define MATCH(FLAG) (flags & (FLAG))

I probably couldn't resist the temptation to abuse the MATCH macro to get rid of half of the lines below. But this is not worth the effort and what you have is correct.

+static bool +virNodeDeviceMatch(virNodeDeviceObjPtr devobj, + unsigned int flags) +{ + /* filter by cap type */ + if (MATCH(VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_CAP)) { + if (!((MATCH(VIR_CONNECT_LIST_NODE_DEVICES_CAP_SYSTEM) && + virNodeDeviceCapMatch(devobj, VIR_NODE_DEV_CAP_SYSTEM)) || + (MATCH(VIR_CONNECT_LIST_NODE_DEVICES_CAP_PCI_DEV) && + virNodeDeviceCapMatch(devobj, VIR_NODE_DEV_CAP_PCI_DEV)) || + (MATCH(VIR_CONNECT_LIST_NODE_DEVICES_CAP_USB_DEV) && + virNodeDeviceCapMatch(devobj, VIR_NODE_DEV_CAP_USB_DEV)) || + (MATCH(VIR_CONNECT_LIST_NODE_DEVICES_CAP_USB_INTERFACE) && + virNodeDeviceCapMatch(devobj, VIR_NODE_DEV_CAP_USB_INTERFACE)) || + (MATCH(VIR_CONNECT_LIST_NODE_DEVICES_CAP_NET) && + virNodeDeviceCapMatch(devobj, VIR_NODE_DEV_CAP_NET)) || + (MATCH(VIR_CONNECT_LIST_NODE_DEVICES_CAP_SCSI_HOST) && + virNodeDeviceCapMatch(devobj, VIR_NODE_DEV_CAP_SCSI_HOST)) || + (MATCH(VIR_CONNECT_LIST_NODE_DEVICES_CAP_SCSI_TARGET) && + virNodeDeviceCapMatch(devobj, VIR_NODE_DEV_CAP_SCSI_TARGET)) || + (MATCH(VIR_CONNECT_LIST_NODE_DEVICES_CAP_SCSI) && + virNodeDeviceCapMatch(devobj, VIR_NODE_DEV_CAP_SCSI)) || + (MATCH(VIR_CONNECT_LIST_NODE_DEVICES_CAP_STORAGE) && + virNodeDeviceCapMatch(devobj, VIR_NODE_DEV_CAP_STORAGE)))) + return false; + } + + return true; +} +#undef MATCH + +int +virNodeDeviceList(virConnectPtr conn, + virNodeDeviceObjList devobjs, + virNodeDevicePtr **devices, + unsigned int flags) +{ + virNodeDevicePtr *tmp_devices = NULL; + virNodeDevicePtr device = NULL; + int ndevices = 0; + int ret = -1; + int i; + + if (devices) { + if (VIR_ALLOC_N(tmp_devices, devobjs.count + 1) < 0) { + virReportOOMError(); + goto cleanup; + } + } + + for (i = 0; i < devobjs.count; i++) { + virNodeDeviceObjPtr devobj = devobjs.objs[i]; + virNodeDeviceObjLock(devobj); + if (virNodeDeviceMatch(devobj, flags)) { + if (devices) { + if (!(device = virGetNodeDevice(conn, + devobj->def->name))) { + virNodeDeviceObjUnlock(devobj); + goto cleanup; + } + tmp_devices[ndevices] = device; + } + ndevices++; + } + virNodeDeviceObjUnlock(devobj); + } + + if (tmp_devices) { + /* trim the array to the final size */ + ignore_value(VIR_REALLOC_N(tmp_devices, ndevices + 1)); + *devices = tmp_devices; + tmp_devices = NULL; + } + + ret = ndevices; + +cleanup: + if (tmp_devices) { + for (i = 0; i < ndevices; i++) { + if (tmp_devices[i]) + virNodeDeviceFree(tmp_devices[i]); + } + } + + VIR_FREE(tmp_devices); + return ret; +} diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h index 41c9fcc..b8ee881 100644 --- a/src/conf/node_device_conf.h +++ b/src/conf/node_device_conf.h @@ -261,4 +261,20 @@ void virNodeDevCapsDefFree(virNodeDevCapsDefPtr caps); void virNodeDeviceObjLock(virNodeDeviceObjPtr obj); void virNodeDeviceObjUnlock(virNodeDeviceObjPtr obj);

+# define VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_CAP \ + (VIR_CONNECT_LIST_NODE_DEVICES_CAP_SYSTEM | \ + VIR_CONNECT_LIST_NODE_DEVICES_CAP_PCI_DEV | \ + VIR_CONNECT_LIST_NODE_DEVICES_CAP_USB_DEV | \ + VIR_CONNECT_LIST_NODE_DEVICES_CAP_USB_INTERFACE | \ + VIR_CONNECT_LIST_NODE_DEVICES_CAP_NET | \ + VIR_CONNECT_LIST_NODE_DEVICES_CAP_SCSI_HOST | \ + VIR_CONNECT_LIST_NODE_DEVICES_CAP_SCSI_TARGET | \ + VIR_CONNECT_LIST_NODE_DEVICES_CAP_SCSI | \ + VIR_CONNECT_LIST_NODE_DEVICES_CAP_STORAGE) + +int virNodeDeviceList(virConnectPtr conn, + virNodeDeviceObjList devobjs, + virNodeDevicePtr **devices, + unsigned int flags); + #endif /* __VIR_NODE_DEVICE_CONF_H__ */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7464c59..43928f1 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -852,6 +852,7 @@ virPortGroupFindByName;

# node_device_conf.h +a;

Is this related directly to this API adition? If not, split it out. (Do a separate patch doing this change and then rebase)

virNodeDevCapTypeToString; virNodeDevCapsDefFree; virNodeDeviceAssignDef; @@ -865,6 +866,7 @@ virNodeDeviceFindBySysfsPath; virNodeDeviceGetParentHost; virNodeDeviceGetWWNs; virNodeDeviceHasCap; +virNodeDeviceList; virNodeDeviceObjListFree; virNodeDeviceObjLock; virNodeDeviceObjRemove;

ACK if you clarify/remove adding virNodeDevCapTypeFromString to symbols in this patch. Peter

On 09/05/12 07:34, 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 --- tools/virsh-nodedev.c | 302 ++++++++++++++++++++++++++++++++++++++++++------ tools/virsh.pod | 8 +- 2 files changed, 269 insertions(+), 41 deletions(-)

diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c index cd88538..2c054ea 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"

Gah this include isn't nice :/. We probably should put the filter flags in a more public header file.

/* * "nodedev-create" command @@ -138,6 +139,186 @@ 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)); +} + +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) { + vshResetLibvirtError();

This reset isn't necessary as you jump right to fallback after this.

+ 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.9.13 and older) */ + vshResetLibvirtError();

Or better, get rid of this one.

+ + 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"));

Align the second line of the string with the first please.

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

This loop would be better with a comment explaining what it does.

+ 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,70 +330,115 @@ 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) { + ncaps = vshStringToArray((char *)cap_str, &caps);

You shouldn't modify const strings.

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

- if (vshCommandOptString(cmd, "cap", &cap) <= 0) - cap = NULL; - - 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 < list->ndevices; i++) + names[i] = vshStrdup(ctl, virNodeDeviceGetName(list->devices[i]));

- 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++) { + virNodeDevicePtr dev = list->devices[i]; + if (STRNEQ(names[i], "computer")) {

You need to modify this condition to fix the bug with missing output when you specify filters. You will have to abuse virNodeDeviceGetParent() that returns NULL if it has no parent. This will probably induce a situation where multiple parents will be printed but that's right in this situation.

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++) { - if (vshTreePrint(ctl, vshNodeListLookup, - &arrays, num_devices, i) < 0) + + for (i = 0 ; i < list->ndevices; i++) { + if (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(cap_str); + VIR_FREE(caps); + vshNodeDeviceListFree(list); return ret; }

diff --git a/tools/virsh.pod b/tools/virsh.pod index c806335..8709503 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1855,9 +1855,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.

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

This patch will need a new version that will deal with finding the correct root of the trees in case "computer" isn't present in the filtered list. (And the other comments) Peter

On 2012年09月07日 06:05, Peter Krempa wrote:

On 09/05/12 07:34, 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 --- tools/virsh-nodedev.c | 302 ++++++++++++++++++++++++++++++++++++++++++------ tools/virsh.pod | 8 +- 2 files changed, 269 insertions(+), 41 deletions(-)

diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c index cd88538..2c054ea 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"

Gah this include isn't nice :/. We probably should put the filter flags in a more public header file.

I kept this, and will clean up all the internal header including later (including the ones before the atomic apis). Regards, Osier

On 09/13/12 08:56, Osier Yang wrote:

This improve helper vshStringToArray to accept const string as

s/improve/improves/

argument instead. To not convert the const string when using vshStringToArray, and thus avoid motifying it.

I'd write the last sentence as: This avoids modifying const strings when using vshStringToArray.

v4 - v5: * Except both are PATCH 6/7, totally different patches.

Remove this before pushing please.

--- tools/virsh-domain.c | 2 +- tools/virsh-pool.c | 2 +- tools/virsh.c | 10 ++++++---- tools/virsh.h | 2 +- 4 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index c6695b3..18422b7 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -2337,7 +2337,7 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd) vshUndefineVolume *vlist = NULL; int nvols = 0; const char *volumes_arg = NULL; - char *volumes = NULL; + const char *volumes = NULL;

The volumes variable will need to be removed completely with the new functionality. In cmdUndefine the volumes string was the copy of the const argument that you're trying to avoid. The unfortunate thing here is that "volumes" is used very wildly across cmdUndefine. But the cure should be easy: you need to rename volumes_arg to volumes. But this poses another problem: Your function now allocates two things, but the previous callers (that you didn't update free only one) The first one is the array of strings that is populated with the string fragments. This variable is freed normaly. The second one is the string copy that is exploded into the array. And you don't free this one. Fortunately, the way strsep and your code works has one advantage you might use: The first element in the array is always pointing to the beginning of the tokenized string (in your case to the memory you allocated for the copy). So to free this you might want to do something like: VIR_FREE(*token_var); VIR_FREE(token_var); For this I'd go with a macro that does this (and adds a check as token_var might be NULL).

char **volume_tokens = NULL; char *volume_tok = NULL; int nvolume_tokens = 0; diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c index 15d1883..2ca7a18 100644 --- a/tools/virsh-pool.c +++ b/tools/virsh-pool.c @@ -856,7 +856,7 @@ cmdPoolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) char **poolTypes = NULL; int npoolTypes = 0;

- npoolTypes = vshStringToArray((char *)type, &poolTypes); + npoolTypes = vshStringToArray(type, &poolTypes);

You'll need to call the cleanup macro or anything you implement for freeing poolTypes.

for (i = 0; i < npoolTypes; i++) { if ((poolType = virStoragePoolTypeFromString(poolTypes[i])) < 0) { diff --git a/tools/virsh.c b/tools/virsh.c index 242f789..d0b302a 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -174,19 +174,20 @@ vshPrettyCapacity(unsigned long long val, const char **unit) * on error. */ int -vshStringToArray(char *str, +vshStringToArray(const char *str, char ***array) { + char *str_copied = vshStrdup(NULL, str); char *str_tok = NULL; unsigned int nstr_tokens = 0; char **arr = NULL;

/* tokenize the string from user and save it's parts into an array */ - if (str) { + if (str_copied) { nstr_tokens = 1;

/* count the delimiters */ - str_tok = str; + str_tok = str_copied; while (*str_tok) { if (*str_tok == ',') nstr_tokens++; @@ -195,12 +196,13 @@ vshStringToArray(char *str,

if (VIR_ALLOC_N(arr, nstr_tokens) < 0) { virReportOOMError(); + VIR_FREE(str_copied); return -1; }

/* tokenize the input string */ nstr_tokens = 0; - str_tok = str; + str_tok = str_copied; do { arr[nstr_tokens] = strsep(&str_tok, ","); nstr_tokens++; diff --git a/tools/virsh.h b/tools/virsh.h index 30eff4b..1220079 100644 --- a/tools/virsh.h +++ b/tools/virsh.h @@ -330,7 +330,7 @@ int vshAskReedit(vshControl *ctl, const char *msg); int vshStreamSink(virStreamPtr st, const char *bytes, size_t nbytes, void *opaque); double vshPrettyCapacity(unsigned long long val, const char **unit); -int vshStringToArray(char *str, char ***array); +int vshStringToArray(const char *str, char ***array);

/* Typedefs, function prototypes for job progress reporting. * There are used by some long lingering commands like

A nice improvement. The rest of the code looks okay, but I'd like to see the fixed version before you push. Peter

This improve helper vshStringToArray to accept const string as argument instead. To not convert the const string when using vshStringToArray, and thus avoid motifying it. --- tools/virsh-domain.c | 7 +++---- tools/virsh-pool.c | 3 ++- tools/virsh.c | 10 ++++++---- tools/virsh.h | 2 +- 4 files changed, 12 insertions(+), 10 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index c6695b3..3ad02eb 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -2336,8 +2336,7 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd) /* list of volumes to remove along with this domain */ vshUndefineVolume *vlist = NULL; int nvols = 0; - const char *volumes_arg = NULL; - char *volumes = NULL; + const char *volumes = NULL; char **volume_tokens = NULL; char *volume_tok = NULL; int nvolume_tokens = 0; @@ -2352,8 +2351,7 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd) int nvolumes = 0; bool vol_not_found = false; - ignore_value(vshCommandOptString(cmd, "storage", &volumes_arg)); - volumes = vshStrdup(ctl, volumes_arg); + ignore_value(vshCommandOptString(cmd, "storage", &volumes)); if (managed_save) { flags |= VIR_DOMAIN_UNDEFINE_MANAGED_SAVE; @@ -2606,6 +2604,7 @@ cleanup: VIR_FREE(vlist); VIR_FREE(volumes); + VIR_FREE(*volume_tokens); VIR_FREE(volume_tokens); VIR_FREE(def); VIR_FREE(vol_nodes); diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c index bc10f76..f41d01e 100644 --- a/tools/virsh-pool.c +++ b/tools/virsh-pool.c @@ -854,7 +854,7 @@ cmdPoolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) char **poolTypes = NULL; int npoolTypes = 0; - npoolTypes = vshStringToArray((char *)type, &poolTypes); + npoolTypes = vshStringToArray(type, &poolTypes); for (i = 0; i < npoolTypes; i++) { if ((poolType = virStoragePoolTypeFromString(poolTypes[i])) < 0) { @@ -895,6 +895,7 @@ cmdPoolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) break; } } + VIR_FREE(*poolTypes); VIR_FREE(poolTypes); } diff --git a/tools/virsh.c b/tools/virsh.c index 242f789..d0b302a 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -174,19 +174,20 @@ vshPrettyCapacity(unsigned long long val, const char **unit) * on error. */ int -vshStringToArray(char *str, +vshStringToArray(const char *str, char ***array) { + char *str_copied = vshStrdup(NULL, str); char *str_tok = NULL; unsigned int nstr_tokens = 0; char **arr = NULL; /* tokenize the string from user and save it's parts into an array */ - if (str) { + if (str_copied) { nstr_tokens = 1; /* count the delimiters */ - str_tok = str; + str_tok = str_copied; while (*str_tok) { if (*str_tok == ',') nstr_tokens++; @@ -195,12 +196,13 @@ vshStringToArray(char *str, if (VIR_ALLOC_N(arr, nstr_tokens) < 0) { virReportOOMError(); + VIR_FREE(str_copied); return -1; } /* tokenize the input string */ nstr_tokens = 0; - str_tok = str; + str_tok = str_copied; do { arr[nstr_tokens] = strsep(&str_tok, ","); nstr_tokens++; diff --git a/tools/virsh.h b/tools/virsh.h index 30eff4b..1220079 100644 --- a/tools/virsh.h +++ b/tools/virsh.h @@ -330,7 +330,7 @@ int vshAskReedit(vshControl *ctl, const char *msg); int vshStreamSink(virStreamPtr st, const char *bytes, size_t nbytes, void *opaque); double vshPrettyCapacity(unsigned long long val, const char **unit); -int vshStringToArray(char *str, char ***array); +int vshStringToArray(const char *str, char ***array); /* Typedefs, function prototypes for job progress reporting. * There are used by some long lingering commands like -- 1.7.7.3

On 2012年09月15日 06:05, Peter Krempa wrote:

On 09/14/12 18:21, Osier Yang wrote:

...

This improve helper vshStringToArray to accept const string as argument instead. To not convert the const string when using vshStringToArray, and thus avoid motifying it. --- tools/virsh-domain.c | 7 +++---- tools/virsh-pool.c | 3 ++- tools/virsh.c | 10 ++++++---- tools/virsh.h | 2 +- 4 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index c6695b3..3ad02eb 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -2336,8 +2336,7 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd) /* list of volumes to remove along with this domain */ vshUndefineVolume *vlist = NULL; int nvols = 0; - const char *volumes_arg = NULL; - char *volumes = NULL; + const char *volumes = NULL; char **volume_tokens = NULL; char *volume_tok = NULL; int nvolume_tokens = 0; @@ -2352,8 +2351,7 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd) int nvolumes = 0; bool vol_not_found = false;

- ignore_value(vshCommandOptString(cmd, "storage", &volumes_arg)); - volumes = vshStrdup(ctl, volumes_arg); + ignore_value(vshCommandOptString(cmd, "storage", &volumes));

if (managed_save) { flags |= VIR_DOMAIN_UNDEFINE_MANAGED_SAVE; @@ -2606,6 +2604,7 @@ cleanup: VIR_FREE(vlist);

VIR_FREE(volumes); + VIR_FREE(*volume_tokens);

Doing this guarantees a segfault. In many cases the token array may be NULL and you dereference it. This would cause a huge regression:

Ah, I rushed. Okay, now I pushed the series with the nits fixed. Thanks for the reviewing! Regards, Osier

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