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

Re: [libvirt] Allow custom metadata in domain configuration XML



On 01/23/2012 07:26 PM, Zeeshan Ali (Khattak) wrote:
> From: "Zeeshan Ali (Khattak)" <zeeshanak gnome org>
> 
> Applications can now insert custom nodes and hierarchies into domain
> cofiguration XML. Although currently not enforced, application are

s/cofiguration/configuration/
s/application are/applications are/

> required to use their own namespaces on every custom node they insert.

I also mentioned that we request only one element per namespace, in
light of Dan's proposal for a new API that can get and set metadata on a
per-namespace basis, so that applications don't have to filter all
elements for the one they are interested in:

https://www.redhat.com/archives/libvir-list/2012-January/msg00902.html

> ---
>  docs/formatdomain.html.in                          |   18 +++++++++
>  docs/schemas/domaincommon.rng                      |   26 +++++++++++++
>  src/conf/domain_conf.c                             |   24 ++++++++++++
>  src/conf/domain_conf.h                             |    3 ++
>  tests/domainsnapshotxml2xmlout/metadata.xml        |   38 ++++++++++++++++++++
>  tests/domainsnapshotxml2xmltest.c                  |    1 +
>  tests/qemuxml2argvdata/qemuxml2argv-metadata.args  |    4 ++
>  tests/qemuxml2argvdata/qemuxml2argv-metadata.xml   |   29 +++++++++++++++
>  .../qemuxml2xmloutdata/qemuxml2xmlout-metadata.xml |   29 +++++++++++++++
>  tests/qemuxml2xmltest.c                            |    2 +

Thanks for taking in my suggestions.  'git send-email
--subject-prefix=PATCHv2' makes it easier to state when you are sending
a v2.

I added you to AUTHORS; let me know if I need to update the spelling to
match your preferred listing.

> +<pre>
> + ...
> +  &lt;metadata&gt;
> +    &lt;app1:foo xmlns:app1="http://app1.org/app1/"&gt;..&lt;/app1:foo&gt;
> +    &lt;app2:bar xmlns:app2="http://app1.org/app2/"&gt;..&lt;/app2:bar&gt;
> +  &lt;/metadata&gt;
> +  ...</pre>

Someday, we might try to list actual metadata (like danpb's suggestion
of <virtman:guest xmlns:virtman="http://virt-manager.org/guets/1.0";;>);
but I'm okay waiting for some implementation experience before listing
something that actually appears in typical distro setups.

> +
> +    <dl>
> +      <dt><code>metadata</code></dt>
> +      <dd><code>metadata</code> node could be used by applications to
> +      store custom metadata in the form of XML nodes/trees. Applications
> +      must use custom namespaces on their XML nodes/trees.
> +      <span class="since">Since 0.9.9</span></dd>

since 0.9.10; and document the further restriction of only one element
per namespace.

I also think that this section belongs up next to <description>; but
I'll move it as a followup patch.

> +++ b/docs/schemas/domaincommon.rng
> @@ -43,6 +43,9 @@
>            <ref name="seclabel"/>
>          </optional>
>          <optional>
> +          <ref name="metadata"/>
> +        </optional>
> +        <optional>

I'm moving this up next to description now.  That is, both <description>
and <metadata> are overall metadata about the domain, and thus belong
next to one another.

>            <ref name='qemucmdline'/>
>          </optional>
>        </interleave>
> @@ -2942,6 +2945,29 @@
>      </element>
>    </define>
>  
> +  <define name="metadata">
> +    <element name="metadata">
> +      <zeroOrMore>
> +          <ref name="customElement"/>

Indentation.

> +++ b/src/conf/domain_conf.c
> @@ -1500,6 +1500,9 @@ void virDomainDefFree(virDomainDefPtr def)
>      if (def->namespaceData && def->ns.free)
>          (def->ns.free)(def->namespaceData);
>  
> +    if (def->metadata)
> +        xmlFreeNode(def->metadata);

Useless use of if before free, since xmlFreeNode is documented as a
no-op on NULL.  I'll update our list of function names to include
xmlFreeNode(), so that 'make syntax-check' will prevent this in the
future, as a followup patch.

