[libvirt] [PATCHv2 2/2] virsh: add new --details option to vol-list

[Justin Clift]

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