6:52 a.m.
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
6:52 a.m.
New subject: [libvirt] [PATCH 1/2] storage: pool: Fix handling of errors on pool lookup failure
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
10:12 a.m.
New subject: [libvirt] [PATCH 1/2] storage: pool: Fix handling of errors on pool lookup failure
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
3:29 a.m.
New subject: [libvirt] [PATCH 1/2] storage: pool: Fix handling of errors on pool lookup failure
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
6:52 a.m.
New subject: [libvirt] [PATCH 2/2] storage: volume: Rework lookup of volume objects
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
10:13 a.m.
New subject: [libvirt] [PATCH 2/2] storage: volume: Rework lookup of volume objects
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
3:30 a.m.
New subject: [libvirt] [PATCH 2/2] storage: volume: Rework lookup of volume objects
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
3816
days inactive
3823
days old
6 comments
2 participants
participants (2)
-
Ján Tomko -
Peter Krempa