[6/6] conf: sync error reporting for object configFile unlinking

Message ID 4f5de6852f65c4b7c305b05975a8c4ed05b36387.1466640165.git.crobinso@redhat.com
State New
Headers show

Commit Message

Cole Robinson June 23, 2016, 12:12 a.m.
The various object implementations for configFile unlinking
have subtly different error handling behavior. Sync the impls
to use a single error string, and consistently ignore ENOENT,
to allow undefining an object whose configFile was deleted
behind libvirt's back
---
 src/conf/domain_conf.c   | 7 ++-----
 src/conf/network_conf.c  | 6 ++----
 src/conf/nwfilter_conf.c | 6 ++----
 src/conf/storage_conf.c  | 7 +++----
 src/conf/virsecretobj.c  | 2 +-
 5 files changed, 10 insertions(+), 18 deletions(-)

-- 
2.7.4

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

Comments

Cole Robinson June 24, 2016, 1:48 p.m. | #1
On 06/24/2016 09:38 AM, Ján Tomko wrote:
> On Wed, Jun 22, 2016 at 08:12:16PM -0400, Cole Robinson wrote:

>> The various object implementations for configFile unlinking

>> have subtly different error handling behavior. Sync the impls

>> to use a single error string, and consistently ignore ENOENT,

>> to allow undefining an object whose configFile was deleted

>> behind libvirt's back

>> ---

>> src/conf/domain_conf.c   | 7 ++-----

>> src/conf/network_conf.c  | 6 ++----

>> src/conf/nwfilter_conf.c | 6 ++----

>> src/conf/storage_conf.c  | 7 +++----

>> src/conf/virsecretobj.c  | 2 +-

>> 5 files changed, 10 insertions(+), 18 deletions(-)

>>

>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c

>> index 75ad03f..9e5af3f 100644

>> --- a/src/conf/domain_conf.c

>> +++ b/src/conf/domain_conf.c

