Commit 28f8dfdc (1.0.0) added a flag to virDomainGetXMLDesc, but failed to document its effects. And considering that the MIGRATABLE flag has been the source of past bugs (CVE-2014-7823, fixed in commit b1674ad5 (1.2.11), or even cf2d4c60 (1.2.13) where flag mismatch broke virsh edit), make the wording wishy-washy enough to discourage using the flag casually, by mentioning that the resulting XML is more for internal use than for validation against the schema. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- src/libvirt-domain.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index e2ed8b2772..072b92b717 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -2561,6 +2561,15 @@ virDomainGetControlInfo(virDomainPtr domain, * describing CPU capabilities is modified to match actual * capabilities of the host. * + * If @flags contains VIR_DOMAIN_XML_MIGRATABLE, the XML is altered to + * assist in migrations, since the source and destination may be + * running different libvirt versions. This may include trimming + * redundant or default information that might confuse an older + * recipient, or exposing internal details that aid a newer recipient; + * this flag is rejected on read-only connections, and the resulting + * XML might not validate against the schema, so it is mainly for + * internal use. + * * Returns a 0 terminated UTF-8 encoded XML instance, or NULL in case of error. * the caller must free() the returned value. */ -- 2.20.1

virDomainSnapshotDefFormat currently takes two sets of knobs: an 'unsigned int flags' argument that can currently just be VIR_DOMAIN_DEF_FORMAT_SECURE, and an 'int internal' argument used as a bool to determine whether to output an additional element. It then reuses the 'flags' knob to call into virDomainDefFormatInternal(), which takes a different set of flags. In fact, prior to commit 0ecd6851 (1.2.12), the 'flags' argument actually took the public VIR_DOMAIN_XML_SECURE, which was even more confusing. Let's borrow from the style of that earlier commit, by introducing a function for translating from the public flags (VIR_DOMAIN_SNAPSHOT_XML_SECURE was just recently introduced) into a new enum specific to snapshot formatting, and adjust all callers to use snapshot-specific enum values when formatting, and where the formatter now uses a new variable 'domainflags' to make it obvious when we are translating from snapshot flags back to domain flags. We don't even have to use the conversion function for drivers that don't accept the public VIR_DOMAIN_SNAPSHOT_XML_SECURE flag. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- src/conf/snapshot_conf.h | 10 ++++++++-- src/conf/snapshot_conf.c | 28 ++++++++++++++++++++++------ src/esx/esx_driver.c | 1 - src/libvirt_private.syms | 1 + src/qemu/qemu_domain.c | 3 +-- src/qemu/qemu_driver.c | 3 +-- src/test/test_driver.c | 3 +-- src/vbox/vbox_common.c | 5 ++--- src/vz/vz_driver.c | 3 +-- tests/domainsnapshotxml2xmltest.c | 16 +++++++++------- 10 files changed, 46 insertions(+), 27 deletions(-) diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h index 7a175dfc96..4d6e6134a3 100644 --- a/src/conf/snapshot_conf.h +++ b/src/conf/snapshot_conf.h @@ -101,6 +101,13 @@ typedef enum { VIR_DOMAIN_SNAPSHOT_PARSE_OFFLINE = 1 << 3, } virDomainSnapshotParseFlags; +typedef enum { + VIR_DOMAIN_SNAPSHOT_FORMAT_SECURE = 1 << 0, + VIR_DOMAIN_SNAPSHOT_FORMAT_INTERNAL = 1 << 1, +} virDomainSnapshotFormatFlags; + +unsigned int virDomainSnapshotFormatConvertXMLFlags(unsigned int flags); + virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, virCapsPtr caps, virDomainXMLOptionPtr xmlopt, @@ -115,8 +122,7 @@ char *virDomainSnapshotDefFormat(const char *uuidstr, virDomainSnapshotDefPtr def, virCapsPtr caps, virDomainXMLOptionPtr xmlopt, - unsigned int flags, - int internal); + unsigned int flags); int virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr snapshot, int default_snapshot, bool require_match); diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 3372c4df86..652d963f84 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -652,6 +652,19 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def, return ret; } +/* Converts public VIR_DOMAIN_SNAPSHOT_XML_* into + * VIR_DOMAIN_SNAPSHOT_FORMAT_* flags, and silently ignores any other + * flags. */ +unsigned int virDomainSnapshotFormatConvertXMLFlags(unsigned int flags) +{ + unsigned int formatFlags = 0; + + if (flags & VIR_DOMAIN_SNAPSHOT_XML_SECURE) + formatFlags |= VIR_DOMAIN_SNAPSHOT_FORMAT_SECURE; + + return formatFlags; +} + static int virDomainSnapshotDiskDefFormat(virBufferPtr buf, virDomainSnapshotDiskDefPtr disk, @@ -692,15 +705,17 @@ virDomainSnapshotDefFormat(const char *uuidstr, virDomainSnapshotDefPtr def, virCapsPtr caps, virDomainXMLOptionPtr xmlopt, - unsigned int flags, - int internal) + unsigned int flags) { virBuffer buf = VIR_BUFFER_INITIALIZER; size_t i; + int domainflags = VIR_DOMAIN_DEF_FORMAT_INACTIVE; - virCheckFlags(VIR_DOMAIN_DEF_FORMAT_SECURE, NULL); + virCheckFlags(VIR_DOMAIN_SNAPSHOT_FORMAT_SECURE | + VIR_DOMAIN_SNAPSHOT_FORMAT_INTERNAL, NULL); - flags |= VIR_DOMAIN_DEF_FORMAT_INACTIVE; + if (flags & VIR_DOMAIN_SNAPSHOT_FORMAT_SECURE) + domainflags |= VIR_DOMAIN_DEF_FORMAT_SECURE; virBufferAddLit(&buf, "<domainsnapshot>\n"); virBufferAdjustIndent(&buf, 2); @@ -742,7 +757,8 @@ virDomainSnapshotDefFormat(const char *uuidstr, } if (def->dom) { - if (virDomainDefFormatInternal(def->dom, caps, flags, &buf, xmlopt) < 0) + if (virDomainDefFormatInternal(def->dom, caps, domainflags, &buf, + xmlopt) < 0) goto error; } else if (uuidstr) { virBufferAddLit(&buf, "<domain>\n"); @@ -756,7 +772,7 @@ virDomainSnapshotDefFormat(const char *uuidstr, virDomainXMLOptionGetSaveCookie(xmlopt)) < 0) goto error; - if (internal) + if (flags & VIR_DOMAIN_SNAPSHOT_FORMAT_INTERNAL) virBufferAsprintf(&buf, "<active>%d</active>\n", def->current); virBufferAdjustIndent(&buf, -2); diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 379c2bae73..8ddfa93847 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -4204,7 +4204,6 @@ esxDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, virUUIDFormat(snapshot->domain->uuid, uuid_string); xml = virDomainSnapshotDefFormat(uuid_string, &def, priv->caps, priv->xmlopt, - virDomainDefFormatConvertXMLFlags(flags), 0); cleanup: diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index aa1b2c77be..6bfb497648 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -887,6 +887,7 @@ virDomainSnapshotFindByName; virDomainSnapshotForEach; virDomainSnapshotForEachChild; virDomainSnapshotForEachDescendant; +virDomainSnapshotFormatConvertXMLFlags; virDomainSnapshotIsExternal; virDomainSnapshotLocationTypeFromString; virDomainSnapshotLocationTypeToString; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 8682b27037..84b923fbbb 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8397,8 +8397,7 @@ qemuDomainSnapshotWriteMetadata(virDomainObjPtr vm, virUUIDFormat(vm->def->uuid, uuidstr); newxml = virDomainSnapshotDefFormat( uuidstr, snapshot->def, caps, xmlopt, - virDomainDefFormatConvertXMLFlags(QEMU_DOMAIN_FORMAT_LIVE_FLAGS), - 1); + VIR_DOMAIN_SNAPSHOT_FORMAT_SECURE | VIR_DOMAIN_SNAPSHOT_FORMAT_INTERNAL); if (newxml == NULL) return -1; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1f37107a20..b3f4dc6f5c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16287,8 +16287,7 @@ qemuDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, xml = virDomainSnapshotDefFormat(uuidstr, snap->def, driver->caps, driver->xmlopt, - virDomainDefFormatConvertXMLFlags(flags), - 0); + virDomainSnapshotFormatConvertXMLFlags(flags)); cleanup: virDomainObjEndAPI(&vm); diff --git a/src/test/test_driver.c b/src/test/test_driver.c index ce0df1f8e3..a6a67d42e2 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -6209,8 +6209,7 @@ testDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, xml = virDomainSnapshotDefFormat(uuidstr, snap->def, privconn->caps, privconn->xmlopt, - virDomainDefFormatConvertXMLFlags(flags), - 0); + virDomainSnapshotFormatConvertXMLFlags(flags)); cleanup: virDomainObjEndAPI(&vm); diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index d95fe7c7ae..1efd357b49 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -5373,7 +5373,7 @@ vboxSnapshotRedefine(virDomainPtr dom, VIR_FREE(currentSnapshotXmlFilePath); if (virAsprintf(&currentSnapshotXmlFilePath, "%s%s.xml", machineLocationPath, snapshotMachineDesc->currentSnapshot) < 0) goto cleanup; - char *snapshotContent = virDomainSnapshotDefFormat(NULL, def, data->caps, data->xmlopt, VIR_DOMAIN_DEF_FORMAT_SECURE, 0); + char *snapshotContent = virDomainSnapshotDefFormat(NULL, def, data->caps, data->xmlopt, VIR_DOMAIN_SNAPSHOT_FORMAT_SECURE); if (snapshotContent == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unable to get snapshot content")); @@ -6321,8 +6321,7 @@ static char *vboxDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, virUUIDFormat(dom->uuid, uuidstr); memcpy(def->dom->uuid, dom->uuid, VIR_UUID_BUFLEN); ret = virDomainSnapshotDefFormat(uuidstr, def, data->caps, data->xmlopt, - virDomainDefFormatConvertXMLFlags(flags), - 0); + 0); cleanup: virDomainSnapshotDefFree(def); diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index 2d2eaf88a6..066d617524 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -2291,8 +2291,7 @@ vzDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, unsigned int flags) xml = virDomainSnapshotDefFormat(uuidstr, snap->def, privconn->driver->caps, privconn->driver->xmlopt, - virDomainDefFormatConvertXMLFlags(flags), - 0); + virDomainSnapshotFormatConvertXMLFlags(flags)); cleanup: virDomainSnapshotObjListFree(snapshots); diff --git a/tests/domainsnapshotxml2xmltest.c b/tests/domainsnapshotxml2xmltest.c index 2a07fe0789..9eb71780fc 100644 --- a/tests/domainsnapshotxml2xmltest.c +++ b/tests/domainsnapshotxml2xmltest.c @@ -78,13 +78,16 @@ testCompareXMLToXMLFiles(const char *inxml, char *actual = NULL; int ret = -1; virDomainSnapshotDefPtr def = NULL; - unsigned int flags = VIR_DOMAIN_SNAPSHOT_PARSE_DISKS; + unsigned int parseflags = VIR_DOMAIN_SNAPSHOT_PARSE_DISKS; + unsigned int formatflags = VIR_DOMAIN_SNAPSHOT_FORMAT_SECURE; - if (internal) - flags |= VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL; + if (internal) { + parseflags |= VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL; + formatflags |= VIR_DOMAIN_SNAPSHOT_FORMAT_INTERNAL; + } if (redefine) - flags |= VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE; + parseflags |= VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE; if (virTestLoadFile(inxml, &inXmlData) < 0) goto cleanup; @@ -94,13 +97,12 @@ testCompareXMLToXMLFiles(const char *inxml, if (!(def = virDomainSnapshotDefParseString(inXmlData, driver.caps, driver.xmlopt, - flags))) + parseflags))) goto cleanup; if (!(actual = virDomainSnapshotDefFormat(uuid, def, driver.caps, driver.xmlopt, - VIR_DOMAIN_DEF_FORMAT_SECURE, - internal))) + formatflags))) goto cleanup; if (!redefine) { -- 2.20.1

Split out an internal helper that produces format into a virBuffer, similar to what domain_conf.c does, and making the next patch easier to write. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- src/conf/snapshot_conf.c | 103 ++++++++++++++++++++++----------------- 1 file changed, 59 insertions(+), 44 deletions(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 652d963f84..5cb977336a 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -700,92 +700,107 @@ virDomainSnapshotDiskDefFormat(virBufferPtr buf, } -char * -virDomainSnapshotDefFormat(const char *uuidstr, - virDomainSnapshotDefPtr def, - virCapsPtr caps, - virDomainXMLOptionPtr xmlopt, - unsigned int flags) +static int +virDomainSnapshotDefFormatInternal(virBufferPtr buf, + const char *uuidstr, + virDomainSnapshotDefPtr def, + virCapsPtr caps, + virDomainXMLOptionPtr xmlopt, + unsigned int flags) { - virBuffer buf = VIR_BUFFER_INITIALIZER; size_t i; int domainflags = VIR_DOMAIN_DEF_FORMAT_INACTIVE; - virCheckFlags(VIR_DOMAIN_SNAPSHOT_FORMAT_SECURE | - VIR_DOMAIN_SNAPSHOT_FORMAT_INTERNAL, NULL); - if (flags & VIR_DOMAIN_SNAPSHOT_FORMAT_SECURE) domainflags |= VIR_DOMAIN_DEF_FORMAT_SECURE; - virBufferAddLit(&buf, "<domainsnapshot>\n"); - virBufferAdjustIndent(&buf, 2); + virBufferAddLit(buf, "<domainsnapshot>\n"); + virBufferAdjustIndent(buf, 2); - virBufferEscapeString(&buf, "<name>%s</name>\n", def->name); + virBufferEscapeString(buf, "<name>%s</name>\n", def->name); if (def->description) - virBufferEscapeString(&buf, "<description>%s</description>\n", + virBufferEscapeString(buf, "<description>%s</description>\n", def->description); - virBufferAsprintf(&buf, "<state>%s</state>\n", + virBufferAsprintf(buf, "<state>%s</state>\n", virDomainSnapshotStateTypeToString(def->state)); if (def->parent) { - virBufferAddLit(&buf, "<parent>\n"); - virBufferAdjustIndent(&buf, 2); - virBufferEscapeString(&buf, "<name>%s</name>\n", def->parent); - virBufferAdjustIndent(&buf, -2); - virBufferAddLit(&buf, "</parent>\n"); + virBufferAddLit(buf, "<parent>\n"); + virBufferAdjustIndent(buf, 2); + virBufferEscapeString(buf, "<name>%s</name>\n", def->parent); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</parent>\n"); } - virBufferAsprintf(&buf, "<creationTime>%lld</creationTime>\n", + virBufferAsprintf(buf, "<creationTime>%lld</creationTime>\n", def->creationTime); if (def->memory) { - virBufferAsprintf(&buf, "<memory snapshot='%s'", + virBufferAsprintf(buf, "<memory snapshot='%s'", virDomainSnapshotLocationTypeToString(def->memory)); - virBufferEscapeString(&buf, " file='%s'", def->file); - virBufferAddLit(&buf, "/>\n"); + virBufferEscapeString(buf, " file='%s'", def->file); + virBufferAddLit(buf, "/>\n"); } if (def->ndisks) { - virBufferAddLit(&buf, "<disks>\n"); - virBufferAdjustIndent(&buf, 2); + virBufferAddLit(buf, "<disks>\n"); + virBufferAdjustIndent(buf, 2); for (i = 0; i < def->ndisks; i++) { - if (virDomainSnapshotDiskDefFormat(&buf, &def->disks[i], xmlopt) < 0) + if (virDomainSnapshotDiskDefFormat(buf, &def->disks[i], xmlopt) < 0) goto error; } - virBufferAdjustIndent(&buf, -2); - virBufferAddLit(&buf, "</disks>\n"); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</disks>\n"); } if (def->dom) { - if (virDomainDefFormatInternal(def->dom, caps, domainflags, &buf, + if (virDomainDefFormatInternal(def->dom, caps, domainflags, buf, xmlopt) < 0) goto error; } else if (uuidstr) { - virBufferAddLit(&buf, "<domain>\n"); - virBufferAdjustIndent(&buf, 2); - virBufferAsprintf(&buf, "<uuid>%s</uuid>\n", uuidstr); - virBufferAdjustIndent(&buf, -2); - virBufferAddLit(&buf, "</domain>\n"); + virBufferAddLit(buf, "<domain>\n"); + virBufferAdjustIndent(buf, 2); + virBufferAsprintf(buf, "<uuid>%s</uuid>\n", uuidstr); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</domain>\n"); } - if (virSaveCookieFormatBuf(&buf, def->cookie, + if (virSaveCookieFormatBuf(buf, def->cookie, virDomainXMLOptionGetSaveCookie(xmlopt)) < 0) goto error; if (flags & VIR_DOMAIN_SNAPSHOT_FORMAT_INTERNAL) - virBufferAsprintf(&buf, "<active>%d</active>\n", def->current); + virBufferAsprintf(buf, "<active>%d</active>\n", def->current); - virBufferAdjustIndent(&buf, -2); - virBufferAddLit(&buf, "</domainsnapshot>\n"); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</domainsnapshot>\n"); - if (virBufferCheckError(&buf) < 0) - return NULL; + if (virBufferCheckError(buf) < 0) + goto error; - return virBufferContentAndReset(&buf); + return 0; error: - virBufferFreeAndReset(&buf); - return NULL; + virBufferFreeAndReset(buf); + return -1; +} + +char * +virDomainSnapshotDefFormat(const char *uuidstr, + virDomainSnapshotDefPtr def, + virCapsPtr caps, + virDomainXMLOptionPtr xmlopt, + unsigned int flags) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + + virCheckFlags(VIR_DOMAIN_SNAPSHOT_FORMAT_SECURE | + VIR_DOMAIN_SNAPSHOT_FORMAT_INTERNAL, NULL); + if (virDomainSnapshotDefFormatInternal(&buf, uuidstr, def, caps, + xmlopt, flags) < 0) + return NULL; + + return virBufferContentAndReset(&buf); } /* Snapshot Obj functions */ -- 2.20.1

Add a new function to output all of the domain's snapshots in one buffer. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- src/conf/snapshot_conf.h | 8 +++++++- src/conf/snapshot_conf.c | 39 +++++++++++++++++++++++++++++++++++++++ src/libvirt_private.syms | 1 + 3 files changed, 47 insertions(+), 1 deletion(-) diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h index 4d6e6134a3..19ab75f895 100644 --- a/src/conf/snapshot_conf.h +++ b/src/conf/snapshot_conf.h @@ -1,7 +1,7 @@ /* * snapshot_conf.h: domain snapshot XML processing * - * Copyright (C) 2006-2014 Red Hat, Inc. + * Copyright (C) 2006-2019 Red Hat, Inc. * Copyright (C) 2006-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -123,6 +123,12 @@ char *virDomainSnapshotDefFormat(const char *uuidstr, virCapsPtr caps, virDomainXMLOptionPtr xmlopt, unsigned int flags); +int virDomainSnapshotObjListFormat(virBufferPtr buf, + const char *uuidstr, + virDomainSnapshotObjListPtr snapshots, + virCapsPtr caps, + virDomainXMLOptionPtr xmlopt, + unsigned int flags); int virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr snapshot, int default_snapshot, bool require_match); diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 5cb977336a..a572da5b58 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -803,6 +803,45 @@ virDomainSnapshotDefFormat(const char *uuidstr, return virBufferContentAndReset(&buf); } +struct virDomainSnapshotFormatData { + virBufferPtr buf; + const char *uuidstr; + virCapsPtr caps; + virDomainXMLOptionPtr xmlopt; + unsigned int flags; +}; + +static int +virDomainSnapshotFormatOne(void *payload, const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + virDomainSnapshotObjPtr snap = payload; + struct virDomainSnapshotFormatData *data = opaque; + return virDomainSnapshotDefFormatInternal(data->buf, data->uuidstr, + snap->def, data->caps, + data->xmlopt, data->flags); +} + + +int +virDomainSnapshotObjListFormat(virBufferPtr buf, + const char *uuidstr, + virDomainSnapshotObjListPtr snapshots, + virCapsPtr caps, + virDomainXMLOptionPtr xmlopt, + unsigned int flags) +{ + struct virDomainSnapshotFormatData data = { + .buf = buf, + .uuidstr = uuidstr, + .caps = caps, + .xmlopt = xmlopt, + .flags = flags, + }; + return virDomainSnapshotForEach(snapshots, virDomainSnapshotFormatOne, + &data); +} + /* Snapshot Obj functions */ static virDomainSnapshotObjPtr virDomainSnapshotObjNew(void) { diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6bfb497648..c623737c30 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -891,6 +891,7 @@ virDomainSnapshotFormatConvertXMLFlags; virDomainSnapshotIsExternal; virDomainSnapshotLocationTypeFromString; virDomainSnapshotLocationTypeToString; +virDomainSnapshotObjListFormat; virDomainSnapshotObjListFree; virDomainSnapshotObjListGetNames; virDomainSnapshotObjListNew; -- 2.20.1

Make it possible to grab all snapshot XMLs via a single API call, by adding a new internal flag. All callers are adjusted, for now passing NULL and not using the new flag. A new wrapper virDomainDefFormatFull() is added to make it easier for the next patch to add a few callers without having to revisit all existing clients of virDomainDefFormat(). Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- src/conf/domain_conf.h | 8 +++++ src/conf/domain_conf.c | 58 +++++++++++++++++++++++++++++++------ src/conf/snapshot_conf.c | 4 +-- src/network/bridge_driver.c | 3 +- src/qemu/qemu_domain.c | 2 +- 5 files changed, 62 insertions(+), 13 deletions(-) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 9bccd8bcd1..9b13c147da 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3047,6 +3047,7 @@ typedef enum { VIR_DOMAIN_DEF_FORMAT_ALLOW_ROM = 1 << 6, VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT = 1 << 7, VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST = 1 << 8, + VIR_DOMAIN_DEF_FORMAT_SNAPSHOTS = 1 << 9, } virDomainDefFormatFlags; /* Use these flags to skip specific domain ABI consistency checks done @@ -3124,12 +3125,19 @@ unsigned int virDomainDefFormatConvertXMLFlags(unsigned int flags); char *virDomainDefFormat(virDomainDefPtr def, virCapsPtr caps, unsigned int flags); +char *virDomainDefFormatFull(virDomainDefPtr def, + virCapsPtr caps, + virDomainSnapshotObjListPtr snapshots, + virDomainSnapshotObjPtr current_snapshot, + unsigned int flags); char *virDomainObjFormat(virDomainXMLOptionPtr xmlopt, virDomainObjPtr obj, virCapsPtr caps, unsigned int flags); int virDomainDefFormatInternal(virDomainDefPtr def, virCapsPtr caps, + virDomainSnapshotObjListPtr snapshots, + virDomainSnapshotObjPtr current_snapshot, unsigned int flags, virBufferPtr buf, virDomainXMLOptionPtr xmlopt); diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ceeb247ef4..46ae79e738 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1,7 +1,7 @@ /* * domain_conf.c: domain XML processing * - * Copyright (C) 2006-2016 Red Hat, Inc. + * Copyright (C) 2006-2019 Red Hat, Inc. * Copyright (C) 2006-2008 Daniel P. Berrange * Copyright (c) 2015 SUSE LINUX Products GmbH, Nuernberg, Germany. * @@ -28212,6 +28212,8 @@ virDomainVsockDefFormat(virBufferPtr buf, int virDomainDefFormatInternal(virDomainDefPtr def, virCapsPtr caps, + virDomainSnapshotObjListPtr snapshots, + virDomainSnapshotObjPtr current_snapshot, unsigned int flags, virBufferPtr buf, virDomainXMLOptionPtr xmlopt) @@ -28229,9 +28231,16 @@ virDomainDefFormatInternal(virDomainDefPtr def, VIR_DOMAIN_DEF_FORMAT_STATUS | VIR_DOMAIN_DEF_FORMAT_ACTUAL_NET | VIR_DOMAIN_DEF_FORMAT_PCI_ORIG_STATES | - VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST, + VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST | + VIR_DOMAIN_DEF_FORMAT_SNAPSHOTS, -1); + if ((flags & VIR_DOMAIN_DEF_FORMAT_SNAPSHOTS) && !snapshots) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("snapshots requested but not provided")); + goto error; + } + if (!(type = virDomainVirtTypeToString(def->virtType))) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected domain type %d"), def->virtType); @@ -29016,6 +29025,23 @@ virDomainDefFormatInternal(virDomainDefPtr def, virDomainSEVDefFormat(buf, def->sev); + if (flags & VIR_DOMAIN_DEF_FORMAT_SNAPSHOTS) { + unsigned int snapflags = flags & VIR_DOMAIN_DEF_FORMAT_SECURE ? + VIR_DOMAIN_SNAPSHOT_FORMAT_SECURE : 0; + + virBufferAddLit(buf, "<snapshots"); + if (current_snapshot) + virBufferEscapeString(buf, " current='%s'", + current_snapshot->def->name); + virBufferAddLit(buf, ">\n"); + virBufferAdjustIndent(buf, 2); + if (virDomainSnapshotObjListFormat(buf, uuidstr, snapshots, caps, + xmlopt, snapflags)) + goto error; + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</snapshots>\n"); + } + virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</domain>\n"); @@ -29051,16 +29077,29 @@ unsigned int virDomainDefFormatConvertXMLFlags(unsigned int flags) } +char * +virDomainDefFormatFull(virDomainDefPtr def, virCapsPtr caps, + virDomainSnapshotObjListPtr snapshots, + virDomainSnapshotObjPtr current_snapshot, + unsigned int flags) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + + virCheckFlags(VIR_DOMAIN_DEF_FORMAT_COMMON_FLAGS | + VIR_DOMAIN_DEF_FORMAT_SNAPSHOTS, NULL); + if (virDomainDefFormatInternal(def, caps, snapshots, current_snapshot, + flags, &buf, NULL) < 0) + return NULL; + + return virBufferContentAndReset(&buf); +} + + char * virDomainDefFormat(virDomainDefPtr def, virCapsPtr caps, unsigned int flags) { - virBuffer buf = VIR_BUFFER_INITIALIZER; - virCheckFlags(VIR_DOMAIN_DEF_FORMAT_COMMON_FLAGS, NULL); - if (virDomainDefFormatInternal(def, caps, flags, &buf, NULL) < 0) - return NULL; - - return virBufferContentAndReset(&buf); + return virDomainDefFormatFull(def, caps, NULL, NULL, flags); } @@ -29092,7 +29131,8 @@ virDomainObjFormat(virDomainXMLOptionPtr xmlopt, xmlopt->privateData.format(&buf, obj) < 0) goto error; - if (virDomainDefFormatInternal(obj->def, caps, flags, &buf, xmlopt) < 0) + if (virDomainDefFormatInternal(obj->def, caps, NULL, NULL, flags, &buf, + xmlopt) < 0) goto error; virBufferAdjustIndent(&buf, -2); diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index a572da5b58..963dc10247 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -754,8 +754,8 @@ virDomainSnapshotDefFormatInternal(virBufferPtr buf, } if (def->dom) { - if (virDomainDefFormatInternal(def->dom, caps, domainflags, buf, - xmlopt) < 0) + if (virDomainDefFormatInternal(def->dom, caps, NULL, NULL, domainflags, + buf, xmlopt) < 0) goto error; } else if (uuidstr) { virBufferAddLit(buf, "<domain>\n"); diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index b3ca5b8a15..bf2045a753 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -226,7 +226,8 @@ networkRunHook(virNetworkObjPtr obj, goto cleanup; if (virNetworkDefFormatBuf(&buf, def, 0) < 0) goto cleanup; - if (dom && virDomainDefFormatInternal(dom, NULL, 0, &buf, NULL) < 0) + if (dom && virDomainDefFormatInternal(dom, NULL, NULL, NULL, 0, &buf, + NULL) < 0) goto cleanup; virBufferAdjustIndent(&buf, -2); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 84b923fbbb..cb1665c8f9 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7881,7 +7881,7 @@ qemuDomainDefFormatBufInternal(virQEMUDriverPtr driver, } format: - ret = virDomainDefFormatInternal(def, caps, + ret = virDomainDefFormatInternal(def, caps, NULL, NULL, virDomainDefFormatConvertXMLFlags(flags), buf, driver->xmlopt); -- 2.20.1

On 2/27/19 10:27 AM, John Ferlan wrote: Ooh, nice idea, especially since I _will_ be adding more parameters for bulk listing of checkpoints :) Will do for v3 (since, as you point out, everything beyond patch 1 is now 5.2 material). Yep. Can do. Or maybe virDomainSnapshotObjListFormat() should do ALL of the work (but then it needs a pointer to the current snapshot - but then again, you also made me question whether the current snapshot should be an internal detail to virDomainSnapshotObjList rather than tracked separately, so that alone is a separate cleanup potentially worth doing first). I'm not sure a childrenBuf adds much. After all, whether we do: most output direct to main buf snapshot output to children buf concatenate children buf to main buf or most output direct to main buf snapshot output to main buf we get the same result: on success, main buf is populated; on failure, main buf may be half-built but it is going to be thrown away. The alternative is to fix all current callers to pass the extra parameter(s) even when they don't use them. Which may not be too bad, if I take your advice to use a struct (so all existing callers pass NULL, and don't have to be touched again), instead of my current approach of adding yet more parameters for every additional thing to be printed. I don't necessarily need this helper function (test_driver.c was the only immediate beneficiary), but having it reduced the churn (all other non-qemu drivers have to be touched due to an added parameter, if I don't add it). It may go away once I implement your idea of an auxiliary struct (where we have the one-time hit to other drivers to pass NULL, but then avoid further churn). This is slightly different than FormatFull() - that one accepts VIR_DOMAIN_XML_SNAPSHOTS, while this one rejects it. (In other words, it is one last safety check that no one requests snapshot XML without also passing in a snapshots objlist to be printed). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org

On 2/23/19 4:24 PM, Eric Blake wrote: Some day maybe there's a helper that can handle both uses... virDomainSnapshotObjListParse (consistency w/ virDomainSnapshotObjListFormat) int virDomainSnapshotObjListParse(...) VIR_AUTOFREE(char *) current = NULL; Typically it's been requested to keep all the VIR_AUTO* at the end of arguments... Of course beyond the one change Peter decided not to re-open his editor to modify after review comment requested otherwise. [...] virDomainSnapshotObjListParse; Simple enough adjustments... Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; John

On 2/23/19 4:24 PM, Eric Blake wrote: [...] Should we even try to say that "and supported by the target libvirt system with the appropriate version of the software installed" ;-)... I know implied somewhat - but perhaps notable in this (and future) cases because of the issue mentioned in the commit message that outward facing docs consumers may never read. Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; John

Continue the work of the previous patch in making it possible to copy the state of a transient domain with snapshots from one host to another, by allowing the destination to perform bulk redefines. Note that the destination still has to do separate calls for creating/defining the domain first, and then redefining the snapshots (that is, there is intentional asymmetry between dumping the list in virDomainGetXMLDesc() but redefining it via virDomainSnapshotCreateXML()), but this is better than the previous state of having to make multiple REDEFINE calls. The bulk flag requires no pre-existing snapshot metadata (as that makes life much easier if there is a failure encountered partway through the list processing - simply remove all other snapshot metadatas), and makes no guarantees on which snapshot (when there are multiple) will actually be returned. Actual driver implementations will be in later patches. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- include/libvirt/libvirt-domain-snapshot.h | 3 +++ src/libvirt-domain-snapshot.c | 21 ++++++++++++++++++--- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/include/libvirt/libvirt-domain-snapshot.h b/include/libvirt/libvirt-domain-snapshot.h index 2532b99c58..113fe4a18b 100644 --- a/include/libvirt/libvirt-domain-snapshot.h +++ b/include/libvirt/libvirt-domain-snapshot.h @@ -71,6 +71,9 @@ typedef enum { VIR_DOMAIN_SNAPSHOT_CREATE_LIVE = (1 << 8), /* create the snapshot while the guest is running */ + VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST = (1 << 9), /* parse multiple + snapshots in one + API call */ } virDomainSnapshotCreateFlags; /* Take a snapshot of the current VM state */ diff --git a/src/libvirt-domain-snapshot.c b/src/libvirt-domain-snapshot.c index 4e46f0876b..a661bcbff9 100644 --- a/src/libvirt-domain-snapshot.c +++ b/src/libvirt-domain-snapshot.c @@ -102,7 +102,7 @@ virDomainSnapshotGetConnect(virDomainSnapshotPtr snapshot) * @flags: bitwise-OR of virDomainSnapshotCreateFlags * * Creates a new snapshot of a domain based on the snapshot xml - * contained in xmlDesc. + * contained in xmlDesc, with a top-level <domainsnapshot> element. * * If @flags is 0, the domain can be active, in which case the * snapshot will be a system checkpoint (both disk state and runtime @@ -131,8 +131,17 @@ virDomainSnapshotGetConnect(virDomainSnapshotPtr snapshot) * guest-visible layout. When redefining a snapshot name that does * not exist, the hypervisor may validate that reverting to the * snapshot appears to be possible (for example, disk images have - * snapshot contents by the requested name). Not all hypervisors - * support these flags. + * snapshot contents by the requested name). Alternatively, if @flags + * includes VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST (which requires + * VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE and is incompatible with + * VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT), and the domain has no existing + * snapshot metadata, then @xmlDesc is parsed as a top-level + * <snapshots> element with an optional current='name' attribute, and + * containing one or more <domainsnapshot> children (as produced by + * virDomainGetXMLDesc() with the flag VIR_DOMAIN_XML_SNAPSHOTS), to + * do a bulk redefine of all snapshots at once (it is unspecified + * which of the redefined snapshots will be used as the return value + * on success). Not all hypervisors support these flags. * * If @flags includes VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA, then the * domain's disk images are modified according to @xmlDesc, but then @@ -218,6 +227,9 @@ virDomainSnapshotCreateXML(virDomainPtr domain, VIR_REQUIRE_FLAG_GOTO(VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT, VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE, error); + VIR_REQUIRE_FLAG_GOTO(VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST, + VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE, + error); VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE, VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA, @@ -225,6 +237,9 @@ virDomainSnapshotCreateXML(virDomainPtr domain, VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE, VIR_DOMAIN_SNAPSHOT_CREATE_HALT, error); + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST, + VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT, + error); if (conn->driver->domainSnapshotCreateXML) { virDomainSnapshotPtr ret; -- 2.20.1

Add flags to the 'dumpxml' and 'snapshot-create' commands to pass the newly-added bulk snapshot flags through. For command-line convenience, I intentionally made --redefine-list imply --redefine, even though the counterpart C flags are distinct (and you get an error if you pass _REDEFINE_LIST without _REDEFINE). Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- tools/virsh-domain.c | 7 +++++++ tools/virsh-snapshot.c | 14 ++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 5699018dcc..78854b1e0a 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -10055,6 +10055,10 @@ static const vshCmdOptDef opts_dumpxml[] = { .type = VSH_OT_BOOL, .help = N_("provide XML suitable for migrations") }, + {.name = "snapshots", + .type = VSH_OT_BOOL, + .help = N_("include all domain snapshots in XML dump"), + }, {.name = NULL} }; @@ -10069,6 +10073,7 @@ cmdDumpXML(vshControl *ctl, const vshCmd *cmd) bool secure = vshCommandOptBool(cmd, "security-info"); bool update = vshCommandOptBool(cmd, "update-cpu"); bool migratable = vshCommandOptBool(cmd, "migratable"); + bool snapshots = vshCommandOptBool(cmd, "snapshots"); if (inactive) flags |= VIR_DOMAIN_XML_INACTIVE; @@ -10078,6 +10083,8 @@ cmdDumpXML(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_XML_UPDATE_CPU; if (migratable) flags |= VIR_DOMAIN_XML_MIGRATABLE; + if (snapshots) + flags |= VIR_DOMAIN_XML_SNAPSHOTS; if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) return false; diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index 6cadb2b0d6..a58731c46e 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -80,6 +80,13 @@ virshSnapshotCreate(vshControl *ctl, virDomainPtr dom, const char *buffer, goto cleanup; } + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST) { + vshPrintExtra(ctl, "%s", + _("Domain snapshot list imported successfully")); + ret = true; + goto cleanup; + } + name = virDomainSnapshotGetName(snapshot); if (!name) { vshError(ctl, "%s", _("Could not get snapshot name")); @@ -122,6 +129,10 @@ static const vshCmdOptDef opts_snapshot_create[] = { .help = N_("redefine metadata for existing snapshot") }, VIRSH_COMMON_OPT_CURRENT(N_("with redefine, set current snapshot")), + {.name = "redefine-list", + .type = VSH_OT_BOOL, + .help = N_("bulk define a set of snapshots, implies --redefine"), + }, {.name = "no-metadata", .type = VSH_OT_BOOL, .help = N_("take snapshot but create no metadata") @@ -177,6 +188,9 @@ cmdSnapshotCreate(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC; if (vshCommandOptBool(cmd, "live")) flags |= VIR_DOMAIN_SNAPSHOT_CREATE_LIVE; + if (vshCommandOptBool(cmd, "redefine-list")) + flags |= VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE | + VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST; if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) goto cleanup; -- 2.20.1

On 2/23/19 4:24 PM, Eric Blake wrote: Hmm... I don't think @snap is used since cleanup: code filters on REDEFINE_LIST, so it would seem this would be leaked then. Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; John

Implement the new flags for bulk snapshot dump and redefine. This borrows from ideas in the test driver, but is further complicated by the fact that qemu must write snapshot XML to disk, and thus must do additional validation after the initial parse to ensure the user didn't attempt to rename a snapshot with "../" or similar. Of note: all prior callers of qemuDomainSnapshotDiscardAllMetadata() were at points where it did not matter if vm->current_snapshot and the metaroot in vm->snapshots were left pointing to stale memory, because we were about to delete the entire vm object; but now it is important to reset things properly so that the domain still shows as having no snapshots on failure. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- src/qemu/qemu_domain.h | 2 +- src/qemu/qemu_domain.c | 35 +++++++++++++++++------ src/qemu/qemu_driver.c | 64 ++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 89 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 7c6b50184c..37c9813ec5 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -683,7 +683,7 @@ int qemuDomainSnapshotWriteMetadata(virDomainObjPtr vm, virDomainSnapshotObjPtr snapshot, virCapsPtr caps, virDomainXMLOptionPtr xmlopt, - char *snapshotDir); + const char *snapshotDir); int qemuDomainSnapshotForEachQcow2(virQEMUDriverPtr driver, virDomainObjPtr vm, diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index cb1665c8f9..cf8b6e8eaf 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7704,6 +7704,7 @@ qemuDomainDefCopy(virQEMUDriverPtr driver, static int qemuDomainDefFormatBufInternal(virQEMUDriverPtr driver, + virDomainObjPtr vm, virDomainDefPtr def, virCPUDefPtr origCPU, unsigned int flags, @@ -7713,8 +7714,10 @@ qemuDomainDefFormatBufInternal(virQEMUDriverPtr driver, virDomainDefPtr copy = NULL; virCapsPtr caps = NULL; virQEMUCapsPtr qemuCaps = NULL; + bool snapshots = flags & VIR_DOMAIN_XML_SNAPSHOTS; - virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS | VIR_DOMAIN_XML_UPDATE_CPU, -1); + virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS | VIR_DOMAIN_XML_UPDATE_CPU | + VIR_DOMAIN_XML_SNAPSHOTS, -1); if (!(caps = virQEMUDriverGetCapabilities(driver, false))) goto cleanup; @@ -7881,7 +7884,14 @@ qemuDomainDefFormatBufInternal(virQEMUDriverPtr driver, } format: - ret = virDomainDefFormatInternal(def, caps, NULL, NULL, + if (snapshots && !vm) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("snapshots XML requested but not provided")); + goto cleanup; + } + ret = virDomainDefFormatInternal(def, caps, + snapshots ? vm->snapshots : NULL, + snapshots ? vm->current_snapshot : NULL, virDomainDefFormatConvertXMLFlags(flags), buf, driver->xmlopt); @@ -7899,19 +7909,21 @@ qemuDomainDefFormatBuf(virQEMUDriverPtr driver, unsigned int flags, virBufferPtr buf) { - return qemuDomainDefFormatBufInternal(driver, def, NULL, flags, buf); + return qemuDomainDefFormatBufInternal(driver, NULL, def, NULL, flags, buf); } static char * qemuDomainDefFormatXMLInternal(virQEMUDriverPtr driver, + virDomainObjPtr vm, virDomainDefPtr def, virCPUDefPtr origCPU, unsigned int flags) { virBuffer buf = VIR_BUFFER_INITIALIZER; - if (qemuDomainDefFormatBufInternal(driver, def, origCPU, flags, &buf) < 0) + if (qemuDomainDefFormatBufInternal(driver, vm, def, origCPU, flags, + &buf) < 0) return NULL; return virBufferContentAndReset(&buf); @@ -7923,7 +7935,7 @@ qemuDomainDefFormatXML(virQEMUDriverPtr driver, virDomainDefPtr def, unsigned int flags) { - return qemuDomainDefFormatXMLInternal(driver, def, NULL, flags); + return qemuDomainDefFormatXMLInternal(driver, NULL, def, NULL, flags); } @@ -7942,7 +7954,7 @@ char *qemuDomainFormatXML(virQEMUDriverPtr driver, origCPU = priv->origCPU; } - return qemuDomainDefFormatXMLInternal(driver, def, origCPU, flags); + return qemuDomainDefFormatXMLInternal(driver, vm, def, origCPU, flags); } char * @@ -7959,7 +7971,7 @@ qemuDomainDefFormatLive(virQEMUDriverPtr driver, if (compatible) flags |= VIR_DOMAIN_XML_MIGRATABLE; - return qemuDomainDefFormatXMLInternal(driver, def, origCPU, flags); + return qemuDomainDefFormatXMLInternal(driver, NULL, def, origCPU, flags); } @@ -8386,7 +8398,7 @@ qemuDomainSnapshotWriteMetadata(virDomainObjPtr vm, virDomainSnapshotObjPtr snapshot, virCapsPtr caps, virDomainXMLOptionPtr xmlopt, - char *snapshotDir) + const char *snapshotDir) { char *newxml = NULL; int ret = -1; @@ -8605,6 +8617,7 @@ qemuDomainSnapshotDiscardAllMetadata(virQEMUDriverPtr driver, virDomainObjPtr vm) { virQEMUSnapRemove rem; + virDomainSnapshotObjPtr snap; rem.driver = driver; rem.vm = vm; @@ -8612,6 +8625,12 @@ qemuDomainSnapshotDiscardAllMetadata(virQEMUDriverPtr driver, rem.err = 0; virDomainSnapshotForEach(vm->snapshots, qemuDomainSnapshotDiscardAll, &rem); + if (rem.current) + vm->current_snapshot = NULL; + /* Update the metaroot to match that all children were dropped */ + snap = virDomainSnapshotFindByName(vm->snapshots, NULL); + snap->nchildren = 0; + snap->first_child = NULL; return rem.err; } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b3f4dc6f5c..4df9e18e4c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7337,8 +7337,8 @@ static char virDomainObjPtr vm; char *ret = NULL; - virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS | VIR_DOMAIN_XML_UPDATE_CPU, - NULL); + virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS | VIR_DOMAIN_XML_UPDATE_CPU | + VIR_DOMAIN_XML_SNAPSHOTS, NULL); if (!(vm = qemuDomObjFromDomain(dom))) goto cleanup; @@ -15733,6 +15733,31 @@ qemuDomainSnapshotValidate(virDomainSnapshotDefPtr def, int state, return 0; } +/* Struct and hash-iterator callback used when bulk redefining snapshots */ +struct qemuDomainSnapshotBulk { + virDomainObjPtr vm; + virQEMUDriverPtr driver; + const char *snapshotDir; + unsigned int flags; +}; + +static int +qemuDomainSnapshotBulkRedefine(void *payload, const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + virDomainSnapshotObjPtr snap = payload; + struct qemuDomainSnapshotBulk *data = opaque; + + if (qemuDomainSnapshotValidate(snap->def, snap->def->state, + data->flags) < 0) + return -1; + if (qemuDomainSnapshotWriteMetadata(data->vm, snap, data->driver->caps, + data->driver->xmlopt, + data->snapshotDir) < 0) + return -1; + return 0; +} + static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, const char *xmlDesc, @@ -15762,7 +15787,8 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT | VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE | VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC | - VIR_DOMAIN_SNAPSHOT_CREATE_LIVE, NULL); + VIR_DOMAIN_SNAPSHOT_CREATE_LIVE | + VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST, NULL); VIR_REQUIRE_FLAG_RET(VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE, VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY, @@ -15794,6 +15820,38 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, goto cleanup; } + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST) { + struct qemuDomainSnapshotBulk bulk = { + .vm = vm, + .driver = driver, + .snapshotDir = cfg->snapshotDir, + .flags = flags, + }; + + if (virDomainSnapshotDefParseList(xmlDesc, vm->def->uuid, + vm->snapshots, &vm->current_snapshot, + caps, driver->xmlopt, + parse_flags) < 0) + goto cleanup; + /* Validate and save the snapshots to disk. Since we don't get + * here unless there were no snapshots beforehand, just delete + * everything if anything failed, ignoring further errors. */ + if (virDomainSnapshotForEach(vm->snapshots, + qemuDomainSnapshotBulkRedefine, + &bulk) < 0) { + virErrorPtr orig_err = virSaveLastError(); + + qemuDomainSnapshotDiscardAllMetadata(driver, vm); + virSetError(orig_err); + virFreeError(orig_err); + goto cleanup; + } + /* Return is arbitrary, so use the first root */ + snap = virDomainSnapshotFindByName(vm->snapshots, NULL); + snapshot = virGetDomainSnapshot(domain, snap->first_child->def->name); + goto cleanup; + } + if (!vm->persistent && (flags & VIR_DOMAIN_SNAPSHOT_CREATE_HALT)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cannot halt after transient domain snapshot")); -- 2.20.1

On 2/27/19 11:27 AM, John Ferlan wrote: My fault for too many patches in flight at once. As I'll be doing a v3 anyways, that one wil be cleaner. Yeah, I debated about it. Since you called me on it, I'll make the split. It's a programmer's error, not user-triggerable. (If we could assert() or abort(), that's what I'd have done instead). And really, it is no snapshots were provided, because it is vm->snapshots that we depend on. It's a ripple effect - I had to decide how many interfaces needed an additional parameter, even if the caller wouldn't be using it. But your idea of an auxiliary struct may make it nicer. Yeah, but it's not leaked, and it let me avoid a long line. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org