[libvirt] [PATCH 0/4] Couple of storage related fixes
After an e-mail on libvirt-users [1] I've realized there are some things that we can do better. The first two patches introduce a new feature. The other two are just some fixes. Michal Privoznik (4): cmdVolCreateAs: Rework to follow usual func pattern virsh: Teach vol-create-as to --print-xml storageVolCreateXML: Swap order of two operations storageVolCreateXMLFrom: Check if backend knows how to createVol src/storage/storage_driver.c | 15 +++++++++++---- tools/virsh-volume.c | 33 +++++++++++++++++++++------------ tools/virsh.pod | 6 ++++-- 3 files changed, 36 insertions(+), 18 deletions(-) -- 2.4.10
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@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) + virStorageVolFree(vol); virStoragePoolFree(pool); - return false; + VIR_FREE(xml); + return ret; } /* -- 2.4.10
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@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.
+ 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
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@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. 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
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@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
We have the same argument to many other commands that produce an XML based on what user typed. But unfortunately vol-create-as was missing it. Maybe nobody had needed it yet. Well, I did just now. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh-volume.c | 17 +++++++++++++---- tools/virsh.pod | 6 ++++-- 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c index 569f555..cfb8cfc 100644 --- a/tools/virsh-volume.c +++ b/tools/virsh-volume.c @@ -195,6 +195,10 @@ static const vshCmdOptDef opts_vol_create_as[] = { .type = VSH_OT_BOOL, .help = N_("preallocate metadata (for qcow2 instead of full allocation)") }, + {.name = "print-xml", + .type = VSH_OT_BOOL, + .help = N_("print XML document, but don't define/create") + }, {.name = NULL} }; @@ -213,6 +217,7 @@ cmdVolCreateAs(vshControl *ctl, const vshCmd *cmd) virStoragePoolPtr pool; virStorageVolPtr vol = NULL; char *xml = NULL; + bool printXML = vshCommandOptBool(cmd, "print-xml"); const char *name, *capacityStr = NULL, *allocationStr = NULL, *format = NULL; const char *snapshotStrVol = NULL, *snapshotStrFormat = NULL; unsigned long long capacity, allocation = 0; @@ -337,12 +342,16 @@ cmdVolCreateAs(vshControl *ctl, const vshCmd *cmd) } xml = virBufferContentAndReset(&buf); - if (!(vol = virStorageVolCreateXML(pool, xml, flags))) { - vshError(ctl, _("Failed to create vol %s"), name); - goto cleanup; + if (printXML) { + vshPrint(ctl, "%s", xml); + } else { + if (!(vol = virStorageVolCreateXML(pool, xml, flags))) { + vshError(ctl, _("Failed to create vol %s"), name); + goto cleanup; + } + vshPrint(ctl, _("Vol %s created\n"), name); } - vshPrint(ctl, _("Vol %s created\n"), name); ret = true; cleanup: diff --git a/tools/virsh.pod b/tools/virsh.pod index 435c649..4662658 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -3427,9 +3427,11 @@ If this is not possible, the copy fails. =item B<vol-create-as> I<pool-or-uuid> I<name> I<capacity> [I<--allocation> I<size>] [I<--format> I<string>] [I<--backing-vol> I<vol-name-or-key-or-path>] [I<--backing-vol-format> I<string>] -[I<--prealloc-metadata>] +[I<--prealloc-metadata>] [I<--print-xml>] -Create a volume from a set of arguments. +Create a volume from a set of arguments unless I<--print-xml> is specified, in +which case just the XML of the volume object is printed out without any actual +object creation. I<pool-or-uuid> is the name or UUID of the storage pool to create the volume in. I<name> is the name of the new volume. For a disk pool, this must match the -- 2.4.10
On 10/02/16 17:28, Michal Privoznik wrote:
We have the same argument to many other commands that produce an XML based on what user typed. But unfortunately vol-create-as was missing it. Maybe nobody had needed it yet. Well, I did just now.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh-volume.c | 17 +++++++++++++---- tools/virsh.pod | 6 ++++-- 2 files changed, 17 insertions(+), 6 deletions(-)
... ACK Erik
Firstly, we realloc internal list to hold new item (=volume that will be potentially be created) and then we check whether we actually know how to create it. If we don't we consume more memory than we really need for no good reason. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/storage/storage_driver.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 8ee2840..e0ded01 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1856,10 +1856,6 @@ storageVolCreateXML(virStoragePoolPtr obj, goto cleanup; } - if (VIR_REALLOC_N(pool->volumes.objs, - pool->volumes.count+1) < 0) - goto cleanup; - if (!backend->createVol) { virReportError(VIR_ERR_NO_SUPPORT, "%s", _("storage pool does not support volume " @@ -1867,6 +1863,10 @@ storageVolCreateXML(virStoragePoolPtr obj, goto cleanup; } + if (VIR_REALLOC_N(pool->volumes.objs, + pool->volumes.count+1) < 0) + goto cleanup; + /* Wipe any key the user may have suggested, as volume creation * will generate the canonical key. */ VIR_FREE(voldef->key); -- 2.4.10
On 10/02/16 17:28, Michal Privoznik wrote:
Firstly, we realloc internal list to hold new item (=volume that will be potentially be created) and then we check whether we
s/\(potentially\)\@<= be// If I screwed up the positive lookbehind just remove the second "be" :)
actually know how to create it. If we don't we consume more memory than we really need for no good reason.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/storage/storage_driver.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
... ACK Erik
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@redhat.com> --- 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" -- 2.4.10
On 10/02/16 17:28, Michal Privoznik wrote:
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@redhat.com> --- 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"
Although I'm still not 100% convinced, how would one attempt to implement volBuild which takes a freshly created volume in that pool without actually supporting volume creation. And if one's aware of this, in my mind, this would effectively end up a deadcode. But the logic is correct and being a little paranoid never hurts, so it's an ACK, unless someone would like to oppose, in which case, feel free to speak. Erik
On Wed, Feb 10, 2016 at 05:28:53PM +0100, Michal Privoznik wrote:
After an e-mail on libvirt-users [1]
I think you meant this thread :-) [1] https://www.redhat.com/archives/libvirt-users/2016-February/msg00033.html -- "Unable to create raw volume on netfs storage (Operation not permitted)"
I've realized there are some things that we can do better. The first two patches introduce a new feature. The other two are just some fixes.
Michal Privoznik (4): cmdVolCreateAs: Rework to follow usual func pattern virsh: Teach vol-create-as to --print-xml storageVolCreateXML: Swap order of two operations storageVolCreateXMLFrom: Check if backend knows how to createVol
src/storage/storage_driver.c | 15 +++++++++++---- tools/virsh-volume.c | 33 +++++++++++++++++++++------------ tools/virsh.pod | 6 ++++-- 3 files changed, 36 insertions(+), 18 deletions(-)
-- /kashyap
participants (3)
-
Erik Skultety -
Kashyap Chamarthy -
Michal Privoznik