[libvirt] [PATCH 12/12] storage: On 'buildVol' failure don't delete the volume
Peter Krempa
pkrempa at redhat.com
Tue Oct 13 12:39:25 UTC 2015
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.
> + storageVolRemoveFromPool(pool, voldef);
> voldef = NULL;
> goto cleanup;
ACK if you move the comment to a reasonable place.
Peter
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20151013/c744fe93/attachment-0001.sig>
More information about the libvir-list
mailing list