[libvirt] [PATCHv5 0/7] Gluster snapshot series
New version after the recent feedback and fixing a few bugs found while testing. Please see individual patches for changes. Peter Krempa (7): storage: Add APIs for internal handling of files via the storage driver storage: Implement file storage APIs in the default storage driver storage: add file functions for local and block files storage: Add storage file backends for gluster qemu: Switch snapshot deletion to the new API functions qemu: snapshot: Use new APIs to detect presence of existing storage files qemu: snapshot: Add support for external active snapshots on gluster docs/formatsnapshot.html.in | 5 +- docs/hvsupport.pl | 4 +- po/POTFILES.in | 1 + src/Makefile.am | 1 + src/check-aclrules.pl | 1 + src/check-drivername.pl | 1 + src/driver.h | 30 ++++- src/libvirt_private.c | 180 ++++++++++++++++++++++++++++++ src/libvirt_private.h | 61 ++++++++++ src/libvirt_private.syms | 9 ++ src/qemu/qemu_command.c | 2 +- src/qemu/qemu_command.h | 9 ++ src/qemu/qemu_driver.c | 204 ++++++++++++++++++++++++++-------- src/storage/storage_backend.c | 42 +++++++ src/storage/storage_backend.h | 43 +++++++ src/storage/storage_backend_fs.c | 48 ++++++++ src/storage/storage_backend_fs.h | 2 + src/storage/storage_backend_gluster.c | 143 ++++++++++++++++++++++++ src/storage/storage_backend_gluster.h | 1 + src/storage/storage_driver.c | 95 ++++++++++++++++ 20 files changed, 829 insertions(+), 53 deletions(-) create mode 100644 src/libvirt_private.c create mode 100644 src/libvirt_private.h -- 1.8.5.3
Some remote filesystems are not accessible via the local filesystem calls, but libvirt needs means to do operations on such files. This patch adds internal APIs into the storage driver that will allow operations on various networked and other filesystems. --- Notes: Version 5: - changed error reporting scheme and re-documented - fix indentation of makefile - simplified conditions according to review - fixed typos docs/hvsupport.pl | 4 +- po/POTFILES.in | 1 + src/Makefile.am | 1 + src/check-drivername.pl | 1 + src/driver.h | 30 +++++++- src/libvirt_private.c | 180 +++++++++++++++++++++++++++++++++++++++++++++++ src/libvirt_private.h | 61 ++++++++++++++++ src/libvirt_private.syms | 9 +++ 8 files changed, 285 insertions(+), 2 deletions(-) create mode 100644 src/libvirt_private.c create mode 100644 src/libvirt_private.h diff --git a/docs/hvsupport.pl b/docs/hvsupport.pl index f8483f9..bed04f9 100755 --- a/docs/hvsupport.pl +++ b/docs/hvsupport.pl @@ -206,6 +206,8 @@ while (defined($line = <FILE>)) { $api = "vir$name"; } elsif ($name =~ /\w+(Open|Close)/) { next; + } elsif ($name =~ /^StorageFile\w+/) { + next; } else { die "driver $name does not have a public API"; } @@ -256,7 +258,7 @@ foreach my $src (@srcs) { my $meth = $2; my $vers = $3; - next if $api eq "no" || $api eq "name"; + next if $api eq "no" || $api eq "name" || $api =~ /^storageFile\w+/; die "Method $meth in $src is missing version" unless defined $vers; diff --git a/po/POTFILES.in b/po/POTFILES.in index 0359b2f..9304942 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -50,6 +50,7 @@ src/interface/interface_backend_netcf.c src/interface/interface_backend_udev.c src/internal.h src/libvirt.c +src/libvirt_private.c src/libvirt-lxc.c src/libvirt-qemu.c src/locking/lock_daemon.c diff --git a/src/Makefile.am b/src/Makefile.am index 3f8d22f..39447ed 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -174,6 +174,7 @@ DRIVER_SOURCES = \ fdstream.c fdstream.h \ $(NODE_INFO_SOURCES) \ libvirt.c libvirt_internal.h \ + libvirt_private.h libvirt_private.c \ 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..a98d7db 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 =~ /virDrvStorageFile/; my $sym = $drv; $sym =~ s/virDrv/vir/; diff --git a/src/driver.h b/src/driver.h index 5f4cd8d..20f944a 100644 --- a/src/driver.h +++ b/src/driver.h @@ -2,7 +2,7 @@ * driver.h: description of the set of interfaces provided by a * entry point to the virtualization engine * - * Copyright (C) 2006-2013 Red Hat, Inc. + * Copyright (C) 2006-2014 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 @@ -24,6 +24,8 @@ # include <unistd.h> +# include <sys/stat.h> + # include "internal.h" # include "libvirt_internal.h" # include "viruri.h" @@ -1755,6 +1757,25 @@ typedef int (*virDrvStoragePoolIsPersistent)(virStoragePoolPtr pool); +/* -------- Internal storage driver APIs ----------- */ +typedef struct _virStorageFile virStorageFile; +typedef virStorageFile *virStorageFilePtr; + +typedef int +(*virDrvStorageFileInit)(virStorageFilePtr file); + +typedef void +(*virDrvStorageFileDeinit)(virStorageFilePtr file); + +typedef int +(*virDrvStorageFileCreate)(virStorageFilePtr file); + +typedef int +(*virDrvStorageFileUnlink)(virStorageFilePtr file); + +typedef int +(*virDrvStorageFileStat)(virStorageFilePtr file, + struct stat *st); typedef struct _virStorageDriver virStorageDriver; typedef virStorageDriver *virStorageDriverPtr; @@ -1813,6 +1834,13 @@ struct _virStorageDriver { virDrvStorageVolResize storageVolResize; virDrvStoragePoolIsActive storagePoolIsActive; virDrvStoragePoolIsPersistent storagePoolIsPersistent; + + /* -------- Internal storage driver APIs ----------- */ + virDrvStorageFileInit storageFileInit; + virDrvStorageFileDeinit storageFileDeinit; + virDrvStorageFileCreate storageFileCreate; + virDrvStorageFileUnlink storageFileUnlink; + virDrvStorageFileStat storageFileStat; }; # ifdef WITH_LIBVIRTD diff --git a/src/libvirt_private.c b/src/libvirt_private.c new file mode 100644 index 0000000..330dd88 --- /dev/null +++ b/src/libvirt_private.c @@ -0,0 +1,180 @@ +/** + * libvirt_private.c: internal driver APIs not exported via RPC + * + * Copyright (C) 2014 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/>. + * + * Author: Peter Krempa <pkrempa@redhat.com> + */ + +#include <config.h> + +#include "libvirt_private.h" + +#include "datatypes.h" +#include "internal.h" +#include "viralloc.h" + +#define VIR_FROM_THIS VIR_FROM_NONE +/* ----------- helpers for APIs cooperating with storage driver ------ */ +virStorageFilePtr +virStorageFileInitFromDiskDef(virConnectPtr conn, + virDomainDiskDefPtr disk) +{ + virStorageFilePtr file = NULL; + + if (!conn->storageDriver) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("can't initialize storage file: no storage driver")); + return NULL; + } + + if (VIR_ALLOC(file) < 0) + return NULL; + + file->driver = conn->storageDriver; + + file->path = disk->src; + file->type = disk->type; + file->protocol = disk->protocol; + file->nhosts = disk->nhosts; + if (!(file->hosts = virDomainDiskHostDefCopy(disk->nhosts, disk->hosts))) + goto error; + + if (file->driver->storageFileInit && + file->driver->storageFileInit(file) < 0) + goto error; + + return file; + +error: + virDomainDiskHostDefFree(file->nhosts, file->hosts); + VIR_FREE(file); + return NULL; +} + + +virStorageFilePtr +virStorageFileInitFromSnapshotDef(virConnectPtr conn, + virDomainSnapshotDiskDefPtr disk) +{ + virStorageFilePtr file = NULL; + + if (!conn->storageDriver) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("can't initialize storage file: no storage driver")); + return NULL; + } + + if (VIR_ALLOC(file) < 0) + return NULL; + + file->driver = conn->storageDriver; + + file->path = disk->file; + file->type = disk->type; + file->protocol = disk->protocol; + file->nhosts = disk->nhosts; + if (!(file->hosts = virDomainDiskHostDefCopy(disk->nhosts, disk->hosts))) + goto error; + + if (file->driver->storageFileInit && + file->driver->storageFileInit(file) < 0) + goto error; + + return file; + +error: + virDomainDiskHostDefFree(file->nhosts, file->hosts); + VIR_FREE(file); + return NULL; +} + + +void +virStorageFileFree(virStorageFilePtr file) +{ + if (!file) + return; + + if (file->driver->storageFileDeinit) + file->driver->storageFileDeinit(file); + + virDomainDiskHostDefFree(file->nhosts, file->hosts); + VIR_FREE(file); +} + +/* ----------- APIs cooperating with storage driver ------------------ */ + +/** + * virStorageFileCreate: Creates an empty storage file via storage driver + * + * @file: file structure pointing to the file + * + * Returns 0 on success, -2 if the function isn't supported by the backend, + * -1 on other failure. Errno is set in case of failure. + */ +int +virStorageFileCreate(virStorageFilePtr file) +{ + if (file->driver->storageFileCreate) + return file->driver->storageFileCreate(file); + + errno = ENOSYS; + return -2; +} + + +/** + * virStorageFileUnlink: Unlink storage file via storage driver + * + * @file: file structure pointing to the file + * + * Unlinks the file described by the @file structure. + * + * Returns 0 on success, -2 if the function isn't supported by the backend, + * -1 on other failure. Errno is set in case of failure. + */ +int +virStorageFileUnlink(virStorageFilePtr file) +{ + if (file->driver->storageFileUnlink) + return file->driver->storageFileUnlink(file); + + errno = ENOSYS; + return -2; +} + + +/** + * virStorageFileStat: returns stat struct of a file via storage driver + * + * @file: file structure pointing to the file + * @stat: stat structure to return data + * + * Returns 0 on success, -2 if the function isn't supported by the backend, + * -1 on other failure. Errno is set in case of failure. +*/ +int +virStorageFileStat(virStorageFilePtr file, + struct stat *stat) +{ + if (file->driver->storageFileStat) + return file->driver->storageFileStat(file, stat); + + errno = ENOSYS; + return -2; +} diff --git a/src/libvirt_private.h b/src/libvirt_private.h new file mode 100644 index 0000000..8b6dd30 --- /dev/null +++ b/src/libvirt_private.h @@ -0,0 +1,61 @@ +/** + * libvirt_private.h: internal driver APIs not exported via RPC + * + * Copyright (C) 2014 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/>. + * + * Author: Peter Krempa <pkrempa@redhat.com> + */ + +#ifndef __VIR_PRIVATE_H__ +# define __VIR_PRIVATE_H__ + +# include "driver.h" +# include "conf/domain_conf.h" +# include "conf/snapshot_conf.h" +# include "viruri.h" + +# include <sys/types.h> +# include <sys/stat.h> + +/* -------- APIs cooperating with storage driver ----------------------- */ +struct _virStorageFile { + virStorageDriverPtr driver; + void *priv; + + char *path; + int type; + int protocol; + + size_t nhosts; + virDomainDiskHostDefPtr hosts; +}; + + +virStorageFilePtr +virStorageFileInitFromDiskDef(virConnectPtr conn, + virDomainDiskDefPtr disk); +virStorageFilePtr +virStorageFileInitFromSnapshotDef(virConnectPtr conn, + virDomainSnapshotDiskDefPtr disk); +void virStorageFileFree(virStorageFilePtr file); + +int virStorageFileCreate(virStorageFilePtr file); +int virStorageFileUnlink(virStorageFilePtr file); +int virStorageFileStat(virStorageFilePtr file, + struct stat *stat); + +#endif /* #ifndef __VIR_PRIVATE_H__ */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 2c9536a..7e2728f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -820,6 +820,15 @@ virRegisterSecretDriver; virRegisterStorageDriver; +# libvirt_private.h +virStorageFileCreate; +virStorageFileFree; +virStorageFileInitFromDiskDef; +virStorageFileInitFromSnapshotDef; +virStorageFileStat; +virStorageFileUnlink; + + # locking/domain_lock.h virDomainLockDiskAttach; virDomainLockDiskDetach; -- 1.8.5.3
On 02/11/14 17:26, Peter Krempa wrote:
Some remote filesystems are not accessible via the local filesystem calls, but libvirt needs means to do operations on such files.
This patch adds internal APIs into the storage driver that will allow operations on various networked and other filesystems. ---
Notes: Version 5: - changed error reporting scheme and re-documented - fix indentation of makefile - simplified conditions according to review - fixed typos
docs/hvsupport.pl | 4 +- po/POTFILES.in | 1 + src/Makefile.am | 1 + src/check-drivername.pl | 1 + src/driver.h | 30 +++++++- src/libvirt_private.c | 180 +++++++++++++++++++++++++++++++++++++++++++++++ src/libvirt_private.h | 61 ++++++++++++++++ src/libvirt_private.syms | 9 +++ 8 files changed, 285 insertions(+), 2 deletions(-) create mode 100644 src/libvirt_private.c create mode 100644 src/libvirt_private.h
diff --git a/src/libvirt_private.c b/src/libvirt_private.c new file mode 100644 index 0000000..330dd88 --- /dev/null +++ b/src/libvirt_private.c
...
+/** + * virStorageFileStat: returns stat struct of a file via storage driver + * + * @file: file structure pointing to the file + * @stat: stat structure to return data + * + * Returns 0 on success, -2 if the function isn't supported by the backend, + * -1 on other failure. Errno is set in case of failure. +*/ +int +virStorageFileStat(virStorageFilePtr file, + struct stat *stat)
Older complilers complain here that "stat" shadows a global symbol. I'll change this variable name to st.
+{ + if (file->driver->storageFileStat) + return file->driver->storageFileStat(file, stat); + + errno = ENOSYS; + return -2; +}
Peter
On Tue, Feb 11, 2014 at 05:26:53PM +0100, Peter Krempa wrote:
Some remote filesystems are not accessible via the local filesystem calls, but libvirt needs means to do operations on such files.
This patch adds internal APIs into the storage driver that will allow operations on various networked and other filesystems. ---
Notes: Version 5: - changed error reporting scheme and re-documented - fix indentation of makefile - simplified conditions according to review - fixed typos
docs/hvsupport.pl | 4 +- po/POTFILES.in | 1 + src/Makefile.am | 1 + src/check-drivername.pl | 1 + src/driver.h | 30 +++++++- src/libvirt_private.c | 180 +++++++++++++++++++++++++++++++++++++++++++++++ src/libvirt_private.h | 61 ++++++++++++++++ src/libvirt_private.syms | 9 +++
I'm not really convinced we should be modifying any of these files for this. None of this stuff is being exposed in either the wire protocol or public API. When we made the QEMU driver have a direct dep on the network driver, so didn't wire that stuff up into the driver framework. We just exported APIs in network/bridge_driver.h and had QEMU call them. I'd expect the storage/storage_driver.h to just export APIs we need in a similar manner. That said, if we're finding we need these extra capabilities from the storage driver, then this is a good sign that our public API themselves need to also be enhanced. Admittedly the latter is a bigger job, so we shouldn't block on that. I do think we shouldn't wire any of this up into the driver framework though. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Implement the APIs added by the previous patch in the default storage driver used by qemu. --- Notes: Version 5: - adapt to error reporting change src/check-aclrules.pl | 1 + src/storage/storage_backend.c | 37 +++++++++++++++++ src/storage/storage_backend.h | 43 ++++++++++++++++++++ src/storage/storage_driver.c | 95 +++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 176 insertions(+) diff --git a/src/check-aclrules.pl b/src/check-aclrules.pl index 9151e6a..8bbdfd9 100755 --- a/src/check-aclrules.pl +++ b/src/check-aclrules.pl @@ -224,6 +224,7 @@ while (<>) { if ($api ne "no" && $api ne "name" && + $api !~ /^storageFile\w+/ && $table ne "virStateDriver" && !exists $acls{$impl} && !exists $whitelist{$api} && diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 19fb1f0..b59b5b7 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -121,6 +121,12 @@ static virStorageBackendPtr backends[] = { NULL }; + +static virStorageFileBackendPtr fileBackends[] = { + NULL +}; + + enum { TOOL_QEMU_IMG, TOOL_KVM_IMG, @@ -1152,6 +1158,37 @@ virStorageBackendForType(int type) } +virStorageFileBackendPtr +virStorageFileBackendForType(int type, + int protocol) +{ + size_t i; + + for (i = 0; fileBackends[i]; i++) { + if (fileBackends[i]->type == type) { + if (type == VIR_DOMAIN_DISK_TYPE_NETWORK && + fileBackends[i]->protocol != protocol) + continue; + + return fileBackends[i]; + } + } + + if (type == VIR_DOMAIN_DISK_TYPE_NETWORK) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("missing storage backend for network files " + "using %s protocol"), + virDomainDiskProtocolTypeToString(protocol)); + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("missing storage backend for '%s' storage"), + virDomainDiskTypeToString(type)); + } + + return NULL; +} + + /* * Allows caller to silently ignore files with improper mode * diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 378bc4d..5613285 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -29,6 +29,7 @@ # include "internal.h" # include "storage_conf.h" # include "vircommand.h" +# include "libvirt_private.h" typedef char * (*virStorageBackendFindPoolSources)(virConnectPtr conn, const char *srcSpec, @@ -189,4 +190,46 @@ virStorageBackendCreateQemuImgCmd(virConnectPtr conn, const char *create_tool, int imgformat); +/* ------- virStorageFile backends ------------ */ +typedef int +(*virStorageFileBackendInit)(virStorageFilePtr file); + +typedef void +(*virStorageFileBackendDeinit)(virStorageFilePtr file); + +typedef int +(*virStorageFileBackendCreate)(virStorageFilePtr file); + +typedef int +(*virStorageFileBackendUnlink)(virStorageFilePtr file); + +typedef int +(*virStorageFileBackendStat)(virStorageFilePtr file, + struct stat *st); + +typedef struct _virStorageFileBackend virStorageFileBackend; +typedef virStorageFileBackend *virStorageFileBackendPtr; + +virStorageFileBackendPtr virStorageFileBackendForType(int type, int protocol); + +typedef struct _virStorageFilePriv virStorageFileBackendPriv; +typedef virStorageFileBackendPriv *virStorageFileBackendPrivPtr; +struct _virStorageFilePriv { + virStorageFileBackendPtr backend; + void *priv; +}; + + +struct _virStorageFileBackend { + int type; + int protocol; + + virStorageFileBackendInit backendInit; + virStorageFileBackendDeinit backendDeinit; + + virStorageFileBackendCreate storageFileCreate; + virStorageFileBackendUnlink storageFileUnlink; + virStorageFileBackendStat storageFileStat; +}; + #endif /* __VIR_STORAGE_BACKEND_H__ */ diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index c83aa8a..a650678 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2585,6 +2585,92 @@ cleanup: return ret; } + +static int +storageFileInit(virStorageFilePtr file) +{ + virStorageFileBackendPrivPtr priv = NULL; + + if (VIR_ALLOC(priv) < 0) + return -1; + + file->priv = priv; + + if (!(priv->backend = virStorageFileBackendForType(file->type, + file->protocol))) + goto error; + + if (priv->backend->backendInit) { + if (priv->backend->backendInit(file) < 0) + goto error; + } + + return 0; + +error: + file->priv = NULL; + VIR_FREE(priv); + return -1; +} + + +static void +storageFileDeinit(virStorageFilePtr file) +{ + virStorageFileBackendPrivPtr priv = file->priv; + + if (priv->backend && + priv->backend->backendDeinit) + priv->backend->backendDeinit(file); + + VIR_FREE(priv); + file->priv = NULL; +} + + +static int +storageFileCreate(virStorageFilePtr file) +{ + virStorageFileBackendPrivPtr priv = file->priv; + + if (!priv->backend->storageFileCreate) { + errno = ENOSYS; + return -2; + } + + return priv->backend->storageFileCreate(file); +} + + +static int +storageFileUnlink(virStorageFilePtr file) +{ + virStorageFileBackendPrivPtr priv = file->priv; + + if (!priv->backend->storageFileUnlink) { + errno = ENOSYS; + return -2; + } + + return priv->backend->storageFileUnlink(file); +} + + +static int +storageFileStat(virStorageFilePtr file, + struct stat *st) +{ + virStorageFileBackendPrivPtr priv = file->priv; + + if (!(priv->backend->storageFileStat)) { + errno = ENOSYS; + return -2; + } + + return priv->backend->storageFileStat(file, st); +} + + static virStorageDriver storageDriver = { .name = "storage", .storageOpen = storageOpen, /* 0.4.0 */ @@ -2631,6 +2717,15 @@ static virStorageDriver storageDriver = { .storagePoolIsActive = storagePoolIsActive, /* 0.7.3 */ .storagePoolIsPersistent = storagePoolIsPersistent, /* 0.7.3 */ + + /* ----- internal APIs ----- */ + .storageFileInit = storageFileInit, + .storageFileDeinit = storageFileDeinit, + + .storageFileUnlink = storageFileUnlink, + .storageFileStat = storageFileStat, + .storageFileCreate = storageFileCreate, + }; -- 1.8.5.3
On 02/11/2014 09:26 AM, Peter Krempa wrote:
Implement the APIs added by the previous patch in the default storage driver used by qemu. ---
Notes: Version 5: - adapt to error reporting change
src/check-aclrules.pl | 1 + src/storage/storage_backend.c | 37 +++++++++++++++++ src/storage/storage_backend.h | 43 ++++++++++++++++++++ src/storage/storage_driver.c | 95 +++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 176 insertions(+)
I tend to agree with Dan's comment on patch 1 (that is, exposing these directly in storage_driver.c instead of making it go through the driver.h interface), which may have some fallout on this patch. That said, I'll review this one.
+++ b/src/storage/storage_backend.c @@ -121,6 +121,12 @@ static virStorageBackendPtr backends[] = { NULL };
+ +static virStorageFileBackendPtr fileBackends[] = { + NULL +};
At first I wondered why an empty array, then I realized - this is the framework, then later patches add in new backends. Okay.
+ +struct _virStorageFileBackend { + int type; + int protocol; + + virStorageFileBackendInit backendInit; + virStorageFileBackendDeinit backendDeinit; + + virStorageFileBackendCreate storageFileCreate; + virStorageFileBackendUnlink storageFileUnlink; + virStorageFileBackendStat storageFileStat;
No need to copy existing bad examples; please document which of these callbacks (if any) must be non-NULL, or explicitly state that all callbacks are optional. (When I first added the gluster backend, I had a hard time figuring out which callbacks storage_driver expected to be able to use). It would also be nice to document the contract placed on implementations of each callback (such as when a callback should return -1 vs -2, or whether errno must be set on error), at the point where you declare typedefs for each callback signature. This patch looks okay in isolation for driving the new callbacks, once later patches implement them. ACK with comments added. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Implement the "stat" and "unlink" function for "file" volumes and "stat" for "block volumes using the regular system calls. --- Notes: Version 5: - added debug messages - adapted to error reporting changes Version 4: - adapt to change in error reporting src/storage/storage_backend.c | 2 ++ src/storage/storage_backend_fs.c | 48 ++++++++++++++++++++++++++++++++++++++++ src/storage/storage_backend_fs.h | 2 ++ 3 files changed, 52 insertions(+) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index b59b5b7..da81da9 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -123,6 +123,8 @@ static virStorageBackendPtr backends[] = { static virStorageFileBackendPtr fileBackends[] = { + &virStorageFileBackendFile, + &virStorageFileBackendBlock, NULL }; diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index fa11e2f..04cc94c 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -1327,4 +1327,52 @@ virStorageBackend virStorageBackendNetFileSystem = { .deleteVol = virStorageBackendFileSystemVolDelete, .resizeVol = virStorageBackendFileSystemVolResize, }; + + +static int +virStorageFileBackendFileUnlink(virStorageFilePtr file) +{ + int ret; + + ret = unlink(file->path); + /* preserve errno */ + + VIR_DEBUG("removing storage file %p(%s): ret=%d,errno=%d", + file, file->path, ret, errno); + + return ret; +} + + +static int +virStorageFileBackendFileStat(virStorageFilePtr file, + struct stat *st) +{ + int ret; + + ret = stat(file->path, st); + /* preserve errno */ + + VIR_DEBUG("stat of storage file %p(%s): ret=%d,errno=%d", + file, file->path, ret, errno); + + return ret; +} + + +virStorageFileBackend virStorageFileBackendFile = { + .type = VIR_DOMAIN_DISK_TYPE_FILE, + + .storageFileUnlink = virStorageFileBackendFileUnlink, + .storageFileStat = virStorageFileBackendFileStat, +}; + + +virStorageFileBackend virStorageFileBackendBlock = { + .type = VIR_DOMAIN_DISK_TYPE_BLOCK, + + .storageFileStat = virStorageFileBackendFileStat, +}; + + #endif /* WITH_STORAGE_FS */ diff --git a/src/storage/storage_backend_fs.h b/src/storage/storage_backend_fs.h index a519b38..347ea9b 100644 --- a/src/storage/storage_backend_fs.h +++ b/src/storage/storage_backend_fs.h @@ -38,4 +38,6 @@ typedef enum { } virStoragePoolProbeResult; extern virStorageBackend virStorageBackendDirectory; +extern virStorageFileBackend virStorageFileBackendFile; +extern virStorageFileBackend virStorageFileBackendBlock; #endif /* __VIR_STORAGE_BACKEND_FS_H__ */ -- 1.8.5.3
On 02/11/2014 09:26 AM, Peter Krempa wrote:
Implement the "stat" and "unlink" function for "file" volumes and "stat" for "block volumes using the regular system calls.
s/"block/&"/
---
+static int +virStorageFileBackendFileUnlink(virStorageFilePtr file) +{ + int ret; + + ret = unlink(file->path); + /* preserve errno */ + + VIR_DEBUG("removing storage file %p(%s): ret=%d,errno=%d",
Maybe a space after the comma, but not the end of the world in a debug message. ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Implement storage backend functions to deal with gluster volumes and implement the "stat" and "unlink" backend APIs. --- Notes: Version 5: - adapted to error reporting changes - tweaked error message src/storage/storage_backend.c | 3 + src/storage/storage_backend_gluster.c | 143 ++++++++++++++++++++++++++++++++++ src/storage/storage_backend_gluster.h | 1 + 3 files changed, 147 insertions(+) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index da81da9..1253343 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -125,6 +125,9 @@ static virStorageBackendPtr backends[] = { static virStorageFileBackendPtr fileBackends[] = { &virStorageFileBackendFile, &virStorageFileBackendBlock, +#if WITH_STORAGE_GLUSTER + &virStorageFileBackendGluster, +#endif NULL }; diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index d7a1553..6f17b52 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -477,3 +477,146 @@ virStorageBackend virStorageBackendGluster = { .deleteVol = virStorageBackendGlusterVolDelete, }; + + +typedef struct _virStorageFileBackendGlusterPriv virStorageFileBackendGlusterPriv; +typedef virStorageFileBackendGlusterPriv *virStorageFileBackendGlusterPrivPtr; + +struct _virStorageFileBackendGlusterPriv { + glfs_t *vol; + char *volname; + char *path; +}; + + +static void +virStorageFileBackendGlusterDeinit(virStorageFilePtr file) +{ + VIR_DEBUG("deinitializing gluster storage file %p(%s@%s)", + file, file->path, file->hosts[0].name); + virStorageFileBackendPrivPtr backPriv = file->priv; + virStorageFileBackendGlusterPrivPtr priv = backPriv->priv; + + glfs_fini(priv->vol); + VIR_FREE(priv->volname); + + VIR_FREE(priv); + backPriv->priv = NULL; +} + +static int +virStorageFileBackendGlusterInit(virStorageFilePtr file) +{ + virStorageFileBackendPrivPtr backPriv = file->priv; + virStorageFileBackendGlusterPrivPtr priv = NULL; + virDomainDiskHostDefPtr host = &(file->hosts[0]); + const char *hostname = host->name; + int port = 0; + + VIR_DEBUG("initializing gluster storage file %p(%s@%s)", + file, file->path, hostname); + + if (VIR_ALLOC(priv) < 0) + return -1; + + if (VIR_STRDUP(priv->volname, file->path) < 0) + goto error; + + if (!(priv->path = strchr(priv->volname, '/'))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("invalid path of gluster volume: '%s'"), + file->path); + goto error; + } + + *priv->path = '\0'; + priv->path++; + + if (host->port && + virStrToLong_i(host->port, NULL, 10, &port) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to parse port number '%s'"), + host->port); + goto error; + } + + if (host->transport == VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) + hostname = host->socket; + + + if (!(priv->vol = glfs_new(priv->volname))) { + virReportOOMError(); + goto error; + } + + if (glfs_set_volfile_server(priv->vol, + virDomainDiskProtocolTransportTypeToString(host->transport), + hostname, port) < 0) { + virReportSystemError(errno, + _("failed to set gluster volfile server '%s'"), + hostname); + goto error; + } + + if (glfs_init(priv->vol) < 0) { + virReportSystemError(errno, + _("failed to initialize gluster connection to " + "server: '%s'"), hostname); + goto error; + } + + backPriv->priv = priv; + + return 0; + +error: + VIR_FREE(priv->volname); + glfs_fini(priv->vol); + + return -1; +} + + +static int +virStorageFileBackendGlusterUnlink(virStorageFilePtr file) +{ + virStorageFileBackendPrivPtr backPriv = file->priv; + virStorageFileBackendGlusterPrivPtr priv = backPriv->priv; + int ret; + + ret = glfs_unlink(priv->vol, priv->path); + /* preserve errno */ + + VIR_DEBUG("removing storage file %p(%s@%s): ret=%d,errno=%d", + file, file->path, file->hosts[0].name, ret, errno); + return ret; +} + + +static int +virStorageFileBackendGlusterStat(virStorageFilePtr file, + struct stat *st) +{ + virStorageFileBackendPrivPtr backPriv = file->priv; + virStorageFileBackendGlusterPrivPtr priv = backPriv->priv; + int ret; + + ret = glfs_stat(priv->vol, priv->path, st); + /* preserve errno */ + + VIR_DEBUG("stat of storage file %p(%s@%s): ret=%d,errno=%d", + file, file->path, file->hosts[0].name, ret, errno); + return ret; +} + + +virStorageFileBackend virStorageFileBackendGluster = { + .type = VIR_DOMAIN_DISK_TYPE_NETWORK, + .protocol = VIR_DOMAIN_DISK_PROTOCOL_GLUSTER, + + .backendInit = virStorageFileBackendGlusterInit, + .backendDeinit = virStorageFileBackendGlusterDeinit, + + .storageFileUnlink = virStorageFileBackendGlusterUnlink, + .storageFileStat = virStorageFileBackendGlusterStat, +}; diff --git a/src/storage/storage_backend_gluster.h b/src/storage/storage_backend_gluster.h index b21bda7..6796016 100644 --- a/src/storage/storage_backend_gluster.h +++ b/src/storage/storage_backend_gluster.h @@ -25,5 +25,6 @@ # include "storage_backend.h" extern virStorageBackend virStorageBackendGluster; +extern virStorageFileBackend virStorageFileBackendGluster; #endif /* __VIR_STORAGE_BACKEND_GLUSTER_H__ */ -- 1.8.5.3
On 02/11/2014 09:26 AM, Peter Krempa wrote:
Implement storage backend functions to deal with gluster volumes and implement the "stat" and "unlink" backend APIs. ---
+ +static void +virStorageFileBackendGlusterDeinit(virStorageFilePtr file) +{ + VIR_DEBUG("deinitializing gluster storage file %p(%s@%s)", + file, file->path, file->hosts[0].name);
A bit unusual to list file@host, instead of host/file, but not a show-stopper.
+static int +virStorageFileBackendGlusterInit(virStorageFilePtr file) +{ + virStorageFileBackendPrivPtr backPriv = file->priv; + virStorageFileBackendGlusterPrivPtr priv = NULL; + virDomainDiskHostDefPtr host = &(file->hosts[0]); + const char *hostname = host->name; + int port = 0; + + VIR_DEBUG("initializing gluster storage file %p(%s@%s)", + file, file->path, hostname); + + if (VIR_ALLOC(priv) < 0) + return -1; + + if (VIR_STRDUP(priv->volname, file->path) < 0) + goto error; + + if (!(priv->path = strchr(priv->volname, '/'))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("invalid path of gluster volume: '%s'"), + file->path); + goto error; + }
This looks like a lot of shared code; any chance we use a common routine as the basis of both this and virStorageBackendGlusterOpen()? But what you have works, so ACK (a refactor can be a followup). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Use the new storage driver APIs to delete snapshot backing files in case of failure instead of directly relying on "unlink". This will help us in the future when we will be adding network based storage without local representation in the host. --- Notes: Version 5: - no change, wasn't reviewed yet src/qemu/qemu_driver.c | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 94844df..b94382d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -94,6 +94,7 @@ #include "virstring.h" #include "viraccessapicheck.h" #include "viraccessapicheckqemu.h" +#include "libvirt_private.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -12629,7 +12630,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, @@ -12647,6 +12649,7 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, int ret = -1; int fd = -1; bool need_unlink = false; + virStorageFilePtr snapfile = NULL; if (snap->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -12666,6 +12669,9 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, virStorageFileFreeMetadata(disk->backingChain); disk->backingChain = NULL; + if (!(snapfile = virStorageFileInitFromSnapshotDef(conn, snap))) + goto cleanup; + switch (snap->type) { case VIR_DOMAIN_DISK_TYPE_BLOCK: reuse = true; @@ -12741,8 +12747,9 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, } cleanup: - if (need_unlink && unlink(source)) + if (need_unlink && virStorageFileUnlink(snapfile)) VIR_WARN("unable to unlink just-created %s", source); + virStorageFileFree(snapfile); VIR_FREE(device); VIR_FREE(source); VIR_FREE(persistSource); @@ -12753,7 +12760,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, @@ -12762,16 +12770,20 @@ qemuDomainSnapshotUndoSingleDiskActive(virQEMUDriverPtr driver, { char *source = NULL; char *persistSource = NULL; + virStorageFilePtr diskfile = NULL; struct stat st; + diskfile = virStorageFileInitFromDiskDef(conn, disk); + 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) + if (need_unlink && diskfile && + virStorageFileStat(diskfile, &st) == 0 && S_ISREG(st.st_mode) && + virStorageFileUnlink(diskfile) < 0) VIR_WARN("Unable to remove just-created %s", disk->src); /* Update vm in place to match changes. */ @@ -12789,13 +12801,15 @@ qemuDomainSnapshotUndoSingleDiskActive(virQEMUDriverPtr driver, } cleanup: + virStorageFileFree(diskfile); 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, @@ -12843,7 +12857,7 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, persistDisk = vm->newDef->disks[indx]; } - ret = qemuDomainSnapshotCreateSingleDiskActive(driver, vm, + ret = qemuDomainSnapshotCreateSingleDiskActive(conn, driver, vm, &snap->def->disks[i], vm->def->disks[i], persistDisk, actions, @@ -12889,7 +12903,7 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, } - qemuDomainSnapshotUndoSingleDiskActive(driver, vm, + qemuDomainSnapshotUndoSingleDiskActive(conn, driver, vm, snap->def->dom->disks[i], vm->def->disks[i], persistDisk, @@ -13031,7 +13045,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.3
On 02/11/2014 09:26 AM, Peter Krempa wrote:
Use the new storage driver APIs to delete snapshot backing files in case of failure instead of directly relying on "unlink". This will help us in the future when we will be adding network based storage without local representation in the host. ---
Notes: Version 5: - no change, wasn't reviewed yet
Finally getting a review :)
src/qemu/qemu_driver.c | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 94844df..b94382d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -94,6 +94,7 @@ #include "virstring.h" #include "viraccessapicheck.h" #include "viraccessapicheckqemu.h" +#include "libvirt_private.h"
Based on Dan's comment on patch 1, it may be sufficient to just include storage_driver.h instead, which may affect the function names you call. But the conversion looks sane - instead of calling direct file system syscalls, we now call polymorphic storage calls, and let the storage driver manage whether it is a syscall or a gluster library call (or any future other backends that also add in their driver). ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Use the new storage driver based "stat" api to detect exiting files just as we did with local files. --- Notes: Version 5: - new in sereis src/qemu/qemu_driver.c | 59 +++++++++++++++++++++++--------------------------- 1 file changed, 27 insertions(+), 32 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b94382d..eec2a44 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12366,7 +12366,6 @@ qemuDomainSnapshotPrepareDiskExternalOverlayInactive(virDomainSnapshotDiskDefPtr } - static int qemuDomainSnapshotPrepareDiskExternal(virConnectPtr conn, virDomainDiskDefPtr disk, @@ -12374,7 +12373,8 @@ qemuDomainSnapshotPrepareDiskExternal(virConnectPtr conn, bool active, bool reuse) { - int actualType; + virStorageFilePtr snapfile = NULL; + int ret = -1; struct stat st; if (qemuTranslateSnapshotDiskSourcePool(conn, snapdisk) < 0) @@ -12397,40 +12397,35 @@ qemuDomainSnapshotPrepareDiskExternal(virConnectPtr conn, return -1; } - actualType = qemuSnapshotDiskGetActualType(snapdisk); + if (!(snapfile = virStorageFileInitFromSnapshotDef(conn, snapdisk))) + return -1; - switch ((enum virDomainDiskType) actualType) { - case VIR_DOMAIN_DISK_TYPE_BLOCK: - case VIR_DOMAIN_DISK_TYPE_FILE: - if (stat(snapdisk->file, &st) < 0) { - if (errno != ENOENT) { - virReportSystemError(errno, - _("unable to stat for disk %s: %s"), - snapdisk->name, snapdisk->file); - return -1; - } else if (reuse) { - virReportSystemError(errno, - _("missing existing file for disk %s: %s"), - snapdisk->name, snapdisk->file); - return -1; - } - } else if (!S_ISBLK(st.st_mode) && st.st_size && !reuse) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("external snapshot file for disk %s already " - "exists and is not a block device: %s"), - snapdisk->name, snapdisk->file); - return -1; + if (virStorageFileStat(snapfile, &st) < 0) { + if (errno != ENOENT) { + virReportSystemError(errno, + _("unable to stat for disk %s: %s"), + snapdisk->name, snapdisk->file); + goto cleanup; + } else if (reuse) { + virReportSystemError(errno, + _("missing existing file for disk %s: %s"), + snapdisk->name, snapdisk->file); + goto cleanup; } - break; - - case VIR_DOMAIN_DISK_TYPE_NETWORK: - case VIR_DOMAIN_DISK_TYPE_DIR: - case VIR_DOMAIN_DISK_TYPE_VOLUME: - case VIR_DOMAIN_DISK_TYPE_LAST: - break; + } else if (!S_ISBLK(st.st_mode) && st.st_size && !reuse) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("external snapshot file for disk %s already " + "exists and is not a block device: %s"), + snapdisk->name, snapdisk->file); + goto cleanup; } - return 0; + ret = 0; + +cleanup: + virStorageFileFree(snapfile); + + return ret;; } -- 1.8.5.3
On 02/11/2014 09:26 AM, Peter Krempa wrote:
Use the new storage driver based "stat" api to detect exiting files just as we did with local files. ---
Notes: Version 5: - new in sereis
src/qemu/qemu_driver.c | 59 +++++++++++++++++++++++--------------------------- 1 file changed, 27 insertions(+), 32 deletions(-)
ACK -- 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. --- Notes: Version 5: - no change, wasn't reviewed yet docs/formatsnapshot.html.in | 5 +- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_command.h | 9 ++++ src/qemu/qemu_driver.c | 113 +++++++++++++++++++++++++++++++++++++++++--- 4 files changed, 119 insertions(+), 10 deletions(-) diff --git a/docs/formatsnapshot.html.in b/docs/formatsnapshot.html.in index 44ed4fd..1f9a6c6 100644 --- a/docs/formatsnapshot.html.in +++ b/docs/formatsnapshot.html.in @@ -183,8 +183,9 @@ 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>network</code> with a protocol of <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 e499d54..ce2cac6 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 82ca4b3..ebb0b1d 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -313,4 +313,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 eec2a44..9983abe 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11932,6 +11932,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, @@ -12329,6 +12347,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: @@ -12638,6 +12679,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; @@ -12652,8 +12696,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 @@ -12667,13 +12710,21 @@ qemuDomainSnapshotCreateSingleDiskActive(virConnectPtr conn, if (!(snapfile = virStorageFileInitFromSnapshotDef(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 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. */ @@ -12693,6 +12744,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"), @@ -12728,17 +12800,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: @@ -12747,7 +12835,10 @@ cleanup: virStorageFileFree(snapfile); VIR_FREE(device); VIR_FREE(source); + VIR_FREE(newsource); VIR_FREE(persistSource); + virDomainDiskHostDefFree(snap->nhosts, newhosts); + virDomainDiskHostDefFree(snap->nhosts, persistHosts); return ret; } @@ -12787,12 +12878,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.3
On 02/11/2014 09:26 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. ---
Looks a bit confusing the number of variables we have to track as we swap between original and new, then fallback or commit. But I didn't spot anything obviously wrong in how you did it. I suspect that if we ever convert over to having domain XML track full backing chain semantics, we will have a lot to rewrite in this area of code to take advantage of that, but the future shouldn't hold up incremental progress in the present. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 02/12/14 20:46, Eric Blake wrote:
On 02/11/2014 09:26 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. ---
Looks a bit confusing the number of variables we have to track as we swap between original and new, then fallback or commit. But I didn't spot anything obviously wrong in how you did it. I suspect that if we ever convert over to having domain XML track full backing chain semantics, we will have a lot to rewrite in this area of code to take advantage of that, but the future shouldn't hold up incremental progress in the present. ACK.
Tracking all the variables for a storage volume is really unpleasant. I'm planing on splitting out the source part for a storage volume (possibly including the backing chain) into a separate structure. This will allow then for easier copying and manipulation for all the tracked data. Unfortunately a change like that is really invasive. Peter
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Peter Krempa