[libvirt] snapshots += domain description?
Hello, I just encountered a problem with the current snapshot implementation for qemu/kvm: To revert back to a snapshot, the domain description must closely match the domain description when the snapshot was created. If the ram-size doesn't match or the VM now contains fewer CPUs or contains additional disks, kvm will either not load the snapshot or the machine will be broken afterwarts. Is this a known problem? Without looking into the implementation details I think saving the full domain description instead of just a reference to the domain uuid as part of the snapshot description would work better. Or am I supposed to save the domain description myself in addition the the data saved in /var/lib/libvirt/qemu/snapshot/$DOMAIN_NAME/$SNAPSHOT_NAME.xml. I think this is very important feature, because when you - for example - notice that you need additional disk space and would like to include an additional block device, this is your ideal time to take a snapshot you can revert to when you do something wrong. Looking at VMware I see that reverting a VM there will not only revert the disks and ram back to the saved state, but also the description including name, ram size, etc. Any comment or advise is appreciated. Sincerely Philipp Hahn -- Philipp Hahn Open Source Software Engineer hahn@univention.de Univention GmbH Linux for Your Business fon: +49 421 22 232- 0 Mary-Somerville-Str.1 28359 Bremen fax: +49 421 22 232-99 http://www.univention.de
Hello, to refresh this old question again: Snapshots are a great feature, but I encountered a problem with Qemu/KVM: The snapshot information does not include the domain description, which breaks the following work-flow: 1. create and start domain with RAM=x1, CPU=y1, ... 2. take snapshot 3. shutdown domain 4. edit domain to RAM=x2, CPU=y2, ... 5. start domain and restore previous snapshot Qemu will start the domain, but will fail to load the saved RAM state:
qemu: warning: error while loading state for instance 0x0 of device 'ram'
For comparison, VMWare also includes the configuration in the snapshot data (see the *.vmsn file), so on restore the right configuration and state of mass storage and RAM are restored. I think the domain XML should be included with the snapshot data, so restoring the exact machine is possible. On restore there would be two cases, when restoring a live vm: 1. when the already running qemu/kvm-process is compatible with the VM to be restored, reuse the running process and issue the loadvm (or what ever). 2. if they are incompatible, kill the running kvm process and start a new one with the right setup to load the VM state. Is someone working on fixing the problem or are there any plans to fix that? Sincerely Philipp -- Philipp Hahn Open Source Software Engineer hahn@univention.de Univention GmbH Linux for Your Business fon: +49 421 22 232- 0 Mary-Somerville-Str.1 28359 Bremen fax: +49 421 22 232-99 http://www.univention.de/ ** Besuchen Sie uns auf der CeBIT in Hannover ** ** Auf dem Univention Stand D36 in Halle 2 ** ** Vom 01. bis 05. März 2011 **
On 12/07/2010 09:00 AM, Philipp Hahn wrote:
Hello,
I just encountered a problem with the current snapshot implementation for qemu/kvm: To revert back to a snapshot, the domain description must closely match the domain description when the snapshot was created. If the ram-size doesn't match or the VM now contains fewer CPUs or contains additional disks, kvm will either not load the snapshot or the machine will be broken afterwarts. Is this a known problem?
Are you referring to a disk snapshot, created via 'virsh snapshot' on a qcow2 image, or a memory snapshot, created via 'virsh save'? The latter _does_ save the guest XML as part of the snapshot, and 'virsh restore' uses that saved XML rather than the current domain definition to resume the guest.
Without looking into the implementation details I think saving the full domain description instead of just a reference to the domain uuid as part of the snapshot description would work better.
So it sounds like you are talking about the 'virsh snapshot' family of commands. Indeed, there's probably quite a bit of work to do in that code to make it more reliable.
Or am I supposed to save the domain description myself in addition the the data saved in /var/lib/libvirt/qemu/snapshot/$DOMAIN_NAME/$SNAPSHOT_NAME.xml. I think this is very important feature, because when you - for example - notice that you need additional disk space and would like to include an additional block device, this is your ideal time to take a snapshot you can revert to when you do something wrong.
I think we are hitting on more fundamental issues. Right now, the 'virsh snapshot' family of commands is geared towards qcow2 images, and relies heavily on the qcow2 internal snapshotting feature. It would be nice to enhance that to support other means of snapshotting (such as raw disk images stored on an lvm partition or using btrfs file cloning), as well as making live snapshots possible (by capturing memory as well as disk images, or even coordinating with the guest to quiesce its file systems so that restoring from a snapshot is less likely to restore a file system in an inconsistent state).
Looking at VMware I see that reverting a VM there will not only revert the disks and ram back to the saved state, but also the description including name, ram size, etc.
Any comment or advise is appreciated.
Likewise - I'm hoping to look into this more myself, and any thoughts or requirements that people can think of that should drive a decent snapshot design should be brought up sooner rather than later. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Hello, this is just a proof-of-concept implementation for storing the domain xml description with the xml snapshot data. This is needed for Qemu, since restoring a Qemu domain after changing the "virtual hardware" (like changing RAM size, number of CPUs, controllers) is not possible: The "loadvm" command will fail to restore the saved state. My idea was to save the domain XML data with the snapshot XML data, which currently just contains a reference to the domain UUID. On snapshot-restore that domain description is restored to overwrites the current domain description. I descided to store a reference to the parsed domainObj with the domainSnapshotObj, since my goal was to detect on revert, if resuing the currently running Qemu process is possible, or the the process needs to be restartet with different arguments to revert changes to the "virtual hardware". I hadn't had time to implement this yet, so currently the Qemu process is restarted on every revert, which disconnects any VNC session and is also a lot slower then simply calling "loadvm". For this implementation I needed to pass "virCapsPtr" down to virDomainSnapshotDefParseString(). I only implemented that correctly for Qemu, so vbox and esx might be broken currently. An alternative (and simpler) implementation would be to just store the unparsed XML description with the runtime domainSnapshotObj. This would simplify copying the domain description, but would complicate detecting if "loadvm" is possible or Qemu must be restarted. The implementation currently works very well for me, but I noticed some memory leak, which might be related to this patch (or some other, since my version of libvirt contains several other patches as well). Philipp Hahn (3): Swap virDomain / virFomainSnapshot declaration snapshot: save domain description with snapshot Snapshot: restore domain description from snapshot src/conf/domain_conf.c | 27 +++++++++++-- src/conf/domain_conf.h | 102 ++++++++++++++++++++++++----------------------- src/esx/esx_driver.c | 2 +- src/qemu/qemu_driver.c | 43 +++++++++++++++++++- src/vbox/vbox_tmpl.c | 2 +- 5 files changed, 117 insertions(+), 59 deletions(-)
In preparation for storing the domain description with the snapshot, swap the order of declaration. Signed-off-by: Philipp Hahn <hahn@univention.de> --- src/conf/domain_conf.h | 100 ++++++++++++++++++++++++------------------------ 1 files changed, 50 insertions(+), 50 deletions(-) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 30aeccc..9e7e1ee 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -982,56 +982,6 @@ struct _virDomainClockDef { # define VIR_DOMAIN_CPUMASK_LEN 1024 -/* Snapshot state */ -typedef struct _virDomainSnapshotDef virDomainSnapshotDef; -typedef virDomainSnapshotDef *virDomainSnapshotDefPtr; -struct _virDomainSnapshotDef { - char *name; - char *description; - char *parent; - time_t creationTime; - int state; - - long active; -}; - -typedef struct _virDomainSnapshotObj virDomainSnapshotObj; -typedef virDomainSnapshotObj *virDomainSnapshotObjPtr; -struct _virDomainSnapshotObj { - int refs; - - virDomainSnapshotDefPtr def; -}; - -typedef struct _virDomainSnapshotObjList virDomainSnapshotObjList; -typedef virDomainSnapshotObjList *virDomainSnapshotObjListPtr; -struct _virDomainSnapshotObjList { - /* name string -> virDomainSnapshotObj mapping - * for O(1), lockless lookup-by-name */ - virHashTable *objs; -}; - -virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, - int newSnapshot); -void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def); -char *virDomainSnapshotDefFormat(char *domain_uuid, - virDomainSnapshotDefPtr def, - int internal); -virDomainSnapshotObjPtr virDomainSnapshotAssignDef(virDomainSnapshotObjListPtr snapshots, - const virDomainSnapshotDefPtr def); - -int virDomainSnapshotObjListInit(virDomainSnapshotObjListPtr objs); -int virDomainSnapshotObjListGetNames(virDomainSnapshotObjListPtr snapshots, - char **const names, int maxnames); -int virDomainSnapshotObjListNum(virDomainSnapshotObjListPtr snapshots); -virDomainSnapshotObjPtr virDomainSnapshotFindByName(const virDomainSnapshotObjListPtr snapshots, - const char *name); -void virDomainSnapshotObjListRemove(virDomainSnapshotObjListPtr snapshots, - virDomainSnapshotObjPtr snapshot); -int virDomainSnapshotObjUnref(virDomainSnapshotObjPtr snapshot); -int virDomainSnapshotHasChildren(virDomainSnapshotObjPtr snap, - virDomainSnapshotObjListPtr snapshots); - /* Guest VM main configuration */ typedef struct _virDomainDef virDomainDef; typedef virDomainDef *virDomainDefPtr; @@ -1122,6 +1072,56 @@ struct _virDomainDef { virDomainXMLNamespace ns; }; +/* Snapshot state */ +typedef struct _virDomainSnapshotDef virDomainSnapshotDef; +typedef virDomainSnapshotDef *virDomainSnapshotDefPtr; +struct _virDomainSnapshotDef { + char *name; + char *description; + char *parent; + time_t creationTime; + int state; + + long active; +}; + +typedef struct _virDomainSnapshotObj virDomainSnapshotObj; +typedef virDomainSnapshotObj *virDomainSnapshotObjPtr; +struct _virDomainSnapshotObj { + int refs; + + virDomainSnapshotDefPtr def; +}; + +typedef struct _virDomainSnapshotObjList virDomainSnapshotObjList; +typedef virDomainSnapshotObjList *virDomainSnapshotObjListPtr; +struct _virDomainSnapshotObjList { + /* name string -> virDomainSnapshotObj mapping + * for O(1), lockless lookup-by-name */ + virHashTable *objs; +}; + +virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, + int newSnapshot); +void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def); +char *virDomainSnapshotDefFormat(char *domain_uuid, + virDomainSnapshotDefPtr def, + int internal); +virDomainSnapshotObjPtr virDomainSnapshotAssignDef(virDomainSnapshotObjListPtr snapshots, + const virDomainSnapshotDefPtr def); + +int virDomainSnapshotObjListInit(virDomainSnapshotObjListPtr objs); +int virDomainSnapshotObjListGetNames(virDomainSnapshotObjListPtr snapshots, + char **const names, int maxnames); +int virDomainSnapshotObjListNum(virDomainSnapshotObjListPtr snapshots); +virDomainSnapshotObjPtr virDomainSnapshotFindByName(const virDomainSnapshotObjListPtr snapshots, + const char *name); +void virDomainSnapshotObjListRemove(virDomainSnapshotObjListPtr snapshots, + virDomainSnapshotObjPtr snapshot); +int virDomainSnapshotObjUnref(virDomainSnapshotObjPtr snapshot); +int virDomainSnapshotHasChildren(virDomainSnapshotObjPtr snap, + virDomainSnapshotObjListPtr snapshots); + /* Guest VM runtime state */ typedef struct _virDomainObj virDomainObj; typedef virDomainObj *virDomainObjPtr; -- 1.7.1
On 04/12/2011 12:06 AM, Philipp Hahn wrote: 0.33 years(!) later:
In preparation for storing the domain description with the snapshot, swap the order of declaration.
Signed-off-by: Philipp Hahn<hahn@univention.de> --- src/conf/domain_conf.h | 100 ++++++++++++++++++++++++------------------------ 1 files changed, 50 insertions(+), 50 deletions(-)
Shouldn't strictly be necessary - can't a forward declaration do the job? But I do like the concept - a snapshot contains a domain, so domain is the smaller struct and should be listed first. It wasn't too hard to rebase this one, so ACK and pushed. I've already found a couple of mails from you in the backlog of my inbox today with valid bug reports or patches. To all list readers: Feel free to ping the list on issues after a week or so if you think people are dragging their feet on giving a reply (even if the reply is a nack or a wait, that's better than silence). -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Save the domain description with the XML snapshot data. TODOs: - XML file is no longer nicely indented - Fix esx driver - Fix vbox driver Signed-off-by: Philipp Hahn <hahn@univention.de> --- src/conf/domain_conf.c | 27 +++++++++++++++++++++++---- src/conf/domain_conf.h | 4 +++- src/esx/esx_driver.c | 2 +- src/qemu/qemu_driver.c | 4 ++-- src/vbox/vbox_tmpl.c | 2 +- 5 files changed, 30 insertions(+), 9 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 16e1291..d568c43 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8564,6 +8564,7 @@ void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def) if (!def) return; + virDomainDefFree(def->dom); VIR_FREE(def->name); VIR_FREE(def->description); VIR_FREE(def->parent); @@ -8571,7 +8572,8 @@ void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def) } virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, - int newSnapshot) + int newSnapshot, + virCapsPtr caps) { xmlXPathContextPtr ctxt = NULL; xmlDocPtr xml = NULL; @@ -8661,6 +8663,14 @@ virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, else def->creationTime = tv.tv_sec; + xmlNodePtr domainNode = virXPathNode("./domain", ctxt); + if (domainNode == NULL) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing domain from existing snapshot")); + def->dom = NULL; + } else + def->dom = virDomainDefParseNode(caps, xml, domainNode, VIR_DOMAIN_XML_INACTIVE); + ret = def; cleanup: @@ -8678,6 +8688,7 @@ char *virDomainSnapshotDefFormat(char *domain_uuid, virDomainSnapshotDefPtr def, int internal) { + char *xml = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; virBufferAddLit(&buf, "<domainsnapshot>\n"); @@ -8694,9 +8705,17 @@ char *virDomainSnapshotDefFormat(char *domain_uuid, } virBufferVSprintf(&buf, " <creationTime>%ld</creationTime>\n", def->creationTime); - virBufferAddLit(&buf, " <domain>\n"); - virBufferVSprintf(&buf, " <uuid>%s</uuid>\n", domain_uuid); - virBufferAddLit(&buf, " </domain>\n"); + if (def->dom != NULL) { + xml = virDomainDefFormat(def->dom, VIR_DOMAIN_XML_INACTIVE | VIR_DOMAIN_XML_SECURE); + } + if (xml != NULL) { + virBufferVSprintf(&buf, " %s", xml); + VIR_FREE(xml); + } else { + virBufferAddLit(&buf, " <domain>\n"); + virBufferVSprintf(&buf, " <uuid>%s</uuid>\n", domain_uuid); + virBufferAddLit(&buf, " </domain>\n"); + } if (internal) virBufferVSprintf(&buf, " <active>%ld</active>\n", def->active); virBufferAddLit(&buf, "</domainsnapshot>\n"); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 9e7e1ee..c8c64ed 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1083,6 +1083,7 @@ struct _virDomainSnapshotDef { int state; long active; + virDomainDefPtr dom; }; typedef struct _virDomainSnapshotObj virDomainSnapshotObj; @@ -1102,7 +1103,8 @@ struct _virDomainSnapshotObjList { }; virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, - int newSnapshot); + int newSnapshot, + virCapsPtr caps); void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def); char *virDomainSnapshotDefFormat(char *domain_uuid, virDomainSnapshotDefPtr def, diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 4f013e8..25c95c9 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -4058,7 +4058,7 @@ esxDomainSnapshotCreateXML(virDomainPtr domain, const char *xmlDesc, return NULL; } - def = virDomainSnapshotDefParseString(xmlDesc, 1); + def = virDomainSnapshotDefParseString(xmlDesc, 1, NULL); /* TODO:caps */ if (def == NULL) { return NULL; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4481261..5041d32 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -330,7 +330,7 @@ static void qemuDomainSnapshotLoad(void *payload, continue; } - def = virDomainSnapshotDefParseString(xmlStr, 0); + def = virDomainSnapshotDefParseString(xmlStr, 0, qemu_driver->caps); if (def == NULL) { /* Nothing we can do here, skip this one */ VIR_ERROR(_("Failed to parse snapshot XML from file '%s'"), fullpath); @@ -6308,7 +6308,7 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, if (!qemuDomainSnapshotIsAllowed(vm)) goto cleanup; - if (!(def = virDomainSnapshotDefParseString(xmlDesc, 1))) + if (!(def = virDomainSnapshotDefParseString(xmlDesc, 1, qemu_driver->caps))) goto cleanup; if (!(snap = virDomainSnapshotAssignDef(&vm->snapshots, def))) diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index e8ac48f..a8c8570 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -5547,7 +5547,7 @@ vboxDomainSnapshotCreateXML(virDomainPtr dom, virCheckFlags(0, NULL); - if (!(def = virDomainSnapshotDefParseString(xmlDesc, 1))) + if (!(def = virDomainSnapshotDefParseString(xmlDesc, 1, NULL))) /* TODO:caps */ goto cleanup; vboxIIDFromUUID(&domiid, dom->uuid); -- 1.7.1
On 04/12/2011 12:16 AM, Philipp Hahn wrote:
Save the domain description with the XML snapshot data. TODOs: - XML file is no longer nicely indented
Cosmetic, and can be fixed later.
- Fix esx driver - Fix vbox driver
Do these need to save domain xml state in the first place? They aren't using libvirt to track domain state in the first time, but call out to the hypervisor for everything. And if the hypervisor is already doing a good job of reverting across configuration changes, then it doesn't hurt if they continue to use just <domain>/<uuid> instead of full <domain> in the snapshot output that libvirt generates on virDomainSnapshotGetXMLDesc.
@@ -8661,6 +8663,14 @@ virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, else def->creationTime = tv.tv_sec;
+ xmlNodePtr domainNode = virXPathNode("./domain", ctxt); + if (domainNode == NULL) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing domain from existing snapshot")); + def->dom = NULL; + } else + def->dom = virDomainDefParseNode(caps, xml, domainNode, VIR_DOMAIN_XML_INACTIVE);
For virDomainSnapshotCreateXML, <domain> should be ignored (unless I get around to implementing my RFC of VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE); it is normally an output-only field, so I'm not sure this hunk is right.
@@ -8694,9 +8705,17 @@ char *virDomainSnapshotDefFormat(char *domain_uuid, } virBufferVSprintf(&buf, "<creationTime>%ld</creationTime>\n", def->creationTime); - virBufferAddLit(&buf, "<domain>\n"); - virBufferVSprintf(&buf, "<uuid>%s</uuid>\n", domain_uuid); - virBufferAddLit(&buf, "</domain>\n"); + if (def->dom != NULL) { + xml = virDomainDefFormat(def->dom, VIR_DOMAIN_XML_INACTIVE | VIR_DOMAIN_XML_SECURE);
Security hole. You cannot blindly add VIR_DOMAIN_XML_SECURE if this is destined to external output, rather, it has to be passed in from the user's flags, and libvirt.c has to validate that virDomainSnapshotGetXMLDesc rejects the flag on read-only connections.
+ } + if (xml != NULL) { + virBufferVSprintf(&buf, " %s", xml);
Rather than printing the xml in place, we have to touch up the domain formatter to take a variable indentation; it's a lot of refactoring just for a pretty result (worth it in the long run, but not a show-stopper to getting this feature in first). I'm seeing how easy this is to rebase, but I'll certainly be using something similar to this. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Minor semantic change - allow domain xml to be generated in place within a larger buffer, rather than having to go through a temporary string. * src/conf/domain_conf.c (virDomainDefFormatInternal): Add parameter. (virDomainDefFormat, virDomainObjFormat): Update callers. --- src/conf/domain_conf.c | 229 ++++++++++++++++++++++++----------------------- 1 files changed, 117 insertions(+), 112 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b3770c4..881350e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9863,12 +9863,15 @@ verify(((VIR_DOMAIN_XML_INTERNAL_STATUS | & DUMPXML_FLAGS) == 0); /* This internal version can accept VIR_DOMAIN_XML_INTERNAL_*, - * whereas the public version cannot. */ -static char * + * whereas the public version cannot. Also, it appends to an existing + * buffer and lets the caller flaten to string. Return -1 on failure. */ +static int virDomainDefFormatInternal(virDomainDefPtr def, - unsigned int flags) + unsigned int flags, + virBufferPtr buf) { - virBuffer buf = VIR_BUFFER_INITIALIZER; + /* XXX Also need to take an indentation parameter - either int or + * string prefix, so that snapshot xml gets uniform indentation. */ unsigned char *uuid; char uuidstr[VIR_UUID_STRING_BUFLEN]; const char *type = NULL; @@ -9877,7 +9880,7 @@ virDomainDefFormatInternal(virDomainDefPtr def, virCheckFlags(DUMPXML_FLAGS | VIR_DOMAIN_XML_INTERNAL_STATUS | VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET, - NULL); + -1); if (!(type = virDomainVirtTypeToString(def->virtType))) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, @@ -9888,99 +9891,99 @@ virDomainDefFormatInternal(virDomainDefPtr def, if (def->id == -1) flags |= VIR_DOMAIN_XML_INACTIVE; - virBufferAsprintf(&buf, "<domain type='%s'", type); + virBufferAsprintf(buf, "<domain type='%s'", type); if (!(flags & VIR_DOMAIN_XML_INACTIVE)) - virBufferAsprintf(&buf, " id='%d'", def->id); + virBufferAsprintf(buf, " id='%d'", def->id); if (def->namespaceData && def->ns.href) - virBufferAsprintf(&buf, " %s", (def->ns.href)()); - virBufferAddLit(&buf, ">\n"); + virBufferAsprintf(buf, " %s", (def->ns.href)()); + virBufferAddLit(buf, ">\n"); - virBufferEscapeString(&buf, " <name>%s</name>\n", def->name); + virBufferEscapeString(buf, " <name>%s</name>\n", def->name); uuid = def->uuid; virUUIDFormat(uuid, uuidstr); - virBufferAsprintf(&buf, " <uuid>%s</uuid>\n", uuidstr); + virBufferAsprintf(buf, " <uuid>%s</uuid>\n", uuidstr); if (def->description) - virBufferEscapeString(&buf, " <description>%s</description>\n", + virBufferEscapeString(buf, " <description>%s</description>\n", def->description); - virBufferAsprintf(&buf, " <memory>%lu</memory>\n", def->mem.max_balloon); - virBufferAsprintf(&buf, " <currentMemory>%lu</currentMemory>\n", + virBufferAsprintf(buf, " <memory>%lu</memory>\n", def->mem.max_balloon); + virBufferAsprintf(buf, " <currentMemory>%lu</currentMemory>\n", def->mem.cur_balloon); /* add blkiotune only if there are any */ if (def->blkio.weight) { - virBufferAsprintf(&buf, " <blkiotune>\n"); - virBufferAsprintf(&buf, " <weight>%u</weight>\n", + virBufferAsprintf(buf, " <blkiotune>\n"); + virBufferAsprintf(buf, " <weight>%u</weight>\n", def->blkio.weight); - virBufferAsprintf(&buf, " </blkiotune>\n"); + virBufferAsprintf(buf, " </blkiotune>\n"); } /* add memtune only if there are any */ if (def->mem.hard_limit || def->mem.soft_limit || def->mem.min_guarantee || def->mem.swap_hard_limit) - virBufferAsprintf(&buf, " <memtune>\n"); + virBufferAsprintf(buf, " <memtune>\n"); if (def->mem.hard_limit) { - virBufferAsprintf(&buf, " <hard_limit>%lu</hard_limit>\n", + virBufferAsprintf(buf, " <hard_limit>%lu</hard_limit>\n", def->mem.hard_limit); } if (def->mem.soft_limit) { - virBufferAsprintf(&buf, " <soft_limit>%lu</soft_limit>\n", + virBufferAsprintf(buf, " <soft_limit>%lu</soft_limit>\n", def->mem.soft_limit); } if (def->mem.min_guarantee) { - virBufferAsprintf(&buf, " <min_guarantee>%lu</min_guarantee>\n", + virBufferAsprintf(buf, " <min_guarantee>%lu</min_guarantee>\n", def->mem.min_guarantee); } if (def->mem.swap_hard_limit) { - virBufferAsprintf(&buf, " <swap_hard_limit>%lu</swap_hard_limit>\n", + virBufferAsprintf(buf, " <swap_hard_limit>%lu</swap_hard_limit>\n", def->mem.swap_hard_limit); } if (def->mem.hard_limit || def->mem.soft_limit || def->mem.min_guarantee || def->mem.swap_hard_limit) - virBufferAsprintf(&buf, " </memtune>\n"); + virBufferAsprintf(buf, " </memtune>\n"); if (def->mem.hugepage_backed) { - virBufferAddLit(&buf, " <memoryBacking>\n"); - virBufferAddLit(&buf, " <hugepages/>\n"); - virBufferAddLit(&buf, " </memoryBacking>\n"); + virBufferAddLit(buf, " <memoryBacking>\n"); + virBufferAddLit(buf, " <hugepages/>\n"); + virBufferAddLit(buf, " </memoryBacking>\n"); } for (n = 0 ; n < def->cpumasklen ; n++) if (def->cpumask[n] != 1) allones = 0; - virBufferAddLit(&buf, " <vcpu"); + virBufferAddLit(buf, " <vcpu"); if (!allones) { char *cpumask = NULL; if ((cpumask = virDomainCpuSetFormat(def->cpumask, def->cpumasklen)) == NULL) goto cleanup; - virBufferAsprintf(&buf, " cpuset='%s'", cpumask); + virBufferAsprintf(buf, " cpuset='%s'", cpumask); VIR_FREE(cpumask); } if (def->vcpus != def->maxvcpus) - virBufferAsprintf(&buf, " current='%u'", def->vcpus); - virBufferAsprintf(&buf, ">%u</vcpu>\n", def->maxvcpus); + virBufferAsprintf(buf, " current='%u'", def->vcpus); + virBufferAsprintf(buf, ">%u</vcpu>\n", def->maxvcpus); if (def->cputune.shares || def->cputune.vcpupin || def->cputune.period || def->cputune.quota) - virBufferAddLit(&buf, " <cputune>\n"); + virBufferAddLit(buf, " <cputune>\n"); if (def->cputune.shares) - virBufferAsprintf(&buf, " <shares>%lu</shares>\n", + virBufferAsprintf(buf, " <shares>%lu</shares>\n", def->cputune.shares); if (def->cputune.period) - virBufferAsprintf(&buf, " <period>%llu</period>\n", + virBufferAsprintf(buf, " <period>%llu</period>\n", def->cputune.period); if (def->cputune.quota) - virBufferAsprintf(&buf, " <quota>%lld</quota>\n", + virBufferAsprintf(buf, " <quota>%lld</quota>\n", def->cputune.quota); if (def->cputune.vcpupin) { int i; for (i = 0; i < def->cputune.nvcpupin; i++) { - virBufferAsprintf(&buf, " <vcpupin vcpu='%u' ", + virBufferAsprintf(buf, " <vcpupin vcpu='%u' ", def->cputune.vcpupin[i]->vcpuid); char *cpumask = NULL; @@ -9993,17 +9996,17 @@ virDomainDefFormatInternal(virDomainDefPtr def, goto cleanup; } - virBufferAsprintf(&buf, "cpuset='%s'/>\n", cpumask); + virBufferAsprintf(buf, "cpuset='%s'/>\n", cpumask); VIR_FREE(cpumask); } } if (def->cputune.shares || def->cputune.vcpupin || def->cputune.period || def->cputune.quota) - virBufferAddLit(&buf, " </cputune>\n"); + virBufferAddLit(buf, " </cputune>\n"); if (def->numatune.memory.nodemask) - virBufferAddLit(&buf, " <numatune>\n"); + virBufferAddLit(buf, " <numatune>\n"); if (def->numatune.memory.nodemask) { char *nodemask = NULL; @@ -10015,32 +10018,32 @@ virDomainDefFormatInternal(virDomainDefPtr def, goto cleanup; } - virBufferAsprintf(&buf, " <memory mode='%s' nodeset='%s'/>\n", + virBufferAsprintf(buf, " <memory mode='%s' nodeset='%s'/>\n", virDomainNumatuneMemModeTypeToString(def->numatune.memory.mode), nodemask); VIR_FREE(nodemask); } if (def->numatune.memory.nodemask) - virBufferAddLit(&buf, " </numatune>\n"); + virBufferAddLit(buf, " </numatune>\n"); if (def->sysinfo) - virDomainSysinfoDefFormat(&buf, def->sysinfo); + virDomainSysinfoDefFormat(buf, def->sysinfo); if (def->os.bootloader) { - virBufferEscapeString(&buf, " <bootloader>%s</bootloader>\n", + virBufferEscapeString(buf, " <bootloader>%s</bootloader>\n", def->os.bootloader); if (def->os.bootloaderArgs) - virBufferEscapeString(&buf, " <bootloader_args>%s</bootloader_args>\n", + virBufferEscapeString(buf, " <bootloader_args>%s</bootloader_args>\n", def->os.bootloaderArgs); } - virBufferAddLit(&buf, " <os>\n"); + virBufferAddLit(buf, " <os>\n"); - virBufferAddLit(&buf, " <type"); + virBufferAddLit(buf, " <type"); if (def->os.arch) - virBufferAsprintf(&buf, " arch='%s'", def->os.arch); + virBufferAsprintf(buf, " arch='%s'", def->os.arch); if (def->os.machine) - virBufferAsprintf(&buf, " machine='%s'", def->os.machine); + virBufferAsprintf(buf, " machine='%s'", def->os.machine); /* * HACK: For xen driver we previously used bogus 'linux' as the * os type for paravirt, whereas capabilities declare it to @@ -10048,27 +10051,27 @@ virDomainDefFormatInternal(virDomainDefPtr def, */ if (def->virtType == VIR_DOMAIN_VIRT_XEN && STREQ(def->os.type, "xen")) - virBufferAsprintf(&buf, ">%s</type>\n", "linux"); + virBufferAsprintf(buf, ">%s</type>\n", "linux"); else - virBufferAsprintf(&buf, ">%s</type>\n", def->os.type); + virBufferAsprintf(buf, ">%s</type>\n", def->os.type); if (def->os.init) - virBufferEscapeString(&buf, " <init>%s</init>\n", + virBufferEscapeString(buf, " <init>%s</init>\n", def->os.init); if (def->os.loader) - virBufferEscapeString(&buf, " <loader>%s</loader>\n", + virBufferEscapeString(buf, " <loader>%s</loader>\n", def->os.loader); if (def->os.kernel) - virBufferEscapeString(&buf, " <kernel>%s</kernel>\n", + virBufferEscapeString(buf, " <kernel>%s</kernel>\n", def->os.kernel); if (def->os.initrd) - virBufferEscapeString(&buf, " <initrd>%s</initrd>\n", + virBufferEscapeString(buf, " <initrd>%s</initrd>\n", def->os.initrd); if (def->os.cmdline) - virBufferEscapeString(&buf, " <cmdline>%s</cmdline>\n", + virBufferEscapeString(buf, " <cmdline>%s</cmdline>\n", def->os.cmdline); if (def->os.root) - virBufferEscapeString(&buf, " <root>%s</root>\n", + virBufferEscapeString(buf, " <root>%s</root>\n", def->os.root); if (!def->os.bootloader) { @@ -10081,21 +10084,21 @@ virDomainDefFormatInternal(virDomainDefPtr def, def->os.bootDevs[n]); goto cleanup; } - virBufferAsprintf(&buf, " <boot dev='%s'/>\n", boottype); + virBufferAsprintf(buf, " <boot dev='%s'/>\n", boottype); } if (def->os.bootmenu != VIR_DOMAIN_BOOT_MENU_DEFAULT) { const char *enabled = (def->os.bootmenu == VIR_DOMAIN_BOOT_MENU_ENABLED ? "yes" : "no"); - virBufferAsprintf(&buf, " <bootmenu enable='%s'/>\n", enabled); + virBufferAsprintf(buf, " <bootmenu enable='%s'/>\n", enabled); } if (def->os.bios.useserial) { const char *useserial = (def->os.bios.useserial == VIR_DOMAIN_BIOS_USESERIAL_YES ? "yes" : "no"); - virBufferAsprintf(&buf, " <bios useserial='%s'/>\n", useserial); + virBufferAsprintf(buf, " <bios useserial='%s'/>\n", useserial); } } @@ -10108,14 +10111,14 @@ virDomainDefFormatInternal(virDomainDefPtr def, _("unexpected smbios mode %d"), def->os.smbios_mode); goto cleanup; } - virBufferAsprintf(&buf, " <smbios mode='%s'/>\n", mode); + virBufferAsprintf(buf, " <smbios mode='%s'/>\n", mode); } - virBufferAddLit(&buf, " </os>\n"); + virBufferAddLit(buf, " </os>\n"); if (def->features) { int i; - virBufferAddLit(&buf, " <features>\n"); + virBufferAddLit(buf, " <features>\n"); for (i = 0 ; i < VIR_DOMAIN_FEATURE_LAST ; i++) { if (def->features & (1 << i)) { const char *name = virDomainFeatureTypeToString(i); @@ -10124,91 +10127,91 @@ virDomainDefFormatInternal(virDomainDefPtr def, _("unexpected feature %d"), i); goto cleanup; } - virBufferAsprintf(&buf, " <%s/>\n", name); + virBufferAsprintf(buf, " <%s/>\n", name); } } - virBufferAddLit(&buf, " </features>\n"); + virBufferAddLit(buf, " </features>\n"); } - if (virCPUDefFormatBuf(&buf, def->cpu, " ", 0) < 0) + if (virCPUDefFormatBuf(buf, def->cpu, " ", 0) < 0) goto cleanup; - virBufferAsprintf(&buf, " <clock offset='%s'", + virBufferAsprintf(buf, " <clock offset='%s'", virDomainClockOffsetTypeToString(def->clock.offset)); switch (def->clock.offset) { case VIR_DOMAIN_CLOCK_OFFSET_VARIABLE: - virBufferAsprintf(&buf, " adjustment='%lld'", def->clock.data.adjustment); + virBufferAsprintf(buf, " adjustment='%lld'", def->clock.data.adjustment); break; case VIR_DOMAIN_CLOCK_OFFSET_TIMEZONE: - virBufferEscapeString(&buf, " timezone='%s'", def->clock.data.timezone); + virBufferEscapeString(buf, " timezone='%s'", def->clock.data.timezone); break; } if (def->clock.ntimers == 0) { - virBufferAddLit(&buf, "/>\n"); + virBufferAddLit(buf, "/>\n"); } else { - virBufferAddLit(&buf, ">\n"); + virBufferAddLit(buf, ">\n"); for (n = 0; n < def->clock.ntimers; n++) { - if (virDomainTimerDefFormat(&buf, def->clock.timers[n]) < 0) + if (virDomainTimerDefFormat(buf, def->clock.timers[n]) < 0) goto cleanup; } - virBufferAddLit(&buf, " </clock>\n"); + virBufferAddLit(buf, " </clock>\n"); } - if (virDomainLifecycleDefFormat(&buf, def->onPoweroff, + if (virDomainLifecycleDefFormat(buf, def->onPoweroff, "on_poweroff", virDomainLifecycleTypeToString) < 0) goto cleanup; - if (virDomainLifecycleDefFormat(&buf, def->onReboot, + if (virDomainLifecycleDefFormat(buf, def->onReboot, "on_reboot", virDomainLifecycleTypeToString) < 0) goto cleanup; - if (virDomainLifecycleDefFormat(&buf, def->onCrash, + if (virDomainLifecycleDefFormat(buf, def->onCrash, "on_crash", virDomainLifecycleCrashTypeToString) < 0) goto cleanup; - virBufferAddLit(&buf, " <devices>\n"); + virBufferAddLit(buf, " <devices>\n"); if (def->emulator) - virBufferEscapeString(&buf, " <emulator>%s</emulator>\n", + virBufferEscapeString(buf, " <emulator>%s</emulator>\n", def->emulator); for (n = 0 ; n < def->ndisks ; n++) - if (virDomainDiskDefFormat(&buf, def->disks[n], flags) < 0) + if (virDomainDiskDefFormat(buf, def->disks[n], flags) < 0) goto cleanup; for (n = 0 ; n < def->ncontrollers ; n++) - if (virDomainControllerDefFormat(&buf, def->controllers[n], flags) < 0) + if (virDomainControllerDefFormat(buf, def->controllers[n], flags) < 0) goto cleanup; for (n = 0 ; n < def->nleases ; n++) - if (virDomainLeaseDefFormat(&buf, def->leases[n]) < 0) + if (virDomainLeaseDefFormat(buf, def->leases[n]) < 0) goto cleanup; for (n = 0 ; n < def->nfss ; n++) - if (virDomainFSDefFormat(&buf, def->fss[n], flags) < 0) + if (virDomainFSDefFormat(buf, def->fss[n], flags) < 0) goto cleanup; for (n = 0 ; n < def->nnets ; n++) - if (virDomainNetDefFormat(&buf, def->nets[n], flags) < 0) + if (virDomainNetDefFormat(buf, def->nets[n], flags) < 0) goto cleanup; for (n = 0 ; n < def->nsmartcards ; n++) - if (virDomainSmartcardDefFormat(&buf, def->smartcards[n], flags) < 0) + if (virDomainSmartcardDefFormat(buf, def->smartcards[n], flags) < 0) goto cleanup; for (n = 0 ; n < def->nserials ; n++) - if (virDomainChrDefFormat(&buf, def->serials[n], flags) < 0) + if (virDomainChrDefFormat(buf, def->serials[n], flags) < 0) goto cleanup; for (n = 0 ; n < def->nparallels ; n++) - if (virDomainChrDefFormat(&buf, def->parallels[n], flags) < 0) + if (virDomainChrDefFormat(buf, def->parallels[n], flags) < 0) goto cleanup; /* If there's a PV console that's preferred.. */ if (def->console) { - if (virDomainChrDefFormat(&buf, def->console, flags) < 0) + if (virDomainChrDefFormat(buf, def->console, flags) < 0) goto cleanup; } else if (def->nserials != 0) { /* ..else for legacy compat duplicate the first serial device as a @@ -10216,17 +10219,17 @@ virDomainDefFormatInternal(virDomainDefPtr def, virDomainChrDef console; memcpy(&console, def->serials[0], sizeof(console)); console.deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE; - if (virDomainChrDefFormat(&buf, &console, flags) < 0) + if (virDomainChrDefFormat(buf, &console, flags) < 0) goto cleanup; } for (n = 0 ; n < def->nchannels ; n++) - if (virDomainChrDefFormat(&buf, def->channels[n], flags) < 0) + if (virDomainChrDefFormat(buf, def->channels[n], flags) < 0) goto cleanup; for (n = 0 ; n < def->ninputs ; n++) if (def->inputs[n]->bus == VIR_DOMAIN_INPUT_BUS_USB && - virDomainInputDefFormat(&buf, def->inputs[n], flags) < 0) + virDomainInputDefFormat(buf, def->inputs[n], flags) < 0) goto cleanup; if (def->ngraphics > 0) { @@ -10238,33 +10241,33 @@ virDomainDefFormatInternal(virDomainDefPtr def, { .alias = NULL }, }; - if (virDomainInputDefFormat(&buf, &autoInput, flags) < 0) + if (virDomainInputDefFormat(buf, &autoInput, flags) < 0) goto cleanup; for (n = 0 ; n < def->ngraphics ; n++) - if (virDomainGraphicsDefFormat(&buf, def->graphics[n], flags) < 0) + if (virDomainGraphicsDefFormat(buf, def->graphics[n], flags) < 0) goto cleanup; } for (n = 0 ; n < def->nsounds ; n++) - if (virDomainSoundDefFormat(&buf, def->sounds[n], flags) < 0) + if (virDomainSoundDefFormat(buf, def->sounds[n], flags) < 0) goto cleanup; for (n = 0 ; n < def->nvideos ; n++) - if (virDomainVideoDefFormat(&buf, def->videos[n], flags) < 0) + if (virDomainVideoDefFormat(buf, def->videos[n], flags) < 0) goto cleanup; for (n = 0 ; n < def->nhostdevs ; n++) - if (virDomainHostdevDefFormat(&buf, def->hostdevs[n], flags) < 0) + if (virDomainHostdevDefFormat(buf, def->hostdevs[n], flags) < 0) goto cleanup; if (def->watchdog) - virDomainWatchdogDefFormat (&buf, def->watchdog, flags); + virDomainWatchdogDefFormat (buf, def->watchdog, flags); if (def->memballoon) - virDomainMemballoonDefFormat (&buf, def->memballoon, flags); + virDomainMemballoonDefFormat (buf, def->memballoon, flags); - virBufferAddLit(&buf, " </devices>\n"); + virBufferAddLit(buf, " </devices>\n"); if (def->seclabel.model) { const char *sectype = virDomainSeclabelTypeToString(def->seclabel.type); @@ -10276,47 +10279,52 @@ virDomainDefFormatInternal(virDomainDefPtr def, (flags & VIR_DOMAIN_XML_INACTIVE)) { /* This is the default for inactive xml, so nothing to output. */ } else { - virBufferAsprintf(&buf, " <seclabel type='%s' model='%s' relabel='%s'>\n", + virBufferAsprintf(buf, " <seclabel type='%s' model='%s' relabel='%s'>\n", sectype, def->seclabel.model, def->seclabel.norelabel ? "no" : "yes"); if (def->seclabel.label) - virBufferEscapeString(&buf, " <label>%s</label>\n", + virBufferEscapeString(buf, " <label>%s</label>\n", def->seclabel.label); if (!def->seclabel.norelabel && def->seclabel.imagelabel) - virBufferEscapeString(&buf, " <imagelabel>%s</imagelabel>\n", + virBufferEscapeString(buf, " <imagelabel>%s</imagelabel>\n", def->seclabel.imagelabel); if (def->seclabel.baselabel && (def->seclabel.type == VIR_DOMAIN_SECLABEL_DYNAMIC)) - virBufferEscapeString(&buf, " <baselabel>%s</baselabel>\n", + virBufferEscapeString(buf, " <baselabel>%s</baselabel>\n", def->seclabel.baselabel); - virBufferAddLit(&buf, " </seclabel>\n"); + virBufferAddLit(buf, " </seclabel>\n"); } } if (def->namespaceData && def->ns.format) { - if ((def->ns.format)(&buf, def->namespaceData) < 0) + if ((def->ns.format)(buf, def->namespaceData) < 0) goto cleanup; } - virBufferAddLit(&buf, "</domain>\n"); + virBufferAddLit(buf, "</domain>\n"); - if (virBufferError(&buf)) + if (virBufferError(buf)) goto no_memory; - return virBufferContentAndReset(&buf); + return 0; no_memory: virReportOOMError(); cleanup: - virBufferFreeAndReset(&buf); - return NULL; + virBufferFreeAndReset(buf); + return -1; } char * virDomainDefFormat(virDomainDefPtr def, unsigned int flags) { + virBuffer buf = VIR_BUFFER_INITIALIZER; + virCheckFlags(DUMPXML_FLAGS, NULL); - return virDomainDefFormatInternal(def, flags); + if (virDomainDefFormatInternal(def, flags, &buf) < 0) + return NULL; + + return virBufferContentAndReset(&buf); } @@ -10324,7 +10332,6 @@ static char *virDomainObjFormat(virCapsPtr caps, virDomainObjPtr obj, unsigned int flags) { - char *config_xml = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; int state; int reason; @@ -10346,11 +10353,9 @@ static char *virDomainObjFormat(virCapsPtr caps, ((caps->privateDataXMLFormat)(&buf, obj->privateData)) < 0) goto error; - if (!(config_xml = virDomainDefFormatInternal(obj->def, flags))) + if (virDomainDefFormatInternal(obj->def, flags, &buf) < 0) goto error; - virBufferAdd(&buf, config_xml, strlen(config_xml)); - VIR_FREE(config_xml); virBufferAddLit(&buf, "</domstatus>\n"); if (virBufferError(&buf)) -- 1.7.4.4
Just like VM saved state images (virsh save), snapshots MUST track the inactive domain xml to detect any ABI incompatibilities. The indentation is not perfect, but functionality comes before form. * src/conf/domain_conf.h (_virDomainSnapshotDef): Add member. (virDomainSnapshotDefParseString, virDomainSnapshotDefFormat): Update signature. * src/conf/domain_conf.c (virDomainSnapshotDefFree): Clean up. (virDomainSnapshotDefParseString): Optionally parse domain. (virDomainSnapshotDefFormat): Output full domain. * src/esx/esx_driver.c (esxDomainSnapshotCreateXML) (esxDomainSnapshotGetXMLDesc): Update callers. * src/vbox/vbox_tmpl.c (vboxDomainSnapshotCreateXML) (vboxDomainSnapshotGetXMLDesc): Likewise. * src/qemu/qemu_driver.c (qemuDomainSnapshotCreateXML) (qemuDomainSnapshotLoad, qemuDomainSnapshotGetXMLDesc) (qemuDomainSnapshotWriteMetadata): Likewise. Based on a patch by Philipp Hahn. --- Differences from Philipp's v1: rebase to latest libvirt.git caps are only needed when parsing an internal xml (that is, esx and vbox no longer have a to-do) on format, pass flags down from user src/conf/domain_conf.c | 49 +++++++++++++++++++++++++++++++++++++++++------ src/conf/domain_conf.h | 7 +++++- src/esx/esx_driver.c | 4 +- src/qemu/qemu_driver.c | 18 +++++++++++++--- src/vbox/vbox_tmpl.c | 4 +- 5 files changed, 66 insertions(+), 16 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 881350e..295b92f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10915,11 +10915,17 @@ void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def) VIR_FREE(def->name); VIR_FREE(def->description); VIR_FREE(def->parent); + virDomainDefFree(def->dom); VIR_FREE(def); } -virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, - int newSnapshot) +/* If newSnapshot is true, caps, expectedVirtTypes, and flags are ignored. */ +virDomainSnapshotDefPtr +virDomainSnapshotDefParseString(const char *xmlStr, + int newSnapshot, + virCapsPtr caps, + unsigned int expectedVirtTypes, + unsigned int flags) { xmlXPathContextPtr ctxt = NULL; xmlDocPtr xml = NULL; @@ -10927,6 +10933,7 @@ virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, virDomainSnapshotDefPtr ret = NULL; char *creation = NULL, *state = NULL; struct timeval tv; + char *tmp; xml = virXMLParse(NULL, xmlStr, "domainsnapshot.xml"); if (!xml) { @@ -10995,9 +11002,28 @@ virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, _("Could not find 'active' element")); goto cleanup; } - } - else + + /* Older snapshots were created with just <domain>/<uuid>, and + * lack domain/@type. In that case, leave dom NULL, and + * clients will have to decide between best effort + * initialization or outright failure. */ + if ((tmp = virXPathString("string(./domain/@type)", ctxt))) { + VIR_FREE(tmp); + xmlNodePtr domainNode = virXPathNode("./domain", ctxt); + if (!domainNode) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing domain in snapshot")); + goto cleanup; + } + def->dom = virDomainDefParseNode(caps, xml, domainNode, + expectedVirtTypes, flags); + if (!def->dom) + goto cleanup; + } else { + VIR_WARN("parsing older snapshot that lacks domain"); + } def->creationTime = tv.tv_sec; + } ret = def; @@ -11014,10 +11040,15 @@ cleanup: char *virDomainSnapshotDefFormat(char *domain_uuid, virDomainSnapshotDefPtr def, + unsigned int flags, int internal) { virBuffer buf = VIR_BUFFER_INITIALIZER; + virCheckFlags(VIR_DOMAIN_XML_SECURE, NULL); + + flags |= VIR_DOMAIN_XML_INACTIVE; + virBufferAddLit(&buf, "<domainsnapshot>\n"); virBufferAsprintf(&buf, " <name>%s</name>\n", def->name); if (def->description) @@ -11032,9 +11063,13 @@ char *virDomainSnapshotDefFormat(char *domain_uuid, } virBufferAsprintf(&buf, " <creationTime>%lld</creationTime>\n", def->creationTime); - virBufferAddLit(&buf, " <domain>\n"); - virBufferAsprintf(&buf, " <uuid>%s</uuid>\n", domain_uuid); - virBufferAddLit(&buf, " </domain>\n"); + if (def->dom) { + virDomainDefFormatInternal(def->dom, flags, &buf); + } else { + virBufferAddLit(&buf, " <domain>\n"); + virBufferAsprintf(&buf, " <uuid>%s</uuid>\n", domain_uuid); + virBufferAddLit(&buf, " </domain>\n"); + } if (internal) virBufferAsprintf(&buf, " <active>%ld</active>\n", def->active); virBufferAddLit(&buf, "</domainsnapshot>\n"); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index e37cf26..0241838 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1291,6 +1291,7 @@ struct _virDomainSnapshotDef { char *parent; long long creationTime; /* in seconds */ int state; + virDomainDefPtr dom; long active; /* XXX make this internal use only to identify current snap */ }; @@ -1313,10 +1314,14 @@ struct _virDomainSnapshotObjList { }; virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, - int newSnapshot); + int newSnapshot, + virCapsPtr caps, + unsigned int expectedVirtTypes, + unsigned int flags); void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def); char *virDomainSnapshotDefFormat(char *domain_uuid, virDomainSnapshotDefPtr def, + unsigned int flags, int internal); virDomainSnapshotObjPtr virDomainSnapshotAssignDef(virDomainSnapshotObjListPtr snapshots, const virDomainSnapshotDefPtr def); diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index c71b1b3..eb1633d 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -4223,7 +4223,7 @@ esxDomainSnapshotCreateXML(virDomainPtr domain, const char *xmlDesc, return NULL; } - def = virDomainSnapshotDefParseString(xmlDesc, 1); + def = virDomainSnapshotDefParseString(xmlDesc, 1, NULL, 0, 0); if (def == NULL) { return NULL; @@ -4319,7 +4319,7 @@ esxDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, virUUIDFormat(snapshot->domain->uuid, uuid_string); - xml = virDomainSnapshotDefFormat(uuid_string, &def, 0); + xml = virDomainSnapshotDefFormat(uuid_string, &def, flags, 0); cleanup: esxVI_VirtualMachineSnapshotTree_Free(&rootSnapshotList); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index da2703e..565f578 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -336,7 +336,10 @@ static void qemuDomainSnapshotLoad(void *payload, continue; } - def = virDomainSnapshotDefParseString(xmlStr, 0); + def = virDomainSnapshotDefParseString(xmlStr, 0, qemu_driver->caps, + QEMU_EXPECTED_VIRT_TYPES, + (VIR_DOMAIN_XML_INACTIVE | + VIR_DOMAIN_XML_SECURE)); if (def == NULL) { /* Nothing we can do here, skip this one */ VIR_ERROR(_("Failed to parse snapshot XML from file '%s'"), fullpath); @@ -8467,7 +8470,8 @@ static int qemuDomainSnapshotWriteMetadata(virDomainObjPtr vm, char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(vm->def->uuid, uuidstr); - newxml = virDomainSnapshotDefFormat(uuidstr, snapshot->def, 1); + newxml = virDomainSnapshotDefFormat(uuidstr, snapshot->def, + VIR_DOMAIN_XML_SECURE, 1); if (newxml == NULL) { virReportOOMError(); return -1; @@ -8493,6 +8497,12 @@ static int qemuDomainSnapshotWriteMetadata(virDomainObjPtr vm, _("failed to create snapshot file '%s'"), snapFile); goto cleanup; } + /* XXX need virsh snapshot-edit, before this makes sense: + * char *tmp; + * virAsprintf(&tmp, "snapshot-edit %s", vm->def->name); + * virEmitXMLWarning(fd, snapshot->def->name, tmp); + * VIR_FREE(tmp); + */ if (safewrite(fd, newxml, strlen(newxml)) != strlen(newxml)) { virReportSystemError(errno, _("Failed to write snapshot data to %s"), snapFile); @@ -8688,7 +8698,7 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, if (!qemuDomainSnapshotIsAllowed(vm)) goto cleanup; - if (!(def = virDomainSnapshotDefParseString(xmlDesc, 1))) + if (!(def = virDomainSnapshotDefParseString(xmlDesc, 1, NULL, 0, 0))) goto cleanup; if (!(snap = virDomainSnapshotAssignDef(&vm->snapshots, def))) @@ -8929,7 +8939,7 @@ static char *qemuDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, goto cleanup; } - xml = virDomainSnapshotDefFormat(uuidstr, snap->def, 0); + xml = virDomainSnapshotDefFormat(uuidstr, snap->def, flags, 0); cleanup: if (vm) diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 91a5423..b8deed3 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -5661,7 +5661,7 @@ vboxDomainSnapshotCreateXML(virDomainPtr dom, virCheckFlags(0, NULL); - if (!(def = virDomainSnapshotDefParseString(xmlDesc, 1))) + if (!(def = virDomainSnapshotDefParseString(xmlDesc, 1, NULL, 0, 0))) goto cleanup; vboxIIDFromUUID(&domiid, dom->uuid); @@ -5843,7 +5843,7 @@ vboxDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, def->state = VIR_DOMAIN_SHUTOFF; virUUIDFormat(dom->uuid, uuidstr); - ret = virDomainSnapshotDefFormat(uuidstr, def, 0); + ret = virDomainSnapshotDefFormat(uuidstr, def, flags, 0); cleanup: virDomainSnapshotDefFree(def); -- 1.7.4.4
* docs/schemas/domain.rng: Move guts... * docs/schemas/domaincommon.rng: ...to new file. * docs/schemas/domainsnapshot.rng: Allow new xml. * docs/schemas/Makefile.am (schema_DATA): Distribute new file. * tests/domainsnapshotxml2xmlout/full_domain.xml: New test. --- I've shortened this email by trimming the deletions from domain.rng; git shows the code motion and slight changes in the new domaincommon.rng. docs/schemas/Makefile.am | 1 + docs/schemas/domain.rng | 2541 +----------------------- docs/schemas/{domain.rng => domaincommon.rng} | 8 +- docs/schemas/domainsnapshot.rng | 14 +- tests/domainsnapshotxml2xmlout/full_domain.xml | 35 + 5 files changed, 49 insertions(+), 2550 deletions(-) copy docs/schemas/{domain.rng => domaincommon.rng} (99%) create mode 100644 tests/domainsnapshotxml2xmlout/full_domain.xml diff --git a/docs/schemas/Makefile.am b/docs/schemas/Makefile.am index 596c207..4413d9e 100644 --- a/docs/schemas/Makefile.am +++ b/docs/schemas/Makefile.am @@ -6,6 +6,7 @@ schema_DATA = \ basictypes.rng \ capability.rng \ domain.rng \ + domaincommon.rng \ domainsnapshot.rng \ interface.rng \ network.rng \ diff --git a/docs/schemas/domain.rng b/docs/schemas/domain.rng index 6ccbeed..cf0be68 100644 --- a/docs/schemas/domain.rng +++ b/docs/schemas/domain.rng @@ -5,2544 +5,5 @@ <ref name="domain"/> </start> - <include href='basictypes.rng'/> ... - <define name="filter-param-value"> - <data type="string"> - <param name="pattern">[a-zA-Z0-9_\.:]+</param> - </data> - </define> + <include href='domaincommon.rng'/> </grammar> diff --git a/docs/schemas/domain.rng b/docs/schemas/domaincommon.rng similarity index 99% copy from docs/schemas/domain.rng copy to docs/schemas/domaincommon.rng index 6ccbeed..afce778 100644 --- a/docs/schemas/domain.rng +++ b/docs/schemas/domaincommon.rng @@ -1,16 +1,12 @@ <?xml version="1.0"?> <grammar xmlns="http://relaxng.org/ns/structure/1.0" datatypeLibrary="http://www.w3.org/2001/XMLSchema-datatypes"> - <!-- We handle only document defining a domain --> - <start> - <ref name="domain"/> - </start> - + <!-- domain-related definitions used in multiple grammars --> <include href='basictypes.rng'/> <include href='storageencryption.rng'/> <include href='networkcommon.rng'/> <!-- - description element, maybe placed anywhere under the root + description element, may be placed anywhere under the root --> <define name="description"> <element name="description"> diff --git a/docs/schemas/domainsnapshot.rng b/docs/schemas/domainsnapshot.rng index 86bab0b..76caa46 100644 --- a/docs/schemas/domainsnapshot.rng +++ b/docs/schemas/domainsnapshot.rng @@ -1,9 +1,12 @@ +<?xml version="1.0"?> <!-- A Relax NG schema for the libvirt domain snapshot properties XML format --> <grammar xmlns="http://relaxng.org/ns/structure/1.0"> <start> <ref name='domainsnapshot'/> </start> + <include href='domaincommon.rng'/> + <define name='domainsnapshot'> <element name='domainsnapshot'> <interleave> @@ -33,11 +36,14 @@ </element> </optional> <optional> - <element name='domain'> - <element name='uuid'> - <text/> + <choice> + <element name='domain'> + <element name='uuid'> + <ref name="UUID"/> + </element> </element> - </element> + <ref name='domain'/> + </choice> </optional> <optional> <element name='parent'> diff --git a/tests/domainsnapshotxml2xmlout/full_domain.xml b/tests/domainsnapshotxml2xmlout/full_domain.xml new file mode 100644 index 0000000..942bd7f --- /dev/null +++ b/tests/domainsnapshotxml2xmlout/full_domain.xml @@ -0,0 +1,35 @@ +<domainsnapshot> + <name>my snap name</name> + <description>!@#$%^</description> + <parent> + <name>earlier_snap</name> + </parent> + <state>running</state> + <creationTime>1272917631</creationTime> +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219100</memory> + <currentMemory>219100</currentMemory> + <vcpu cpuset='1-4,8-20,525'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' unit='0'/> + </disk> + <controller type='ide' index='0'/> + <memballoon model='virtio'/> + </devices> +</domain> + <active>1</active> +</domainsnapshot> -- 1.7.4.4
Hello Eric, Am Samstag 13 August 2011 00:08:11 schrieb Eric Blake:
On 04/12/2011 12:16 AM, Philipp Hahn wrote:
Save the domain description with the XML snapshot data. TODOs: - XML file is no longer nicely indented
Cosmetic, and can be fixed later.
- Fix esx driver - Fix vbox driver
Do these need to save domain xml state in the first place? They aren't using libvirt to track domain state in the first time, but call out to the hypervisor for everything. And if the hypervisor is already doing a good job of reverting across configuration changes, then it doesn't hurt if they continue to use just <domain>/<uuid> instead of full <domain> in the snapshot output that libvirt generates on virDomainSnapshotGetXMLDesc.
I don't have access to any ESX system, so I couldn't check. At least with our (very) old VMWare Server when doing a snapshot, the configuration is saved, so on revert you get the old configuration again. That difference was actually what got us to implement this for Qemu as well. VBox I didn't check: We're using it for another project I'm currently not working on, but there libvirt isn't used to manage it.
@@ -8694,9 +8705,17 @@ char *virDomainSnapshotDefFormat(char *domain_uuid, } virBufferVSprintf(&buf, "<creationTime>%ld</creationTime>\n", def->creationTime); - virBufferAddLit(&buf, "<domain>\n"); - virBufferVSprintf(&buf, "<uuid>%s</uuid>\n", domain_uuid); - virBufferAddLit(&buf, "</domain>\n"); + if (def->dom != NULL) { + xml = virDomainDefFormat(def->dom, VIR_DOMAIN_XML_INACTIVE | VIR_DOMAIN_XML_SECURE);
Security hole. You cannot blindly add VIR_DOMAIN_XML_SECURE if this is destined to external output, rather, it has to be passed in from the user's flags, and libvirt.c has to validate that virDomainSnapshotGetXMLDesc rejects the flag on read-only connections.
Yes, but for a PoC that was the easiest thing to do. Glad you spotted that. Sincerely Philipp Hahn -- Philipp Hahn Open Source Software Engineer hahn@univention.de Univention GmbH Linux for Your Business fon: +49 421 22 232- 0 Mary-Somerville-Str.1 D-28359 Bremen fax: +49 421 22 232-99 http://www.univention.de/
Restore the domain description from the snapshot information. TODOs: - Only restart the KVM process when the snapshot is incompatible with "loadvm". This would have the benefir of being able to switch saved states very fast and also doesn't disconnect any VNC viewer. Signed-off-by: Philipp Hahn <hahn@univention.de> --- src/qemu/qemu_driver.c | 39 ++++++++++++++++++++++++++++++++++++++- 1 files changed, 38 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5041d32..5610961 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6282,6 +6282,7 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, { struct qemud_driver *driver = domain->conn->privateData; virDomainObjPtr vm = NULL; + virDomainDefPtr newDef = NULL; virDomainSnapshotObjPtr snap = NULL; virDomainSnapshotPtr snapshot = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -6311,6 +6312,18 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, if (!(def = virDomainSnapshotDefParseString(xmlDesc, 1, qemu_driver->caps))) goto cleanup; + /* create copy */ + if (!def->dom) { + char *xml = NULL; + if (!(xml = virDomainDefFormat(vm->def, VIR_DOMAIN_XML_INACTIVE | VIR_DOMAIN_XML_SECURE))) + goto cleanup; + newDef = virDomainDefParseString(driver->caps, xml, VIR_DOMAIN_XML_INACTIVE); + VIR_FREE(xml); + if (!newDef) + goto cleanup; + def->dom = newDef; + } + if (!(snap = virDomainSnapshotAssignDef(&vm->snapshots, def))) goto cleanup; @@ -6561,6 +6574,7 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, virDomainEventPtr event = NULL; qemuDomainObjPrivatePtr priv; int rc; + virDomainDefPtr newDef = NULL; virCheckFlags(0, -1); @@ -6586,10 +6600,23 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0) goto cleanup; + /* create copy */ + if (snap->def->dom) { + char *xml = NULL; + /* TODO:Compare old definition to new definition to decide, if KVM must be restarted or existing KVM can be reused. */ + if (!(xml = virDomainDefFormat(snap->def->dom, VIR_DOMAIN_XML_INACTIVE | VIR_DOMAIN_XML_SECURE))) + goto cleanup; + newDef = virDomainDefParseString(driver->caps, xml, VIR_DOMAIN_XML_INACTIVE); + VIR_FREE(xml); + if (!newDef) + goto cleanup; + virDomainObjAssignDef(vm, newDef, false); + } + if (snap->def->state == VIR_DOMAIN_RUNNING || snap->def->state == VIR_DOMAIN_PAUSED) { - if (virDomainObjIsActive(vm)) { + if (virDomainObjIsActive(vm) && !newDef) { priv = vm->privateData; qemuDomainObjEnterMonitorWithDriver(driver, vm); rc = qemuMonitorLoadSnapshot(priv->mon, snap->def->name); @@ -6598,6 +6625,16 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, goto endjob; } else { + if (virDomainObjIsActive(vm)) { + //qemudShutdownVMDaemon(driver, vm, 0); // <= 0.12.4 + qemuProcessStop(driver, vm, 0); // >= 14 + if (!vm->persistent) { + if (qemuDomainObjEndJob(vm) > 0) + virDomainRemoveInactive(&driver->domains, vm); + vm = NULL; + goto cleanup; + } + } if (qemuDomainSnapshotSetCurrentActive(vm, driver->snapshotDir) < 0) goto endjob; -- 1.7.1
Hello Eric, On Monday April, 11th 2011 23:39:05 Eric Blake wrote:
Are you referring to a disk snapshot, created via 'virsh snapshot' on a qcow2 image, or a memory snapshot, created via 'virsh save'?
The first one (snapshot).
So it sounds like you are talking about the 'virsh snapshot' family of commands. Indeed, there's probably quite a bit of work to do in that code to make it more reliable.
See my proof-of-concept implementation, which I just posted for comments. BYtE Philipp -- Philipp Hahn Open Source Software Engineer hahn@univention.de Univention GmbH Linux for Your Business fon: +49 421 22 232- 0 Mary-Somerville-Str.1 D-28359 Bremen fax: +49 421 22 232-99 http://www.univention.de/
participants (2)
-
Eric Blake -
Philipp Hahn