11:55 a.m.
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org