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.
> + virStorageVolFree(vol);
> virStoragePoolFree(pool);
> - return false;
> + VIR_FREE(xml);
> + return ret;
> }
>
> /*
>
ACK with that "if" condition removed, only leaving the
virStorageVolFree(vol) bit in place.
Erik
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list