Re: [libvirt] [PATCHv2 2/2] virsh: add new --details option to vol-list
On 06/18/2010 11:27 AM, Justin Clift wrote:
>> + /* 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. :)
Outside of virsh, VIR_ALLOC_N is a winner, hands down. But inside
virsh.c, the vshMalloc wrapper reports a line number where any
allocation failure occurs, which might be considered useful in debugging
an over-allocation failure. And given that VIR_ALLOC_N is only used
once in virsh.c, consistency would argue for vshMalloc.
Anyone else have an opinion?
>> + /* 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. :)
One other thing - rather than using virBuffer, with a single
virBufferVSprintf before you reset the buffer, it is faster to just use
virAsprintf (no need for an intermediate buffer unless you are
constructing a string in mutiple chunks).
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 619 bytes)