[libvirt] [PATCH 0/6] storage: fix issues with pool state cleanup
This series fixes several issues with pool shutdown cleanup regarding transient pool removal, newDef assignment, and stateFile removal. Cole Robinson (6): storage: Break out storageDriverAutostartPool storage: Add storagePoolSetInactive storage: Handle transient pool removal in driver startup storage: delete: Don't unlink stateFile storage: Unlink stateFile in storagePoolSetInactive conf: sync error reporting for object configFile unlinking 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 +- src/storage/storage_driver.c | 198 ++++++++++++++++++++++--------------------- 6 files changed, 113 insertions(+), 113 deletions(-) -- 2.7.4
For autostarting a single pool. Lets us exit the function early which simplifies the control flow, and it matches the pattern of storagePoolUpdateAllState --- src/storage/storage_driver.c | 84 +++++++++++++++++++++++--------------------- 1 file changed, 43 insertions(+), 41 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index e2d729f..3bdc13f 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -149,6 +149,48 @@ storagePoolUpdateAllState(void) } static void +storageDriverAutostartPool(virConnectPtr conn, + virStoragePoolObjPtr pool) +{ + virStorageBackendPtr backend; + char *stateFile = NULL; + + if ((backend = virStorageBackendForType(pool->def->type)) == NULL) + goto cleanup; + + if (virStoragePoolObjIsActive(pool) || !pool->autostart) + goto cleanup; + + if (backend->startPool && + backend->startPool(conn, pool) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to autostart storage pool '%s': %s"), + pool->def->name, virGetLastErrorMessage()); + goto cleanup; + } + + virStoragePoolObjClearVols(pool); + stateFile = virFileBuildPath(driver->stateDir, + pool->def->name, ".xml"); + if (!stateFile || + virStoragePoolSaveState(stateFile, pool->def) < 0 || + backend->refreshPool(conn, pool) < 0) { + if (stateFile) + unlink(stateFile); + if (backend->stopPool) + backend->stopPool(conn, pool); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to autostart storage pool '%s': %s"), + pool->def->name, virGetLastErrorMessage()); + goto cleanup; + } + + pool->active = true; + cleanup: + VIR_FREE(stateFile); +} + +static void storageDriverAutostart(void) { size_t i; @@ -163,49 +205,9 @@ storageDriverAutostart(void) for (i = 0; i < driver->pools.count; i++) { virStoragePoolObjPtr pool = driver->pools.objs[i]; - virStorageBackendPtr backend; - bool started = false; virStoragePoolObjLock(pool); - if ((backend = virStorageBackendForType(pool->def->type)) == NULL) { - virStoragePoolObjUnlock(pool); - continue; - } - - if (pool->autostart && - !virStoragePoolObjIsActive(pool)) { - if (backend->startPool && - backend->startPool(conn, pool) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to autostart storage pool '%s': %s"), - pool->def->name, virGetLastErrorMessage()); - virStoragePoolObjUnlock(pool); - continue; - } - started = true; - } - - if (started) { - char *stateFile; - - virStoragePoolObjClearVols(pool); - stateFile = virFileBuildPath(driver->stateDir, - pool->def->name, ".xml"); - if (!stateFile || - virStoragePoolSaveState(stateFile, pool->def) < 0 || - backend->refreshPool(conn, pool) < 0) { - if (stateFile) - unlink(stateFile); - if (backend->stopPool) - backend->stopPool(conn, pool); - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to autostart storage pool '%s': %s"), - pool->def->name, virGetLastErrorMessage()); - } else { - pool->active = true; - } - VIR_FREE(stateFile); - } + storageDriverAutostartPool(conn, pool); virStoragePoolObjUnlock(pool); } -- 2.7.4
On Wed, Jun 22, 2016 at 08:12:11PM -0400, Cole Robinson wrote:
For autostarting a single pool. Lets us exit the function early which simplifies the control flow, and it matches the pattern of storagePoolUpdateAllState --- src/storage/storage_driver.c | 84 +++++++++++++++++++++++--------------------- 1 file changed, 43 insertions(+), 41 deletions(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index e2d729f..3bdc13f 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -149,6 +149,48 @@ storagePoolUpdateAllState(void) }
static void +storageDriverAutostartPool(virConnectPtr conn, + virStoragePoolObjPtr pool) +{ + virStorageBackendPtr backend; + char *stateFile = NULL; + + if ((backend = virStorageBackendForType(pool->def->type)) == NULL) + goto cleanup; +
return;
+ if (virStoragePoolObjIsActive(pool) || !pool->autostart) + goto cleanup; +
return;
+ if (backend->startPool && + backend->startPool(conn, pool) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to autostart storage pool '%s': %s"), + pool->def->name, virGetLastErrorMessage());
The error message could be shared with an 'error' label. Jan
+ goto cleanup; + } + + virStoragePoolObjClearVols(pool); + stateFile = virFileBuildPath(driver->stateDir, + pool->def->name, ".xml"); + if (!stateFile || + virStoragePoolSaveState(stateFile, pool->def) < 0 || + backend->refreshPool(conn, pool) < 0) { + if (stateFile) + unlink(stateFile); + if (backend->stopPool) + backend->stopPool(conn, pool); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to autostart storage pool '%s': %s"), + pool->def->name, virGetLastErrorMessage()); + goto cleanup;
This function handles newDef assignment and transient pool removal when an object is set inactive. The return value notifies callers if the pool was removed, so they know not to try to access the pool object anymore. Some users don't gain anything from it at this point, but future patches will improve that. --- src/storage/storage_driver.c | 54 +++++++++++++++++++++++++++++++------------- 1 file changed, 38 insertions(+), 16 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 3bdc13f..c7ffea8 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -76,6 +76,32 @@ static void storageDriverUnlock(void) virMutexUnlock(&driver->lock); } +/* + * storagePoolSetInactive: + * Helper for setting a pool object as 'inactive'. Handles reassigning + * newDef for persistent pools, and removing and freeing the object + * for transient pools. + * + * Returns true if pool was removed from driver->pools + */ +static bool +storagePoolSetInactive(virStoragePoolObjPtr pool) +{ + bool ret = false; + pool->active = false; + + if (pool->configFile == NULL) { + virStoragePoolObjRemove(&driver->pools, pool); + ret = true; + } else if (pool->newDef) { + virStoragePoolDefFree(pool->def); + pool->def = pool->newDef; + pool->newDef = NULL; + } + + return ret; +} + static void storagePoolUpdateState(virStoragePoolObjPtr pool) { @@ -182,6 +208,10 @@ storageDriverAutostartPool(virConnectPtr conn, virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to autostart storage pool '%s': %s"), pool->def->name, virGetLastErrorMessage()); + + /* Don't check the return value, it should never be 'true' here + * since this function requires a non-transient pool */ + storagePoolSetInactive(pool); goto cleanup; } @@ -739,8 +769,8 @@ storagePoolCreateXML(virConnectPtr conn, unlink(stateFile); if (backend->stopPool) backend->stopPool(conn, pool); - virStoragePoolObjRemove(&driver->pools, pool); - pool = NULL; + if (storagePoolSetInactive(pool)) + pool = NULL; goto cleanup; } @@ -956,6 +986,10 @@ storagePoolCreate(virStoragePoolPtr obj, unlink(stateFile); if (backend->stopPool) backend->stopPool(obj->conn, pool); + + /* Don't check the return value, it should never be 'true' here + * since this function requires a non-transient pool */ + storagePoolSetInactive(pool); goto cleanup; } @@ -1070,16 +1104,8 @@ storagePoolDestroy(virStoragePoolPtr obj) VIR_STORAGE_POOL_EVENT_STOPPED, 0); - pool->active = false; - - if (pool->configFile == NULL) { - virStoragePoolObjRemove(&driver->pools, pool); + if (storagePoolSetInactive(pool)) pool = NULL; - } else if (pool->newDef) { - virStoragePoolDefFree(pool->def); - pool->def = pool->newDef; - pool->newDef = NULL; - } ret = 0; @@ -1199,12 +1225,8 @@ storagePoolRefresh(virStoragePoolPtr obj, pool->def->uuid, VIR_STORAGE_POOL_EVENT_STOPPED, 0); - pool->active = false; - - if (pool->configFile == NULL) { - virStoragePoolObjRemove(&driver->pools, pool); + if (storagePoolSetInactive(pool)) pool = NULL; - } goto cleanup; } -- 2.7.4
On Wed, Jun 22, 2016 at 08:12:12PM -0400, Cole Robinson wrote:
This function handles newDef assignment and transient pool removal when an object is set inactive. The return value notifies callers if the pool was removed, so they know not to try to access the pool object anymore.
Some users don't gain anything from it at this point, but future patches will improve that. --- src/storage/storage_driver.c | 54 +++++++++++++++++++++++++++++++------------- 1 file changed, 38 insertions(+), 16 deletions(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 3bdc13f..c7ffea8 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -76,6 +76,32 @@ static void storageDriverUnlock(void) virMutexUnlock(&driver->lock); }
+/* + * storagePoolSetInactive: + * Helper for setting a pool object as 'inactive'. Handles reassigning + * newDef for persistent pools, and removing and freeing the object + * for transient pools. + * + * Returns true if pool was removed from driver->pools + */ +static bool +storagePoolSetInactive(virStoragePoolObjPtr pool)
Can be void (virStoragePoolObjPtr *pool) and clear the original pointer if it was freed. Jan
On 06/24/2016 09:26 AM, Ján Tomko wrote:
On Wed, Jun 22, 2016 at 08:12:12PM -0400, Cole Robinson wrote:
This function handles newDef assignment and transient pool removal when an object is set inactive. The return value notifies callers if the pool was removed, so they know not to try to access the pool object anymore.
Some users don't gain anything from it at this point, but future patches will improve that. --- src/storage/storage_driver.c | 54 +++++++++++++++++++++++++++++++------------- 1 file changed, 38 insertions(+), 16 deletions(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 3bdc13f..c7ffea8 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -76,6 +76,32 @@ static void storageDriverUnlock(void) virMutexUnlock(&driver->lock); }
+/* + * storagePoolSetInactive: + * Helper for setting a pool object as 'inactive'. Handles reassigning + * newDef for persistent pools, and removing and freeing the object + * for transient pools. + * + * Returns true if pool was removed from driver->pools + */ +static bool +storagePoolSetInactive(virStoragePoolObjPtr pool)
Can be void (virStoragePoolObjPtr *pool) and clear the original pointer if it was freed.
That is what Jovanka's similar patch did, but after playing with it some I changed it to this. IMO it's safer to require the caller to set pool=NULL, otherwise it isn't exactly clear just scanning the caller's code that pool can become NULL, possibly leading to accidental NULL dereference if the code is extended in the future, or new uses of the storagePoolSetInactive are added. (I probably should have strapped on an ATTRIBUTE_RETURN_CHECK to be extra safe) But I'm not married to the idea, so if you still prefer the other way I can change it. Thanks, Cole
Currently transient storage pools can end up in a shutoff state via driver startup https://bugzilla.redhat.com/show_bug.cgi?id=1227475 Use storagePoolSetInactive to handle transient pool removal in this case. There's some weirdness here though because this can happen while we are iterating over the storage pool list --- src/storage/storage_driver.c | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index c7ffea8..45f00e0 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -102,12 +102,16 @@ storagePoolSetInactive(virStoragePoolObjPtr pool) return ret; } -static void +/* + * Returns true if pool was removed from driver->pools, via + * storagePoolSetInactive + */ +static bool storagePoolUpdateState(virStoragePoolObjPtr pool) { - bool active; + bool active = false; virStorageBackendPtr backend; - int ret = -1; + bool ret = false; char *stateFile; if (!(stateFile = virFileBuildPath(driver->stateDir, @@ -123,7 +127,6 @@ storagePoolUpdateState(virStoragePoolObjPtr pool) /* Backends which do not support 'checkPool' are considered * inactive by default. */ - active = false; if (backend->checkPool && backend->checkPool(pool, &active) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -149,15 +152,15 @@ storagePoolUpdateState(virStoragePoolObjPtr pool) } pool->active = active; - ret = 0; error: - if (ret < 0) { + if (!active) { + ret = storagePoolSetInactive(pool); if (stateFile) unlink(stateFile); } VIR_FREE(stateFile); - return; + return ret; } static void @@ -169,7 +172,13 @@ storagePoolUpdateAllState(void) virStoragePoolObjPtr pool = driver->pools.objs[i]; virStoragePoolObjLock(pool); - storagePoolUpdateState(pool); + if (storagePoolUpdateState(pool)) { + /* The transient pool was removed from the list, while we + are iterating over the list. Adjust the counter backwards + to match */ + --i; + continue; + } virStoragePoolObjUnlock(pool); } } -- 2.7.4
On Wed, Jun 22, 2016 at 08:12:13PM -0400, Cole Robinson wrote:
Currently transient storage pools can end up in a shutoff state via driver startup
https://bugzilla.redhat.com/show_bug.cgi?id=1227475
Use storagePoolSetInactive to handle transient pool removal in this case. There's some weirdness here though because this can happen while we are iterating over the storage pool list --- src/storage/storage_driver.c | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index c7ffea8..45f00e0 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -102,12 +102,16 @@ storagePoolSetInactive(virStoragePoolObjPtr pool) return ret; }
-static void +/* + * Returns true if pool was removed from driver->pools, via + * storagePoolSetInactive + */ +static bool
storagePoolUpdateState(virStoragePoolObjPtr pool) { - bool active; + bool active = false; virStorageBackendPtr backend; - int ret = -1; + bool ret = false; char *stateFile;
if (!(stateFile = virFileBuildPath(driver->stateDir, @@ -123,7 +127,6 @@ storagePoolUpdateState(virStoragePoolObjPtr pool) /* Backends which do not support 'checkPool' are considered * inactive by default. */ - active = false; if (backend->checkPool && backend->checkPool(pool, &active) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -149,15 +152,15 @@ storagePoolUpdateState(virStoragePoolObjPtr pool) }
pool->active = active; - ret = 0; error: - if (ret < 0) { + if (!active) { + ret = storagePoolSetInactive(pool); if (stateFile) unlink(stateFile); } VIR_FREE(stateFile);
- return; + return ret; }
static void @@ -169,7 +172,13 @@ storagePoolUpdateAllState(void) virStoragePoolObjPtr pool = driver->pools.objs[i];
virStoragePoolObjLock(pool); - storagePoolUpdateState(pool); + if (storagePoolUpdateState(pool)) {
UpdateState should update the state, not remove pools from the list. If it marks it as inactive, then we'd call storagePoolSetInactive here, or even better, a subset of it since we do not need to iterate over the pool array to find the pool index - we already have it. Jan
+ /* The transient pool was removed from the list, while we + are iterating over the list. Adjust the counter backwards + to match */ + --i; + continue; + } virStoragePoolObjUnlock(pool); } } -- 2.7.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
PoolDelete only works on an inactive VM, which shouldn't have a stateFile on disk, so this isn't the place to attempt to remove it. --- src/storage/storage_driver.c | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 45f00e0..9e97f62 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1133,7 +1133,6 @@ storagePoolDelete(virStoragePoolPtr obj, { virStoragePoolObjPtr pool; virStorageBackendPtr backend; - char *stateFile = NULL; int ret = -1; if (!(pool = virStoragePoolObjFromStoragePool(obj))) @@ -1161,14 +1160,6 @@ storagePoolDelete(virStoragePoolPtr obj, goto cleanup; } - if (!(stateFile = virFileBuildPath(driver->stateDir, - pool->def->name, - ".xml"))) - goto cleanup; - - unlink(stateFile); - VIR_FREE(stateFile); - if (!backend->deletePool) { virReportError(VIR_ERR_NO_SUPPORT, "%s", _("pool does not support pool deletion")); -- 2.7.4
On Wed, Jun 22, 2016 at 08:12:14PM -0400, Cole Robinson wrote:
PoolDelete only works on an inactive VM, which shouldn't have a stateFile on disk, so this isn't the place to attempt to remove it. --- src/storage/storage_driver.c | 9 --------- 1 file changed, 9 deletions(-)
ACK Jan
There are several places in the storage driver where a runtime stateFile is not being unlinked when the pool is shutdown. Move the unlinking to storagePoolSetInactive to fix those locations One minor semantic change is that we no longer fail PoolDestroy if we fail to build the stateFile path string, but I don't think that matters. --- src/storage/storage_driver.c | 32 ++++++++------------------------ 1 file changed, 8 insertions(+), 24 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 9e97f62..8f8d098 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -89,6 +89,13 @@ storagePoolSetInactive(virStoragePoolObjPtr pool) { bool ret = false; pool->active = false; + char *stateFile; + + stateFile = virFileBuildPath(driver->stateDir, pool->def->name, ".xml"); + if (stateFile) { + unlink(stateFile); + VIR_FREE(stateFile); + } if (pool->configFile == NULL) { virStoragePoolObjRemove(&driver->pools, pool); @@ -112,11 +119,6 @@ storagePoolUpdateState(virStoragePoolObjPtr pool) bool active = false; virStorageBackendPtr backend; bool ret = false; - char *stateFile; - - if (!(stateFile = virFileBuildPath(driver->stateDir, - pool->def->name, ".xml"))) - goto error; if ((backend = virStorageBackendForType(pool->def->type)) == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -153,12 +155,8 @@ storagePoolUpdateState(virStoragePoolObjPtr pool) pool->active = active; error: - if (!active) { + if (!active) ret = storagePoolSetInactive(pool); - if (stateFile) - unlink(stateFile); - } - VIR_FREE(stateFile); return ret; } @@ -210,8 +208,6 @@ storageDriverAutostartPool(virConnectPtr conn, if (!stateFile || virStoragePoolSaveState(stateFile, pool->def) < 0 || backend->refreshPool(conn, pool) < 0) { - if (stateFile) - unlink(stateFile); if (backend->stopPool) backend->stopPool(conn, pool); virReportError(VIR_ERR_INTERNAL_ERROR, @@ -774,8 +770,6 @@ storagePoolCreateXML(virConnectPtr conn, virStoragePoolObjClearVols(pool); if (!stateFile || virStoragePoolSaveState(stateFile, pool->def) < 0 || backend->refreshPool(conn, pool) < 0) { - if (stateFile) - unlink(stateFile); if (backend->stopPool) backend->stopPool(conn, pool); if (storagePoolSetInactive(pool)) @@ -991,8 +985,6 @@ storagePoolCreate(virStoragePoolPtr obj, virStoragePoolObjClearVols(pool); if (!stateFile || virStoragePoolSaveState(stateFile, pool->def) < 0 || backend->refreshPool(obj->conn, pool) < 0) { - if (stateFile) - unlink(stateFile); if (backend->stopPool) backend->stopPool(obj->conn, pool); @@ -1060,7 +1052,6 @@ storagePoolDestroy(virStoragePoolPtr obj) virStoragePoolObjPtr pool; virStorageBackendPtr backend; virObjectEventPtr event = NULL; - char *stateFile = NULL; int ret = -1; storageDriverLock(); @@ -1094,13 +1085,6 @@ storagePoolDestroy(virStoragePoolPtr obj) goto cleanup; } - if (!(stateFile = virFileBuildPath(driver->stateDir, - pool->def->name, - ".xml"))) - goto cleanup; - - unlink(stateFile); - VIR_FREE(stateFile); if (backend->stopPool && backend->stopPool(obj->conn, pool) < 0) -- 2.7.4
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; } 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; } -- 2.7.4
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. Jan
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
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. Jan
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
participants (2)
-
Cole Robinson -
Ján Tomko