[libvirt] [PATCH v2 00/12] Continue altering storage pool for privatization
Consider this round 1 of 2.... The next series will be 18 patches, but the majority of those deal with change every {pool|obj}->def->X to use the accessor virStoragePoolObjGetDef. v1: https://www.redhat.com/archives/libvir-list/2017-May/msg00218.html Probably not even worth looking at the v1, but this picks up where v1 left off somewhere around patch 10, but adding smaller steps between patches. John Ferlan (12): storage: Create accessor API's for virStoragePoolObj storage: Introduce virStoragePoolObjNew storage: Fill in storage pool @active properly storage: Introduce storage volume add, delete, count APIs storage: Introduce APIs to search/scan storage pool volumes list storage: Use virStoragePoolObj{Get|Set}ConfigFile storage: Use virStoragePoolObjGetAutostartLink storage: Use virStoragePoolObj{Is|Set}Active storage: Use virStoragePoolObj{Is|Set}Autostart storage: Internally represent @autostart to bool storage: Use virStoragePoolObj{Get|Incr}Decr}Asyncjobs storage: Use virStoragePoolObjDefUseNewDef src/conf/virstorageobj.c | 211 +++++++++++++++++++++++++++++++-- src/conf/virstorageobj.h | 84 ++++++++++++- src/libvirt_private.syms | 20 ++++ src/storage/storage_backend_disk.c | 93 +++++++++------ src/storage/storage_backend_gluster.c | 5 +- src/storage/storage_backend_logical.c | 4 +- src/storage/storage_backend_mpath.c | 3 +- src/storage/storage_backend_rbd.c | 4 +- src/storage/storage_backend_scsi.c | 4 +- src/storage/storage_backend_sheepdog.c | 4 +- src/storage/storage_backend_zfs.c | 6 +- src/storage/storage_driver.c | 142 +++++++++------------- src/storage/storage_util.c | 8 +- src/test/test_driver.c | 54 ++++----- tests/storagevolxml2argvtest.c | 20 ++-- 15 files changed, 465 insertions(+), 197 deletions(-) -- 2.9.5
In preparation for making a private object, create accessor API's for consumer storage functions to use: virStoragePoolObjGetDef virStoragePoolObjSetDef virStoragePoolObjGetNewDef virStoragePoolObjDefUseNewDef virStoragePoolObjGetConfigFile virStoragePoolObjSetConfigFile virStoragePoolObjGetAutostartLink virStoragePoolObjIsActive virStoragePoolObjSetActive virStoragePoolObjIsAutostart virStoragePoolObjSetAutostart virStoragePoolObjGetAsyncjobs virStoragePoolObjIncrAsyncjobs virStoragePoolObjDecrAsyncjobs Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virstorageobj.c | 109 +++++++++++++++++++++++++++++++++++++++++++++++ src/conf/virstorageobj.h | 50 +++++++++++++++++++--- src/libvirt_private.syms | 14 ++++++ 3 files changed, 168 insertions(+), 5 deletions(-) diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index e83044b..ebda9fe 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -37,6 +37,115 @@ VIR_LOG_INIT("conf.virstorageobj"); +virStoragePoolDefPtr +virStoragePoolObjGetDef(virStoragePoolObjPtr obj) +{ + return obj->def; +} + + +void +virStoragePoolObjSetDef(virStoragePoolObjPtr obj, + virStoragePoolDefPtr def) +{ + virStoragePoolDefFree(obj->def); + obj->def = def; +} + + +virStoragePoolDefPtr +virStoragePoolObjGetNewDef(virStoragePoolObjPtr obj) +{ + return obj->newDef; +} + + +void +virStoragePoolObjDefUseNewDef(virStoragePoolObjPtr obj) +{ + virStoragePoolDefFree(obj->def); + obj->def = obj->newDef; + obj->newDef = NULL; +} + + +char * +virStoragePoolObjGetConfigFile(virStoragePoolObjPtr obj) +{ + return obj->configFile; +} + + +void +virStoragePoolObjSetConfigFile(virStoragePoolObjPtr obj, + char *configFile) +{ + VIR_FREE(obj->configFile); + obj->configFile = configFile; +} + + +char * +virStoragePoolObjGetAutostartLink(virStoragePoolObjPtr obj) +{ + return obj->autostartLink; +} + + +bool +virStoragePoolObjIsActive(virStoragePoolObjPtr obj) +{ + return obj->active; +} + + +void +virStoragePoolObjSetActive(virStoragePoolObjPtr obj, + bool active) +{ + obj->active = active; +} + + +bool +virStoragePoolObjIsAutostart(virStoragePoolObjPtr obj) +{ + if (!obj->configFile) + return 0; + + return obj->autostart == 1; +} + + +void +virStoragePoolObjSetAutostart(virStoragePoolObjPtr obj, + int autostart) +{ + obj->autostart = autostart; +} + + +unsigned int +virStoragePoolObjGetAsyncjobs(virStoragePoolObjPtr obj) +{ + return obj->asyncjobs; +} + + +void +virStoragePoolObjIncrAsyncjobs(virStoragePoolObjPtr obj) +{ + obj->asyncjobs++; +} + + +void +virStoragePoolObjDecrAsyncjobs(virStoragePoolObjPtr obj) +{ + obj->asyncjobs--; +} + + void virStoragePoolObjFree(virStoragePoolObjPtr obj) { diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h index ac51b5f..a5ed8f8 100644 --- a/src/conf/virstorageobj.h +++ b/src/conf/virstorageobj.h @@ -70,11 +70,51 @@ typedef bool (*virStoragePoolObjListFilter)(virConnectPtr conn, virStoragePoolDefPtr def); -static inline int -virStoragePoolObjIsActive(virStoragePoolObjPtr obj) -{ - return obj->active; -} +virStoragePoolDefPtr +virStoragePoolObjGetDef(virStoragePoolObjPtr obj); + +void +virStoragePoolObjSetDef(virStoragePoolObjPtr obj, + virStoragePoolDefPtr def); + +virStoragePoolDefPtr +virStoragePoolObjGetNewDef(virStoragePoolObjPtr obj); + +void +virStoragePoolObjDefUseNewDef(virStoragePoolObjPtr obj); + +char * +virStoragePoolObjGetConfigFile(virStoragePoolObjPtr obj); + +void +virStoragePoolObjSetConfigFile(virStoragePoolObjPtr obj, + char *configFile); + +char * +virStoragePoolObjGetAutostartLink(virStoragePoolObjPtr obj); + +bool +virStoragePoolObjIsActive(virStoragePoolObjPtr obj); + +void +virStoragePoolObjSetActive(virStoragePoolObjPtr obj, + bool active); + +bool +virStoragePoolObjIsAutostart(virStoragePoolObjPtr obj); + +void +virStoragePoolObjSetAutostart(virStoragePoolObjPtr obj, + int autostart); + +unsigned int +virStoragePoolObjGetAsyncjobs(virStoragePoolObjPtr obj); + +void +virStoragePoolObjIncrAsyncjobs(virStoragePoolObjPtr obj); + +void +virStoragePoolObjDecrAsyncjobs(virStoragePoolObjPtr obj); int virStoragePoolObjLoadAllConfigs(virStoragePoolObjListPtr pools, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 2149b11..8bb1cbc 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1051,10 +1051,20 @@ virSecretObjSetValueSize; # conf/virstorageobj.h virStoragePoolObjAssignDef; virStoragePoolObjClearVols; +virStoragePoolObjDecrAsyncjobs; +virStoragePoolObjDefUseNewDef; virStoragePoolObjDeleteDef; virStoragePoolObjFindByName; virStoragePoolObjFindByUUID; +virStoragePoolObjGetAsyncjobs; +virStoragePoolObjGetAutostartLink; +virStoragePoolObjGetConfigFile; +virStoragePoolObjGetDef; virStoragePoolObjGetNames; +virStoragePoolObjGetNewDef; +virStoragePoolObjIncrAsyncjobs; +virStoragePoolObjIsActive; +virStoragePoolObjIsAutostart; virStoragePoolObjIsDuplicate; virStoragePoolObjListExport; virStoragePoolObjListFree; @@ -1065,6 +1075,10 @@ virStoragePoolObjNumOfStoragePools; virStoragePoolObjNumOfVolumes; virStoragePoolObjRemove; virStoragePoolObjSaveDef; +virStoragePoolObjSetActive; +virStoragePoolObjSetAutostart; +virStoragePoolObjSetConfigFile; +virStoragePoolObjSetDef; virStoragePoolObjSourceFindDuplicate; virStoragePoolObjUnlock; virStoragePoolObjVolumeGetNames; -- 2.9.5
Create/use a helper to perform object allocation. Adjust storagevolxml2argvtest.c in order to use the allocator and setting of the obj->def. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virstorageobj.c | 31 +++++++++++++++++++++---------- src/conf/virstorageobj.h | 3 +++ src/libvirt_private.syms | 1 + tests/storagevolxml2argvtest.c | 20 +++++++++++--------- 4 files changed, 36 insertions(+), 19 deletions(-) diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index ebda9fe..eb7664c 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -37,6 +37,26 @@ VIR_LOG_INIT("conf.virstorageobj"); +virStoragePoolObjPtr +virStoragePoolObjNew(void) +{ + virStoragePoolObjPtr obj; + + if (VIR_ALLOC(obj) < 0) + return NULL; + + if (virMutexInit(&obj->lock) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot initialize mutex")); + VIR_FREE(obj); + return NULL; + } + virStoragePoolObjLock(obj); + obj->active = 0; + return obj; +} + + virStoragePoolDefPtr virStoragePoolObjGetDef(virStoragePoolObjPtr obj) { @@ -421,17 +441,8 @@ virStoragePoolObjAssignDef(virStoragePoolObjListPtr pools, return obj; } - if (VIR_ALLOC(obj) < 0) - return NULL; - - if (virMutexInit(&obj->lock) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cannot initialize mutex")); - VIR_FREE(obj); + if (!(obj = virStoragePoolObjNew())) return NULL; - } - virStoragePoolObjLock(obj); - obj->active = 0; if (VIR_APPEND_ELEMENT_COPY(pools->objs, pools->count, obj) < 0) { virStoragePoolObjUnlock(obj); diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h index a5ed8f8..401c4d5 100644 --- a/src/conf/virstorageobj.h +++ b/src/conf/virstorageobj.h @@ -70,6 +70,9 @@ typedef bool (*virStoragePoolObjListFilter)(virConnectPtr conn, virStoragePoolDefPtr def); +virStoragePoolObjPtr +virStoragePoolObjNew(void); + virStoragePoolDefPtr virStoragePoolObjGetDef(virStoragePoolObjPtr obj); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 8bb1cbc..fc884fa 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1071,6 +1071,7 @@ virStoragePoolObjListFree; virStoragePoolObjLoadAllConfigs; virStoragePoolObjLoadAllState; virStoragePoolObjLock; +virStoragePoolObjNew; virStoragePoolObjNumOfStoragePools; virStoragePoolObjNumOfVolumes; virStoragePoolObjRemove; diff --git a/tests/storagevolxml2argvtest.c b/tests/storagevolxml2argvtest.c index 9e43204..2ee3dba 100644 --- a/tests/storagevolxml2argvtest.c +++ b/tests/storagevolxml2argvtest.c @@ -50,18 +50,19 @@ testCompareXMLToArgvFiles(bool shouldFail, virConnectPtr conn; virStorageVolDefPtr vol = NULL, inputvol = NULL; - virStoragePoolDefPtr pool = NULL; + virStoragePoolDefPtr def = NULL; virStoragePoolDefPtr inputpool = NULL; - virStoragePoolObj poolobj = {.def = NULL }; - + virStoragePoolObjPtr obj = NULL; if (!(conn = virGetConnect())) goto cleanup; - if (!(pool = virStoragePoolDefParseFile(poolxml))) + if (!(def = virStoragePoolDefParseFile(poolxml))) goto cleanup; - poolobj.def = pool; + if (!(obj = virStoragePoolObjNew())) + goto cleanup; + virStoragePoolObjSetDef(obj, def); if (inputpoolxml) { if (!(inputpool = virStoragePoolDefParseFile(inputpoolxml))) @@ -71,17 +72,17 @@ testCompareXMLToArgvFiles(bool shouldFail, if (inputvolxml) parse_flags |= VIR_VOL_XML_PARSE_NO_CAPACITY; - if (!(vol = virStorageVolDefParseFile(pool, volxml, parse_flags))) + if (!(vol = virStorageVolDefParseFile(def, volxml, parse_flags))) goto cleanup; if (inputvolxml && !(inputvol = virStorageVolDefParseFile(inputpool, inputvolxml, 0))) goto cleanup; - testSetVolumeType(vol, pool); + testSetVolumeType(vol, def); testSetVolumeType(inputvol, inputpool); - cmd = virStorageBackendCreateQemuImgCmdFromVol(conn, &poolobj, vol, + cmd = virStorageBackendCreateQemuImgCmdFromVol(conn, obj, vol, inputvol, flags, create_tool, imgformat, NULL); @@ -102,12 +103,13 @@ testCompareXMLToArgvFiles(bool shouldFail, ret = 0; cleanup: - virStoragePoolDefFree(pool); + virStoragePoolDefFree(def); virStoragePoolDefFree(inputpool); virStorageVolDefFree(vol); virStorageVolDefFree(inputvol); virCommandFree(cmd); VIR_FREE(actualCmdline); + virStoragePoolObjUnlock(obj); virObjectUnref(conn); return ret; } -- 2.9.5
On 08/24/2017 03:08 PM, John Ferlan wrote:
Create/use a helper to perform object allocation.
Adjust storagevolxml2argvtest.c in order to use the allocator and setting of the obj->def.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virstorageobj.c | 31 +++++++++++++++++++++---------- src/conf/virstorageobj.h | 3 +++ src/libvirt_private.syms | 1 + tests/storagevolxml2argvtest.c | 20 +++++++++++--------- 4 files changed, 36 insertions(+), 19 deletions(-)
diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index ebda9fe..eb7664c 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -37,6 +37,26 @@ VIR_LOG_INIT("conf.virstorageobj");
+virStoragePoolObjPtr +virStoragePoolObjNew(void) +{ + virStoragePoolObjPtr obj; + + if (VIR_ALLOC(obj) < 0) + return NULL; + + if (virMutexInit(&obj->lock) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot initialize mutex")); + VIR_FREE(obj); + return NULL; + } + virStoragePoolObjLock(obj); + obj->active = 0; + return obj; +} + + virStoragePoolDefPtr virStoragePoolObjGetDef(virStoragePoolObjPtr obj) { @@ -421,17 +441,8 @@ virStoragePoolObjAssignDef(virStoragePoolObjListPtr pools, return obj; }
- if (VIR_ALLOC(obj) < 0) - return NULL; - - if (virMutexInit(&obj->lock) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cannot initialize mutex")); - VIR_FREE(obj); + if (!(obj = virStoragePoolObjNew())) return NULL; - } - virStoragePoolObjLock(obj); - obj->active = 0;
if (VIR_APPEND_ELEMENT_COPY(pools->objs, pools->count, obj) < 0) { virStoragePoolObjUnlock(obj); diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h index a5ed8f8..401c4d5 100644 --- a/src/conf/virstorageobj.h +++ b/src/conf/virstorageobj.h @@ -70,6 +70,9 @@ typedef bool (*virStoragePoolObjListFilter)(virConnectPtr conn, virStoragePoolDefPtr def);
+virStoragePoolObjPtr +virStoragePoolObjNew(void); + virStoragePoolDefPtr virStoragePoolObjGetDef(virStoragePoolObjPtr obj);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 8bb1cbc..fc884fa 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1071,6 +1071,7 @@ virStoragePoolObjListFree; virStoragePoolObjLoadAllConfigs; virStoragePoolObjLoadAllState; virStoragePoolObjLock; +virStoragePoolObjNew; virStoragePoolObjNumOfStoragePools; virStoragePoolObjNumOfVolumes; virStoragePoolObjRemove; diff --git a/tests/storagevolxml2argvtest.c b/tests/storagevolxml2argvtest.c index 9e43204..2ee3dba 100644 --- a/tests/storagevolxml2argvtest.c +++ b/tests/storagevolxml2argvtest.c @@ -50,18 +50,19 @@ testCompareXMLToArgvFiles(bool shouldFail, virConnectPtr conn;
virStorageVolDefPtr vol = NULL, inputvol = NULL; - virStoragePoolDefPtr pool = NULL; + virStoragePoolDefPtr def = NULL; virStoragePoolDefPtr inputpool = NULL; - virStoragePoolObj poolobj = {.def = NULL }; - + virStoragePoolObjPtr obj = NULL;
if (!(conn = virGetConnect())) goto cleanup;
- if (!(pool = virStoragePoolDefParseFile(poolxml))) + if (!(def = virStoragePoolDefParseFile(poolxml))) goto cleanup;
- poolobj.def = pool; + if (!(obj = virStoragePoolObjNew())) + goto cleanup; + virStoragePoolObjSetDef(obj, def);
if (inputpoolxml) { if (!(inputpool = virStoragePoolDefParseFile(inputpoolxml))) @@ -71,17 +72,17 @@ testCompareXMLToArgvFiles(bool shouldFail, if (inputvolxml) parse_flags |= VIR_VOL_XML_PARSE_NO_CAPACITY;
- if (!(vol = virStorageVolDefParseFile(pool, volxml, parse_flags))) + if (!(vol = virStorageVolDefParseFile(def, volxml, parse_flags))) goto cleanup;
if (inputvolxml && !(inputvol = virStorageVolDefParseFile(inputpool, inputvolxml, 0))) goto cleanup;
- testSetVolumeType(vol, pool); + testSetVolumeType(vol, def); testSetVolumeType(inputvol, inputpool);
- cmd = virStorageBackendCreateQemuImgCmdFromVol(conn, &poolobj, vol, + cmd = virStorageBackendCreateQemuImgCmdFromVol(conn, obj, vol, inputvol, flags, create_tool, imgformat, NULL); @@ -102,12 +103,13 @@ testCompareXMLToArgvFiles(bool shouldFail, ret = 0;
cleanup: - virStoragePoolDefFree(pool); + virStoragePoolDefFree(def); virStoragePoolDefFree(inputpool); virStorageVolDefFree(vol); virStorageVolDefFree(inputvol); virCommandFree(cmd); VIR_FREE(actualCmdline); + virStoragePoolObjUnlock(obj);
Almost. Firstly, @def is now part of @obj so it shouldn't be freed separately. Secondly, @obj is leaked. ACK if you squash this in: diff --git i/tests/Makefile.am w/tests/Makefile.am index 1f8c4cd42..813888575 100644 --- i/tests/Makefile.am +++ w/tests/Makefile.am @@ -885,7 +885,10 @@ storagevolxml2argvtest_SOURCES = \ testutils.c testutils.h storagevolxml2argvtest_LDADD = \ $(LIBXML_LIBS) \ - ../src/libvirt_driver_storage_impl.la $(LDADDS) + ../src/libvirt_driver_storage_impl.la \ + ../src/libvirt_conf.la \ + ../src/libvirt_util.la \ + $(LDADDS) else ! WITH_STORAGE EXTRA_DIST += storagevolxml2argvtest.c diff --git i/tests/storagevolxml2argvtest.c w/tests/storagevolxml2argvtest.c index 2ee3dbad9..1b3003216 100644 --- i/tests/storagevolxml2argvtest.c +++ w/tests/storagevolxml2argvtest.c @@ -60,8 +60,10 @@ testCompareXMLToArgvFiles(bool shouldFail, if (!(def = virStoragePoolDefParseFile(poolxml))) goto cleanup; - if (!(obj = virStoragePoolObjNew())) + if (!(obj = virStoragePoolObjNew())) { + virStoragePoolDefFree(def); goto cleanup; + } virStoragePoolObjSetDef(obj, def); if (inputpoolxml) { @@ -103,13 +105,13 @@ testCompareXMLToArgvFiles(bool shouldFail, ret = 0; cleanup: - virStoragePoolDefFree(def); virStoragePoolDefFree(inputpool); virStorageVolDefFree(vol); virStorageVolDefFree(inputvol); virCommandFree(cmd); VIR_FREE(actualCmdline); virStoragePoolObjUnlock(obj); + virStoragePoolObjFree(obj); virObjectUnref(conn); return ret; } Alternatively, you can leave the virStoragePoolDefFree() call as is and just set @def = NULL right after virStoragePoolObjSetDef(). I'll leave it up to you which one you prefer. Michal
It's a bool not an int, so use true/false and not 1/0 Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virstorageobj.c | 4 ++-- src/test/test_driver.c | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index eb7664c..a9fa190 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -52,7 +52,7 @@ virStoragePoolObjNew(void) return NULL; } virStoragePoolObjLock(obj); - obj->active = 0; + obj->active = false; return obj; } @@ -544,7 +544,7 @@ virStoragePoolObjLoadState(virStoragePoolObjListPtr pools, * as active */ - obj->active = 1; + obj->active = true; cleanup: VIR_FREE(stateFile); diff --git a/src/test/test_driver.c b/src/test/test_driver.c index aa38f54..989c3a8 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1130,7 +1130,7 @@ testParseStorage(testDriverPtr privconn, virStoragePoolObjUnlock(obj); goto error; } - obj->active = 1; + obj->active = true; /* Find storage volumes */ if (testOpenVolumesForPool(file, ctxt, obj, i+1) < 0) { @@ -4322,7 +4322,7 @@ testStoragePoolCreate(virStoragePoolPtr pool, if (!(obj = testStoragePoolObjFindInactiveByName(privconn, pool->name))) return -1; - obj->active = 1; + obj->active = true; event = virStoragePoolEventLifecycleNew(pool->name, pool->uuid, VIR_STORAGE_POOL_EVENT_STARTED, @@ -4470,7 +4470,7 @@ testStoragePoolCreateXML(virConnectPtr conn, * code will not Remove the pool */ VIR_FREE(obj->configFile); - obj->active = 1; + obj->active = true; event = virStoragePoolEventLifecycleNew(obj->def->name, obj->def->uuid, VIR_STORAGE_POOL_EVENT_STARTED, @@ -4618,7 +4618,7 @@ testStoragePoolDestroy(virStoragePoolPtr pool) if (!(obj = testStoragePoolObjFindActiveByName(privconn, pool->name))) return -1; - obj->active = 0; + obj->active = false; if (obj->def->source.adapter.type == VIR_STORAGE_ADAPTER_TYPE_FC_HOST) { -- 2.9.5
Create/use virStoragePoolObjAddVol in order to add volumes onto list. Create/use virStoragePoolObjRemoveVol in order to remove volumes from list. Create/use virStoragePoolObjGetVolumesCount to get count of volumes on list. For the storage driver, the logic alters when the volumes.obj list grows to after we've fetched the volobj. This is an optimization of sorts, but also doesn't "needlessly" grow the volumes.objs list and then just decr the count if the virGetStorageVol fails. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virstorageobj.c | 37 ++++++++++++++++++++++++ src/conf/virstorageobj.h | 11 +++++++ src/libvirt_private.syms | 3 ++ src/storage/storage_backend_disk.c | 5 ++-- src/storage/storage_backend_gluster.c | 3 +- src/storage/storage_backend_logical.c | 4 +-- src/storage/storage_backend_mpath.c | 3 +- src/storage/storage_backend_rbd.c | 4 +-- src/storage/storage_backend_sheepdog.c | 4 +-- src/storage/storage_backend_zfs.c | 6 ++-- src/storage/storage_driver.c | 53 +++++++++------------------------- src/storage/storage_util.c | 8 ++--- src/test/test_driver.c | 18 ++++-------- 13 files changed, 85 insertions(+), 74 deletions(-) diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index a9fa190..912c27a 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -282,6 +282,43 @@ virStoragePoolObjClearVols(virStoragePoolObjPtr obj) } +int +virStoragePoolObjAddVol(virStoragePoolObjPtr obj, + virStorageVolDefPtr voldef) +{ + if (VIR_APPEND_ELEMENT(obj->volumes.objs, obj->volumes.count, voldef) < 0) + return -1; + return 0; +} + + +void +virStoragePoolObjRemoveVol(virStoragePoolObjPtr obj, + virStorageVolDefPtr voldef) +{ + virStoragePoolDefPtr def = virStoragePoolObjGetDef(obj); + size_t i; + + for (i = 0; i < obj->volumes.count; i++) { + if (obj->volumes.objs[i] == voldef) { + VIR_INFO("Deleting volume '%s' from storage pool '%s'", + voldef->name, def->name); + virStorageVolDefFree(voldef); + + VIR_DELETE_ELEMENT(obj->volumes.objs, i, obj->volumes.count); + return; + } + } +} + + +size_t +virStoragePoolObjGetVolumesCount(virStoragePoolObjPtr obj) +{ + return obj->volumes.count; +} + + virStorageVolDefPtr virStorageVolDefFindByKey(virStoragePoolObjPtr obj, const char *key) diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h index 401c4d5..d1a1247 100644 --- a/src/conf/virstorageobj.h +++ b/src/conf/virstorageobj.h @@ -136,6 +136,17 @@ virStoragePoolObjPtr virStoragePoolObjFindByName(virStoragePoolObjListPtr pools, const char *name); +int +virStoragePoolObjAddVol(virStoragePoolObjPtr obj, + virStorageVolDefPtr voldef); + +void +virStoragePoolObjRemoveVol(virStoragePoolObjPtr obj, + virStorageVolDefPtr voldef); + +size_t +virStoragePoolObjGetVolumesCount(virStoragePoolObjPtr obj); + virStorageVolDefPtr virStorageVolDefFindByKey(virStoragePoolObjPtr obj, const char *key); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fc884fa..a35299b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1049,6 +1049,7 @@ virSecretObjSetValueSize; # conf/virstorageobj.h +virStoragePoolObjAddVol; virStoragePoolObjAssignDef; virStoragePoolObjClearVols; virStoragePoolObjDecrAsyncjobs; @@ -1062,6 +1063,7 @@ virStoragePoolObjGetConfigFile; virStoragePoolObjGetDef; virStoragePoolObjGetNames; virStoragePoolObjGetNewDef; +virStoragePoolObjGetVolumesCount; virStoragePoolObjIncrAsyncjobs; virStoragePoolObjIsActive; virStoragePoolObjIsAutostart; @@ -1075,6 +1077,7 @@ virStoragePoolObjNew; virStoragePoolObjNumOfStoragePools; virStoragePoolObjNumOfVolumes; virStoragePoolObjRemove; +virStoragePoolObjRemoveVol; virStoragePoolObjSaveDef; virStoragePoolObjSetActive; virStoragePoolObjSetAutostart; diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index e8f67bb..0bf5567 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -65,8 +65,7 @@ virStorageBackendDiskMakeDataVol(virStoragePoolObjPtr pool, if (VIR_ALLOC(vol) < 0) return -1; if (VIR_STRDUP(vol->name, partname) < 0 || - VIR_APPEND_ELEMENT_COPY(pool->volumes.objs, - pool->volumes.count, vol) < 0) { + virStoragePoolObjAddVol(pool, vol) < 0) { virStorageVolDefFree(vol); return -1; } @@ -595,7 +594,7 @@ virStorageBackendDiskPartFormat(virStoragePoolObjPtr pool, break; } } - if (i == pool->volumes.count) { + if (i == virStoragePoolObjGetVolumesCount(pool)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no extended partition found and no primary partition available")); return -1; diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index 92038c1..90f31b0 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -386,8 +386,7 @@ virStorageBackendGlusterRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED, if (okay < 0) goto cleanup; - if (vol && VIR_APPEND_ELEMENT(pool->volumes.objs, pool->volumes.count, - vol) < 0) + if (vol && virStoragePoolObjAddVol(pool, vol) < 0) goto cleanup; } if (errno) { diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 67f70e5..7bfe211 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -356,9 +356,9 @@ virStorageBackendLogicalMakeVol(char **const groups, if (virStorageBackendLogicalParseVolExtents(vol, groups) < 0) goto cleanup; - if (is_new_vol && - VIR_APPEND_ELEMENT(pool->volumes.objs, pool->volumes.count, vol) < 0) + if (is_new_vol && virStoragePoolObjAddVol(pool, vol) < 0) goto cleanup; + vol = NULL; ret = 0; diff --git a/src/storage/storage_backend_mpath.c b/src/storage/storage_backend_mpath.c index 434a477..46818ef 100644 --- a/src/storage/storage_backend_mpath.c +++ b/src/storage/storage_backend_mpath.c @@ -71,8 +71,9 @@ virStorageBackendMpathNewVol(virStoragePoolObjPtr pool, if (VIR_STRDUP(vol->key, vol->target.path) < 0) goto cleanup; - if (VIR_APPEND_ELEMENT_COPY(pool->volumes.objs, pool->volumes.count, vol) < 0) + if (virStoragePoolObjAddVol(pool, vol) < 0) goto cleanup; + pool->def->capacity += vol->target.capacity; pool->def->allocation += vol->target.allocation; ret = 0; diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index 7b8887b..6731677 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -506,7 +506,7 @@ virStorageBackendRBDRefreshPool(virConnectPtr conn, goto cleanup; } - if (VIR_APPEND_ELEMENT(pool->volumes.objs, pool->volumes.count, vol) < 0) { + if (virStoragePoolObjAddVol(pool, vol) < 0) { virStorageVolDefFree(vol); virStoragePoolObjClearVols(pool); goto cleanup; @@ -514,7 +514,7 @@ virStorageBackendRBDRefreshPool(virConnectPtr conn, } VIR_DEBUG("Found %zu images in RBD pool %s", - pool->volumes.count, pool->def->source.name); + virStoragePoolObjGetVolumesCount(pool), pool->def->source.name); ret = 0; diff --git a/src/storage/storage_backend_sheepdog.c b/src/storage/storage_backend_sheepdog.c index b55d96a..e72dcda 100644 --- a/src/storage/storage_backend_sheepdog.c +++ b/src/storage/storage_backend_sheepdog.c @@ -130,11 +130,9 @@ virStorageBackendSheepdogAddVolume(virConnectPtr conn ATTRIBUTE_UNUSED, if (virStorageBackendSheepdogRefreshVol(conn, pool, vol) < 0) goto error; - if (VIR_EXPAND_N(pool->volumes.objs, pool->volumes.count, 1) < 0) + if (virStoragePoolObjAddVol(pool, vol) < 0) goto error; - pool->volumes.objs[pool->volumes.count - 1] = vol; - return 0; error: diff --git a/src/storage/storage_backend_zfs.c b/src/storage/storage_backend_zfs.c index c6dae71..c266281 100644 --- a/src/storage/storage_backend_zfs.c +++ b/src/storage/storage_backend_zfs.c @@ -161,11 +161,9 @@ virStorageBackendZFSParseVol(virStoragePoolObjPtr pool, if (volume->target.allocation < volume->target.capacity) volume->target.sparse = true; - if (is_new_vol && - VIR_APPEND_ELEMENT(pool->volumes.objs, - pool->volumes.count, - volume) < 0) + if (is_new_vol && virStoragePoolObjAddVol(pool, volume) < 0) goto cleanup; + volume = NULL; ret = 0; cleanup: diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 8552120..b780f9a 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1626,25 +1626,6 @@ storagePoolLookupByTargetPath(virConnectPtr conn, } -static void -storageVolRemoveFromPool(virStoragePoolObjPtr obj, - virStorageVolDefPtr voldef) -{ - size_t i; - - for (i = 0; i < obj->volumes.count; i++) { - if (obj->volumes.objs[i] == voldef) { - VIR_INFO("Deleting volume '%s' from storage pool '%s'", - voldef->name, obj->def->name); - virStorageVolDefFree(voldef); - - VIR_DELETE_ELEMENT(obj->volumes.objs, i, obj->volumes.count); - break; - } - } -} - - static int storageVolDeleteInternal(virStorageVolPtr vol, virStorageBackendPtr backend, @@ -1676,7 +1657,7 @@ storageVolDeleteInternal(virStorageVolPtr vol, } } - storageVolRemoveFromPool(obj, voldef); + virStoragePoolObjRemoveVol(obj, voldef); ret = 0; cleanup: @@ -1815,24 +1796,19 @@ storageVolCreateXML(virStoragePoolPtr pool, goto cleanup; } - if (VIR_REALLOC_N(obj->volumes.objs, - obj->volumes.count + 1) < 0) - goto cleanup; - /* Wipe any key the user may have suggested, as volume creation * will generate the canonical key. */ VIR_FREE(voldef->key); if (backend->createVol(pool->conn, obj, voldef) < 0) goto cleanup; - obj->volumes.objs[obj->volumes.count++] = voldef; - newvol = virGetStorageVol(pool->conn, obj->def->name, voldef->name, - voldef->key, NULL, NULL); - if (!newvol) { - obj->volumes.count--; + if (!(newvol = virGetStorageVol(pool->conn, obj->def->name, voldef->name, + voldef->key, NULL, NULL))) goto cleanup; - } + /* NB: Upon success voldef "owned" by storage pool for deletion purposes */ + if (virStoragePoolObjAddVol(obj, voldef) < 0) + goto cleanup; if (backend->buildVol) { int buildret; @@ -1867,7 +1843,7 @@ storageVolCreateXML(virStoragePoolPtr pool, if (buildret < 0) { /* buildVol handles deleting volume on failure */ - storageVolRemoveFromPool(obj, voldef); + virStoragePoolObjRemoveVol(obj, voldef); voldef = NULL; goto cleanup; } @@ -2018,9 +1994,6 @@ storageVolCreateXMLFrom(virStoragePoolPtr pool, backend->refreshVol(pool->conn, obj, voldefsrc) < 0) goto cleanup; - if (VIR_REALLOC_N(obj->volumes.objs, obj->volumes.count + 1) < 0) - goto cleanup; - /* 'Define' the new volume so we get async progress reporting. * Wipe any key the user may have suggested, as volume creation * will generate the canonical key. */ @@ -2037,13 +2010,13 @@ storageVolCreateXMLFrom(virStoragePoolPtr pool, memcpy(shadowvol, voldef, sizeof(*voldef)); - obj->volumes.objs[obj->volumes.count++] = voldef; - newvol = virGetStorageVol(pool->conn, obj->def->name, voldef->name, - voldef->key, NULL, NULL); - if (!newvol) { - obj->volumes.count--; + if (!(newvol = virGetStorageVol(pool->conn, obj->def->name, voldef->name, + voldef->key, NULL, NULL))) + goto cleanup; + + /* NB: Upon success voldef "owned" by storage pool for deletion purposes */ + if (virStoragePoolObjAddVol(obj, voldef) < 0) goto cleanup; - } /* Drop the pool lock during volume allocation */ obj->asyncjobs++; diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index e1fe162..3efa181 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -3597,13 +3597,13 @@ virStorageBackendRefreshLocal(virConnectPtr conn ATTRIBUTE_UNUSED, * An error message was raised, but we just continue. */ } - if (VIR_APPEND_ELEMENT(pool->volumes.objs, pool->volumes.count, vol) < 0) + if (virStoragePoolObjAddVol(pool, vol) < 0) goto cleanup; + vol = NULL; } if (direrr < 0) goto cleanup; VIR_DIR_CLOSE(dir); - vol = NULL; if (VIR_ALLOC(target)) goto cleanup; @@ -3785,10 +3785,10 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool, pool->def->capacity += vol->target.capacity; pool->def->allocation += vol->target.allocation; - if (VIR_APPEND_ELEMENT(pool->volumes.objs, pool->volumes.count, vol) < 0) + if (virStoragePoolObjAddVol(pool, vol) < 0) goto cleanup; - vol = NULL; + retval = 0; cleanup: diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 989c3a8..67b115f 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1079,7 +1079,8 @@ testOpenVolumesForPool(const char *file, if (!def->key && VIR_STRDUP(def->key, def->target.path) < 0) goto error; - if (VIR_APPEND_ELEMENT_COPY(obj->volumes.objs, obj->volumes.count, def) < 0) + + if (virStoragePoolObjAddVol(obj, def) < 0) goto error; obj->def->allocation += def->target.allocation; @@ -4995,8 +4996,7 @@ testStorageVolCreateXML(virStoragePoolPtr pool, goto cleanup; if (VIR_STRDUP(privvol->key, privvol->target.path) < 0 || - VIR_APPEND_ELEMENT_COPY(obj->volumes.objs, - obj->volumes.count, privvol) < 0) + virStoragePoolObjAddVol(obj, privvol) < 0) goto cleanup; obj->def->allocation += privvol->target.allocation; @@ -5063,8 +5063,7 @@ testStorageVolCreateXMLFrom(virStoragePoolPtr pool, goto cleanup; if (VIR_STRDUP(privvol->key, privvol->target.path) < 0 || - VIR_APPEND_ELEMENT_COPY(obj->volumes.objs, - obj->volumes.count, privvol) < 0) + virStoragePoolObjAddVol(obj, privvol) < 0) goto cleanup; obj->def->allocation += privvol->target.allocation; @@ -5089,7 +5088,6 @@ testStorageVolDelete(virStorageVolPtr vol, testDriverPtr privconn = vol->conn->privateData; virStoragePoolObjPtr obj; virStorageVolDefPtr privvol; - size_t i; int ret = -1; virCheckFlags(0, -1); @@ -5103,14 +5101,8 @@ testStorageVolDelete(virStorageVolPtr vol, obj->def->allocation -= privvol->target.allocation; obj->def->available = (obj->def->capacity - obj->def->allocation); - for (i = 0; i < obj->volumes.count; i++) { - if (obj->volumes.objs[i] == privvol) { - virStorageVolDefFree(privvol); + virStoragePoolObjRemoveVol(obj, privvol); - VIR_DELETE_ELEMENT(obj->volumes.objs, i, obj->volumes.count); - break; - } - } ret = 0; cleanup: -- 2.9.5
Introduce virStoragePoolObjForEachVolume to scan each volume calling the passed callback function until all volumes have been processed in the storage pool volume list, unless the callback function returns an error. Introduce virStoragePoolObjSearchVolume to search each volume calling the passed callback function until it returns true indicating that the desired volume was found. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virstorageobj.c | 32 ++++++++++++++ src/conf/virstorageobj.h | 18 ++++++++ src/libvirt_private.syms | 2 + src/storage/storage_backend_disk.c | 90 +++++++++++++++++++++++--------------- 4 files changed, 107 insertions(+), 35 deletions(-) diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index 912c27a..8ee40e2 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -319,6 +319,38 @@ virStoragePoolObjGetVolumesCount(virStoragePoolObjPtr obj) } +int +virStoragePoolObjForEachVolume(virStoragePoolObjPtr obj, + virStorageVolObjListIterator iter, + const void *opaque) +{ + size_t i; + + for (i = 0; i < obj->volumes.count; i++) { + if (iter(obj->volumes.objs[i], opaque) < 0) + return -1; + } + + return 0; +} + + +virStorageVolDefPtr +virStoragePoolObjSearchVolume(virStoragePoolObjPtr obj, + virStorageVolObjListSearcher iter, + const void *opaque) +{ + size_t i; + + for (i = 0; i < obj->volumes.count; i++) { + if (iter(obj->volumes.objs[i], opaque)) + return obj->volumes.objs[i]; + } + + return NULL; +} + + virStorageVolDefPtr virStorageVolDefFindByKey(virStoragePoolObjPtr obj, const char *key) diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h index d1a1247..c2f3f23 100644 --- a/src/conf/virstorageobj.h +++ b/src/conf/virstorageobj.h @@ -147,6 +147,24 @@ virStoragePoolObjRemoveVol(virStoragePoolObjPtr obj, size_t virStoragePoolObjGetVolumesCount(virStoragePoolObjPtr obj); +typedef int +(*virStorageVolObjListIterator)(virStorageVolDefPtr voldef, + const void *opaque); + +int +virStoragePoolObjForEachVolume(virStoragePoolObjPtr obj, + virStorageVolObjListIterator iter, + const void *opaque); + +typedef bool +(*virStorageVolObjListSearcher)(virStorageVolDefPtr voldef, + const void *opaque); + +virStorageVolDefPtr +virStoragePoolObjSearchVolume(virStoragePoolObjPtr obj, + virStorageVolObjListSearcher iter, + const void *opaque); + virStorageVolDefPtr virStorageVolDefFindByKey(virStoragePoolObjPtr obj, const char *key); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a35299b..67b531f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1057,6 +1057,7 @@ virStoragePoolObjDefUseNewDef; virStoragePoolObjDeleteDef; virStoragePoolObjFindByName; virStoragePoolObjFindByUUID; +virStoragePoolObjForEachVolume; virStoragePoolObjGetAsyncjobs; virStoragePoolObjGetAutostartLink; virStoragePoolObjGetConfigFile; @@ -1079,6 +1080,7 @@ virStoragePoolObjNumOfVolumes; virStoragePoolObjRemove; virStoragePoolObjRemoveVol; virStoragePoolObjSaveDef; +virStoragePoolObjSearchVolume; virStoragePoolObjSetActive; virStoragePoolObjSetAutostart; virStoragePoolObjSetConfigFile; diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index 0bf5567..dfe9938 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -43,6 +43,17 @@ VIR_LOG_INIT("storage.storage_backend_disk"); #define SECTOR_SIZE 512 +static bool +virStorageVolPartFindExtended(virStorageVolDefPtr def, + const void *opaque ATTRIBUTE_UNUSED) +{ + if (def->source.partType == VIR_STORAGE_VOL_DISK_TYPE_EXTENDED) + return true; + + return false; +} + + static int virStorageBackendDiskMakeDataVol(virStoragePoolObjPtr pool, char **const groups, @@ -191,16 +202,13 @@ virStorageBackendDiskMakeDataVol(virStoragePoolObjPtr pool, /* Find the extended partition and increase the allocation value */ if (vol->source.partType == VIR_STORAGE_VOL_DISK_TYPE_LOGICAL) { - size_t i; + virStorageVolDefPtr voldef; - for (i = 0; i < pool->volumes.count; i++) { - if (pool->volumes.objs[i]->source.partType == - VIR_STORAGE_VOL_DISK_TYPE_EXTENDED) { - pool->volumes.objs[i]->target.allocation += - vol->target.allocation; - break; - } - } + voldef = virStoragePoolObjSearchVolume(pool, + virStorageVolPartFindExtended, + NULL); + if (voldef) + voldef->target.allocation += vol->target.allocation; } if (STRNEQ(groups[2], "metadata")) @@ -521,6 +529,25 @@ virStorageBackendDiskBuildPool(virConnectPtr conn ATTRIBUTE_UNUSED, return ret; } + +struct virStorageVolNumData { + int count; +}; + +static int +virStorageVolNumOfPartTypes(virStorageVolDefPtr def, + const void *opaque) +{ + struct virStorageVolNumData *data = (struct virStorageVolNumData *)opaque; + + if (def->source.partType == VIR_STORAGE_VOL_DISK_TYPE_PRIMARY || + def->source.partType == VIR_STORAGE_VOL_DISK_TYPE_EXTENDED) + data->count++; + + return 0; +} + + /** * Decides what kind of partition type that should be created. * Important when the partition table is of msdos type @@ -528,19 +555,16 @@ virStorageBackendDiskBuildPool(virConnectPtr conn ATTRIBUTE_UNUSED, static int virStorageBackendDiskPartTypeToCreate(virStoragePoolObjPtr pool) { + struct virStorageVolNumData data = { .count = 0 }; + if (pool->def->source.format == VIR_STORAGE_POOL_DISK_DOS) { /* count primary and extended partitions, can't be more than 3 to create a new primary partition */ - size_t i; - int count = 0; - for (i = 0; i < pool->volumes.count; i++) { - int partType = pool->volumes.objs[i]->source.partType; - if (partType == VIR_STORAGE_VOL_DISK_TYPE_PRIMARY || - partType == VIR_STORAGE_VOL_DISK_TYPE_EXTENDED) - count++; + if (virStoragePoolObjForEachVolume(pool, virStorageVolNumOfPartTypes, + &data) == 0) { + if (data.count >= 4) + return VIR_STORAGE_VOL_DISK_TYPE_LOGICAL; } - if (count >= 4) - return VIR_STORAGE_VOL_DISK_TYPE_LOGICAL; } /* for all other cases, all partitions are primary */ @@ -552,7 +576,6 @@ virStorageBackendDiskPartFormat(virStoragePoolObjPtr pool, virStorageVolDefPtr vol, char** partFormat) { - size_t i; if (pool->def->source.format == VIR_STORAGE_POOL_DISK_DOS) { const char *partedFormat; partedFormat = virStoragePartedFsTypeToString(vol->target.format); @@ -563,13 +586,12 @@ virStorageBackendDiskPartFormat(virStoragePoolObjPtr pool, } if (vol->target.format == VIR_STORAGE_VOL_DISK_EXTENDED) { /* make sure we don't have an extended partition already */ - for (i = 0; i < pool->volumes.count; i++) { - if (pool->volumes.objs[i]->source.partType == - VIR_STORAGE_VOL_DISK_TYPE_EXTENDED) { + if (virStoragePoolObjSearchVolume(pool, + virStorageVolPartFindExtended, + NULL)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("extended partition already exists")); return -1; - } } if (VIR_STRDUP(*partFormat, partedFormat) < 0) return -1; @@ -585,18 +607,16 @@ virStorageBackendDiskPartFormat(virStoragePoolObjPtr pool, break; case VIR_STORAGE_VOL_DISK_TYPE_LOGICAL: /* make sure we have an extended partition */ - for (i = 0; i < pool->volumes.count; i++) { - if (pool->volumes.objs[i]->source.partType == - VIR_STORAGE_VOL_DISK_TYPE_EXTENDED) { - if (virAsprintf(partFormat, "logical %s", - partedFormat) < 0) - return -1; - break; - } - } - if (i == virStoragePoolObjGetVolumesCount(pool)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("no extended partition found and no primary partition available")); + if (virStoragePoolObjSearchVolume(pool, + virStorageVolPartFindExtended, + NULL)) { + if (virAsprintf(partFormat, "logical %s", + partedFormat) < 0) + return -1; + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("no extended partition found and no " + "primary partition available")); return -1; } break; -- 2.9.5
Use the new accessor APIs for storage_driver and test_driver. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_scsi.c | 4 +++- src/storage/storage_driver.c | 14 +++++++------- src/test/test_driver.c | 20 ++++++++++++-------- 3 files changed, 22 insertions(+), 16 deletions(-) diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index 575e6a6..02fd4b6 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -443,8 +443,10 @@ static int virStorageBackendSCSIStartPool(virConnectPtr conn, virStoragePoolObjPtr pool) { + const char *configFile = virStoragePoolObjGetConfigFile(pool); + if (pool->def->source.adapter.type == VIR_STORAGE_ADAPTER_TYPE_FC_HOST) - return createVport(conn, pool->def, pool->configFile, + return createVport(conn, pool->def, configFile, &pool->def->source.adapter.data.fchost); return 0; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index b780f9a..7b1396f 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -91,7 +91,7 @@ virStoragePoolUpdateInactive(virStoragePoolObjPtr *objptr) { virStoragePoolObjPtr obj = *objptr; - if (obj->configFile == NULL) { + if (!virStoragePoolObjGetConfigFile(obj)) { virStoragePoolObjRemove(&driver->pools, obj); *objptr = NULL; } else if (obj->newDef) { @@ -643,7 +643,7 @@ storagePoolIsPersistent(virStoragePoolPtr pool) if (virStoragePoolIsPersistentEnsureACL(pool->conn, obj->def) < 0) goto cleanup; - ret = obj->configFile ? 1 : 0; + ret = virStoragePoolObjGetConfigFile(obj) ? 1 : 0; cleanup: virStoragePoolObjUnlock(obj); @@ -849,7 +849,6 @@ storagePoolUndefine(virStoragePoolPtr pool) obj->autostartLink, virStrerror(errno, ebuf, sizeof(ebuf))); } - VIR_FREE(obj->configFile); VIR_FREE(obj->autostartLink); event = virStoragePoolEventLifecycleNew(obj->def->name, @@ -1250,7 +1249,7 @@ storagePoolGetAutostart(virStoragePoolPtr pool, if (virStoragePoolGetAutostartEnsureACL(pool->conn, obj->def) < 0) goto cleanup; - if (!obj->configFile) { + if (!virStoragePoolObjGetConfigFile(obj)) { *autostart = 0; } else { *autostart = obj->autostart; @@ -1268,6 +1267,7 @@ storagePoolSetAutostart(virStoragePoolPtr pool, int autostart) { virStoragePoolObjPtr obj; + const char *configFile; int ret = -1; storageDriverLock(); @@ -1277,7 +1277,7 @@ storagePoolSetAutostart(virStoragePoolPtr pool, if (virStoragePoolSetAutostartEnsureACL(pool->conn, obj->def) < 0) goto cleanup; - if (!obj->configFile) { + if (!(configFile = virStoragePoolObjGetConfigFile(obj))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("pool has no config file")); goto cleanup; @@ -1294,10 +1294,10 @@ storagePoolSetAutostart(virStoragePoolPtr pool, goto cleanup; } - if (symlink(obj->configFile, obj->autostartLink) < 0) { + if (symlink(configFile, obj->autostartLink) < 0) { virReportSystemError(errno, _("Failed to create symlink '%s' to '%s'"), - obj->autostartLink, obj->configFile); + obj->autostartLink, configFile); goto cleanup; } } else { diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 67b115f..36e5ba1 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -4056,12 +4056,17 @@ testInterfaceDestroy(virInterfacePtr iface, static int testStoragePoolObjSetDefaults(virStoragePoolObjPtr obj) { + char *configFile; obj->def->capacity = defaultPoolCap; obj->def->allocation = defaultPoolAlloc; obj->def->available = defaultPoolCap - defaultPoolAlloc; - return VIR_STRDUP(obj->configFile, ""); + if (VIR_STRDUP(configFile, "") < 0) + return -1; + + virStoragePoolObjSetConfigFile(obj, configFile); + return 0; } @@ -4303,7 +4308,7 @@ testStoragePoolIsPersistent(virStoragePoolPtr pool) if (!(obj = testStoragePoolObjFindByUUID(privconn, pool->uuid))) return -1; - ret = obj->configFile ? 1 : 0; + ret = virStoragePoolObjGetConfigFile(obj) ? 1 : 0; virStoragePoolObjUnlock(obj); return ret; @@ -4469,7 +4474,7 @@ testStoragePoolCreateXML(virConnectPtr conn, /* *SetDefaults fills this in for the persistent pools, but this * would be a transient pool so remove it; otherwise, the Destroy * code will not Remove the pool */ - VIR_FREE(obj->configFile); + virStoragePoolObjSetConfigFile(obj, NULL); obj->active = true; @@ -4634,7 +4639,7 @@ testStoragePoolDestroy(virStoragePoolPtr pool) VIR_STORAGE_POOL_EVENT_STOPPED, 0); - if (obj->configFile == NULL) { + if (!(virStoragePoolObjGetConfigFile(obj))) { virStoragePoolObjRemove(&privconn->pools, obj); obj = NULL; } @@ -4740,11 +4745,10 @@ testStoragePoolGetAutostart(virStoragePoolPtr pool, if (!(obj = testStoragePoolObjFindByName(privconn, pool->name))) return -1; - if (!obj->configFile) { + if (!virStoragePoolObjGetConfigFile(obj)) *autostart = 0; - } else { + else *autostart = obj->autostart; - } virStoragePoolObjUnlock(obj); return 0; @@ -4762,7 +4766,7 @@ testStoragePoolSetAutostart(virStoragePoolPtr pool, if (!(obj = testStoragePoolObjFindByName(privconn, pool->name))) return -1; - if (!obj->configFile) { + if (!virStoragePoolObjGetConfigFile(obj)) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("pool has no config file")); goto cleanup; -- 2.9.5
Use the new accessor API for storage_driver. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_driver.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 7b1396f..a7a77ba 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -814,6 +814,7 @@ static int storagePoolUndefine(virStoragePoolPtr pool) { virStoragePoolObjPtr obj; + const char *autostartLink; virObjectEventPtr event = NULL; int ret = -1; @@ -838,18 +839,17 @@ storagePoolUndefine(virStoragePoolPtr pool) goto cleanup; } + autostartLink = virStoragePoolObjGetAutostartLink(obj); if (virStoragePoolObjDeleteDef(obj) < 0) goto cleanup; - if (unlink(obj->autostartLink) < 0 && - errno != ENOENT && - errno != ENOTDIR) { + if (autostartLink && unlink(autostartLink) < 0 && + errno != ENOENT && errno != ENOTDIR) { char ebuf[1024]; VIR_ERROR(_("Failed to delete autostart link '%s': %s"), - obj->autostartLink, virStrerror(errno, ebuf, sizeof(ebuf))); + autostartLink, virStrerror(errno, ebuf, sizeof(ebuf))); } - VIR_FREE(obj->autostartLink); event = virStoragePoolEventLifecycleNew(obj->def->name, obj->def->uuid, @@ -1268,6 +1268,7 @@ storagePoolSetAutostart(virStoragePoolPtr pool, { virStoragePoolObjPtr obj; const char *configFile; + const char *autostartLink; int ret = -1; storageDriverLock(); @@ -1283,6 +1284,8 @@ storagePoolSetAutostart(virStoragePoolPtr pool, goto cleanup; } + autostartLink = virStoragePoolObjGetAutostartLink(obj); + autostart = (autostart != 0); if (obj->autostart != autostart) { @@ -1294,18 +1297,18 @@ storagePoolSetAutostart(virStoragePoolPtr pool, goto cleanup; } - if (symlink(configFile, obj->autostartLink) < 0) { + if (symlink(configFile, autostartLink) < 0) { virReportSystemError(errno, _("Failed to create symlink '%s' to '%s'"), - obj->autostartLink, configFile); + autostartLink, configFile); goto cleanup; } } else { - if (unlink(obj->autostartLink) < 0 && + if (autostartLink && unlink(autostartLink) < 0 && errno != ENOENT && errno != ENOTDIR) { virReportSystemError(errno, _("Failed to delete symlink '%s'"), - obj->autostartLink); + autostartLink); goto cleanup; } } -- 2.9.5
On 08/24/2017 03:09 PM, John Ferlan wrote:
Use the new accessor API for storage_driver.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_driver.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 7b1396f..a7a77ba 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -814,6 +814,7 @@ static int storagePoolUndefine(virStoragePoolPtr pool) { virStoragePoolObjPtr obj; + const char *autostartLink; virObjectEventPtr event = NULL; int ret = -1;
@@ -838,18 +839,17 @@ storagePoolUndefine(virStoragePoolPtr pool) goto cleanup; }
+ autostartLink = virStoragePoolObjGetAutostartLink(obj);
virStoragePoolObjGetAutostartLink() should look a bit different. Now it's returning char * which suggests that caller is supposed to free the retval. But in fact they shouldn't. const char * would be better. But lets save that for a follow up patch. Michal
Use the new accessor APIs for storage_driver, test_driver, and gluster backend. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_gluster.c | 2 +- src/storage/storage_driver.c | 16 ++++++++-------- src/test/test_driver.c | 10 +++++----- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index 90f31b0..05e7bff 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -544,7 +544,7 @@ virStorageBackendGlusterCheckPool(virStoragePoolObjPtr pool, /* Return previous state remembered by the status XML. If the pool is not * available we will fail to refresh it and end up in the same situation. * This will save one attempt to open the connection to the remote server */ - *active = pool->active; + *active = virStoragePoolObjIsActive(pool); return 0; } diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index a7a77ba..89eaee9 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -145,9 +145,9 @@ storagePoolUpdateState(virStoragePoolObjPtr obj) } } - obj->active = active; + virStoragePoolObjSetActive(obj, active); - if (!obj->active) + if (!virStoragePoolObjIsActive(obj)) virStoragePoolUpdateInactive(&obj); cleanup: @@ -226,7 +226,7 @@ storageDriverAutostart(void) _("Failed to autostart storage pool '%s': %s"), obj->def->name, virGetLastErrorMessage()); } else { - obj->active = true; + virStoragePoolObjSetActive(obj, true); } VIR_FREE(stateFile); } @@ -735,7 +735,7 @@ storagePoolCreateXML(virConnectPtr conn, 0); VIR_INFO("Creating storage pool '%s'", obj->def->name); - obj->active = true; + virStoragePoolObjSetActive(obj, true); pool = virGetStoragePool(conn, obj->def->name, obj->def->uuid, NULL, NULL); @@ -940,7 +940,7 @@ storagePoolCreate(virStoragePoolPtr pool, VIR_STORAGE_POOL_EVENT_STARTED, 0); - obj->active = true; + virStoragePoolObjSetActive(obj, true); ret = 0; cleanup: @@ -1040,7 +1040,7 @@ storagePoolDestroy(virStoragePoolPtr pool) VIR_STORAGE_POOL_EVENT_STOPPED, 0); - obj->active = false; + virStoragePoolObjSetActive(obj, false); virStoragePoolUpdateInactive(&obj); @@ -1156,7 +1156,7 @@ storagePoolRefresh(virStoragePoolPtr pool, obj->def->uuid, VIR_STORAGE_POOL_EVENT_STOPPED, 0); - obj->active = false; + virStoragePoolObjSetActive(obj, false); virStoragePoolUpdateInactive(&obj); @@ -1194,7 +1194,7 @@ storagePoolGetInfo(virStoragePoolPtr pool, goto cleanup; memset(info, 0, sizeof(virStoragePoolInfo)); - if (obj->active) + if (virStoragePoolObjIsActive(obj)) info->state = VIR_STORAGE_POOL_RUNNING; else info->state = VIR_STORAGE_POOL_INACTIVE; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 36e5ba1..33ce9e4 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1131,7 +1131,7 @@ testParseStorage(testDriverPtr privconn, virStoragePoolObjUnlock(obj); goto error; } - obj->active = true; + virStoragePoolObjSetActive(obj, true); /* Find storage volumes */ if (testOpenVolumesForPool(file, ctxt, obj, i+1) < 0) { @@ -4328,7 +4328,7 @@ testStoragePoolCreate(virStoragePoolPtr pool, if (!(obj = testStoragePoolObjFindInactiveByName(privconn, pool->name))) return -1; - obj->active = true; + virStoragePoolObjSetActive(obj, true); event = virStoragePoolEventLifecycleNew(pool->name, pool->uuid, VIR_STORAGE_POOL_EVENT_STARTED, @@ -4476,7 +4476,7 @@ testStoragePoolCreateXML(virConnectPtr conn, * code will not Remove the pool */ virStoragePoolObjSetConfigFile(obj, NULL); - obj->active = true; + virStoragePoolObjSetActive(obj, true); event = virStoragePoolEventLifecycleNew(obj->def->name, obj->def->uuid, VIR_STORAGE_POOL_EVENT_STARTED, @@ -4624,7 +4624,7 @@ testStoragePoolDestroy(virStoragePoolPtr pool) if (!(obj = testStoragePoolObjFindActiveByName(privconn, pool->name))) return -1; - obj->active = false; + virStoragePoolObjSetActive(obj, false); if (obj->def->source.adapter.type == VIR_STORAGE_ADAPTER_TYPE_FC_HOST) { @@ -4702,7 +4702,7 @@ testStoragePoolGetInfo(virStoragePoolPtr pool, return -1; memset(info, 0, sizeof(virStoragePoolInfo)); - if (obj->active) + if (virStoragePoolObjIsActive(obj)) info->state = VIR_STORAGE_POOL_RUNNING; else info->state = VIR_STORAGE_POOL_INACTIVE; -- 2.9.5
Use the new accessor APIs for storage_driver and test_driver. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_driver.c | 20 +++++++++----------- src/test/test_driver.c | 4 ++-- 2 files changed, 11 insertions(+), 13 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 89eaee9..c9156e6 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -196,7 +196,7 @@ storageDriverAutostart(void) continue; } - if (obj->autostart && + if (virStoragePoolObjIsAutostart(obj) && !virStoragePoolObjIsActive(obj)) { if (backend->startPool && backend->startPool(conn, obj) < 0) { @@ -1249,11 +1249,7 @@ storagePoolGetAutostart(virStoragePoolPtr pool, if (virStoragePoolGetAutostartEnsureACL(pool->conn, obj->def) < 0) goto cleanup; - if (!virStoragePoolObjGetConfigFile(obj)) { - *autostart = 0; - } else { - *autostart = obj->autostart; - } + *autostart = virStoragePoolObjIsAutostart(obj) ? 1 : 0; ret = 0; @@ -1269,6 +1265,8 @@ storagePoolSetAutostart(virStoragePoolPtr pool, virStoragePoolObjPtr obj; const char *configFile; const char *autostartLink; + bool new_autostart; + bool cur_autostart; int ret = -1; storageDriverLock(); @@ -1286,10 +1284,10 @@ storagePoolSetAutostart(virStoragePoolPtr pool, autostartLink = virStoragePoolObjGetAutostartLink(obj); - autostart = (autostart != 0); - - if (obj->autostart != autostart) { - if (autostart) { + new_autostart = (autostart != 0); + cur_autostart = virStoragePoolObjIsAutostart(obj); + if (cur_autostart != new_autostart) { + if (new_autostart) { if (virFileMakePath(driver->autostartDir) < 0) { virReportSystemError(errno, _("cannot create autostart directory %s"), @@ -1312,7 +1310,7 @@ storagePoolSetAutostart(virStoragePoolPtr pool, goto cleanup; } } - obj->autostart = autostart; + virStoragePoolObjSetAutostart(obj, autostart); } ret = 0; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 33ce9e4..b793187 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -4748,7 +4748,7 @@ testStoragePoolGetAutostart(virStoragePoolPtr pool, if (!virStoragePoolObjGetConfigFile(obj)) *autostart = 0; else - *autostart = obj->autostart; + *autostart = virStoragePoolObjIsAutostart(obj) ? 1 : 0; virStoragePoolObjUnlock(obj); return 0; @@ -4773,7 +4773,7 @@ testStoragePoolSetAutostart(virStoragePoolPtr pool, } autostart = (autostart != 0); - obj->autostart = autostart; + virStoragePoolObjSetAutostart(obj, autostart); ret = 0; cleanup: -- 2.9.5
Since it's been used that way anyway, let's just convert it to a bool and only make the external representation be an int. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virstorageobj.c | 4 ++-- src/conf/virstorageobj.h | 4 ++-- src/storage/storage_driver.c | 2 +- src/test/test_driver.c | 4 ++-- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index 8ee40e2..1364bdd 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -133,13 +133,13 @@ virStoragePoolObjIsAutostart(virStoragePoolObjPtr obj) if (!obj->configFile) return 0; - return obj->autostart == 1; + return obj->autostart; } void virStoragePoolObjSetAutostart(virStoragePoolObjPtr obj, - int autostart) + bool autostart) { obj->autostart = autostart; } diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h index c2f3f23..b65b160 100644 --- a/src/conf/virstorageobj.h +++ b/src/conf/virstorageobj.h @@ -33,7 +33,7 @@ struct _virStoragePoolObj { char *configFile; char *autostartLink; bool active; - int autostart; + bool autostart; unsigned int asyncjobs; virStoragePoolDefPtr def; @@ -108,7 +108,7 @@ virStoragePoolObjIsAutostart(virStoragePoolObjPtr obj); void virStoragePoolObjSetAutostart(virStoragePoolObjPtr obj, - int autostart); + bool autostart); unsigned int virStoragePoolObjGetAsyncjobs(virStoragePoolObjPtr obj); diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index c9156e6..6fae6f4 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1310,7 +1310,7 @@ storagePoolSetAutostart(virStoragePoolPtr pool, goto cleanup; } } - virStoragePoolObjSetAutostart(obj, autostart); + virStoragePoolObjSetAutostart(obj, new_autostart); } ret = 0; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index b793187..69d61b5 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -4761,6 +4761,7 @@ testStoragePoolSetAutostart(virStoragePoolPtr pool, { testDriverPtr privconn = pool->conn->privateData; virStoragePoolObjPtr obj; + bool new_autostart = (autostart != 0); int ret = -1; if (!(obj = testStoragePoolObjFindByName(privconn, pool->name))) @@ -4772,8 +4773,7 @@ testStoragePoolSetAutostart(virStoragePoolPtr pool, goto cleanup; } - autostart = (autostart != 0); - virStoragePoolObjSetAutostart(obj, autostart); + virStoragePoolObjSetAutostart(obj, new_autostart); ret = 0; cleanup: -- 2.9.5
On 08/24/2017 03:09 PM, John Ferlan wrote:
Since it's been used that way anyway, let's just convert it to a bool and only make the external representation be an int.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virstorageobj.c | 4 ++-- src/conf/virstorageobj.h | 4 ++-- src/storage/storage_driver.c | 2 +- src/test/test_driver.c | 4 ++-- 4 files changed, 7 insertions(+), 7 deletions(-)
I'm no native speaker but probably s/to bool/as bool/ in $SUBJ? Michal
Use the new accessor APIs for storage_driver. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_driver.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 6fae6f4..83d9ce3 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -832,7 +832,7 @@ storagePoolUndefine(virStoragePoolPtr pool) goto cleanup; } - if (obj->asyncjobs > 0) { + if (virStoragePoolObjGetAsyncjobs(obj) > 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("pool '%s' has asynchronous jobs running."), obj->def->name); @@ -1014,7 +1014,7 @@ storagePoolDestroy(virStoragePoolPtr pool) goto cleanup; } - if (obj->asyncjobs > 0) { + if (virStoragePoolObjGetAsyncjobs(obj) > 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("pool '%s' has asynchronous jobs running."), obj->def->name); @@ -1082,7 +1082,7 @@ storagePoolDelete(virStoragePoolPtr pool, goto cleanup; } - if (obj->asyncjobs > 0) { + if (virStoragePoolObjGetAsyncjobs(obj) > 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("pool '%s' has asynchronous jobs running."), obj->def->name); @@ -1140,7 +1140,7 @@ storagePoolRefresh(virStoragePoolPtr pool, goto cleanup; } - if (obj->asyncjobs > 0) { + if (virStoragePoolObjGetAsyncjobs(obj) > 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("pool '%s' has asynchronous jobs running."), obj->def->name); @@ -1827,7 +1827,7 @@ storageVolCreateXML(virStoragePoolPtr pool, memcpy(buildvoldef, voldef, sizeof(*voldef)); /* Drop the pool lock during volume allocation */ - obj->asyncjobs++; + virStoragePoolObjIncrAsyncjobs(obj); voldef->building = true; virStoragePoolObjUnlock(obj); @@ -1840,7 +1840,7 @@ storageVolCreateXML(virStoragePoolPtr pool, storageDriverUnlock(); voldef->building = false; - obj->asyncjobs--; + virStoragePoolObjDecrAsyncjobs(obj); if (buildret < 0) { /* buildVol handles deleting volume on failure */ @@ -2020,13 +2020,13 @@ storageVolCreateXMLFrom(virStoragePoolPtr pool, goto cleanup; /* Drop the pool lock during volume allocation */ - obj->asyncjobs++; + virStoragePoolObjIncrAsyncjobs(obj); voldef->building = true; voldefsrc->in_use++; virStoragePoolObjUnlock(obj); if (objsrc) { - objsrc->asyncjobs++; + virStoragePoolObjIncrAsyncjobs(objsrc); virStoragePoolObjUnlock(objsrc); } @@ -2040,10 +2040,10 @@ storageVolCreateXMLFrom(virStoragePoolPtr pool, voldefsrc->in_use--; voldef->building = false; - obj->asyncjobs--; + virStoragePoolObjDecrAsyncjobs(obj); if (objsrc) { - objsrc->asyncjobs--; + virStoragePoolObjDecrAsyncjobs(objsrc); virStoragePoolObjUnlock(objsrc); objsrc = NULL; } -- 2.9.5
On 08/24/2017 03:09 PM, John Ferlan wrote:
Use the new accessor APIs for storage_driver.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_driver.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)
s/Incr}Decr/Incr|Decr/ in $SUBJ. Michal
Use the new accessor API for storage_driver. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_driver.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 83d9ce3..90d4bf3 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -94,10 +94,8 @@ virStoragePoolUpdateInactive(virStoragePoolObjPtr *objptr) if (!virStoragePoolObjGetConfigFile(obj)) { virStoragePoolObjRemove(&driver->pools, obj); *objptr = NULL; - } else if (obj->newDef) { - virStoragePoolDefFree(obj->def); - obj->def = obj->newDef; - obj->newDef = NULL; + } else if (virStoragePoolObjGetNewDef(obj)) { + virStoragePoolObjDefUseNewDef(obj); } } -- 2.9.5
ping? Would be nice to make a bit more progress here. Tks, John On 08/24/2017 09:08 AM, John Ferlan wrote:
Consider this round 1 of 2.... The next series will be 18 patches, but the majority of those deal with change every {pool|obj}->def->X to use the accessor virStoragePoolObjGetDef.
v1: https://www.redhat.com/archives/libvir-list/2017-May/msg00218.html
Probably not even worth looking at the v1, but this picks up where v1 left off somewhere around patch 10, but adding smaller steps between patches.
John Ferlan (12): storage: Create accessor API's for virStoragePoolObj storage: Introduce virStoragePoolObjNew storage: Fill in storage pool @active properly storage: Introduce storage volume add, delete, count APIs storage: Introduce APIs to search/scan storage pool volumes list storage: Use virStoragePoolObj{Get|Set}ConfigFile storage: Use virStoragePoolObjGetAutostartLink storage: Use virStoragePoolObj{Is|Set}Active storage: Use virStoragePoolObj{Is|Set}Autostart storage: Internally represent @autostart to bool storage: Use virStoragePoolObj{Get|Incr}Decr}Asyncjobs storage: Use virStoragePoolObjDefUseNewDef
src/conf/virstorageobj.c | 211 +++++++++++++++++++++++++++++++-- src/conf/virstorageobj.h | 84 ++++++++++++- src/libvirt_private.syms | 20 ++++ src/storage/storage_backend_disk.c | 93 +++++++++------ src/storage/storage_backend_gluster.c | 5 +- src/storage/storage_backend_logical.c | 4 +- src/storage/storage_backend_mpath.c | 3 +- src/storage/storage_backend_rbd.c | 4 +- src/storage/storage_backend_scsi.c | 4 +- src/storage/storage_backend_sheepdog.c | 4 +- src/storage/storage_backend_zfs.c | 6 +- src/storage/storage_driver.c | 142 +++++++++------------- src/storage/storage_util.c | 8 +- src/test/test_driver.c | 54 ++++----- tests/storagevolxml2argvtest.c | 20 ++-- 15 files changed, 465 insertions(+), 197 deletions(-)
On 08/24/2017 03:08 PM, John Ferlan wrote:
Consider this round 1 of 2.... The next series will be 18 patches, but the majority of those deal with change every {pool|obj}->def->X to use the accessor virStoragePoolObjGetDef.
v1: https://www.redhat.com/archives/libvir-list/2017-May/msg00218.html
Probably not even worth looking at the v1, but this picks up where v1 left off somewhere around patch 10, but adding smaller steps between patches.
John Ferlan (12): storage: Create accessor API's for virStoragePoolObj storage: Introduce virStoragePoolObjNew storage: Fill in storage pool @active properly storage: Introduce storage volume add, delete, count APIs storage: Introduce APIs to search/scan storage pool volumes list storage: Use virStoragePoolObj{Get|Set}ConfigFile storage: Use virStoragePoolObjGetAutostartLink storage: Use virStoragePoolObj{Is|Set}Active storage: Use virStoragePoolObj{Is|Set}Autostart storage: Internally represent @autostart to bool storage: Use virStoragePoolObj{Get|Incr}Decr}Asyncjobs storage: Use virStoragePoolObjDefUseNewDef
src/conf/virstorageobj.c | 211 +++++++++++++++++++++++++++++++-- src/conf/virstorageobj.h | 84 ++++++++++++- src/libvirt_private.syms | 20 ++++ src/storage/storage_backend_disk.c | 93 +++++++++------ src/storage/storage_backend_gluster.c | 5 +- src/storage/storage_backend_logical.c | 4 +- src/storage/storage_backend_mpath.c | 3 +- src/storage/storage_backend_rbd.c | 4 +- src/storage/storage_backend_scsi.c | 4 +- src/storage/storage_backend_sheepdog.c | 4 +- src/storage/storage_backend_zfs.c | 6 +- src/storage/storage_driver.c | 142 +++++++++------------- src/storage/storage_util.c | 8 +- src/test/test_driver.c | 54 ++++----- tests/storagevolxml2argvtest.c | 20 ++-- 15 files changed, 465 insertions(+), 197 deletions(-)
ACK series, but please see my comments before pushing. Michal
On 09/19/2017 02:39 AM, Michal Privoznik wrote:
On 08/24/2017 03:08 PM, John Ferlan wrote:
Consider this round 1 of 2.... The next series will be 18 patches, but the majority of those deal with change every {pool|obj}->def->X to use the accessor virStoragePoolObjGetDef.
v1: https://www.redhat.com/archives/libvir-list/2017-May/msg00218.html
Probably not even worth looking at the v1, but this picks up where v1 left off somewhere around patch 10, but adding smaller steps between patches.
John Ferlan (12): storage: Create accessor API's for virStoragePoolObj storage: Introduce virStoragePoolObjNew storage: Fill in storage pool @active properly storage: Introduce storage volume add, delete, count APIs storage: Introduce APIs to search/scan storage pool volumes list storage: Use virStoragePoolObj{Get|Set}ConfigFile storage: Use virStoragePoolObjGetAutostartLink storage: Use virStoragePoolObj{Is|Set}Active storage: Use virStoragePoolObj{Is|Set}Autostart storage: Internally represent @autostart to bool storage: Use virStoragePoolObj{Get|Incr}Decr}Asyncjobs storage: Use virStoragePoolObjDefUseNewDef
src/conf/virstorageobj.c | 211 +++++++++++++++++++++++++++++++-- src/conf/virstorageobj.h | 84 ++++++++++++- src/libvirt_private.syms | 20 ++++ src/storage/storage_backend_disk.c | 93 +++++++++------ src/storage/storage_backend_gluster.c | 5 +- src/storage/storage_backend_logical.c | 4 +- src/storage/storage_backend_mpath.c | 3 +- src/storage/storage_backend_rbd.c | 4 +- src/storage/storage_backend_scsi.c | 4 +- src/storage/storage_backend_sheepdog.c | 4 +- src/storage/storage_backend_zfs.c | 6 +- src/storage/storage_driver.c | 142 +++++++++------------- src/storage/storage_util.c | 8 +- src/test/test_driver.c | 54 ++++----- tests/storagevolxml2argvtest.c | 20 ++-- 15 files changed, 465 insertions(+), 197 deletions(-)
ACK series, but please see my comments before pushing.
Michal
Adjusted patch 2 as recommended and fixed the commit messages in patches 10 and 11. I'll push shortly and follow-up with alteration of virStoragePoolObjGetAutostartLink (and virStoragePoolObjGetConfigFile) to change the prototype return to be const char * Thanks for the review! John
participants (2)
-
John Ferlan -
Michal Privoznik