> +
>      VIR_FREE(def);
>  }
>  
> @@ -8072,6 +8075,11 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
>          def->os.smbios_mode = VIR_DOMAIN_SMBIOS_NONE; /* not present */
>      }
>  
> +    /* Extract custom metadata */
> +    if ((node = virXPathNode("./metadata[1]", ctxt)) != NULL) {
> +        def->metadata = xmlCopyNode (node, 1);

Style - libvirt doesn't use space before ( in function calls.

> @@ -11833,6 +11841,22 @@ virDomainDefFormatInternal(virDomainDefPtr def,
>              goto cleanup;
>      }
>  
> +    /* Custom metadata comes at the end */
> +    if (def->metadata) {
> +        xmlBufferPtr xmlbuf;

I'll move this earlier in a subsequent patch.

> +
> +        xmlbuf = xmlBufferCreate();
> +        if (xmlNodeDump(xmlbuf, def->metadata->doc, def->metadata, 4, 1) < 0) {

I'm still not convinced you got this right.

</me 2 hours later, after reading libxml2 docs>

Nope.  The problem is that for indentation on output to work, we have to
explicitly request ignoring extra whitespace on input, via
xmlKeepBlanksDefault() (it defaults to 1, but must be 0 during the
parse).  Furthermore, the indentation generated by libxml defaults to 2
spaces per 1 level, which means that we need virBufferGetIndent / 2, to
properly adjust things.

> +            xmlBufferFree(xmlbuf);
> +            goto cleanup;
> +        }
> +        virBufferAdjustIndent(buf, 2);
> +        virBufferAdd(buf, (char *) xmlBufferContent(xmlbuf), xmlBufferLength(xmlbuf));

It's nice to stay in 80 columns, when possible.

> diff --git a/tests/domainsnapshotxml2xmlout/metadata.xml b/tests/domainsnapshotxml2xmlout/metadata.xml
> new file mode 100644
> index 0000000..45180c9
> --- /dev/null
> +++ b/tests/domainsnapshotxml2xmlout/metadata.xml
> @@ -0,0 +1,38 @@
> +<domainsnapshot>
> +  <name>my snap name</name>
> +  <description>! #$%^</description>
> +  <state>running</state>
> +  <parent>
> +    <name>earlier_snap</name>
> +  </parent>
> +  <creationTime>1272917631</creationTime>
> +  <domain type='qemu'>
> +    <name>QEMUGuest1</name>

> +    </devices>
> +    <metadata>
> +      <app1:foo xmlns:app1="http://foo.org/";>fooish</app1:foo>
> +      <app2:bar xmlns:app2="http://bar.com/"; maman="baz">barish</app2:bar>
> +    </metadata>

Looks like a good test; the reason that you couldn't trigger any
indentation issues was that you weren't actually regenerating
indentation, but reusing the spacing that was present on input.  I'll
have to tweak this as well when I reorder things.

Here's what I'm squashing in to your patch, before I work on the
followups to reorder things.

diff --git i/docs/formatdomain.html.in w/docs/formatdomain.html.in
index 69a85b6..6b025e8 100644
--- i/docs/formatdomain.html.in
+++ w/docs/formatdomain.html.in
@@ -3559,7 +3559,7 @@ qemu-kvm -net nic,model=? /dev/null
     <h3><a name="customMetadata">Custom metadata</a></h3>

 <pre>
- ...
+  ...
   &lt;metadata&gt;
     &lt;app1:foo xmlns:app1="http://app1.org/app1/"&gt;..&lt;/app1:foo&gt;
     &lt;app2:bar xmlns:app2="http://app1.org/app2/"&gt;..&lt;/app2:bar&gt;
@@ -3568,10 +3568,12 @@ qemu-kvm -net nic,model=? /dev/null

     <dl>
       <dt><code>metadata</code></dt>
-      <dd><code>metadata</code> node could be used by applications to
+      <dd>The <code>metadata</code> node can be used by applications to
       store custom metadata in the form of XML nodes/trees. Applications
-      must use custom namespaces on their XML nodes/trees.
-      <span class="since">Since 0.9.9</span></dd>
+      must use custom namespaces on their XML nodes/trees, with only
+      one top-level element per namespace (if the application needs
+      structure, they should have sub-elements to their namespace
+      element). <span class="since">Since 0.9.10</span></dd>
     </dl>

     <h2><a name="examples">Example configs</a></h2>
diff --git i/docs/schemas/domaincommon.rng w/docs/schemas/domaincommon.rng
index b42d758..4fa968d 100644
--- i/docs/schemas/domaincommon.rng
+++ w/docs/schemas/domaincommon.rng
@@ -26,6 +26,9 @@
           <ref name="description"/>
         </optional>
         <optional>
+          <ref name="metadata"/>
+        </optional>
+        <optional>
           <ref name="cpu"/>
         </optional>
         <optional>
@@ -43,9 +46,6 @@
           <ref name="seclabel"/>
         </optional>
         <optional>
-          <ref name="metadata"/>
-        </optional>
-        <optional>
           <ref name='qemucmdline'/>
         </optional>
       </interleave>
@@ -2948,7 +2948,7 @@
   <define name="metadata">
     <element name="metadata">
       <zeroOrMore>
-          <ref name="customElement"/>
+        <ref name="customElement"/>
       </zeroOrMore>
     </element>
   </define>
diff --git i/src/conf/domain_conf.c w/src/conf/domain_conf.c
index fb78d93..f2c8d02 100644
--- i/src/conf/domain_conf.c
+++ w/src/conf/domain_conf.c
@@ -1500,8 +1500,7 @@ void virDomainDefFree(virDomainDefPtr def)
     if (def->namespaceData && def->ns.free)
         (def->ns.free)(def->namespaceData);

-    if (def->metadata)
-        xmlFreeNode(def->metadata);
+    xmlFreeNode(def->metadata);

     VIR_FREE(def);
 }
@@ -8077,7 +8076,7 @@ static virDomainDefPtr
virDomainDefParseXML(virCapsPtr caps,

     /* Extract custom metadata */
     if ((node = virXPathNode("./metadata[1]", ctxt)) != NULL) {
-        def->metadata = xmlCopyNode (node, 1);
+        def->metadata = xmlCopyNode(node, 1);
     }

     /* we have to make a copy of all of the callback pointers here since
@@ -8219,6 +8218,7 @@ virDomainDefParse(const char *xmlStr,
 {
     xmlDocPtr xml;
     virDomainDefPtr def = NULL;
+    int keepBlanksDefault = xmlKeepBlanksDefault(0);

     if ((xml = virXMLParse(filename, xmlStr, _("(domain_definition)")))) {
         def = virDomainDefParseNode(caps, xml, xmlDocGetRootElement(xml),
@@ -8226,6 +8226,7 @@ virDomainDefParse(const char *xmlStr,
         xmlFreeDoc(xml);
     }

+    xmlKeepBlanksDefault(keepBlanksDefault);
     return def;
 }

@@ -8319,6 +8320,7 @@ virDomainObjParseFile(virCapsPtr caps,
 {
     xmlDocPtr xml;
     virDomainObjPtr obj = NULL;
+    int keepBlanksDefault = xmlKeepBlanksDefault(0);

     if ((xml = virXMLParseFile(filename))) {
         obj = virDomainObjParseNode(caps, xml,
@@ -8327,6 +8329,7 @@ virDomainObjParseFile(virCapsPtr caps,
         xmlFreeDoc(xml);
     }

+    xmlKeepBlanksDefault(keepBlanksDefault);
     return obj;
 }

@@ -11844,17 +11847,25 @@ virDomainDefFormatInternal(virDomainDefPtr def,
     /* Custom metadata comes at the end */
     if (def->metadata) {
         xmlBufferPtr xmlbuf;
-
+        int oldIndentTreeOutput = xmlIndentTreeOutput;
+
+        /* Indentation on output requires that we previously set
+         * xmlKeepBlanksDefault to 0 when parsing; also, libxml does 2
+         * spaces per level of indentation of intermediate elements,
+         * but no leading indentation before the starting element.
+         * Thankfully, libxml maps what looks like globals into
+         * thread-local uses, so we are thread-safe.  */
+        xmlIndentTreeOutput = 1;
         xmlbuf = xmlBufferCreate();
-        if (xmlNodeDump(xmlbuf, def->metadata->doc, def->metadata, 4,
1) < 0) {
+        if (xmlNodeDump(xmlbuf, def->metadata->doc, def->metadata,
+                        virBufferGetIndent(buf, false) / 2 + 1, 1) < 0) {
             xmlBufferFree(xmlbuf);
+            xmlIndentTreeOutput = oldIndentTreeOutput;
             goto cleanup;
         }
-        virBufferAdjustIndent(buf, 2);
-        virBufferAdd(buf, (char *) xmlBufferContent(xmlbuf),
xmlBufferLength(xmlbuf));
-        virBufferAdjustIndent(buf, -2);
+        virBufferAsprintf(buf, "  %s\n", (char *)
xmlBufferContent(xmlbuf));
         xmlBufferFree(xmlbuf);
-        virBufferAddLit(buf, "\n");
+        xmlIndentTreeOutput = oldIndentTreeOutput;
     }

     virBufferAddLit(buf, "</domain>\n");
@@ -12541,11 +12552,14 @@ virDomainSnapshotDefParseString(const char
*xmlStr,
     struct timeval tv;
     int active;
     char *tmp;
+    int keepBlanksDefault = xmlKeepBlanksDefault(0);

     xml = virXMLParseCtxt(NULL, xmlStr, _("(domain_snapshot)"), &ctxt);
     if (!xml) {
+        xmlKeepBlanksDefault(keepBlanksDefault);
         return NULL;
     }
+    xmlKeepBlanksDefault(keepBlanksDefault);

     if (VIR_ALLOC(def) < 0) {
         virReportOOMError();
diff --git i/tests/qemuxml2argvdata/qemuxml2argv-metadata.xml
w/tests/qemuxml2argvdata/qemuxml2argv-metadata.xml
index a6888ee..aa9de07 100644
--- i/tests/qemuxml2argvdata/qemuxml2argv-metadata.xml
+++ w/tests/qemuxml2argvdata/qemuxml2argv-metadata.xml
@@ -23,7 +23,8 @@
     <memballoon model='virtio'/>
   </devices>
+  <!-- intentional mis-indentation -->
   <metadata>
-    <app1:foo xmlns:app1="http://foo.org/";>fooish</app1:foo>
-    <app2:bar xmlns:app2="http://bar.com/"; maman="baz">barish</app2:bar>
+      <app1:foo xmlns:app1="http://foo.org/";>fooish</app1:foo>
+  <app2:bar xmlns:app2="http://bar.com/"; maman="baz">barish</app2:bar>
   </metadata>
 </domain>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


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