[libvirt] [PATCH 00/10] Snapshots on gluster volumes

This series has to be applied on top of http://www.redhat.com/archives/libvir-list/2014-January/msg00192.html Peter Krempa (10): storage: Introduce internal pool support storage: Add new argument for createVol backend API storage: gluster: Introduce dummy functions for creating a volume storage: Add internal API to create temporary storage pools and vols snapshot: Add support for specifying snapshot disk backing type snapshot: Test snapshot disk type specification storage: Implement ephemeral storage APIs for local disk volumes storage: gluster: Support conversion of gluster volumes to temp volumes qemu: snapshot: Switch snapshot file deletion to the new storage API qemu: snapshot: Add support for external active snapshots on gluster docs/formatsnapshot.html.in | 15 + docs/hvsupport.pl | 3 + docs/schemas/domainsnapshot.rng | 76 ++++- src/Makefile.am | 3 +- src/check-aclrules.pl | 3 + src/check-drivername.pl | 1 + src/conf/snapshot_conf.c | 25 +- src/conf/snapshot_conf.h | 15 +- src/conf/storage_conf.c | 3 + src/conf/storage_conf.h | 1 + src/driver.h | 13 + src/libvirt_internal.c | 71 ++++ src/libvirt_internal.h | 22 ++ src/libvirt_private.syms | 3 + src/qemu/qemu_command.c | 2 +- src/qemu/qemu_command.h | 9 + src/qemu/qemu_driver.c | 228 ++++++++++--- src/storage/storage_backend.h | 2 +- src/storage/storage_backend_disk.c | 3 +- src/storage/storage_backend_fs.c | 30 +- src/storage/storage_backend_gluster.c | 68 ++++ src/storage/storage_backend_logical.c | 6 +- src/storage/storage_backend_rbd.c | 3 +- src/storage/storage_backend_sheepdog.c | 3 +- src/storage/storage_driver.c | 416 ++++++++++++++++++++++- tests/domainsnapshotxml2xmlin/disk_snapshot.xml | 18 + tests/domainsnapshotxml2xmlout/disk_snapshot.xml | 18 + 27 files changed, 979 insertions(+), 81 deletions(-) create mode 100644 src/libvirt_internal.c -- 1.8.5.2

To allow using the storage driver APIs to do operation on generic domain disks we will need to introduce internal storage pools that will give is a base to support this stuff even on files that weren't originally defined as a part of the pool. This patch introduces the 'internal' flag for a storage pool that will prevent it from being listed along with the user defined storage pools. --- src/conf/storage_conf.c | 3 +++ src/conf/storage_conf.h | 1 + src/storage/storage_driver.c | 12 ++++++++---- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index ed492cf..379bbd6 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -2172,6 +2172,9 @@ static bool virStoragePoolMatch(virStoragePoolObjPtr poolobj, unsigned int flags) { + if (poolobj->internal) + return false; + /* filter by active state */ if (MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_ACTIVE) && !((MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_ACTIVE) && diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 485bdba..62ac749 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -332,6 +332,7 @@ struct _virStoragePoolObj { int active; int autostart; unsigned int asyncjobs; + bool internal; virStoragePoolDefPtr def; virStoragePoolDefPtr newDef; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index aaa0f02..683d61d 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -377,7 +377,8 @@ storageConnectNumOfStoragePools(virConnectPtr conn) { virStoragePoolObjPtr obj = driver->pools.objs[i]; virStoragePoolObjLock(obj); if (virConnectNumOfStoragePoolsCheckACL(conn, obj->def) && - virStoragePoolObjIsActive(obj)) + virStoragePoolObjIsActive(obj) && + !obj->internal) nactive++; virStoragePoolObjUnlock(obj); } @@ -402,7 +403,8 @@ storageConnectListStoragePools(virConnectPtr conn, virStoragePoolObjPtr obj = driver->pools.objs[i]; virStoragePoolObjLock(obj); if (virConnectListStoragePoolsCheckACL(conn, obj->def) && - virStoragePoolObjIsActive(obj)) { + virStoragePoolObjIsActive(obj) && + !obj->internal) { if (VIR_STRDUP(names[got], obj->def->name) < 0) { virStoragePoolObjUnlock(obj); goto cleanup; @@ -436,7 +438,8 @@ storageConnectNumOfDefinedStoragePools(virConnectPtr conn) { virStoragePoolObjPtr obj = driver->pools.objs[i]; virStoragePoolObjLock(obj); if (virConnectNumOfDefinedStoragePoolsCheckACL(conn, obj->def) && - !virStoragePoolObjIsActive(obj)) + !virStoragePoolObjIsActive(obj) && + !obj->internal) nactive++; virStoragePoolObjUnlock(obj); } @@ -461,7 +464,8 @@ storageConnectListDefinedStoragePools(virConnectPtr conn, virStoragePoolObjPtr obj = driver->pools.objs[i]; virStoragePoolObjLock(obj); if (virConnectListDefinedStoragePoolsCheckACL(conn, obj->def) && - !virStoragePoolObjIsActive(obj)) { + !virStoragePoolObjIsActive(obj) && + !obj->internal) { if (VIR_STRDUP(names[got], obj->def->name) < 0) { virStoragePoolObjUnlock(obj); goto cleanup; -- 1.8.5.2

On 01/09/2014 09:15 AM, Peter Krempa wrote:
To allow using the storage driver APIs to do operation on generic domain disks we will need to introduce internal storage pools that will give is a base to support this stuff even on files that weren't originally defined as a part of the pool.
This patch introduces the 'internal' flag for a storage pool that will prevent it from being listed along with the user defined storage pools. --- src/conf/storage_conf.c | 3 +++ src/conf/storage_conf.h | 1 + src/storage/storage_driver.c | 12 ++++++++---- 3 files changed, 12 insertions(+), 4 deletions(-)
This definitely counts as a new feature, too risky for inclusion for 1.2.1, but I like what it does. ACK - this merely adds the flag, although nothing sets it until later. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 01/09/14 23:47, Eric Blake wrote:
On 01/09/2014 09:15 AM, Peter Krempa wrote:
To allow using the storage driver APIs to do operation on generic domain disks we will need to introduce internal storage pools that will give is a base to support this stuff even on files that weren't originally defined as a part of the pool.
This patch introduces the 'internal' flag for a storage pool that will prevent it from being listed along with the user defined storage pools. --- src/conf/storage_conf.c | 3 +++ src/conf/storage_conf.h | 1 + src/storage/storage_driver.c | 12 ++++++++---- 3 files changed, 12 insertions(+), 4 deletions(-)
This definitely counts as a new feature, too risky for inclusion for 1.2.1, but I like what it does.
ACK - this merely adds the flag, although nothing sets it until later.
Pushed now; Thanks. Peter

