[libvirt] [PATCH v2 00/11] bulk snapshot list/redefine (incremental backup saga)
While looking at my work on incremental backups, Nir raised a good point: if we want to recreate a set of known checkpoints on one machine that will be taking over a domain from another machine, my initial proposal required making multiple API calls to list the XML for each checkpoint on the source, and then more API calls to redefine each checkpoint on the destination; it also had the drawback that the list has to be presented in topological order (the API won't let you define a child checkpoint if the parent is not defined first). He asked if I could instead add bulk APIs, for getting the XML for all checkpoints at once, and then for redefining all checkpoints at once. Since my checkpoint code borrows heavily from concepts in the snapshot code, I chose to tackle the problem by starting with this series, which does the same thing for snapshots as what I plan to do for checkpoints. That is, since this patch series adds virDomainGetXMLDesc(, VIR_DOMAIN_XML_SNAPSHOTS) and virDomainSnapshotCreateXML(, VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST), the checkpoint series will add virDOmainGetXMLDesc(, VIR_DOMAIN_XML_CHECKPOINTS) and virDomainCheckpointCreateXML(, VIR_DOMAIN_CHECKPOINT_CREATE_REDEFINE_LIST) with very similar code. Also available at: https://repo.or.cz/libvirt/ericb.git/shortlog/refs/tags/snapshot-bulk-v2 Depends on: https://www.redhat.com/archives/libvir-list/2019-February/msg01263.html (virsh: Treat \n like ; in batch mode) https://www.redhat.com/archives/libvir-list/2019-February/msg01343.html (fix snapshot --redefine bugs) Since v1: Finish implementing the redefine code Split the patches slightly differently: all snapshot_conf code first, then just the public API changes, then virsh wrappers, then per-driver implementations for test and qemu. Rebase on top of bug fix and virsh feature addition found in the meantime. 001/11:[----] [--] 'domain: Document VIR_DOMAIN_XML_MIGRATABLE' 002/11:[----] [-C] 'snapshot: Give virDomainSnapshotDefFormat its own flags' 003/11:[0012] [FC] 'snapshot: Refactor virDomainSnapshotDefFormat' 004/11:[----] [-C] 'snapshot: Add virDomainSnapshotObjListFormat' 005/11:[----] [-C] 'domain: Expand virDomainDefFormatInternal with snapshots' 006/11:[down] 'snapshot: Add virDomainSnapshotDefParseList' 007/11:[0041] [FC] 'domain: Add VIR_DOMAIN_XML_SNAPSHOTS flag' 008/11:[0013] [FC] 'snapshot: Add VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST flag' 009/11:[down] 'virsh: Expose bulk snapshot dumpxml/redefine' 010/11:[down] 'test: Implement bulk snapshot operations' 011/11:[down] 'qemu: Implement bulk snapshot operations' Eric Blake (11): domain: Document VIR_DOMAIN_XML_MIGRATABLE snapshot: Give virDomainSnapshotDefFormat its own flags snapshot: Refactor virDomainSnapshotDefFormat snapshot: Add virDomainSnapshotObjListFormat domain: Expand virDomainDefFormatInternal with snapshots snapshot: Add virDomainSnapshotDefParseList domain: Add VIR_DOMAIN_XML_SNAPSHOTS flag snapshot: Add VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST flag virsh: Expose bulk snapshot dumpxml/redefine test: Implement bulk snapshot operations qemu: Implement bulk snapshot operations include/libvirt/libvirt-domain-snapshot.h | 3 + include/libvirt/libvirt-domain.h | 1 + src/conf/domain_conf.h | 8 + src/conf/snapshot_conf.h | 25 +- src/qemu/qemu_domain.h | 2 +- src/conf/domain_conf.c | 71 +++++- src/conf/snapshot_conf.c | 297 ++++++++++++++++++---- src/esx/esx_driver.c | 1 - src/libvirt-domain-snapshot.c | 21 +- src/libvirt-domain.c | 14 + src/libvirt_private.syms | 3 + src/network/bridge_driver.c | 3 +- src/qemu/qemu_domain.c | 36 ++- src/qemu/qemu_driver.c | 67 ++++- src/test/test_driver.c | 28 +- src/vbox/vbox_common.c | 5 +- src/vz/vz_driver.c | 3 +- tests/domainsnapshotxml2xmltest.c | 16 +- tools/virsh-domain.c | 7 + tools/virsh-snapshot.c | 14 + 20 files changed, 523 insertions(+), 102 deletions(-) -- 2.20.1
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@redhat.com> --- 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
On 2/23/19 4:24 PM, Eric Blake wrote:
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@redhat.com> --- src/libvirt-domain.c | 9 +++++++++ 1 file changed, 9 insertions(+)
Reviewed-by: John Ferlan <jferlan@redhat.com> This one is seemingly safe for freeze - John
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@redhat.com> --- 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(¤tSnapshotXmlFilePath, "%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
On 2/23/19 4:24 PM, Eric Blake wrote:
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@redhat.com> --- 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.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. */
NIT: extra spaces between . and *
+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; +} +
[...]
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
[...]
@@ -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);
NIT: The "0);" could fit on the previous line.
cleanup: virDomainSnapshotDefFree(def);
[...] Reviewed-by: John Ferlan <jferlan@redhat.com> This and the rest would be 5.2.0 related... John
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@redhat.com> --- 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
On 2/23/19 4:24 PM, Eric Blake wrote:
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@redhat.com> --- src/conf/snapshot_conf.c | 103 ++++++++++++++++++++++----------------- 1 file changed, 59 insertions(+), 44 deletions(-)
If you feel compelled - you could add an extra blank line between virDomainSnapshotDefFormat and virDomainSnapshotDefFormatInternal. Reviewed-by: John Ferlan <jferlan@redhat.com> John
Add a new function to output all of the domain's snapshots in one buffer. Signed-off-by: Eric Blake <eblake@redhat.com> --- 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
On 2/23/19 4:24 PM, Eric Blake wrote:
Add a new function to output all of the domain's snapshots in one buffer.
Signed-off-by: Eric Blake <eblake@redhat.com> --- 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.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,
One arg per line.
+ void *opaque) +{ + virDomainSnapshotObjPtr snap = payload; + struct virDomainSnapshotFormatData *data = opaque; + return virDomainSnapshotDefFormatInternal(data->buf, data->uuidstr, + snap->def, data->caps, + data->xmlopt, data->flags); +} + +
There's no parameter and method description here leading me to wonder...
+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, + };
...Hmm... so should this also clear and reset buffer if virDomainSnapshotForEach returns -1 or trust the caller to do that? With a bit of extra docs and since I have read the next patch... Reviewed-by: John Ferlan <jferlan@redhat.com> John
+ 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;
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@redhat.com> --- 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/23/19 4:24 PM, Eric Blake wrote:
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
Not really the next any more it seems - definitely in future ones though.
next patch to add a few callers without having to revisit all existing clients of virDomainDefFormat().
Signed-off-by: Eric Blake <eblake@redhat.com> --- 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)
Rather than possibly continually adding parameters to virDomainDefFormatInternal, maybe we should take the plunge now to create a local struct that would be passed along that would fill in fields based on what "extra" format flags beyond the common set are being used. struct virDomainDefFormatInternalFlagsData { virDomainSnapshotObjListPtr snapshots; virDomainSnapshotObjPtr current_snapshot; }; I am of course assuming checkpoints in the future will do something similar.
@@ -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);
I think all of the next hunk should be in it's own helper method
+ 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))
So this answers my question from patch4... Anyway, the return checking should be < 0 and I believe the @childrenBuf is how other similar constructs attempted for format the child XML output - I believe it could be used in this case as well or at least a similarly named/used childrenBuf in a method/helper.
+ 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)
Naming is hard, but is "Full" just another way of saying "all flags" not just the common ones? Not that we necessarily want to see N different implementations of virDomainDefFormatXXXX; however, if at some point in the future someone wanted to list all Checkpoints (;-)), but not all Snapshots - then would *Full still fit the bill or would we need a new method. Just want to be sure we rightly name it considering your knowledge of intended future changes. It almost seems DefFormat is "format only common flags" while DefFormatFull would be format all flags that virDomainDefFormatConvertXMLFlags can handle beyond the original set. If you feel *Full is fine - I'm not against it - just throwing out a something to consider...
+{ + 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);
Considering virDomainDefFormatFull does check this - do we need to also check it? John
- if (virDomainDefFormatInternal(def, caps, flags, &buf, NULL) < 0) - return NULL; - - return virBufferContentAndReset(&buf); + return virDomainDefFormatFull(def, caps, NULL, NULL, flags); }
[...]
On 2/27/19 10:27 AM, John Ferlan wrote:
On 2/23/19 4:24 PM, Eric Blake wrote:
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
Not really the next any more it seems - definitely in future ones though.
next patch to add a few callers without having to revisit all existing clients of virDomainDefFormat().
@@ -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)
Rather than possibly continually adding parameters to virDomainDefFormatInternal, maybe we should take the plunge now to create a local struct that would be passed along that would fill in fields based on what "extra" format flags beyond the common set are being used.
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).
struct virDomainDefFormatInternalFlagsData { virDomainSnapshotObjListPtr snapshots; virDomainSnapshotObjPtr current_snapshot; };
I am of course assuming checkpoints in the future will do something similar.
Yep.
@@ -29016,6 +29025,23 @@ virDomainDefFormatInternal(virDomainDefPtr def,
virDomainSEVDefFormat(buf, def->sev);
I think all of the next hunk should be in it's own helper method
+ 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))
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).
So this answers my question from patch4...
Anyway, the return checking should be < 0 and I believe the @childrenBuf is how other similar constructs attempted for format the child XML output - I believe it could be used in this case as well or at least a similarly named/used childrenBuf in a method/helper.
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.
+char * +virDomainDefFormatFull(virDomainDefPtr def, virCapsPtr caps, + virDomainSnapshotObjListPtr snapshots, + virDomainSnapshotObjPtr current_snapshot, + unsigned int flags)
Naming is hard, but is "Full" just another way of saying "all flags" not just the common ones?
Not that we necessarily want to see N different implementations of virDomainDefFormatXXXX; however, if at some point in the future someone wanted to list all Checkpoints (;-)), but not all Snapshots - then would *Full still fit the bill or would we need a new method.
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).
Just want to be sure we rightly name it considering your knowledge of intended future changes.
It almost seems DefFormat is "format only common flags" while DefFormatFull would be format all flags that virDomainDefFormatConvertXMLFlags can handle beyond the original set.
If you feel *Full is fine - I'm not against it - just throwing out a something to consider...
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).
+{ + 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);
Considering virDomainDefFormatFull does check this - do we need to also check it?
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
Add a new function to make it possible to parse a list of snapshots at once. This is a counterpart to the previous patch making it possible to produce all snapshots in a single XML string, and intentionally parses the same top-level element <snapshots> with an optional attribute current='name'. Note that existing REDEFINE code uses virDomainSnapshotRedefinePrep() for some sanity checking - much of that checking involves parent/child relationships (which make sense when doing one element at a time and worrying about replacement), but where this patch gets away with the much simpler final call to virDomainSnapshotUpdateRelations() (for all relationship checking after the end, since we know we started with no relations at all, and since checking parent relationships per-snapshot is not viable as we don't control which order the snapshots appear in). All other domain-agnostic sanity checks used during a redefinition are copied here. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/snapshot_conf.h | 7 ++ src/conf/snapshot_conf.c | 135 +++++++++++++++++++++++++++++++++++++++ src/libvirt_private.syms | 1 + 3 files changed, 143 insertions(+) diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h index 19ab75f895..6a92241fe6 100644 --- a/src/conf/snapshot_conf.h +++ b/src/conf/snapshot_conf.h @@ -117,6 +117,13 @@ virDomainSnapshotDefPtr virDomainSnapshotDefParseNode(xmlDocPtr xml, virCapsPtr caps, virDomainXMLOptionPtr xmlopt, unsigned int flags); +int virDomainSnapshotDefParseList(const char *xmlSstr, + const unsigned char *domain_uuid, + virDomainSnapshotObjListPtr snapshots, + virDomainSnapshotObjPtr *current_snap, + virCapsPtr caps, + virDomainXMLOptionPtr xmlopt, + unsigned int flags); void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def); char *virDomainSnapshotDefFormat(const char *uuidstr, virDomainSnapshotDefPtr def, diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 963dc10247..61e26726e9 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -426,6 +426,141 @@ virDomainSnapshotDefParseString(const char *xmlStr, } +int virDomainSnapshotDefParseList(const char *xmlStr, + const unsigned char *domain_uuid, + virDomainSnapshotObjListPtr snapshots, + virDomainSnapshotObjPtr *current_snap, + virCapsPtr caps, + virDomainXMLOptionPtr xmlopt, + unsigned int flags) +{ + int ret = -1; + xmlDocPtr xml; + xmlNodePtr root; + xmlXPathContextPtr ctxt = NULL; + char *current = NULL; + int n; + size_t i; + VIR_AUTOFREE(xmlNodePtr *) nodes = NULL; + int keepBlanksDefault = xmlKeepBlanksDefault(0); + + if (!(flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE) || + (flags & VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("incorrect flags for bulk parse")); + return -1; + } + if (snapshots->metaroot.nchildren || *current_snap) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("bulk define of snapshots only possible with " + "no existing snapshot")); + return -1; + } + + if (!(xml = virXMLParse(NULL, xmlStr, _("(domain_snapshot)")))) + goto cleanup; + + root = xmlDocGetRootElement(xml); + if (!virXMLNodeNameEqual(root, "snapshots")) { + virReportError(VIR_ERR_XML_ERROR, + _("unexpected root element <%s>, " + "expecting <snapshots>"), root->name); + goto cleanup; + } + ctxt = xmlXPathNewContext(xml); + if (ctxt == NULL) { + virReportOOMError(); + goto cleanup; + } + ctxt->node = root; + current = virXMLPropString(root, "current"); + + if ((n = virXPathNodeSet("./domainsnapshot", ctxt, &nodes)) < 0) + goto cleanup; + if (!n) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("expected at least one <domainsnapshot> child")); + goto cleanup; + } + + for (i = 0; i < n; i++) { + virDomainSnapshotDefPtr def; + virDomainSnapshotObjPtr snap; + int align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL; + bool align_match = true; + bool offline; + + def = virDomainSnapshotDefParseNode(xml, nodes[i], caps, xmlopt, flags); + if (!def) + goto cleanup; + if (!(snap = virDomainSnapshotAssignDef(snapshots, def))) { + virDomainSnapshotDefFree(def); + goto cleanup; + } + /* Sanity checking, similar to virDomainSnapshotRedefinePrep */ + offline = def->state == VIR_DOMAIN_DISK_SNAPSHOT || + virDomainSnapshotDefIsExternal(def); + if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) && !offline) { + virReportError(VIR_ERR_INVALID_ARG, + _("disk-only flag for snapshot %s requires " + "disk-snapshot state"), + def->name); + goto cleanup; + } + if (def->dom) { + if (memcmp(def->dom->uuid, domain_uuid, VIR_UUID_BUFLEN)) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + + virUUIDFormat(domain_uuid, uuidstr); + virReportError(VIR_ERR_INVALID_ARG, + _("definition for snapshot %s must use uuid %s"), + def->name, uuidstr); + goto cleanup; + } + if (offline || + def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { + align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; + align_match = false; + } + if (virDomainSnapshotAlignDisks(def, align_location, + align_match) < 0) + goto cleanup; + } + } + + if (virDomainSnapshotUpdateRelations(snapshots) < 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("<snapshots> contains inconsistent parent-child " + "relationships")); + goto cleanup; + } + + if (current) { + if (!(*current_snap = virDomainSnapshotFindByName(snapshots, + current))) { + virReportError(VIR_ERR_NO_DOMAIN_SNAPSHOT, + _("no snapshot matching current='%s'"), current); + goto cleanup; + } + (*current_snap)->def->current = true; + } + + ret = 0; + cleanup: + if (ret < 0) { + /* There were no snapshots before this call; so on error, just + * blindly delete anything created before the failure. */ + virHashRemoveAll(snapshots->objs); + snapshots->metaroot.nchildren = 0; + snapshots->metaroot.first_child = NULL; + } + VIR_FREE(current); + xmlXPathFreeContext(ctxt); + xmlFreeDoc(xml); + xmlKeepBlanksDefault(keepBlanksDefault); + return ret; +} + /** * virDomainSnapshotDefAssignExternalNames: * @def: snapshot def object diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c623737c30..96f54a97fe 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -881,6 +881,7 @@ virDomainSnapshotAssignDef; virDomainSnapshotDefFormat; virDomainSnapshotDefFree; virDomainSnapshotDefIsExternal; +virDomainSnapshotDefParseList; virDomainSnapshotDefParseString; virDomainSnapshotDropParent; virDomainSnapshotFindByName; -- 2.20.1
On 2/23/19 4:24 PM, Eric Blake wrote:
Add a new function to make it possible to parse a list of snapshots at once. This is a counterpart to the previous patch making it possible to produce all snapshots in a single XML string, and intentionally parses the same top-level element <snapshots> with an optional attribute current='name'.
Note that existing REDEFINE code uses virDomainSnapshotRedefinePrep() for some sanity checking - much of that checking involves parent/child relationships (which make sense when doing one element at a time and worrying about replacement), but where this patch gets away with the much simpler final call to virDomainSnapshotUpdateRelations() (for all relationship checking after the end, since we know we started with no relations at all, and since checking parent relationships per-snapshot is not viable as we don't control which order the snapshots appear in). All other domain-agnostic sanity checks used during a redefinition are copied here.
Some day maybe there's a helper that can handle both uses...
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/snapshot_conf.h | 7 ++ src/conf/snapshot_conf.c | 135 +++++++++++++++++++++++++++++++++++++++ src/libvirt_private.syms | 1 + 3 files changed, 143 insertions(+)
diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h index 19ab75f895..6a92241fe6 100644 --- a/src/conf/snapshot_conf.h +++ b/src/conf/snapshot_conf.h @@ -117,6 +117,13 @@ virDomainSnapshotDefPtr virDomainSnapshotDefParseNode(xmlDocPtr xml, virCapsPtr caps, virDomainXMLOptionPtr xmlopt, unsigned int flags); +int virDomainSnapshotDefParseList(const char *xmlSstr, + const unsigned char *domain_uuid, + virDomainSnapshotObjListPtr snapshots, + virDomainSnapshotObjPtr *current_snap, + virCapsPtr caps, + virDomainXMLOptionPtr xmlopt, + unsigned int flags);
virDomainSnapshotObjListParse (consistency w/ virDomainSnapshotObjListFormat)
void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def); char *virDomainSnapshotDefFormat(const char *uuidstr, virDomainSnapshotDefPtr def, diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 963dc10247..61e26726e9 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -426,6 +426,141 @@ virDomainSnapshotDefParseString(const char *xmlStr, }
+int virDomainSnapshotDefParseList(const char *xmlStr, + const unsigned char *domain_uuid, + virDomainSnapshotObjListPtr snapshots, + virDomainSnapshotObjPtr *current_snap, + virCapsPtr caps, + virDomainXMLOptionPtr xmlopt, + unsigned int flags)
int virDomainSnapshotObjListParse(...)
+{ + int ret = -1; + xmlDocPtr xml; + xmlNodePtr root; + xmlXPathContextPtr ctxt = NULL; + char *current = NULL; + int n; + size_t i; + VIR_AUTOFREE(xmlNodePtr *) nodes = NULL; + int keepBlanksDefault = xmlKeepBlanksDefault(0); 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. [...]
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c623737c30..96f54a97fe 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -881,6 +881,7 @@ virDomainSnapshotAssignDef; virDomainSnapshotDefFormat; virDomainSnapshotDefFree; virDomainSnapshotDefIsExternal; +virDomainSnapshotDefParseList;
virDomainSnapshotObjListParse;
virDomainSnapshotDefParseString; virDomainSnapshotDropParent; virDomainSnapshotFindByName;
Simple enough adjustments... Reviewed-by: John Ferlan <jferlan@redhat.com> John
Right now, copying the state of a transient domain with snapshots from one host to another requires multiple API calls on both machines - on the host: get the domain XML, get a list of the snapshots, and then for each snapshot get the snapshot's XML; then on the destination: create the domain, then multiple domain snapshot create calls with the REDEFINE flag. This patch aims to make the process use fewer APIs by making it possible to grab the XML for all snapshots at the same time as grabbing the domain XML. Note that we had to do the modification to virDomainGetXMLDesc(), rather than virDomainSnapshotGetXMLDesc(), since the latter requires a single non-NULL snapshot object, whereas we want the list of all snapshots for the domain (even if the list has 0 elements). Once wired up in drivers in later patches, the new information is provided as: <domain ...> ... </devices> <snapshots current='name'> <domainsnapshot> ... </domainsnapshot> <domainsnapshot> ... </domainsnapshot> </snapshots> </domain> For now, I did not modify the schema to permit this information during virDomainDefineXML; it is still necessary to use virDomainSnapshotCreateXML with VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE multiple times to recreate the added state output here. Unfortunately, libvirt versions between 1.2.12 and 5.0.0 will silently ignore the new flag, rather than diagnosing that they don't support it; but at least silent lack of snapshots from an older server is not a security hole. Signed-off-by: Eric Blake <eblake@redhat.com> --- include/libvirt/libvirt-domain.h | 1 + src/conf/domain_conf.c | 13 ++++++++----- src/libvirt-domain.c | 5 +++++ 3 files changed, 14 insertions(+), 5 deletions(-) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 1d5bdb545d..a8ebb68388 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -1570,6 +1570,7 @@ typedef enum { VIR_DOMAIN_XML_INACTIVE = (1 << 1), /* dump inactive domain information */ VIR_DOMAIN_XML_UPDATE_CPU = (1 << 2), /* update guest CPU requirements according to host CPU */ VIR_DOMAIN_XML_MIGRATABLE = (1 << 3), /* dump XML suitable for migration */ + VIR_DOMAIN_XML_SNAPSHOTS = (1 << 4), /* include all snapshots in the dump */ } virDomainXMLFlags; typedef enum { diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 46ae79e738..90e2dbe465 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -29057,11 +29057,12 @@ virDomainDefFormatInternal(virDomainDefPtr def, return -1; } -/* Converts VIR_DOMAIN_XML_COMMON_FLAGS into VIR_DOMAIN_DEF_FORMAT_* - * flags, and silently ignores any other flags. Note that the caller - * should validate the set of flags it is willing to accept; see also - * the comment on VIR_DOMAIN_XML_COMMON_FLAGS about security - * considerations with adding new flags. */ +/* Converts VIR_DOMAIN_XML_COMMON_FLAGS and VIR_DOMAIN_XML_SNAPSHOTS + * into VIR_DOMAIN_DEF_FORMAT_* flags, and silently ignores any other + * flags. Note that the caller should validate the set of flags it is + * willing to accept; see also the comment on + * VIR_DOMAIN_XML_COMMON_FLAGS about security considerations with + * adding new flags. */ unsigned int virDomainDefFormatConvertXMLFlags(unsigned int flags) { unsigned int formatFlags = 0; @@ -29072,6 +29073,8 @@ unsigned int virDomainDefFormatConvertXMLFlags(unsigned int flags) formatFlags |= VIR_DOMAIN_DEF_FORMAT_INACTIVE; if (flags & VIR_DOMAIN_XML_MIGRATABLE) formatFlags |= VIR_DOMAIN_DEF_FORMAT_MIGRATABLE; + if (flags & VIR_DOMAIN_XML_SNAPSHOTS) + formatFlags |= VIR_DOMAIN_DEF_FORMAT_SNAPSHOTS; return formatFlags; } diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 072b92b717..2691698bd5 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -2570,6 +2570,11 @@ virDomainGetControlInfo(virDomainPtr domain, * XML might not validate against the schema, so it is mainly for * internal use. * + * If @flags contains VIR_DOMAIN_XML_SNAPSHOTS, the XML will include + * an additional <snapshots> child element describing all snapshots + * belonging to the domain, including an attribute current='name' if + * one of those snapshots is current. + * * 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
On 2/23/19 4:24 PM, Eric Blake wrote:
Right now, copying the state of a transient domain with snapshots from one host to another requires multiple API calls on both machines - on the host: get the domain XML, get a list of the snapshots, and then for each snapshot get the snapshot's XML; then on the destination: create the domain, then multiple domain snapshot create calls with the REDEFINE flag. This patch aims to make the process use fewer APIs by making it possible to grab the XML for all snapshots at the same time as grabbing the domain XML. Note that we had to do the modification to virDomainGetXMLDesc(), rather than virDomainSnapshotGetXMLDesc(), since the latter requires a single non-NULL snapshot object, whereas we want the list of all snapshots for the domain (even if the list has 0 elements).
Once wired up in drivers in later patches, the new information is provided as:
<domain ...> ... </devices> <snapshots current='name'> <domainsnapshot> ... </domainsnapshot> <domainsnapshot> ... </domainsnapshot> </snapshots> </domain>
For now, I did not modify the schema to permit this information during virDomainDefineXML; it is still necessary to use virDomainSnapshotCreateXML with VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE multiple times to recreate the added state output here.
Unfortunately, libvirt versions between 1.2.12 and 5.0.0 will silently ignore the new flag, rather than diagnosing that they don't support it; but at least silent lack of snapshots from an older server is not a security hole.
Signed-off-by: Eric Blake <eblake@redhat.com> --- include/libvirt/libvirt-domain.h | 1 + src/conf/domain_conf.c | 13 ++++++++----- src/libvirt-domain.c | 5 +++++ 3 files changed, 14 insertions(+), 5 deletions(-)
[...]
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 072b92b717..2691698bd5 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -2570,6 +2570,11 @@ virDomainGetControlInfo(virDomainPtr domain, * XML might not validate against the schema, so it is mainly for * internal use. * + * If @flags contains VIR_DOMAIN_XML_SNAPSHOTS, the XML will include
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.
+ * an additional <snapshots> child element describing all snapshots + * belonging to the domain, including an attribute current='name' if + * one of those snapshots is current. + * * Returns a 0 terminated UTF-8 encoded XML instance, or NULL in case of error. * the caller must free() the returned value. */
Reviewed-by: John Ferlan <jferlan@redhat.com> John
On 2/27/19 10:32 AM, John Ferlan wrote:
Unfortunately, libvirt versions between 1.2.12 and 5.0.0 will silently ignore the new flag, rather than diagnosing that they don't support it; but at least silent lack of snapshots from an older server is not a security hole.
Signed-off-by: Eric Blake <eblake@redhat.com> --- include/libvirt/libvirt-domain.h | 1 + src/conf/domain_conf.c | 13 ++++++++----- src/libvirt-domain.c | 5 +++++ 3 files changed, 14 insertions(+), 5 deletions(-)
[...]
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 072b92b717..2691698bd5 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -2570,6 +2570,11 @@ virDomainGetControlInfo(virDomainPtr domain, * XML might not validate against the schema, so it is mainly for * internal use. * + * If @flags contains VIR_DOMAIN_XML_SNAPSHOTS, the XML will include
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.
Maybe, since this is indeed enough of a break from the usual norms of rejecting unknown flags (at least for a couple of years) to be worth it. The upcoming VIR_DOMAIN_XML_CHECKPOINTS will have the same wording, of course. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
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@redhat.com> --- 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
On 2/23/19 4:24 PM, Eric Blake wrote:
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@redhat.com> --- include/libvirt/libvirt-domain-snapshot.h | 3 +++ src/libvirt-domain-snapshot.c | 21 ++++++++++++++++++--- 2 files changed, 21 insertions(+), 3 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
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@redhat.com> --- 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:
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@redhat.com> --- tools/virsh-domain.c | 7 +++++++ tools/virsh-snapshot.c | 14 ++++++++++++++ 2 files changed, 21 insertions(+)
Need to update virsh.pod as well to describe new arguments. Do you think that a separation of the dumpxml and the parse/create into order pertinent patches would be better? That is the --snapshots for dumpxml after patch7 (which probably could swap places w/ patch6) so that the dumpxml and parse/create functionality is all in logical order. It's not that important. John [...]
On 2/27/19 10:54 AM, John Ferlan wrote:
On 2/23/19 4:24 PM, Eric Blake wrote:
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@redhat.com> --- tools/virsh-domain.c | 7 +++++++ tools/virsh-snapshot.c | 14 ++++++++++++++ 2 files changed, 21 insertions(+)
Need to update virsh.pod as well to describe new arguments. Do you think that a separation of the dumpxml and the parse/create into order pertinent patches would be better? That is the --snapshots for dumpxml after patch7 (which probably could swap places w/ patch6) so that the dumpxml and parse/create functionality is all in logical order. It's not that important.
Yeah, I waffled on that. v1 was definitely more of a "do all dumpxml changes, then do all redefine changes" breakup. But I intentionally refactored this way (do all API changes, then all virsh changes), especially since the virsh changes are so small that it's easy to review both at once. Depending on how you feel about the qemu changes, I'm still open to the idea of doing dumpxml separate from redefine in each of the drivers, if it makes it easier to backport one but not the other. Furthermore, your comments against v4 of the incremental backup sparked my idea of possibly using VIR_DOMAIN_XML_SNAPSHOTS internally (so that domains have just ONE xml file on disk, rather than the main domain xml + one xml per snapshot), and that may have fallout on making the dumpxml portion reasonable to backport in isolation. I'll have to see how things play out in the rest of the series before deciding if splitting this one is worth it. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Implement the new flags for bulk snapshot dump and redefine. The bulk of the work is already done by the common code. Since each connection to test:///default restarts at the same canned state, this can easily be tested with: $ virsh -c test:///default " snapshot-create-as test s1 snapshot-create-as test s2 dumpxml test --snapshots" | sed -n '/<snapshots/,/<.snapshots/p' > list.xml $ virsh -c test:///default " snapshot-list test snapshot-create test --redefine-list list.xml snapshot-current --name test snapshot-list --parent test " Name Creation Time State ------------------------------- Domain snapshot list imported successfully s2 Name Creation Time State Parent ------------------------------------------------------ s1 2019-02-20 22:26:52 -0600 running s2 2019-02-20 22:26:52 -0600 running s1 The test driver also makes it easy to test input validation, by modifying list.xml incorrectly (such as trying to attempt circular dependencies). Signed-off-by: Eric Blake <eblake@redhat.com> --- src/test/test_driver.c | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index a6a67d42e2..fd0fd6a67a 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -2628,7 +2628,7 @@ static char *testDomainGetXMLDesc(virDomainPtr domain, unsigned int flags) virDomainObjPtr privdom; char *ret = NULL; - virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS, NULL); + virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS | VIR_DOMAIN_XML_SNAPSHOTS, NULL); if (!(privdom = testDomObjFromDomain(domain))) return NULL; @@ -2636,8 +2636,9 @@ static char *testDomainGetXMLDesc(virDomainPtr domain, unsigned int flags) def = (flags & VIR_DOMAIN_XML_INACTIVE) && privdom->newDef ? privdom->newDef : privdom->def; - ret = virDomainDefFormat(def, privconn->caps, - virDomainDefFormatConvertXMLFlags(flags)); + ret = virDomainDefFormatFull(def, privconn->caps, + privdom->snapshots, privdom->current_snapshot, + virDomainDefFormatConvertXMLFlags(flags)); virDomainObjEndAPI(&privdom); return ret; @@ -6314,6 +6315,7 @@ testDomainSnapshotCreateXML(virDomainPtr domain, * QUIESCE: Nothing to do * ATOMIC: Nothing to do * LIVE: Nothing to do + * REDEFINE_LIST: Implemented */ virCheckFlags( VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE | @@ -6321,7 +6323,8 @@ testDomainSnapshotCreateXML(virDomainPtr domain, VIR_DOMAIN_SNAPSHOT_CREATE_HALT | 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); if ((redefine && !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT))) update_current = false; @@ -6337,6 +6340,18 @@ testDomainSnapshotCreateXML(virDomainPtr domain, goto cleanup; } + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST) { + if (virDomainSnapshotDefParseList(xmlDesc, vm->def->uuid, vm->snapshots, + &vm->current_snapshot, privconn->caps, + privconn->xmlopt, parse_flags) < 0) + 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 (!(def = virDomainSnapshotDefParseString(xmlDesc, privconn->caps, privconn->xmlopt, @@ -6384,7 +6399,7 @@ testDomainSnapshotCreateXML(virDomainPtr domain, snapshot = virGetDomainSnapshot(domain, snap->def->name); cleanup: if (vm) { - if (snapshot) { + if (snapshot && !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST)) { virDomainSnapshotObjPtr other; if (update_current) vm->current_snapshot = snap; -- 2.20.1
On 2/23/19 4:24 PM, Eric Blake wrote:
Implement the new flags for bulk snapshot dump and redefine. The bulk of the work is already done by the common code.
Since each connection to test:///default restarts at the same canned state, this can easily be tested with:
$ virsh -c test:///default " snapshot-create-as test s1 snapshot-create-as test s2 dumpxml test --snapshots" | sed -n '/<snapshots/,/<.snapshots/p' > list.xml $ virsh -c test:///default " snapshot-list test snapshot-create test --redefine-list list.xml snapshot-current --name test snapshot-list --parent test " Name Creation Time State -------------------------------
Domain snapshot list imported successfully s2 Name Creation Time State Parent ------------------------------------------------------ s1 2019-02-20 22:26:52 -0600 running s2 2019-02-20 22:26:52 -0600 running s1
The test driver also makes it easy to test input validation, by modifying list.xml incorrectly (such as trying to attempt circular dependencies).
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/test/test_driver.c | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index a6a67d42e2..fd0fd6a67a 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -2628,7 +2628,7 @@ static char *testDomainGetXMLDesc(virDomainPtr domain, unsigned int flags) virDomainObjPtr privdom; char *ret = NULL;
- virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS, NULL); + virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS | VIR_DOMAIN_XML_SNAPSHOTS, NULL);
if (!(privdom = testDomObjFromDomain(domain))) return NULL; @@ -2636,8 +2636,9 @@ static char *testDomainGetXMLDesc(virDomainPtr domain, unsigned int flags) def = (flags & VIR_DOMAIN_XML_INACTIVE) && privdom->newDef ? privdom->newDef : privdom->def;
- ret = virDomainDefFormat(def, privconn->caps, - virDomainDefFormatConvertXMLFlags(flags)); + ret = virDomainDefFormatFull(def, privconn->caps, + privdom->snapshots, privdom->current_snapshot, + virDomainDefFormatConvertXMLFlags(flags));
virDomainObjEndAPI(&privdom); return ret; @@ -6314,6 +6315,7 @@ testDomainSnapshotCreateXML(virDomainPtr domain, * QUIESCE: Nothing to do * ATOMIC: Nothing to do * LIVE: Nothing to do + * REDEFINE_LIST: Implemented */ virCheckFlags( VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE | @@ -6321,7 +6323,8 @@ testDomainSnapshotCreateXML(virDomainPtr domain, VIR_DOMAIN_SNAPSHOT_CREATE_HALT | 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);
if ((redefine && !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT))) update_current = false; @@ -6337,6 +6340,18 @@ testDomainSnapshotCreateXML(virDomainPtr domain, goto cleanup; }
+ if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST) { + if (virDomainSnapshotDefParseList(xmlDesc, vm->def->uuid, vm->snapshots, + &vm->current_snapshot, privconn->caps, + privconn->xmlopt, parse_flags) < 0) + goto cleanup; + + /* Return is arbitrary, so use the first root */ + snap = virDomainSnapshotFindByName(vm->snapshots, NULL);
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@redhat.com> John
+ snapshot = virGetDomainSnapshot(domain, snap->first_child->def->name); + goto cleanup; + } + if (!(def = virDomainSnapshotDefParseString(xmlDesc, privconn->caps, privconn->xmlopt, @@ -6384,7 +6399,7 @@ testDomainSnapshotCreateXML(virDomainPtr domain, snapshot = virGetDomainSnapshot(domain, snap->def->name); cleanup: if (vm) { - if (snapshot) { + if (snapshot && !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST)) { virDomainSnapshotObjPtr other; if (update_current) vm->current_snapshot = snap;
On 2/27/19 11:03 AM, John Ferlan wrote:
On 2/23/19 4:24 PM, Eric Blake wrote:
Implement the new flags for bulk snapshot dump and redefine. The bulk of the work is already done by the common code.
Since each connection to test:///default restarts at the same canned state, this can easily be tested with:
@@ -6337,6 +6340,18 @@ testDomainSnapshotCreateXML(virDomainPtr domain, goto cleanup; }
+ if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST) { + if (virDomainSnapshotDefParseList(xmlDesc, vm->def->uuid, vm->snapshots, + &vm->current_snapshot, privconn->caps, + privconn->xmlopt, parse_flags) < 0) + goto cleanup; + + /* Return is arbitrary, so use the first root */ + snap = virDomainSnapshotFindByName(vm->snapshots, NULL);
Hmm... I don't think @snap is used since cleanup: code filters on REDEFINE_LIST, so it would seem this would be leaked then.
snap is a stolen pointer into something residing in vm->snapshots (that is, the result of virDomainSnapshotFindByName() never leaks, and does not need explicit cleanup).
Reviewed-by: John Ferlan <jferlan@redhat.com>
John
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
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@redhat.com> --- 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/23/19 4:24 PM, Eric Blake wrote:
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@redhat.com> --- 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(-)
NB: I couldn't get this one to git am -3 apply - I didn't chase the conflict though.
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);
Theoretically separable.
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"));
Error msg is a bit odd - the error is that a snapshot listing was desired, but consumer didn't pass the @vm object.
+ goto cleanup; + } + ret = virDomainDefFormatInternal(def, caps, + snapshots ? vm->snapshots : NULL, + snapshots ? vm->current_snapshot : NULL, virDomainDefFormatConvertXMLFlags(flags), buf, driver->xmlopt);
Perhaps this one should [be | have been] turned into a virDomainDefFormatFull (or whatever new name is chosen if one is chosen). I think it shows the hazards of exposing *Internal to many consumers.
@@ -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)
Existing; however, considering earlier comment about snapshot list being filled into the buffer until error, but the *ListFormat not clearing the buffer on error, I think another patch should be added here to do the virBufferFreeAndReset(&buf); before return NULL.
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,
one arg each line
+ 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);
Similar to test driver - this isn't used during cleanup. John
+ 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"));
On 2/27/19 11:27 AM, John Ferlan wrote:
On 2/23/19 4:24 PM, Eric Blake wrote:
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@redhat.com> --- 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(-)
NB: I couldn't get this one to git am -3 apply - I didn't chase the conflict though.
My fault for too many patches in flight at once. As I'll be doing a v3 anyways, that one wil be cleaner.
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);
Theoretically separable.
Yeah, I debated about it. Since you called me on it, I'll make the split.
@@ -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"));
Error msg is a bit odd - the error is that a snapshot listing was desired, but consumer didn't pass the @vm object.
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.
+ goto cleanup; + } + ret = virDomainDefFormatInternal(def, caps, + snapshots ? vm->snapshots : NULL, + snapshots ? vm->current_snapshot : NULL, virDomainDefFormatConvertXMLFlags(flags), buf, driver->xmlopt);
Perhaps this one should [be | have been] turned into a virDomainDefFormatFull (or whatever new name is chosen if one is chosen). I think it shows the hazards of exposing *Internal to many consumers.
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.
+ /* Return is arbitrary, so use the first root */ + snap = virDomainSnapshotFindByName(vm->snapshots, NULL);
Similar to test driver - this isn't used during cleanup.
John
+ snapshot = virGetDomainSnapshot(domain, snap->first_child->def->name);
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
participants (2)
-
Eric Blake -
John Ferlan