[libvirt] [PATCH 0/4] Convert virStoragePoolObj into virObjectLockable
Next in the push to get the storage pool objects to be more common is to use the virObjectLockable for the Pool objects. The patches describe the transformation. John Ferlan (4): storage: Introduce virStoragePoolObjEndAPI storage: Introduce virStoragePoolObjListForEach storage: Introduce virStoragePoolObjListSearch storage: Convert virStoragePoolObj into virObjectLockable src/conf/virstorageobj.c | 194 +++++++++++----- src/conf/virstorageobj.h | 27 ++- src/libvirt_private.syms | 5 +- src/storage/storage_backend_scsi.c | 2 +- src/storage/storage_driver.c | 446 +++++++++++++++++++------------------ src/test/test_driver.c | 158 +++++++------ tests/storagevolxml2argvtest.c | 4 +- 7 files changed, 476 insertions(+), 360 deletions(-) -- 2.13.6
For now it'll just call the virStoragePoolObjUnlock, but a future adjustment will do something different. Since the new API will check for a NULL object before the Unlock call, callers no longer need to check for NULL before calling. The virStoragePoolObjUnlock is now private/static to virstorageobj.c with a short term forward reference. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virstorageobj.c | 15 ++++- src/conf/virstorageobj.h | 6 +- src/libvirt_private.syms | 2 +- src/storage/storage_backend_scsi.c | 2 +- src/storage/storage_driver.c | 115 +++++++++++++++++-------------------- src/test/test_driver.c | 71 +++++++++++------------ tests/storagevolxml2argvtest.c | 3 +- 7 files changed, 106 insertions(+), 108 deletions(-) diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index 50dbd7bf4d..2ca8453139 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -36,6 +36,9 @@ VIR_LOG_INIT("conf.virstorageobj"); +static void +virStoragePoolObjUnlock(virStoragePoolObjPtr obj); + struct _virStorageVolDefList { size_t count; @@ -77,6 +80,16 @@ virStoragePoolObjNew(void) } +void +virStoragePoolObjEndAPI(virStoragePoolObjPtr *obj) +{ + if (!*obj) + return; + + virStoragePoolObjUnlock(*obj); +} + + virStoragePoolDefPtr virStoragePoolObjGetDef(virStoragePoolObjPtr obj) { @@ -1274,7 +1287,7 @@ virStoragePoolObjLock(virStoragePoolObjPtr obj) } -void +static void virStoragePoolObjUnlock(virStoragePoolObjPtr obj) { virMutexUnlock(&obj->lock); diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h index 69e737226b..a4d7186d12 100644 --- a/src/conf/virstorageobj.h +++ b/src/conf/virstorageobj.h @@ -58,6 +58,9 @@ typedef bool virStoragePoolObjPtr virStoragePoolObjNew(void); +void +virStoragePoolObjEndAPI(virStoragePoolObjPtr *obj); + virStoragePoolDefPtr virStoragePoolObjGetDef(virStoragePoolObjPtr obj); @@ -240,9 +243,6 @@ virStoragePoolObjSourceFindDuplicate(virConnectPtr conn, void virStoragePoolObjLock(virStoragePoolObjPtr obj); -void -virStoragePoolObjUnlock(virStoragePoolObjPtr obj); - int virStoragePoolObjListExport(virConnectPtr conn, virStoragePoolObjListPtr poolobjs, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a91b87d09a..57ba8f4038 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1073,6 +1073,7 @@ virStoragePoolObjClearVols; virStoragePoolObjDecrAsyncjobs; virStoragePoolObjDefUseNewDef; virStoragePoolObjDeleteDef; +virStoragePoolObjEndAPI; virStoragePoolObjFindByName; virStoragePoolObjFindByUUID; virStoragePoolObjForEachVolume; @@ -1104,7 +1105,6 @@ virStoragePoolObjSetAutostart; virStoragePoolObjSetConfigFile; virStoragePoolObjSetDef; virStoragePoolObjSourceFindDuplicate; -virStoragePoolObjUnlock; virStoragePoolObjVolumeGetNames; virStoragePoolObjVolumeListExport; diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index ee79ad72f5..9347d66384 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -166,7 +166,7 @@ virStoragePoolFCRefreshThread(void *opaque) virStoragePoolObjClearVols(pool); found = virStorageBackendSCSIFindLUs(pool, host); } - virStoragePoolObjUnlock(pool); + virStoragePoolObjEndAPI(&pool); } while (!found && --tries); if (pool && !found) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index d209f5afb8..7cc3c518b4 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -167,7 +167,7 @@ storagePoolUpdateAllState(void) virStoragePoolObjLock(obj); storagePoolUpdateState(obj); - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); } } @@ -192,7 +192,7 @@ storageDriverAutostart(void) virStoragePoolObjLock(obj); if ((backend = virStorageBackendForType(def->type)) == NULL) { - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); continue; } @@ -203,7 +203,7 @@ storageDriverAutostart(void) virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to autostart storage pool '%s': %s"), def->name, virGetLastErrorMessage()); - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); continue; } started = true; @@ -229,7 +229,7 @@ storageDriverAutostart(void) } VIR_FREE(stateFile); } - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); } virObjectUnref(conn); @@ -458,7 +458,7 @@ storagePoolLookupByUUID(virConnectPtr conn, pool = virGetStoragePool(conn, def->name, def->uuid, NULL, NULL); cleanup: - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); return pool; } @@ -480,7 +480,7 @@ storagePoolLookupByName(virConnectPtr conn, pool = virGetStoragePool(conn, def->name, def->uuid, NULL, NULL); cleanup: - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); return pool; } @@ -501,7 +501,7 @@ storagePoolLookupByVolume(virStorageVolPtr vol) pool = virGetStoragePool(vol->conn, def->name, def->uuid, NULL, NULL); cleanup: - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); return pool; } @@ -632,7 +632,7 @@ storagePoolIsActive(virStoragePoolPtr pool) ret = virStoragePoolObjIsActive(obj); cleanup: - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); return ret; } @@ -654,7 +654,7 @@ storagePoolIsPersistent(virStoragePoolPtr pool) ret = virStoragePoolObjGetConfigFile(obj) ? 1 : 0; cleanup: - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); return ret; } @@ -753,8 +753,7 @@ storagePoolCreateXML(virConnectPtr conn, virStoragePoolDefFree(newDef); if (event) virObjectEventStateQueue(driver->storageEventState, event); - if (obj) - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); storageDriverUnlock(); return pool; } @@ -813,8 +812,7 @@ storagePoolDefineXML(virConnectPtr conn, if (event) virObjectEventStateQueue(driver->storageEventState, event); virStoragePoolDefFree(newDef); - if (obj) - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); storageDriverUnlock(); return pool; } @@ -874,8 +872,7 @@ storagePoolUndefine(virStoragePoolPtr pool) cleanup: if (event) virObjectEventStateQueue(driver->storageEventState, event); - if (obj) - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); storageDriverUnlock(); return ret; } @@ -958,8 +955,7 @@ storagePoolCreate(virStoragePoolPtr pool, VIR_FREE(stateFile); if (event) virObjectEventStateQueue(driver->storageEventState, event); - if (obj) - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); return ret; } @@ -1004,7 +1000,7 @@ storagePoolBuild(virStoragePoolPtr pool, cleanup: if (event) virObjectEventStateQueue(driver->storageEventState, event); - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); return ret; } @@ -1071,8 +1067,7 @@ storagePoolDestroy(virStoragePoolPtr pool) cleanup: if (event) virObjectEventStateQueue(driver->storageEventState, event); - if (obj) - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); storageDriverUnlock(); return ret; } @@ -1138,7 +1133,7 @@ storagePoolDelete(virStoragePoolPtr pool, cleanup: if (event) virObjectEventStateQueue(driver->storageEventState, event); - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); return ret; } @@ -1202,8 +1197,7 @@ storagePoolRefresh(virStoragePoolPtr pool, cleanup: if (event) virObjectEventStateQueue(driver->storageEventState, event); - if (obj) - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); storageDriverUnlock(); return ret; } @@ -1238,7 +1232,7 @@ storagePoolGetInfo(virStoragePoolPtr pool, ret = 0; cleanup: - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); return ret; } @@ -1270,7 +1264,7 @@ storagePoolGetXMLDesc(virStoragePoolPtr pool, ret = virStoragePoolDefFormat(curDef); cleanup: - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); return ret; } @@ -1293,7 +1287,7 @@ storagePoolGetAutostart(virStoragePoolPtr pool, ret = 0; cleanup: - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); return ret; } @@ -1356,8 +1350,7 @@ storagePoolSetAutostart(virStoragePoolPtr pool, ret = 0; cleanup: - if (obj) - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); storageDriverUnlock(); return ret; } @@ -1387,7 +1380,7 @@ storagePoolNumOfVolumes(virStoragePoolPtr pool) virStoragePoolNumOfVolumesCheckACL); cleanup: - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); return ret; } @@ -1418,7 +1411,7 @@ storagePoolListVolumes(virStoragePoolPtr pool, virStoragePoolListVolumesCheckACL, names, maxnames); cleanup: - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); return n; } @@ -1452,7 +1445,7 @@ storagePoolListAllVolumes(virStoragePoolPtr pool, cleanup: - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); return ret; } @@ -1492,7 +1485,7 @@ storageVolLookupByName(virStoragePoolPtr pool, voldef->key, NULL, NULL); cleanup: - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); return vol; } @@ -1516,7 +1509,7 @@ storageVolLookupByKey(virConnectPtr conn, if (voldef) { if (virStorageVolLookupByKeyEnsureACL(conn, def, voldef) < 0) { - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); goto cleanup; } @@ -1525,7 +1518,7 @@ storageVolLookupByKey(virConnectPtr conn, NULL, NULL); } } - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); } if (!vol) @@ -1560,7 +1553,7 @@ storageVolLookupByPath(virConnectPtr conn, def = virStoragePoolObjGetDef(obj); if (!virStoragePoolObjIsActive(obj)) { - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); continue; } @@ -1583,7 +1576,7 @@ storageVolLookupByPath(virConnectPtr conn, */ VIR_WARN("Failed to get stable path for pool '%s'", def->name); - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); continue; } break; @@ -1594,7 +1587,7 @@ storageVolLookupByPath(virConnectPtr conn, case VIR_STORAGE_POOL_ZFS: case VIR_STORAGE_POOL_LAST: if (VIR_STRDUP(stable_path, path) < 0) { - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); goto cleanup; } break; @@ -1605,7 +1598,7 @@ storageVolLookupByPath(virConnectPtr conn, if (voldef) { if (virStorageVolLookupByPathEnsureACL(conn, def, voldef) < 0) { - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); goto cleanup; } @@ -1614,7 +1607,7 @@ storageVolLookupByPath(virConnectPtr conn, NULL, NULL); } - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); } if (!vol) { @@ -1655,14 +1648,14 @@ storagePoolLookupByTargetPath(virConnectPtr conn, def = virStoragePoolObjGetDef(obj); if (!virStoragePoolObjIsActive(obj)) { - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); continue; } if (STREQ(path, def->target.path)) pool = virGetStoragePool(conn, def->name, def->uuid, NULL, NULL); - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); } storageDriverUnlock(); @@ -1751,7 +1744,7 @@ virStorageVolDefFromVol(virStorageVolPtr vol, return voldef; error: - virStoragePoolObjUnlock(*obj); + virStoragePoolObjEndAPI(obj); *obj = NULL; return NULL; @@ -1794,7 +1787,7 @@ storageVolDelete(virStorageVolPtr vol, ret = 0; cleanup: - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); return ret; } @@ -1885,7 +1878,7 @@ storageVolCreateXML(virStoragePoolPtr pool, /* Drop the pool lock during volume allocation */ virStoragePoolObjIncrAsyncjobs(obj); voldef->building = true; - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); buildret = backend->buildVol(pool->conn, obj, buildvoldef, flags); @@ -1932,8 +1925,7 @@ storageVolCreateXML(virStoragePoolPtr pool, cleanup: virObjectUnref(newvol); virStorageVolDefFree(voldef); - if (obj) - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); return vol; } @@ -1961,7 +1953,7 @@ storageVolCreateXMLFrom(virStoragePoolPtr pool, storageDriverLock(); obj = virStoragePoolObjFindByUUID(&driver->pools, pool->uuid); if (obj && STRNEQ(pool->name, volsrc->pool)) { - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); objsrc = virStoragePoolObjFindByName(&driver->pools, volsrc->pool); virStoragePoolObjLock(obj); } @@ -2082,11 +2074,11 @@ storageVolCreateXMLFrom(virStoragePoolPtr pool, virStoragePoolObjIncrAsyncjobs(obj); voldef->building = true; voldefsrc->in_use++; - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); if (objsrc) { virStoragePoolObjIncrAsyncjobs(objsrc); - virStoragePoolObjUnlock(objsrc); + virStoragePoolObjEndAPI(&objsrc); } buildret = backend->buildVolFrom(pool->conn, obj, shadowvol, voldefsrc, flags); @@ -2103,7 +2095,7 @@ storageVolCreateXMLFrom(virStoragePoolPtr pool, if (objsrc) { virStoragePoolObjDecrAsyncjobs(objsrc); - virStoragePoolObjUnlock(objsrc); + virStoragePoolObjEndAPI(&objsrc); objsrc = NULL; } @@ -2133,10 +2125,8 @@ storageVolCreateXMLFrom(virStoragePoolPtr pool, virObjectUnref(newvol); virStorageVolDefFree(voldef); VIR_FREE(shadowvol); - if (obj) - virStoragePoolObjUnlock(obj); - if (objsrc) - virStoragePoolObjUnlock(objsrc); + virStoragePoolObjEndAPI(&obj); + virStoragePoolObjEndAPI(&objsrc); return vol; } @@ -2179,7 +2169,7 @@ storageVolDownload(virStorageVolPtr vol, offset, length, flags); cleanup: - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); return ret; } @@ -2286,8 +2276,7 @@ virStorageVolPoolRefreshThread(void *opaque) cleanup: if (event) virObjectEventStateQueue(driver->storageEventState, event); - if (obj) - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); storageDriverUnlock(); virStorageVolPoolRefreshDataFree(cbdata); } @@ -2386,7 +2375,7 @@ storageVolUpload(virStorageVolPtr vol, cbdata = NULL; cleanup: - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); if (cbdata) virStorageVolPoolRefreshDataFree(cbdata); @@ -2488,7 +2477,7 @@ storageVolResize(virStorageVolPtr vol, ret = 0; cleanup: - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); return ret; } @@ -2554,7 +2543,7 @@ storageVolWipePattern(virStorageVolPtr vol, ret = 0; cleanup: - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); return ret; } @@ -2601,7 +2590,7 @@ storageVolGetInfoFlags(virStorageVolPtr vol, ret = 0; cleanup: - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); return ret; } @@ -2640,7 +2629,7 @@ storageVolGetXMLDesc(virStorageVolPtr vol, ret = virStorageVolDefFormat(def, voldef); cleanup: - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); return ret; } @@ -2662,7 +2651,7 @@ storageVolGetPath(virStorageVolPtr vol) ignore_value(VIR_STRDUP(ret, voldef->target.path)); cleanup: - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); return ret; } diff --git a/src/test/test_driver.c b/src/test/test_driver.c index dbde37cf39..72b3c6db5d 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1119,18 +1119,18 @@ testParseStorage(testDriverPtr privconn, } if (testStoragePoolObjSetDefaults(obj) == -1) { - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); goto error; } virStoragePoolObjSetActive(obj, true); /* Find storage volumes */ if (testOpenVolumesForPool(file, ctxt, obj, i+1) < 0) { - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); goto error; } - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); } ret = 0; @@ -4100,7 +4100,7 @@ testStoragePoolObjFindActiveByName(testDriverPtr privconn, if (!virStoragePoolObjIsActive(obj)) { virReportError(VIR_ERR_OPERATION_INVALID, _("storage pool '%s' is not active"), name); - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); return NULL; } @@ -4120,7 +4120,7 @@ testStoragePoolObjFindInactiveByName(testDriverPtr privconn, if (virStoragePoolObjIsActive(obj)) { virReportError(VIR_ERR_OPERATION_INVALID, _("storage pool '%s' is active"), name); - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); return NULL; } @@ -4165,7 +4165,7 @@ testStoragePoolLookupByUUID(virConnectPtr conn, pool = virGetStoragePool(conn, def->name, def->uuid, NULL, NULL); - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); return pool; } @@ -4185,7 +4185,7 @@ testStoragePoolLookupByName(virConnectPtr conn, pool = virGetStoragePool(conn, def->name, def->uuid, NULL, NULL); - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); return pool; } @@ -4294,7 +4294,7 @@ testStoragePoolIsActive(virStoragePoolPtr pool) cleanup: if (obj) - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); return ret; } @@ -4311,7 +4311,7 @@ testStoragePoolIsPersistent(virStoragePoolPtr pool) ret = virStoragePoolObjGetConfigFile(obj) ? 1 : 0; - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); return ret; } @@ -4336,7 +4336,7 @@ testStoragePoolCreate(virStoragePoolPtr pool, 0); testObjectEventQueue(privconn, event); - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); return 0; } @@ -4490,8 +4490,7 @@ testStoragePoolCreateXML(virConnectPtr conn, cleanup: virStoragePoolDefFree(newDef); testObjectEventQueue(privconn, event); - if (obj) - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); testDriverUnlock(privconn); return pool; } @@ -4539,8 +4538,7 @@ testStoragePoolDefineXML(virConnectPtr conn, cleanup: virStoragePoolDefFree(newDef); testObjectEventQueue(privconn, event); - if (obj) - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); testDriverUnlock(privconn); return pool; } @@ -4584,7 +4582,7 @@ testStoragePoolBuild(virStoragePoolPtr pool, VIR_STORAGE_POOL_EVENT_CREATED, 0); - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); testObjectEventQueue(privconn, event); return 0; @@ -4660,8 +4658,7 @@ testStoragePoolDestroy(virStoragePoolPtr pool) cleanup: testObjectEventQueue(privconn, event); - if (obj) - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); return ret; } @@ -4685,7 +4682,7 @@ testStoragePoolDelete(virStoragePoolPtr pool, testObjectEventQueue(privconn, event); - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); return 0; } @@ -4706,7 +4703,7 @@ testStoragePoolRefresh(virStoragePoolPtr pool, event = virStoragePoolEventRefreshNew(pool->name, pool->uuid); testObjectEventQueue(privconn, event); - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); return 0; } @@ -4732,7 +4729,7 @@ testStoragePoolGetInfo(virStoragePoolPtr pool, info->allocation = def->allocation; info->available = def->available; - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); return 0; } @@ -4752,7 +4749,7 @@ testStoragePoolGetXMLDesc(virStoragePoolPtr pool, ret = virStoragePoolDefFormat(virStoragePoolObjGetDef(obj)); - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); return ret; } @@ -4772,7 +4769,7 @@ testStoragePoolGetAutostart(virStoragePoolPtr pool, else *autostart = virStoragePoolObjIsAutostart(obj) ? 1 : 0; - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); return 0; } @@ -4799,7 +4796,7 @@ testStoragePoolSetAutostart(virStoragePoolPtr pool, ret = 0; cleanup: - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); return ret; } @@ -4816,7 +4813,7 @@ testStoragePoolNumOfVolumes(virStoragePoolPtr pool) ret = virStoragePoolObjNumOfVolumes(obj, pool->conn, NULL); - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); return ret; } @@ -4835,7 +4832,7 @@ testStoragePoolListVolumes(virStoragePoolPtr pool, n = virStoragePoolObjVolumeGetNames(obj, pool->conn, NULL, names, maxnames); - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); return n; } @@ -4863,7 +4860,7 @@ testStoragePoolListAllVolumes(virStoragePoolPtr pool, ret = virStoragePoolObjVolumeListExport(pool->conn, obj, vols, NULL); cleanup: - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); return ret; } @@ -4906,7 +4903,7 @@ testStorageVolLookupByName(virStoragePoolPtr pool, NULL, NULL); cleanup: - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); return ret; } @@ -4933,11 +4930,11 @@ testStorageVolLookupByKey(virConnectPtr conn, ret = virGetStorageVol(conn, def->name, privvol->name, privvol->key, NULL, NULL); - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); break; } } - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); } testDriverUnlock(privconn); @@ -4971,11 +4968,11 @@ testStorageVolLookupByPath(virConnectPtr conn, ret = virGetStorageVol(conn, def->name, privvol->name, privvol->key, NULL, NULL); - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); break; } } - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); } testDriverUnlock(privconn); @@ -5041,7 +5038,7 @@ testStorageVolCreateXML(virStoragePoolPtr pool, cleanup: virStorageVolDefFree(privvol); - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); return ret; } @@ -5109,7 +5106,7 @@ testStorageVolCreateXMLFrom(virStoragePoolPtr pool, cleanup: virStorageVolDefFree(privvol); - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); return ret; } @@ -5141,7 +5138,7 @@ testStorageVolDelete(virStorageVolPtr vol, ret = 0; cleanup: - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); return ret; } @@ -5185,7 +5182,7 @@ testStorageVolGetInfo(virStorageVolPtr vol, ret = 0; cleanup: - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); return ret; } @@ -5210,7 +5207,7 @@ testStorageVolGetXMLDesc(virStorageVolPtr vol, ret = virStorageVolDefFormat(virStoragePoolObjGetDef(obj), privvol); cleanup: - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); return ret; } @@ -5232,7 +5229,7 @@ testStorageVolGetPath(virStorageVolPtr vol) ignore_value(VIR_STRDUP(ret, privvol->target.path)); cleanup: - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); return ret; } diff --git a/tests/storagevolxml2argvtest.c b/tests/storagevolxml2argvtest.c index 7b62243df3..0b2f2bb3d3 100644 --- a/tests/storagevolxml2argvtest.c +++ b/tests/storagevolxml2argvtest.c @@ -110,8 +110,7 @@ testCompareXMLToArgvFiles(bool shouldFail, virStorageVolDefFree(inputvol); virCommandFree(cmd); VIR_FREE(actualCmdline); - if (obj) - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); virStoragePoolObjFree(obj); virObjectUnref(conn); return ret; -- 2.13.6
On Thu, Nov 16, 2017 at 11:58:02AM -0500, John Ferlan wrote:
For now it'll just call the virStoragePoolObjUnlock, but a future adjustment will do something different. Since the new API will check for a NULL object before the Unlock call, callers no longer need to check for NULL before calling.
The virStoragePoolObjUnlock is now private/static to virstorageobj.c with a short term forward reference.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Create an API to walk the pools->objs[] list in order to perform a callback function for each element of the objs array that doesn't care about whether the action succeeds or fails as the desire is to run the code over every element in the array rather than fail as soon as or if one fails. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virstorageobj.c | 29 +++++++++++ src/conf/virstorageobj.h | 9 ++++ src/libvirt_private.syms | 1 + src/storage/storage_driver.c | 115 +++++++++++++++++++++++-------------------- 4 files changed, 100 insertions(+), 54 deletions(-) diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index 2ca8453139..3cae34d970 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -230,6 +230,35 @@ virStoragePoolObjListFree(virStoragePoolObjListPtr pools) } +/** + * virStoragePoolObjListForEach + * @pools: Pointer to pools object + * @iter: Callback iteration helper + * @opaque: Opaque data to use as argument to helper + * + * For each object in @pools, call the @iter helper using @opaque as + * an argument. This function doesn't care whether the @iter fails or + * not as it's being used for Autostart and UpdateAllState callers + * that want to iterate over all the @pools objects not stopping if + * one happens to fail. + */ +void +virStoragePoolObjListForEach(virStoragePoolObjListPtr pools, + virStoragePoolObjListIterator iter, + const void *opaque) +{ + size_t i; + virStoragePoolObjPtr obj; + + for (i = 0; i < pools->count; i++) { + obj = pools->objs[i]; + virStoragePoolObjLock(obj); + iter(obj, opaque); + virStoragePoolObjUnlock(obj); + } +} + + void virStoragePoolObjRemove(virStoragePoolObjListPtr pools, virStoragePoolObjPtr obj) diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h index a4d7186d12..c84877694e 100644 --- a/src/conf/virstorageobj.h +++ b/src/conf/virstorageobj.h @@ -226,6 +226,15 @@ virStoragePoolObjFree(virStoragePoolObjPtr obj); void virStoragePoolObjListFree(virStoragePoolObjListPtr pools); +typedef void +(*virStoragePoolObjListIterator)(virStoragePoolObjPtr obj, + const void *opaque); + +void +virStoragePoolObjListForEach(virStoragePoolObjListPtr pools, + virStoragePoolObjListIterator iter, + const void *opaque); + void virStoragePoolObjRemove(virStoragePoolObjListPtr pools, virStoragePoolObjPtr obj); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 57ba8f4038..62f423649a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1089,6 +1089,7 @@ virStoragePoolObjIsActive; virStoragePoolObjIsAutostart; virStoragePoolObjIsDuplicate; virStoragePoolObjListExport; +virStoragePoolObjListForEach; virStoragePoolObjListFree; virStoragePoolObjLoadAllConfigs; virStoragePoolObjLoadAllState; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 7cc3c518b4..db327a875a 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -157,24 +157,75 @@ storagePoolUpdateState(virStoragePoolObjPtr obj) return; } + + +static void +storagePoolUpdateAllStateCallback(virStoragePoolObjPtr obj, + const void *opaque ATTRIBUTE_UNUSED) +{ + storagePoolUpdateState(obj); +} + + static void storagePoolUpdateAllState(void) { - size_t i; + virStoragePoolObjListForEach(&driver->pools, + storagePoolUpdateAllStateCallback, + NULL); +} - for (i = 0; i < driver->pools.count; i++) { - virStoragePoolObjPtr obj = driver->pools.objs[i]; - virStoragePoolObjLock(obj); - storagePoolUpdateState(obj); - virStoragePoolObjEndAPI(&obj); +static void +storageDriverAutostartCallback(virStoragePoolObjPtr obj, + const void *opaque) +{ + virStoragePoolDefPtr def = virStoragePoolObjGetDef(obj); + virConnectPtr conn = (virConnectPtr) opaque; + virStorageBackendPtr backend; + bool started = false; + + if (!(backend = virStorageBackendForType(def->type))) + return; + + if (virStoragePoolObjIsAutostart(obj) && + !virStoragePoolObjIsActive(obj)) { + if (backend->startPool && + backend->startPool(conn, obj) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to autostart storage pool '%s': %s"), + def->name, virGetLastErrorMessage()); + return; + } + started = true; + } + + if (started) { + char *stateFile; + + virStoragePoolObjClearVols(obj); + stateFile = virFileBuildPath(driver->stateDir, def->name, ".xml"); + if (!stateFile || + virStoragePoolSaveState(stateFile, def) < 0 || + backend->refreshPool(conn, obj) < 0) { + if (stateFile) + unlink(stateFile); + if (backend->stopPool) + backend->stopPool(conn, obj); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to autostart storage pool '%s': %s"), + def->name, virGetLastErrorMessage()); + } else { + virStoragePoolObjSetActive(obj, true); + } + VIR_FREE(stateFile); } } + static void storageDriverAutostart(void) { - size_t i; virConnectPtr conn = NULL; /* XXX Remove hardcoding of QEMU URI */ @@ -184,53 +235,9 @@ storageDriverAutostart(void) conn = virConnectOpen("qemu:///session"); /* Ignoring NULL conn - let backends decide */ - for (i = 0; i < driver->pools.count; i++) { - virStoragePoolObjPtr obj = driver->pools.objs[i]; - virStoragePoolDefPtr def = virStoragePoolObjGetDef(obj); - virStorageBackendPtr backend; - bool started = false; - - virStoragePoolObjLock(obj); - if ((backend = virStorageBackendForType(def->type)) == NULL) { - virStoragePoolObjEndAPI(&obj); - continue; - } - - if (virStoragePoolObjIsAutostart(obj) && - !virStoragePoolObjIsActive(obj)) { - if (backend->startPool && - backend->startPool(conn, obj) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to autostart storage pool '%s': %s"), - def->name, virGetLastErrorMessage()); - virStoragePoolObjEndAPI(&obj); - continue; - } - started = true; - } - - if (started) { - char *stateFile; - - virStoragePoolObjClearVols(obj); - stateFile = virFileBuildPath(driver->stateDir, def->name, ".xml"); - if (!stateFile || - virStoragePoolSaveState(stateFile, def) < 0 || - backend->refreshPool(conn, obj) < 0) { - if (stateFile) - unlink(stateFile); - if (backend->stopPool) - backend->stopPool(conn, obj); - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to autostart storage pool '%s': %s"), - def->name, virGetLastErrorMessage()); - } else { - virStoragePoolObjSetActive(obj, true); - } - VIR_FREE(stateFile); - } - virStoragePoolObjEndAPI(&obj); - } + virStoragePoolObjListForEach(&driver->pools, + storageDriverAutostartCallback, + conn); virObjectUnref(conn); } -- 2.13.6
On Thu, Nov 16, 2017 at 11:58:03AM -0500, John Ferlan wrote:
Create an API to walk the pools->objs[] list in order to perform a callback function for each element of the objs array that doesn't care about whether the action succeeds or fails as the desire is to run the code over every element in the array rather than fail as soon as or if one fails.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virstorageobj.c | 29 +++++++++++ src/conf/virstorageobj.h | 9 ++++ src/libvirt_private.syms | 1 + src/storage/storage_driver.c | 115 +++++++++++++++++++++++-------------------- 4 files changed, 100 insertions(+), 54 deletions(-)
diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index 2ca8453139..3cae34d970 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -230,6 +230,35 @@ virStoragePoolObjListFree(virStoragePoolObjListPtr pools) }
+/** + * virStoragePoolObjListForEach + * @pools: Pointer to pools object + * @iter: Callback iteration helper + * @opaque: Opaque data to use as argument to helper + * + * For each object in @pools, call the @iter helper using @opaque as + * an argument. This function doesn't care whether the @iter fails or + * not as it's being used for Autostart and UpdateAllState callers + * that want to iterate over all the @pools objects not stopping if + * one happens to fail. + */ +void +virStoragePoolObjListForEach(virStoragePoolObjListPtr pools, + virStoragePoolObjListIterator iter, + const void *opaque) +{ + size_t i; + virStoragePoolObjPtr obj; + + for (i = 0; i < pools->count; i++) { + obj = pools->objs[i]; + virStoragePoolObjLock(obj); + iter(obj, opaque); + virStoragePoolObjUnlock(obj); + } +} + + void virStoragePoolObjRemove(virStoragePoolObjListPtr pools, virStoragePoolObjPtr obj) diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h index a4d7186d12..c84877694e 100644 --- a/src/conf/virstorageobj.h +++ b/src/conf/virstorageobj.h @@ -226,6 +226,15 @@ virStoragePoolObjFree(virStoragePoolObjPtr obj); void virStoragePoolObjListFree(virStoragePoolObjListPtr pools);
+typedef void +(*virStoragePoolObjListIterator)(virStoragePoolObjPtr obj, + const void *opaque); + +void +virStoragePoolObjListForEach(virStoragePoolObjListPtr pools, + virStoragePoolObjListIterator iter, + const void *opaque); + void virStoragePoolObjRemove(virStoragePoolObjListPtr pools, virStoragePoolObjPtr obj); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 57ba8f4038..62f423649a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1089,6 +1089,7 @@ virStoragePoolObjIsActive; virStoragePoolObjIsAutostart; virStoragePoolObjIsDuplicate; virStoragePoolObjListExport; +virStoragePoolObjListForEach; virStoragePoolObjListFree; virStoragePoolObjLoadAllConfigs; virStoragePoolObjLoadAllState; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 7cc3c518b4..db327a875a 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -157,24 +157,75 @@ storagePoolUpdateState(virStoragePoolObjPtr obj) return; }
+ + +static void +storagePoolUpdateAllStateCallback(virStoragePoolObjPtr obj, + const void *opaque ATTRIBUTE_UNUSED) +{ + storagePoolUpdateState(obj); +}
May I suggest converting storagePoolUpdateState to storagePoolUpdateStateCallback? I this particular case I find the additional level of wrapping functions unnecessary. Reviewed-by: Erik Skultety <eskultet@redhat.com>
On 11/23/2017 04:42 AM, Erik Skultety wrote:
On Thu, Nov 16, 2017 at 11:58:03AM -0500, John Ferlan wrote:
Create an API to walk the pools->objs[] list in order to perform a callback function for each element of the objs array that doesn't care about whether the action succeeds or fails as the desire is to run the code over every element in the array rather than fail as soon as or if one fails.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virstorageobj.c | 29 +++++++++++ src/conf/virstorageobj.h | 9 ++++ src/libvirt_private.syms | 1 + src/storage/storage_driver.c | 115 +++++++++++++++++++++++-------------------- 4 files changed, 100 insertions(+), 54 deletions(-)
diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index 2ca8453139..3cae34d970 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -230,6 +230,35 @@ virStoragePoolObjListFree(virStoragePoolObjListPtr pools) }
+/** + * virStoragePoolObjListForEach + * @pools: Pointer to pools object + * @iter: Callback iteration helper + * @opaque: Opaque data to use as argument to helper + * + * For each object in @pools, call the @iter helper using @opaque as + * an argument. This function doesn't care whether the @iter fails or + * not as it's being used for Autostart and UpdateAllState callers + * that want to iterate over all the @pools objects not stopping if + * one happens to fail. + */ +void +virStoragePoolObjListForEach(virStoragePoolObjListPtr pools, + virStoragePoolObjListIterator iter, + const void *opaque) +{ + size_t i; + virStoragePoolObjPtr obj; + + for (i = 0; i < pools->count; i++) { + obj = pools->objs[i]; + virStoragePoolObjLock(obj); + iter(obj, opaque); + virStoragePoolObjUnlock(obj); + } +} + + void virStoragePoolObjRemove(virStoragePoolObjListPtr pools, virStoragePoolObjPtr obj) diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h index a4d7186d12..c84877694e 100644 --- a/src/conf/virstorageobj.h +++ b/src/conf/virstorageobj.h @@ -226,6 +226,15 @@ virStoragePoolObjFree(virStoragePoolObjPtr obj); void virStoragePoolObjListFree(virStoragePoolObjListPtr pools);
+typedef void +(*virStoragePoolObjListIterator)(virStoragePoolObjPtr obj, + const void *opaque); + +void +virStoragePoolObjListForEach(virStoragePoolObjListPtr pools, + virStoragePoolObjListIterator iter, + const void *opaque); + void virStoragePoolObjRemove(virStoragePoolObjListPtr pools, virStoragePoolObjPtr obj); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 57ba8f4038..62f423649a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1089,6 +1089,7 @@ virStoragePoolObjIsActive; virStoragePoolObjIsAutostart; virStoragePoolObjIsDuplicate; virStoragePoolObjListExport; +virStoragePoolObjListForEach; virStoragePoolObjListFree; virStoragePoolObjLoadAllConfigs; virStoragePoolObjLoadAllState; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 7cc3c518b4..db327a875a 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -157,24 +157,75 @@ storagePoolUpdateState(virStoragePoolObjPtr obj) return; }
+ + +static void +storagePoolUpdateAllStateCallback(virStoragePoolObjPtr obj, + const void *opaque ATTRIBUTE_UNUSED) +{ + storagePoolUpdateState(obj); +}
May I suggest converting storagePoolUpdateState to storagePoolUpdateStateCallback? I this particular case I find the additional level of wrapping functions unnecessary.
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Hmm.. right... I looked ahead in the other series to see if there was a reason for this and I couldn't find one (it's been a few weeks since I made the changes so I couldn't remember). At some point in time (shortly) there's a conversion to use hash tables too (it's really close). Tks - John (
Create an API to search through the storage pool objects looking for a specific truism from a callback API in order to return the specific storage pool object that is desired. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virstorageobj.c | 32 ++++++ src/conf/virstorageobj.h | 9 ++ src/libvirt_private.syms | 1 + src/storage/storage_driver.c | 226 +++++++++++++++++++++++-------------------- src/test/test_driver.c | 91 ++++++++++------- 5 files changed, 218 insertions(+), 141 deletions(-) diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index 3cae34d970..eb8a57324b 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -259,6 +259,38 @@ virStoragePoolObjListForEach(virStoragePoolObjListPtr pools, } +/** + * virStoragePoolObjListSearch + * @pools: Pointer to pools object + * @search: Callback searcher helper + * @opaque: Opaque data to use as argument to helper + * + * Search through the @pools objects calling the @search helper using + * the @opaque data in order to find an object that matches some criteria + * and return that object locked. + * + * Returns a locked object when found and NULL when not found + */ +virStoragePoolObjPtr +virStoragePoolObjListSearch(virStoragePoolObjListPtr pools, + virStoragePoolObjListSearcher searcher, + const void *opaque) +{ + size_t i; + virStoragePoolObjPtr obj; + + for (i = 0; i < pools->count; i++) { + obj = pools->objs[i]; + virStoragePoolObjLock(obj); + if (searcher(obj, opaque)) + return obj; + virStoragePoolObjUnlock(obj); + } + + return NULL; +} + + void virStoragePoolObjRemove(virStoragePoolObjListPtr pools, virStoragePoolObjPtr obj) diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h index c84877694e..7fe4a9f68a 100644 --- a/src/conf/virstorageobj.h +++ b/src/conf/virstorageobj.h @@ -235,6 +235,15 @@ virStoragePoolObjListForEach(virStoragePoolObjListPtr pools, virStoragePoolObjListIterator iter, const void *opaque); +typedef bool +(*virStoragePoolObjListSearcher)(virStoragePoolObjPtr obj, + const void *opaque); + +virStoragePoolObjPtr +virStoragePoolObjListSearch(virStoragePoolObjListPtr pools, + virStoragePoolObjListSearcher searcher, + const void *opaque); + void virStoragePoolObjRemove(virStoragePoolObjListPtr pools, virStoragePoolObjPtr obj); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 62f423649a..e4aebb3ca5 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1091,6 +1091,7 @@ virStoragePoolObjIsDuplicate; virStoragePoolObjListExport; virStoragePoolObjListForEach; virStoragePoolObjListFree; +virStoragePoolObjListSearch; virStoragePoolObjLoadAllConfigs; virStoragePoolObjLoadAllState; virStoragePoolObjLock; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index db327a875a..033196dc95 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1497,171 +1497,188 @@ storageVolLookupByName(virStoragePoolPtr pool, } +struct storageVolLookupData { + virConnectPtr conn; + const char *key; + char *cleanpath; + const char *path; + virStorageVolDefPtr voldef; +}; + +static bool +storageVolLookupByKeyCallback(virStoragePoolObjPtr obj, + const void *opaque) +{ + struct storageVolLookupData *data = (struct storageVolLookupData *) opaque; + + if (virStoragePoolObjIsActive(obj)) + data->voldef = virStorageVolDefFindByKey(obj, data->key); + + return !!data->voldef; +} + + static virStorageVolPtr storageVolLookupByKey(virConnectPtr conn, const char *key) { - size_t i; + virStoragePoolObjPtr obj; + virStoragePoolDefPtr def; + struct storageVolLookupData data = { + .conn = conn, .key = key, .voldef = NULL }; virStorageVolPtr vol = NULL; storageDriverLock(); - for (i = 0; i < driver->pools.count && !vol; i++) { - virStoragePoolObjPtr obj = driver->pools.objs[i]; - virStoragePoolDefPtr def; - - virStoragePoolObjLock(obj); + if ((obj = virStoragePoolObjListSearch(&driver->pools, + storageVolLookupByKeyCallback, + &data)) && data.voldef) { def = virStoragePoolObjGetDef(obj); - if (virStoragePoolObjIsActive(obj)) { - virStorageVolDefPtr voldef = virStorageVolDefFindByKey(obj, key); - - if (voldef) { - if (virStorageVolLookupByKeyEnsureACL(conn, def, voldef) < 0) { - virStoragePoolObjEndAPI(&obj); - goto cleanup; - } - - vol = virGetStorageVol(conn, def->name, - voldef->name, voldef->key, - NULL, NULL); - } + if (virStorageVolLookupByKeyEnsureACL(conn, def, data.voldef) == 0) { + vol = virGetStorageVol(conn, def->name, + data.voldef->name, data.voldef->key, + NULL, NULL); } virStoragePoolObjEndAPI(&obj); } + storageDriverUnlock(); if (!vol) virReportError(VIR_ERR_NO_STORAGE_VOL, _("no storage vol with matching key %s"), key); - cleanup: - storageDriverUnlock(); return vol; } + +static bool +storageVolLookupByPathCallback(virStoragePoolObjPtr obj, + const void *opaque) +{ + struct storageVolLookupData *data = (struct storageVolLookupData *) opaque; + virStoragePoolDefPtr def; + char *stable_path = NULL; + + if (!virStoragePoolObjIsActive(obj)) + return false; + + def = virStoragePoolObjGetDef(obj); + + switch ((virStoragePoolType) def->type) { + case VIR_STORAGE_POOL_DIR: + case VIR_STORAGE_POOL_FS: + case VIR_STORAGE_POOL_NETFS: + case VIR_STORAGE_POOL_LOGICAL: + case VIR_STORAGE_POOL_DISK: + case VIR_STORAGE_POOL_ISCSI: + case VIR_STORAGE_POOL_SCSI: + case VIR_STORAGE_POOL_MPATH: + case VIR_STORAGE_POOL_VSTORAGE: + stable_path = virStorageBackendStablePath(obj, data->cleanpath, + false); + break; + + case VIR_STORAGE_POOL_GLUSTER: + case VIR_STORAGE_POOL_RBD: + case VIR_STORAGE_POOL_SHEEPDOG: + case VIR_STORAGE_POOL_ZFS: + case VIR_STORAGE_POOL_LAST: + ignore_value(VIR_STRDUP(stable_path, data->path)); + break; + } + + /* Don't break the whole lookup process if it fails on + * getting the stable path for some of the pools. */ + if (!stable_path) { + VIR_WARN("Failed to get stable path for pool '%s'", def->name); + return false; + } + + data->voldef = virStorageVolDefFindByPath(obj, stable_path); + VIR_FREE(stable_path); + + return !!data->voldef; +} + + static virStorageVolPtr storageVolLookupByPath(virConnectPtr conn, const char *path) { - size_t i; + virStoragePoolObjPtr obj; + virStoragePoolDefPtr def; + struct storageVolLookupData data = { + .conn = conn, .path = path, .voldef = NULL }; virStorageVolPtr vol = NULL; - char *cleanpath; - cleanpath = virFileSanitizePath(path); - if (!cleanpath) + if (!(data.cleanpath = virFileSanitizePath(path))) return NULL; storageDriverLock(); - for (i = 0; i < driver->pools.count && !vol; i++) { - virStoragePoolObjPtr obj = driver->pools.objs[i]; - virStoragePoolDefPtr def; - virStorageVolDefPtr voldef; - char *stable_path = NULL; - - virStoragePoolObjLock(obj); + if ((obj = virStoragePoolObjListSearch(&driver->pools, + storageVolLookupByPathCallback, + &data)) && data.voldef) { def = virStoragePoolObjGetDef(obj); - if (!virStoragePoolObjIsActive(obj)) { - virStoragePoolObjEndAPI(&obj); - continue; - } - - switch ((virStoragePoolType) def->type) { - case VIR_STORAGE_POOL_DIR: - case VIR_STORAGE_POOL_FS: - case VIR_STORAGE_POOL_NETFS: - case VIR_STORAGE_POOL_LOGICAL: - case VIR_STORAGE_POOL_DISK: - case VIR_STORAGE_POOL_ISCSI: - case VIR_STORAGE_POOL_SCSI: - case VIR_STORAGE_POOL_MPATH: - case VIR_STORAGE_POOL_VSTORAGE: - stable_path = virStorageBackendStablePath(obj, - cleanpath, - false); - if (stable_path == NULL) { - /* Don't break the whole lookup process if it fails on - * getting the stable path for some of the pools. - */ - VIR_WARN("Failed to get stable path for pool '%s'", - def->name); - virStoragePoolObjEndAPI(&obj); - continue; - } - break; - - case VIR_STORAGE_POOL_GLUSTER: - case VIR_STORAGE_POOL_RBD: - case VIR_STORAGE_POOL_SHEEPDOG: - case VIR_STORAGE_POOL_ZFS: - case VIR_STORAGE_POOL_LAST: - if (VIR_STRDUP(stable_path, path) < 0) { - virStoragePoolObjEndAPI(&obj); - goto cleanup; - } - break; - } - - voldef = virStorageVolDefFindByPath(obj, stable_path); - VIR_FREE(stable_path); - - if (voldef) { - if (virStorageVolLookupByPathEnsureACL(conn, def, voldef) < 0) { - virStoragePoolObjEndAPI(&obj); - goto cleanup; - } - + if (virStorageVolLookupByPathEnsureACL(conn, def, data.voldef) == 0) { vol = virGetStorageVol(conn, def->name, - voldef->name, voldef->key, + data.voldef->name, data.voldef->key, NULL, NULL); } - virStoragePoolObjEndAPI(&obj); } + storageDriverUnlock(); if (!vol) { - if (STREQ(path, cleanpath)) { + if (STREQ(path, data.cleanpath)) { virReportError(VIR_ERR_NO_STORAGE_VOL, _("no storage vol with matching path '%s'"), path); } else { virReportError(VIR_ERR_NO_STORAGE_VOL, _("no storage vol with matching path '%s' (%s)"), - path, cleanpath); + path, data.cleanpath); } } - cleanup: - VIR_FREE(cleanpath); - storageDriverUnlock(); + VIR_FREE(data.cleanpath); return vol; } + +static bool +storagePoolLookupByTargetPathCallback(virStoragePoolObjPtr obj, + const void *opaque) +{ + const char *path = opaque; + virStoragePoolDefPtr def; + + if (!virStoragePoolObjIsActive(obj)) + return false; + + def = virStoragePoolObjGetDef(obj); + return STREQ(path, def->target.path); +} + + virStoragePoolPtr storagePoolLookupByTargetPath(virConnectPtr conn, const char *path) { - size_t i; + virStoragePoolObjPtr obj; + virStoragePoolDefPtr def; virStoragePoolPtr pool = NULL; char *cleanpath; cleanpath = virFileSanitizePath(path); if (!cleanpath) return NULL; + VIR_FREE(cleanpath); storageDriverLock(); - for (i = 0; i < driver->pools.count && !pool; i++) { - virStoragePoolObjPtr obj = driver->pools.objs[i]; - virStoragePoolDefPtr def; - - virStoragePoolObjLock(obj); + if ((obj == virStoragePoolObjListSearch(&driver->pools, + storagePoolLookupByTargetPathCallback, + path))) { def = virStoragePoolObjGetDef(obj); - - if (!virStoragePoolObjIsActive(obj)) { - virStoragePoolObjEndAPI(&obj); - continue; - } - - if (STREQ(path, def->target.path)) - pool = virGetStoragePool(conn, def->name, def->uuid, NULL, NULL); - + pool = virGetStoragePool(conn, def->name, def->uuid, NULL, NULL); virStoragePoolObjEndAPI(&obj); } storageDriverUnlock(); @@ -1672,7 +1689,6 @@ storagePoolLookupByTargetPath(virConnectPtr conn, path); } - VIR_FREE(cleanpath); return pool; } diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 72b3c6db5d..25b6592bcd 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -4908,6 +4908,26 @@ testStorageVolLookupByName(virStoragePoolPtr pool, } +struct storageVolLookupData { + virConnectPtr conn; + const char *key; + const char *path; + virStorageVolDefPtr voldef; +}; + +static bool +testStorageVolLookupByKeyCallback(virStoragePoolObjPtr obj, + const void *opaque) +{ + struct storageVolLookupData *data = (struct storageVolLookupData *) opaque; + + if (virStoragePoolObjIsActive(obj)) + data->voldef = virStorageVolDefFindByKey(obj, data->key); + + return !!data->voldef; +} + + static virStorageVolPtr testStorageVolLookupByKey(virConnectPtr conn, const char *key) @@ -4915,34 +4935,40 @@ testStorageVolLookupByKey(virConnectPtr conn, testDriverPtr privconn = conn->privateData; virStoragePoolObjPtr obj; virStoragePoolDefPtr def; - size_t i; - virStorageVolPtr ret = NULL; + struct storageVolLookupData data = { + .conn = conn, .key = key, .voldef = NULL }; + virStorageVolPtr vol = NULL; testDriverLock(privconn); - for (i = 0; i < privconn->pools.count; i++) { - obj = privconn->pools.objs[i]; - virStoragePoolObjLock(obj); + if ((obj = virStoragePoolObjListSearch(&privconn->pools, + testStorageVolLookupByKeyCallback, + &data)) && data.voldef) { def = virStoragePoolObjGetDef(obj); - if (virStoragePoolObjIsActive(obj)) { - virStorageVolDefPtr privvol = virStorageVolDefFindByKey(obj, key); - - if (privvol) { - ret = virGetStorageVol(conn, def->name, - privvol->name, privvol->key, - NULL, NULL); - virStoragePoolObjEndAPI(&obj); - break; - } - } + vol = virGetStorageVol(conn, def->name, + data.voldef->name, data.voldef->key, + NULL, NULL); virStoragePoolObjEndAPI(&obj); } testDriverUnlock(privconn); - if (!ret) + if (!vol) virReportError(VIR_ERR_NO_STORAGE_VOL, _("no storage vol with matching key '%s'"), key); - return ret; + return vol; +} + + +static bool +testStorageVolLookupByPathCallback(virStoragePoolObjPtr obj, + const void *opaque) +{ + struct storageVolLookupData *data = (struct storageVolLookupData *) opaque; + + if (virStoragePoolObjIsActive(obj)) + data->voldef = virStorageVolDefFindByPath(obj, data->path); + + return !!data->voldef; } @@ -4953,34 +4979,27 @@ testStorageVolLookupByPath(virConnectPtr conn, testDriverPtr privconn = conn->privateData; virStoragePoolObjPtr obj; virStoragePoolDefPtr def; - size_t i; - virStorageVolPtr ret = NULL; + struct storageVolLookupData data = { + .conn = conn, .path = path, .voldef = NULL }; + virStorageVolPtr vol = NULL; testDriverLock(privconn); - for (i = 0; i < privconn->pools.count; i++) { - obj = privconn->pools.objs[i]; - virStoragePoolObjLock(obj); + if ((obj = virStoragePoolObjListSearch(&privconn->pools, + testStorageVolLookupByPathCallback, + &data)) && data.voldef) { def = virStoragePoolObjGetDef(obj); - if (virStoragePoolObjIsActive(obj)) { - virStorageVolDefPtr privvol = virStorageVolDefFindByPath(obj, path); - - if (privvol) { - ret = virGetStorageVol(conn, def->name, - privvol->name, privvol->key, - NULL, NULL); - virStoragePoolObjEndAPI(&obj); - break; - } - } + vol = virGetStorageVol(conn, def->name, + data.voldef->name, data.voldef->key, + NULL, NULL); virStoragePoolObjEndAPI(&obj); } testDriverUnlock(privconn); - if (!ret) + if (!vol) virReportError(VIR_ERR_NO_STORAGE_VOL, _("no storage vol with matching path '%s'"), path); - return ret; + return vol; } -- 2.13.6
On Thu, Nov 16, 2017 at 11:58:04AM -0500, John Ferlan wrote:
Create an API to search through the storage pool objects looking for a specific truism from a callback API in order to return the specific storage pool object that is desired.
Signed-off-by: John Ferlan <jferlan@redhat.com> ---
As for the searcher that is a copy-paste of the 'for each iterator' with a single difference, I assume the list is going to be converted to a hash table further on, right? ...
+ +static bool +storagePoolLookupByTargetPathCallback(virStoragePoolObjPtr obj, + const void *opaque) +{ + const char *path = opaque; + virStoragePoolDefPtr def; + + if (!virStoragePoolObjIsActive(obj)) + return false; + + def = virStoragePoolObjGetDef(obj); + return STREQ(path, def->target.path); +} + + virStoragePoolPtr storagePoolLookupByTargetPath(virConnectPtr conn, const char *path) { - size_t i; + virStoragePoolObjPtr obj; + virStoragePoolDefPtr def; virStoragePoolPtr pool = NULL; char *cleanpath;
cleanpath = virFileSanitizePath(path); if (!cleanpath) return NULL; + VIR_FREE(cleanpath);
Existing prior your patch, but I'm clearly missing the point of running virFileSanitizePath here, since it can only return NULL on a failed allocation and the way we're treating it now is "it either failed or some sanitization may or may not have happened" - my question therefore is, why don't we care about the result of sanitizing the path but we have to run it anyway?
storageDriverLock(); - for (i = 0; i < driver->pools.count && !pool; i++) { - virStoragePoolObjPtr obj = driver->pools.objs[i]; - virStoragePoolDefPtr def; - - virStoragePoolObjLock(obj); + if ((obj == virStoragePoolObjListSearch(&driver->pools, + storagePoolLookupByTargetPathCallback, + path))) { def = virStoragePoolObjGetDef(obj); - - if (!virStoragePoolObjIsActive(obj)) { - virStoragePoolObjEndAPI(&obj); - continue; - } - - if (STREQ(path, def->target.path)) - pool = virGetStoragePool(conn, def->name, def->uuid, NULL, NULL); - + pool = virGetStoragePool(conn, def->name, def->uuid, NULL, NULL); virStoragePoolObjEndAPI(&obj); } storageDriverUnlock(); @@ -1672,7 +1689,6 @@ storagePoolLookupByTargetPath(virConnectPtr conn, path); }
- VIR_FREE(cleanpath); return pool; }
Reviewed-by: Erik Skultety <eskultet@redhat.com>
On 11/23/2017 09:00 AM, Erik Skultety wrote:
On Thu, Nov 16, 2017 at 11:58:04AM -0500, John Ferlan wrote:
Create an API to search through the storage pool objects looking for a specific truism from a callback API in order to return the specific storage pool object that is desired.
Signed-off-by: John Ferlan <jferlan@redhat.com> ---
As for the searcher that is a copy-paste of the 'for each iterator' with a single difference, I assume the list is going to be converted to a hash table further on, right?
...
Your assumption is correct... And the fact that Hash is used instead of linear search would be obfuscated by the virStoragePoolObjList API's. IIRC this driver ended up being a little different than others w/r/t what's done that requires keeping certain functionality in the driver as opposed to moving it to virstoragepoolobj.c. Of course it's been a while since I first thought about it though! This adaptation comes from the original pile I wrote in Jan/Feb...
+ +static bool +storagePoolLookupByTargetPathCallback(virStoragePoolObjPtr obj, + const void *opaque) +{ + const char *path = opaque; + virStoragePoolDefPtr def; + + if (!virStoragePoolObjIsActive(obj)) + return false; + + def = virStoragePoolObjGetDef(obj); + return STREQ(path, def->target.path); +} + + virStoragePoolPtr storagePoolLookupByTargetPath(virConnectPtr conn, const char *path) { - size_t i; + virStoragePoolObjPtr obj; + virStoragePoolDefPtr def; virStoragePoolPtr pool = NULL; char *cleanpath;
cleanpath = virFileSanitizePath(path); if (!cleanpath) return NULL; + VIR_FREE(cleanpath);
Existing prior your patch, but I'm clearly missing the point of running virFileSanitizePath here, since it can only return NULL on a failed allocation and the way we're treating it now is "it either failed or some sanitization may or may not have happened" - my question therefore is, why don't we care about the result of sanitizing the path but we have to run it anyway?
Hmm.. no idea... Good question... It was added by commit id '5ab746b83'... I just saw that it wasn't used after getting it so I moved the VIR_FREE. I wonder if the original intent was to copy the same error message logic that storageVolLookupByPath uses... Or even use it in the STREQ comparison. I'll mark this as something to look at prior to the next series... tks - John
storageDriverLock(); - for (i = 0; i < driver->pools.count && !pool; i++) { - virStoragePoolObjPtr obj = driver->pools.objs[i]; - virStoragePoolDefPtr def; - - virStoragePoolObjLock(obj); + if ((obj == virStoragePoolObjListSearch(&driver->pools, + storagePoolLookupByTargetPathCallback, + path))) { def = virStoragePoolObjGetDef(obj); - - if (!virStoragePoolObjIsActive(obj)) { - virStoragePoolObjEndAPI(&obj); - continue; - } - - if (STREQ(path, def->target.path)) - pool = virGetStoragePool(conn, def->name, def->uuid, NULL, NULL); - + pool = virGetStoragePool(conn, def->name, def->uuid, NULL, NULL); virStoragePoolObjEndAPI(&obj); } storageDriverUnlock(); @@ -1672,7 +1689,6 @@ storagePoolLookupByTargetPath(virConnectPtr conn, path); }
- VIR_FREE(cleanpath); return pool; }
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Now that we're moved the object into virstorageobj, let's make the code use the lockable object. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virstorageobj.c | 136 +++++++++++++++++++++-------------------- src/conf/virstorageobj.h | 3 - src/libvirt_private.syms | 1 - src/storage/storage_driver.c | 18 +++--- tests/storagevolxml2argvtest.c | 1 - 5 files changed, 77 insertions(+), 82 deletions(-) diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index eb8a57324b..357e6a967e 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -36,8 +36,10 @@ VIR_LOG_INIT("conf.virstorageobj"); +static virClassPtr virStoragePoolObjClass; + static void -virStoragePoolObjUnlock(virStoragePoolObjPtr obj); +virStoragePoolObjDispose(void *opaque); struct _virStorageVolDefList { @@ -46,7 +48,7 @@ struct _virStorageVolDefList { }; struct _virStoragePoolObj { - virMutex lock; + virObjectLockable parent; char *configFile; char *autostartLink; @@ -60,21 +62,34 @@ struct _virStoragePoolObj { virStorageVolDefList volumes; }; + +static int +virStoragePoolObjOnceInit(void) +{ + if (!(virStoragePoolObjClass = virClassNew(virClassForObjectLockable(), + "virStoragePoolObj", + sizeof(virStoragePoolObj), + virStoragePoolObjDispose))) + return -1; + + return 0; +} + +VIR_ONCE_GLOBAL_INIT(virStoragePoolObj) + + virStoragePoolObjPtr virStoragePoolObjNew(void) { virStoragePoolObjPtr obj; - if (VIR_ALLOC(obj) < 0) + if (virStoragePoolObjInitialize() < 0) return NULL; - if (virMutexInit(&obj->lock) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cannot initialize mutex")); - VIR_FREE(obj); + if (!(obj = virObjectLockableNew(virStoragePoolObjClass))) return NULL; - } - virStoragePoolObjLock(obj); + + virObjectLock(obj); obj->active = false; return obj; } @@ -86,7 +101,9 @@ virStoragePoolObjEndAPI(virStoragePoolObjPtr *obj) if (!*obj) return; - virStoragePoolObjUnlock(*obj); + virObjectUnlock(*obj); + virObjectUnref(*obj); + *obj = NULL; } @@ -200,8 +217,10 @@ virStoragePoolObjDecrAsyncjobs(virStoragePoolObjPtr obj) void -virStoragePoolObjFree(virStoragePoolObjPtr obj) +virStoragePoolObjDispose(void *opaque) { + virStoragePoolObjPtr obj = opaque; + if (!obj) return; @@ -212,10 +231,6 @@ virStoragePoolObjFree(virStoragePoolObjPtr obj) VIR_FREE(obj->configFile); VIR_FREE(obj->autostartLink); - - virMutexDestroy(&obj->lock); - - VIR_FREE(obj); } @@ -224,7 +239,7 @@ virStoragePoolObjListFree(virStoragePoolObjListPtr pools) { size_t i; for (i = 0; i < pools->count; i++) - virStoragePoolObjFree(pools->objs[i]); + virObjectUnref(pools->objs[i]); VIR_FREE(pools->objs); pools->count = 0; } @@ -252,9 +267,9 @@ virStoragePoolObjListForEach(virStoragePoolObjListPtr pools, for (i = 0; i < pools->count; i++) { obj = pools->objs[i]; - virStoragePoolObjLock(obj); + virObjectLock(obj); iter(obj, opaque); - virStoragePoolObjUnlock(obj); + virObjectUnlock(obj); } } @@ -269,7 +284,7 @@ virStoragePoolObjListForEach(virStoragePoolObjListPtr pools, * the @opaque data in order to find an object that matches some criteria * and return that object locked. * - * Returns a locked object when found and NULL when not found + * Returns a locked and reffed object when found and NULL when not found */ virStoragePoolObjPtr virStoragePoolObjListSearch(virStoragePoolObjListPtr pools, @@ -281,10 +296,10 @@ virStoragePoolObjListSearch(virStoragePoolObjListPtr pools, for (i = 0; i < pools->count; i++) { obj = pools->objs[i]; - virStoragePoolObjLock(obj); + virObjectLock(obj); if (searcher(obj, opaque)) - return obj; - virStoragePoolObjUnlock(obj); + return virObjectRef(obj); + virObjectUnlock(obj); } return NULL; @@ -297,18 +312,18 @@ virStoragePoolObjRemove(virStoragePoolObjListPtr pools, { size_t i; - virStoragePoolObjUnlock(obj); + virObjectUnlock(obj); for (i = 0; i < pools->count; i++) { - virStoragePoolObjLock(pools->objs[i]); + virObjectLock(pools->objs[i]); if (pools->objs[i] == obj) { - virStoragePoolObjUnlock(pools->objs[i]); - virStoragePoolObjFree(pools->objs[i]); + virObjectUnlock(pools->objs[i]); + virObjectUnref(pools->objs[i]); VIR_DELETE_ELEMENT(pools->objs, i, pools->count); break; } - virStoragePoolObjUnlock(pools->objs[i]); + virObjectUnlock(pools->objs[i]); } } @@ -320,10 +335,12 @@ virStoragePoolObjFindByUUID(virStoragePoolObjListPtr pools, size_t i; for (i = 0; i < pools->count; i++) { - virStoragePoolObjLock(pools->objs[i]); - if (!memcmp(pools->objs[i]->def->uuid, uuid, VIR_UUID_BUFLEN)) - return pools->objs[i]; - virStoragePoolObjUnlock(pools->objs[i]); + virStoragePoolObjPtr obj = pools->objs[i]; + + virObjectLock(obj); + if (!memcmp(obj->def->uuid, uuid, VIR_UUID_BUFLEN)) + return virObjectRef(obj); + virObjectUnlock(obj); } return NULL; @@ -337,10 +354,12 @@ virStoragePoolObjFindByName(virStoragePoolObjListPtr pools, size_t i; for (i = 0; i < pools->count; i++) { - virStoragePoolObjLock(pools->objs[i]); - if (STREQ(pools->objs[i]->def->name, name)) - return pools->objs[i]; - virStoragePoolObjUnlock(pools->objs[i]); + virStoragePoolObjPtr obj = pools->objs[i]; + + virObjectLock(obj); + if (STREQ(obj->def->name, name)) + return virObjectRef(obj); + virObjectUnlock(obj); } return NULL; @@ -608,13 +627,12 @@ virStoragePoolObjAssignDef(virStoragePoolObjListPtr pools, return NULL; if (VIR_APPEND_ELEMENT_COPY(pools->objs, pools->count, obj) < 0) { - virStoragePoolObjUnlock(obj); - virStoragePoolObjFree(obj); + virStoragePoolObjEndAPI(&obj); return NULL; } obj->def = def; - return obj; + return virObjectRef(obj); } @@ -741,7 +759,7 @@ virStoragePoolObjLoadAllState(virStoragePoolObjListPtr pools, if (!(obj = virStoragePoolObjLoadState(pools, stateDir, entry->d_name))) continue; - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); } VIR_DIR_CLOSE(dir); @@ -780,8 +798,7 @@ virStoragePoolObjLoadAllConfigs(virStoragePoolObjListPtr pools, } obj = virStoragePoolObjLoad(pools, entry->d_name, path, autostartLink); - if (obj) - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); VIR_FREE(path); VIR_FREE(autostartLink); @@ -852,12 +869,12 @@ virStoragePoolObjNumOfStoragePools(virStoragePoolObjListPtr pools, for (i = 0; i < pools->count; i++) { virStoragePoolObjPtr obj = pools->objs[i]; - virStoragePoolObjLock(obj); + virObjectLock(obj); if (!filter || filter(conn, obj->def)) { if (wantActive == virStoragePoolObjIsActive(obj)) npools++; } - virStoragePoolObjUnlock(obj); + virObjectUnlock(obj); } return npools; @@ -877,17 +894,17 @@ virStoragePoolObjGetNames(virStoragePoolObjListPtr pools, for (i = 0; i < pools->count && nnames < maxnames; i++) { virStoragePoolObjPtr obj = pools->objs[i]; - virStoragePoolObjLock(obj); + virObjectLock(obj); if (!filter || filter(conn, obj->def)) { if (wantActive == virStoragePoolObjIsActive(obj)) { if (VIR_STRDUP(names[nnames], obj->def->name) < 0) { - virStoragePoolObjUnlock(obj); + virObjectUnlock(obj); goto failure; } nnames++; } } - virStoragePoolObjUnlock(obj); + virObjectUnlock(obj); } return nnames; @@ -957,8 +974,7 @@ virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools, } cleanup: - if (obj) - virStoragePoolObjUnlock(obj); + virStoragePoolObjEndAPI(&obj); return ret; } @@ -1273,7 +1289,7 @@ virStoragePoolObjSourceFindDuplicate(virConnectPtr conn, if (STREQ(obj->def->name, def->name)) continue; - virStoragePoolObjLock(obj); + virObjectLock(obj); switch ((virStoragePoolType)obj->def->type) { case VIR_STORAGE_POOL_DIR: @@ -1325,7 +1341,7 @@ virStoragePoolObjSourceFindDuplicate(virConnectPtr conn, case VIR_STORAGE_POOL_LAST: break; } - virStoragePoolObjUnlock(obj); + virObjectUnlock(obj); if (matchobj) break; @@ -1341,20 +1357,6 @@ virStoragePoolObjSourceFindDuplicate(virConnectPtr conn, } -void -virStoragePoolObjLock(virStoragePoolObjPtr obj) -{ - virMutexLock(&obj->lock); -} - - -static void -virStoragePoolObjUnlock(virStoragePoolObjPtr obj) -{ - virMutexUnlock(&obj->lock); -} - - #define MATCH(FLAG) (flags & (FLAG)) static bool virStoragePoolMatch(virStoragePoolObjPtr obj, @@ -1438,7 +1440,7 @@ virStoragePoolObjListExport(virConnectPtr conn, for (i = 0; i < poolobjs->count; i++) { virStoragePoolObjPtr obj = poolobjs->objs[i]; - virStoragePoolObjLock(obj); + virObjectLock(obj); if ((!filter || filter(conn, obj->def)) && virStoragePoolMatch(obj, flags)) { if (pools) { @@ -1446,14 +1448,14 @@ virStoragePoolObjListExport(virConnectPtr conn, obj->def->name, obj->def->uuid, NULL, NULL))) { - virStoragePoolObjUnlock(obj); + virObjectUnlock(obj); goto cleanup; } tmp_pools[npools] = pool; } npools++; } - virStoragePoolObjUnlock(obj); + virObjectUnlock(obj); } if (tmp_pools) { diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h index 7fe4a9f68a..34c4c9e10b 100644 --- a/src/conf/virstorageobj.h +++ b/src/conf/virstorageobj.h @@ -258,9 +258,6 @@ virStoragePoolObjSourceFindDuplicate(virConnectPtr conn, virStoragePoolObjListPtr pools, virStoragePoolDefPtr def); -void -virStoragePoolObjLock(virStoragePoolObjPtr obj); - int virStoragePoolObjListExport(virConnectPtr conn, virStoragePoolObjListPtr poolobjs, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index e4aebb3ca5..e408df8671 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1094,7 +1094,6 @@ virStoragePoolObjListFree; virStoragePoolObjListSearch; virStoragePoolObjLoadAllConfigs; virStoragePoolObjLoadAllState; -virStoragePoolObjLock; virStoragePoolObjNew; virStoragePoolObjNumOfStoragePools; virStoragePoolObjNumOfVolumes; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 033196dc95..e19b5a2071 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1768,7 +1768,6 @@ virStorageVolDefFromVol(virStorageVolPtr vol, error: virStoragePoolObjEndAPI(obj); - *obj = NULL; return NULL; } @@ -1901,14 +1900,14 @@ storageVolCreateXML(virStoragePoolPtr pool, /* Drop the pool lock during volume allocation */ virStoragePoolObjIncrAsyncjobs(obj); voldef->building = true; - virStoragePoolObjEndAPI(&obj); + virObjectUnlock(obj); buildret = backend->buildVol(pool->conn, obj, buildvoldef, flags); VIR_FREE(buildvoldef); storageDriverLock(); - virStoragePoolObjLock(obj); + virObjectLock(obj); storageDriverUnlock(); voldef->building = false; @@ -1976,9 +1975,9 @@ storageVolCreateXMLFrom(virStoragePoolPtr pool, storageDriverLock(); obj = virStoragePoolObjFindByUUID(&driver->pools, pool->uuid); if (obj && STRNEQ(pool->name, volsrc->pool)) { - virStoragePoolObjEndAPI(&obj); + virObjectUnlock(obj); objsrc = virStoragePoolObjFindByName(&driver->pools, volsrc->pool); - virStoragePoolObjLock(obj); + virObjectLock(obj); } storageDriverUnlock(); if (!obj) { @@ -2097,19 +2096,19 @@ storageVolCreateXMLFrom(virStoragePoolPtr pool, virStoragePoolObjIncrAsyncjobs(obj); voldef->building = true; voldefsrc->in_use++; - virStoragePoolObjEndAPI(&obj); + virObjectUnlock(obj); if (objsrc) { virStoragePoolObjIncrAsyncjobs(objsrc); - virStoragePoolObjEndAPI(&objsrc); + virObjectUnlock(objsrc); } buildret = backend->buildVolFrom(pool->conn, obj, shadowvol, voldefsrc, flags); storageDriverLock(); - virStoragePoolObjLock(obj); + virObjectLock(obj); if (objsrc) - virStoragePoolObjLock(objsrc); + virObjectLock(objsrc); storageDriverUnlock(); voldefsrc->in_use--; @@ -2119,7 +2118,6 @@ storageVolCreateXMLFrom(virStoragePoolPtr pool, if (objsrc) { virStoragePoolObjDecrAsyncjobs(objsrc); virStoragePoolObjEndAPI(&objsrc); - objsrc = NULL; } if (buildret < 0 || diff --git a/tests/storagevolxml2argvtest.c b/tests/storagevolxml2argvtest.c index 0b2f2bb3d3..1cd083766a 100644 --- a/tests/storagevolxml2argvtest.c +++ b/tests/storagevolxml2argvtest.c @@ -111,7 +111,6 @@ testCompareXMLToArgvFiles(bool shouldFail, virCommandFree(cmd); VIR_FREE(actualCmdline); virStoragePoolObjEndAPI(&obj); - virStoragePoolObjFree(obj); virObjectUnref(conn); return ret; } -- 2.13.6
On Thu, Nov 16, 2017 at 11:58:05AM -0500, John Ferlan wrote:
Now that we're moved the object into virstorageobj, let's make the code use the lockable object.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
participants (2)
-
Erik Skultety -
John Ferlan