[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 2/3] snapshot: save domain description with snapshot

On 04/12/2011 12:16 AM, Philipp Hahn wrote:
Save the domain description with the XML snapshot data.
- 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,
          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",
-    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

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]