>> @@ -23881,11 +23881,8 @@ virDomainDeleteConfig(const char *configDir,

>>     unlink(autostartLink);

>>     dom->autostart = 0;

>>

>> -    if (unlink(configFile) < 0 &&

>> -        errno != ENOENT) {

>> -        virReportSystemError(errno,

>> -                             _("cannot remove config %s"),

>> -                             configFile);

>> +    if (unlink(configFile) < 0 && errno != ENOENT) {

>> +        virReportSystemError(errno, _("cannot remove config %s"), configFile);

>>         goto cleanup;

> 

> Using a helper function that ignores errno and only calls

> virReportSystemError from one place would be even more consistent.

> 


Okay, something like virFileUnlinkSkipMissing in virfile.c ?  (I suck at
function names) Or something specific to this driver state handling? if the
latter, I don't know where it should live...

Thanks,
Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Cole Robinson June 24, 2016, 1:58 p.m. | #2
On 06/24/2016 09:57 AM, Ján Tomko wrote:
> On Fri, Jun 24, 2016 at 09:48:12AM -0400, Cole Robinson wrote:

>> On 06/24/2016 09:38 AM, Ján Tomko wrote:

>>> On Wed, Jun 22, 2016 at 08:12:16PM -0400, Cole Robinson wrote:

>>>> The various object implementations for configFile unlinking

>>>> have subtly different error handling behavior. Sync the impls

>>>> to use a single error string, and consistently ignore ENOENT,

>>>> to allow undefining an object whose configFile was deleted

>>>> behind libvirt's back

>>>> ---

>>>> src/conf/domain_conf.c   | 7 ++-----

>>>> src/conf/network_conf.c  | 6 ++----

>>>> src/conf/nwfilter_conf.c | 6 ++----

>>>> src/conf/storage_conf.c  | 7 +++----

>>>> src/conf/virsecretobj.c  | 2 +-

>>>> 5 files changed, 10 insertions(+), 18 deletions(-)

>>>>

> 

>>>> -    if (unlink(configFile) < 0 &&

>>>> -        errno != ENOENT) {

>>>> -        virReportSystemError(errno,

>>>> -                             _("cannot remove config %s"),

>>>> -                             configFile);

>>>> +    if (unlink(configFile) < 0 && errno != ENOENT) {

>>>> +        virReportSystemError(errno, _("cannot remove config %s"),

>>>> configFile);

>>>>         goto cleanup;

>>>

>>> Using a helper function that ignores errno and only calls

>>> virReportSystemError from one place would be even more consistent.

>>>

>>

>> Okay, something like virFileUnlinkSkipMissing in virfile.c ?  (I suck at

>> function names) Or something specific to this driver state handling? if the

>> latter, I don't know where it should live...

>>

> 

> I created virDirOpenIfExists with those semantics recently,

> but I'm not really proud of the name.

> 

> virfile.c should be the right place.


virFileUnlinkIfExists is better than mine anyways, and at least it will be
consistent, so I'll go with that

Thanks,
Cole

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

Patch

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 75ad03f..9e5af3f 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -23881,11 +23881,8 @@  virDomainDeleteConfig(const char *configDir,
     unlink(autostartLink);
     dom->autostart = 0;
 
-    if (unlink(configFile) < 0 &&
-        errno != ENOENT) {
-        virReportSystemError(errno,
-                             _("cannot remove config %s"),
-                             configFile);
+    if (unlink(configFile) < 0 && errno != ENOENT) {
+        virReportSystemError(errno, _("cannot remove config %s"), configFile);
         goto cleanup;
     }
 
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 02b8cd7..4732766 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -3319,10 +3319,8 @@  int virNetworkDeleteConfig(const char *configDir,
     unlink(autostartLink);
     net->autostart = 0;
 
-    if (unlink(configFile) < 0) {
-        virReportSystemError(errno,
-                             _("cannot remove config file '%s'"),
-                             configFile);
+    if (unlink(configFile) < 0 && errno != ENOENT) {
+        virReportSystemError(errno, _("cannot remove config %s"), configFile);
         goto error;
     }
 
diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c
index 3f90f65..64dbe8b 100644
--- a/src/conf/nwfilter_conf.c
+++ b/src/conf/nwfilter_conf.c
@@ -3283,10 +3283,8 @@  virNWFilterObjDeleteDef(const char *configDir,
         goto error;
     }
 
-    if (unlink(configFile) < 0) {
-        virReportError(VIR_ERR_INTERNAL_ERROR,
-                       _("cannot remove config for %s"),
-                       nwfilter->def->name);
+    if (unlink(configFile) < 0 && errno != ENOENT) {
+        virReportSystemError(errno, _("cannot remove config %s"), configFile);
         goto error;
     }
 
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 6932195..e45e197 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -2129,10 +2129,9 @@  virStoragePoolObjDeleteDef(virStoragePoolObjPtr pool)
         return -1;
     }
 
-    if (unlink(pool->configFile) < 0) {
-        virReportError(VIR_ERR_INTERNAL_ERROR,
-                       _("cannot remove config for %s"),
-                       pool->def->name);
+    if (unlink(pool->configFile) < 0 && errno != ENOENT) {
+        virReportSystemError(errno, _("cannot remove config %s"),
+                             pool->configFile);
         return -1;
     }
 
diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
index c46d22c..2a5cd31 100644
--- a/src/conf/virsecretobj.c
+++ b/src/conf/virsecretobj.c
@@ -663,7 +663,7 @@  virSecretObjDeleteConfig(virSecretObjPtr secret)
 {
     if (!secret->def->isephemeral &&
         unlink(secret->configFile) < 0 && errno != ENOENT) {
-        virReportSystemError(errno, _("cannot unlink '%s'"),
+        virReportSystemError(errno, _("cannot remove config %s"),
                              secret->configFile);
         return -1;
     }