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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org