On 10/13/2015 08:39 AM, Peter Krempa wrote:
On Fri, Oct 09, 2015 at 09:34:11 -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1233003
Commit id 'fdda3760' only managed a symptom where it was possible to create a file in a pool without libvirt's knowledge, so it was reverted.
The real fix is to have all the createVol API's which actually create a volume (disk, logical, zfs) and the buildVol API's which handle the real creation of some volume file (fs, rbd, sheepdog) manage deleting any volume which they create when there is some sort of error in processing the volume.
This way the onus isn't left up to the storage_driver to determine whether the buildVol failure was due to some failure as a result of adjustments made to the volume after creation such as getting sizes, changing ownership, changing volume protections, etc. or simple a failure in creation.
Without needing to consider that the volume has to be removed, the buildVol failure path only needs to remove the volume from the pool. This way if a creation failed due to duplicate name, libvirt wouldn't remove a volume that it didn't create in the pool target.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_driver.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 58bc4bd..0c70482 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1869,8 +1869,17 @@ storageVolCreateXML(virStoragePoolPtr obj, pool->asyncjobs--;
if (buildret < 0) { - storageVolDeleteInternal(volobj, backend, pool, voldef, - 0, false); + /* NB: A 'buildVol' backend must remove any volume created + * on error since this code does not distinguish whether the + * failure is to create the volume, reserve any space necessary + * for the volume, get data about the volume, change it's + * accessibility, etc. All that should be done at this point + * is remove the volume from the pool. This avoids issues + * arising from a creation failure due to some external action + * which created a volume of the same name that libvirt was + * not aware of between the time checked and create attempt. + */
This comment should document the required behavior at the point where we define the callback structure. Otherwise somebody might miss it as it'd be stashed in a random function.
OK, I left: /* buildVol handles deleting volume on failure */ Since a few lines later the refreshVol calls the DeleteVol - I just wanted to make it clear that the same delete call is not necessary.
+ storageVolRemoveFromPool(pool, voldef); voldef = NULL; goto cleanup;
ACK if you move the comment to a reasonable place.
I hope src/storage/storage_backend.h is your idea of the reasonable place - I added the following text there right above the typedef: /* A 'buildVol' backend must remove any volume created on error since * the storage driver does not distinguish whether the failure is due * to failure to create the volume, to reserve any space necessary for * the volume, to get data about the volume, to change it's accessibility, * etc. This avoids issues arising from a creation failure due to some * external action which created a volume of the same name that libvirt * was not aware of between checking the pool and the create attempt. It * also avoids extra round trips to just delete a file. */ typedef int (*virStorageBackendBuildVol)(virConnectPtr conn, If you had a different location in mind, then let me know. I'll wait on comments from my responses from patch 8 & 9 pushing this. Since in my mind those should be done first. John