[libvirt] [PATCHv2 2/2] virsh: add new --details option to vol-list
This patch adds a new --details option to the virsh vol-list command, making its output more useful to people who use virsh for significant lengths of time. Addresses BZ # 605543 https://bugzilla.redhat.com/show_bug.cgi?id=605543 --- This new version of the patch uses the existing virsh output format when the --details option isn't given, maintaining backwards compatibility for existing scripts. When the new --details option is given though, the additional info is displayed and all columns are sized to their widest string. Output from the new option (hopefully this doesn't wrap): virsh # vol-list default Name Path ----------------------------------------- CentOS-5.5-x86_64-bin-DVD-1of2.iso /var/lib/libvirt/images/CentOS-5.5-x86_64-bin-DVD-1of2.iso CentOS-5.5-x86_64-bin-DVD-2of2.iso /var/lib/libvirt/images/CentOS-5.5-x86_64-bin-DVD-2of2.iso virsh # vol-list default --details Name Path Type Capacity Allocation --------------------------------------------------------------------------------------------------------------------------- CentOS-5.5-x86_64-bin-DVD-1of2.iso /var/lib/libvirt/images/CentOS-5.5-x86_64-bin-DVD-1of2.iso file 4.09 GB 4.10 GB CentOS-5.5-x86_64-bin-DVD-2of2.iso /var/lib/libvirt/images/CentOS-5.5-x86_64-bin-DVD-2of2.iso file 412.33 MB 412.74 MB virsh # vol-list tmp Name Path ----------------------------------------- disk1.img /tmp/images/disk1.img disk2.img /tmp/images/disk2.img disk3.img /tmp/images/disk3.img disk4.img /tmp/images/disk4.img disk5.img /tmp/images/disk5.img disk6.img /tmp/images/disk6.img virsh # vol-list tmp --details Name Path Type Capacity Allocation ------------------------------------------------------------ disk1.img /tmp/images/disk1.img file 20.00 GB 136.00 KB disk2.img /tmp/images/disk2.img file 20.00 GB 136.00 KB disk3.img /tmp/images/disk3.img file 20.00 GB 136.00 KB disk4.img /tmp/images/disk4.img file 20.00 GB 136.00 KB disk5.img /tmp/images/disk5.img file 20.00 GB 136.00 KB disk6.img /tmp/images/disk6.img file 20.00 GB 136.00 KB virsh # Much nicer to use when pools have a bunch of luns in them. :) tools/virsh.c | 225 ++++++++++++++++++++++++++++++++++++++++++++++++------ tools/virsh.pod | 4 +- 2 files changed, 203 insertions(+), 26 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 7261d19..2a9c353 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -6075,67 +6075,242 @@ static const vshCmdInfo info_vol_list[] = { static const vshCmdOptDef opts_vol_list[] = { {"pool", VSH_OT_DATA, VSH_OFLAG_REQ, N_("pool name or uuid")}, + {"details", VSH_OT_BOOL, 0, N_("display extended details for volumes")}, {NULL, 0, 0, NULL} }; static int cmdVolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) { + virStorageVolInfo volumeInfo; virStoragePoolPtr pool; - int maxactive = 0, i; + int details = vshCommandOptBool(cmd, "details"); + int maxAlloc = 0, maxCap = 0, maxName = 0; + int maxPath = 0, maxType = 0; + int numVolumes = 0, i; + int stringLength = 0; + double val; + const char *unit; char **activeNames = NULL; + struct volInfoText { + char *allocation; + char *capacity; + char *path; + char *type; + }; + struct volInfoText **volInfoTexts; + /* Check the connection to libvirtd daemon is still working */ if (!vshConnectionUsability(ctl, ctl->conn, TRUE)) return FALSE; + /* Look up the pool information given to us by the user */ if (!(pool = vshCommandOptPool(ctl, cmd, "pool", NULL))) return FALSE; - maxactive = virStoragePoolNumOfVolumes(pool); - if (maxactive < 0) { + /* Determine the number of volumes in the pool */ + numVolumes = virStoragePoolNumOfVolumes(pool); + if (numVolumes < 0) { virStoragePoolFree(pool); vshError(ctl, "%s", _("Failed to list active vols")); return FALSE; } - if (maxactive) { - activeNames = vshMalloc(ctl, sizeof(char *) * maxactive); - if ((maxactive = virStoragePoolListVolumes(pool, activeNames, - maxactive)) < 0) { + /* Retrieve the list of volume names in the pool */ + if (numVolumes) { + activeNames = vshMalloc(ctl, sizeof(char *) * numVolumes); + if ((numVolumes = virStoragePoolListVolumes(pool, activeNames, + numVolumes)) < 0) { vshError(ctl, "%s", _("Failed to list active vols")); VIR_FREE(activeNames); virStoragePoolFree(pool); return FALSE; } - qsort(&activeNames[0], maxactive, sizeof(char *), namesorter); + /* Sort the volume names */ + qsort(&activeNames[0], numVolumes, sizeof(char *), namesorter); } - vshPrintExtra(ctl, "%-20s %-40s\n", _("Name"), _("Path")); - vshPrintExtra(ctl, "-----------------------------------------\n"); - for (i = 0; i < maxactive; i++) { - virStorageVolPtr vol = virStorageVolLookupByName(pool, activeNames[i]); - char *path; + /* Set aside memory for volume information pointers */ + volInfoTexts = vshMalloc(ctl, sizeof(struct volInfoText *) * numVolumes); - /* this kind of work with vols is not atomic operation */ - if (!vol) { - VIR_FREE(activeNames[i]); - continue; + /* Collect the rest of the volume information for display */ + for (i = 0; i < numVolumes; i++) { + /* Retrieve volume info */ + virStorageVolPtr vol = virStorageVolLookupByName(pool, + activeNames[i]); + + /* Allocate memory for one row of volume info */ + volInfoTexts[i] = vshMalloc(ctl, sizeof(struct volInfoText)); + + /* Retrieve the volume path */ + if ((volInfoTexts[i]->path = virStorageVolGetPath(vol)) == NULL) { + /* Something went wrong retrieving a volume path, cope with it */ + volInfoTexts[i]->path = vshStrdup(ctl, _("unknown")); } - if ((path = virStorageVolGetPath(vol)) == NULL) { - virStorageVolFree(vol); - continue; + /* Retrieve volume type and sizing information */ + if (virStorageVolGetInfo(vol, &volumeInfo) != 0) { + /* Something went wrong retrieving volume info, cope with it */ + volInfoTexts[i]->allocation = vshStrdup(ctl, _("unknown")); + volInfoTexts[i]->capacity = vshStrdup(ctl, _("unknown")); + volInfoTexts[i]->type = vshStrdup(ctl, _("unknown")); + } else { + /* Convert the returned volume info into output strings */ + virBuffer bufStr = VIR_BUFFER_INITIALIZER; + + /* Volume type */ + if (volumeInfo.type == VIR_STORAGE_VOL_FILE) + volInfoTexts[i]->type = vshStrdup(ctl, _("file")); + else + volInfoTexts[i]->type = vshStrdup(ctl, _("block")); + + // The capacity value to output + val = prettyCapacity(volumeInfo.capacity, &unit); + virBufferVSprintf(&bufStr, "%.2lf %s", val, unit); + volInfoTexts[i]->capacity = + vshStrdup(ctl, virBufferContentAndReset(&bufStr)); + + // The allocation value to output + val = prettyCapacity(volumeInfo.allocation, &unit); + virBufferVSprintf(&bufStr, "%.2lf %s", val, unit); + volInfoTexts[i]->allocation = + vshStrdup(ctl, virBufferContentAndReset(&bufStr)); } + /** Remember the longest output size of each string, ** + ** so we can use a printf style output format template ** + ** later on for both the header and volume info rows **/ + + /* Keep the length of name string if longest so far */ + stringLength = strlen(activeNames[i]); + if (stringLength > maxName) + maxName = stringLength; - vshPrint(ctl, "%-20s %-40s\n", - virStorageVolGetName(vol), - path); - VIR_FREE(path); + /* Keep the length of path string if longest so far */ + stringLength = strlen(volInfoTexts[i]->path); + if (stringLength > maxPath) + maxPath = stringLength; + + /* Keep the length of type string if longest so far */ + stringLength = strlen(volInfoTexts[i]->type); + if (stringLength > maxType) + maxType = stringLength; + + /* Keep the length of capacity string if longest so far */ + stringLength = strlen(volInfoTexts[i]->capacity); + if (stringLength > maxCap) + maxCap = stringLength; + + /* Keep the length of allocation string if longest so far */ + stringLength = strlen(volInfoTexts[i]->allocation); + if (stringLength > maxAlloc) + maxAlloc = stringLength; + + /* Cleanup memory allocation */ virStorageVolFree(vol); - VIR_FREE(activeNames[i]); } + + /** If the --details option wasn't selected, we output the volume ** + ** info using the fixed string format from previous versions to ** + ** maintain backward compatibility. **/ + + /* Output basic info then return if --details option not selected */ + if (!details) { + /* 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], + volInfoTexts[i]->path); + + /* Cleanup the memory for this volume row */ + VIR_FREE(volInfoTexts[i]->path); + VIR_FREE(volInfoTexts[i]->type); + VIR_FREE(volInfoTexts[i]->capacity); + VIR_FREE(volInfoTexts[i]->allocation); + VIR_FREE(volInfoTexts[i]); + } + + /* Cleanup remaining memory and return */ + VIR_FREE(volInfoTexts); + VIR_FREE(activeNames); + virStoragePoolFree(pool); + return TRUE; + } + + /** We only get here if the --details option was selected. ** + ** Column now resize to the longest string to be output. **/ + + /* Determine the length of the header strings. These must be + * calculated because we may be outputing a translated heading + */ + /* Use the length of name header string if it's longest */ + stringLength = strlen(_("Name")); + if (stringLength > maxName) + maxName = stringLength; + + /* Use the length of path header string if it's longest */ + stringLength = strlen(_("Path")); + if (stringLength > maxPath) + maxPath = stringLength; + + /* Use the length of type header string if it's longest */ + stringLength = strlen(_("Type")); + if (stringLength > maxType) + maxType = stringLength; + + /* Use the length of capacity header string if it's longest */ + stringLength = strlen(_("Capacity")); + if (stringLength > maxCap) + maxCap = stringLength; + + /* Use the length of allocation header string if it's longest */ + stringLength = strlen(_("Allocation")); + if (stringLength > maxAlloc) + maxAlloc = stringLength; + + /* Display the string lengths for debugging */ + vshDebug(ctl, 5, "Longest name string = %d chars\n", maxName); + vshDebug(ctl, 5, "Longest path string = %d chars\n", maxPath); + vshDebug(ctl, 5, "Longest type string = %d chars\n", maxType); + vshDebug(ctl, 5, "Longest capacity string = %d chars\n", maxCap); + vshDebug(ctl, 5, "Longest allocation string = %d chars\n", maxAlloc); + + /* Create the output template */ + char *outputStr; + virBuffer bufStr = VIR_BUFFER_INITIALIZER; + virBufferVSprintf(&bufStr, "%%-%us %%-%us %%-%us %%-%us %%-%us\n", + maxName, maxPath, maxType, maxCap, maxAlloc); + outputStr = virBufferContentAndReset(&bufStr); + + /* Display the header */ + vshPrint(ctl, outputStr, _("Name"), _("Path"), _("Type"), + ("Capacity"), _("Allocation")); + for (i = maxName + maxPath + maxType + maxCap + maxAlloc + 8; i > 0; i--) + vshPrintExtra(ctl, "-"); + vshPrintExtra(ctl, "\n"); + + /* Display the volume info rows */ + for (i = 0; i < numVolumes; i++) { + vshPrint(ctl, outputStr, + activeNames[i], + volInfoTexts[i]->path, + volInfoTexts[i]->type, + volInfoTexts[i]->capacity, + volInfoTexts[i]->allocation); + + /* Cleanup the memory for this volume row */ + VIR_FREE(volInfoTexts[i]->path); + VIR_FREE(volInfoTexts[i]->type); + VIR_FREE(volInfoTexts[i]->capacity); + VIR_FREE(volInfoTexts[i]->allocation); + VIR_FREE(volInfoTexts[i]); + } + + /* Cleanup remaining memory */ + VIR_FREE(outputStr); + VIR_FREE(volInfoTexts); VIR_FREE(activeNames); virStoragePoolFree(pool); return TRUE; diff --git a/tools/virsh.pod b/tools/virsh.pod index 7c75edc..a217cba 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -839,10 +839,12 @@ Returns basic information about the given storage volume. I<--pool> I<pool-or-uuid> is the name or UUID of the storage pool the volume is in. I<vol-name-or-key-or-path> is the name or key or path of the volume to return information for. -=item B<vol-list> I<--pool> I<pool-or-uuid> +=item B<vol-list> [optional I<--pool>] I<pool-or-uuid> optional I<--details> Return the list of volumes in the given storage pool. I<--pool> I<pool-or-uuid> is the name or UUID of the storage pool. +The I<--details> option instructs virsh to additionally display volume +type and capacity related information where available. =item B<vol-pool> [optional I<--uuid>] I<vol-key-or-path> -- 1.7.0.1
On 06/18/2010 06:31 PM, Justin Clift wrote: <snip>
+ // The capacity value to output + val = prettyCapacity(volumeInfo.capacity,&unit); + virBufferVSprintf(&bufStr, "%.2lf %s", val, unit); + volInfoTexts[i]->capacity = + vshStrdup(ctl, virBufferContentAndReset(&bufStr)); + + // The allocation value to output
Ugh. Just noticed these two // comments instead of /* */ ones. Should I fix and resubmit? -- Salasaga - Open Source eLearning IDE http://www.salasaga.org
On 06/18/2010 10:08 AM, Justin Clift wrote:
On 06/18/2010 06:31 PM, Justin Clift wrote: <snip>
+ // The capacity value to output + val = prettyCapacity(volumeInfo.capacity,&unit); + virBufferVSprintf(&bufStr, "%.2lf %s", val, unit); + volInfoTexts[i]->capacity = + vshStrdup(ctl, virBufferContentAndReset(&bufStr)); + + // The allocation value to output
Ugh. Just noticed these two // comments instead of /* */ ones.
Should I fix and resubmit?
If that's the only issue, then probably not worth a resubmission. I'll see if I spot anything else while reviewing the actual patch, though... -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 06/18/2010 02:31 AM, Justin Clift wrote:
This patch adds a new --details option to the virsh vol-list command, making its output more useful to people who use virsh for significant lengths of time.
Addresses BZ # 605543
https://bugzilla.redhat.com/show_bug.cgi?id=605543
---
This new version of the patch uses the existing virsh output format when the --details option isn't given, maintaining backwards compatibility for existing scripts. When the new --details option is given though, the additional info is displayed and all columns are sized to their widest string.
Output from the new option (hopefully this doesn't wrap):
virsh # vol-list default Name Path ----------------------------------------- CentOS-5.5-x86_64-bin-DVD-1of2.iso /var/lib/libvirt/images/CentOS-5.5-x86_64-bin-DVD-1of2.iso CentOS-5.5-x86_64-bin-DVD-2of2.iso /var/lib/libvirt/images/CentOS-5.5-x86_64-bin-DVD-2of2.iso
virsh # vol-list default --details Name Path Type Capacity Allocation --------------------------------------------------------------------------------------------------------------------------- CentOS-5.5-x86_64-bin-DVD-1of2.iso /var/lib/libvirt/images/CentOS-5.5-x86_64-bin-DVD-1of2.iso file 4.09 GB 4.10 GB CentOS-5.5-x86_64-bin-DVD-2of2.iso /var/lib/libvirt/images/CentOS-5.5-x86_64-bin-DVD-2of2.iso file 412.33 MB 412.74 MB
It would be nice to right-justify the Capacity and Allocation columns, so that the decimal points line up.
static int cmdVolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) { + virStorageVolInfo volumeInfo; virStoragePoolPtr pool; - int maxactive = 0, i; + int details = vshCommandOptBool(cmd, "details"); + int maxAlloc = 0, maxCap = 0, maxName = 0; + int maxPath = 0, maxType = 0;
Hmm, maybe these should be size_t instead of int, since width is inherently unsigned in nature. And the naming is a bit off - maxAlloc makes it sound like the largest allocation size seen (2GB > 200MB), whereas a name like allocWidth or maxAllocWidth makes it obvious that len("200MB") > len("2GB").
+ /* Retrieve the list of volume names in the pool */ + if (numVolumes) { + activeNames = vshMalloc(ctl, sizeof(char *) * numVolumes);
Wondering if VIR_ALLOC_N is better than vshMalloc here. Both styles occur in virsh.c, so you're probably okay as-is.
+ // The allocation value to output + val = prettyCapacity(volumeInfo.allocation, &unit); + virBufferVSprintf(&bufStr, "%.2lf %s", val, unit); + volInfoTexts[i]->allocation = + vshStrdup(ctl, virBufferContentAndReset(&bufStr)); }
+ /** Remember the longest output size of each string, ** + ** so we can use a printf style output format template ** + ** later on for both the header and volume info rows **/ + + /* Keep the length of name string if longest so far */
In addition to the // style comment that you already notices, the /** block comment looks unusual in relation to the rest of the file. You can probably replace all 6 of those ** with just *.
+ + /** We only get here if the --details option was selected. ** + ** Column now resize to the longest string to be output. **/
Another instance of odd /** **/ comments. And that second sentence reads poorly; how about: Size each column according to the longest string to be output.
+ + /* Determine the length of the header strings. These must be + * calculated because we may be outputing a translated heading
s/outputing/outputting/
+ /* Display the string lengths for debugging */ + vshDebug(ctl, 5, "Longest name string = %d chars\n", maxName);
Since I asked you to change these variables to be size_t, you'll have to remember that %zu is not necessarily portable, so here you'd have to use vshDebug(,,"%lu", (unsigned long) maxName).
+ /* Create the output template */ + char *outputStr; + virBuffer bufStr = VIR_BUFFER_INITIALIZER; + virBufferVSprintf(&bufStr, "%%-%us %%-%us %%-%us %%-%us %%-%us\n", + maxName, maxPath, maxType, maxCap, maxAlloc); + outputStr = virBufferContentAndReset(&bufStr);
If you generate a printf format string like this, you need to check for allocation failure. Otherwise, you could end up passing NULL as the format string in the vshPrint below. Having said that...
+ + /* Display the header */ + vshPrint(ctl, outputStr, _("Name"), _("Path"), _("Type"), + ("Capacity"), _("Allocation")); + for (i = maxName + maxPath + maxType + maxCap + maxAlloc + 8; i > 0; i--) + vshPrintExtra(ctl, "-"); + vshPrintExtra(ctl, "\n"); + + /* Display the volume info rows */ + for (i = 0; i < numVolumes; i++) { + vshPrint(ctl, outputStr, + activeNames[i], + volInfoTexts[i]->path, + volInfoTexts[i]->type, + volInfoTexts[i]->capacity, + volInfoTexts[i]->allocation);
Hmm. Rather than generating a printf format string, what if you used %-*s instead? As in: vshPrint(ctl, "%-*s...\n", (int) maxName, activeNames[i], ...); But here, since %*s requires that the corresponding argument be an int (and not a size_t), that makes me wonder if you really want maxNames to be size_t, or if keeping it as int makes sense after all. And it may make sense to add some sanity checking that, if we stick with int, we don't encounter any (unlikely) situation where the user tries to print more than INT_MAX (but less than SIZE_MAX) bytes (mainly on a 64-bit machine). Overall, I like the idea, but I think I had enough comments to warrant seeing a v3 of the patch. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 06/19/2010 02:55 AM, Eric Blake wrote:
Name Path Type Capacity Allocation --------------------------------------------------------------------------------------------------------------------------- CentOS-5.5-x86_64-bin-DVD-1of2.iso /var/lib/libvirt/images/CentOS-5.5-x86_64-bin-DVD-1of2.iso file 4.09 GB 4.10 GB CentOS-5.5-x86_64-bin-DVD-2of2.iso /var/lib/libvirt/images/CentOS-5.5-x86_64-bin-DVD-2of2.iso file 412.33 MB 412.74 MB
It would be nice to right-justify the Capacity and Allocation columns, so that the decimal points line up.
Very good point. It's kind of obvious now you point it out. :)
Hmm, maybe these should be size_t instead of int, since width is inherently unsigned in nature.
Makes sense. size_t also better communicates the purpose of the variables.
And the naming is a bit off - maxAlloc makes it sound like the largest allocation size seen (2GB> 200MB), whereas a name like allocWidth or maxAllocWidth makes it obvious that len("200MB")> len("2GB").
Yep, I can see that. How about names like allocStrWidth, nameStrWidth, pathStrWidth, etc?
+ /* Retrieve the list of volume names in the pool */ + if (numVolumes) { + activeNames = vshMalloc(ctl, sizeof(char *) * numVolumes);
Wondering if VIR_ALLOC_N is better than vshMalloc here. Both styles occur in virsh.c, so you're probably okay as-is.
Cool, hadn't come across VIR_ALLOC_N before. You've probably picked up that I was using vshMalloc() having seen it used elsewhere in the virsh code before. :) Looking at the VIR_ALLOC_N definition in memory.h, and looking at the definition of vshMalloc(), I can't really tell if one is better than the other. There's only one other use of VIR_ALLOC_N in virsh.c, but I'm open to changing my stuff to use VIR_ALLOC_N if it's better?
+ /** Remember the longest output size of each string, ** + ** so we can use a printf style output format template ** + ** later on for both the header and volume info rows **/ + + /* Keep the length of name string if longest so far */
In addition to the // style comment that you already notices, the /** block comment looks unusual in relation to the rest of the file. You can probably replace all 6 of those ** with just *.
Cool, will do.
Another instance of odd /** **/ comments. And that second sentence reads poorly; how about:
Size each column according to the longest string to be output.
That's a lot better. I clearly remember having troubles picking words, and couldn't come up with anything really good. ;)
+ /* Determine the length of the header strings. These must be + * calculated because we may be outputing a translated heading
s/outputing/outputting/
Oops, thanks.
+ /* Create the output template */ + char *outputStr; + virBuffer bufStr = VIR_BUFFER_INITIALIZER; + virBufferVSprintf(&bufStr, "%%-%us %%-%us %%-%us %%-%us %%-%us\n", + maxName, maxPath, maxType, maxCap, maxAlloc); + outputStr = virBufferContentAndReset(&bufStr);
If you generate a printf format string like this, you need to check for allocation failure. Otherwise, you could end up passing NULL as the format string in the vshPrint below. Having said that...
Good point. Hadn't thought of that, but it makes sense. :)
+ + /* Display the header */ + vshPrint(ctl, outputStr, _("Name"), _("Path"), _("Type"), + ("Capacity"), _("Allocation")); + for (i = maxName + maxPath + maxType + maxCap + maxAlloc + 8; i> 0; i--) + vshPrintExtra(ctl, "-"); + vshPrintExtra(ctl, "\n"); + + /* Display the volume info rows */ + for (i = 0; i< numVolumes; i++) { + vshPrint(ctl, outputStr, + activeNames[i], + volInfoTexts[i]->path, + volInfoTexts[i]->type, + volInfoTexts[i]->capacity, + volInfoTexts[i]->allocation);
Hmm. Rather than generating a printf format string, what if you used %-*s instead? As in:
vshPrint(ctl, "%-*s...\n", (int) maxName, activeNames[i], ...);
But here, since %*s requires that the corresponding argument be an int (and not a size_t), that makes me wonder if you really want maxNames to be size_t, or if keeping it as int makes sense after all. And it may make sense to add some sanity checking that, if we stick with int, we don't encounter any (unlikely) situation where the user tries to print more than INT_MAX (but less than SIZE_MAX) bytes (mainly on a 64-bit machine).
I'm kind of liking the size_t approach for the moment while looking at things. I'll think it though more, and see what looks better as I have a go at it. ;)
Overall, I like the idea, but I think I had enough comments to warrant seeing a v3 of the patch.
Yep, no worries. Getting the feeling this will be in 0.8.3 instead. Heh. ;) Thanks Eric. :) Regards and best wishes, Justin Clift -- Salasaga - Open Source eLearning IDE http://www.salasaga.org
On 06/18/2010 11:27 AM, Justin Clift wrote:
+ /* Retrieve the list of volume names in the pool */ + if (numVolumes) { + activeNames = vshMalloc(ctl, sizeof(char *) * numVolumes);
Wondering if VIR_ALLOC_N is better than vshMalloc here. Both styles occur in virsh.c, so you're probably okay as-is.
Cool, hadn't come across VIR_ALLOC_N before. You've probably picked up that I was using vshMalloc() having seen it used elsewhere in the virsh code before. :)
Outside of virsh, VIR_ALLOC_N is a winner, hands down. But inside virsh.c, the vshMalloc wrapper reports a line number where any allocation failure occurs, which might be considered useful in debugging an over-allocation failure. And given that VIR_ALLOC_N is only used once in virsh.c, consistency would argue for vshMalloc. Anyone else have an opinion?
+ /* Create the output template */ + char *outputStr; + virBuffer bufStr = VIR_BUFFER_INITIALIZER; + virBufferVSprintf(&bufStr, "%%-%us %%-%us %%-%us %%-%us %%-%us\n", + maxName, maxPath, maxType, maxCap, maxAlloc); + outputStr = virBufferContentAndReset(&bufStr);
If you generate a printf format string like this, you need to check for allocation failure. Otherwise, you could end up passing NULL as the format string in the vshPrint below. Having said that...
Good point. Hadn't thought of that, but it makes sense. :)
One other thing - rather than using virBuffer, with a single virBufferVSprintf before you reset the buffer, it is faster to just use virAsprintf (no need for an intermediate buffer unless you are constructing a string in mutiple chunks). -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
This patch adds a new --details option to the virsh vol-list command, making its output more useful when many luns are present. Addresses BZ # 605543 https://bugzilla.redhat.com/show_bug.cgi?id=605543 --- Updated version of this patch, taking into account all feedback given for this and the pool-list --details patch. Also, the embedded space in the size related outputs: 1.40 TB has been removed, so they're now like: 1.40TB It's not as human friendly, but should be more reliable for parsing by scripts. (as per Richard W. M. Jones pointers) New example output: virsh # vol-list default --details Name Path Type Capacity Allocation -------------------------------------------------------------------------------------------------------------------------------------------------- Fedora-12-x86_64-DVD.iso /var/lib/libvirt/images/Fedora-12-x86_64-DVD.iso file 3.29GB 3.30GB Fedora-13-x86_64-DVD.iso /var/lib/libvirt/images/Fedora-13-x86_64-DVD.iso file 3.38GB 3.38GB RHEL6.0-20100622.1-Server-i386-DVD1.iso /var/lib/libvirt/images/RHEL6.0-20100622.1-Server-i386-DVD1.iso file 3.42GB 3.43GB RHEL6.0-20100622.1-Server-x86_64-DVD1.iso /var/lib/libvirt/images/RHEL6.0-20100622.1-Server-x86_64-DVD1.iso file 3.91GB 3.91GB RHEL6.0-20100622.1-Workstation-i386-DVD1.iso /var/lib/libvirt/images/RHEL6.0-20100622.1-Workstation-i386-DVD1.iso file 3.42GB 3.42GB RHEL6.0-20100622.1-Workstation-x86_64-DVD1.iso /var/lib/libvirt/images/RHEL6.0-20100622.1-Workstation-x86_64-DVD1.iso file 3.90GB 3.91GB virsh # Example output when no volumes are in a pool: virsh # vol-list tmp Name Path ----------------------------------------- virsh # vol-list tmp --details Name Path Type Capacity Allocation -------------------------------------- virsh # tools/virsh.c | 259 ++++++++++++++++++++++++++++++++++++++++++++++++------- tools/virsh.pod | 4 +- 2 files changed, 232 insertions(+), 31 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index e07fef3..825e0d2 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -6260,70 +6260,269 @@ static const vshCmdInfo info_vol_list[] = { static const vshCmdOptDef opts_vol_list[] = { {"pool", VSH_OT_DATA, VSH_OFLAG_REQ, N_("pool name or uuid")}, + {"details", VSH_OT_BOOL, 0, N_("display extended details for volumes")}, {NULL, 0, 0, NULL} }; static int cmdVolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) { + virStorageVolInfo volumeInfo; virStoragePoolPtr pool; - int maxactive = 0, i; char **activeNames = NULL; + char *outputStr = NULL; + const char *unit; + double val; + int details = vshCommandOptBool(cmd, "details"); + int numVolumes = 0, i; + int ret, functionReturn; + int stringLength = 0; + size_t allocStrLength = 0, capStrLength = 0; + size_t nameStrLength = 0, pathStrLength = 0; + size_t typeStrLength = 0; + struct volInfoText { + char *allocation; + char *capacity; + char *path; + char *type; + }; + struct volInfoText *volInfoTexts = NULL; + /* Check the connection to libvirtd daemon is still working */ if (!vshConnectionUsability(ctl, ctl->conn, TRUE)) return FALSE; + /* Look up the pool information given to us by the user */ if (!(pool = vshCommandOptPool(ctl, cmd, "pool", NULL))) return FALSE; - maxactive = virStoragePoolNumOfVolumes(pool); - if (maxactive < 0) { - virStoragePoolFree(pool); - vshError(ctl, "%s", _("Failed to list active vols")); - return FALSE; - } - if (maxactive) { - activeNames = vshMalloc(ctl, sizeof(char *) * maxactive); + /* Determine the number of volumes in the pool */ + numVolumes = virStoragePoolNumOfVolumes(pool); - if ((maxactive = virStoragePoolListVolumes(pool, activeNames, - maxactive)) < 0) { + /* Retrieve the list of volume names in the pool */ + if (numVolumes) { + 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; } - qsort(&activeNames[0], maxactive, sizeof(char *), namesorter); + /* Sort the volume names */ + qsort(&activeNames[0], numVolumes, sizeof(*activeNames), namesorter); + + /* Set aside memory for volume information pointers */ + volInfoTexts = vshCalloc(ctl, numVolumes, sizeof(*volInfoTexts)); } - vshPrintExtra(ctl, "%-20s %-40s\n", _("Name"), _("Path")); - vshPrintExtra(ctl, "-----------------------------------------\n"); - for (i = 0; i < maxactive; i++) { - virStorageVolPtr vol = virStorageVolLookupByName(pool, activeNames[i]); - char *path; + /* Collect the rest of the volume information for display */ + for (i = 0; i < numVolumes; i++) { + /* Retrieve volume info */ + virStorageVolPtr vol = virStorageVolLookupByName(pool, + activeNames[i]); - /* this kind of work with vols is not atomic operation */ - if (!vol) { - VIR_FREE(activeNames[i]); - continue; + /* Retrieve the volume path */ + if ((volInfoTexts[i].path = virStorageVolGetPath(vol)) == NULL) { + /* Something went wrong retrieving a volume path, cope with it */ + volInfoTexts[i].path = vshStrdup(ctl, _("unknown")); } - if ((path = virStorageVolGetPath(vol)) == NULL) { - virStorageVolFree(vol); - continue; - } + /* If requested, retrieve volume type and sizing information */ + if (details) { + if (virStorageVolGetInfo(vol, &volumeInfo) != 0) { + /* Something went wrong retrieving volume info, cope with it */ + volInfoTexts[i].allocation = vshStrdup(ctl, _("unknown")); + volInfoTexts[i].capacity = vshStrdup(ctl, _("unknown")); + volInfoTexts[i].type = vshStrdup(ctl, _("unknown")); + } else { + /* Convert the returned volume info into output strings */ + + /* Volume type */ + if (volumeInfo.type == VIR_STORAGE_VOL_FILE) + volInfoTexts[i].type = vshStrdup(ctl, _("file")); + else + volInfoTexts[i].type = vshStrdup(ctl, _("block")); + + /* Create the capacity output string */ + val = prettyCapacity(volumeInfo.capacity, &unit); + ret = virAsprintf(&volInfoTexts[i].capacity, + "%.2lf%s", val, unit); + if (ret < 0) { + /* An error occurred creating the string, return */ + goto asprintf_failure; + } + + /* Create the allocation output string */ + val = prettyCapacity(volumeInfo.allocation, &unit); + ret = virAsprintf(&volInfoTexts[i].allocation, + "%.2lf%s", val, unit); + if (ret < 0) { + /* An error occurred creating the string, return */ + goto asprintf_failure; + } + } + + /* Remember the largest length for each output string. + * This lets us displaying header and volume information rows + * using a single, properly sized, printf style output string. + */ + /* Keep the length of name string if longest so far */ + stringLength = strlen(activeNames[i]); + if (stringLength > nameStrLength) + nameStrLength = stringLength; + + /* Keep the length of path string if longest so far */ + stringLength = strlen(volInfoTexts[i].path); + if (stringLength > pathStrLength) + pathStrLength = stringLength; + + /* Keep the length of type string if longest so far */ + stringLength = strlen(volInfoTexts[i].type); + if (stringLength > typeStrLength) + typeStrLength = stringLength; + + /* Keep the length of capacity string if longest so far */ + stringLength = strlen(volInfoTexts[i].capacity); + if (stringLength > capStrLength) + capStrLength = stringLength; + + /* Keep the length of allocation string if longest so far */ + stringLength = strlen(volInfoTexts[i].allocation); + if (stringLength > allocStrLength) + allocStrLength = stringLength; + } - vshPrint(ctl, "%-20s %-40s\n", - virStorageVolGetName(vol), - path); - VIR_FREE(path); + /* Cleanup memory allocation */ virStorageVolFree(vol); + } + + /* If the --details option wasn't selected, we output the volume + * info using the fixed string format from previous versions to + * maintain backward compatibility. + */ + + /* Output basic info then return if --details option not selected */ + if (!details) { + /* 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], + volInfoTexts[i].path); + } + + /* Cleanup and return */ + functionReturn = TRUE; + goto cleanup; + } + + /* We only get here if the --details option was selected. */ + + /* Use the length of name header string if it's longest */ + stringLength = strlen(_("Name")); + if (stringLength > nameStrLength) + nameStrLength = stringLength; + + /* Use the length of path header string if it's longest */ + stringLength = strlen(_("Path")); + if (stringLength > pathStrLength) + pathStrLength = stringLength; + + /* Use the length of type header string if it's longest */ + stringLength = strlen(_("Type")); + if (stringLength > typeStrLength) + typeStrLength = stringLength; + + /* Use the length of capacity header string if it's longest */ + stringLength = strlen(_("Capacity")); + if (stringLength > capStrLength) + capStrLength = stringLength; + + /* Use the length of allocation header string if it's longest */ + stringLength = strlen(_("Allocation")); + if (stringLength > allocStrLength) + allocStrLength = stringLength; + + /* Display the string lengths for debugging */ + vshDebug(ctl, 5, "Longest name string = %lu chars\n", nameStrLength); + vshDebug(ctl, 5, "Longest path string = %lu chars\n", pathStrLength); + vshDebug(ctl, 5, "Longest type string = %lu chars\n", typeStrLength); + vshDebug(ctl, 5, "Longest capacity string = %lu chars\n", capStrLength); + vshDebug(ctl, 5, "Longest allocation string = %lu chars\n", allocStrLength); + + /* Create the output template */ + ret = virAsprintf(&outputStr, + "%%-%lus %%-%lus %%-%lus %%%lus %%%lus\n", + (unsigned long) nameStrLength, + (unsigned long) pathStrLength, + (unsigned long) typeStrLength, + (unsigned long) capStrLength, + (unsigned long) allocStrLength); + if (ret < 0) { + /* An error occurred creating the string, return */ + goto asprintf_failure; + } + + /* Display the header */ + vshPrint(ctl, outputStr, _("Name"), _("Path"), _("Type"), + ("Capacity"), _("Allocation")); + for (i = nameStrLength + pathStrLength + typeStrLength + + capStrLength + allocStrLength + + 8; i > 0; i--) + vshPrintExtra(ctl, "-"); + vshPrintExtra(ctl, "\n"); + + /* Display the volume info rows */ + for (i = 0; i < numVolumes; i++) { + vshPrint(ctl, outputStr, + activeNames[i], + volInfoTexts[i].path, + volInfoTexts[i].type, + volInfoTexts[i].capacity, + volInfoTexts[i].allocation); + } + + /* Cleanup and return */ + functionReturn = TRUE; + goto cleanup; + +asprintf_failure: + + /* Display an appropriate error message then cleanup and return */ + switch (errno) { + case ENOMEM: + /* Couldn't allocate memory */ + vshError(ctl, "%s", _("Out of memory")); + break; + default: + /* Some other error */ + vshError(ctl, _("virAsprintf failed (errno %d)"), errno); + } + functionReturn = FALSE; + +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]); } + + /* Cleanup remaining memory */ + VIR_FREE(outputStr); + VIR_FREE(volInfoTexts); VIR_FREE(activeNames); virStoragePoolFree(pool); - return TRUE; + + /* Return the desired value */ + return functionReturn; } diff --git a/tools/virsh.pod b/tools/virsh.pod index 2b2227f..b2dff8b 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -850,10 +850,12 @@ Returns basic information about the given storage volume. I<--pool> I<pool-or-uuid> is the name or UUID of the storage pool the volume is in. I<vol-name-or-key-or-path> is the name or key or path of the volume to return information for. -=item B<vol-list> I<--pool> I<pool-or-uuid> +=item B<vol-list> [optional I<--pool>] I<pool-or-uuid> optional I<--details> Return the list of volumes in the given storage pool. I<--pool> I<pool-or-uuid> is the name or UUID of the storage pool. +The I<--details> option instructs virsh to additionally display volume +type and capacity related information where available. =item B<vol-pool> [optional I<--uuid>] I<vol-key-or-path> -- 1.7.1
participants (2)
-
Eric Blake -
Justin Clift