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

Re: [libvirt] [PATCHv2 4/9] conf: support abstracted interface info in domain interface XML



On 07/20/2011 05:54 PM, Eric Blake wrote:
On 07/20/2011 12:21 AM, Laine Stump wrote:


<p>
-      Provides a virtual network using a bridge device in the host.
...
-      overridden with the&lt;target&gt; element (see
+
+      Provides a connection whose details are described by the named

Why the blank line  between <p> and the start of the paragraph?


I usually put a few blank lines in between what I'm changing and the surrounding text to minimize the amount of changed lines due to re-flowing. I then close up the blank lines before I'm done. OR I don't - this time I forgot.



[and I really hate it that thunderbird is back to its habits of munging quoted text in my emails again - for a while there, I was running a version where the problem went away, but the latest distro upgrade made it resurface]

I've been very bothered lately by Thunderbird's belief that it's okay to mess around with my text however it sees fit. For example, I can't get it to honor extra indenting whitespace I put at the beginning of lines when I'm putting XML examples in my message.




+void
+virDomainActualNetDefFree(virDomainActualNetDefPtr def)

Add this to cfg.mk's list of free-like functions.

+{
+    if (!def)
+        return;
+
+    switch (def->type) {
+    case VIR_DOMAIN_NET_TYPE_BRIDGE:
+        VIR_FREE(def->data.bridge.brname);
+        break;
+    case VIR_DOMAIN_NET_TYPE_DIRECT:
+        VIR_FREE(def->data.direct.linkdev);
+        VIR_FREE(def->data.direct.virtPortProfile);
+        break;
+    default:
+        break;
+    }
+}

Memory leak of def itself.


Derp!




+static int
+virDomainActualNetDefParseXML(xmlNodePtr node,
+                              xmlXPathContextPtr ctxt,
+                              virDomainActualNetDefPtr *def)
+{
+    virDomainActualNetDefPtr actual = NULL;
+    int ret = -1;
+    xmlNodePtr save_ctxt = ctxt->node;
+    char *type = NULL;
+    char *mode = NULL;
+
+    if (VIR_ALLOC(actual)<  0) {
+        virReportOOMError();
+        return -1;
+    }
+
+    ctxt->node = node;
+
+    type = virXMLPropString(node, "type");
+    if (!type) {
+        virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("missing type attribute in interface's<actual> element"));
+        goto error;
+    }
+    if ((int)(actual->type = virDomainNetTypeFromString(type))<  0) {

This cast is not needed if you tweak domain_conf.h.

+
+    *def = actual;
+    actual = NULL;
+    ret = 0;
+error:
+    VIR_FREE(type);
+    VIR_FREE(mode);
+
+    ctxt->node = save_ctxt;
+    return ret;

Memory leak of actual on error cases.


Double derp! (This previously didn't leak because I allocated directly into the parent object, and let the caller's destructor cleanup on failure).



@@ -6772,7 +6887,8 @@ virDomainDefPtr virDomainDefParseNode(virCapsPtr caps,
      }

      ctxt->node = root;
- def = virDomainDefParseXML(caps, xml, root, ctxt, expectedVirtTypes, flags); + def = virDomainDefParseXML(caps, xml, root, ctxt, expectedVirtTypes,
+                               flags);

Your change and then undo made for a spurious hunk.


  cleanup:
      xmlXPathFreeContext(ctxt);
@@ -6803,7 +6919,8 @@ virDomainObjParseNode(virCapsPtr caps,
      }

      ctxt->node = root;
- obj = virDomainObjParseXML(caps, xml, ctxt, expectedVirtTypes, flags);
+    obj = virDomainObjParseXML(caps, xml, ctxt, expectedVirtTypes,
+                               flags);

and another one.

@@ -10008,7 +10201,10 @@ int virDomainSaveStatus(virCapsPtr caps,
                          const char *statusDir,
                          virDomainObjPtr obj)
  {
- unsigned int flags = VIR_DOMAIN_XML_SECURE|VIR_DOMAIN_XML_INTERNAL_STATUS;
+    unsigned int flags = (VIR_DOMAIN_XML_SECURE |
+                          VIR_DOMAIN_XML_INTERNAL_STATUS |
+                          VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET);
+
      int ret = -1;
      char *xml;

@@ -10099,7 +10295,8 @@ static virDomainObjPtr virDomainLoadStatus(virCapsPtr caps,
          goto error;

if (!(obj = virDomainObjParseFile(caps, statusFile, expectedVirtTypes,
-                                      VIR_DOMAIN_XML_INTERNAL_STATUS)))
+                                      VIR_DOMAIN_XML_INTERNAL_STATUS |
+ VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET)))

Thinking out loud here - it looks like we never use _INTERNAL_STATUS or _INTERNAL_ACTUAL_NET in isolation - as of this patch, it is always both or neither. Maybe we could have let _INTERNAL_STATUS be the key on whether to also output/parse <actual> elements rather than adding a new flag. On the other hand, if we ever change our mind and decide that it makes sense to let the user do 'virsh dumpxml dom --actual' to see which resources actually got used, then it is easier to change VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET into a public flag, than it is if we lump all internal actions under a single _INTERNAL_STATUS flags. So no change necessary for now.

+++ b/src/conf/domain_conf.h
@@ -343,6 +343,27 @@ enum virDomainNetVirtioTxModeType {
      VIR_DOMAIN_NET_VIRTIO_TX_MODE_LAST,
  };

+/* Config that was actually used to bring up interface, after
+ * resolving network reference. This is private data, only used within
+ * libvirt, but still must maintain backward compatibility, because
+ * different versions of libvirt may read the same data file.
+ */
+typedef struct _virDomainActualNetDef virDomainActualNetDef;
+typedef virDomainActualNetDef *virDomainActualNetDefPtr;
+struct _virDomainActualNetDef {
+    enum virDomainNetType type;

Per the earlier cast comment, use int here, to match most other _virDomain*Def typed unions.


Okay. But the original struct this was copied from (virDomainNetDef) uses the enum directly.



@@ -743,7 +749,6 @@ virNetworkSaveConfig;
  virNetworkSetBridgeMacAddr;
  virNetworkSetBridgeName;

-
  # node_device_conf.h

A spurious whitespace change.

+++ b/src/libxl/libxl_driver.c
@@ -707,7 +707,7 @@ libxlVmStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
      }

      vm->def->id = domid;
-    if ((dom_xml = virDomainDefFormat(vm->def, 0)) == NULL)
+    if ((dom_xml = virDomainDefFormat(vm->def)) == NULL)

Oops, that's a compile failure, caused by over-undoing your now-abandoned idea of adding a parameter.

ACK with this squashed in:

Squashed and pushed. Thanks!




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