[libvirt] [PATCH 0/5] Cleanup some storage driver paths
See the patches for details - everything leads to the last one. John Ferlan (5): storage: Clean up stateFile if refreshPool fails storage: Clean up storagePoolUpdateStateCallback processing storage: Create error label path for storagePoolCreateXML storage: Introduce storagePoolRefreshFailCleanup storage: Save error during refresh failure processing src/storage/storage_driver.c | 73 ++++++++++++++++++++---------------- 1 file changed, 40 insertions(+), 33 deletions(-) -- 2.17.1
If the virStoragePoolRefresh fails and we call stopPool, the code neglected to clean up the state file leading to the next libvirtd restart attempting to start the pool. For a transient pool this could make it unexpectedly reappear. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_driver.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 8943df1f84..1dbeb213e3 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1174,8 +1174,13 @@ storagePoolRefresh(virStoragePoolPtr pool, virStoragePoolObjClearVols(obj); if (backend->refreshPool(obj) < 0) { + char *stateFile = virFileBuildPath(driver->stateDir, def->name, ".xml"); + + if (stateFile) + unlink(stateFile); if (backend->stopPool) backend->stopPool(obj); + VIR_FREE(stateFile); event = virStoragePoolEventLifecycleNew(def->name, def->uuid, -- 2.17.1
Alter the code path to remove the need to to go cleanup and thus remove the label completely. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_driver.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 1dbeb213e3..d0e7e6904c 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -111,15 +111,15 @@ storagePoolUpdateStateCallback(virStoragePoolObjPtr obj, virStorageBackendPtr backend; char *stateFile; - if (!(stateFile = virFileBuildPath(driver->stateDir, def->name, ".xml"))) - goto cleanup; - if ((backend = virStorageBackendForType(def->type)) == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Missing backend %d"), def->type); - goto cleanup; + return; } + if (!(stateFile = virFileBuildPath(driver->stateDir, def->name, ".xml"))) + return; + /* Backends which do not support 'checkPool' are considered * inactive by default. */ if (backend->checkPool && @@ -151,8 +151,7 @@ storagePoolUpdateStateCallback(virStoragePoolObjPtr obj, if (!virStoragePoolObjIsActive(obj)) virStoragePoolUpdateInactive(&obj); - cleanup: - if (!active && stateFile) + if (!active) ignore_value(unlink(stateFile)); VIR_FREE(stateFile); -- 2.17.1
Rather than duplicate the error code, let's create an error label to keep code common. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_driver.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index d0e7e6904c..5a8871bd07 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -730,12 +730,8 @@ storagePoolCreateXML(virConnectPtr conn, } if (backend->startPool && - backend->startPool(obj) < 0) { - virStoragePoolObjRemove(driver->pools, obj); - virObjectUnref(obj); - obj = NULL; - goto cleanup; - } + backend->startPool(obj) < 0) + goto error; stateFile = virFileBuildPath(driver->stateDir, def->name, ".xml"); @@ -746,10 +742,7 @@ storagePoolCreateXML(virConnectPtr conn, unlink(stateFile); if (backend->stopPool) backend->stopPool(obj); - virStoragePoolObjRemove(driver->pools, obj); - virObjectUnref(obj); - obj = NULL; - goto cleanup; + goto error; } event = virStoragePoolEventLifecycleNew(def->name, @@ -768,6 +761,12 @@ storagePoolCreateXML(virConnectPtr conn, virObjectEventStateQueue(driver->storageEventState, event); virStoragePoolObjEndAPI(&obj); return pool; + + error: + virStoragePoolObjRemove(driver->pools, obj); + virObjectUnref(obj); + obj = NULL; + goto cleanup; } static virStoragePoolPtr -- 2.17.1
Create a common pool refresh failure handling method as the same code is repeated multiple times. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_driver.c | 38 +++++++++++++++++------------------- 1 file changed, 18 insertions(+), 20 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 5a8871bd07..8aa3191f7b 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -79,6 +79,18 @@ static void storageDriverUnlock(void) } +static void +storagePoolRefreshFailCleanup(virStorageBackendPtr backend, + virStoragePoolObjPtr obj, + const char *stateFile) +{ + if (stateFile) + ignore_value(unlink(stateFile)); + if (backend->stopPool) + backend->stopPool(obj); +} + + /** * virStoragePoolUpdateInactive: * @poolptr: pointer to a variable holding the pool object pointer @@ -127,6 +139,7 @@ storagePoolUpdateStateCallback(virStoragePoolObjPtr obj, virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to initialize storage pool '%s': %s"), def->name, virGetLastErrorMessage()); + ignore_value(unlink(stateFile)); active = false; } @@ -137,8 +150,7 @@ storagePoolUpdateStateCallback(virStoragePoolObjPtr obj, if (active) { virStoragePoolObjClearVols(obj); if (backend->refreshPool(obj) < 0) { - if (backend->stopPool) - backend->stopPool(obj); + storagePoolRefreshFailCleanup(backend, obj, stateFile); virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to restart storage pool '%s': %s"), def->name, virGetLastErrorMessage()); @@ -151,8 +163,6 @@ storagePoolUpdateStateCallback(virStoragePoolObjPtr obj, if (!virStoragePoolObjIsActive(obj)) virStoragePoolUpdateInactive(&obj); - if (!active) - ignore_value(unlink(stateFile)); VIR_FREE(stateFile); return; @@ -199,10 +209,7 @@ storageDriverAutostartCallback(virStoragePoolObjPtr obj, if (!stateFile || virStoragePoolSaveState(stateFile, def) < 0 || backend->refreshPool(obj) < 0) { - if (stateFile) - unlink(stateFile); - if (backend->stopPool) - backend->stopPool(obj); + storagePoolRefreshFailCleanup(backend, obj, stateFile); virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to autostart storage pool '%s': %s"), def->name, virGetLastErrorMessage()); @@ -738,10 +745,7 @@ storagePoolCreateXML(virConnectPtr conn, virStoragePoolObjClearVols(obj); if (!stateFile || virStoragePoolSaveState(stateFile, def) < 0 || backend->refreshPool(obj) < 0) { - if (stateFile) - unlink(stateFile); - if (backend->stopPool) - backend->stopPool(obj); + storagePoolRefreshFailCleanup(backend, obj, stateFile); goto error; } @@ -939,10 +943,7 @@ storagePoolCreate(virStoragePoolPtr pool, virStoragePoolObjClearVols(obj); if (!stateFile || virStoragePoolSaveState(stateFile, def) < 0 || backend->refreshPool(obj) < 0) { - if (stateFile) - unlink(stateFile); - if (backend->stopPool) - backend->stopPool(obj); + storagePoolRefreshFailCleanup(backend, obj, stateFile); goto cleanup; } @@ -1174,10 +1175,7 @@ storagePoolRefresh(virStoragePoolPtr pool, if (backend->refreshPool(obj) < 0) { char *stateFile = virFileBuildPath(driver->stateDir, def->name, ".xml"); - if (stateFile) - unlink(stateFile); - if (backend->stopPool) - backend->stopPool(obj); + storagePoolRefreshFailCleanup(backend, obj, stateFile); VIR_FREE(stateFile); event = virStoragePoolEventLifecycleNew(def->name, -- 2.17.1
On 09/12/2018 06:09 PM, John Ferlan wrote:
Create a common pool refresh failure handling method as the same code is repeated multiple times.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_driver.c | 38 +++++++++++++++++------------------- 1 file changed, 18 insertions(+), 20 deletions(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 5a8871bd07..8aa3191f7b 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -79,6 +79,18 @@ static void storageDriverUnlock(void) }
+static void +storagePoolRefreshFailCleanup(virStorageBackendPtr backend, + virStoragePoolObjPtr obj, + const char *stateFile) +{ + if (stateFile) + ignore_value(unlink(stateFile));
I was about to ask this in 1/5. Is this ignore_value() needed? Quick `git grep' shows we are not consistent.
+ if (backend->stopPool) + backend->stopPool(obj); +} +
Michal
On 09/20/2018 03:30 AM, Michal Privoznik wrote:
On 09/12/2018 06:09 PM, John Ferlan wrote:
Create a common pool refresh failure handling method as the same code is repeated multiple times.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_driver.c | 38 +++++++++++++++++------------------- 1 file changed, 18 insertions(+), 20 deletions(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 5a8871bd07..8aa3191f7b 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -79,6 +79,18 @@ static void storageDriverUnlock(void) }
+static void +storagePoolRefreshFailCleanup(virStorageBackendPtr backend, + virStoragePoolObjPtr obj, + const char *stateFile) +{ + if (stateFile) + ignore_value(unlink(stateFile));
I was about to ask this in 1/5. Is this ignore_value() needed? Quick `git grep' shows we are not consistent.
True, in both cases though it's a copy of existing code. I'm assuming it's a Coverity thing though... As a test I just removed all ignore_value from unlink and ran a Coverity build with no issues. I'll generate and post a patch to remove them all shortly. Tks for the review - John
+ if (backend->stopPool) + backend->stopPool(obj); +} +
Michal
https://bugzilla.redhat.com/show_bug.cgi?id=1614283 Save the error from the refresh failure because the stopPool processing may overwrite the error or even worse clear it due to calling an external libvirt API that resets the last error such as is the case with the SCSI pool which may call virGetConnectNodeDev (see commit decaeb288) in order to process deleting an NPIV vport. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_driver.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 8aa3191f7b..f032d0dfdd 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -84,10 +84,16 @@ storagePoolRefreshFailCleanup(virStorageBackendPtr backend, virStoragePoolObjPtr obj, const char *stateFile) { + virErrorPtr orig_err = virSaveLastError(); + if (stateFile) ignore_value(unlink(stateFile)); if (backend->stopPool) backend->stopPool(obj); + if (orig_err) { + virSetError(orig_err); + virFreeError(orig_err); + } } -- 2.17.1
ping? Tks, John On 09/12/2018 12:08 PM, John Ferlan wrote:
See the patches for details - everything leads to the last one.
John Ferlan (5): storage: Clean up stateFile if refreshPool fails storage: Clean up storagePoolUpdateStateCallback processing storage: Create error label path for storagePoolCreateXML storage: Introduce storagePoolRefreshFailCleanup storage: Save error during refresh failure processing
src/storage/storage_driver.c | 73 ++++++++++++++++++++---------------- 1 file changed, 40 insertions(+), 33 deletions(-)
On 09/12/2018 06:08 PM, John Ferlan wrote:
See the patches for details - everything leads to the last one.
John Ferlan (5): storage: Clean up stateFile if refreshPool fails storage: Clean up storagePoolUpdateStateCallback processing storage: Create error label path for storagePoolCreateXML storage: Introduce storagePoolRefreshFailCleanup storage: Save error during refresh failure processing
src/storage/storage_driver.c | 73 ++++++++++++++++++++---------------- 1 file changed, 40 insertions(+), 33 deletions(-)
ACK Michal
participants (2)
-
John Ferlan -
Michal Privoznik