[libvirt] [PATCH 12/12] storage: On 'buildVol' failure don't delete the volume
John Ferlan
jferlan at redhat.com
Tue Oct 13 20:58:53 UTC 2015
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 at 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
More information about the libvir-list
mailing list