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@xxxxxxxxxx +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list