--- src/storage/storage_backend.h | 2 +- src/storage/storage_backend_disk.c | 3 ++- src/storage/storage_backend_fs.c | 30 +++++++++++++++++++++++------- src/storage/storage_backend_logical.c | 6 ++++-- src/storage/storage_backend_rbd.c | 3 ++- src/storage/storage_backend_sheepdog.c | 3 ++- src/storage/storage_driver.c | 4 ++-- 7 files changed, 36 insertions(+), 15 deletions(-) diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 9e07dd8..4d0c057 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -41,7 +41,7 @@ typedef int (*virStorageBackendDeletePool)(virConnectPtr conn, virStoragePoolObj typedef int (*virStorageBackendBuildVol)(virConnectPtr conn, virStoragePoolObjPtr pool, virStorageVolDefPtr vol, unsigned int flags); -typedef int (*virStorageBackendCreateVol)(virConnectPtr conn, virStoragePoolObjPtr pool, virStorageVolDefPtr vol); +typedef int (*virStorageBackendCreateVol)(virConnectPtr conn, virStoragePoolObjPtr pool, virStorageVolDefPtr vol, bool internal); typedef int (*virStorageBackendRefreshVol)(virConnectPtr conn, virStoragePoolObjPtr pool, virStorageVolDefPtr vol); typedef int (*virStorageBackendDeleteVol)(virConnectPtr conn, virStoragePoolObjPtr pool, virStorageVolDefPtr vol, unsigned int flags); typedef int (*virStorageBackendBuildVolFrom)(virConnectPtr conn, virStoragePoolObjPtr pool, diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index aa3b72f..a77c298 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -618,7 +618,8 @@ virStorageBackendDiskPartBoundries(virStoragePoolObjPtr pool, static int virStorageBackendDiskCreateVol(virConnectPtr conn ATTRIBUTE_UNUSED, virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, - virStorageVolDefPtr vol) + virStorageVolDefPtr vol, + bool internal ATTRIBUTE_UNUSED) { if (vol->target.encryption != NULL) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 6ebdd46..1077da7 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -997,8 +997,10 @@ virStorageBackendFileSystemDelete(virConnectPtr conn ATTRIBUTE_UNUSED, static int virStorageBackendFileSystemVolCreate(virConnectPtr conn ATTRIBUTE_UNUSED, virStoragePoolObjPtr pool, - virStorageVolDefPtr vol) + virStorageVolDefPtr vol, + bool internal) { + struct stat st; vol->type = VIR_STORAGE_VOL_FILE; @@ -1008,15 +1010,29 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn ATTRIBUTE_UNUSED, vol->name) == -1) return -1; - if (virFileExists(vol->target.path)) { - virReportError(VIR_ERR_OPERATION_INVALID, - _("volume target path '%s' already exists"), - vol->target.path); - return -1; + if (internal) { + if (stat(vol->target.path, &st) == 0) { + if (S_ISDIR(st.st_mode)) + vol->type = VIR_STORAGE_VOL_DIR; + else if (S_ISBLK(st.st_mode)) + vol->type = VIR_STORAGE_VOL_BLOCK; + else + vol->type = VIR_STORAGE_VOL_FILE; + } + } else { + if (virFileExists(vol->target.path)) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("volume target path '%s' already exists"), + vol->target.path); + return -1; + } } VIR_FREE(vol->key); - return VIR_STRDUP(vol->key, vol->target.path); + if (VIR_STRDUP(vol->key, vol->target.path) < 0) + return -1; + + return 0; } static int createFileDir(virConnectPtr conn ATTRIBUTE_UNUSED, diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 039d962..2e2560f 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -784,7 +784,8 @@ error: static int virStorageBackendLogicalCreateVol(virConnectPtr conn ATTRIBUTE_UNUSED, virStoragePoolObjPtr pool, - virStorageVolDefPtr vol) + virStorageVolDefPtr vol, + bool internal) { if (vol->target.encryption != NULL) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -800,7 +801,8 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn ATTRIBUTE_UNUSED, vol->name) == -1) return -1; - if (virFileExists(vol->target.path)) { + if (!internal && + virFileExists(vol->target.path)) { virReportError(VIR_ERR_OPERATION_INVALID, _("volume target path '%s' already exists"), vol->target.path); diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index c5f0bc5..75425f4 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -439,7 +439,8 @@ cleanup: static int virStorageBackendRBDCreateVol(virConnectPtr conn ATTRIBUTE_UNUSED, virStoragePoolObjPtr pool, - virStorageVolDefPtr vol) + virStorageVolDefPtr vol, + bool internal ATTRIBUTE_UNUSED) { vol->type = VIR_STORAGE_VOL_NETWORK; diff --git a/src/storage/storage_backend_sheepdog.c b/src/storage/storage_backend_sheepdog.c index a6981ce..705451b 100644 --- a/src/storage/storage_backend_sheepdog.c +++ b/src/storage/storage_backend_sheepdog.c @@ -152,7 +152,8 @@ virStorageBackendSheepdogDeleteVol(virConnectPtr conn ATTRIBUTE_UNUSED, static int virStorageBackendSheepdogCreateVol(virConnectPtr conn ATTRIBUTE_UNUSED, virStoragePoolObjPtr pool, - virStorageVolDefPtr vol) + virStorageVolDefPtr vol, + bool internal ATTRIBUTE_UNUSED) { if (vol->target.encryption != NULL) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 683d61d..a392d53 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1643,7 +1643,7 @@ storageVolCreateXML(virStoragePoolPtr obj, /* Wipe any key the user may have suggested, as volume creation * will generate the canonical key. */ VIR_FREE(voldef->key); - if (backend->createVol(obj->conn, pool, voldef) < 0) { + if (backend->createVol(obj->conn, pool, voldef, false) < 0) { goto cleanup; } @@ -1822,7 +1822,7 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj, * Wipe any key the user may have suggested, as volume creation * will generate the canonical key. */ VIR_FREE(newvol->key); - if (backend->createVol(obj->conn, pool, newvol) < 0) { + if (backend->createVol(obj->conn, pool, newvol, false) < 0) { goto cleanup; } -- 1.8.5.2

On 01/09/2014 09:15 AM, Peter Krempa wrote:
---
A little sparse on the intent of the new flag...
src/storage/storage_backend.h | 2 +- src/storage/storage_backend_disk.c | 3 ++- src/storage/storage_backend_fs.c | 30 +++++++++++++++++++++++------- src/storage/storage_backend_logical.c | 6 ++++-- src/storage/storage_backend_rbd.c | 3 ++- src/storage/storage_backend_sheepdog.c | 3 ++- src/storage/storage_driver.c | 4 ++-- 7 files changed, 36 insertions(+), 15 deletions(-)
diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 9e07dd8..4d0c057 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -41,7 +41,7 @@ typedef int (*virStorageBackendDeletePool)(virConnectPtr conn, virStoragePoolObj typedef int (*virStorageBackendBuildVol)(virConnectPtr conn, virStoragePoolObjPtr pool, virStorageVolDefPtr vol, unsigned int flags); -typedef int (*virStorageBackendCreateVol)(virConnectPtr conn, virStoragePoolObjPtr pool, virStorageVolDefPtr vol); +typedef int (*virStorageBackendCreateVol)(virConnectPtr conn, virStoragePoolObjPtr pool, virStorageVolDefPtr vol, bool internal);
...and maybe worth line-wrapping the long lines in this area of code. But know I know what the patch is doing - adding an internal flag; where use of the flag can suppress error reporting (since presumably all internal creation would be done on a name already tied to a <domain>, where we can assume that the name is either valid or the domain will fail to start).
@@ -1008,15 +1010,29 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn ATTRIBUTE_UNUSED, vol->name) == -1) return -1;
- if (virFileExists(vol->target.path)) { - virReportError(VIR_ERR_OPERATION_INVALID, - _("volume target path '%s' already exists"), - vol->target.path); - return -1; + if (internal) { + if (stat(vol->target.path, &st) == 0) { + if (S_ISDIR(st.st_mode)) + vol->type = VIR_STORAGE_VOL_DIR; + else if (S_ISBLK(st.st_mode)) + vol->type = VIR_STORAGE_VOL_BLOCK; + else + vol->type = VIR_STORAGE_VOL_FILE; + }
And if the stat() fails, you just leave vol->type uninitialized? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

The temporary pool code will need to initialize dummy gluster volumes which needs the createVol function of the storage backend. This patch implements it only for that purpose. When an user will get an error message that it is not implemented on an attempt to create a volume in a gluster pool. --- src/storage/storage_backend_gluster.c | 68 +++++++++++++++++++++++++++++++++++ 1 file changed, 68 insertions(+) diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index c73cf8a..2d22801 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -444,10 +444,78 @@ cleanup: } +static int +virStorageBackendGlusterVolCreate(virConnectPtr conn ATTRIBUTE_UNUSED, + virStoragePoolObjPtr pool, + virStorageVolDefPtr vol, + bool internal) +{ + virStorageBackendGlusterStatePtr state = NULL; + struct stat st; + char *tmp; + int ret = -1; + + if (!(state = virStorageBackendGlusterOpen(pool))) + goto cleanup; + + vol->type = VIR_STORAGE_VOL_NETWORK; + vol->target.format = VIR_STORAGE_FILE_RAW; + + VIR_FREE(vol->key); + if (virAsprintf(&vol->key, "/%s%s%s", state->volname, state->dir, + vol->name) < 0) + goto cleanup; + + tmp = state->uri->path; + state->uri->path = vol->key; + VIR_FREE(vol->target.path); + if (!(vol->target.path = virURIFormat(state->uri))) { + state->uri->path = tmp; + goto cleanup; + } + state->uri->path = tmp; + + if (internal) { + if (glfs_stat(state->vol, vol->name, &st) == 0 && + S_ISDIR(st.st_mode)) { + vol->type = VIR_STORAGE_VOL_NETDIR; + vol->target.format = VIR_STORAGE_FILE_DIR; + } + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("gluster pool backend doesn't " + "yet support volume creation")); + goto cleanup; + } + + ret = 0; + +cleanup: + virStorageBackendGlusterClose(state); + return ret; +} + + +static int +virStorageBackendGlusterVolBuild(virConnectPtr conn ATTRIBUTE_UNUSED, + virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, + virStorageVolDefPtr vol ATTRIBUTE_UNUSED, + unsigned int flags) +{ + virCheckFlags(0, -1); + + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("gluster pool backend doesn't yet support volume building")); + return -1; +} + + virStorageBackend virStorageBackendGluster = { .type = VIR_STORAGE_POOL_GLUSTER, .refreshPool = virStorageBackendGlusterRefreshPool, + .createVol = virStorageBackendGlusterVolCreate, + .buildVol = virStorageBackendGlusterVolBuild, .deleteVol = virStorageBackendGlusterVolDelete, }; -- 1.8.5.2

On 01/09/2014 09:15 AM, Peter Krempa wrote:
The temporary pool code will need to initialize dummy gluster volumes which needs the createVol function of the storage backend. This patch implements it only for that purpose. When an user will get an error
s/When an user/A user/
message that it is not implemented on an attempt to create a volume in a gluster pool. --- src/storage/storage_backend_gluster.c | 68 +++++++++++++++++++++++++++++++++++ 1 file changed, 68 insertions(+)
+ + VIR_FREE(vol->key); + if (virAsprintf(&vol->key, "/%s%s%s", state->volname, state->dir, + vol->name) < 0)
This doesn't match the docs, which say gluster volume keys lack a leading slash; see also virStorageBackendGlusterRefreshVol (where it took me a couple tries to get it right).
+ goto cleanup; + + tmp = state->uri->path; + state->uri->path = vol->key; + VIR_FREE(vol->target.path); + if (!(vol->target.path = virURIFormat(state->uri))) { + state->uri->path = tmp; + goto cleanup; + } + state->uri->path = tmp;
In fact, since both this function and virStorageBackendGlusterRefreshVol are doing the same thing, it might be nice to factor it into a helper function, so that if we change our minds yet again about the leading slash in gluster key names, we only have to change one spot in code.
+ + if (internal) { + if (glfs_stat(state->vol, vol->name, &st) == 0 && + S_ISDIR(st.st_mode)) { + vol->type = VIR_STORAGE_VOL_NETDIR; + vol->target.format = VIR_STORAGE_FILE_DIR; + } + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("gluster pool backend doesn't " + "yet support volume creation")); + goto cleanup;
It might be nice to hoist the error reporting earlier in the function, instead of first doing malloc's only to throw them away. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

If a VM driver wants to access stuff provided by a storage driver the volume needs to be a part of a storage pool. As this wasn't designed in from the beginning we need a way to convert generic domain disk and snapshot disk definitions into temporary pools and volumes. This patch allows that by adding private storage driver APIs that can be used to obtain a pool and vol definition. --- docs/hvsupport.pl | 3 ++ src/Makefile.am | 3 +- src/check-drivername.pl | 1 + src/driver.h | 13 +++++++++ src/libvirt_internal.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++ src/libvirt_internal.h | 22 +++++++++++++++ src/libvirt_private.syms | 3 ++ 7 files changed, 115 insertions(+), 1 deletion(-) create mode 100644 src/libvirt_internal.c diff --git a/docs/hvsupport.pl b/docs/hvsupport.pl index f8483f9..a7d1f6b 100755 --- a/docs/hvsupport.pl +++ b/docs/hvsupport.pl @@ -176,6 +176,9 @@ $apis{virDomainMigratePerform3Params} = "1.1.0"; $apis{virDomainMigrateFinish3Params} = "1.1.0"; $apis{virDomainMigrateConfirm3Params} = "1.1.0"; +$apis{virStorageEphemeralFree} = "1.2.1"; +$apis{virStorageEphemeralFromDiskDef} = "1.2.1"; +$apis{virStorageEphemeralFromSnapshotDiskDef} = "1.2.1"; # Now we want to get the mapping between public APIs diff --git a/src/Makefile.am b/src/Makefile.am index 57e163f..c0c535d 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -176,7 +176,8 @@ DRIVER_SOURCES = \ $(DATATYPES_SOURCES) \ fdstream.c fdstream.h \ $(NODE_INFO_SOURCES) \ - libvirt.c libvirt_internal.h \ + libvirt.c \ + libvirt_internal.c libvirt_internal.h \ locking/lock_manager.c locking/lock_manager.h \ locking/lock_driver.h \ locking/lock_driver_nop.h locking/lock_driver_nop.c \ diff --git a/src/check-drivername.pl b/src/check-drivername.pl index 5c8de0a..50b3480 100755 --- a/src/check-drivername.pl +++ b/src/check-drivername.pl @@ -50,6 +50,7 @@ while (<DRVFILE>) { next if $drv =~ /virDrvState/; next if $drv =~ /virDrvDomainMigrate(Prepare|Perform|Confirm|Begin|Finish)/; + next if $drv =~ /virDrvStorageEphemeral/; my $sym = $drv; $sym =~ s/virDrv/vir/; diff --git a/src/driver.h b/src/driver.h index 5f4cd8d..b72c5e9 100644 --- a/src/driver.h +++ b/src/driver.h @@ -1754,6 +1754,16 @@ typedef int typedef int (*virDrvStoragePoolIsPersistent)(virStoragePoolPtr pool); +typedef void +(*virDrvStorageEphemeralFree)(virStorageEphemeralPtr def); + +typedef virStorageEphemeralPtr +(*virDrvStorageEphemeralFromDiskDef)(virConnectPtr conn, + virDomainDiskDefPtr def); + +typedef virStorageEphemeralPtr +(*virDrvStorageEphemeralFromSnapshotDiskDef)(virConnectPtr conn, + virDomainSnapshotDiskDefPtr def); typedef struct _virStorageDriver virStorageDriver; @@ -1813,6 +1823,9 @@ struct _virStorageDriver { virDrvStorageVolResize storageVolResize; virDrvStoragePoolIsActive storagePoolIsActive; virDrvStoragePoolIsPersistent storagePoolIsPersistent; + virDrvStorageEphemeralFree storageEphemeralFree; + virDrvStorageEphemeralFromDiskDef storageEphemeralFromDiskDef; + virDrvStorageEphemeralFromSnapshotDiskDef storageEphemeralFromSnapshotDiskDef; }; # ifdef WITH_LIBVIRTD diff --git a/src/libvirt_internal.c b/src/libvirt_internal.c new file mode 100644 index 0000000..ea10885 --- /dev/null +++ b/src/libvirt_internal.c @@ -0,0 +1,71 @@ +/* + * libvirt_internal.c: internally exported APIs, not for public use + * + * Copyright (C) 2013 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */ + +#include <config.h> +#include "libvirt_internal.h" + +#include "datatypes.h" +#include "viralloc.h" + +#define VIR_FROM_THIS VIR_FROM_NONE + +void +virStorageEphemeralFree(virStorageEphemeralPtr def) +{ + if (!def) + return; + + if (!def->pool) { + VIR_FREE(def); + return; + } + + if (def->pool->conn->storageDriver->storageEphemeralFree) { + def->pool->conn->storageDriver->storageEphemeralFree(def); + return; + } + + return; +} + + +virStorageEphemeralPtr +virStorageEphemeralFromDiskDef(virConnectPtr conn, + virDomainDiskDefPtr def) +{ + if (conn->storageDriver->storageEphemeralFromDiskDef) + return conn->storageDriver->storageEphemeralFromDiskDef(conn, def); + + virReportError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + return NULL; +} + + +virStorageEphemeralPtr +virStorageEphemeralFromSnapshotDiskDef(virConnectPtr conn, + virDomainSnapshotDiskDefPtr def) +{ + if (conn->storageDriver->storageEphemeralFromSnapshotDiskDef) + return conn->storageDriver->storageEphemeralFromSnapshotDiskDef(conn, def); + + virReportError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + return NULL; + +} diff --git a/src/libvirt_internal.h b/src/libvirt_internal.h index 115d8d1..0db46d8 100644 --- a/src/libvirt_internal.h +++ b/src/libvirt_internal.h @@ -27,6 +27,9 @@ # include "internal.h" +# include "conf/domain_conf.h" +# include "conf/snapshot_conf.h" + typedef void (*virStateInhibitCallback)(bool inhibit, void *opaque); @@ -280,4 +283,23 @@ int virDomainMigrateConfirm3Params(virDomainPtr domain, int cookieinlen, unsigned int flags, int cancelled); + +/* storage related internal stuff */ +typedef struct _virStorageEphemeral virStorageEphemeral; +typedef virStorageEphemeral *virStorageEphemeralPtr; + +struct _virStorageEphemeral +{ + bool existing; + virStorageVolPtr vol; + virStoragePoolPtr pool; +}; + +void virStorageEphemeralFree(virStorageEphemeralPtr def); + +virStorageEphemeralPtr virStorageEphemeralFromDiskDef(virConnectPtr conn, + virDomainDiskDefPtr def); + +virStorageEphemeralPtr virStorageEphemeralFromSnapshotDiskDef(virConnectPtr conn, + virDomainSnapshotDiskDefPtr def); #endif diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3b3de15..445fa94 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -814,6 +814,9 @@ virRegisterNodeDeviceDriver; virRegisterNWFilterDriver; virRegisterSecretDriver; virRegisterStorageDriver; +virStorageEphemeralFree; +virStorageEphemeralFromDiskDef; +virStorageEphemeralFromSnapshotDiskDef; # locking/domain_lock.h -- 1.8.5.2

On 01/09/2014 09:15 AM, Peter Krempa wrote:
If a VM driver wants to access stuff provided by a storage driver the volume needs to be a part of a storage pool. As this wasn't designed in from the beginning we need a way to convert generic domain disk and snapshot disk definitions into temporary pools and volumes. This patch allows that by adding private storage driver APIs that can be used to obtain a pool and vol definition. --- docs/hvsupport.pl | 3 ++ src/Makefile.am | 3 +- src/check-drivername.pl | 1 + src/driver.h | 13 +++++++++ src/libvirt_internal.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++ src/libvirt_internal.h | 22 +++++++++++++++ src/libvirt_private.syms | 3 ++ 7 files changed, 115 insertions(+), 1 deletion(-) create mode 100644 src/libvirt_internal.c
diff --git a/docs/hvsupport.pl b/docs/hvsupport.pl index f8483f9..a7d1f6b 100755 --- a/docs/hvsupport.pl +++ b/docs/hvsupport.pl @@ -176,6 +176,9 @@ $apis{virDomainMigratePerform3Params} = "1.1.0"; $apis{virDomainMigrateFinish3Params} = "1.1.0"; $apis{virDomainMigrateConfirm3Params} = "1.1.0";
+$apis{virStorageEphemeralFree} = "1.2.1"; +$apis{virStorageEphemeralFromDiskDef} = "1.2.1"; +$apis{virStorageEphemeralFromSnapshotDiskDef} = "1.2.1";
Missed 1.2.1 (this is definitely post-release material), but yeah, I've always wanted to have a way to have domains be able to refer to polymorphic storage volume APIs that just do the right thing for a variety of storage types, instead of recoding things in two locations.
+++ b/src/libvirt_internal.c @@ -0,0 +1,71 @@ +/* + * libvirt_internal.c: internally exported APIs, not for public use + * + * Copyright (C) 2013 Red Hat, Inc.
2014, now :)
+virStorageEphemeralPtr +virStorageEphemeralFromDiskDef(virConnectPtr conn, + virDomainDiskDefPtr def) +{ + if (conn->storageDriver->storageEphemeralFromDiskDef) + return conn->storageDriver->storageEphemeralFromDiskDef(conn, def); + + virReportError(VIR_ERR_NO_SUPPORT, __FUNCTION__);
My recent cleanup series to libvirt.c include virReportUnsupportedError() insated of raw use of this error type.
+ return NULL; +} + + +virStorageEphemeralPtr +virStorageEphemeralFromSnapshotDiskDef(virConnectPtr conn, + virDomainSnapshotDiskDefPtr def) +{ + if (conn->storageDriver->storageEphemeralFromSnapshotDiskDef) + return conn->storageDriver->storageEphemeralFromSnapshotDiskDef(conn, def); + + virReportError(VIR_ERR_NO_SUPPORT, __FUNCTION__);
and again. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

Add support for specifying various types when doing snapshots. This will later allow to do snapshots on network backed volumes. Disks of type 'volume' are not supported by snapshots (yet). --- docs/formatsnapshot.html.in | 15 ++++++++ docs/schemas/domainsnapshot.rng | 76 +++++++++++++++++++++++++++++++++++------ src/conf/snapshot_conf.c | 25 +++++++++++--- src/conf/snapshot_conf.h | 15 ++++---- src/qemu/qemu_driver.c | 59 +++++++++++++++++++++----------- 5 files changed, 149 insertions(+), 41 deletions(-) diff --git a/docs/formatsnapshot.html.in b/docs/formatsnapshot.html.in index 76689cb..c2cd18c 100644 --- a/docs/formatsnapshot.html.in +++ b/docs/formatsnapshot.html.in @@ -170,6 +170,21 @@ snapshots, the original file name becomes the read-only snapshot, and the new file name contains the read-write delta of all disk changes since the snapshot. + + <span class="since">Since 1.2.2</span> the <code>disk</code> element + supports an optional attribute <code>type</code> if the + <code>snapshot</code> attribute is set to <code>external</code>. + This attribute specifies the snapshot target storage type and allows + to overwrite the default <code>file</code>type. The <code>type</code> + attribute along with the format of the <code>source</code> + sub-element is identical to the <code>source</code> element used in + domain disk definitions. See the + <a href="formatdomain.html#elementsDisks">disk devices</a> section + documentation for further information. + + Libvirt currently supports the <code>type</code> element in the qemu + driver and supported values are <code>file</code> and + <code>block</code> <span class="since">(since 1.2.2)</span>. </dd> </dl> </dd> diff --git a/docs/schemas/domainsnapshot.rng b/docs/schemas/domainsnapshot.rng index 169fcfb..de9e788 100644 --- a/docs/schemas/domainsnapshot.rng +++ b/docs/schemas/domainsnapshot.rng @@ -123,19 +123,73 @@ <value>external</value> </attribute> </optional> - <interleave> - <ref name='disksnapshotdriver'/> - <optional> - <element name='source'> + <choice> + <group> + <optional> + <attribute name='type'> + <value>file</value> + </attribute> + </optional> + <interleave> <optional> - <attribute name='file'> - <ref name='absFilePath'/> - </attribute> + <element name='source'> + <optional> + <attribute name='file'> + <ref name='absFilePath'/> + </attribute> + </optional> + <empty/> + </element> </optional> - <empty/> - </element> - </optional> - </interleave> + <ref name='disksnapshotdriver'/> + </interleave> + </group> + <group> + <attribute name='type'> + <value>block</value> + </attribute> + <interleave> + <optional> + <element name="source"> + <attribute name="dev"> + <ref name="absFilePath"/> + </attribute> + <empty/> + </element> + </optional> + <ref name='disksnapshotdriver'/> + </interleave> + </group> + <group> + <attribute name="type"> + <value>dir</value> + </attribute> + <interleave> + <optional> + <element name="source"> + <attribute name="dir"> + <ref name="absFilePath"/> + </attribute> + <empty/> + </element> + </optional> + <ref name='disksnapshotdriver'/> + </interleave> + </group> + <group> + <attribute name="type"> + <value>network</value> + </attribute> + <interleave> + <optional> + <element name="source"> + <ref name='diskSourceNetwork'/> + </element> + </optional> + <ref name='disksnapshotdriver'/> + </interleave> + </group> + </choice> </group> </choice> </element> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index c18b99b..6ce65f6 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -108,6 +108,7 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node, { int ret = -1; char *snapshot = NULL; + char *type = NULL; xmlNodePtr cur; def->name = virXMLPropString(node, "name"); @@ -129,6 +130,14 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node, } def->type = -1; + if ((type = virXMLPropString(node, "type"))) { + if ((def->type = virDomainDiskTypeFromString(type)) < 0 || + def->type == VIR_DOMAIN_DISK_TYPE_VOLUME) { + virReportError(VIR_ERR_XML_ERROR, + _("unknown disk snapshot type '%s'"), type); + goto cleanup; + } + } for (cur = node->children; cur; cur = cur->next) { if (cur->type != XML_ELEMENT_NODE) @@ -145,9 +154,9 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node, if (virDomainDiskSourceDefParse(cur, backingtype, &def->file, - NULL, - NULL, - NULL, + &def->protocol, + &def->nhosts, + &def->hosts, NULL) < 0) goto cleanup; @@ -174,6 +183,7 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node, ret = 0; cleanup: VIR_FREE(snapshot); + VIR_FREE(type); if (ret < 0) virDomainSnapshotDiskDefClear(def); return ret; @@ -616,6 +626,9 @@ virDomainSnapshotDiskDefFormat(virBufferPtr buf, if (type < 0) type = VIR_DOMAIN_DISK_TYPE_FILE; + else + virBufferAsprintf(buf, " type='%s'", + virDomainDiskTypeToString(type)); if (!disk->file && disk->format == 0) { virBufferAddLit(buf, "/>\n"); @@ -630,7 +643,11 @@ virDomainSnapshotDiskDefFormat(virBufferPtr buf, virDomainDiskSourceDefFormatInternal(buf, type, disk->file, - 0, 0, 0, NULL, 0, NULL, NULL, 0); + 0, + disk->protocol, + disk->nhosts, + disk->hosts, + 0, NULL, NULL, 0); virBufferAddLit(buf, " </disk>\n"); } diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h index 241d63c..bcd92dc 100644 --- a/src/conf/snapshot_conf.h +++ b/src/conf/snapshot_conf.h @@ -48,12 +48,15 @@ enum virDomainSnapshotState { typedef struct _virDomainSnapshotDiskDef virDomainSnapshotDiskDef; typedef virDomainSnapshotDiskDef *virDomainSnapshotDiskDefPtr; struct _virDomainSnapshotDiskDef { - char *name; /* name matching the <target dev='...' of the domain */ - int index; /* index within snapshot->dom->disks that matches name */ - int snapshot; /* enum virDomainSnapshotLocation */ - int type; /* enum virDomainDiskType */ - char *file; /* new source file when snapshot is external */ - int format; /* enum virStorageFileFormat */ + char *name; /* name matching the <target dev='...' of the domain */ + int index; /* index within snapshot->dom->disks that matches name */ + int snapshot; /* enum virDomainSnapshotLocation */ + int type; /* enum virDomainDiskType */ + char *file; /* new source file when snapshot is external */ + int format; /* enum virStorageFileFormat */ + int protocol; /* network source protocol */ + size_t nhosts; /* network source hosts count */ + virDomainDiskHostDefPtr hosts; /* network source hosts */ }; /* Stores the complete snapshot metadata */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1949abe..2afcb0a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12205,33 +12205,48 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, } if (virAsprintf(&device, "drive-%s", disk->info.alias) < 0 || - VIR_STRDUP(source, snap->file) < 0 || (persistDisk && VIR_STRDUP(persistSource, source) < 0)) goto cleanup; - /* create the stub file and set selinux labels; manipulate disk in - * place, in a way that can be reverted on failure. */ - if (!reuse) { - fd = qemuOpenFile(driver, vm, source, O_WRONLY | O_TRUNC | O_CREAT, - &need_unlink, NULL); - if (fd < 0) - goto cleanup; - VIR_FORCE_CLOSE(fd); - } - /* XXX Here, we know we are about to alter disk->backingChain if - * successful, so we nuke the existing chain so that future - * commands will recompute it. Better would be storing the chain - * ourselves rather than reprobing, but this requires modifying - * domain_conf and our XML to fully track the chain across - * libvirtd restarts. */ + * successful, so we nuke the existing chain so that future commands will + * recompute it. Better would be storing the chain ourselves rather than + * reprobing, but this requires modifying domain_conf and our XML to fully + * track the chain across libvirtd restarts. */ virStorageFileFreeMetadata(disk->backingChain); disk->backingChain = NULL; - if (qemuDomainPrepareDiskChainElement(driver, vm, disk, source, - VIR_DISK_CHAIN_READ_WRITE) < 0) { - qemuDomainPrepareDiskChainElement(driver, vm, disk, source, - VIR_DISK_CHAIN_NO_ACCESS); + switch (snap->type) { + case VIR_DOMAIN_DISK_TYPE_BLOCK: + reuse = true; + /* fallthrough */ + case -1: /* type was not provided in snapshot conf */ + case VIR_DOMAIN_DISK_TYPE_FILE: + if (VIR_STRDUP(source, snap->file) < 0) + goto cleanup; + + /* create the stub file and set selinux labels; manipulate disk in + * place, in a way that can be reverted on failure. */ + if (!reuse) { + fd = qemuOpenFile(driver, vm, source, O_WRONLY | O_TRUNC | O_CREAT, + &need_unlink, NULL); + if (fd < 0) + goto cleanup; + VIR_FORCE_CLOSE(fd); + } + + if (qemuDomainPrepareDiskChainElement(driver, vm, disk, source, + VIR_DISK_CHAIN_READ_WRITE) < 0) { + qemuDomainPrepareDiskChainElement(driver, vm, disk, source, + VIR_DISK_CHAIN_NO_ACCESS); + goto cleanup; + } + break; + + default: + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("snapshots are not supported on '%s' volumes"), + virDomainDiskTypeToString(snap->type)); goto cleanup; } @@ -12250,11 +12265,13 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, disk->src = source; source = NULL; disk->format = format; + disk->type = snap->type; if (persistDisk) { VIR_FREE(persistDisk->src); persistDisk->src = persistSource; persistSource = NULL; persistDisk->format = format; + persistDisk->type = snap->type; } cleanup: @@ -12296,11 +12313,13 @@ qemuDomainSnapshotUndoSingleDiskActive(virQEMUDriverPtr driver, disk->src = source; source = NULL; disk->format = origdisk->format; + disk->type = origdisk->type; if (persistDisk) { VIR_FREE(persistDisk->src); persistDisk->src = persistSource; persistSource = NULL; persistDisk->format = origdisk->format; + persistDisk->type = origdisk->type; } cleanup: -- 1.8.5.2

On 01/09/2014 09:15 AM, Peter Krempa wrote:
Add support for specifying various types when doing snapshots. This will later allow to do snapshots on network backed volumes. Disks of type 'volume' are not supported by snapshots (yet). --- docs/formatsnapshot.html.in | 15 ++++++++ docs/schemas/domainsnapshot.rng | 76 +++++++++++++++++++++++++++++++++++------ src/conf/snapshot_conf.c | 25 +++++++++++--- src/conf/snapshot_conf.h | 15 ++++---- src/qemu/qemu_driver.c | 59 +++++++++++++++++++++----------- 5 files changed, 149 insertions(+), 41 deletions(-)
Hopefully some tests are added later in the series to exercise the new RNG schema...
diff --git a/docs/formatsnapshot.html.in b/docs/formatsnapshot.html.in index 76689cb..c2cd18c 100644 --- a/docs/formatsnapshot.html.in +++ b/docs/formatsnapshot.html.in @@ -170,6 +170,21 @@ snapshots, the original file name becomes the read-only snapshot, and the new file name contains the read-write delta of all disk changes since the snapshot. + + <span class="since">Since 1.2.2</span> the <code>disk</code> element
Ah, here you admit the post-release nature of the patch :)
@@ -616,6 +626,9 @@ virDomainSnapshotDiskDefFormat(virBufferPtr buf,
if (type < 0) type = VIR_DOMAIN_DISK_TYPE_FILE; + else + virBufferAsprintf(buf, " type='%s'", + virDomainDiskTypeToString(type));
Should we always output the type, or is this a case where for back-compat, if the user omitted type, it's okay for us to omit it too? Users already have to be prepared for more xml on output than what they put on input, and being explicit about type='file' even when the user didn't specify it may help us down the road. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 01/10/14 00:30, Eric Blake wrote:
On 01/09/2014 09:15 AM, Peter Krempa wrote:
Add support for specifying various types when doing snapshots. This will later allow to do snapshots on network backed volumes. Disks of type 'volume' are not supported by snapshots (yet). --- docs/formatsnapshot.html.in | 15 ++++++++ docs/schemas/domainsnapshot.rng | 76 +++++++++++++++++++++++++++++++++++------ src/conf/snapshot_conf.c | 25 +++++++++++--- src/conf/snapshot_conf.h | 15 ++++---- src/qemu/qemu_driver.c | 59 +++++++++++++++++++++----------- 5 files changed, 149 insertions(+), 41 deletions(-)
Hopefully some tests are added later in the series to exercise the new RNG schema...
diff --git a/docs/formatsnapshot.html.in b/docs/formatsnapshot.html.in index 76689cb..c2cd18c 100644 --- a/docs/formatsnapshot.html.in +++ b/docs/formatsnapshot.html.in @@ -170,6 +170,21 @@ snapshots, the original file name becomes the read-only snapshot, and the new file name contains the read-write delta of all disk changes since the snapshot. + + <span class="since">Since 1.2.2</span> the <code>disk</code> element
Ah, here you admit the post-release nature of the patch :)
Yeah, I wrote those docs after it was clear it won't make 1.2.2 but I forgot to update the previous patch when I was posting it.
@@ -616,6 +626,9 @@ virDomainSnapshotDiskDefFormat(virBufferPtr buf,
if (type < 0) type = VIR_DOMAIN_DISK_TYPE_FILE; + else + virBufferAsprintf(buf, " type='%s'", + virDomainDiskTypeToString(type));
Should we always output the type, or is this a case where for back-compat, if the user omitted type, it's okay for us to omit it too? Users already have to be prepared for more xml on output than what they put on input, and being explicit about type='file' even when the user didn't specify it may help us down the road.
I certainly can change it to output that element always and assume type='file' if it's not present. I wasn't sure about backcompat and thus I went for extra state for the missing element. I'll change it to this approach in the next version. Peter

Amend tests to check parsing of the various new disk types that can now be specified. --- tests/domainsnapshotxml2xmlin/disk_snapshot.xml | 18 ++++++++++++++++++ tests/domainsnapshotxml2xmlout/disk_snapshot.xml | 18 ++++++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/tests/domainsnapshotxml2xmlin/disk_snapshot.xml b/tests/domainsnapshotxml2xmlin/disk_snapshot.xml index ee6b46a..aa1522a 100644 --- a/tests/domainsnapshotxml2xmlin/disk_snapshot.xml +++ b/tests/domainsnapshotxml2xmlin/disk_snapshot.xml @@ -12,5 +12,23 @@ <disk name='hde' snapshot='external'> <source file='/path/to/new'/> </disk> + <disk name='hde' snapshot='external' type='file'> + <source file='/path/to/new2'/> + </disk> + <disk name='hdf' snapshot='external' type='block'> + <source dev='/path/to/new3'/> + </disk> + <disk name='hdg' snapshot='external' type='network'> + <source protocol='gluster' name='volume/path'> + <host name='host' port='1234'/> + </source> + </disk> + <disk name='hdh' snapshot='external' type='network'> + <source protocol='rbd' name='name'> + <host name='host' port='1234'/> + <host name='host2' port='1234' transport='rdma'/> + <host name='host3' port='1234'/> + </source> + </disk> </disks> </domainsnapshot> diff --git a/tests/domainsnapshotxml2xmlout/disk_snapshot.xml b/tests/domainsnapshotxml2xmlout/disk_snapshot.xml index 1a1fc02..316df43 100644 --- a/tests/domainsnapshotxml2xmlout/disk_snapshot.xml +++ b/tests/domainsnapshotxml2xmlout/disk_snapshot.xml @@ -11,5 +11,23 @@ <disk name='hde' snapshot='external'> <source file='/path/to/new'/> </disk> + <disk name='hde' snapshot='external' type='file'> + <source file='/path/to/new2'/> + </disk> + <disk name='hdf' snapshot='external' type='block'> + <source dev='/path/to/new3'/> + </disk> + <disk name='hdg' snapshot='external' type='network'> + <source protocol='gluster' name='volume/path'> + <host name='host' port='1234'/> + </source> + </disk> + <disk name='hdh' snapshot='external' type='network'> + <source protocol='rbd' name='name'> + <host name='host' port='1234'/> + <host name='host2' port='1234' transport='rdma'/> + <host name='host3' port='1234'/> + </source> + </disk> </disks> </domainsnapshot> -- 1.8.5.2

On 01/09/2014 09:15 AM, Peter Krempa wrote:
Amend tests to check parsing of the various new disk types that can now be specified. --- tests/domainsnapshotxml2xmlin/disk_snapshot.xml | 18 ++++++++++++++++++ tests/domainsnapshotxml2xmlout/disk_snapshot.xml | 18 ++++++++++++++++++ 2 files changed, 36 insertions(+)
...Ah, this was what I was looking for in 5/10. I guess it's okay as separate patches, to keep review size down.
+ <disk name='hdh' snapshot='external' type='network'>
Wow - that VM has lots of disks :) ACK to this patch, but obviously it has to be applied as part of the series. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

This patch implements the APIs for getting temporary storage pools for the local filesystem driver using storage_backend_fs. --- src/check-aclrules.pl | 3 + src/storage/storage_driver.c | 253 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 256 insertions(+) diff --git a/src/check-aclrules.pl b/src/check-aclrules.pl index 057517e..7174868 100755 --- a/src/check-aclrules.pl +++ b/src/check-aclrules.pl @@ -73,6 +73,9 @@ my %implwhitelist = ( "xenUnifiedDomainIsPersistent" => 1, "xenUnifiedDomainIsUpdated" => 1, "xenUnifiedDomainOpenConsole" => 1, + "storageEphemeralFree" => 1, # internal API, no RPC + "storageEphemeralFromDiskDef" => 1, # internal API, no RPC + "storageEphemeralFromSnapshotDiskDef" => 1, # internal API, no RPC ); my %filterimplwhitelist = ( "xenUnifiedConnectListDomains" => 1, diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index a392d53..5927ebf 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -50,6 +50,8 @@ #include "virstring.h" #include "viraccessapicheck.h" +#include "dirname.h" + #define VIR_FROM_THIS VIR_FROM_STORAGE static virStorageDriverStatePtr driverState; @@ -2586,6 +2588,253 @@ cleanup: return ret; } + +static void +storageEphemeralFree(virStorageEphemeralPtr def) +{ + virConnectPtr conn; + virStorageDriverStatePtr driver; + virStoragePoolObjPtr pool; + + if (!def) + return; + + if (!def->existing && + def->pool) { + conn = def->pool->conn; + driver = conn->storagePrivateData; + + if ((pool = virStoragePoolObjFindByUUID(&driver->pools, + def->pool->uuid))) { + virStoragePoolObjRemove(&driver->pools, pool); + } + } + + if (def->vol) + virStorageVolFree(def->vol); + + if (def->pool) + virStoragePoolFree(def->pool); +} + + +static void +storageEphemeralGenerateUniquePoolID(virStoragePoolDefPtr def) +{ + virUUIDGenerate(def->uuid); + virUUIDFormat(def->uuid, def->name); +} + + +static virStoragePoolPtr +storageEphemeralCreateDirPool(virConnectPtr conn, + const char *source) +{ + virStorageDriverStatePtr driver = conn->storagePrivateData; + virStoragePoolDefPtr def = NULL; + virStoragePoolObjPtr pool = NULL; + virStorageBackendPtr backend; + virStoragePoolPtr ret = NULL; + + if (VIR_ALLOC(def) < 0) + goto cleanup; + + if (VIR_ALLOC_N(def->name, VIR_UUID_STRING_BUFLEN) < 0) + goto cleanup; + + if (VIR_STRDUP(def->target.path, source) < 0) + goto cleanup; + + if (VIR_STRDUP(def->source.dir, def->target.path) < 0) + goto cleanup; + + def->type = VIR_STORAGE_POOL_DIR; + + storageDriverLock(driver); + + /* generate a unique name */ + do { + if (pool) { + virStoragePoolObjUnlock(pool); + pool = NULL; + } + + storageEphemeralGenerateUniquePoolID(def); + + if ((pool = virStoragePoolObjFindByUUID(&driver->pools, def->uuid))) + continue; + + pool = virStoragePoolObjFindByName(&driver->pools, def->name); + } while (pool); + + if (!(backend = virStorageBackendForType(def->type))) + goto cleanup; + + if (!(pool = virStoragePoolObjAssignDef(&driver->pools, def))) + goto cleanup; + def = NULL; + + if (backend->startPool && + backend->startPool(conn, pool) < 0) { + virStoragePoolObjRemove(&driver->pools, pool); + pool = NULL; + goto cleanup; + } + + pool->active = 1; + pool->internal = true; + + ret = virGetStoragePool(conn, pool->def->name, pool->def->uuid, + NULL, NULL); + +cleanup: + virStoragePoolDefFree(def); + if (pool) + virStoragePoolObjUnlock(pool); + storageDriverUnlock(driver); + return ret; +} + + +static virStorageVolPtr +storageEphemeralCreateVol(virStoragePoolPtr obj, + const char *source) +{ + + virStorageDriverStatePtr driver = obj->conn->storagePrivateData; + virStoragePoolObjPtr pool; + virStorageBackendPtr backend; + virStorageVolDefPtr vol = NULL; + virStorageVolPtr ret = NULL; + + storageDriverLock(driver); + pool = virStoragePoolObjFindByUUID(&driver->pools, obj->uuid); + storageDriverUnlock(driver); + + if (!pool) { + virReportError(VIR_ERR_NO_STORAGE_POOL, + _("no storage pool with matching uuid %s"), obj->uuid); + goto cleanup; + } + + if ((backend = virStorageBackendForType(pool->def->type)) == NULL) + goto cleanup; + + if (VIR_ALLOC(vol) < 0) + goto cleanup; + + if (VIR_STRDUP(vol->name, source) < 0) + goto cleanup; + + if (backend->createVol(obj->conn, pool, vol, true) < 0) + goto cleanup; + + if (VIR_APPEND_ELEMENT(pool->volumes.objs, pool->volumes.count, vol) < 0) + goto cleanup; + + if (!(ret = virGetStorageVol(obj->conn, pool->def->name, + pool->volumes.objs[pool->volumes.count - 1]->name, + pool->volumes.objs[pool->volumes.count - 1]->key, + NULL, NULL))) { + vol = pool->volumes.objs[pool->volumes.count--]; + goto cleanup; + } + +cleanup: + virStorageVolDefFree(vol); + if (pool) + virStoragePoolObjUnlock(pool); + return ret; +} + + +static virStorageEphemeralPtr +storageEphemeralCreate(virConnectPtr conn, + int type, + const char *source, + virDomainDiskSourcePoolDefPtr srcpool) +{ + virStorageEphemeralPtr ret = NULL; + char *dirname = NULL; + char *filename = NULL; + + if (VIR_ALLOC(ret) < 0) + goto error; + + switch ((enum virDomainDiskType) type) { + case VIR_DOMAIN_DISK_TYPE_LAST: + case VIR_DOMAIN_DISK_TYPE_BLOCK: + case VIR_DOMAIN_DISK_TYPE_FILE: + case VIR_DOMAIN_DISK_TYPE_DIR: + if (!(dirname = mdir_name(source))) { + virReportOOMError(); + goto error; + } + + if (VIR_STRDUP(filename, last_component(source)) < 1) + goto error; + + if (!(ret->pool = storageEphemeralCreateDirPool(conn, dirname))) + goto error; + + if (!(ret->vol = storageEphemeralCreateVol(ret->pool, filename))) + goto error; + + break; + + case VIR_DOMAIN_DISK_TYPE_NETWORK: + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("ephemeral network based volumes are not yet supported")); + goto error; + break; + + case VIR_DOMAIN_DISK_TYPE_VOLUME: + ret->existing = true; + + if (!(ret->pool = virStoragePoolLookupByName(conn, srcpool->pool))) + goto error; + + if (!(ret->vol = virStorageVolLookupByName(ret->pool, srcpool->volume))) + goto error; + + break; + } + +cleanup: + VIR_FREE(dirname); + VIR_FREE(filename); + + return ret; + +error: + storageEphemeralFree(ret); + ret = NULL; + goto cleanup; +} + + +static virStorageEphemeralPtr +storageEphemeralFromDiskDef(virConnectPtr conn, + virDomainDiskDefPtr def) +{ + return storageEphemeralCreate(conn, + def->type, + def->src, + def->srcpool); + +} + + +static virStorageEphemeralPtr +storageEphemeralFromSnapshotDiskDef(virConnectPtr conn, + virDomainSnapshotDiskDefPtr def) +{ + return storageEphemeralCreate(conn, + def->type, + def->file, + NULL); +} + static virStorageDriver storageDriver = { .name = "storage", .storageOpen = storageOpen, /* 0.4.0 */ @@ -2632,6 +2881,10 @@ static virStorageDriver storageDriver = { .storagePoolIsActive = storagePoolIsActive, /* 0.7.3 */ .storagePoolIsPersistent = storagePoolIsPersistent, /* 0.7.3 */ + + .storageEphemeralFree = storageEphemeralFree, /* 1.2.1 */ + .storageEphemeralFromDiskDef = storageEphemeralFromDiskDef, /* 1.2.1 */ + .storageEphemeralFromSnapshotDiskDef = storageEphemeralFromSnapshotDiskDef, /* 1.2.1 */ }; -- 1.8.5.2

On 01/09/2014 09:15 AM, Peter Krempa wrote:
This patch implements the APIs for getting temporary storage pools for the local filesystem driver using storage_backend_fs. --- src/check-aclrules.pl | 3 + src/storage/storage_driver.c | 253 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 256 insertions(+)
+ /* generate a unique name */ + do { + if (pool) { + virStoragePoolObjUnlock(pool); + pool = NULL; + } + + storageEphemeralGenerateUniquePoolID(def); + + if ((pool = virStoragePoolObjFindByUUID(&driver->pools, def->uuid))) + continue; + + pool = virStoragePoolObjFindByName(&driver->pools, def->name); + } while (pool);
This avoids a collision with any existing pools, but what if we pick a name that a later public API also picks - will the public API be rejected, or will our internal pool be automatically upgraded to be tagged as an existing pool? (Not that generated UUIDs are likely to collide, but you never know...)
@@ -2632,6 +2881,10 @@ static virStorageDriver storageDriver = {
.storagePoolIsActive = storagePoolIsActive, /* 0.7.3 */ .storagePoolIsPersistent = storagePoolIsPersistent, /* 0.7.3 */ + + .storageEphemeralFree = storageEphemeralFree, /* 1.2.1 */ + .storageEphemeralFromDiskDef = storageEphemeralFromDiskDef, /* 1.2.1 */ + .storageEphemeralFromSnapshotDiskDef = storageEphemeralFromSnapshotDiskDef, /* 1.2.1 */
1.2.2 -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 01/10/14 00:39, Eric Blake wrote:
On 01/09/2014 09:15 AM, Peter Krempa wrote:
This patch implements the APIs for getting temporary storage pools for the local filesystem driver using storage_backend_fs. --- src/check-aclrules.pl | 3 + src/storage/storage_driver.c | 253 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 256 insertions(+)
+ /* generate a unique name */ + do { + if (pool) { + virStoragePoolObjUnlock(pool); + pool = NULL; + } + + storageEphemeralGenerateUniquePoolID(def); + + if ((pool = virStoragePoolObjFindByUUID(&driver->pools, def->uuid))) + continue; + + pool = virStoragePoolObjFindByName(&driver->pools, def->name); + } while (pool);
This avoids a collision with any existing pools, but what if we pick a name that a later public API also picks - will the public API be rejected, or will our internal pool be automatically upgraded to be tagged as an existing pool? (Not that generated UUIDs are likely to collide, but you never know...)
The new user defined pool will be rejected in that case. I think that the chance of collision is minimal (but still existing). We could also dedicate a forbidden prefix or look for characters forbidden in the RNG schema and use those as a prefix to avoid "legitimate" collisions.
Peter

Add code to convert an regular guest volume into a ephemeral pool. --- src/storage/storage_driver.c | 153 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 150 insertions(+), 3 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 5927ebf..9b1aa9c 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2627,6 +2627,106 @@ storageEphemeralGenerateUniquePoolID(virStoragePoolDefPtr def) static virStoragePoolPtr +storageEphemeralCreateGlusterPool(virConnectPtr conn, + const char *source, + virDomainDiskHostDefPtr host) +{ + virStorageDriverStatePtr driver = conn->storagePrivateData; + virStoragePoolDefPtr def = NULL; + virStoragePoolObjPtr pool = NULL; + virStorageBackendPtr backend; + virStoragePoolPtr ret = NULL; + + char *volume = NULL; + char *path = NULL; + char *tmp; + + if (VIR_STRDUP(volume, source) < 0) + goto cleanup; + + if ((tmp = strchr(volume, '/'))) + *tmp++ = '0'; + + if (tmp && virAsprintf(&path, "/%s", tmp) < 0) + goto cleanup; + + if (VIR_ALLOC(def) < 0) + goto cleanup; + + if (VIR_ALLOC_N(def->source.hosts, 1)) + goto cleanup; + + def->source.nhost = 1; + + if (VIR_STRDUP(def->source.hosts->name, host->name) < 0) + goto cleanup; + + if (host->port && + virStrToLong_i(host->port, NULL, 10, &def->source.hosts->port) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("failed to parse port number '%s'"), + host->port); + goto cleanup; + } + + if (VIR_ALLOC_N(def->name, VIR_UUID_STRING_BUFLEN) < 0) + goto cleanup; + + def->source.name = volume; + volume = NULL; + + def->source.dir = path; + path = NULL; + + def->type = VIR_STORAGE_POOL_GLUSTER; + + storageDriverLock(driver); + + /* generate a unique name */ + do { + if (pool) { + virStoragePoolObjUnlock(pool); + pool = NULL; + } + + storageEphemeralGenerateUniquePoolID(def); + + if ((pool = virStoragePoolObjFindByUUID(&driver->pools, def->uuid))) + continue; + + pool = virStoragePoolObjFindByName(&driver->pools, def->name); + } while (pool); + + if (!(backend = virStorageBackendForType(def->type))) + goto cleanup; + + if (!(pool = virStoragePoolObjAssignDef(&driver->pools, def))) + goto cleanup; + def = NULL; + + if (backend->startPool && + backend->startPool(conn, pool) < 0) { + virStoragePoolObjRemove(&driver->pools, pool); + pool = NULL; + goto cleanup; + } + + pool->active = 1; + + ret = virGetStoragePool(conn, pool->def->name, pool->def->uuid, + NULL, NULL); + +cleanup: + virStoragePoolDefFree(def); + if (pool) + virStoragePoolObjUnlock(pool); + storageDriverUnlock(driver); + return ret; +} + + + +static virStoragePoolPtr storageEphemeralCreateDirPool(virConnectPtr conn, const char *source) { @@ -2752,6 +2852,9 @@ static virStorageEphemeralPtr storageEphemeralCreate(virConnectPtr conn, int type, const char *source, + int protocol, + size_t nhosts, + virDomainDiskHostDefPtr hosts, virDomainDiskSourcePoolDefPtr srcpool) { virStorageEphemeralPtr ret = NULL; @@ -2783,9 +2886,47 @@ storageEphemeralCreate(virConnectPtr conn, break; case VIR_DOMAIN_DISK_TYPE_NETWORK: - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("ephemeral network based volumes are not yet supported")); - goto error; + switch ((enum virDomainDiskProtocol) protocol) { + case VIR_DOMAIN_DISK_PROTOCOL_GLUSTER: + if (nhosts != 1) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Gluster disk supports only 1 source")); + goto error; + } + + if (!(dirname = mdir_name(source))) { + virReportOOMError(); + goto error; + } + + if (VIR_STRDUP(filename, last_component(source)) < 1) + goto error; + + if (!(ret->pool = storageEphemeralCreateGlusterPool(conn, dirname, hosts))) + goto error; + + if (!(ret->vol = storageEphemeralCreateVol(ret->pool, filename))) + goto error; + + break; + + case VIR_DOMAIN_DISK_PROTOCOL_NBD: + case VIR_DOMAIN_DISK_PROTOCOL_RBD: + case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG: + case VIR_DOMAIN_DISK_PROTOCOL_ISCSI: + case VIR_DOMAIN_DISK_PROTOCOL_HTTP: + case VIR_DOMAIN_DISK_PROTOCOL_HTTPS: + case VIR_DOMAIN_DISK_PROTOCOL_FTP: + case VIR_DOMAIN_DISK_PROTOCOL_FTPS: + case VIR_DOMAIN_DISK_PROTOCOL_TFTP: + case VIR_DOMAIN_DISK_PROTOCOL_LAST: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("ephemeral network based volumes using '%s' " + "protocol are not yet supported"), + virDomainDiskProtocolTypeToString(protocol)); + goto error; + break; + } break; case VIR_DOMAIN_DISK_TYPE_VOLUME: @@ -2820,6 +2961,9 @@ storageEphemeralFromDiskDef(virConnectPtr conn, return storageEphemeralCreate(conn, def->type, def->src, + def->protocol, + def->nhosts, + def->hosts, def->srcpool); } @@ -2832,6 +2976,9 @@ storageEphemeralFromSnapshotDiskDef(virConnectPtr conn, return storageEphemeralCreate(conn, def->type, def->file, + def->protocol, + def->nhosts, + def->hosts, NULL); } -- 1.8.5.2

On 01/09/2014 09:15 AM, Peter Krempa wrote:
Add code to convert an regular guest volume into a ephemeral pool.
s/an regular/a regular/
+ + if (VIR_STRDUP(volume, source) < 0) + goto cleanup; + + if ((tmp = strchr(volume, '/'))) + *tmp++ = '0';
'0'? Don't you mean '\0'?
+ if (backend->startPool && + backend->startPool(conn, pool) < 0) { + virStoragePoolObjRemove(&driver->pools, pool); + pool = NULL; + goto cleanup; + } + + pool->active = 1;
Don't you also need to set the ephemeral bit? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

Use the new storage conversion APIs to delete garbage left behind after a failed snapshot attempt using the storage driver. --- src/qemu/qemu_driver.c | 60 ++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 46 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2afcb0a..f9ffe2e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12180,7 +12180,8 @@ cleanup: /* The domain is expected to hold monitor lock. */ static int -qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, +qemuDomainSnapshotCreateSingleDiskActive(virConnectPtr conn, + virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainSnapshotDiskDefPtr snap, virDomainDiskDefPtr disk, @@ -12197,6 +12198,7 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, int ret = -1; int fd = -1; bool need_unlink = false; + virStorageEphemeralPtr temppool = NULL; if (snap->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -12216,6 +12218,9 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, virStorageFileFreeMetadata(disk->backingChain); disk->backingChain = NULL; + if (!(temppool = virStorageEphemeralFromSnapshotDiskDef(conn, snap))) + goto cleanup; + switch (snap->type) { case VIR_DOMAIN_DISK_TYPE_BLOCK: reuse = true; @@ -12275,8 +12280,16 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, } cleanup: - if (need_unlink && unlink(source)) - VIR_WARN("unable to unlink just-created %s", source); + if (need_unlink) { + if (virStorageVolDelete(temppool->vol, 0) < 0) { + char *path = virStorageVolGetPath(temppool->vol); + VIR_WARN("unable to remove just-created snapshot storage '%s'", + NULLSTR(path)); + VIR_FREE(path); + } + } + + virStorageEphemeralFree(temppool); VIR_FREE(device); VIR_FREE(source); VIR_FREE(persistSource); @@ -12287,7 +12300,8 @@ cleanup: * counterpart to qemuDomainSnapshotCreateSingleDiskActive, called * only on a failed transaction. */ static void -qemuDomainSnapshotUndoSingleDiskActive(virQEMUDriverPtr driver, +qemuDomainSnapshotUndoSingleDiskActive(virConnectPtr conn, + virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDiskDefPtr origdisk, virDomainDiskDefPtr disk, @@ -12296,17 +12310,33 @@ qemuDomainSnapshotUndoSingleDiskActive(virQEMUDriverPtr driver, { char *source = NULL; char *persistSource = NULL; - struct stat st; + + virStorageEphemeralPtr temppool = NULL; if (VIR_STRDUP(source, origdisk->src) < 0 || (persistDisk && VIR_STRDUP(persistSource, source) < 0)) goto cleanup; - qemuDomainPrepareDiskChainElement(driver, vm, disk, disk->src, - VIR_DISK_CHAIN_NO_ACCESS); - if (need_unlink && stat(disk->src, &st) == 0 && - S_ISREG(st.st_mode) && unlink(disk->src) < 0) - VIR_WARN("Unable to remove just-created %s", disk->src); + switch (origdisk->type) { + case VIR_DOMAIN_DISK_TYPE_BLOCK: + case -1: /* type was not provided in snapshot conf */ + case VIR_DOMAIN_DISK_TYPE_FILE: + qemuDomainPrepareDiskChainElement(driver, vm, disk, disk->src, + VIR_DISK_CHAIN_NO_ACCESS); + break; + + default: + break; + } + + temppool = virStorageEphemeralFromDiskDef(conn, disk); + + if (need_unlink && temppool && + virStorageVolDelete(temppool->vol, 0) < 0) { + char *path = virStorageVolGetPath(temppool->vol); + VIR_WARN("Unable to remove just-created '%s'", NULLSTR(path)); + VIR_FREE(path); + } /* Update vm in place to match changes. */ VIR_FREE(disk->src); @@ -12323,13 +12353,15 @@ qemuDomainSnapshotUndoSingleDiskActive(virQEMUDriverPtr driver, } cleanup: + virStorageEphemeralFree(temppool); VIR_FREE(source); VIR_FREE(persistSource); } /* The domain is expected to be locked and active. */ static int -qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, +qemuDomainSnapshotCreateDiskActive(virConnectPtr conn, + virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainSnapshotObjPtr snap, unsigned int flags, @@ -12382,7 +12414,7 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, } } - ret = qemuDomainSnapshotCreateSingleDiskActive(driver, vm, + ret = qemuDomainSnapshotCreateSingleDiskActive(conn, driver, vm, &snap->def->disks[i], vm->def->disks[i], persistDisk, actions, @@ -12411,7 +12443,7 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, persistDisk = vm->newDef->disks[indx]; } - qemuDomainSnapshotUndoSingleDiskActive(driver, vm, + qemuDomainSnapshotUndoSingleDiskActive(conn, driver, vm, snap->def->dom->disks[i], vm->def->disks[i], persistDisk, @@ -12554,7 +12586,7 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr conn, * * Next we snapshot the disks. */ - if ((ret = qemuDomainSnapshotCreateDiskActive(driver, vm, snap, flags, + if ((ret = qemuDomainSnapshotCreateDiskActive(conn, driver, vm, snap, flags, QEMU_ASYNC_JOB_SNAPSHOT)) < 0) goto endjob; -- 1.8.5.2

On 01/09/2014 09:15 AM, Peter Krempa wrote:
Use the new storage conversion APIs to delete garbage left behind after a failed snapshot attempt using the storage driver. --- src/qemu/qemu_driver.c | 60 ++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 46 insertions(+), 14 deletions(-)
cleanup: - if (need_unlink && unlink(source)) - VIR_WARN("unable to unlink just-created %s", source); + if (need_unlink) { + if (virStorageVolDelete(temppool->vol, 0) < 0) {
YES! This is where sticking the polymorphism in the storage pool backends, so that the front-ends call simple wrappers without knowing whether it wraps unlink() or a complex series of gluster API calls, proves that we are finally getting the separation of design correct. There's probably a lot more places where we can/should make use of ephemeral pools, but this is a step in the right direction. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

Add support for gluster backed images as sources for snapshots in the qemu driver. This will also simplify adding further network backed volumes as sources for snapshot in case qemu will support them. --- docs/formatsnapshot.html.in | 4 +- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_command.h | 9 ++++ src/qemu/qemu_driver.c | 113 +++++++++++++++++++++++++++++++++++++++++--- 4 files changed, 118 insertions(+), 10 deletions(-) diff --git a/docs/formatsnapshot.html.in b/docs/formatsnapshot.html.in index c2cd18c..d990c39 100644 --- a/docs/formatsnapshot.html.in +++ b/docs/formatsnapshot.html.in @@ -183,8 +183,8 @@ documentation for further information. Libvirt currently supports the <code>type</code> element in the qemu - driver and supported values are <code>file</code> and - <code>block</code> <span class="since">(since 1.2.2)</span>. + driver and supported values are <code>file</code>, <code>block</code> + and <code>gluster</code> <span class="since">(since 1.2.2)</span>. </dd> </dl> </dd> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 35b7c67..4607740 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3822,7 +3822,7 @@ cleanup: } -static int +int qemuGetDriveSourceString(int type, const char *src, int protocol, diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 66c23cc..ec944ea 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -312,4 +312,13 @@ qemuParseKeywords(const char *str, int *retnkeywords, int allowEmptyValue); +int qemuGetDriveSourceString(int type, + const char *src, + int protocol, + size_t nhosts, + virDomainDiskHostDefPtr hosts, + const char *username, + const char *secret, + char **path); + #endif /* __QEMU_COMMAND_H__*/ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f9ffe2e..a57c0be 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11502,6 +11502,24 @@ cleanup: return ret; } + +static int +qemuDomainSnapshotDiskGetSourceString(virDomainSnapshotDiskDefPtr disk, + char **source) +{ + *source = NULL; + + return qemuGetDriveSourceString(qemuSnapshotDiskGetActualType(disk), + disk->file, + disk->protocol, + disk->nhosts, + disk->hosts, + NULL, + NULL, + source); +} + + typedef enum { VIR_DISK_CHAIN_NO_ACCESS, VIR_DISK_CHAIN_READ_ONLY, @@ -11882,6 +11900,29 @@ qemuDomainSnapshotPrepareDiskExternalOverlayActive(virDomainSnapshotDiskDefPtr d return 0; case VIR_DOMAIN_DISK_TYPE_NETWORK: + switch ((enum virDomainDiskProtocol) disk->protocol) { + case VIR_DOMAIN_DISK_PROTOCOL_GLUSTER: + return 0; + + case VIR_DOMAIN_DISK_PROTOCOL_NBD: + case VIR_DOMAIN_DISK_PROTOCOL_RBD: + case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG: + case VIR_DOMAIN_DISK_PROTOCOL_ISCSI: + case VIR_DOMAIN_DISK_PROTOCOL_HTTP: + case VIR_DOMAIN_DISK_PROTOCOL_HTTPS: + case VIR_DOMAIN_DISK_PROTOCOL_FTP: + case VIR_DOMAIN_DISK_PROTOCOL_FTPS: + case VIR_DOMAIN_DISK_PROTOCOL_TFTP: + case VIR_DOMAIN_DISK_PROTOCOL_LAST: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("external active snapshots are not supported on " + "'network' disks using '%s' protocol"), + virDomainDiskProtocolTypeToString(disk->protocol)); + return -1; + + } + break; + case VIR_DOMAIN_DISK_TYPE_DIR: case VIR_DOMAIN_DISK_TYPE_VOLUME: case VIR_DOMAIN_DISK_TYPE_LAST: @@ -12192,6 +12233,9 @@ qemuDomainSnapshotCreateSingleDiskActive(virConnectPtr conn, qemuDomainObjPrivatePtr priv = vm->privateData; char *device = NULL; char *source = NULL; + char *newsource = NULL; + virDomainDiskHostDefPtr newhosts = NULL; + virDomainDiskHostDefPtr persistHosts = NULL; int format = snap->format; const char *formatStr = NULL; char *persistSource = NULL; @@ -12206,8 +12250,7 @@ qemuDomainSnapshotCreateSingleDiskActive(virConnectPtr conn, return -1; } - if (virAsprintf(&device, "drive-%s", disk->info.alias) < 0 || - (persistDisk && VIR_STRDUP(persistSource, source) < 0)) + if (virAsprintf(&device, "drive-%s", disk->info.alias) < 0) goto cleanup; /* XXX Here, we know we are about to alter disk->backingChain if @@ -12221,14 +12264,22 @@ qemuDomainSnapshotCreateSingleDiskActive(virConnectPtr conn, if (!(temppool = virStorageEphemeralFromSnapshotDiskDef(conn, snap))) goto cleanup; + if (qemuDomainSnapshotDiskGetSourceString(snap, &source) < 0) + goto cleanup; + + if (VIR_STRDUP(newsource, snap->file) < 0) + goto cleanup; + + if (persistDisk && + VIR_STRDUP(persistSource, snap->file) < 0) + goto cleanup; + switch (snap->type) { case VIR_DOMAIN_DISK_TYPE_BLOCK: reuse = true; /* fallthrough */ case -1: /* type was not provided in snapshot conf */ case VIR_DOMAIN_DISK_TYPE_FILE: - if (VIR_STRDUP(source, snap->file) < 0) - goto cleanup; /* create the stub file and set selinux labels; manipulate disk in * place, in a way that can be reverted on failure. */ @@ -12248,6 +12299,27 @@ qemuDomainSnapshotCreateSingleDiskActive(virConnectPtr conn, } break; + case VIR_DOMAIN_DISK_TYPE_NETWORK: + switch (snap->protocol) { + case VIR_DOMAIN_DISK_PROTOCOL_GLUSTER: + if (!(newhosts = virDomainDiskHostDefCopy(snap->nhosts, snap->hosts))) + goto cleanup; + + if (persistDisk && + !(persistHosts = virDomainDiskHostDefCopy(snap->nhosts, snap->hosts))) + goto cleanup; + + break; + + default: + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("snapshots on volumes using '%s' protocol " + "are not supported"), + virDomainDiskProtocolTypeToString(snap->protocol)); + goto cleanup; + } + break; + default: virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("snapshots are not supported on '%s' volumes"), @@ -12266,17 +12338,33 @@ qemuDomainSnapshotCreateSingleDiskActive(virConnectPtr conn, /* Update vm in place to match changes. */ need_unlink = false; + VIR_FREE(disk->src); - disk->src = source; - source = NULL; + virDomainDiskHostDefFree(disk->nhosts, disk->hosts); + + disk->src = newsource; disk->format = format; disk->type = snap->type; + disk->protocol = snap->protocol; + disk->nhosts = snap->nhosts; + disk->hosts = newhosts; + + newsource = NULL; + newhosts = NULL; + if (persistDisk) { VIR_FREE(persistDisk->src); + virDomainDiskHostDefFree(persistDisk->nhosts, persistDisk->hosts); + persistDisk->src = persistSource; - persistSource = NULL; persistDisk->format = format; persistDisk->type = snap->type; + persistDisk->protocol = snap->protocol; + persistDisk->nhosts = snap->nhosts; + persistDisk->hosts = persistHosts; + + persistSource = NULL; + persistHosts = NULL; } cleanup: @@ -12292,7 +12380,10 @@ cleanup: virStorageEphemeralFree(temppool); VIR_FREE(device); VIR_FREE(source); + VIR_FREE(newsource); VIR_FREE(persistSource); + virDomainDiskHostDefFree(snap->nhosts, newhosts); + virDomainDiskHostDefFree(snap->nhosts, persistHosts); return ret; } @@ -12344,12 +12435,20 @@ qemuDomainSnapshotUndoSingleDiskActive(virConnectPtr conn, source = NULL; disk->format = origdisk->format; disk->type = origdisk->type; + disk->protocol = origdisk->protocol; + virDomainDiskHostDefFree(disk->nhosts, disk->hosts); + disk->nhosts = origdisk->nhosts; + disk->hosts = virDomainDiskHostDefCopy(origdisk->nhosts, origdisk->hosts); if (persistDisk) { VIR_FREE(persistDisk->src); persistDisk->src = persistSource; persistSource = NULL; persistDisk->format = origdisk->format; persistDisk->type = origdisk->type; + persistDisk->protocol = origdisk->protocol; + virDomainDiskHostDefFree(persistDisk->nhosts, persistDisk->hosts); + persistDisk->nhosts = origdisk->nhosts; + persistDisk->hosts = virDomainDiskHostDefCopy(origdisk->nhosts, origdisk->hosts); } cleanup: -- 1.8.5.2

On 01/09/2014 09:15 AM, Peter Krempa wrote:
Add support for gluster backed images as sources for snapshots in the qemu driver. This will also simplify adding further network backed volumes as sources for snapshot in case qemu will support them. --- docs/formatsnapshot.html.in | 4 +- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_command.h | 9 ++++ src/qemu/qemu_driver.c | 113 +++++++++++++++++++++++++++++++++++++++++--- 4 files changed, 118 insertions(+), 10 deletions(-)
Overall this is a nice series. I'll have to start playing with it compiled into scratch builds. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake
-
Peter Krempa