[libvirt] [PATCH 0/2] storage: Fix error messages containing UUIDs
Peter Krempa (2): storage: pool: Fix handling of errors on pool lookup failure storage: volume: Rework lookup of volume objects src/storage/storage_driver.c | 555 +++++++++++++------------------------------ 1 file changed, 159 insertions(+), 396 deletions(-) -- 1.9.3
Rework internal pool lookup code to avoid printing the raw UUID buffer in the case a storage pool can't be found: $ virsh pool-name e012ace0-0460-5810-39ef-1bce5fa5a4dd error: failed to get pool 'e012ace0-0460-5810-39ef-1bce5fa5a4dd' error: Storage pool not found: no storage pool with matching uuid à¬à`X9ï_¥¤Ý The rework is mostly done by switching the lookup code to the newly introduced helper virStoragePoolObjFromStoragePoo Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1104993 --- src/storage/storage_driver.c | 263 +++++++++++++++---------------------------- 1 file changed, 90 insertions(+), 173 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 4f51517..c9916ff 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -275,9 +275,11 @@ storagePoolLookupByUUID(virConnectPtr conn, storageDriverUnlock(driver); if (!pool) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(uuid, uuidstr); virReportError(VIR_ERR_NO_STORAGE_POOL, - _("no storage pool with matching uuid %s"), uuid); - goto cleanup; + _("no storage pool with matching uuid '%s'"), uuidstr); + return NULL; } if (virStoragePoolLookupByUUIDEnsureACL(conn, pool->def) < 0) @@ -287,8 +289,7 @@ storagePoolLookupByUUID(virConnectPtr conn, NULL, NULL); cleanup: - if (pool) - virStoragePoolObjUnlock(pool); + virStoragePoolObjUnlock(pool); return ret; } @@ -307,7 +308,7 @@ storagePoolLookupByName(virConnectPtr conn, if (!pool) { virReportError(VIR_ERR_NO_STORAGE_POOL, _("no storage pool with matching name '%s'"), name); - goto cleanup; + return NULL; } if (virStoragePoolLookupByNameEnsureACL(conn, pool->def) < 0) @@ -317,8 +318,7 @@ storagePoolLookupByName(virConnectPtr conn, NULL, NULL); cleanup: - if (pool) - virStoragePoolObjUnlock(pool); + virStoragePoolObjUnlock(pool); return ret; } @@ -336,7 +336,7 @@ storagePoolLookupByVolume(virStorageVolPtr vol) if (!pool) { virReportError(VIR_ERR_NO_STORAGE_POOL, _("no storage pool with matching name '%s'"), vol->pool); - goto cleanup; + return NULL; } if (virStoragePoolLookupByVolumeEnsureACL(vol->conn, pool->def) < 0) @@ -346,8 +346,7 @@ storagePoolLookupByVolume(virStorageVolPtr vol) NULL, NULL); cleanup: - if (pool) - virStoragePoolObjUnlock(pool); + virStoragePoolObjUnlock(pool); return ret; } @@ -535,19 +534,33 @@ storageConnectFindStoragePoolSources(virConnectPtr conn, } -static int storagePoolIsActive(virStoragePoolPtr pool) +static virStoragePoolObjPtr +virStoragePoolObjFromStoragePool(virStoragePoolPtr pool) { virStorageDriverStatePtr driver = pool->conn->storagePrivateData; - virStoragePoolObjPtr obj; - int ret = -1; + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virStoragePoolObjPtr ret; storageDriverLock(driver); - obj = virStoragePoolObjFindByUUID(&driver->pools, pool->uuid); - storageDriverUnlock(driver); - if (!obj) { - virReportError(VIR_ERR_NO_STORAGE_POOL, NULL); - goto cleanup; + if (!(ret = virStoragePoolObjFindByUUID(&driver->pools, pool->uuid))) { + virUUIDFormat(pool->uuid, uuidstr); + virReportError(VIR_ERR_NO_STORAGE_POOL, + _("no storage pool with matching uuid '%s' (%s)"), + uuidstr, pool->name); } + storageDriverUnlock(driver); + + return ret; +} + + +static int storagePoolIsActive(virStoragePoolPtr pool) +{ + virStoragePoolObjPtr obj; + int ret = -1; + + if (!(obj = virStoragePoolObjFromStoragePool(pool))) + return -1; if (virStoragePoolIsActiveEnsureACL(pool->conn, obj->def) < 0) goto cleanup; @@ -555,24 +568,17 @@ static int storagePoolIsActive(virStoragePoolPtr pool) ret = virStoragePoolObjIsActive(obj); cleanup: - if (obj) - virStoragePoolObjUnlock(obj); + virStoragePoolObjUnlock(obj); return ret; } static int storagePoolIsPersistent(virStoragePoolPtr pool) { - virStorageDriverStatePtr driver = pool->conn->storagePrivateData; virStoragePoolObjPtr obj; int ret = -1; - storageDriverLock(driver); - obj = virStoragePoolObjFindByUUID(&driver->pools, pool->uuid); - storageDriverUnlock(driver); - if (!obj) { - virReportError(VIR_ERR_NO_STORAGE_POOL, NULL); - goto cleanup; - } + if (!(obj = virStoragePoolObjFromStoragePool(pool))) + return -1; if (virStoragePoolIsPersistentEnsureACL(pool->conn, obj->def) < 0) goto cleanup; @@ -580,8 +586,7 @@ static int storagePoolIsPersistent(virStoragePoolPtr pool) ret = obj->configFile ? 1 : 0; cleanup: - if (obj) - virStoragePoolObjUnlock(obj); + virStoragePoolObjUnlock(obj); return ret; } @@ -705,10 +710,12 @@ storagePoolUndefine(virStoragePoolPtr obj) int ret = -1; storageDriverLock(driver); - pool = virStoragePoolObjFindByUUID(&driver->pools, obj->uuid); - if (!pool) { + if (!(pool = virStoragePoolObjFindByUUID(&driver->pools, obj->uuid))) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(obj->uuid, uuidstr); virReportError(VIR_ERR_NO_STORAGE_POOL, - _("no storage pool with matching uuid %s"), obj->uuid); + _("no storage pool with matching uuid '%s' (%s)"), + uuidstr, obj->name); goto cleanup; } @@ -757,22 +764,14 @@ static int storagePoolCreate(virStoragePoolPtr obj, unsigned int flags) { - virStorageDriverStatePtr driver = obj->conn->storagePrivateData; virStoragePoolObjPtr pool; virStorageBackendPtr backend; int ret = -1; virCheckFlags(0, -1); - storageDriverLock(driver); - pool = virStoragePoolObjFindByUUID(&driver->pools, obj->uuid); - storageDriverUnlock(driver); - - if (!pool) { - virReportError(VIR_ERR_NO_STORAGE_POOL, - _("no storage pool with matching uuid %s"), obj->uuid); - goto cleanup; - } + if (!(pool = virStoragePoolObjFromStoragePool(obj))) + return -1; if (virStoragePoolCreateEnsureACL(obj->conn, pool->def) < 0) goto cleanup; @@ -801,8 +800,7 @@ storagePoolCreate(virStoragePoolPtr obj, ret = 0; cleanup: - if (pool) - virStoragePoolObjUnlock(pool); + virStoragePoolObjUnlock(pool); return ret; } @@ -810,20 +808,12 @@ static int storagePoolBuild(virStoragePoolPtr obj, unsigned int flags) { - virStorageDriverStatePtr driver = obj->conn->storagePrivateData; virStoragePoolObjPtr pool; virStorageBackendPtr backend; int ret = -1; - storageDriverLock(driver); - pool = virStoragePoolObjFindByUUID(&driver->pools, obj->uuid); - storageDriverUnlock(driver); - - if (!pool) { - virReportError(VIR_ERR_NO_STORAGE_POOL, - _("no storage pool with matching uuid %s"), obj->uuid); - goto cleanup; - } + if (!(pool = virStoragePoolObjFromStoragePool(obj))) + return -1; if (virStoragePoolBuildEnsureACL(obj->conn, pool->def) < 0) goto cleanup; @@ -844,8 +834,7 @@ storagePoolBuild(virStoragePoolPtr obj, ret = 0; cleanup: - if (pool) - virStoragePoolObjUnlock(pool); + virStoragePoolObjUnlock(pool); return ret; } @@ -859,11 +848,12 @@ storagePoolDestroy(virStoragePoolPtr obj) int ret = -1; storageDriverLock(driver); - pool = virStoragePoolObjFindByUUID(&driver->pools, obj->uuid); - - if (!pool) { + if (!(pool = virStoragePoolObjFindByUUID(&driver->pools, obj->uuid))) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(obj->uuid, uuidstr); virReportError(VIR_ERR_NO_STORAGE_POOL, - _("no storage pool with matching uuid %s"), obj->uuid); + _("no storage pool with matching uuid '%s' (%s)"), + uuidstr, obj->name); goto cleanup; } @@ -916,20 +906,12 @@ static int storagePoolDelete(virStoragePoolPtr obj, unsigned int flags) { - virStorageDriverStatePtr driver = obj->conn->storagePrivateData; virStoragePoolObjPtr pool; virStorageBackendPtr backend; int ret = -1; - storageDriverLock(driver); - pool = virStoragePoolObjFindByUUID(&driver->pools, obj->uuid); - storageDriverUnlock(driver); - - if (!pool) { - virReportError(VIR_ERR_NO_STORAGE_POOL, - _("no storage pool with matching uuid %s"), obj->uuid); - goto cleanup; - } + if (!(pool = virStoragePoolObjFromStoragePool(obj))) + return -1; if (virStoragePoolDeleteEnsureACL(obj->conn, pool->def) < 0) goto cleanup; @@ -962,8 +944,7 @@ storagePoolDelete(virStoragePoolPtr obj, ret = 0; cleanup: - if (pool) - virStoragePoolObjUnlock(pool); + virStoragePoolObjUnlock(pool); return ret; } @@ -980,11 +961,12 @@ storagePoolRefresh(virStoragePoolPtr obj, virCheckFlags(0, -1); storageDriverLock(driver); - pool = virStoragePoolObjFindByUUID(&driver->pools, obj->uuid); - - if (!pool) { + if (!(pool = virStoragePoolObjFindByUUID(&driver->pools, obj->uuid))) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(obj->uuid, uuidstr); virReportError(VIR_ERR_NO_STORAGE_POOL, - _("no storage pool with matching uuid %s"), obj->uuid); + _("no storage pool with matching uuid '%s' (%s)"), + uuidstr, obj->name); goto cleanup; } @@ -1034,19 +1016,11 @@ static int storagePoolGetInfo(virStoragePoolPtr obj, virStoragePoolInfoPtr info) { - virStorageDriverStatePtr driver = obj->conn->storagePrivateData; virStoragePoolObjPtr pool; int ret = -1; - storageDriverLock(driver); - pool = virStoragePoolObjFindByUUID(&driver->pools, obj->uuid); - storageDriverUnlock(driver); - - if (!pool) { - virReportError(VIR_ERR_NO_STORAGE_POOL, - _("no storage pool with matching uuid %s"), obj->uuid); - goto cleanup; - } + if (!(pool = virStoragePoolObjFromStoragePool(obj))) + return -1; if (virStoragePoolGetInfoEnsureACL(obj->conn, pool->def) < 0) goto cleanup; @@ -1065,8 +1039,7 @@ storagePoolGetInfo(virStoragePoolPtr obj, ret = 0; cleanup: - if (pool) - virStoragePoolObjUnlock(pool); + virStoragePoolObjUnlock(pool); return ret; } @@ -1074,22 +1047,14 @@ static char * storagePoolGetXMLDesc(virStoragePoolPtr obj, unsigned int flags) { - virStorageDriverStatePtr driver = obj->conn->storagePrivateData; virStoragePoolObjPtr pool; virStoragePoolDefPtr def; char *ret = NULL; virCheckFlags(VIR_STORAGE_XML_INACTIVE, NULL); - storageDriverLock(driver); - pool = virStoragePoolObjFindByUUID(&driver->pools, obj->uuid); - storageDriverUnlock(driver); - - if (!pool) { - virReportError(VIR_ERR_NO_STORAGE_POOL, - _("no storage pool with matching uuid %s"), obj->uuid); - goto cleanup; - } + if (!(pool = virStoragePoolObjFromStoragePool(obj))) + return NULL; if (virStoragePoolGetXMLDescEnsureACL(obj->conn, pool->def) < 0) goto cleanup; @@ -1102,8 +1067,7 @@ storagePoolGetXMLDesc(virStoragePoolPtr obj, ret = virStoragePoolDefFormat(def); cleanup: - if (pool) - virStoragePoolObjUnlock(pool); + virStoragePoolObjUnlock(pool); return ret; } @@ -1111,19 +1075,11 @@ static int storagePoolGetAutostart(virStoragePoolPtr obj, int *autostart) { - virStorageDriverStatePtr driver = obj->conn->storagePrivateData; virStoragePoolObjPtr pool; int ret = -1; - storageDriverLock(driver); - pool = virStoragePoolObjFindByUUID(&driver->pools, obj->uuid); - storageDriverUnlock(driver); - - if (!pool) { - virReportError(VIR_ERR_NO_STORAGE_POOL, - _("no storage pool with matching uuid %s"), obj->uuid); - goto cleanup; - } + if (!(pool = virStoragePoolObjFromStoragePool(obj))) + return -1; if (virStoragePoolGetAutostartEnsureACL(obj->conn, pool->def) < 0) goto cleanup; @@ -1136,8 +1092,7 @@ storagePoolGetAutostart(virStoragePoolPtr obj, ret = 0; cleanup: - if (pool) - virStoragePoolObjUnlock(pool); + virStoragePoolObjUnlock(pool); return ret; } @@ -1153,8 +1108,11 @@ storagePoolSetAutostart(virStoragePoolPtr obj, pool = virStoragePoolObjFindByUUID(&driver->pools, obj->uuid); if (!pool) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(obj->uuid, uuidstr); virReportError(VIR_ERR_NO_STORAGE_POOL, - _("no storage pool with matching uuid %s"), obj->uuid); + _("no storage pool with matching uuid '%s' (%s)"), + uuidstr, obj->name); goto cleanup; } @@ -1208,20 +1166,12 @@ storagePoolSetAutostart(virStoragePoolPtr obj, static int storagePoolNumOfVolumes(virStoragePoolPtr obj) { - virStorageDriverStatePtr driver = obj->conn->storagePrivateData; virStoragePoolObjPtr pool; int ret = -1; size_t i; - storageDriverLock(driver); - pool = virStoragePoolObjFindByUUID(&driver->pools, obj->uuid); - storageDriverUnlock(driver); - - if (!pool) { - virReportError(VIR_ERR_NO_STORAGE_POOL, - _("no storage pool with matching uuid %s"), obj->uuid); - goto cleanup; - } + if (!(pool = virStoragePoolObjFromStoragePool(obj))) + return -1; if (virStoragePoolNumOfVolumesEnsureACL(obj->conn, pool->def) < 0) goto cleanup; @@ -1239,8 +1189,7 @@ storagePoolNumOfVolumes(virStoragePoolPtr obj) } cleanup: - if (pool) - virStoragePoolObjUnlock(pool); + virStoragePoolObjUnlock(pool); return ret; } @@ -1249,22 +1198,14 @@ storagePoolListVolumes(virStoragePoolPtr obj, char **const names, int maxnames) { - virStorageDriverStatePtr driver = obj->conn->storagePrivateData; virStoragePoolObjPtr pool; size_t i; int n = 0; memset(names, 0, maxnames * sizeof(*names)); - storageDriverLock(driver); - pool = virStoragePoolObjFindByUUID(&driver->pools, obj->uuid); - storageDriverUnlock(driver); - - if (!pool) { - virReportError(VIR_ERR_NO_STORAGE_POOL, - _("no storage pool with matching uuid %s"), obj->uuid); - goto cleanup; - } + if (!(pool = virStoragePoolObjFromStoragePool(obj))) + return -1; if (virStoragePoolListVolumesEnsureACL(obj->conn, pool->def) < 0) goto cleanup; @@ -1287,8 +1228,7 @@ storagePoolListVolumes(virStoragePoolPtr obj, return n; cleanup: - if (pool) - virStoragePoolObjUnlock(pool); + virStoragePoolObjUnlock(pool); for (n = 0; n < maxnames; n++) VIR_FREE(names[n]); @@ -1301,7 +1241,6 @@ storagePoolListAllVolumes(virStoragePoolPtr pool, virStorageVolPtr **vols, unsigned int flags) { - virStorageDriverStatePtr driver = pool->conn->storagePrivateData; virStoragePoolObjPtr obj; size_t i; virStorageVolPtr *tmp_vols = NULL; @@ -1311,16 +1250,8 @@ storagePoolListAllVolumes(virStoragePoolPtr pool, virCheckFlags(0, -1); - storageDriverLock(driver); - obj = virStoragePoolObjFindByUUID(&driver->pools, pool->uuid); - storageDriverUnlock(driver); - - if (!obj) { - virReportError(VIR_ERR_NO_STORAGE_POOL, - _("no storage pool with matching uuid %s"), - pool->uuid); - goto cleanup; - } + if (!(obj = virStoragePoolObjFromStoragePool(pool))) + return -1; if (virStoragePoolListAllVolumesEnsureACL(pool->conn, obj->def) < 0) goto cleanup; @@ -1365,8 +1296,7 @@ storagePoolListAllVolumes(virStoragePoolPtr pool, VIR_FREE(tmp_vols); } - if (obj) - virStoragePoolObjUnlock(obj); + virStoragePoolObjUnlock(obj); return ret; } @@ -1375,20 +1305,12 @@ static virStorageVolPtr storageVolLookupByName(virStoragePoolPtr obj, const char *name) { - virStorageDriverStatePtr driver = obj->conn->storagePrivateData; virStoragePoolObjPtr pool; virStorageVolDefPtr vol; virStorageVolPtr ret = NULL; - storageDriverLock(driver); - pool = virStoragePoolObjFindByUUID(&driver->pools, obj->uuid); - storageDriverUnlock(driver); - - if (!pool) { - virReportError(VIR_ERR_NO_STORAGE_POOL, - _("no storage pool with matching uuid %s"), obj->uuid); - goto cleanup; - } + if (!(pool = virStoragePoolObjFromStoragePool(obj))) + return NULL; if (!virStoragePoolObjIsActive(pool)) { virReportError(VIR_ERR_OPERATION_INVALID, @@ -1412,8 +1334,7 @@ storageVolLookupByName(virStoragePoolPtr obj, NULL, NULL); cleanup: - if (pool) - virStoragePoolObjUnlock(pool); + virStoragePoolObjUnlock(pool); return ret; } @@ -1681,15 +1602,8 @@ storageVolCreateXML(virStoragePoolPtr obj, virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, NULL); - storageDriverLock(driver); - pool = virStoragePoolObjFindByUUID(&driver->pools, obj->uuid); - storageDriverUnlock(driver); - - if (!pool) { - virReportError(VIR_ERR_NO_STORAGE_POOL, - _("no storage pool with matching uuid %s"), obj->uuid); - goto cleanup; - } + if (!(pool = virStoragePoolObjFromStoragePool(obj))) + return NULL; if (!virStoragePoolObjIsActive(pool)) { virReportError(VIR_ERR_OPERATION_INVALID, @@ -1821,8 +1735,11 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj, } storageDriverUnlock(driver); if (!pool) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(obj->uuid, uuidstr); virReportError(VIR_ERR_NO_STORAGE_POOL, - _("no storage pool with matching uuid %s"), obj->uuid); + _("no storage pool with matching uuid '%s' (%s)"), + uuidstr, obj->name); goto cleanup; } -- 1.9.3
On 06/05/2014 01:52 PM, Peter Krempa wrote:
Rework internal pool lookup code to avoid printing the raw UUID buffer in the case a storage pool can't be found:
$ virsh pool-name e012ace0-0460-5810-39ef-1bce5fa5a4dd error: failed to get pool 'e012ace0-0460-5810-39ef-1bce5fa5a4dd' error: Storage pool not found: no storage pool with matching uuid à¬à`X9ï_¥¤Ý
The rework is mostly done by switching the lookup code to the newly introduced helper virStoragePoolObjFromStoragePoo
*Pool
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1104993 --- src/storage/storage_driver.c | 263 +++++++++++++++---------------------------- 1 file changed, 90 insertions(+), 173 deletions(-)
ACK
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 4f51517..c9916ff 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -287,8 +289,7 @@ storagePoolLookupByUUID(virConnectPtr conn, NULL, NULL);
cleanup: - if (pool) - virStoragePoolObjUnlock(pool); + virStoragePoolObjUnlock(pool);
But IMO it would look neater with these no-op changes separated. Jan
On 06/11/14 17:12, Ján Tomko wrote:
On 06/05/2014 01:52 PM, Peter Krempa wrote:
Rework internal pool lookup code to avoid printing the raw UUID buffer in the case a storage pool can't be found:
$ virsh pool-name e012ace0-0460-5810-39ef-1bce5fa5a4dd error: failed to get pool 'e012ace0-0460-5810-39ef-1bce5fa5a4dd' error: Storage pool not found: no storage pool with matching uuid à¬à`X9ï_¥¤Ý
The rework is mostly done by switching the lookup code to the newly introduced helper virStoragePoolObjFromStoragePoo
*Pool
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1104993 --- src/storage/storage_driver.c | 263 +++++++++++++++---------------------------- 1 file changed, 90 insertions(+), 173 deletions(-)
ACK
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 4f51517..c9916ff 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -287,8 +289,7 @@ storagePoolLookupByUUID(virConnectPtr conn, NULL, NULL);
cleanup: - if (pool) - virStoragePoolObjUnlock(pool); + virStoragePoolObjUnlock(pool);
But IMO it would look neater with these no-op changes separated.
I've separated these cleanups into a separate patch and pushed both of those.
Jan
Peter
Add a helper to do all the lookup steps and remove a ton of duplicated code. --- src/storage/storage_driver.c | 292 ++++++++++--------------------------------- 1 file changed, 69 insertions(+), 223 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index c9916ff..26b2601 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1520,45 +1520,75 @@ storageVolDeleteInternal(virStorageVolPtr obj, } -static int -storageVolDelete(virStorageVolPtr obj, - unsigned int flags) +static virStorageVolDefPtr +virStorageVolDefFromVol(virStorageVolPtr obj, + virStoragePoolObjPtr *pool, + virStorageBackendPtr *backend) { virStorageDriverStatePtr driver = obj->conn->storagePrivateData; - virStoragePoolObjPtr pool; - virStorageBackendPtr backend; virStorageVolDefPtr vol = NULL; - int ret = -1; + + *pool = NULL; + if (backend) + *backend = NULL; storageDriverLock(driver); - pool = virStoragePoolObjFindByName(&driver->pools, obj->pool); + *pool = virStoragePoolObjFindByName(&driver->pools, obj->pool); storageDriverUnlock(driver); - if (!pool) { + if (!*pool) { virReportError(VIR_ERR_NO_STORAGE_POOL, _("no storage pool with matching name '%s'"), obj->pool); - goto cleanup; + return NULL; } - if (!virStoragePoolObjIsActive(pool)) { + if (!virStoragePoolObjIsActive(*pool)) { virReportError(VIR_ERR_OPERATION_INVALID, - _("storage pool '%s' is not active"), pool->def->name); - goto cleanup; + _("storage pool '%s' is not active"), + (*pool)->def->name); + goto error; } - if ((backend = virStorageBackendForType(pool->def->type)) == NULL) - goto cleanup; - - vol = virStorageVolDefFindByName(pool, obj->name); + if (backend) { + if (!(*backend = virStorageBackendForType((*pool)->def->type))) + goto error; + } - if (!vol) { + if (!(vol = virStorageVolDefFindByName(*pool, obj->name))) { virReportError(VIR_ERR_NO_STORAGE_VOL, _("no storage vol with matching name '%s'"), obj->name); - goto cleanup; + goto error; } + return vol; + + error: + if (*pool) { + virStoragePoolObjUnlock(*pool); + *pool = NULL; + } + + if (backend) + *backend = NULL; + + return NULL; +} + + +static int +storageVolDelete(virStorageVolPtr obj, + unsigned int flags) +{ + virStoragePoolObjPtr pool; + virStorageBackendPtr backend; + virStorageVolDefPtr vol = NULL; + int ret = -1; + + if (!(vol = virStorageVolDefFromVol(obj, &pool, &backend))) + goto cleanup; + if (virStorageVolDeleteEnsureACL(obj->conn, pool->def, vol) < 0) goto cleanup; @@ -1898,38 +1928,14 @@ storageVolDownload(virStorageVolPtr obj, unsigned long long length, unsigned int flags) { - virStorageDriverStatePtr driver = obj->conn->storagePrivateData; virStoragePoolObjPtr pool = NULL; virStorageVolDefPtr vol = NULL; int ret = -1; virCheckFlags(0, -1); - storageDriverLock(driver); - pool = virStoragePoolObjFindByName(&driver->pools, obj->pool); - storageDriverUnlock(driver); - - if (!pool) { - virReportError(VIR_ERR_NO_STORAGE_POOL, - _("no storage pool with matching name '%s'"), - obj->pool); - goto cleanup; - } - - if (!virStoragePoolObjIsActive(pool)) { - virReportError(VIR_ERR_OPERATION_INVALID, - _("storage pool '%s' is not active"), pool->def->name); - goto cleanup; - } - - vol = virStorageVolDefFindByName(pool, obj->name); - - if (vol == NULL) { - virReportError(VIR_ERR_NO_STORAGE_VOL, - _("no storage vol with matching name '%s'"), - obj->name); - goto cleanup; - } + if (!(vol = virStorageVolDefFromVol(obj, &pool, NULL))) + return -1; if (virStorageVolDownloadEnsureACL(obj->conn, pool->def, vol) < 0) goto cleanup; @@ -1950,8 +1956,7 @@ storageVolDownload(virStorageVolPtr obj, ret = 0; cleanup: - if (pool) - virStoragePoolObjUnlock(pool); + virStoragePoolObjUnlock(pool); return ret; } @@ -1964,38 +1969,14 @@ storageVolUpload(virStorageVolPtr obj, unsigned long long length, unsigned int flags) { - virStorageDriverStatePtr driver = obj->conn->storagePrivateData; virStoragePoolObjPtr pool = NULL; virStorageVolDefPtr vol = NULL; int ret = -1; virCheckFlags(0, -1); - storageDriverLock(driver); - pool = virStoragePoolObjFindByName(&driver->pools, obj->pool); - storageDriverUnlock(driver); - - if (!pool) { - virReportError(VIR_ERR_NO_STORAGE_POOL, - _("no storage pool with matching name '%s'"), - obj->pool); - goto cleanup; - } - - if (!virStoragePoolObjIsActive(pool)) { - virReportError(VIR_ERR_OPERATION_INVALID, - _("storage pool '%s' is not active"), pool->def->name); - goto cleanup; - } - - vol = virStorageVolDefFindByName(pool, obj->name); - - if (vol == NULL) { - virReportError(VIR_ERR_NO_STORAGE_VOL, - _("no storage vol with matching name '%s'"), - obj->name); - goto cleanup; - } + if (!(vol = virStorageVolDefFromVol(obj, &pool, NULL))) + return -1; if (virStorageVolUploadEnsureACL(obj->conn, pool->def, vol) < 0) goto cleanup; @@ -2044,8 +2025,7 @@ storageVolUpload(virStorageVolPtr obj, ret = 0; cleanup: - if (pool) - virStoragePoolObjUnlock(pool); + virStoragePoolObjUnlock(pool); return ret; } @@ -2055,7 +2035,6 @@ storageVolResize(virStorageVolPtr obj, unsigned long long capacity, unsigned int flags) { - virStorageDriverStatePtr driver = obj->conn->storagePrivateData; virStorageBackendPtr backend; virStoragePoolObjPtr pool = NULL; virStorageVolDefPtr vol = NULL; @@ -2066,34 +2045,8 @@ storageVolResize(virStorageVolPtr obj, VIR_STORAGE_VOL_RESIZE_DELTA | VIR_STORAGE_VOL_RESIZE_SHRINK, -1); - storageDriverLock(driver); - pool = virStoragePoolObjFindByName(&driver->pools, obj->pool); - storageDriverUnlock(driver); - - if (!pool) { - virReportError(VIR_ERR_NO_STORAGE_POOL, - _("no storage pool with matching name '%s'"), - obj->pool); - goto cleanup; - } - - if (!virStoragePoolObjIsActive(pool)) { - virReportError(VIR_ERR_OPERATION_INVALID, - _("storage pool '%s' is not active"), pool->def->name); - goto cleanup; - } - - if ((backend = virStorageBackendForType(pool->def->type)) == NULL) - goto cleanup; - - vol = virStorageVolDefFindByName(pool, obj->name); - - if (vol == NULL) { - virReportError(VIR_ERR_NO_STORAGE_VOL, - _("no storage vol with matching name '%s'"), - obj->name); - goto cleanup; - } + if (!(vol = virStorageVolDefFromVol(obj, &pool, &backend))) + return -1; if (virStorageVolResizeEnsureACL(obj->conn, pool->def, vol) < 0) goto cleanup; @@ -2161,8 +2114,7 @@ storageVolResize(virStorageVolPtr obj, ret = 0; cleanup: - if (pool) - virStoragePoolObjUnlock(pool); + virStoragePoolObjUnlock(pool); return ret; } @@ -2369,7 +2321,6 @@ storageVolWipePattern(virStorageVolPtr obj, unsigned int algorithm, unsigned int flags) { - virStorageDriverStatePtr driver = obj->conn->storagePrivateData; virStoragePoolObjPtr pool = NULL; virStorageVolDefPtr vol = NULL; int ret = -1; @@ -2383,31 +2334,9 @@ storageVolWipePattern(virStorageVolPtr obj, return -1; } - storageDriverLock(driver); - pool = virStoragePoolObjFindByName(&driver->pools, obj->pool); - storageDriverUnlock(driver); - - if (!pool) { - virReportError(VIR_ERR_NO_STORAGE_POOL, - _("no storage pool with matching name '%s'"), - obj->pool); - goto cleanup; - } - - if (!virStoragePoolObjIsActive(pool)) { - virReportError(VIR_ERR_OPERATION_INVALID, - _("storage pool '%s' is not active"), pool->def->name); - goto cleanup; - } + if (!(vol = virStorageVolDefFromVol(obj, &pool, NULL))) + return -1; - vol = virStorageVolDefFindByName(pool, obj->name); - - if (vol == NULL) { - virReportError(VIR_ERR_NO_STORAGE_VOL, - _("no storage vol with matching name '%s'"), - obj->name); - goto cleanup; - } if (virStorageVolWipePatternEnsureACL(obj->conn, pool->def, vol) < 0) goto cleanup; @@ -2433,12 +2362,9 @@ storageVolWipePattern(virStorageVolPtr obj, ret = 0; cleanup: - if (pool) { - virStoragePoolObjUnlock(pool); - } + virStoragePoolObjUnlock(pool); return ret; - } static int @@ -2453,44 +2379,17 @@ static int storageVolGetInfo(virStorageVolPtr obj, virStorageVolInfoPtr info) { - virStorageDriverStatePtr driver = obj->conn->storagePrivateData; virStoragePoolObjPtr pool; virStorageBackendPtr backend; virStorageVolDefPtr vol; int ret = -1; - storageDriverLock(driver); - pool = virStoragePoolObjFindByName(&driver->pools, obj->pool); - storageDriverUnlock(driver); - - if (!pool) { - virReportError(VIR_ERR_NO_STORAGE_POOL, - _("no storage pool with matching name '%s'"), - obj->pool); - goto cleanup; - } - - if (!virStoragePoolObjIsActive(pool)) { - virReportError(VIR_ERR_OPERATION_INVALID, - _("storage pool '%s' is not active"), pool->def->name); - goto cleanup; - } - - vol = virStorageVolDefFindByName(pool, obj->name); - - if (!vol) { - virReportError(VIR_ERR_NO_STORAGE_VOL, - _("no storage vol with matching name '%s'"), - obj->name); - goto cleanup; - } + if (!(vol = virStorageVolDefFromVol(obj, &pool, &backend))) + return -1; if (virStorageVolGetInfoEnsureACL(obj->conn, pool->def, vol) < 0) goto cleanup; - if ((backend = virStorageBackendForType(pool->def->type)) == NULL) - goto cleanup; - if (backend->refreshVol && backend->refreshVol(obj->conn, pool, vol) < 0) goto cleanup; @@ -2502,8 +2401,7 @@ storageVolGetInfo(virStorageVolPtr obj, ret = 0; cleanup: - if (pool) - virStoragePoolObjUnlock(pool); + virStoragePoolObjUnlock(pool); return ret; } @@ -2511,7 +2409,6 @@ static char * storageVolGetXMLDesc(virStorageVolPtr obj, unsigned int flags) { - virStorageDriverStatePtr driver = obj->conn->storagePrivateData; virStoragePoolObjPtr pool; virStorageBackendPtr backend; virStorageVolDefPtr vol; @@ -2519,38 +2416,12 @@ storageVolGetXMLDesc(virStorageVolPtr obj, virCheckFlags(0, NULL); - storageDriverLock(driver); - pool = virStoragePoolObjFindByName(&driver->pools, obj->pool); - storageDriverUnlock(driver); - - if (!pool) { - virReportError(VIR_ERR_NO_STORAGE_POOL, - _("no storage pool with matching name '%s'"), - obj->pool); - goto cleanup; - } - - if (!virStoragePoolObjIsActive(pool)) { - virReportError(VIR_ERR_OPERATION_INVALID, - _("storage pool '%s' is not active"), pool->def->name); - goto cleanup; - } - - vol = virStorageVolDefFindByName(pool, obj->name); - - if (!vol) { - virReportError(VIR_ERR_NO_STORAGE_VOL, - _("no storage vol with matching name '%s'"), - obj->name); - goto cleanup; - } + if (!(vol = virStorageVolDefFromVol(obj, &pool, &backend))) + return NULL; if (virStorageVolGetXMLDescEnsureACL(obj->conn, pool->def, vol) < 0) goto cleanup; - if ((backend = virStorageBackendForType(pool->def->type)) == NULL) - goto cleanup; - if (backend->refreshVol && backend->refreshVol(obj->conn, pool, vol) < 0) goto cleanup; @@ -2558,8 +2429,7 @@ storageVolGetXMLDesc(virStorageVolPtr obj, ret = virStorageVolDefFormat(pool->def, vol); cleanup: - if (pool) - virStoragePoolObjUnlock(pool); + virStoragePoolObjUnlock(pool); return ret; } @@ -2567,35 +2437,12 @@ storageVolGetXMLDesc(virStorageVolPtr obj, static char * storageVolGetPath(virStorageVolPtr obj) { - virStorageDriverStatePtr driver = obj->conn->storagePrivateData; virStoragePoolObjPtr pool; virStorageVolDefPtr vol; char *ret = NULL; - storageDriverLock(driver); - pool = virStoragePoolObjFindByName(&driver->pools, obj->pool); - storageDriverUnlock(driver); - if (!pool) { - virReportError(VIR_ERR_NO_STORAGE_POOL, - _("no storage pool with matching name '%s'"), - obj->pool); - goto cleanup; - } - - if (!virStoragePoolObjIsActive(pool)) { - virReportError(VIR_ERR_OPERATION_INVALID, - _("storage pool '%s' is not active"), pool->def->name); - goto cleanup; - } - - vol = virStorageVolDefFindByName(pool, obj->name); - - if (!vol) { - virReportError(VIR_ERR_NO_STORAGE_VOL, - _("no storage vol with matching name '%s'"), - obj->name); - goto cleanup; - } + if (!(vol = virStorageVolDefFromVol(obj, &pool, NULL))) + return NULL; if (virStorageVolGetPathEnsureACL(obj->conn, pool->def, vol) < 0) goto cleanup; @@ -2603,8 +2450,7 @@ storageVolGetPath(virStorageVolPtr obj) ignore_value(VIR_STRDUP(ret, vol->target.path)); cleanup: - if (pool) - virStoragePoolObjUnlock(pool); + virStoragePoolObjUnlock(pool); return ret; } -- 1.9.3
On 06/05/2014 01:52 PM, Peter Krempa wrote:
Add a helper to do all the lookup steps and remove a ton of duplicated code. --- src/storage/storage_driver.c | 292 ++++++++++--------------------------------- 1 file changed, 69 insertions(+), 223 deletions(-)
ACK
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index c9916ff..26b2601 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1520,45 +1520,75 @@ storageVolDeleteInternal(virStorageVolPtr obj, }
-static int -storageVolDelete(virStorageVolPtr obj, - unsigned int flags) +static virStorageVolDefPtr +virStorageVolDefFromVol(virStorageVolPtr obj, + virStoragePoolObjPtr *pool, + virStorageBackendPtr *backend) { virStorageDriverStatePtr driver = obj->conn->storagePrivateData; - virStoragePoolObjPtr pool; - virStorageBackendPtr backend; virStorageVolDefPtr vol = NULL; - int ret = -1;
+ + *pool = NULL; + if (backend) + *backend = NULL;
Initializing *pool here might be useful if someone decides to rearrange the code again, since it's used in the cleanup section, but I don't think we need to initialize backend - the caller should only use the values if this function returns non-NULL.
storageDriverLock(driver); - pool = virStoragePoolObjFindByName(&driver->pools, obj->pool); + *pool = virStoragePoolObjFindByName(&driver->pools, obj->pool); storageDriverUnlock(driver);
- if (!pool) { + if (!*pool) { virReportError(VIR_ERR_NO_STORAGE_POOL, _("no storage pool with matching name '%s'"), obj->pool); - goto cleanup; + return NULL; }
+ + +static int +storageVolDelete(virStorageVolPtr obj, + unsigned int flags) +{ + virStoragePoolObjPtr pool; + virStorageBackendPtr backend; + virStorageVolDefPtr vol = NULL; + int ret = -1; + + if (!(vol = virStorageVolDefFromVol(obj, &pool, &backend))) + goto cleanup;
You can return -1 here.
+ if (virStorageVolDeleteEnsureACL(obj->conn, pool->def, vol) < 0) goto cleanup;
Jan
On 06/11/14 17:13, Ján Tomko wrote:
On 06/05/2014 01:52 PM, Peter Krempa wrote:
Add a helper to do all the lookup steps and remove a ton of duplicated code. --- src/storage/storage_driver.c | 292 ++++++++++--------------------------------- 1 file changed, 69 insertions(+), 223 deletions(-)
ACK
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index c9916ff..26b2601 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1520,45 +1520,75 @@ storageVolDeleteInternal(virStorageVolPtr obj, }
-static int -storageVolDelete(virStorageVolPtr obj, - unsigned int flags) +static virStorageVolDefPtr +virStorageVolDefFromVol(virStorageVolPtr obj, + virStoragePoolObjPtr *pool, + virStorageBackendPtr *backend) { virStorageDriverStatePtr driver = obj->conn->storagePrivateData; - virStoragePoolObjPtr pool; - virStorageBackendPtr backend; virStorageVolDefPtr vol = NULL; - int ret = -1;
+ + *pool = NULL; + if (backend) + *backend = NULL;
Initializing *pool here might be useful if someone decides to rearrange the code again, since it's used in the cleanup section, but I don't think we need to initialize backend - the caller should only use the values if this function returns non-NULL.
You are right, I've removed it. Also by rearranging the code I was able to remove touching of "backend" in the error: section.
storageDriverLock(driver); - pool = virStoragePoolObjFindByName(&driver->pools, obj->pool); + *pool = virStoragePoolObjFindByName(&driver->pools, obj->pool); storageDriverUnlock(driver);
- if (!pool) { + if (!*pool) { virReportError(VIR_ERR_NO_STORAGE_POOL, _("no storage pool with matching name '%s'"), obj->pool); - goto cleanup; + return NULL; }
+ + +static int +storageVolDelete(virStorageVolPtr obj, + unsigned int flags) +{ + virStoragePoolObjPtr pool; + virStorageBackendPtr backend; + virStorageVolDefPtr vol = NULL; + int ret = -1; + + if (!(vol = virStorageVolDefFromVol(obj, &pool, &backend))) + goto cleanup;
You can return -1 here.
Thanks; I've done so.
+ if (virStorageVolDeleteEnsureACL(obj->conn, pool->def, vol) < 0) goto cleanup;
Jan
And pushed this one too. Thanks for reviewing. Peter
participants (2)
-
Ján Tomko -
Peter Krempa