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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list