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

Eric Blake eblake at redhat.com
Wed Jul 20 21:54:22 UTC 2011


On 07/20/2011 12:21 AM, Laine Stump wrote:
> the domain XML<interface>  element is updated in the following ways:
>
> 1)<virtualportprofile>  can be specified when source type='network'
> (previously it was only valid for source type='direct')
>
> (Since virtualPortProfile is going to be used in both the domain and
> network RNG, its RNG definition was moved into a separate file that will
> be included by both.)
>
> 2) A new attribute "portgroup" has been added to the<source>
> element. When source type='network' (the only time portgroup is
> recognized), extra configuration information will be taken from the
> <portgroup>  element of the given name.
>
> 3) Each virDomainNetDef now also potentially has a virDomainActualNetDef
> which is a private object (never exported/imported via the public API,
> and not defined in the RNG) that is used to maintain information about
> the physical device that was actually used for a NetDef that was of
> type VIR_DOMAIN_NET_TYPE_NETWORK.
>
> The virDomainActualNetDef will only be parsed/formatted if the
> parse/format function is called with the
> VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET flag set (which is only
> needed when saving/loading a running domain's state info to the
> stateDir).
> ---
>
> The internal flag created a lot of discussion on the list, and what
> was finally decided on was to leave the existing
> VIR_DOMAIN_XML_INTERNAL_STATUS in the regular flags (and also put my
> new VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET there), but put in a
> compile-time protection against re-using those bits for the public
> API, and a runtime check to make sure nobody calls the public API with
> those bits on. Eric took care of this in a separate patch, which was
> pushed on Monday.

And in case it isn't obvious:

If we ever add a new public bit in the future such that the compile-time 
test fails, we simply move the internal bits to another free location 
and recompile - since they are internal, they do not leak over RPC, so 
there are no cross-version dependencies and thus can be changed at will. 
  Meanwhile, if a newer version of libvirt adds new bits and talks to an 
older version, the runtime check guarantees that the new bits will be 
rejected as unknown rather than accidentally turning on the internal 
behavior of the older version.

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

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

[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]

>
> +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.

>
> +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.

> @@ -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.

> @@ -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:

diff --git i/cfg.mk w/cfg.mk
index 773a3df..f2a951d 100644
--- i/cfg.mk
+++ w/cfg.mk
@@ -97,6 +97,7 @@ useless_free_options =				\
    --name=virCommandFree				\
    --name=virConfFreeList			\
    --name=virConfFreeValue			\
+  --name=virDomainActualNetDefFree		\
    --name=virDomainChrDefFree			\
    --name=virDomainChrSourceDefFree		\
    --name=virDomainControllerDefFree		\
diff --git i/src/conf/domain_conf.c w/src/conf/domain_conf.c
index e6d07d1..f4a42db 100644
--- i/src/conf/domain_conf.c
+++ w/src/conf/domain_conf.c
@@ -752,6 +752,8 @@ virDomainActualNetDefFree(virDomainActualNetDefPtr def)
      default:
          break;
      }
+
+    VIR_FREE(def);
  }

  void virDomainNetDefFree(virDomainNetDefPtr def)
@@ -2633,7 +2635,7 @@ virDomainActualNetDefParseXML(xmlNodePtr node,
                               _("missing type attribute in interface's 
<actual> element"));
          goto error;
      }
-    if ((int)(actual->type = virDomainNetTypeFromString(type)) < 0) {
+    if ((actual->type = virDomainNetTypeFromString(type)) < 0) {
          virDomainReportError(VIR_ERR_INTERNAL_ERROR,
                               _("unknown type '%s' in interface's 
<actual> element"), type);
          goto error;
@@ -2679,6 +2681,7 @@ virDomainActualNetDefParseXML(xmlNodePtr node,
  error:
      VIR_FREE(type);
      VIR_FREE(mode);
+    virDomainActualNetDefFree(actual);

      ctxt->node = save_ctxt;
      return ret;
@@ -6887,8 +6890,7 @@ virDomainDefPtr virDomainDefParseNode(virCapsPtr caps,
      }

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

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

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

  cleanup:
      xmlXPathFreeContext(ctxt);
diff --git i/src/conf/domain_conf.h w/src/conf/domain_conf.h
index 9e9db41..11d27f3 100644
--- i/src/conf/domain_conf.h
+++ w/src/conf/domain_conf.h
@@ -351,7 +351,7 @@ enum virDomainNetVirtioTxModeType {
  typedef struct _virDomainActualNetDef virDomainActualNetDef;
  typedef virDomainActualNetDef *virDomainActualNetDefPtr;
  struct _virDomainActualNetDef {
-    enum virDomainNetType type;
+    int type; /* enum virDomainNetType */
      union {
          struct {
              char *brname;
diff --git i/src/libvirt_private.syms w/src/libvirt_private.syms
index f03951f..306550b 100644
--- i/src/libvirt_private.syms
+++ w/src/libvirt_private.syms
@@ -743,6 +743,7 @@ virNetworkSaveConfig;
  virNetworkSetBridgeMacAddr;
  virNetworkSetBridgeName;

+
  # node_device_conf.h
  virNodeDevCapTypeToString;
  virNodeDevCapsDefFree;
diff --git i/src/libxl/libxl_driver.c w/src/libxl/libxl_driver.c
index fa3f6a8..e84fa36 100644
--- i/src/libxl/libxl_driver.c
+++ w/src/libxl/libxl_driver.c
@@ -707,7 +707,7 @@ libxlVmStart(libxlDriverPrivatePtr driver, 
virDomainObjPtr vm,
      }

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

      if (libxl_userdata_store(&priv->ctx, domid, "libvirt-xml",
@@ -1852,7 +1852,7 @@ libxlDoDomainSave(libxlDriverPrivatePtr driver, 
virDomainObjPtr vm,
          goto cleanup;
      }

-    if ((xml = virDomainDefFormat(vm->def)) == NULL)
+    if ((xml = virDomainDefFormat(vm->def, 0)) == NULL)
          goto cleanup;
      xml_len = strlen(xml) + 1;



-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org




More information about the libvir-list mailing list