[libvirt] [PATCH 0/3] storage: refactor storage file helpers in storage driver after virStorageSource introduction

Peter Krempa (3): conf: Refactor helpers to retrieve actual storage type storage: Refactor storage file initialization to use virStorageSourcePtr storage: Refactor location of metadata for storage drive access to files src/conf/domain_conf.c | 10 --- src/conf/domain_conf.h | 1 - src/conf/snapshot_conf.c | 7 --- src/conf/snapshot_conf.h | 2 - src/libvirt_private.syms | 3 +- src/qemu/qemu_command.c | 6 +- src/qemu/qemu_driver.c | 37 ++++++------ src/storage/storage_backend.c | 1 + src/storage/storage_backend.h | 20 ++++-- src/storage/storage_backend_fs.c | 12 ++-- src/storage/storage_backend_gluster.c | 33 +++++----- src/storage/storage_driver.c | 111 ++++++++++++---------------------- src/storage/storage_driver.h | 35 +++-------- src/util/virstoragefile.c | 10 +++ src/util/virstoragefile.h | 6 ++ 15 files changed, 122 insertions(+), 172 deletions(-) -- 1.9.1

Now that the storage source definition is uniform convert the helpers to retrieve the actual storage type to a single one. --- src/conf/domain_conf.c | 10 ---------- src/conf/domain_conf.h | 1 - src/conf/snapshot_conf.c | 7 ------- src/conf/snapshot_conf.h | 2 -- src/libvirt_private.syms | 3 +-- src/qemu/qemu_command.c | 6 +++--- src/qemu/qemu_driver.c | 12 ++++++------ src/storage/storage_driver.c | 4 ++-- src/util/virstoragefile.c | 10 ++++++++++ src/util/virstoragefile.h | 1 + 10 files changed, 23 insertions(+), 33 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 52bbf87..56a7f70 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1215,16 +1215,6 @@ virDomainDiskSetType(virDomainDiskDefPtr def, int type) } -int -virDomainDiskGetActualType(virDomainDiskDefPtr def) -{ - if (def->src.type == VIR_STORAGE_TYPE_VOLUME && def->src.srcpool) - return def->src.srcpool->actualtype; - - return def->src.type; -} - - const char * virDomainDiskGetSource(virDomainDiskDefPtr def) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index addf654..2ff0bc4 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2132,7 +2132,6 @@ void virDomainDiskDefFree(virDomainDiskDefPtr def); void virDomainLeaseDefFree(virDomainLeaseDefPtr def); int virDomainDiskGetType(virDomainDiskDefPtr def); void virDomainDiskSetType(virDomainDiskDefPtr def, int type); -int virDomainDiskGetActualType(virDomainDiskDefPtr def); const char *virDomainDiskGetSource(virDomainDiskDefPtr def); int virDomainDiskSetSource(virDomainDiskDefPtr def, const char *src) ATTRIBUTE_RETURN_CHECK; diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 852a286..070e50d 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -1290,10 +1290,3 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain, cleanup: return ret; } - - -int -virDomainSnapshotDiskGetActualType(virDomainSnapshotDiskDefPtr def) -{ - return def->src.type; -} diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h index 1444d77..5f74b62 100644 --- a/src/conf/snapshot_conf.h +++ b/src/conf/snapshot_conf.h @@ -183,8 +183,6 @@ int virDomainSnapshotRedefinePrep(virDomainPtr domain, bool *update_current, unsigned int flags); -int virDomainSnapshotDiskGetActualType(virDomainSnapshotDiskDefPtr def); - VIR_ENUM_DECL(virDomainSnapshotLocation) VIR_ENUM_DECL(virDomainSnapshotState) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 55aa586..9c189dc 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -197,7 +197,6 @@ virDomainDiskErrorPolicyTypeToString; virDomainDiskFindByBusAndDst; virDomainDiskGeometryTransTypeFromString; virDomainDiskGeometryTransTypeToString; -virDomainDiskGetActualType; virDomainDiskGetDriver; virDomainDiskGetFormat; virDomainDiskGetSource; @@ -663,7 +662,6 @@ virDomainSnapshotDefFormat; virDomainSnapshotDefFree; virDomainSnapshotDefIsExternal; virDomainSnapshotDefParseString; -virDomainSnapshotDiskGetActualType; virDomainSnapshotDropParent; virDomainSnapshotFindByName; virDomainSnapshotForEach; @@ -1843,6 +1841,7 @@ virStorageNetHostTransportTypeToString; virStorageNetProtocolTypeToString; virStorageSourceAuthClear; virStorageSourceClear; +virStorageSourceGetActualType; virStorageSourcePoolDefFree; virStorageSourcePoolModeTypeFromString; virStorageSourcePoolModeTypeToString; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 379c094..8b58d5e 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3863,7 +3863,7 @@ qemuDomainDiskGetSourceString(virConnectPtr conn, virDomainDiskDefPtr disk, char **source) { - int actualType = virDomainDiskGetActualType(disk); + int actualType = virStorageSourceGetActualType(&disk->src); char *secret = NULL; int ret = -1; @@ -3893,7 +3893,7 @@ qemuDomainDiskGetSourceString(virConnectPtr conn, goto cleanup; } - ret = qemuGetDriveSourceString(virDomainDiskGetActualType(disk), + ret = qemuGetDriveSourceString(actualType, disk->src.path, disk->src.protocol, disk->src.nhosts, @@ -3921,7 +3921,7 @@ qemuBuildDriveStr(virConnectPtr conn, int idx = virDiskNameToIndex(disk->dst); int busid = -1, unitid = -1; char *source = NULL; - int actualType = virDomainDiskGetActualType(disk); + int actualType = virStorageSourceGetActualType(&disk->src); if (idx < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4bb4819..5b0e484 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12005,7 +12005,7 @@ qemuDomainSnapshotDiskGetSourceString(virDomainSnapshotDiskDefPtr disk, { *source = NULL; - return qemuGetDriveSourceString(virDomainSnapshotDiskGetActualType(disk), + return qemuGetDriveSourceString(virStorageSourceGetActualType(&disk->src), disk->src.path, disk->src.protocol, disk->src.nhosts, @@ -12326,7 +12326,7 @@ qemuDomainSnapshotCreateActiveInternal(virConnectPtr conn, static int qemuDomainSnapshotPrepareDiskExternalBackingInactive(virDomainDiskDefPtr disk) { - int actualType = virDomainDiskGetActualType(disk); + int actualType = virStorageSourceGetActualType(&disk->src); switch ((enum virStorageType) actualType) { case VIR_STORAGE_TYPE_BLOCK: @@ -12371,7 +12371,7 @@ qemuDomainSnapshotPrepareDiskExternalBackingInactive(virDomainDiskDefPtr disk) static int qemuDomainSnapshotPrepareDiskExternalBackingActive(virDomainDiskDefPtr disk) { - int actualType = virDomainDiskGetActualType(disk); + int actualType = virStorageSourceGetActualType(&disk->src); if (actualType == VIR_STORAGE_TYPE_BLOCK && disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) { @@ -12388,7 +12388,7 @@ qemuDomainSnapshotPrepareDiskExternalBackingActive(virDomainDiskDefPtr disk) static int qemuDomainSnapshotPrepareDiskExternalOverlayActive(virDomainSnapshotDiskDefPtr disk) { - int actualType = virDomainSnapshotDiskGetActualType(disk); + int actualType = virStorageSourceGetActualType(&disk->src); switch ((enum virStorageType) actualType) { case VIR_STORAGE_TYPE_BLOCK: @@ -12436,7 +12436,7 @@ qemuDomainSnapshotPrepareDiskExternalOverlayActive(virDomainSnapshotDiskDefPtr d static int qemuDomainSnapshotPrepareDiskExternalOverlayInactive(virDomainSnapshotDiskDefPtr disk) { - int actualType = virDomainSnapshotDiskGetActualType(disk); + int actualType = virStorageSourceGetActualType(&disk->src); switch ((enum virStorageType) actualType) { case VIR_STORAGE_TYPE_BLOCK: @@ -12534,7 +12534,7 @@ qemuDomainSnapshotPrepareDiskInternal(virConnectPtr conn, if (qemuTranslateDiskSourcePool(conn, disk) < 0) return -1; - actualType = virDomainDiskGetActualType(disk); + actualType = virStorageSourceGetActualType(&disk->src); switch ((enum virStorageType) actualType) { case VIR_STORAGE_TYPE_BLOCK: diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 2f98f78..629525a 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2790,7 +2790,7 @@ virStorageFileInitInternal(int type, virStorageFilePtr virStorageFileInitFromDiskDef(virDomainDiskDefPtr disk) { - return virStorageFileInitInternal(virDomainDiskGetActualType(disk), + return virStorageFileInitInternal(virStorageSourceGetActualType(&disk->src), disk->src.path, disk->src.protocol, disk->src.nhosts, @@ -2801,7 +2801,7 @@ virStorageFileInitFromDiskDef(virDomainDiskDefPtr disk) virStorageFilePtr virStorageFileInitFromSnapshotDef(virDomainSnapshotDiskDefPtr disk) { - return virStorageFileInitInternal(virDomainSnapshotDiskGetActualType(disk), + return virStorageFileInitInternal(virStorageSourceGetActualType(&disk->src), disk->src.path, disk->src.protocol, disk->src.nhosts, diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 3cdc89c..2bf3a53 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1561,6 +1561,16 @@ virStorageSourceAuthClear(virStorageSourcePtr def) } +int +virStorageSourceGetActualType(virStorageSourcePtr def) +{ + if (def->type == VIR_STORAGE_TYPE_VOLUME && def->srcpool) + return def->srcpool->actualtype; + + return def->type; +} + + void virStorageSourceClear(virStorageSourcePtr def) { diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 83ec2bd..22198a8 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -298,5 +298,6 @@ virStorageNetHostDefPtr virStorageNetHostDefCopy(size_t nhosts, void virStorageSourceAuthClear(virStorageSourcePtr def); void virStorageSourcePoolDefFree(virStorageSourcePoolDefPtr def); void virStorageSourceClear(virStorageSourcePtr def); +int virStorageSourceGetActualType(virStorageSourcePtr def); #endif /* __VIR_STORAGE_FILE_H__ */ -- 1.9.1

On 04/08/2014 08:12 AM, Peter Krempa wrote:
Now that the storage source definition is uniform convert the helpers to retrieve the actual storage type to a single one. --- src/conf/domain_conf.c | 10 ---------- src/conf/domain_conf.h | 1 - src/conf/snapshot_conf.c | 7 ------- src/conf/snapshot_conf.h | 2 -- src/libvirt_private.syms | 3 +-- src/qemu/qemu_command.c | 6 +++--- src/qemu/qemu_driver.c | 12 ++++++------ src/storage/storage_driver.c | 4 ++-- src/util/virstoragefile.c | 10 ++++++++++ src/util/virstoragefile.h | 1 + 10 files changed, 23 insertions(+), 33 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

Now that storage source metadata is stored in a single struct we don't need two initialization functions for different structs. --- src/qemu/qemu_driver.c | 6 +++--- src/storage/storage_backend.c | 1 + src/storage/storage_driver.c | 41 +++++++---------------------------------- src/storage/storage_driver.h | 8 +++----- 4 files changed, 14 insertions(+), 42 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5b0e484..d824e24 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12489,7 +12489,7 @@ qemuDomainSnapshotPrepareDiskExternal(virConnectPtr conn, return -1; } - if (!(snapfile = virStorageFileInitFromSnapshotDef(snapdisk))) + if (!(snapfile = virStorageFileInit(&snapdisk->src))) return -1; if (virStorageFileStat(snapfile, &st) < 0) { @@ -12757,7 +12757,7 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, virStorageFileFreeMetadata(disk->backingChain); disk->backingChain = NULL; - if (!(snapfile = virStorageFileInitFromSnapshotDef(snap))) + if (!(snapfile = virStorageFileInit(&snap->src))) goto cleanup; if (qemuDomainSnapshotDiskGetSourceString(snap, &source) < 0) @@ -12914,7 +12914,7 @@ qemuDomainSnapshotUndoSingleDiskActive(virQEMUDriverPtr driver, virStorageFilePtr diskfile = NULL; struct stat st; - diskfile = virStorageFileInitFromDiskDef(disk); + diskfile = virStorageFileInit(&disk->src); if (VIR_STRDUP(source, origdisk->src.path) < 0 || (persistDisk && VIR_STRDUP(persistSource, source) < 0)) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index b56fefe..8f9df78 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -55,6 +55,7 @@ #include "virfile.h" #include "stat-time.h" #include "virstring.h" +#include "virxml.h" #if WITH_STORAGE_LVM # include "storage_backend_logical.h" diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 629525a..b26d791 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2747,26 +2747,22 @@ virStorageFileFree(virStorageFilePtr file) } -static virStorageFilePtr -virStorageFileInitInternal(int type, - const char *path, - int protocol, - size_t nhosts, - virStorageNetHostDefPtr hosts) +virStorageFilePtr +virStorageFileInit(virStorageSourcePtr src) { virStorageFilePtr file = NULL; if (VIR_ALLOC(file) < 0) return NULL; - file->type = type; - file->protocol = protocol; - file->nhosts = nhosts; + file->type = virStorageSourceGetActualType(src); + file->protocol = src->protocol; + file->nhosts = src->nhosts; - if (VIR_STRDUP(file->path, path) < 0) + if (VIR_STRDUP(file->path, src->path) < 0) goto error; - if (!(file->hosts = virStorageNetHostDefCopy(nhosts, hosts))) + if (!(file->hosts = virStorageNetHostDefCopy(src->nhosts, src->hosts))) goto error; if (!(file->backend = virStorageFileBackendForType(file->type, @@ -2787,29 +2783,6 @@ virStorageFileInitInternal(int type, } -virStorageFilePtr -virStorageFileInitFromDiskDef(virDomainDiskDefPtr disk) -{ - return virStorageFileInitInternal(virStorageSourceGetActualType(&disk->src), - disk->src.path, - disk->src.protocol, - disk->src.nhosts, - disk->src.hosts); -} - - -virStorageFilePtr -virStorageFileInitFromSnapshotDef(virDomainSnapshotDiskDefPtr disk) -{ - return virStorageFileInitInternal(virStorageSourceGetActualType(&disk->src), - disk->src.path, - disk->src.protocol, - disk->src.nhosts, - disk->src.hosts); -} - - - /** * virStorageFileCreate: Creates an empty storage file via storage driver * diff --git a/src/storage/storage_driver.h b/src/storage/storage_driver.h index 886a4d5..d5dda00 100644 --- a/src/storage/storage_driver.h +++ b/src/storage/storage_driver.h @@ -25,8 +25,8 @@ # define __VIR_STORAGE_DRIVER_H__ # include "storage_conf.h" -# include "conf/domain_conf.h" -# include "conf/snapshot_conf.h" +# include "virstoragefile.h" +# include <sys/stat.h> typedef struct _virStorageFileBackend virStorageFileBackend; typedef virStorageFileBackend *virStorageFileBackendPtr; @@ -46,9 +46,7 @@ struct _virStorageFile { }; virStorageFilePtr -virStorageFileInitFromDiskDef(virDomainDiskDefPtr disk); -virStorageFilePtr -virStorageFileInitFromSnapshotDef(virDomainSnapshotDiskDefPtr disk); +virStorageFileInit(virStorageSourcePtr src); void virStorageFileFree(virStorageFilePtr file); int virStorageFileCreate(virStorageFilePtr file); -- 1.9.1

On 04/08/2014 08:12 AM, Peter Krempa wrote:
Now that storage source metadata is stored in a single struct we don't need two initialization functions for different structs. --- src/qemu/qemu_driver.c | 6 +++--- src/storage/storage_backend.c | 1 + src/storage/storage_driver.c | 41 +++++++---------------------------------- src/storage/storage_driver.h | 8 +++----- 4 files changed, 14 insertions(+), 42 deletions(-)
+++ b/src/storage/storage_driver.h @@ -25,8 +25,8 @@ # define __VIR_STORAGE_DRIVER_H__
# include "storage_conf.h" -# include "conf/domain_conf.h" -# include "conf/snapshot_conf.h" +# include "virstoragefile.h" +# include <sys/stat.h>
We tend to list <system> includes prior to "local" includes. ACK with that reordered. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

Now that we store all metadata about a storage image in a virStorageSource struct lets use it also to store information needed by the storage driver to access and do operations on the files. --- src/qemu/qemu_driver.c | 25 +++++------ src/storage/storage_backend.h | 20 ++++++--- src/storage/storage_backend_fs.c | 12 ++--- src/storage/storage_backend_gluster.c | 33 +++++++------- src/storage/storage_driver.c | 82 ++++++++++++++++------------------- src/storage/storage_driver.h | 27 +++--------- src/util/virstoragefile.h | 5 +++ 7 files changed, 96 insertions(+), 108 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d824e24..72834c2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12465,7 +12465,6 @@ qemuDomainSnapshotPrepareDiskExternal(virConnectPtr conn, bool active, bool reuse) { - virStorageFilePtr snapfile = NULL; int ret = -1; struct stat st; @@ -12489,10 +12488,10 @@ qemuDomainSnapshotPrepareDiskExternal(virConnectPtr conn, return -1; } - if (!(snapfile = virStorageFileInit(&snapdisk->src))) + if (virStorageFileInit(&snapdisk->src) < 0) return -1; - if (virStorageFileStat(snapfile, &st) < 0) { + if (virStorageFileStat(&snapdisk->src, &st) < 0) { if (errno != ENOENT) { virReportSystemError(errno, _("unable to stat for disk %s: %s"), @@ -12515,7 +12514,7 @@ qemuDomainSnapshotPrepareDiskExternal(virConnectPtr conn, ret = 0; cleanup: - virStorageFileFree(snapfile); + virStorageFileDeinit(&snapdisk->src); return ret; } @@ -12738,7 +12737,6 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, int ret = -1; int fd = -1; bool need_unlink = false; - virStorageFilePtr snapfile = NULL; if (snap->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -12757,7 +12755,7 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, virStorageFileFreeMetadata(disk->backingChain); disk->backingChain = NULL; - if (!(snapfile = virStorageFileInit(&snap->src))) + if (virStorageFileInit(&snap->src) < 0) goto cleanup; if (qemuDomainSnapshotDiskGetSourceString(snap, &source) < 0) @@ -12886,9 +12884,9 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, } cleanup: - if (need_unlink && virStorageFileUnlink(snapfile)) + if (need_unlink && virStorageFileUnlink(&snap->src)) VIR_WARN("unable to unlink just-created %s", source); - virStorageFileFree(snapfile); + virStorageFileDeinit(&snap->src); VIR_FREE(device); VIR_FREE(source); VIR_FREE(newsource); @@ -12911,10 +12909,9 @@ qemuDomainSnapshotUndoSingleDiskActive(virQEMUDriverPtr driver, { char *source = NULL; char *persistSource = NULL; - virStorageFilePtr diskfile = NULL; struct stat st; - diskfile = virStorageFileInit(&disk->src); + ignore_value(virStorageFileInit(&disk->src)); if (VIR_STRDUP(source, origdisk->src.path) < 0 || (persistDisk && VIR_STRDUP(persistSource, source) < 0)) @@ -12922,9 +12919,9 @@ qemuDomainSnapshotUndoSingleDiskActive(virQEMUDriverPtr driver, qemuDomainPrepareDiskChainElement(driver, vm, disk, disk->src.path, VIR_DISK_CHAIN_NO_ACCESS); - if (need_unlink && diskfile && - virStorageFileStat(diskfile, &st) == 0 && S_ISREG(st.st_mode) && - virStorageFileUnlink(diskfile) < 0) + if (need_unlink && + virStorageFileStat(&disk->src, &st) == 0 && S_ISREG(st.st_mode) && + virStorageFileUnlink(&disk->src) < 0) VIR_WARN("Unable to remove just-created %s", disk->src.path); /* Update vm in place to match changes. */ @@ -12953,7 +12950,7 @@ qemuDomainSnapshotUndoSingleDiskActive(virQEMUDriverPtr driver, } cleanup: - virStorageFileFree(diskfile); + virStorageFileDeinit(&disk->src); VIR_FREE(source); VIR_FREE(persistSource); } diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 4f95000..5997077 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -160,24 +160,34 @@ virStorageBackendCreateQemuImgCmd(virConnectPtr conn, int imgformat); /* ------- virStorageFile backends ------------ */ +typedef struct _virStorageFileBackend virStorageFileBackend; +typedef virStorageFileBackend *virStorageFileBackendPtr; + +struct _virStorageDriverData { + virStorageFileBackendPtr backend; + void *priv; +}; + typedef int -(*virStorageFileBackendInit)(virStorageFilePtr file); +(*virStorageFileBackendInit)(virStorageSourcePtr src); typedef void -(*virStorageFileBackendDeinit)(virStorageFilePtr file); +(*virStorageFileBackendDeinit)(virStorageSourcePtr src); typedef int -(*virStorageFileBackendCreate)(virStorageFilePtr file); +(*virStorageFileBackendCreate)(virStorageSourcePtr src); typedef int -(*virStorageFileBackendUnlink)(virStorageFilePtr file); +(*virStorageFileBackendUnlink)(virStorageSourcePtr src); typedef int -(*virStorageFileBackendStat)(virStorageFilePtr file, +(*virStorageFileBackendStat)(virStorageSourcePtr src, struct stat *st); virStorageFileBackendPtr virStorageFileBackendForType(int type, int protocol); + + struct _virStorageFileBackend { int type; int protocol; diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 4e4a7ae..bb20385 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -1335,31 +1335,31 @@ virStorageBackend virStorageBackendNetFileSystem = { static int -virStorageFileBackendFileUnlink(virStorageFilePtr file) +virStorageFileBackendFileUnlink(virStorageSourcePtr src) { int ret; - ret = unlink(file->path); + ret = unlink(src->path); /* preserve errno */ VIR_DEBUG("removing storage file %p(%s): ret=%d, errno=%d", - file, file->path, ret, errno); + src, src->path, ret, errno); return ret; } static int -virStorageFileBackendFileStat(virStorageFilePtr file, +virStorageFileBackendFileStat(virStorageSourcePtr src, struct stat *st) { int ret; - ret = stat(file->path, st); + ret = stat(src->path, st); /* preserve errno */ VIR_DEBUG("stat of storage file %p(%s): ret=%d, errno=%d", - file, file->path, ret, errno); + src, src->path, ret, errno); return ret; } diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index d131f13..2593c1c 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -546,41 +546,41 @@ struct _virStorageFileBackendGlusterPriv { static void -virStorageFileBackendGlusterDeinit(virStorageFilePtr file) +virStorageFileBackendGlusterDeinit(virStorageSourcePtr src) { VIR_DEBUG("deinitializing gluster storage file %p(%s/%s)", - file, file->hosts[0].name, file->path); - virStorageFileBackendGlusterPrivPtr priv = file->priv; + src, src->hosts[0].name, src->path); + virStorageFileBackendGlusterPrivPtr priv = src->drv->priv; if (priv->vol) glfs_fini(priv->vol); VIR_FREE(priv->volname); VIR_FREE(priv); - file->priv = NULL; + src->drv->priv = NULL; } static int -virStorageFileBackendGlusterInit(virStorageFilePtr file) +virStorageFileBackendGlusterInit(virStorageSourcePtr src) { virStorageFileBackendGlusterPrivPtr priv = NULL; - virStorageNetHostDefPtr host = &(file->hosts[0]); + virStorageNetHostDefPtr host = &(src->hosts[0]); const char *hostname = host->name; int port = 0; VIR_DEBUG("initializing gluster storage file %p(%s/%s)", - file, hostname, file->path); + src, hostname, src->path); if (VIR_ALLOC(priv) < 0) return -1; - if (VIR_STRDUP(priv->volname, file->path) < 0) + if (VIR_STRDUP(priv->volname, src->path) < 0) goto error; if (!(priv->path = strchr(priv->volname, '/'))) { virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid path of gluster volume: '%s'"), - file->path); + src->path); goto error; } @@ -598,7 +598,6 @@ virStorageFileBackendGlusterInit(virStorageFilePtr file) if (host->transport == VIR_STORAGE_NET_HOST_TRANS_UNIX) hostname = host->socket; - if (!(priv->vol = glfs_new(priv->volname))) { virReportOOMError(); goto error; @@ -620,7 +619,7 @@ virStorageFileBackendGlusterInit(virStorageFilePtr file) goto error; } - file->priv = priv; + src->drv->priv = priv; return 0; @@ -635,32 +634,32 @@ virStorageFileBackendGlusterInit(virStorageFilePtr file) static int -virStorageFileBackendGlusterUnlink(virStorageFilePtr file) +virStorageFileBackendGlusterUnlink(virStorageSourcePtr src) { - virStorageFileBackendGlusterPrivPtr priv = file->priv; + virStorageFileBackendGlusterPrivPtr priv = src->drv->priv; int ret; ret = glfs_unlink(priv->vol, priv->path); /* preserve errno */ VIR_DEBUG("removing storage file %p(%s/%s): ret=%d, errno=%d", - file, file->hosts[0].name, file->path, ret, errno); + src, src->hosts[0].name, src->path, ret, errno); return ret; } static int -virStorageFileBackendGlusterStat(virStorageFilePtr file, +virStorageFileBackendGlusterStat(virStorageSourcePtr src, struct stat *st) { - virStorageFileBackendGlusterPrivPtr priv = file->priv; + virStorageFileBackendGlusterPrivPtr priv = src->drv->priv; int ret; ret = glfs_stat(priv->vol, priv->path, st); /* preserve errno */ VIR_DEBUG("stat of storage file %p(%s/%s): ret=%d, errno=%d", - file, file->hosts[0].name, file->path, ret, errno); + src, src->hosts[0].name, src->path, ret, errno); return ret; } diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index b26d791..de0e04c 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2731,82 +2731,74 @@ int storageRegister(void) /* ----------- file handlers cooperating with storage driver --------------- */ +static bool +virStorageFileIsInitialized(virStorageSourcePtr src) +{ + return !!src->drv; +} + void -virStorageFileFree(virStorageFilePtr file) +virStorageFileDeinit(virStorageSourcePtr src) { - if (!file) + if (!virStorageFileIsInitialized(src)) return; - if (file->backend && - file->backend->backendDeinit) - file->backend->backendDeinit(file); + if (src->drv->backend && + src->drv->backend->backendDeinit) + src->drv->backend->backendDeinit(src); - VIR_FREE(file->path); - virStorageNetHostDefFree(file->nhosts, file->hosts); - VIR_FREE(file); + VIR_FREE(src->drv); } -virStorageFilePtr +int virStorageFileInit(virStorageSourcePtr src) { - virStorageFilePtr file = NULL; - - if (VIR_ALLOC(file) < 0) - return NULL; - - file->type = virStorageSourceGetActualType(src); - file->protocol = src->protocol; - file->nhosts = src->nhosts; - - if (VIR_STRDUP(file->path, src->path) < 0) - goto error; - - if (!(file->hosts = virStorageNetHostDefCopy(src->nhosts, src->hosts))) - goto error; + int actualType = virStorageSourceGetActualType(src); + if (VIR_ALLOC(src->drv) < 0) + return -1; - if (!(file->backend = virStorageFileBackendForType(file->type, - file->protocol))) + if (!(src->drv->backend = virStorageFileBackendForType(actualType, + src->protocol))) goto error; - if (file->backend->backendInit && - file->backend->backendInit(file) < 0) + if (src->drv->backend->backendInit && + src->drv->backend->backendInit(src) < 0) goto error; - return file; + return 0; error: - VIR_FREE(file->path); - virStorageNetHostDefFree(file->nhosts, file->hosts); - VIR_FREE(file); - return NULL; + VIR_FREE(src->drv); + return -1; } /** * virStorageFileCreate: Creates an empty storage file via storage driver * - * @file: file structure pointing to the file + * @src: file structure pointing to the file * * Returns 0 on success, -2 if the function isn't supported by the backend, * -1 on other failure. Errno is set in case of failure. */ int -virStorageFileCreate(virStorageFilePtr file) +virStorageFileCreate(virStorageSourcePtr src) { - if (!file->backend->storageFileCreate) { + if (!virStorageFileIsInitialized(src) || + !src->drv->backend->storageFileCreate) { errno = ENOSYS; return -2; } - return file->backend->storageFileCreate(file); + return src->drv->backend->storageFileCreate(src); } /** * virStorageFileUnlink: Unlink storage file via storage driver * - * @file: file structure pointing to the file + * @src: file structure pointing to the file * * Unlinks the file described by the @file structure. * @@ -2814,34 +2806,36 @@ virStorageFileCreate(virStorageFilePtr file) * -1 on other failure. Errno is set in case of failure. */ int -virStorageFileUnlink(virStorageFilePtr file) +virStorageFileUnlink(virStorageSourcePtr src) { - if (!file->backend->storageFileUnlink) { + if (!virStorageFileIsInitialized(src) || + !src->drv->backend->storageFileUnlink) { errno = ENOSYS; return -2; } - return file->backend->storageFileUnlink(file); + return src->drv->backend->storageFileUnlink(src); } /** * virStorageFileStat: returns stat struct of a file via storage driver * - * @file: file structure pointing to the file + * @src: file structure pointing to the file * @stat: stat structure to return data * * Returns 0 on success, -2 if the function isn't supported by the backend, * -1 on other failure. Errno is set in case of failure. */ int -virStorageFileStat(virStorageFilePtr file, +virStorageFileStat(virStorageSourcePtr src, struct stat *st) { - if (!(file->backend->storageFileStat)) { + if (!virStorageFileIsInitialized(src) || + !src->drv->backend->storageFileStat) { errno = ENOSYS; return -2; } - return file->backend->storageFileStat(file, st); + return src->drv->backend->storageFileStat(src, st); } diff --git a/src/storage/storage_driver.h b/src/storage/storage_driver.h index d5dda00..e1f5ffd 100644 --- a/src/storage/storage_driver.h +++ b/src/storage/storage_driver.h @@ -28,30 +28,13 @@ # include "virstoragefile.h" # include <sys/stat.h> -typedef struct _virStorageFileBackend virStorageFileBackend; -typedef virStorageFileBackend *virStorageFileBackendPtr; - -typedef struct _virStorageFile virStorageFile; -typedef virStorageFile *virStorageFilePtr; -struct _virStorageFile { - virStorageFileBackendPtr backend; - void *priv; - - char *path; - int type; - int protocol; - - size_t nhosts; - virStorageNetHostDefPtr hosts; -}; - -virStorageFilePtr +int virStorageFileInit(virStorageSourcePtr src); -void virStorageFileFree(virStorageFilePtr file); +void virStorageFileDeinit(virStorageSourcePtr src); -int virStorageFileCreate(virStorageFilePtr file); -int virStorageFileUnlink(virStorageFilePtr file); -int virStorageFileStat(virStorageFilePtr file, +int virStorageFileCreate(virStorageSourcePtr src); +int virStorageFileUnlink(virStorageSourcePtr src); +int virStorageFileStat(virStorageSourcePtr src, struct stat *stat); int storageRegister(void); diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 22198a8..7cb0f2b 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -208,6 +208,8 @@ enum virStorageSecretType { VIR_STORAGE_SECRET_TYPE_LAST }; +typedef struct _virStorageDriverData virStorageDriverData; +typedef virStorageDriverData *virStorageDriverDataPtr; typedef struct _virStorageSource virStorageSource; typedef virStorageSource *virStorageSourcePtr; @@ -243,6 +245,9 @@ struct _virStorageSource { unsigned long long capacity; /* in bytes, 0 if unknown */ size_t nseclabels; virSecurityDeviceLabelDefPtr *seclabels; + + /* metadata for storage driver access to remote and local volumes */ + virStorageDriverDataPtr drv; }; -- 1.9.1

On 04/08/2014 08:12 AM, Peter Krempa wrote:
Now that we store all metadata about a storage image in a virStorageSource struct lets use it also to store information needed by
s/lets/let's/
the storage driver to access and do operations on the files. --- src/qemu/qemu_driver.c | 25 +++++------ src/storage/storage_backend.h | 20 ++++++--- src/storage/storage_backend_fs.c | 12 ++--- src/storage/storage_backend_gluster.c | 33 +++++++------- src/storage/storage_driver.c | 82 ++++++++++++++++------------------- src/storage/storage_driver.h | 27 +++--------- src/util/virstoragefile.h | 5 +++ 7 files changed, 96 insertions(+), 108 deletions(-)
Fairly even trade, but does look nicer. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

Refactor the function to avoid multiple wrappers splitting identical fields from the now common metadata struct. The refactor is done by folding in the wrapper used for disk sources which allows us to lookup secrets via the secret driver. This may allow using stored secrets for snapshot disk images too in the future. --- src/qemu/qemu_command.c | 122 +++++++++++++++++++++--------------------------- src/qemu/qemu_command.h | 12 ++--- src/qemu/qemu_driver.c | 19 +------- 3 files changed, 58 insertions(+), 95 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 1766a5b..8b4a57a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3816,37 +3816,68 @@ qemuBuildNetworkDriveURI(int protocol, int -qemuGetDriveSourceString(int type, - const char *src, - int protocol, - size_t nhosts, - virStorageNetHostDefPtr hosts, - const char *username, - const char *secret, - char **path) +qemuGetDriveSourceString(virStorageSourcePtr src, + virConnectPtr conn, + char **source) { - *path = NULL; + int actualType = virStorageSourceGetActualType(src); + char *secret = NULL; + char *username = NULL; + int ret = -1; + + *source = NULL; + + if (conn) { + if (actualType == VIR_STORAGE_TYPE_NETWORK && + src->auth.username && + (src->protocol == VIR_STORAGE_NET_PROTOCOL_ISCSI || + src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD)) { + bool encode = false; + int secretType = VIR_SECRET_USAGE_TYPE_ISCSI; + const char *protocol = virStorageNetProtocolTypeToString(src->protocol); + + username = src->auth.username; - switch ((enum virStorageType) type) { + if (src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD) { + /* qemu requires the secret to be encoded for RBD */ + encode = true; + secretType = VIR_SECRET_USAGE_TYPE_CEPH; + } + + if (!(secret = qemuGetSecretString(conn, + protocol, + encode, + src->auth.secretType, + username, + src->auth.secret.uuid, + src->auth.secret.usage, + secretType))) + goto cleanup; + } + } + + switch ((enum virStorageType) actualType) { case VIR_STORAGE_TYPE_BLOCK: case VIR_STORAGE_TYPE_FILE: case VIR_STORAGE_TYPE_DIR: - if (!src) - return 1; + if (!src->path) { + ret = 1; + goto cleanup; + } - if (VIR_STRDUP(*path, src) < 0) - return -1; + if (VIR_STRDUP(*source, src->path) < 0) + goto cleanup; break; case VIR_STORAGE_TYPE_NETWORK: - if (!(*path = qemuBuildNetworkDriveURI(protocol, - src, - nhosts, - hosts, - username, - secret))) - return -1; + if (!(*source = qemuBuildNetworkDriveURI(src->protocol, + src->path, + src->nhosts, + src->hosts, + username, + secret))) + goto cleanup; break; case VIR_STORAGE_TYPE_VOLUME: @@ -3855,52 +3886,7 @@ qemuGetDriveSourceString(int type, break; } - return 0; -} - -static int -qemuDomainDiskGetSourceString(virConnectPtr conn, - virDomainDiskDefPtr disk, - char **source) -{ - int actualType = virStorageSourceGetActualType(&disk->src); - char *secret = NULL; - int ret = -1; - - *source = NULL; - - if (actualType == VIR_STORAGE_TYPE_NETWORK && - disk->src.auth.username && - (disk->src.protocol == VIR_STORAGE_NET_PROTOCOL_ISCSI || - disk->src.protocol == VIR_STORAGE_NET_PROTOCOL_RBD)) { - bool encode = false; - int secretType = VIR_SECRET_USAGE_TYPE_ISCSI; - - if (disk->src.protocol == VIR_STORAGE_NET_PROTOCOL_RBD) { - /* qemu requires the secret to be encoded for RBD */ - encode = true; - secretType = VIR_SECRET_USAGE_TYPE_CEPH; - } - - if (!(secret = qemuGetSecretString(conn, - virStorageNetProtocolTypeToString(disk->src.protocol), - encode, - disk->src.auth.secretType, - disk->src.auth.username, - disk->src.auth.secret.uuid, - disk->src.auth.secret.usage, - secretType))) - goto cleanup; - } - - ret = qemuGetDriveSourceString(actualType, - disk->src.path, - disk->src.protocol, - disk->src.nhosts, - disk->src.hosts, - disk->src.auth.username, - secret, - source); + ret = 0; cleanup: VIR_FREE(secret); @@ -4003,7 +3989,7 @@ qemuBuildDriveStr(virConnectPtr conn, break; } - if (qemuDomainDiskGetSourceString(conn, disk, &source) < 0) + if (qemuGetDriveSourceString(&disk->src, conn, &source) < 0) goto error; if (source && diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 7f8b875..bad6ac0 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -307,13 +307,7 @@ qemuParseKeywords(const char *str, int *retnkeywords, int allowEmptyValue); -int qemuGetDriveSourceString(int type, - const char *src, - int protocol, - size_t nhosts, - virStorageNetHostDefPtr hosts, - const char *username, - const char *secret, - char **path); - +int qemuGetDriveSourceString(virStorageSourcePtr src, + virConnectPtr conn, + char **source); #endif /* __QEMU_COMMAND_H__*/ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 72834c2..d26a22b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11999,23 +11999,6 @@ qemuDomainMigrateGetMaxSpeed(virDomainPtr dom, } -static int -qemuDomainSnapshotDiskGetSourceString(virDomainSnapshotDiskDefPtr disk, - char **source) -{ - *source = NULL; - - return qemuGetDriveSourceString(virStorageSourceGetActualType(&disk->src), - disk->src.path, - disk->src.protocol, - disk->src.nhosts, - disk->src.hosts, - NULL, - NULL, - source); -} - - typedef enum { VIR_DISK_CHAIN_NO_ACCESS, VIR_DISK_CHAIN_READ_ONLY, @@ -12758,7 +12741,7 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, if (virStorageFileInit(&snap->src) < 0) goto cleanup; - if (qemuDomainSnapshotDiskGetSourceString(snap, &source) < 0) + if (qemuGetDriveSourceString(&snap->src, NULL, &source) < 0) goto cleanup; if (VIR_STRDUP(newsource, snap->src.path) < 0) -- 1.9.1

On 04/08/2014 09:55 AM, Peter Krempa wrote:
Refactor the function to avoid multiple wrappers splitting identical fields from the now common metadata struct.
The refactor is done by folding in the wrapper used for disk sources which allows us to lookup secrets via the secret driver. This may allow using stored secrets for snapshot disk images too in the future. --- src/qemu/qemu_command.c | 122 +++++++++++++++++++++--------------------------- src/qemu/qemu_command.h | 12 ++--- src/qemu/qemu_driver.c | 19 +------- 3 files changed, 58 insertions(+), 95 deletions(-)
ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 04/09/14 03:40, Eric Blake wrote:
On 04/08/2014 09:55 AM, Peter Krempa wrote:
Refactor the function to avoid multiple wrappers splitting identical fields from the now common metadata struct.
The refactor is done by folding in the wrapper used for disk sources which allows us to lookup secrets via the secret driver. This may allow using stored secrets for snapshot disk images too in the future. --- src/qemu/qemu_command.c | 122 +++++++++++++++++++++--------------------------- src/qemu/qemu_command.h | 12 ++--- src/qemu/qemu_driver.c | 19 +------- 3 files changed, 58 insertions(+), 95 deletions(-)
ACK
Thanks; Series is now pushed. Peter
participants (2)
-
Eric Blake
-
Peter Krempa