[libvirt] [PATCH 0/3] storage: Fix operations working with local storage only
Peter Krempa (3): conf: storage: Add helper to determine whether storage vol is local storage: wipe: Don't attempt to wipe remote storage storage: Split out volume upload/download as separate backend function src/conf/storage_conf.c | 19 +++++++++++++ src/conf/storage_conf.h | 2 ++ src/libvirt_private.syms | 1 + src/storage/storage_backend.c | 31 ++++++++++++++++++++ src/storage/storage_backend.h | 30 ++++++++++++++++++++ src/storage/storage_backend_disk.c | 2 ++ src/storage/storage_backend_fs.c | 7 +++++ src/storage/storage_backend_iscsi.c | 2 ++ src/storage/storage_backend_logical.c | 2 ++ src/storage/storage_backend_mpath.c | 2 ++ src/storage/storage_backend_scsi.c | 2 ++ src/storage/storage_driver.c | 53 ++++++++++++++--------------------- 12 files changed, 121 insertions(+), 32 deletions(-) -- 2.0.0
Add a simple helper that returns true if a storage vol definition points to a volume accessible as a file in the host. --- src/conf/storage_conf.c | 19 +++++++++++++++++++ src/conf/storage_conf.h | 2 ++ src/libvirt_private.syms | 1 + 3 files changed, 22 insertions(+) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 9ac5975..55e44b2 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -335,6 +335,25 @@ virStorageVolDefFree(virStorageVolDefPtr def) VIR_FREE(def); } +/* Returns true if storage volume can be accessed as a file in the host */ +bool +virStorageVolIsLocalStorage(virStorageVolDefPtr vol) +{ + switch ((virStorageVolType) vol->type) { + case VIR_STORAGE_VOL_FILE: + case VIR_STORAGE_VOL_BLOCK: + case VIR_STORAGE_VOL_DIR: + return true; + + case VIR_STORAGE_VOL_NETWORK: + case VIR_STORAGE_VOL_NETDIR: + case VIR_STORAGE_VOL_LAST: + return false; + } + + return false; +} + static void virStoragePoolSourceAdapterClear(virStoragePoolSourceAdapter adapter) { diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 47f769b..175c3b7 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -328,6 +328,8 @@ virStorageVolDefPtr virStorageVolDefFindByName(virStoragePoolObjPtr pool, const char *name); +bool virStorageVolIsLocalStorage(virStorageVolDefPtr vol); + void virStoragePoolObjClearVols(virStoragePoolObjPtr pool); virStoragePoolDefPtr virStoragePoolDefParseString(const char *xml); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index e59ea4c..ccb1de7 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -764,6 +764,7 @@ virStorageVolDefFree; virStorageVolDefParseFile; virStorageVolDefParseNode; virStorageVolDefParseString; +virStorageVolIsLocalStorage; virStorageVolTypeFromString; virStorageVolTypeToString; -- 2.0.0
Volume wiping uses external tool to wipe the storage. This will not work with purely remote storage as native gluster or RBD. Add a check to report better error when wiping a remote volume. Before: $ virsh vol-wipe --pool glusterpool qcow3 error: Failed to wipe vol qcow3 error: Failed to open storage volume with path 'gluster://gluster-node-1/gv0/qcow3': No such file or directory After: $ virsh vol-wipe --pool glusterpool qcow3 error: Failed to wipe vol qcow3 error: Operation not supported: Volume wiping is not supported on non-local storage --- src/storage/storage_driver.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index ae86c69..7303dbf 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2226,6 +2226,12 @@ storageVolWipeInternal(virStorageVolDefPtr def, VIR_DEBUG("Wiping volume with path '%s' and algorithm %u", def->target.path, algorithm); + if (!virStorageVolIsLocalStorage(def)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("Volume wiping is not supported on non-local storage")); + goto cleanup; + } + fd = open(def->target.path, O_RDWR); if (fd == -1) { virReportSystemError(errno, -- 2.0.0
On 07/10/2014 08:30 AM, Peter Krempa wrote:
Volume wiping uses external tool to wipe the storage. This will not work with purely remote storage as native gluster or RBD. Add a check to report better error when wiping a remote volume.
Before: $ virsh vol-wipe --pool glusterpool qcow3 error: Failed to wipe vol qcow3 error: Failed to open storage volume with path 'gluster://gluster-node-1/gv0/qcow3': No such file or directory
After: $ virsh vol-wipe --pool glusterpool qcow3 error: Failed to wipe vol qcow3 error: Operation not supported: Volume wiping is not supported on non-local storage
Technically, remote wiping MAY be possible (in fact, it may even be MORE efficient than calling out to external storage - doesn't gluster provide some APIs to request background file wiping?). But that's where we'd need per-pool callbacks for handling the request (local storage uses the existing code, remote storage supplies new callbacks to hook into the proper wipe protocol for that storage). I'd feel a bit better if we refactored this function into delegating the wipe to a _backend.c file callback, and failing when the callback is not yet defined, rather than open-coding it to be local-only. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
For non-local storage drivers we can't expect to use the FDStream backend for up/downloading volumes. Split the code into a separate backend function so that we can add protocol specific code later. --- src/storage/storage_backend.c | 31 +++++++++++++++++++++++ src/storage/storage_backend.h | 30 ++++++++++++++++++++++ src/storage/storage_backend_disk.c | 2 ++ src/storage/storage_backend_fs.c | 7 ++++++ src/storage/storage_backend_iscsi.c | 2 ++ src/storage/storage_backend_logical.c | 2 ++ src/storage/storage_backend_mpath.c | 2 ++ src/storage/storage_backend_scsi.c | 2 ++ src/storage/storage_driver.c | 47 +++++++++++------------------------ 9 files changed, 93 insertions(+), 32 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index e83a108..7b17ca4 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -56,6 +56,7 @@ #include "stat-time.h" #include "virstring.h" #include "virxml.h" +#include "fdstream.h" #if WITH_STORAGE_LVM # include "storage_backend_logical.h" @@ -1669,6 +1670,36 @@ virStorageBackendStablePath(virStoragePoolObjPtr pool, return stablepath; } +int +virStorageBackendVolUploadLocal(virConnectPtr conn ATTRIBUTE_UNUSED, + virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, + virStorageVolDefPtr vol, + virStreamPtr stream, + unsigned long long offset, + unsigned long long len, + unsigned int flags) +{ + virCheckFlags(0, -1); + + /* Not using O_CREAT because the file is required to already exist at + * this point */ + return virFDStreamOpenFile(stream, vol->target.path, offset, len, O_WRONLY); +} + +int +virStorageBackendVolDownloadLocal(virConnectPtr conn ATTRIBUTE_UNUSED, + virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, + virStorageVolDefPtr vol, + virStreamPtr stream, + unsigned long long offset, + unsigned long long len, + unsigned int flags) +{ + virCheckFlags(0, -1); + + return virFDStreamOpenFile(stream, vol->target.path, offset, len, O_RDONLY); +} + #ifdef GLUSTER_CLI int virStorageBackendFindGlusterPoolSources(const char *host, diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 76c1afa..4d7d4b0 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -73,6 +73,20 @@ typedef int (*virStorageBackendVolumeResize)(virConnectPtr conn, virStorageVolDefPtr vol, unsigned long long capacity, unsigned int flags); +typedef int (*virStorageBackendVolumeDownload)(virConnectPtr conn, + virStoragePoolObjPtr obj, + virStorageVolDefPtr vol, + virStreamPtr stream, + unsigned long long offset, + unsigned long long length, + unsigned int flags); +typedef int (*virStorageBackendVolumeUpload)(virConnectPtr conn, + virStoragePoolObjPtr obj, + virStorageVolDefPtr vol, + virStreamPtr stream, + unsigned long long offset, + unsigned long long len, + unsigned int flags); /* File creation/cloning functions used for cloning between backends */ int virStorageBackendCreateRaw(virConnectPtr conn, @@ -91,6 +105,20 @@ int virStorageBackendFindGlusterPoolSources(const char *host, int pooltype, virStoragePoolSourceListPtr list); +int virStorageBackendVolUploadLocal(virConnectPtr conn, + virStoragePoolObjPtr pool, + virStorageVolDefPtr vol, + virStreamPtr stream, + unsigned long long offset, + unsigned long long len, + unsigned int flags); +int virStorageBackendVolDownloadLocal(virConnectPtr conn, + virStoragePoolObjPtr pool, + virStorageVolDefPtr vol, + virStreamPtr stream, + unsigned long long offset, + unsigned long long len, + unsigned int flags); typedef struct _virStorageBackend virStorageBackend; typedef virStorageBackend *virStorageBackendPtr; @@ -114,6 +142,8 @@ struct _virStorageBackend { virStorageBackendRefreshVol refreshVol; virStorageBackendDeleteVol deleteVol; virStorageBackendVolumeResize resizeVol; + virStorageBackendVolumeUpload uploadVol; + virStorageBackendVolumeDownload downloadVol; }; virStorageBackendPtr virStorageBackendForType(int type); diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index 8e12974..f900dee 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -792,4 +792,6 @@ virStorageBackend virStorageBackendDisk = { .createVol = virStorageBackendDiskCreateVol, .deleteVol = virStorageBackendDiskDeleteVol, .buildVolFrom = virStorageBackendDiskBuildVolFrom, + .uploadVol = virStorageBackendVolUploadLocal, + .downloadVol = virStorageBackendVolDownloadLocal, }; diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index c93fc1e..1615c12 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -1275,6 +1275,7 @@ virStorageBackendFileSystemVolResize(virConnectPtr conn ATTRIBUTE_UNUSED, } } + virStorageBackend virStorageBackendDirectory = { .type = VIR_STORAGE_POOL_DIR, @@ -1288,6 +1289,8 @@ virStorageBackend virStorageBackendDirectory = { .refreshVol = virStorageBackendFileSystemVolRefresh, .deleteVol = virStorageBackendFileSystemVolDelete, .resizeVol = virStorageBackendFileSystemVolResize, + .uploadVol = virStorageBackendVolUploadLocal, + .downloadVol = virStorageBackendVolDownloadLocal, }; #if WITH_STORAGE_FS @@ -1306,6 +1309,8 @@ virStorageBackend virStorageBackendFileSystem = { .refreshVol = virStorageBackendFileSystemVolRefresh, .deleteVol = virStorageBackendFileSystemVolDelete, .resizeVol = virStorageBackendFileSystemVolResize, + .uploadVol = virStorageBackendVolUploadLocal, + .downloadVol = virStorageBackendVolDownloadLocal, }; virStorageBackend virStorageBackendNetFileSystem = { .type = VIR_STORAGE_POOL_NETFS, @@ -1323,6 +1328,8 @@ virStorageBackend virStorageBackendNetFileSystem = { .refreshVol = virStorageBackendFileSystemVolRefresh, .deleteVol = virStorageBackendFileSystemVolDelete, .resizeVol = virStorageBackendFileSystemVolResize, + .uploadVol = virStorageBackendVolUploadLocal, + .downloadVol = virStorageBackendVolDownloadLocal, }; diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index 3aac9d3..7345571 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -474,4 +474,6 @@ virStorageBackend virStorageBackendISCSI = { .refreshPool = virStorageBackendISCSIRefreshPool, .stopPool = virStorageBackendISCSIStopPool, .findPoolSources = virStorageBackendISCSIFindPoolSources, + .uploadVol = virStorageBackendVolUploadLocal, + .downloadVol = virStorageBackendVolDownloadLocal, }; diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index a597e67..faa9a4b 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -841,4 +841,6 @@ virStorageBackend virStorageBackendLogical = { .buildVolFrom = virStorageBackendLogicalBuildVolFrom, .createVol = virStorageBackendLogicalCreateVol, .deleteVol = virStorageBackendLogicalDeleteVol, + .uploadVol = virStorageBackendVolUploadLocal, + .downloadVol = virStorageBackendVolDownloadLocal, }; diff --git a/src/storage/storage_backend_mpath.c b/src/storage/storage_backend_mpath.c index 8c3b0df..402faa0 100644 --- a/src/storage/storage_backend_mpath.c +++ b/src/storage/storage_backend_mpath.c @@ -287,4 +287,6 @@ virStorageBackend virStorageBackendMpath = { .checkPool = virStorageBackendMpathCheckPool, .refreshPool = virStorageBackendMpathRefreshPool, + .uploadVol = virStorageBackendVolUploadLocal, + .downloadVol = virStorageBackendVolDownloadLocal, }; diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index f6f3ca2..0b44f71 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -728,4 +728,6 @@ virStorageBackend virStorageBackendSCSI = { .refreshPool = virStorageBackendSCSIRefreshPool, .startPool = virStorageBackendSCSIStartPool, .stopPool = virStorageBackendSCSIStopPool, + .uploadVol = virStorageBackendVolUploadLocal, + .downloadVol = virStorageBackendVolDownloadLocal, }; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 7303dbf..ba20c2d 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1920,13 +1920,14 @@ storageVolDownload(virStorageVolPtr obj, unsigned long long length, unsigned int flags) { + virStorageBackendPtr backend; virStoragePoolObjPtr pool = NULL; virStorageVolDefPtr vol = NULL; int ret = -1; virCheckFlags(0, -1); - if (!(vol = virStorageVolDefFromVol(obj, &pool, NULL))) + if (!(vol = virStorageVolDefFromVol(obj, &pool, &backend))) return -1; if (virStorageVolDownloadEnsureACL(obj->conn, pool->def, vol) < 0) @@ -1939,13 +1940,14 @@ storageVolDownload(virStorageVolPtr obj, goto cleanup; } - if (virFDStreamOpenFile(stream, - vol->target.path, - offset, length, - O_RDONLY) < 0) + if (!backend->downloadVol) { + virReportError(VIR_ERR_NO_SUPPORT, "%s", + _("storage pool doesn't support volume download")); goto cleanup; + } - ret = 0; + ret = backend->downloadVol(obj->conn, pool, vol, stream, + offset, length, flags); cleanup: virStoragePoolObjUnlock(pool); @@ -1961,13 +1963,14 @@ storageVolUpload(virStorageVolPtr obj, unsigned long long length, unsigned int flags) { + virStorageBackendPtr backend; virStoragePoolObjPtr pool = NULL; virStorageVolDefPtr vol = NULL; int ret = -1; virCheckFlags(0, -1); - if (!(vol = virStorageVolDefFromVol(obj, &pool, NULL))) + if (!(vol = virStorageVolDefFromVol(obj, &pool, &backend))) return -1; if (virStorageVolUploadEnsureACL(obj->conn, pool->def, vol) < 0) @@ -1987,34 +1990,14 @@ storageVolUpload(virStorageVolPtr obj, goto cleanup; } - switch ((virStoragePoolType) pool->def->type) { - case VIR_STORAGE_POOL_DIR: - case VIR_STORAGE_POOL_FS: - case VIR_STORAGE_POOL_NETFS: - case VIR_STORAGE_POOL_LOGICAL: - case VIR_STORAGE_POOL_DISK: - case VIR_STORAGE_POOL_ISCSI: - case VIR_STORAGE_POOL_SCSI: - case VIR_STORAGE_POOL_MPATH: - /* Not using O_CREAT because the file is required to already exist at - * this point */ - if (virFDStreamOpenFile(stream, vol->target.path, - offset, length, O_WRONLY) < 0) - goto cleanup; - - break; - - case VIR_STORAGE_POOL_SHEEPDOG: - case VIR_STORAGE_POOL_RBD: - case VIR_STORAGE_POOL_GLUSTER: - case VIR_STORAGE_POOL_LAST: - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, - _("volume upload is not supported with pools of type %s"), - virStoragePoolTypeToString(pool->def->type)); + if (!backend->uploadVol) { + virReportError(VIR_ERR_NO_SUPPORT, "%s", + _("storage pool doesn't support volume upload")); goto cleanup; } - ret = 0; + ret = backend->uploadVol(obj->conn, pool, vol, stream, + offset, length, flags); cleanup: virStoragePoolObjUnlock(pool); -- 2.0.0
On 07/10/2014 08:30 AM, Peter Krempa wrote:
For non-local storage drivers we can't expect to use the FDStream backend for up/downloading volumes. Split the code into a separate backend function so that we can add protocol specific code later. --- src/storage/storage_backend.c | 31 +++++++++++++++++++++++ src/storage/storage_backend.h | 30 ++++++++++++++++++++++ src/storage/storage_backend_disk.c | 2 ++ src/storage/storage_backend_fs.c | 7 ++++++ src/storage/storage_backend_iscsi.c | 2 ++ src/storage/storage_backend_logical.c | 2 ++ src/storage/storage_backend_mpath.c | 2 ++ src/storage/storage_backend_scsi.c | 2 ++ src/storage/storage_driver.c | 47 +++++++++++------------------------ 9 files changed, 93 insertions(+), 32 deletions(-)
Without even reading this patch yet, this is more the approach I had in mind for 2/3. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 07/10/14 18:48, Eric Blake wrote:
On 07/10/2014 08:30 AM, Peter Krempa wrote:
For non-local storage drivers we can't expect to use the FDStream backend for up/downloading volumes. Split the code into a separate backend function so that we can add protocol specific code later. --- src/storage/storage_backend.c | 31 +++++++++++++++++++++++ src/storage/storage_backend.h | 30 ++++++++++++++++++++++ src/storage/storage_backend_disk.c | 2 ++ src/storage/storage_backend_fs.c | 7 ++++++ src/storage/storage_backend_iscsi.c | 2 ++ src/storage/storage_backend_logical.c | 2 ++ src/storage/storage_backend_mpath.c | 2 ++ src/storage/storage_backend_scsi.c | 2 ++ src/storage/storage_driver.c | 47 +++++++++++------------------------ 9 files changed, 93 insertions(+), 32 deletions(-)
Without even reading this patch yet, this is more the approach I had in mind for 2/3.
I've pushed this one according to Jan's ACK and will re-do 2/3 in the same fashion. Peter
On 07/10/2014 04:30 PM, Peter Krempa wrote:
Peter Krempa (3): conf: storage: Add helper to determine whether storage vol is local storage: wipe: Don't attempt to wipe remote storage storage: Split out volume upload/download as separate backend function
src/conf/storage_conf.c | 19 +++++++++++++ src/conf/storage_conf.h | 2 ++ src/libvirt_private.syms | 1 + src/storage/storage_backend.c | 31 ++++++++++++++++++++ src/storage/storage_backend.h | 30 ++++++++++++++++++++ src/storage/storage_backend_disk.c | 2 ++ src/storage/storage_backend_fs.c | 7 +++++ src/storage/storage_backend_iscsi.c | 2 ++ src/storage/storage_backend_logical.c | 2 ++ src/storage/storage_backend_mpath.c | 2 ++ src/storage/storage_backend_scsi.c | 2 ++ src/storage/storage_driver.c | 53 ++++++++++++++--------------------- 12 files changed, 121 insertions(+), 32 deletions(-)
ACK series Jan
participants (3)
-
Eric Blake -
Ján Tomko -
Peter Krempa