From bd18b9670fd875db8734da4e62edcd027f612c00 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 1 Sep 2011 16:50:17 -0600 Subject: [PATCH] snapshot: add qemu snapshot redefine support Redefining a qemu snapshot requires a bit of a tweak to the common snapshot parsing code, but the end result is quite nice. Be careful that redefinitions do not introduce circular parent chains. Also, we don't want to allow conversion between online and offline existing snapshots. We could probably do some more validation for snapshots that don't already exist to make sure they are even feasible, by parsing qemu-img output, but that can come later. * src/conf/domain_conf.h (virDomainSnapshotParseFlags): New internal flags. * src/conf/domain_conf.c (virDomainSnapshotDefParseString): Alter signature to take internal flags. * src/esx/esx_driver.c (esxDomainSnapshotCreateXML): Update caller. * src/vbox/vbox_tmpl.c (vboxDomainSnapshotCreateXML): Likewise. * src/qemu/qemu_driver.c (qemuDomainSnapshotCreateXML): Support new public flags. --- src/conf/domain_conf.c | 25 +++++++--- src/conf/domain_conf.h | 7 ++- src/esx/esx_driver.c | 2 +- src/qemu/qemu_driver.c | 108 ++++++++++++++++++++++++++++++++++++----- src/vbox/vbox_tmpl.c | 2 +- 5 files changed, 122 insertions(+), 22 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0def87c0aa..94257ee105 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11375,8 +11375,9 @@ void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def) VIR_FREE(def); } -virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, - int newSnapshot) +/* flags are from virDomainSnapshotParseFlags */ +virDomainSnapshotDefPtr +virDomainSnapshotDefParseString(const char *xmlStr, unsigned int flags) { xmlXPathContextPtr ctxt = NULL; xmlDocPtr xml = NULL; @@ -11404,8 +11405,16 @@ virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, gettimeofday(&tv, NULL); def->name = virXPathString("string(./name)", ctxt); - if (def->name == NULL) - ignore_value(virAsprintf(&def->name, "%lld", (long long)tv.tv_sec)); + if (def->name == NULL) { + if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE) { + virDomainReportError(VIR_ERR_XML_ERROR, "%s", + _("a redefined snapshot must have a name")); + goto cleanup; + } else { + ignore_value(virAsprintf(&def->name, "%lld", + (long long)tv.tv_sec)); + } + } if (def->name == NULL) { virReportOOMError(); @@ -11414,7 +11423,7 @@ virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, def->description = virXPathString("string(./description)", ctxt); - if (!newSnapshot) { + if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE) { if (virXPathLongLong("string(./creationTime)", ctxt, &def->creationTime) < 0) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -11440,7 +11449,11 @@ virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, state); goto cleanup; } + } else { + def->creationTime = tv.tv_sec; + } + if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL) { if (virXPathInt("string(./active)", ctxt, &active) < 0) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not find 'active' element")); @@ -11448,8 +11461,6 @@ virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, } def->current = active != 0; } - else - def->creationTime = tv.tv_sec; ret = def; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index f970782b7e..cef0d9ee93 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1407,8 +1407,13 @@ struct _virDomainSnapshotObjList { virHashTable *objs; }; +typedef enum { + VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE = 1 << 0, + VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL = 1 << 1, +} virDomainSnapshotParseFlags; + virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, - int newSnapshot); + unsigned int flags); void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def); char *virDomainSnapshotDefFormat(char *domain_uuid, virDomainSnapshotDefPtr def, diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 695fb03674..4bbb3153f4 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -4217,7 +4217,7 @@ esxDomainSnapshotCreateXML(virDomainPtr domain, const char *xmlDesc, return NULL; } - def = virDomainSnapshotDefParseString(xmlDesc, 1); + def = virDomainSnapshotDefParseString(xmlDesc, 0); if (def == NULL) { return NULL; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9a4bf7d295..b002b1d61b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -297,6 +297,8 @@ static void qemuDomainSnapshotLoad(void *payload, virDomainSnapshotObjPtr snap = NULL; virDomainSnapshotObjPtr current = NULL; char ebuf[1024]; + unsigned int flags = (VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE | + VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL); virDomainObjLock(vm); if (virAsprintf(&snapDir, "%s/%s", baseDir, vm->def->name) < 0) { @@ -338,7 +340,7 @@ static void qemuDomainSnapshotLoad(void *payload, continue; } - def = virDomainSnapshotDefParseString(xmlStr, 0); + def = virDomainSnapshotDefParseString(xmlStr, flags); if (def == NULL) { /* Nothing we can do here, skip this one */ VIR_ERROR(_("Failed to parse snapshot XML from file '%s'"), @@ -8619,8 +8621,19 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, virDomainSnapshotPtr snapshot = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; virDomainSnapshotDefPtr def = NULL; + bool update_current = true; + unsigned int parse_flags = 0; - virCheckFlags(VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA, NULL); + virCheckFlags(VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE | + VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT | + VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA, NULL); + + if (((flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE) && + !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT)) || + (flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA)) + update_current = false; + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE) + parse_flags |= VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE; qemuDriverLock(driver); virUUIDFormat(domain->uuid, uuidstr); @@ -8647,22 +8660,87 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, if (!qemuDomainSnapshotIsAllowed(vm)) goto cleanup; - if (!(def = virDomainSnapshotDefParseString(xmlDesc, 1))) + if (!(def = virDomainSnapshotDefParseString(xmlDesc, parse_flags))) goto cleanup; + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE) { + virDomainSnapshotObjPtr other = NULL; + + /* Prevent circular chains */ + if (def->parent) { + if (STREQ(def->name, def->parent)) { + qemuReportError(VIR_ERR_INVALID_ARG, + _("cannot set snapshot %s as its own parent"), + def->name); + goto cleanup; + } + other = virDomainSnapshotFindByName(&vm->snapshots, def->parent); + if (!other) { + qemuReportError(VIR_ERR_INVALID_ARG, + _("parent %s for snapshot %s not found"), + def->parent, def->name); + goto cleanup; + } + while (other->def->parent) { + if (STREQ(other->def->parent, def->name)) { + qemuReportError(VIR_ERR_INVALID_ARG, + _("parent %s would create cycle to %s"), + other->def->name, def->name); + goto cleanup; + } + other = virDomainSnapshotFindByName(&vm->snapshots, + other->def->parent); + if (!other) { + VIR_WARN("snapshots are inconsistent for %s", + vm->def->name); + break; + } + } + } + + /* Check that any replacement is compatible */ + other = virDomainSnapshotFindByName(&vm->snapshots, def->name); + if (other) { + if ((other->def->state == VIR_DOMAIN_RUNNING || + other->def->state == VIR_DOMAIN_PAUSED) != + (def->state == VIR_DOMAIN_RUNNING || + def->state == VIR_DOMAIN_PAUSED)) { + qemuReportError(VIR_ERR_INVALID_ARG, + _("cannot change between online and offline " + "snapshot state in snapshot %s"), + def->name); + goto cleanup; + } + /* XXX Ensure ABI compatibility before replacing anything. */ + if (other == vm->current_snapshot) { + update_current = true; + vm->current_snapshot = NULL; + } + virDomainSnapshotObjListRemove(&vm->snapshots, other); + } else { + /* XXX Should we do some feasibility checks, like parsing + * qemu-img output to check that def->name matches at + * least one qcow2 snapshot name? */ + } + } + if (!(snap = virDomainSnapshotAssignDef(&vm->snapshots, def))) goto cleanup; def = NULL; - snap->def->state = virDomainObjGetState(vm, NULL); - snap->def->current = true; + if (update_current) + snap->def->current = true; + if (!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE)) + snap->def->state = virDomainObjGetState(vm, NULL); if (vm->current_snapshot) { - snap->def->parent = strdup(vm->current_snapshot->def->name); - if (snap->def->parent == NULL) { - virReportOOMError(); - goto cleanup; + if (!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE)) { + snap->def->parent = strdup(vm->current_snapshot->def->name); + if (snap->def->parent == NULL) { + virReportOOMError(); + goto cleanup; + } } - if (!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA)) { + if (update_current) { vm->current_snapshot->def->current = false; if (qemuDomainSnapshotWriteMetadata(vm, vm->current_snapshot, driver->snapshotDir) < 0) @@ -8672,7 +8750,13 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, } /* actually do the snapshot */ - if (!virDomainObjIsActive(vm)) { + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE) { + /* XXX Should we validate that the redefined snapshot even + * makes sense, such as checking whether the requested parent + * snapshot exists and is not creating a loop, or that + * qemu-img recognizes the snapshot name in at least one of + * the domain's disks? */ + } else if (!virDomainObjIsActive(vm)) { if (qemuDomainSnapshotCreateInactive(vm, snap) < 0) goto cleanup; } else { @@ -8694,7 +8778,7 @@ cleanup: driver->snapshotDir) < 0) VIR_WARN("unable to save metadata for snapshot %s", snap->def->name); - else + else if (update_current) vm->current_snapshot = snap; } else if (snap) { virDomainSnapshotObjListRemove(&vm->snapshots, snap); diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index afe951ccde..636f5f2cf5 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -5655,7 +5655,7 @@ vboxDomainSnapshotCreateXML(virDomainPtr dom, /* VBox has no snapshot metadata, so this flag is trivial. */ virCheckFlags(VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA, NULL); - if (!(def = virDomainSnapshotDefParseString(xmlDesc, 1))) + if (!(def = virDomainSnapshotDefParseString(xmlDesc, 0))) goto cleanup; vboxIIDFromUUID(&domiid, dom->uuid); -- 2.39.5