[libvirt] [PATCH] storage: fix volDelete return when volume still being allocated
volDelete used to return VIR_ERR_INTERNAL_ERROR when attempting to delete a volume which was still being allocated. It should return VIR_ERR_OPERATION_INVALID. * src/storage/storage_driver.c: Fix return of volDelete. --- src/storage/storage_driver.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 2da2feb..d9c2137 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1914,7 +1914,7 @@ storageVolumeDelete(virStorageVolPtr obj, } if (vol->building) { - virStorageReportError(VIR_ERR_INTERNAL_ERROR, + virStorageReportError(VIR_ERR_OPERATION_INVALID, _("volume '%s' is still being allocated."), vol->name); goto cleanup; -- 1.7.4.4
On Wed, May 25, 2011 at 11:55:08AM +0100, Matthew Booth wrote:
volDelete used to return VIR_ERR_INTERNAL_ERROR when attempting to delete a volume which was still being allocated. It should return VIR_ERR_OPERATION_INVALID.
* src/storage/storage_driver.c: Fix return of volDelete. --- src/storage/storage_driver.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 2da2feb..d9c2137 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1914,7 +1914,7 @@ storageVolumeDelete(virStorageVolPtr obj, }
if (vol->building) { - virStorageReportError(VIR_ERR_INTERNAL_ERROR, + virStorageReportError(VIR_ERR_OPERATION_INVALID, _("volume '%s' is still being allocated."), vol->name); goto cleanup;
ACK If someone is motivated, it would be desirable to actually make delete work, even while the volume is building. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Wed, May 25, 2011 at 12:12:46PM +0100, Daniel P. Berrange wrote:
On Wed, May 25, 2011 at 11:55:08AM +0100, Matthew Booth wrote:
volDelete used to return VIR_ERR_INTERNAL_ERROR when attempting to delete a volume which was still being allocated. It should return VIR_ERR_OPERATION_INVALID.
* src/storage/storage_driver.c: Fix return of volDelete. --- src/storage/storage_driver.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 2da2feb..d9c2137 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1914,7 +1914,7 @@ storageVolumeDelete(virStorageVolPtr obj, }
if (vol->building) { - virStorageReportError(VIR_ERR_INTERNAL_ERROR, + virStorageReportError(VIR_ERR_OPERATION_INVALID, _("volume '%s' is still being allocated."), vol->name); goto cleanup;
ACK
Okay, I finally commited this, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
The three patches add support to delete volumes while it is building intead of raising an warning infomation,then abort. For the volume of raw file, delete it directly without any effect on the thread of creating it. For the volume of block, wait for its finish , then delete it. --- src/conf/storage_conf.h | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 271441a..25455df 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -275,6 +275,7 @@ typedef virStoragePoolObj *virStoragePoolObjPtr; struct _virStoragePoolObj { virMutex lock; + virCond cond; char *configFile; char *autostartLink; -- 1.7.1
On Sun, Jul 10, 2011 at 22:29:03 +0800, Guannan Ren wrote:
The three patches add support to delete volumes while it is building intead of raising an warning infomation,then abort. For the volume of raw file, delete it directly without any effect on the thread of creating it. For the volume of block, wait for its finish , then delete it.
Hmm, shouldn't we cancel volume creation instead of waiting for it to finish? Jirka
On 07/11/2011 04:49 PM, Jiri Denemark wrote:
On Sun, Jul 10, 2011 at 22:29:03 +0800, Guannan Ren wrote:
The three patches add support to delete volumes while it is building intead of raising an warning infomation,then abort. For the volume of raw file, delete it directly without any effect on the thread of creating it. For the volume of block, wait for its finish , then delete it. Hmm, shouldn't we cancel volume creation instead of waiting for it to finish?
Jirka Yes, that is expected, but for volumes in pools of disk and logical type, it is comparatively safe way to delete after its finish as the creation of these volume is changing the files system or something low-level.
On Mon, Jul 11, 2011 at 06:11:47PM +0800, Guannan Ren wrote:
On 07/11/2011 04:49 PM, Jiri Denemark wrote:
On Sun, Jul 10, 2011 at 22:29:03 +0800, Guannan Ren wrote:
The three patches add support to delete volumes while it is building intead of raising an warning infomation,then abort. For the volume of raw file, delete it directly without any effect on the thread of creating it. For the volume of block, wait for its finish , then delete it. Hmm, shouldn't we cancel volume creation instead of waiting for it to finish?
Jirka Yes, that is expected, but for volumes in pools of disk and logical type, it is comparatively safe way to delete after its finish as the creation of these volume is changing the files system or something low-level.
No, IMO, we really need to figure out if the operation is something that can be cancelled, and if so, cancel it. Dave
On 07/11/2011 11:40 PM, Dave Allan wrote:
On 07/11/2011 04:49 PM, Jiri Denemark wrote:
On Sun, Jul 10, 2011 at 22:29:03 +0800, Guannan Ren wrote:
The three patches add support to delete volumes while it is building intead of raising an warning infomation,then abort. For the volume of raw file, delete it directly without any effect on the thread of creating it. For the volume of block, wait for its finish , then delete it. Hmm, shouldn't we cancel volume creation instead of waiting for it to finish?
Jirka Yes, that is expected, but for volumes in pools of disk and logical type, it is comparatively safe way to delete after its finish as the creation of these volume is changing the files system or something low-level. No, IMO, we really need to figure out if the operation is something
On Mon, Jul 11, 2011 at 06:11:47PM +0800, Guannan Ren wrote: that can be cancelled, and if so, cancel it.
Dave
There is a version 2 of patches. https://www.redhat.com/archives/libvir-list/2011-July/msg01048.html
--- src/conf/storage_conf.c | 10 ++++++++++ 1 files changed, 10 insertions(+), 0 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index f6f8be1..0279850 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -323,6 +323,7 @@ virStoragePoolObjFree(virStoragePoolObjPtr obj) { VIR_FREE(obj->autostartLink); virMutexDestroy(&obj->lock); + ignore_value(virCondDestroy(&obj->cond)); VIR_FREE(obj); } @@ -1391,6 +1392,15 @@ virStoragePoolObjAssignDef(virStoragePoolObjListPtr pools, VIR_FREE(pool); return NULL; } + + if (virCondInit(&pool->cond) < 0) { + virStorageReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("cannot initialize cond")); + virMutexDestroy(&pool->lock); + VIR_FREE(pool); + return NULL; + } + virStoragePoolObjLock(pool); pool->active = 0; pool->def = def; -- 1.7.1
--- src/storage/storage_driver.c | 19 +++++++++++++++---- 1 files changed, 15 insertions(+), 4 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index d1fef92..61048ce 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1350,6 +1350,7 @@ storageVolumeCreateXML(virStoragePoolPtr obj, storageDriverUnlock(driver); voldef->building = 0; + virCondBroadcast(&pool->cond); pool->asyncjobs--; voldef = NULL; @@ -1508,6 +1509,8 @@ storageVolumeCreateXMLFrom(virStoragePoolPtr obj, origvol->building = 0; newvol->building = 0; + virCondBroadcast(&pool->cond); + virCondBroadcast(&origpool->cond); newvol = NULL; pool->asyncjobs--; @@ -1912,12 +1915,20 @@ storageVolumeDelete(virStorageVolPtr obj, } if (vol->building) { - virStorageReportError(VIR_ERR_INTERNAL_ERROR, - _("volume '%s' is still being allocated."), - vol->name); - goto cleanup; + if (vol->type == VIR_STORAGE_VOL_FILE) { + goto deletevol; + } else if (vol->type == VIR_STORAGE_VOL_BLOCK) { + while(vol->building){ + if (virCondWait(&pool->cond, &pool->lock) < 0){ + virStorageReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("unable to wait on storage condition")); + goto cleanup; + } + } + } } +deletevol: if (!backend->deleteVol) { virStorageReportError(VIR_ERR_NO_SUPPORT, "%s", _("storage pool does not support vol deletion")); -- 1.7.1
participants (6)
-
Daniel P. Berrange -
Daniel Veillard -
Dave Allan -
Guannan Ren -
Jiri Denemark -
Matthew Booth