[libvirt] [PATCH 07/10] network: new XML to support virtual switch functionality
Laine Stump
laine at laine.org
Wed Jul 20 05:36:52 UTC 2011
On 07/08/2011 09:17 AM, Daniel P. Berrange wrote:
> 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.
>
Okay, I'm redid it that way. (Actually, I made two separate
"pre-patches", one that makes the virtportprofile stuff common, and
another that changes the virtportprofile in the domain from being
contained in the object to being pointed to by the object. *Then* I made
a patch to add the new stuff to domain xml from top to bottom, and
finally one that adds the new stuff to the network xml.)
>> 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.
After a lot of back and forth on this, what was decided on was to leave
VIR_DOMAIN_XML_INTERNAL_STATUS in the regular flags (and also put
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 yesterday.
>> +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>
I think you've misread the code. The portgroup is part of <source>, but
it's an attribute and is on the same line. the virtportprofile is a
separate element, and is at the top level, not within <source>. So
<source network='....." will always end on the same line, just as it did
in the past. As proof, I can say that "make check" passes :-)
>
>> + 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.
I wanted to call virNetworkReportError() rather than virSocketError(),
and give a bit more context about where this virtportprofile was located.
Since you and Eric both disliked this, I changed it to report the error
directly in virVirtualPortProfilePramsParseXML
>> + if (nPortGroups> 0) {
>> + int ii;
> Is the more common name 'i' not available ?
I prefer ii because it's easier to search for - if you search for "i" by
itself, you'll get a lot of false hits, but ii hardly ever occurs
naturally. (I've actually been using this name for array indexes for a
long time; I'm surprised you hadn't noticed it in earlier patches).
> 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.
Okay. I had understood the first item, but was a bit lazy in my code. I
hadn't figured on the second (since nat/route mode can only use a single
interface). I've changed it to work as you suggest, and to remove
forwardDev from the struct.
>> 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.
Done.
More information about the libvir-list
mailing list