[libvirt] [PATCHv2 2/2] virsh: add new --details option to vol-list
Eric Blake
eblake at redhat.com
Fri Jun 18 16:55:19 UTC 2010
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 at redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20100618/ea4e1a62/attachment-0001.sig>
More information about the libvir-list
mailing list