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@xxxxxxxxxx>
>>> ---
>>> 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
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list