diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0def87c0aaaa17bce85cff04f81cdcb21b3aabd4..94257ee10586cc721642643c91a17ebf06e8abb6 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 f970782b7e494703ae0ef5d011ff2aaad09cb3ab..cef0d9ee93ab8628c1bfdbd5753705ad87bba420 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 695fb0367436508cab372bd1ca0bf77eec136141..4bbb3153f46d354bb4d1cd54ee6ad84c03be0617 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 9a4bf7d2951ce0bd75366a9b0c14456c8ce8b46b..b002b1d61b5450cb20adf11d03b16bba8025f22f 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 afe951ccde42ad597222cbac267b84d696f69fdf..636f5f2cf5cc79760269e27ee22d692284860afe 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);