[libvirt] [PATCH 7/7] list: Use virConnectListAllNodeDevices in virsh
Peter Krempa
pkrempa at redhat.com
Thu Sep 6 22:05:17 UTC 2012
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
More information about the libvir-list
mailing list