12:27 p.m.
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