External/libvirt
1
0
Fork 0
mirror of https://gitlab.com/libvirt/libvirt.git synced 2024-10-30 09:53:10 +00:00
Code Issues Packages Projects Releases Wiki Activity

snapshot: Permit redefine of offline external snapshot

Browse Source 
Due to historical back-compat, bare 'virsh snapshot-create-as'
favors internal snapshots (but can't be used on domains with raw
storage), while 'virsh snapshot-create-as --disk-only' favors
external snapshots.  What's more, snapshots created with
--disk-only while the domain was running are marked as snapshot
state 'disk-snapshot', while snapshots created while the domain
was offline are marked as snapshot state 'shutdown' (a
'disk-snapshot' image might not be quiescent, while a 'shutdown'
snapshot always is).

But this leads to some interesting problems: if we create a
--disk-only snapshot of an offline guest, and then immediately try
to 'virsh snapshot-create --redefine' using the resulting XML to
overwrite the existing snapashot in place, things silently succeed,
but 'virsh snapshot-create --redefine --disk-only' fails with an
error message that the snapshot state is not 'disk-only'.  Worse,
if we delete the snapshot metadata first and then try to recreate
things, omitting --disk-only fails because the verification code
wants to force the default of an internal snapshot (which doesn't
work with raw disks), and using --disk-only still fails because the
snapshot XML is not 'disk-only' - making it impossible to recreate
the snapshot metadata (or to transfer it from one libvirtd host to
another).  Ideally, the presence or absence of the --disk-only
flag, and the presence or absence of an existing snapshot being
overwritten, shouldn't matter; if the XML is valid for one
situation, it should always be valid to redefine the metadata for
that snapshot.

Fix things by uniformly using virDomainSnapshotDefIsExternal()
(caching the results up front, and eliminating other 'if' clauses
now rendered redundant) when deciding whether the XML being
requested for redefinition should permit external or force internal
state capture (we got it right in only one out of three places in
the function).

See also https://bugzilla.redhat.com/1680304; this fixes the
domain-agnostic problems mentioned there, but another patch is
needed to fix further oddities with the qemu driver.  I did not
check for sure when the problems were introduced (git blame puts
some affected hunks as far back as 1.0.0), but it was definitely
been broken even before when commit 670e86bf (1.1.4) factored
redefine prep out of qemu code into the common snapshot_conf code.

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
This commit is contained in:
Eric Blake 2019-02-23 13:21:38 -06:00
parent d152c727c6
commit dafef600f4
1 changed files with 5 additions and 7 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
src/conf/snapshot_conf.c
View File

@ -1225,6 +1225,8 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
int align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL;
bool align_match = true;
virDomainSnapshotObjPtr other;
bool external = def->state == VIR_DOMAIN_DISK_SNAPSHOT ||
virDomainSnapshotDefIsExternal(def);
/* Prevent circular chains */
if (def->parent) {
@ -1259,14 +1261,12 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
}
/* Check that any replacement is compatible */
if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) &&
def->state != VIR_DOMAIN_DISK_SNAPSHOT) {
if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) && !external) {
virReportError(VIR_ERR_INVALID_ARG,
_("disk-only flag for snapshot %s requires "
"disk-snapshot state"),
def->name);
goto cleanup;
}
if (def->dom &&
@ -1315,8 +1315,7 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
}
if (def->dom) {
if (def->state == VIR_DOMAIN_DISK_SNAPSHOT ||
virDomainSnapshotDefIsExternal(def)) {
if (external) {
align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
align_match = false;
}
@ -1346,8 +1345,7 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
*snap = other;
} else {
if (def->dom) {
if (def->state == VIR_DOMAIN_DISK_SNAPSHOT ||
def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) {
if (external) {
align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
align_match = false;
}