[libvirt] [PATCH 1/4] cmdVolCreateAs: Rework to follow usual func pattern

[Erik Skultety]

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 at 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
>