[libvirt] [PATCH 07/10] network: new XML to support virtual switch functionality
Daniel P. Berrange
berrange at redhat.com
Fri Jul 8 13:17:07 UTC 2011
On Tue, Jul 05, 2011 at 03:45:55AM -0400, Laine Stump wrote:
> This implements the changes to <network> and domain <interface> XML that
> were earlier specified in the RNG.
>
> 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_ACTUAL_NET
> flags set (which is only needed when saving/loading a running domain's
> state info to the stateDir). To prevent this flag bit from
> accidentally being used in the public API, a "RESERVED" placeholder
> was put into the public flags enum (at the same time, I noticed there
> was another private flag that hadn't been reserved, so I added that
> one, making both of these flags #defined from the public RESERVED
> flags, and since it was also only used in domain_conf.c, I unpolluted
> domain_conf.h, putting both #defines in domain_conf.c.
>
> A small change is also made to two functions in bridge_driver.c, to
> prevent a bridge device name and mac address from being automatically
> added to a network definition when it is of one of the new forward
> types (none of which use bridge devices managed by libvirt).
> ---
> include/libvirt/libvirt.h.in | 2 +
> src/conf/domain_conf.c | 276 +++++++++++++++++++++++++++++++++++-
> src/conf/domain_conf.h | 46 ++++++-
> src/conf/network_conf.c | 321 +++++++++++++++++++++++++++++++++++++-----
> src/conf/network_conf.h | 34 ++++-
> src/libvirt_private.syms | 8 +-
> src/network/bridge_driver.c | 28 +++-
> 7 files changed, 657 insertions(+), 58 deletions(-)
I think it would be worth doing a little change in patch split for
this and the previous patch. Instead of splitting between schema
and impl, split between domain & network.
So I think we should have one patch which pulls the common domain.rng
conf schema pieces out, and also modifies domain_conf.h/c at
the same time.
Then a second patch, which pulls the common vport bits into
network.rng and also modifies network_conf.h/.c
Also, each of those patches ought to have at least one new
data file added to their corresponding XML test case at the
same time, so that each patch contains the full self-contained
modifications.
>
> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
> index 8e20f75..b88c96e 100644
> --- a/include/libvirt/libvirt.h.in
> +++ b/include/libvirt/libvirt.h.in
> @@ -1112,6 +1112,8 @@ typedef enum {
> VIR_DOMAIN_XML_SECURE = (1 << 0), /* dump security sensitive information too */
> VIR_DOMAIN_XML_INACTIVE = (1 << 1), /* dump inactive domain information */
> VIR_DOMAIN_XML_UPDATE_CPU = (1 << 2), /* update guest CPU requirements according to host CPU */
> + VIR_DOMAIN_XML_RESERVED1 = (1 << 30), /* reserved for internal used */
> + VIR_DOMAIN_XML_RESERVED2 = (1 << 31), /* reserved for internal used */
> } virDomainXMLFlags;
[snip]
> +/* these flags are only used internally */
> +/* dump internal domain status information */
> +#define VIR_DOMAIN_XML_INTERNAL_STATUS VIR_DOMAIN_XML_RESERVED1
> +/* dump virDomainActualNetDef contents */
> +#define VIR_DOMAIN_XML_ACTUAL_NET VIR_DOMAIN_XML_RESERVED2
Ewww, I really don't like this idea. If we need to pass special
internal only flags to the parser/formating methods, then I think
that should be done as a separate parameter from the public
flags parameter. Either a 'unsigned int internalFlags' or
one or more 'bool someOption' parameters.
> static void
> virDomainObjListDataFree(void *payload, const void *name ATTRIBUTE_UNUSED)
> @@ -714,6 +719,25 @@ void virDomainFSDefFree(virDomainFSDefPtr def)
> VIR_FREE(def);
> }
>
> +void
> +virDomainActualNetDefFree(virDomainActualNetDefPtr def)
> +{
> + 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;
> + }
> +}
> +
> void virDomainNetDefFree(virDomainNetDefPtr def)
> {
> if (!def)
> @@ -736,6 +760,9 @@ void virDomainNetDefFree(virDomainNetDefPtr def)
>
> case VIR_DOMAIN_NET_TYPE_NETWORK:
> VIR_FREE(def->data.network.name);
> + VIR_FREE(def->data.network.portgroup);
> + VIR_FREE(def->data.network.virtPortProfile);
> + virDomainActualNetDefFree(def->data.network.actual);
> break;
>
> case VIR_DOMAIN_NET_TYPE_BRIDGE:
> @@ -2566,6 +2593,85 @@ cleanup:
> goto cleanup;
> }
>
> +static int
> +virDomainActualNetDefParseXML(xmlNodePtr node,
> + xmlXPathContextPtr ctxt,
> + virDomainActualNetDefPtr *actual)
> +{
> + 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) {
> + virDomainReportError(VIR_ERR_INTERNAL_ERROR,
> + _("unknown type '%s' in interface's <actual> element"), type);
> + goto error;
> + }
> + if ((*actual)->type != VIR_DOMAIN_NET_TYPE_BRIDGE &&
> + (*actual)->type != VIR_DOMAIN_NET_TYPE_DIRECT) {
> + virDomainReportError(VIR_ERR_INTERNAL_ERROR,
> + _("unsupported type '%s' in interface's <actual> element"),
> + type);
> + goto error;
> + }
> +
> + if ((*actual)->type == VIR_DOMAIN_NET_TYPE_BRIDGE) {
> +
> + (*actual)->data.bridge.brname = virXPathString("string(./source[1]/@bridge)",
> + ctxt);
> +
> + } else if ((*actual)->type == VIR_DOMAIN_NET_TYPE_DIRECT) {
> + xmlNodePtr virtPortNode;
> + const char *errmsg;
> +
> + (*actual)->data.direct.linkdev = virXPathString("string(./source[1]/@dev)", ctxt);
> +
> + mode = virXPathString("string(./source[1]/@mode)", ctxt);
> + if (mode) {
> + int m;
> + if ((m = virMacvtapModeTypeFromString(mode)) < 0) {
> + virDomainReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unkown mode '%s' in interface <actual> element"),
> + mode);
> + goto error;
> + }
> + (*actual)->data.direct.mode = m;
> + }
> +
> + virtPortNode = virXPathNode("./virtualport", ctxt);
> + if (virtPortNode &&
> + virVirtualPortProfileParamsParseXML(virtPortNode,
> + &(*actual)->data.direct.virtPortProfile,
> + &errmsg) < 0) {
> + if (errmsg)
> + virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s in interface <actual>",
> + errmsg);
> + goto error;
> + }
> + }
> +
> + ret = 0;
> +error:
> + VIR_FREE(type);
> + VIR_FREE(mode);
> +
> + ctxt->node = save_ctxt;
> + return ret;
> +}
>
> /* Parse the XML definition for a network interface
> * @param node XML nodeset to parse for net definition
> @@ -2583,6 +2689,7 @@ virDomainNetDefParseXML(virCapsPtr caps,
> char *macaddr = NULL;
> char *type = NULL;
> char *network = NULL;
> + char *portgroup = NULL;
> char *bridge = NULL;
> char *dev = NULL;
> char *ifname = NULL;
> @@ -2599,6 +2706,7 @@ virDomainNetDefParseXML(virCapsPtr caps,
> char *mode = NULL;
> virNWFilterHashTablePtr filterparams = NULL;
> virVirtualPortProfileParamsPtr virtPort = NULL;
> + virDomainActualNetDefPtr actual = NULL;
> xmlNodePtr oldnode = ctxt->node;
> int ret;
>
> @@ -2630,6 +2738,7 @@ virDomainNetDefParseXML(virCapsPtr caps,
> (def->type == VIR_DOMAIN_NET_TYPE_NETWORK) &&
> (xmlStrEqual(cur->name, BAD_CAST "source"))) {
> network = virXMLPropString(cur, "network");
> + portgroup = virXMLPropString(cur, "portgroup");
> } else if ((internal == NULL) &&
> (def->type == VIR_DOMAIN_NET_TYPE_INTERNAL) &&
> (xmlStrEqual(cur->name, BAD_CAST "source"))) {
> @@ -2645,7 +2754,8 @@ virDomainNetDefParseXML(virCapsPtr caps,
> dev = virXMLPropString(cur, "dev");
> mode = virXMLPropString(cur, "mode");
> } else if ((virtPort == NULL) &&
> - (def->type == VIR_DOMAIN_NET_TYPE_DIRECT) &&
> + ((def->type == VIR_DOMAIN_NET_TYPE_DIRECT) ||
> + (def->type == VIR_DOMAIN_NET_TYPE_NETWORK)) &&
> xmlStrEqual(cur->name, BAD_CAST "virtualport")) {
> const char *errmsg;
> if (virVirtualPortProfileParamsParseXML(cur, &virtPort, &errmsg) < 0) {
> @@ -2697,6 +2807,12 @@ virDomainNetDefParseXML(virCapsPtr caps,
> if (virDomainDeviceBootParseXML(cur, &def->bootIndex,
> bootMap))
> goto error;
> + } else if ((actual == NULL) &&
> + (flags & VIR_DOMAIN_XML_ACTUAL_NET) &&
> + (def->type == VIR_DOMAIN_NET_TYPE_NETWORK) &&
> + xmlStrEqual(cur->name, BAD_CAST "actual")) {
> + if (virDomainActualNetDefParseXML(cur, ctxt, &actual) < 0)
> + goto error;
> }
> }
> cur = cur->next;
> @@ -2745,6 +2861,12 @@ virDomainNetDefParseXML(virCapsPtr caps,
> }
> def->data.network.name = network;
> network = NULL;
> + def->data.network.portgroup = portgroup;
> + portgroup = NULL;
> + def->data.network.virtPortProfile = virtPort;
> + virtPort = NULL;
> + def->data.network.actual = actual;
> + actual = NULL;
> break;
>
> case VIR_DOMAIN_NET_TYPE_ETHERNET:
> @@ -2836,10 +2958,8 @@ virDomainNetDefParseXML(virCapsPtr caps,
> } else
> def->data.direct.mode = VIR_MACVTAP_MODE_VEPA;
>
> - if (virtPort) {
> - def->data.direct.virtPortProfile = virtPort;
> - virtPort = NULL;
> - }
> + def->data.direct.virtPortProfile = virtPort;
> + virtPort = NULL;
>
> def->data.direct.linkdev = dev;
> dev = NULL;
> @@ -2943,11 +3063,13 @@ cleanup:
> ctxt->node = oldnode;
> VIR_FREE(macaddr);
> VIR_FREE(network);
> + VIR_FREE(portgroup);
> VIR_FREE(address);
> VIR_FREE(port);
> VIR_FREE(ifname);
> VIR_FREE(dev);
> VIR_FREE(virtPort);
> + virDomainActualNetDefFree(actual);
> VIR_FREE(script);
> VIR_FREE(bridge);
> VIR_FREE(model);
> @@ -8421,6 +8543,67 @@ virDomainFSDefFormat(virBufferPtr buf,
> }
>
> static int
> +virDomainActualNetDefFormat(virBufferPtr buf,
> + virDomainActualNetDefPtr def,
> + int flags) {
> + int ret = -1;
> + const char *type;
> + const char *mode;
> +
> + if (!(def && (flags & VIR_DOMAIN_XML_INTERNAL_STATUS)))
> + return 0;
> +
> + type = virDomainNetTypeToString(def->type);
> + if (!type) {
> + virDomainReportError(VIR_ERR_INTERNAL_ERROR,
> + _("unexpected net type %d"), def->type);
> + return ret;
> + }
> +
> + if (def->type != VIR_DOMAIN_NET_TYPE_BRIDGE &&
> + def->type != VIR_DOMAIN_NET_TYPE_DIRECT) {
> + virDomainReportError(VIR_ERR_INTERNAL_ERROR,
> + _("unexpected net type %s"), type);
> + goto error;
> + }
> + virBufferAsprintf(buf, " <actual type='%s'>\n", type);
> +
> + switch (def->type) {
> + case VIR_DOMAIN_NET_TYPE_BRIDGE:
> + if (def->data.bridge.brname) {
> + virBufferEscapeString(buf, " <source bridge='%s'/>\n",
> + def->data.bridge.brname);
> + }
> + break;
> +
> + case VIR_DOMAIN_NET_TYPE_DIRECT:
> + virBufferAddLit(buf, " <source");
> + if (def->data.direct.linkdev)
> + virBufferEscapeString(buf, " dev='%s'",
> + def->data.direct.linkdev);
> +
> + mode = virMacvtapModeTypeToString(def->data.direct.mode);
> + if (!mode) {
> + virDomainReportError(VIR_ERR_INTERNAL_ERROR,
> + _("unexpected source mode %d"),
> + def->data.direct.mode);
> + return ret;
> + }
> + virBufferAsprintf(buf, " mode='%s'/>\n", mode);
> + virVirtualPortProfileFormat(buf, def->data.direct.virtPortProfile,
> + " ");
> + break;
> + default:
> + break;
> + }
> + virBufferAddLit(buf, " </actual>\n");
> +
> + ret = 0;
> +error:
> + return ret;
> +}
> +
> +static int
> virDomainNetDefFormat(virBufferPtr buf,
> virDomainNetDefPtr def,
> int flags)
> @@ -8443,8 +8626,17 @@ virDomainNetDefFormat(virBufferPtr buf,
>
> switch (def->type) {
> case VIR_DOMAIN_NET_TYPE_NETWORK:
> - virBufferEscapeString(buf, " <source network='%s'/>\n",
> + virBufferEscapeString(buf, " <source network='%s'",
> def->data.network.name);
> + if (def->data.network.portgroup) {
> + virBufferEscapeString(buf, " portgroup='%s'",
> + def->data.network.portgroup);
> + }
> + virBufferAddLit(buf, "/>\n");
> + virVirtualPortProfileFormat(buf, def->data.network.virtPortProfile,
> + " ");
> + if (virDomainActualNetDefFormat(buf, def->data.network.actual, flags) < 0)
> + return -1;
> break;
This makes the XML formatting a little more verbose than we normally
aim for, in the common case where no portgrp/profile exists. eg we
get an empty
<source network='defualt'>
</source>
> + virtPortNode = virXPathNode("./virtualport", ctxt);
> + if (virtPortNode) {
> + const char *errmsg;
> + if (virVirtualPortProfileParamsParseXML(virtPortNode,
> + &def->virtPortProfile,
> + &errmsg) < 0) {
> + if (errmsg)
> + virNetworkReportError(VIR_ERR_XML_ERROR, "%s", errmsg);
> + goto error;
> + }
> + }
Any reason why we don't just make virVirtualPortProfileParamsParseXML
responsible for raising the error? Passing back the error message as
a string is rather unusual for our code.
> + if (nPortGroups > 0) {
> + int ii;
Is the more common name 'i' not available ?
> +
> + /* allocate array to hold all the portgroups */
> + if (VIR_ALLOC_N(def->portGroups, nPortGroups) < 0) {
> + virReportOOMError();
> + goto error;
> + }
> + /* parse each portgroup */
> + for (ii = 0; ii < nPortGroups; ii++) {
> + int ret = virNetworkPortGroupParseXML(def->name,
> + &def->portGroups[ii],
> + portGroupNodes[ii], ctxt);
> + if (ret < 0)
> + goto error;
> + def->nPortGroups++;
> + }
> + }
> + VIR_FREE(portGroupNodes);
> +
> + def->forwardDev = virXPathString("string(./@dev)", ctxt);
>
> - def->forwardDev = virXPathString("string(./forward[1]/@dev)", ctxt);
> - } else {
> - def->forwardType = VIR_NETWORK_FORWARD_NONE;
> - }
> + switch (def->forwardType) {
> + case VIR_NETWORK_FORWARD_ROUTE:
> + case VIR_NETWORK_FORWARD_NAT:
> + /* It's pointless to specify L3 forwarding without specifying
> + * the network we're on.
> + */
> + if (def->nips == 0) {
> + virNetworkReportError(VIR_ERR_XML_ERROR,
> + _("%s forwarding requested, but no IP address provided for network '%s'"),
> + virNetworkForwardTypeToString(def->forwardType),
> + def->name);
> + goto error;
> + }
> + def->stp = (stp && STREQ(stp, "off")) ? 0 : 1;
> + break;
> + case VIR_NETWORK_FORWARD_PRIVATE:
> + case VIR_NETWORK_FORWARD_VEPA:
> + case VIR_NETWORK_FORWARD_PASSTHROUGH:
> + if (def->bridge) {
> + virNetworkReportError(VIR_ERR_XML_ERROR,
> + _("bridge name not allowed in %s mode (network '%s'"),
> + virNetworkForwardTypeToString(def->forwardType),
> + def->name);
> + goto error;
> + }
> + /* Fall through to next case */
> + case VIR_NETWORK_FORWARD_BRIDGE:
> + if (def->delay || stp) {
> + virNetworkReportError(VIR_ERR_XML_ERROR,
> + _("bridge delay/stp options only allowed in route, nat, and isolated mode, not in %s (network '%s')"),
> + virNetworkForwardTypeToString(def->forwardType),
> + def->name);
> + goto error;
> + }
> + /* all of these modes can use a pool of physical interfaces */
> + nForwardIfs = virXPathNodeSet("./interface", ctxt, &forwardIfNodes);
> + if (nForwardIfs < 0)
> + goto error;
> +
> + if (nForwardIfs > 0) {
> + int ii;
>
> + /* allocate array to hold all the portgroups */
> + if (VIR_ALLOC_N(def->forwardIfs, nForwardIfs) < 0) {
> + virReportOOMError();
> + goto error;
> + }
> +
> + /* parse each forwardIf */
> + for (ii = 0; ii < nForwardIfs; ii++) {
> + def->forwardIfs[ii].usageCount = 0;
> + def->forwardIfs[ii].dev = virXMLPropString(forwardIfNodes[ii],
> + "dev");
> + if (!def->forwardIfs[ii].dev) {
> + virNetworkReportError(VIR_ERR_XML_ERROR,
> + _("Missing required dev attribute in network '%s' forward interface"),
> + def->name);
> + goto error;
> + }
> + def->nForwardIfs++;
> + }
> + }
> + VIR_FREE(forwardIfNodes);
> + break;
> + }
> + }
The handling of the interface device names does not seem right to me.
The following should be identical:
<forward mode='nat' dev='eth0'/>
<forward mode='nat'>
<interface dev='eth0'/>
</forward>
<forward mode='nat' dev='eth0'>
<interface dev='eth0'/>
</forward>
And so should
<forward mode='vepa' dev='eth0'/>
<forward mode='vepa'>
<interface dev='eth0'/>
</forward>
<forward mode='vepa' dev='eth0'>
<interface dev='eth0'/>
</forward>
<forward mode='vepa' dev='eth0'>
<interface dev='eth0'/>
<interface dev='eth1'/>
</forward>
The following should be illegal
<forward mode='vepa' dev='eth0'>
<interface dev='eth2'/>
</forward>
<forward mode='vepa' dev='eth0'>
<interface dev='eth2'/>
<interface dev='eth0'/>
</forward>
ie, @dev must be identical to /interface[0]/@dev if
present, and both syntaxes should be allowed regardless
of the 'mode' attribute value.
> typedef struct _virNetworkDef virNetworkDef;
> typedef virNetworkDef *virNetworkDefPtr;
> struct _virNetworkDef {
> @@ -121,12 +139,22 @@ struct _virNetworkDef {
> bool mac_specified;
>
> int forwardType; /* One of virNetworkForwardType constants */
> - char *forwardDev; /* Destination device for forwarding */
> + char *forwardDev; /* Destination device for forwarding (if just one) */
> +
> + /* If there are multiple forward devices (i.e. a pool of
> + * interfaces), they will be listed here.
> + */
> + size_t nForwardIfs;
> + virNetworkForwardIfDefPtr forwardIfs;
Keeping 'forwardDev' is wrong here. We should only end up with
the array of interfaces, and forwardDev should be folded into
that at parse time, and pulled back out at format time.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
More information about the libvir-list
mailing list