[libvirt] [PATCH 5/6] list: Use virStoragePoolListAllVolumes in virsh
Peter Krempa
pkrempa at redhat.com
Fri Sep 7 13:46:38 UTC 2012
On 09/04/12 17:32, Osier Yang wrote:
> tools/virsh-volume.c:
> * vshStorageVolSorter to sort storage vols by name
>
> * vshStorageVolumeListFree to free the volume objects list
>
> * vshStorageVolumeListCollect to collect the volume objects, trying
> to use new API first, fall back to older APIs if it's not supported.
> ---
> tools/virsh-volume.c | 197 ++++++++++++++++++++++++++++++++++++++------------
> 1 files changed, 150 insertions(+), 47 deletions(-)
>
> diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c
> index 6ab271e..ec0b5b0 100644
> --- a/tools/virsh-volume.c
> +++ b/tools/virsh-volume.c
> @@ -966,6 +966,133 @@ cmdVolDumpXML(vshControl *ctl, const vshCmd *cmd)
> return ret;
> }
>
> +static int
> +vshStorageVolSorter(const void *a, const void *b)
> +{
> + virStorageVolPtr *va = (virStorageVolPtr *) a;
> + virStorageVolPtr *vb = (virStorageVolPtr *) b;
> +
> + if (*va && !*vb)
> + return -1;
> +
> + if (!*va)
> + return *vb != NULL;
> +
> + return vshStrcasecmp(virStorageVolGetName(*va),
> + virStorageVolGetName(*vb));
Missaligned.
> +}
> +
> +struct vshStorageVolList {
> + virStorageVolPtr *vols;
> + size_t nvols;
> +};
> +typedef struct vshStorageVolList *vshStorageVolListPtr;
> +
> +static void
> +vshStorageVolListFree(vshStorageVolListPtr list)
> +{
> + int i;
> +
> + if (list && list->vols) {
> + for (i = 0; i < list->nvols; i++) {
> + if (list->vols[i])
> + virStorageVolFree(list->vols[i]);
> + }
> + VIR_FREE(list->vols);
> + }
> + VIR_FREE(list);
> +}
> +
> +static vshStorageVolListPtr
> +vshStorageVolListCollect(vshControl *ctl,
> + virStoragePoolPtr pool,
> + unsigned int flags)
> +{
> + vshStorageVolListPtr list = vshMalloc(ctl, sizeof(*list));
> + int i;
> + char **names = NULL;
> + virStorageVolPtr vol = NULL;
> + bool success = false;
> + size_t deleted = 0;
> + int nvols = 0;
> + int ret = -1;
> +
> + /* try the list with flags support (0.10.0 and later) */
> + if ((ret = virStoragePoolListAllVolumes(pool,
> + &list->vols,
> + flags)) >= 0) {
> + list->nvols = ret;
> + goto finished;
> + }
> +
> + /* check if the command is actually supported */
> + if (last_error && last_error->code == VIR_ERR_NO_SUPPORT) {
> + vshResetLibvirtError();
> + goto fallback;
> + }
> +
> + /* there was an error during the call */
> + vshError(ctl, "%s", _("Failed to list volumes"));
> + goto cleanup;
> +
> +fallback:
> + /* fall back to old method (0.9.13 and older) */
s/0.9.13/0.10.2/
> + vshResetLibvirtError();
This error reset is not necessary as you don't have two fallback paths
as in domain listing.
> +
> + /* Determine the number of volumes in the pool */
> + if ((nvols = virStoragePoolNumOfVolumes(pool)) < 0) {
> + vshError(ctl, "%s", _("Failed to list storage volumes"));
> + goto cleanup;
> + }
> +
> + if (nvols == 0) {
> + success = true;
> + return list;
> + }
> +
> + /* Retrieve the list of volume names in the pool */
> + names = vshCalloc(ctl, nvols, sizeof(*names));
> + if ((nvols = virStoragePoolListVolumes(pool, names, nvols)) < 0) {
> + vshError(ctl, "%s", _("Failed to list storage volumes"));
> + goto cleanup;
> + }
> +
> + list->vols = vshMalloc(ctl, sizeof(virStorageVolPtr) * (nvols));
> + list->nvols = 0;
> +
> + /* get the vols */
> + for (i = 0; i < nvols; i++) {
> + if (!(vol = virStorageVolLookupByName(pool, names[i])))
> + continue;
> + list->vols[list->nvols++] = vol;
> + }
> +
> + /* truncate the list for not found vols */
> + deleted = nvols - list->nvols;
> +
> +finished:
> + /* sort the list */
> + if (list->vols && list->nvols)
> + qsort(list->vols, list->nvols, sizeof(*list->vols), vshStorageVolSorter);
> +
> + if (deleted)
> + VIR_SHRINK_N(list->vols, list->nvols, deleted);
> +
> + success = true;
> +
> +cleanup:
> + for (i = 0; i < nvols; i++)
> + VIR_FREE(names[i]);
> + VIR_FREE(names);
> +
> + if (!success) {
> + vshStorageVolListFree(list);
> + list = NULL;
> + }
> +
> + return list;
> +}
> +
> /*
> * "vol-list" command
> */
> @@ -986,14 +1113,13 @@ cmdVolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED)
> {
> virStorageVolInfo volumeInfo;
> virStoragePoolPtr pool;
> - char **activeNames = NULL;
> char *outputStr = NULL;
> const char *unit;
> double val;
> bool details = vshCommandOptBool(cmd, "details");
> - int numVolumes = 0, i;
> + int i;
> int ret;
> - bool functionReturn;
> + bool functionReturn = false;
> int stringLength = 0;
> size_t allocStrLength = 0, capStrLength = 0;
> size_t nameStrLength = 0, pathStrLength = 0;
> @@ -1005,43 +1131,22 @@ cmdVolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED)
> char *type;
> };
> struct volInfoText *volInfoTexts = NULL;
> + vshStorageVolListPtr list = NULL;
>
> /* Look up the pool information given to us by the user */
> if (!(pool = vshCommandOptPool(ctl, cmd, "pool", NULL)))
> return false;
>
> - /* Determine the number of volumes in the pool */
> - numVolumes = virStoragePoolNumOfVolumes(pool);
> -
> - if (numVolumes < 0) {
> - vshError(ctl, "%s", _("Failed to list storage volumes"));
> - virStoragePoolFree(pool);
> - return false;
> - }
> -
> - /* Retrieve the list of volume names in the pool */
> - if (numVolumes > 0) {
> - activeNames = vshCalloc(ctl, numVolumes, sizeof(*activeNames));
> - if ((numVolumes = virStoragePoolListVolumes(pool, activeNames,
> - numVolumes)) < 0) {
> - vshError(ctl, "%s", _("Failed to list active vols"));
> - VIR_FREE(activeNames);
> - virStoragePoolFree(pool);
> - return false;
> - }
> -
> - /* Sort the volume names */
> - qsort(&activeNames[0], numVolumes, sizeof(*activeNames), vshNameSorter);
> + if (!(list = vshStorageVolListCollect(ctl, pool, 0)))
> + goto cleanup;
>
> - /* Set aside memory for volume information pointers */
> - volInfoTexts = vshCalloc(ctl, numVolumes, sizeof(*volInfoTexts));
> - }
> + if (list->nvols > 0)
> + volInfoTexts = vshCalloc(ctl, list->nvols, sizeof(*volInfoTexts));
>
> /* Collect the rest of the volume information for display */
> - for (i = 0; i < numVolumes; i++) {
> + for (i = 0; i < list->nvols; i++) {
> /* Retrieve volume info */
> - virStorageVolPtr vol = virStorageVolLookupByName(pool,
> - activeNames[i]);
> + virStorageVolPtr vol = list->vols[i];
>
> /* Retrieve the volume path */
> if ((volInfoTexts[i].path = virStorageVolGetPath(vol)) == NULL) {
> @@ -1099,7 +1204,7 @@ cmdVolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED)
> */
>
> /* Keep the length of name string if longest so far */
> - stringLength = strlen(activeNames[i]);
> + stringLength = strlen(virStorageVolGetName(list->vols[i]));
> if (stringLength > nameStrLength)
> nameStrLength = stringLength;
>
> @@ -1123,9 +1228,6 @@ cmdVolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED)
> if (stringLength > allocStrLength)
> allocStrLength = stringLength;
> }
> -
> - /* Cleanup memory allocation */
> - virStorageVolFree(vol);
> }
>
> /* If the --details option wasn't selected, we output the volume
> @@ -1138,8 +1240,8 @@ cmdVolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED)
> /* The old output format */
> vshPrintExtra(ctl, "%-20s %-40s\n", _("Name"), _("Path"));
> vshPrintExtra(ctl, "-----------------------------------------\n");
> - for (i = 0; i < numVolumes; i++) {
> - vshPrint(ctl, "%-20s %-40s\n", activeNames[i],
> + for (i = 0; i < list->nvols; i++) {
> + vshPrint(ctl, "%-20s %-40s\n", virStorageVolGetName(list->vols[i]),
> volInfoTexts[i].path);
> }
>
> @@ -1210,9 +1312,9 @@ cmdVolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED)
> vshPrintExtra(ctl, "\n");
>
> /* Display the volume info rows */
> - for (i = 0; i < numVolumes; i++) {
> + for (i = 0; i < list->nvols; i++) {
> vshPrint(ctl, outputStr,
> - activeNames[i],
> + virStorageVolGetName(list->vols[i]),
> volInfoTexts[i].path,
> volInfoTexts[i].type,
> volInfoTexts[i].capacity,
> @@ -1240,20 +1342,21 @@ asprintf_failure:
> cleanup:
>
> /* Safely free the memory allocated in this function */
> - for (i = 0; i < numVolumes; i++) {
> - /* Cleanup the memory for one volume info structure per loop */
> - VIR_FREE(volInfoTexts[i].path);
> - VIR_FREE(volInfoTexts[i].type);
> - VIR_FREE(volInfoTexts[i].capacity);
> - VIR_FREE(volInfoTexts[i].allocation);
> - VIR_FREE(activeNames[i]);
> + if (list && list->nvols) {
> + for (i = 0; i < list->nvols; i++) {
> + /* Cleanup the memory for one volume info structure per loop */
> + VIR_FREE(volInfoTexts[i].path);
> + VIR_FREE(volInfoTexts[i].type);
> + VIR_FREE(volInfoTexts[i].capacity);
> + VIR_FREE(volInfoTexts[i].allocation);
> + }
> }
>
> /* Cleanup remaining memory */
> VIR_FREE(outputStr);
> VIR_FREE(volInfoTexts);
> - VIR_FREE(activeNames);
> virStoragePoolFree(pool);
> + vshStorageVolListFree(list);
>
> /* Return the desired value */
> return functionReturn;
>
Looks like a sane replacement. ACK.
Peter
More information about the libvir-list
mailing list