[PATCH 0/5] storage: Implement 'checkPool' for 'disk' pools and various fixes
Peter Krempa (5): storage: Add debug logs for storage pool config loading virStoragePoolObjLoadAllConfigs: Use automatic memory clearing storageDriverAutostartCallback: Refactor control flow virStoragePoolObjSourceFindDuplicateCb: Fix handling of VIR_STORAGE_POOL_ISCSI_DIRECT storage: Implement 'checkPool' method for 'disk' type pools src/conf/virstorageobj.c | 23 ++++---------- src/storage/storage_backend_disk.c | 27 ++++++++++++++++ src/storage/storage_driver.c | 51 ++++++++++++++++-------------- 3 files changed, 61 insertions(+), 40 deletions(-) -- 2.34.1
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/virstorageobj.c | 4 ++++ src/storage/storage_driver.c | 2 ++ 2 files changed, 6 insertions(+) diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index 02903ac487..815ada8584 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -1614,6 +1614,8 @@ virStoragePoolObjLoad(virStoragePoolObjList *pools, virStoragePoolObj *obj; g_autoptr(virStoragePoolDef) def = NULL; + VIR_DEBUG("loading storage pool config XML '%s'", path); + if (!(def = virStoragePoolDefParseFile(path))) return NULL; @@ -1655,6 +1657,8 @@ virStoragePoolObjLoadState(virStoragePoolObjList *pools, if (!(stateFile = virFileBuildPath(stateDir, name, ".xml"))) return NULL; + VIR_DEBUG("loading storage pool state XML '%s'", stateFile); + if (!(xml = virXMLParseCtxt(stateFile, NULL, _("(pool state)"), &ctxt))) return NULL; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 4df2c75a2b..8dfa2497fc 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -156,6 +156,8 @@ storagePoolUpdateStateCallback(virStoragePoolObj *obj, active = false; } + VIR_DEBUG("updating state of storage pool '%s' active=%d", def->name, active); + /* We can pass NULL as connection, most backends do not use * it anyway, but if they do and fail, we want to log error and * continue with other pools. -- 2.34.1
Refactor the inner loop to automatically free temporary variables and remove unreachable error paths. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/virstorageobj.c | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index 815ada8584..4081e12823 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -1736,27 +1736,15 @@ virStoragePoolObjLoadAllConfigs(virStoragePoolObjList *pools, return rc; while ((ret = virDirRead(dir, &entry, configDir)) > 0) { - char *path; - char *autostartLink; + g_autofree char *path = virFileBuildPath(configDir, entry->d_name, NULL); + g_autofree char *autostartLink = virFileBuildPath(autostartDir, entry->d_name, NULL); virStoragePoolObj *obj; if (!virStringHasSuffix(entry->d_name, ".xml")) continue; - if (!(path = virFileBuildPath(configDir, entry->d_name, NULL))) - continue; - - if (!(autostartLink = virFileBuildPath(autostartDir, entry->d_name, - NULL))) { - VIR_FREE(path); - continue; - } - obj = virStoragePoolObjLoad(pools, entry->d_name, path, autostartLink); virStoragePoolObjEndAPI(&obj); - - VIR_FREE(path); - VIR_FREE(autostartLink); } return ret; -- 2.34.1
Use early returns to decrease the indentation level and make it more obvious that the 'cleanup' path is a noop in those cases. 'virStoragePoolObjSetStarting' was called only when the code wanted to start the pool, so if that was skipped, cleanup is noop as it's conditional on the return value of 'virStoragePoolObjIsStarting'. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/storage/storage_driver.c | 49 ++++++++++++++++++------------------ 1 file changed, 25 insertions(+), 24 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 8dfa2497fc..97e0d9b3a0 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -194,38 +194,39 @@ storageDriverAutostartCallback(virStoragePoolObj *obj, { virStoragePoolDef *def = virStoragePoolObjGetDef(obj); virStorageBackend *backend; - bool started = false; + g_autofree char *stateFile = NULL; if (!(backend = virStorageBackendForType(def->type))) return; - if (virStoragePoolObjIsAutostart(obj) && - !virStoragePoolObjIsActive(obj)) { + if (!virStoragePoolObjIsAutostart(obj)) + return; - virStoragePoolObjSetStarting(obj, true); - if (backend->startPool && - backend->startPool(obj) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to autostart storage pool '%s': %s"), - def->name, virGetLastErrorMessage()); - goto cleanup; - } - started = true; + if (virStoragePoolObjIsActive(obj)) + return; + + VIR_DEBUG("autostarting storage pool '%s'", def->name); + + virStoragePoolObjSetStarting(obj, true); + + if (backend->startPool && + backend->startPool(obj) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to autostart storage pool '%s': %s"), + def->name, virGetLastErrorMessage()); + goto cleanup; } - if (started) { - g_autofree char *stateFile = NULL; + stateFile = virFileBuildPath(driver->stateDir, def->name, ".xml"); - stateFile = virFileBuildPath(driver->stateDir, def->name, ".xml"); - if (!stateFile || - virStoragePoolSaveState(stateFile, def) < 0 || - storagePoolRefreshImpl(backend, obj, stateFile) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to autostart storage pool '%s': %s"), - def->name, virGetLastErrorMessage()); - } else { - virStoragePoolObjSetActive(obj, true); - } + if (!stateFile || + virStoragePoolSaveState(stateFile, def) < 0 || + storagePoolRefreshImpl(backend, obj, stateFile) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to autostart storage pool '%s': %s"), + def->name, virGetLastErrorMessage()); + } else { + virStoragePoolObjSetActive(obj, true); } cleanup: -- 2.34.1
The direct SCSI pool doesn't expose the volumes in the host attempting to match it using 'virStoragePoolObjSourceMatchTypeDEVICE' which in turn uses 'virStoragePoolSourceFindDuplicateDevices' doesn't make sense. Remove it from the source matching completely as we can open multiple connections to the target. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/virstorageobj.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index 4081e12823..58c86e8b1b 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -1447,13 +1447,11 @@ virStoragePoolObjSourceFindDuplicateCb(const void *payload, break; case VIR_STORAGE_POOL_ISCSI: - case VIR_STORAGE_POOL_ISCSI_DIRECT: case VIR_STORAGE_POOL_FS: case VIR_STORAGE_POOL_LOGICAL: case VIR_STORAGE_POOL_DISK: case VIR_STORAGE_POOL_ZFS: if ((data->def->type == VIR_STORAGE_POOL_ISCSI || - data->def->type == VIR_STORAGE_POOL_ISCSI_DIRECT || data->def->type == VIR_STORAGE_POOL_FS || data->def->type == VIR_STORAGE_POOL_LOGICAL || data->def->type == VIR_STORAGE_POOL_DISK || @@ -1481,6 +1479,7 @@ virStoragePoolObjSourceFindDuplicateCb(const void *payload, return 1; break; + case VIR_STORAGE_POOL_ISCSI_DIRECT: case VIR_STORAGE_POOL_RBD: case VIR_STORAGE_POOL_LAST: break; -- 2.34.1
If 'checkPool' is not implemented, the pool will be made inactive when restarting libvirtd and subsequently re-loading the state from the pool state XML. Base the 'checkPool' implementation on logic similar to 'startPool'. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1910856 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/storage/storage_backend_disk.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index be8a535570..bf867491d0 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -476,6 +476,32 @@ virStorageBackendDiskStartPool(virStoragePoolObj *pool) } +static int +virStorageBackendDiskCheckPool(virStoragePoolObj *pool, + bool *isActive) +{ + virStoragePoolDef *def = virStoragePoolObjGetDef(pool); + const char *path = def->source.devices[0].path; + + *isActive = false; + + if (!virFileExists(path)) + return 0; + + if (def->source.format == VIR_STORAGE_POOL_DISK_UNKNOWN) + def->source.format = VIR_STORAGE_POOL_DISK_DOS; + + if (!virStorageBackendDeviceIsEmpty(path, + virStoragePoolFormatDiskTypeToString(def->source.format), + false)) + return -1; + + *isActive = true; + + return 0; +} + + /** * Write a new partition table header */ @@ -973,6 +999,7 @@ virStorageBackend virStorageBackendDisk = { .buildPool = virStorageBackendDiskBuildPool, .refreshPool = virStorageBackendDiskRefreshPool, .deletePool = virStorageBackendDiskDeletePool, + .checkPool = virStorageBackendDiskCheckPool, .createVol = virStorageBackendDiskCreateVol, .deleteVol = virStorageBackendDiskDeleteVol, -- 2.34.1
On a Thursday in 2022, Peter Krempa wrote:
Peter Krempa (5): storage: Add debug logs for storage pool config loading virStoragePoolObjLoadAllConfigs: Use automatic memory clearing storageDriverAutostartCallback: Refactor control flow virStoragePoolObjSourceFindDuplicateCb: Fix handling of VIR_STORAGE_POOL_ISCSI_DIRECT storage: Implement 'checkPool' method for 'disk' type pools
src/conf/virstorageobj.c | 23 ++++---------- src/storage/storage_backend_disk.c | 27 ++++++++++++++++ src/storage/storage_driver.c | 51 ++++++++++++++++-------------- 3 files changed, 61 insertions(+), 40 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Peter Krempa