12:36 a.m.
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.