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

[Osier Yang]

On 2012年09月14日 21:55, Peter Krempa wrote:
> 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.

Yeah, but that's why I'd like not indent it. :-)

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

Okay.

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

Okay.

>
>>
>> =item B<nodedev-reattach> I<nodedev>
>>
>>
>
> ACK with the nits addressed.
>
> Peter