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&gt; --- 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)

+ virStorageVolFree(vol); virStoragePoolFree(pool); - return false; + VIR_FREE(xml); + return ret; } /*

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&gt; >> --- >> 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.

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

It is highly unlikely that a backend will know how to create a volume from a different volume (buildVolFrom) and not know how to create an empty volume (createVol). But: 1) we call the function without any prior check so if that's the case we would SIGSEGV immediatelly 2) it's better to be safe than sorry. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/storage/storage_driver.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index e0ded01..81b1584 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2037,6 +2037,13 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj, if (newvol->target.capacity < origvol->target.capacity) newvol->target.capacity = origvol->target.capacity; + if (!backend->createVol) { + virReportError(VIR_ERR_NO_SUPPORT, + "%s", _("storage pool does not support volume " + "creation")); + goto cleanup; + } + if (!backend->buildVolFrom) { virReportError(VIR_ERR_NO_SUPPORT, "%s", _("storage pool does not support"