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