7:08 p.m.
https://bugzilla.redhat.com/show_bug.cgi?id=1092882
Changes as a result of commit id '0c2305b3', see:
http://www.redhat.com/archives/libvir-list/2014-April/msg00375.html
Resulted in a regression when there was a failure to buildVol the
volume wasn't removed from the storage driver list because the wrong
local storage pointer was passed to the DeleteInternal routine.
Additionally a preexisting condition where if deleteVol didn't exist
or failed, the volume would not be removed from volumes.objs[] even
though it doesn't exist. This results in virsh vol-list and vol-info
not finding the volume and emitting error messages.
John Ferlan (2):
storage: Need to ensure removal of voldef from driver list
storage: Resolve issues in failure path
src/storage/storage_driver.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
--
1.9.0
7:08 p.m.
New subject: [libvirt] [PATCH 1/2] storage: Need to ensure removal of voldef from driver list
If for some reason a volume cannot be built in the pool (eg, the
buildVol call fails), then the error code may or may not properly
remove the volume from the pool->volumes.objs[] list.
Removal from the list is dependent on the 'deleteVol()' being present
in the backend *and* calling it returning success. If either failed,
then the volobj would never be removed from the list until a pool
refresh was done (or libvirtd restarted).
Note: This is true prior to commit id '0c2305b3' as well where
the cleanup would have been called prior to removal, but removal
never occurred due to various goto statements.
Since the volume would have been added to the list prior to the
attempt to build the volume, modify the storageVolDeleteInternal()
to perform removal during cleanup.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/storage/storage_driver.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 542b382..789744f 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -1581,6 +1581,9 @@ storageVolDeleteInternal(virStorageVolPtr obj,
pool->def->available += vol->target.allocation;
}
+ ret = 0;
+
+ cleanup:
for (i = 0; i < pool->volumes.count; i++) {
if (pool->volumes.objs[i] == vol) {
VIR_INFO("Deleting volume '%s' from storage pool
'%s'",
@@ -1591,9 +1594,6 @@ storageVolDeleteInternal(virStorageVolPtr obj,
break;
}
}
- ret = 0;
-
- cleanup:
return ret;
}
--
1.9.0
7:52 a.m.
New subject: [libvirt] [PATCH 1/2] storage: Need to ensure removal of voldef from driver list
On 05/07/2014 02:08 AM, John Ferlan wrote:
If for some reason a volume cannot be built in the pool (eg, the
buildVol call fails), then the error code may or may not properly
remove the volume from the pool->volumes.objs[] list.
Removal from the list is dependent on the 'deleteVol()' being present
in the backend *and* calling it returning success. If either failed,
then the volobj would never be removed from the list until a pool
refresh was done (or libvirtd restarted).
Note: This is true prior to commit id '0c2305b3' as well where
the cleanup would have been called prior to removal, but removal
never occurred due to various goto statements.
Since the volume would have been added to the list prior to the
attempt to build the volume, modify the storageVolDeleteInternal()
to perform removal during cleanup.
This makes sense when we're cleaning up after a failed buildVol, but we
shouldn't be removing the volume from the list when the StorageVolumeDelete
API was called and volume deletion is not supported by the backend or when
the deletion failed.
Jan
9:01 a.m.
New subject: [libvirt] [PATCH 1/2] storage: Need to ensure removal of voldef from driver list
On 05/07/2014 08:52 AM, Ján Tomko wrote:
On 05/07/2014 02:08 AM, John Ferlan wrote:
> If for some reason a volume cannot be built in the pool (eg, the
> buildVol call fails), then the error code may or may not properly
> remove the volume from the pool->volumes.objs[] list.
>
> Removal from the list is dependent on the 'deleteVol()' being present
> in the backend *and* calling it returning success. If either failed,
> then the volobj would never be removed from the list until a pool
> refresh was done (or libvirtd restarted).
>
> Note: This is true prior to commit id '0c2305b3' as well where
> the cleanup would have been called prior to removal, but removal
> never occurred due to various goto statements.
>
> Since the volume would have been added to the list prior to the
> attempt to build the volume, modify the storageVolDeleteInternal()
> to perform removal during cleanup.
This makes sense when we're cleaning up after a failed buildVol, but we
shouldn't be removing the volume from the list when the StorageVolumeDelete
API was called and volume deletion is not supported by the backend or when
the deletion failed.
Jan
Hmm. ugh. I see your point. Guess I was looking at it only from the
point of view of buildVol failure... Still if buildVol fails and
deleteVol fails, then we still could have the volume listed in the pool
objs list - although that's existing and a different issue.
In any case, I'll remove this change and just push 2/2.
John
7:08 p.m.
New subject: [libvirt] [PATCH 2/2] storage: Resolve issues in failure path
https://bugzilla.redhat.com/show_bug.cgi?id=1092882
Refactoring in commit id '0c2305b3' resulted in the wrong storage
volume object being passed to the new storageVolDeleteInternal().
It should have passed 'voldef' which is the address found in the
pool->volumes.objs[i] array. By passing 'voldef', the DeleteInternal
code will find and remove the voldef from the volumes.objs[] list.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/storage/storage_driver.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 789744f..8299824 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -1766,12 +1766,11 @@ storageVolCreateXML(virStoragePoolPtr obj,
voldef->building = 0;
pool->asyncjobs--;
- voldef = NULL;
-
if (buildret < 0) {
- storageVolDeleteInternal(volobj, backend, pool, buildvoldef,
+ VIR_FREE(buildvoldef);
+ storageVolDeleteInternal(volobj, backend, pool, voldef,
0, false);
- buildvoldef = NULL;
+ voldef = NULL;
goto cleanup;
}
--
1.9.0
7:52 a.m.
New subject: [libvirt] [PATCH 2/2] storage: Resolve issues in failure path
On 05/07/2014 02:08 AM, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1092882
Refactoring in commit id '0c2305b3' resulted in the wrong storage
volume object being passed to the new storageVolDeleteInternal().
It should have passed 'voldef' which is the address found in the
pool->volumes.objs[i] array. By passing 'voldef', the DeleteInternal
code will find and remove the voldef from the volumes.objs[] list.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/storage/storage_driver.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
ACK
Jan
3852
days inactive
3852
days old
5 comments
2 participants
participants (2)
-
John Ferlan -
Ján Tomko