Re: [libvirt] [PATCH 1/4] cmdVolCreateAs: Rework to follow usual func pattern
On 12/02/16 15:22, Michal Privoznik wrote:
On 12.02.2016 14:17, Erik Skultety wrote:
> On 10/02/16 17:28, Michal Privoznik wrote:
>> The way we usually write functions is that we start the work and
>> if something goes bad we goto cleanup and roll back there. Or
>> just free resources that are no longer needed. Do the same here.
>>
>> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
>> ---
>> tools/virsh-volume.c | 24 ++++++++++++------------
>> 1 file changed, 12 insertions(+), 12 deletions(-)
>>
>> diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c
>> index 35f0cbd..569f555 100644
>> --- a/tools/virsh-volume.c
>> +++ b/tools/virsh-volume.c
>> @@ -211,14 +211,15 @@ static bool
>> cmdVolCreateAs(vshControl *ctl, const vshCmd *cmd)
>> {
>> virStoragePoolPtr pool;
>> - virStorageVolPtr vol;
>> - char *xml;
>> + virStorageVolPtr vol = NULL;
>> + char *xml = NULL;
>> const char *name, *capacityStr = NULL, *allocationStr = NULL, *format =
NULL;
>> const char *snapshotStrVol = NULL, *snapshotStrFormat = NULL;
>> unsigned long long capacity, allocation = 0;
>> virBuffer buf = VIR_BUFFER_INITIALIZER;
>> unsigned long flags = 0;
>> virshControlPtr priv = ctl->privData;
>> + bool ret = false;
>>
>> if (vshCommandOptBool(cmd, "prealloc-metadata"))
>> flags |= VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA;
>> @@ -335,23 +336,22 @@ cmdVolCreateAs(vshControl *ctl, const vshCmd *cmd)
>> goto cleanup;
>> }
>> xml = virBufferContentAndReset(&buf);
>> - vol = virStorageVolCreateXML(pool, xml, flags);
>> - VIR_FREE(xml);
>> - virStoragePoolFree(pool);
>>
>> - if (vol != NULL) {
>> - vshPrint(ctl, _("Vol %s created\n"), name);
>> - virStorageVolFree(vol);
>> - return true;
>> - } else {
>> + if (!(vol = virStorageVolCreateXML(pool, xml, flags))) {
>> vshError(ctl, _("Failed to create vol %s"), name);
>> - return false;
>> + goto cleanup;
>> }
>>
>> + vshPrint(ctl, _("Vol %s created\n"), name);
>> + ret = true;
>> +
>> cleanup:
>> virBufferFreeAndReset(&buf);
>> + if (vol)
>
> Looking at virStorageVolFree, this bit ^^^ is really unnecessary to have
> here. I mean, yeah, it can return -1, but we never test for it.
> In fact, it can return -1 only if the pointer is NULL or the object is
> of a different instance than it should be, and in your case, you don't
> care about the former and that kind of check certainly wouldn't avoid
> trying to unref the latter. The sad thing is, we're inconsistent in
> usage of this throughout modules.
I think we are consistent. I wasn't able to find any occurrence where we
could call virStorageVolFree() over NULL. Secondly, it's important to
check it so that we don't poison logs.
Indeed, let's pretend I didn't say anything before.
Erik
For instance, if something goes
wrong, we print an error message. But there's no point in
printing
another one just because we are lazy to put a check there. Yes, we are
not checking for the return value of virStorageVolFree() itself, but we
never do that and I bet you couldn't find anyone really doing that.
That's why I think we are safe to teach our public free APIs to take
NULL, but that's a different cup of tea.
Long story short, I think we should stay consistent and have the check
there.
Michal