[libvirt] [PATCH v2 0/3] Storage pool common object fixes
v1: https://www.redhat.com/archives/libvir-list/2017-December/msg00543.html Changes since v1... * Added a patch to handle a NULL return with pool obj lock * Alter the IsDuplicate API to use a bool parameter * Use the IsDuplicate API from the test driver. This would have generated the correct error message about a duplicate UUID instead of the Duplicate key that was generated. Ran virt-manager tests prior to Cole's fixes and of course after. John Ferlan (3): conf: Need to unlock pools on object allocation failure conf: Use bool for @check_active parameter test: Use virStoragePoolObjIsDuplicate for storage define/create src/conf/virstorageobj.c | 4 ++-- src/conf/virstorageobj.h | 2 +- src/storage/storage_driver.c | 4 ++-- src/test/test_driver.c | 11 ++++------- 4 files changed, 9 insertions(+), 12 deletions(-) -- 2.13.6
The RW pool could be left unlocked if allocation fails. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virstorageobj.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index 49fe24b28..19903b6c5 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -748,7 +748,7 @@ virStoragePoolObjAssignDef(virStoragePoolObjListPtr pools, } if (!(obj = virStoragePoolObjNew())) - return NULL; + goto error; virUUIDFormat(def->uuid, uuidstr); if (virHashAddEntry(pools->objs, uuidstr, obj) < 0) -- 2.13.6
Use a bool as that's how the variable is used in the function. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virstorageobj.c | 2 +- src/conf/virstorageobj.h | 2 +- src/storage/storage_driver.c | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index 19903b6c5..1eaa53423 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -1133,7 +1133,7 @@ virStoragePoolObjGetNames(virStoragePoolObjListPtr pools, int virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools, virStoragePoolDefPtr def, - unsigned int check_active) + bool check_active) { int ret = -1; virStoragePoolObjPtr obj = NULL; diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h index 4f372386a..dd7001c4b 100644 --- a/src/conf/virstorageobj.h +++ b/src/conf/virstorageobj.h @@ -247,7 +247,7 @@ virStoragePoolObjRemove(virStoragePoolObjListPtr pools, int virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools, virStoragePoolDefPtr def, - unsigned int check_active); + bool check_active); int virStoragePoolObjSourceFindDuplicate(virConnectPtr conn, diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index f86087fb0..f590f6b9b 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -656,7 +656,7 @@ storagePoolCreateXML(virConnectPtr conn, if (virStoragePoolCreateXMLEnsureACL(conn, newDef) < 0) goto cleanup; - if (virStoragePoolObjIsDuplicate(driver->pools, newDef, 1) < 0) + if (virStoragePoolObjIsDuplicate(driver->pools, newDef, true) < 0) goto cleanup; if (virStoragePoolObjSourceFindDuplicate(conn, driver->pools, newDef) < 0) @@ -751,7 +751,7 @@ storagePoolDefineXML(virConnectPtr conn, if (virStoragePoolDefineXMLEnsureACL(conn, newDef) < 0) goto cleanup; - if (virStoragePoolObjIsDuplicate(driver->pools, newDef, 0) < 0) + if (virStoragePoolObjIsDuplicate(driver->pools, newDef, false) < 0) goto cleanup; if (virStoragePoolObjSourceFindDuplicate(conn, driver->pools, newDef) < 0) -- 2.13.6
Avoid the chance that there could be a duplicate storage pool UUID or Name from the test driver storage pool define/create functions. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/test/test_driver.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 8adc2167b..dc743b498 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -4440,14 +4440,8 @@ testStoragePoolCreateXML(virConnectPtr conn, if (!(newDef = virStoragePoolDefParseString(xml))) goto cleanup; - obj = virStoragePoolObjFindByUUID(privconn->pools, newDef->uuid); - if (!obj) - obj = virStoragePoolObjFindByName(privconn->pools, newDef->name); - if (obj) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("storage pool already exists")); + if (virStoragePoolObjIsDuplicate(privconn->pools, newDef, true) < 0) goto cleanup; - } if (!(obj = virStoragePoolObjAssignDef(privconn->pools, newDef))) goto cleanup; @@ -4520,6 +4514,9 @@ testStoragePoolDefineXML(virConnectPtr conn, newDef->allocation = defaultPoolAlloc; newDef->available = defaultPoolCap - defaultPoolAlloc; + if (virStoragePoolObjIsDuplicate(privconn->pools, newDef, false) < 0) + goto cleanup; + if (!(obj = virStoragePoolObjAssignDef(privconn->pools, newDef))) goto cleanup; newDef = NULL; -- 2.13.6
ping? Tks - John On 12/18/2017 07:56 AM, John Ferlan wrote:
v1: https://www.redhat.com/archives/libvir-list/2017-December/msg00543.html
Changes since v1...
* Added a patch to handle a NULL return with pool obj lock
* Alter the IsDuplicate API to use a bool parameter
* Use the IsDuplicate API from the test driver. This would have generated the correct error message about a duplicate UUID instead of the Duplicate key that was generated. Ran virt-manager tests prior to Cole's fixes and of course after.
John Ferlan (3): conf: Need to unlock pools on object allocation failure conf: Use bool for @check_active parameter test: Use virStoragePoolObjIsDuplicate for storage define/create
src/conf/virstorageobj.c | 4 ++-- src/conf/virstorageobj.h | 2 +- src/storage/storage_driver.c | 4 ++-- src/test/test_driver.c | 11 ++++------- 4 files changed, 9 insertions(+), 12 deletions(-)
On Mon, Dec 18, 2017 at 07:56:51AM -0500, John Ferlan wrote:
v1: https://www.redhat.com/archives/libvir-list/2017-December/msg00543.html
Changes since v1...
* Added a patch to handle a NULL return with pool obj lock
* Alter the IsDuplicate API to use a bool parameter
* Use the IsDuplicate API from the test driver. This would have generated the correct error message about a duplicate UUID instead of the Duplicate key that was generated. Ran virt-manager tests prior to Cole's fixes and of course after.
John Ferlan (3): conf: Need to unlock pools on object allocation failure conf: Use bool for @check_active parameter test: Use virStoragePoolObjIsDuplicate for storage define/create
src/conf/virstorageobj.c | 4 ++-- src/conf/virstorageobj.h | 2 +- src/storage/storage_driver.c | 4 ++-- src/test/test_driver.c | 11 ++++------- 4 files changed, 9 insertions(+), 12 deletions(-)
Reviewed-by: Erik Skultety <eskultet@redhat.com>
participants (2)
-
Erik Skultety -
John Ferlan