On 10/15/2012 10:54 AM, Michal Privoznik wrote:
> On 15.10.2012 12:27, Benjamin Cama wrote:
>> It allows to specify forwarding only for IPv4 or IPv6. Not specifying it
>> enables forwarding for both protocols.
Okay, I can understand the usefulness of this option (although I recall
that when I added IPv6 support, we discussed whether or not to have a
separate forward mode for ipv4 and ipv6 on the same network, and decided
against it, because it created unnecessary complexity).
See below though - I'm thinking it might make more sense to add an
attribute to each <ip> element rather than to the <forward> element.
Perhaps something like:
<ip family='ipv6' forward='none' address='fdd6:4978:2ac:5::1' prefix='64'>
<ip family='ipv4' forward='nat' address='192.168.123.1'
netmask='255.255.255.0'>
etc. This setting would override whatever was set in the <forward>
element for that particular IP address.
>> ---
>> src/conf/network_conf.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++-
>> src/conf/network_conf.h | 1 +
>> 2 files changed, 58 insertions(+), 1 deletions(-)
> When you introduce new feature to XML schema, you must update
> docs/schemas/*.rng and docs/format*.html.in
>
>> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
>> index 891d48c..8c2ae9b 100644
>> --- a/src/conf/network_conf.c
>> +++ b/src/conf/network_conf.c
>> @@ -1204,6 +1204,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
>> xmlNodePtr virtPortNode = NULL;
>> xmlNodePtr forwardNode = NULL;
>> int nIps, nPortGroups, nForwardIfs, nForwardPfs, nForwardAddrs;
>> + char *forwardFamily = NULL;
>> char *forwardDev = NULL;
>> char *forwardManaged = NULL;
>> char *type = NULL;
>> @@ -1358,6 +1359,23 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
>> def->forwardType = VIR_NETWORK_FORWARD_NAT;
>> }
>>
>> + forwardFamily = virXPathString("string(./@family)", ctxt);
>> + if (forwardFamily) {
>> + if (STREQ(forwardFamily, "ipv4")) {
>> + def->forwardFamily = AF_INET;
>> + } else if (STREQ(forwardFamily, "ipv6")) {
>> + def->forwardFamily = AF_INET6;
>> + } else {
>> + virNetworkReportError(VIR_ERR_XML_ERROR,
>> + _("Unknown forward family '%s'"), forwardFamily);
> %s/virNetworkReportError/virReportError/
>
>> + VIR_FREE(forwardFamily);
>> + goto error;
>> + }
>> + VIR_FREE(forwardFamily);
>> + } else {
>> + def->forwardFamily = AF_UNSPEC;
>> + }
>> +
> Usually, we create an enum for this and use vir.*TypeFromString() and
> vir.*TypeToString();
>
>> forwardDev = virXPathString("string(./@dev)", ctxt);
>> forwardManaged = virXPathString("string(./@managed)", ctxt);
>> if(forwardManaged != NULL) {
>> @@ -1515,8 +1533,16 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
>> VIR_FREE(forwardIfNodes);
>> VIR_FREE(forwardAddrNodes);
>> switch (def->forwardType) {
>> - case VIR_NETWORK_FORWARD_ROUTE:
>> case VIR_NETWORK_FORWARD_NAT:
>> + if (def->forwardFamily == AF_INET6) {
>> + virNetworkReportError(VIR_ERR_XML_ERROR,
>> + _("%s forwarding is not allowed in IPv6 (network '%s')"),
>> + virNetworkForwardTypeToString(def->forwardType),
>> + def->name);
>> + goto error;
>> + }
Okay. It took me a minute to parse this, but what you're saying is "if
the forward type is 'nat', this implies doing NAT forwarding of IPv4
packets, so it doesn't make sense to say that only IPv6 forwarding is
allowed". Note that in the future that may not remain the case,
depending on what, if anything, libvirt does with IPv6 NAT (this is as
good a place as any to start reading: http://lwn.net/Articles/452293/)
Which actually brings up another topic - what do we do with combined
ipv4/6 networks that have forward mode='nat' if/when we do support some
type of IPv6 NAT? Since there will already be a lot of deployments in
the field that expect the current behavior, we really can't just
suddenly change the meaning of <forward mode='nat'> "NAT ipv4 + route
ipv6" to "NAT ipv4 + NAT ipv6" - that would break too many existing
installations.
We may want to think about the implications of the above before we
commit to any particular attribute scheme for limiting the forwarding to
one type or another.
As a matter of fact, I just thought of something else - what about a
network with multiple IP addresses where we only want to allow
forwarding of traffic with addresses on one of those IP networks?
Perhaps this should be an attribute of the <ip> element rather than of
the <forward> element. (this thought stuck in my head so much that I
went back and mentioned it at the top of this message).
>> + /* fall through to next case */
>> + case VIR_NETWORK_FORWARD_ROUTE:
>> /* It's pointless to specify L3 forwarding without specifying
>> * the network we're on.
>> */
>> @@ -1527,6 +1553,25 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
>> def->name);
>> goto error;
>> }
>> + /* If forwarding for one family only, an address from this family
>> + * must be present
>> + */
>> + if (def->forwardFamily) {
>> + int ii;
>> + for (ii = 0; ii < def->nips; ii++) {
>> + if (VIR_SOCKET_ADDR_IS_FAMILY(&def->ips[ii].address, def->forwardFamily))
>> + break;
>> + }
>> + if (ii == def->nips) {
> useless 'if'.
Looks pretty useful to me :-) The "break" will break you out of the for
look if a match is found anywhere in the ips list, but if no match is
found, then "ii == def->nips" is true.
>> + char ipVersion = def->forwardFamily == AF_INET6 ? '6' : '4';
>> + virNetworkReportError(VIR_ERR_XML_ERROR,
>> + _("%s IPv%c forwarding requested, but no IPv%c address provided for network '%s'"),
It would be better to make the entire string "IPv6" or "IPv4" an arg,
rather than a single character - I'm thinking it may be easier for the
translators to see what's going on.
>> + virNetworkForwardTypeToString(def->forwardType),
>> + ipVersion, ipVersion,
>> + def->name);
>> + goto error;
>> + }
>> + }
>> if (def->nForwardIfs > 1) {
>> virReportError(VIR_ERR_XML_ERROR,
>> _("multiple forwarding interfaces specified for network '%s', only one is supported"),
>> @@ -1555,6 +1600,13 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
>> def->name);
>> goto error;
>> }
>> + if (def->forwardFamily) {
>> + virNetworkReportError(VIR_ERR_XML_ERROR,
>> + _("bridge forward family option only allowed in route and nat mode, not in %s (network '%s')"),
>> + virNetworkForwardTypeToString(def->forwardType),
>> + def->name);
>> + goto error;
>> + }
>> break;
>> }
>> }
>> @@ -1830,6 +1882,8 @@ char *virNetworkDefFormat(const virNetworkDefPtr def, unsigned int flags)
>> if (!def->nForwardPfs)
>> dev = virNetworkDefForwardIf(def, 0);
>> const char *mode = virNetworkForwardTypeToString(def->forwardType);
>> + const char *family = (def->forwardFamily == AF_INET ? "ipv4" :
>> + def->forwardFamily == AF_INET6 ? "ipv6" : NULL);
>>
>> if (!mode) {
>> virReportError(VIR_ERR_INTERNAL_ERROR,
>> @@ -1846,6 +1900,8 @@ char *virNetworkDefFormat(const virNetworkDefPtr def, unsigned int flags)
>> else
>> virBufferAddLit(&buf, " managed='no'");
>> }
>> + if (family)
>> + virBufferEscapeString(&buf, " family='%s'", family);
> Here is the best place for vir.*TypeToString().
>
>> virBufferAsprintf(&buf, "%s>\n",
>> (def->nForwardIfs || def->nForwardPfs) ? "" : "/");
>> virBufferAdjustIndent(&buf, 2);
>> diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
>> index 55502fb..61ecbc1 100644
>> --- a/src/conf/network_conf.h
>> +++ b/src/conf/network_conf.h
>> @@ -186,6 +186,7 @@ struct _virNetworkDef {
>>
>> int forwardType; /* One of virNetworkForwardType constants */
>> int managed; /* managed attribute for hostdev mode */
>> + int forwardFamily; /* AF_INET or AF_INET6 - AF_UNSPEC for both */
>>
>> /* If there are multiple forward devices (i.e. a pool of
>> * interfaces), they will be listed here.
>>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list