12:57 p.m.
This is a resend of
https://www.redhat.com/archives/libvir-list/2010-December/msg00765.html
incorporating changes due to comments from Eric Blake and Paweł Krześniak.
changes from v1 to v2 are noted in each individual mail
-----
Most of this patchset is setup for patch 09/13, which updates the
network XML parser to support IPv6, and 12/13, which turns on IPv6 in
the bridge driver.
In order to have each patch individually pass make check and
(otherwise function properly in case someone is doing a bisect), there
is a bit of extra code churn (lines that are changed in one patch,
only to be changed again in a later patch); I tried to minimize this
as much as possible.
For for IPv6 to work correctly, target *and build* systems will
now need to have ip6tables and radvd available. The way I added
ip6tables into autoconfigure.ac is identical to existing iptables, and
the way I added radvd is identical to dnsmasq. Unfortunately this
doesn't communicate the requirement downstream in a programmatic
fashion, so I guess we need to make sure that this is adequately
reported in the next set of release notes.
12:58 p.m.
New subject: [libvirt] [PATCHv2 01/13] New virSocketAddr utility functions
virSocketPrefixToNetmask: Given a 'prefix', which is the number of 1
bits in a netmask, fill in a virSocketAddr object with a netmask as an
IP address (IPv6 or IPv4).
virSocketAddrMask: Mask off the host bits in one virSocketAddr
according to the netmask in another virSocketAddr.
virSocketAddrMaskByPrefix, Mask off the host bits in a virSocketAddr
according to a prefix (number of 1 bits in netmask).
VIR_SOCKET_FAMILY: return the family of a virSocketAddr
---
V2 changes:
* Simplify virSocketAddrMaskByPrefix by calling virSocketAddrMask instead
of duplicating code.
* do bitwise operations arithmetically rather than in loops
* prefix is now unsigned when it is a function arg.
src/libvirt_private.syms | 3 +
src/util/network.c | 114 ++++++++++++++++++++++++++++++++++++++++++++++
src/util/network.h | 10 ++++
3 files changed, 127 insertions(+), 0 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 0e3033d..19841ef 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -559,6 +559,9 @@ virShrinkN;
# network.h
virSocketAddrIsNetmask;
+virSocketAddrMask;
+virSocketAddrMaskByPrefix;
+virSocketAddrPrefixToNetmask;
virSocketCheckNetmask;
virSocketFormatAddr;
virSocketFormatAddrFull;
diff --git a/src/util/network.c b/src/util/network.c
index 1abe78b..12b56ea 100644
--- a/src/util/network.c
+++ b/src/util/network.c
@@ -288,6 +288,59 @@ int virSocketAddrIsNetmask(virSocketAddrPtr netmask) {
}
/**
+ * virSocketAddrMask:
+ * @addr: address that needs to be masked
+ * @netmask: the netmask address
+ *
+ * Mask off the host bits of @addr according to @netmask, turning it
+ * into a network address.
+ *
+ * Returns 0 in case of success, or -1 on error.
+ */
+int
+virSocketAddrMask(virSocketAddrPtr addr, const virSocketAddrPtr netmask)
+{
+ if (addr->data.stor.ss_family != netmask->data.stor.ss_family)
+ return -1;
+
+ if (addr->data.stor.ss_family == AF_INET) {
+ addr->data.inet4.sin_addr.s_addr
+ &= netmask->data.inet4.sin_addr.s_addr;
+ return 0;
+ }
+ if (addr->data.stor.ss_family == AF_INET6) {
+ int ii;
+ for (ii = 0; ii < 16; ii++)
+ addr->data.inet6.sin6_addr.s6_addr[ii]
+ &= netmask->data.inet6.sin6_addr.s6_addr[ii];
+ return 0;
+ }
+ return -1;
+}
+
+/**
+ * virSocketAddrMaskByPrefix:
+ * @addr: address that needs to be masked
+ * @prefix: prefix (# of 1 bits) of netmask to apply
+ *
+ * Mask off the host bits of @addr according to @prefix, turning it
+ * into a network address.
+ *
+ * Returns 0 in case of success, or -1 on error.
+ */
+int
+virSocketAddrMaskByPrefix(virSocketAddrPtr addr, unsigned int prefix)
+{
+ virSocketAddr netmask;
+
+ if (virSocketAddrPrefixToNetmask(prefix, &netmask,
+ addr->data.stor.ss_family) < 0)
+ return -1;
+
+ return virSocketAddrMask(addr, &netmask);
+}
+
+/**
* virSocketCheckNetmask:
* @addr1: a first network address
* @addr2: a second network address
@@ -486,3 +539,64 @@ int virSocketGetNumNetmaskBits(const virSocketAddrPtr netmask)
}
return -1;
}
+
+/**
+ * virSocketPrefixToNetmask:
+ * @prefix: number of 1 bits to put in the netmask
+ * @netmask: address to fill in with the desired netmask
+ * @family: family of the address (AF_INET or AF_INET6 only)
+ *
+ * given @prefix and @family, fill in @netmask with a netmask
+ * (eg 255.255.255.0).
+ *
+ * Returns 0 on success or -1 on error.
+ */
+
+int
+virSocketAddrPrefixToNetmask(unsigned int prefix,
+ virSocketAddrPtr netmask,
+ int family)
+{
+ int result = -1;
+
+ netmask->data.stor.ss_family = AF_UNSPEC; /* assume failure */
+
+ if (family == AF_INET) {
+ int ip;
+
+ if (prefix > 32)
+ goto error;
+
+ ip = prefix ? ~((1 << (32 - prefix)) - 1) : 0;
+ netmask->data.inet4.sin_addr.s_addr = htonl(ip);
+ netmask->data.stor.ss_family = AF_INET;
+ result = 0;
+
+ } else if (family == AF_INET6) {
+ int ii = 0;
+
+ if (prefix > 128)
+ goto error;
+
+ while (prefix >= 8) {
+ /* do as much as possible an entire byte at a time */
+ netmask->data.inet6.sin6_addr.s6_addr[ii++] = 0xff;
+ prefix -= 8;
+ }
+ if (prefix > 0) {
+ /* final partial byte */
+ netmask->data.inet6.sin6_addr.s6_addr[ii++]
+ = ~((1 << (8 - prefix)) -1);
+ }
+ ii++;
+ while (ii < 16) {
+ /* zerofill remainder in case it wasn't initialized */
+ netmask->data.inet6.sin6_addr.s6_addr[ii++] = 0;
+ }
+ netmask->data.stor.ss_family = AF_INET6;
+ result = 0;
+ }
+
+error:
+ return result;
+}
diff --git a/src/util/network.h b/src/util/network.h
index 521f466..2fcee43 100644
--- a/src/util/network.h
+++ b/src/util/network.h
@@ -41,6 +41,9 @@ typedef struct {
# define VIR_SOCKET_IS_FAMILY(s, f) \
((s)->data.sa.sa_family == f)
+# define VIR_SOCKET_FAMILY(s) \
+ ((s)->data.sa.sa_family)
+
typedef virSocketAddr *virSocketAddrPtr;
int virSocketParseAddr (const char *val,
@@ -70,7 +73,14 @@ int virSocketAddrIsNetmask(virSocketAddrPtr netmask);
int virSocketCheckNetmask (virSocketAddrPtr addr1,
virSocketAddrPtr addr2,
virSocketAddrPtr netmask);
+int virSocketAddrMask (virSocketAddrPtr addr,
+ const virSocketAddrPtr netmask);
+int virSocketAddrMaskByPrefix(virSocketAddrPtr addr,
+ unsigned int prefix);
int virSocketGetNumNetmaskBits(const virSocketAddrPtr netmask);
+int virSocketAddrPrefixToNetmask(unsigned int prefix,
+ virSocketAddrPtr netmask,
+ int family);
#endif /* __VIR_NETWORK_H__ */
--
1.7.3.4
4:58 p.m.
New subject: [libvirt] [PATCHv2 01/13] New virSocketAddr utility functions
On 12/22/2010 11:58 AM, Laine Stump wrote:
virSocketPrefixToNetmask: Given a 'prefix', which is the
number of 1
bits in a netmask, fill in a virSocketAddr object with a netmask as an
IP address (IPv6 or IPv4).
virSocketAddrMask: Mask off the host bits in one virSocketAddr
according to the netmask in another virSocketAddr.
virSocketAddrMaskByPrefix, Mask off the host bits in a virSocketAddr
according to a prefix (number of 1 bits in netmask).
VIR_SOCKET_FAMILY: return the family of a virSocketAddr
ACK after you fix one bug:
+ } else if (family == AF_INET6) {
+ int ii = 0;
+
+ if (prefix > 128)
+ goto error;
+
+ while (prefix >= 8) {
+ /* do as much as possible an entire byte at a time */
+ netmask->data.inet6.sin6_addr.s6_addr[ii++] = 0xff;
+ prefix -= 8;
+ }
+ if (prefix > 0) {
+ /* final partial byte */
+ netmask->data.inet6.sin6_addr.s6_addr[ii++]
+ = ~((1 << (8 - prefix)) -1);
+ }
+ ii++;
Delete this line. You already incremented it if prefix was nonzero in
the lines above, and don't want to increment it if prefix is a multiple
of 8, so that you don't skip a byte.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
12:58 p.m.
New subject: [libvirt] [PATCHv2 02/13] New virNetworkDef utility functions
Later patches will add the possibility to define a network's netmask
as a prefix (0-32, or 0-128 in the case of IPv6). To make it easier to
deal with definition of both kinds (prefix or netmask), add two new
functions:
virNetworkDefNetmask: return a copy of the netmask into a
virSocketAddr. If no netmask was specified in the XML, create a
default netmask based on the network class of the virNetworkDef's IP
address.
virNetworkDefPrefix: return the netmask as numeric prefix (or the
default prefix for the network class of the virNetworkDef's IP
address, if no netmask was specified in the XML)
---
V2 Changes:
* expose the nitty-gritty of detecting class A-C networks, rather
than using IN_CLASSx() macros (which aren't guaranteed to be on
all platforms)
src/conf/network_conf.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
src/conf/network_conf.h | 3 +++
src/libvirt_private.syms | 2 ++
3 files changed, 51 insertions(+), 0 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index b469269..427b338 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -207,6 +207,52 @@ void virNetworkRemoveInactive(virNetworkObjListPtr nets,
}
}
+/* return number of 1 bits in netmask for the network's ipAddress,
+ * or -1 on error
+ */
+int virNetworkDefPrefix(const virNetworkDefPtr def)
+{
+ if (VIR_SOCKET_HAS_ADDR(&def->netmask)) {
+ return virSocketGetNumNetmaskBits(&def->netmask);
+ } else if (VIR_SOCKET_IS_FAMILY(&def->ipAddress, AF_INET)) {
+ /* Return the natural prefix for the network's ip address.
+ * On Linux we could use the IN_CLASSx() macros, but those
+ * aren't guaranteed on all platforms, so we just deal with
+ * the bits ourselves.
+ */
+ const unsigned char *octets
+ = (const unsigned char
*)(&def->ipAddress.data.inet4.sin_addr.s_addr);
+ if ((octets[0] & 0x80) == 0) {
+ /* Class A network */
+ return 8;
+ } else if ((octets[0] & 0xC0) == 0x80) {
+ /* Class B network */
+ return 16;
+ } else if ((octets[0] & 0xE0) == 0xC0) {
+ /* Class C network */
+ return 24;
+ }
+ return -1;
+ }
+ return -1;
+}
+
+/* Fill in a virSocketAddr with the proper netmask for this
+ * definition, based on either the definition's netmask, or its
+ * prefix. Return -1 on error (and set the netmask family to AF_UNSPEC)
+ */
+int virNetworkDefNetmask(const virNetworkDefPtr def,
+ virSocketAddrPtr netmask)
+{
+ if (VIR_SOCKET_IS_FAMILY(&def->netmask, AF_INET)) {
+ *netmask = def->netmask;
+ return 0;
+ }
+
+ return virSocketAddrPrefixToNetmask(virNetworkDefPrefix(def), netmask,
+ VIR_SOCKET_FAMILY(&def->ipAddress));
+}
+
static int
virNetworkDHCPRangeDefParseXML(virNetworkDefPtr def,
diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
index 7d31693..a922d28 100644
--- a/src/conf/network_conf.h
+++ b/src/conf/network_conf.h
@@ -133,6 +133,9 @@ virNetworkDefPtr virNetworkDefParseNode(xmlDocPtr xml,
char *virNetworkDefFormat(const virNetworkDefPtr def);
+int virNetworkDefPrefix(const virNetworkDefPtr def);
+int virNetworkDefNetmask(const virNetworkDefPtr def,
+ virSocketAddrPtr netmask);
int virNetworkSaveXML(const char *configDir,
virNetworkDefPtr def,
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 19841ef..d9ba7b1 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -578,9 +578,11 @@ virNetworkAssignDef;
virNetworkConfigFile;
virNetworkDefFormat;
virNetworkDefFree;
+virNetworkDefNetmask;
virNetworkDefParseFile;
virNetworkDefParseNode;
virNetworkDefParseString;
+virNetworkDefPrefix;
virNetworkDeleteConfig;
virNetworkFindByName;
virNetworkFindByUUID;
--
1.7.3.4
5:10 p.m.
New subject: [libvirt] [PATCHv2 02/13] New virNetworkDef utility functions
On 12/22/2010 11:58 AM, Laine Stump wrote:
Later patches will add the possibility to define a network's
netmask
as a prefix (0-32, or 0-128 in the case of IPv6). To make it easier to
deal with definition of both kinds (prefix or netmask), add two new
functions:
virNetworkDefNetmask: return a copy of the netmask into a
virSocketAddr. If no netmask was specified in the XML, create a
default netmask based on the network class of the virNetworkDef's IP
address.
virNetworkDefPrefix: return the netmask as numeric prefix (or the
default prefix for the network class of the virNetworkDef's IP
address, if no netmask was specified in the XML)
+int virNetworkDefPrefix(const virNetworkDefPtr def)
+{
+ if (VIR_SOCKET_HAS_ADDR(&def->netmask)) {
+ return virSocketGetNumNetmaskBits(&def->netmask);
+ } else if (VIR_SOCKET_IS_FAMILY(&def->ipAddress, AF_INET)) {
+ /* Return the natural prefix for the network's ip address.
+ * On Linux we could use the IN_CLASSx() macros, but those
+ * aren't guaranteed on all platforms, so we just deal with
+ * the bits ourselves.
+ */
+ const unsigned char *octets
+ = (const unsigned char
*)(&def->ipAddress.data.inet4.sin_addr.s_addr);
Is this type-punning guaranteed to work on both big- and little-endian
systems? Or are you better off doing:
unsigned char octet
= ntohl(def->ipAddress.data.inet4.sin_addr.s_addr) >> 24;
and use octet instead of octets[0]?
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
12:58 p.m.
New subject: [libvirt] [PATCHv2 03/13] Fix logging of failed iptables commands
The functions in iptables.c all return -1 on failure, but all their
callers (which all happen to be in bridge_driver.c) assume that they
are returning an errno, and the logging is done accordingly. This
patch fixes all the error checking and logging to assume < 0 is an
error, and nothing else.
---
No Changes from V1.
src/network/bridge_driver.c | 183 +++++++++++++++++++++----------------------
1 files changed, 91 insertions(+), 92 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index b0834ae..81a58a9 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -585,28 +585,28 @@ cleanup:
static int
networkAddMasqueradingIptablesRules(struct network_driver *driver,
virNetworkObjPtr network) {
- int err;
+
/* allow forwarding packets from the bridge interface */
- if ((err = iptablesAddForwardAllowOut(driver->iptables,
- &network->def->ipAddress,
- &network->def->netmask,
- network->def->bridge,
- network->def->forwardDev))) {
- virReportSystemError(err,
- _("failed to add iptables rule to allow forwarding from
'%s'"),
- network->def->bridge);
+ if (iptablesAddForwardAllowOut(driver->iptables,
+ &network->def->ipAddress,
+ &network->def->netmask,
+ network->def->bridge,
+ network->def->forwardDev) < 0) {
+ networkReportError(VIR_ERR_SYSTEM_ERROR,
+ _("failed to add iptables rule to allow forwarding from
'%s'"),
+ network->def->bridge);
goto masqerr1;
}
/* allow forwarding packets to the bridge interface if they are part of an existing
connection */
- if ((err = iptablesAddForwardAllowRelatedIn(driver->iptables,
- &network->def->ipAddress,
- &network->def->netmask,
- network->def->bridge,
- network->def->forwardDev))) {
- virReportSystemError(err,
- _("failed to add iptables rule to allow forwarding to
'%s'"),
- network->def->bridge);
+ if (iptablesAddForwardAllowRelatedIn(driver->iptables,
+ &network->def->ipAddress,
+ &network->def->netmask,
+ network->def->bridge,
+ network->def->forwardDev) < 0) {
+ networkReportError(VIR_ERR_SYSTEM_ERROR,
+ _("failed to add iptables rule to allow forwarding to
'%s'"),
+ network->def->bridge);
goto masqerr2;
}
@@ -634,38 +634,38 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver,
*/
/* First the generic masquerade rule for other protocols */
- if ((err = iptablesAddForwardMasquerade(driver->iptables,
- &network->def->ipAddress,
- &network->def->netmask,
- network->def->forwardDev,
- NULL))) {
- virReportSystemError(err,
- _("failed to add iptables rule to enable masquerading
to '%s'"),
- network->def->forwardDev ?
network->def->forwardDev : NULL);
+ if (iptablesAddForwardMasquerade(driver->iptables,
+ &network->def->ipAddress,
+ &network->def->netmask,
+ network->def->forwardDev,
+ NULL) < 0) {
+ networkReportError(VIR_ERR_SYSTEM_ERROR,
+ _("failed to add iptables rule to enable masquerading to
'%s'"),
+ network->def->forwardDev ?
network->def->forwardDev : NULL);
goto masqerr3;
}
/* UDP with a source port restriction */
- if ((err = iptablesAddForwardMasquerade(driver->iptables,
- &network->def->ipAddress,
- &network->def->netmask,
- network->def->forwardDev,
- "udp"))) {
- virReportSystemError(err,
- _("failed to add iptables rule to enable UDP
masquerading to '%s'"),
- network->def->forwardDev ?
network->def->forwardDev : NULL);
+ if (iptablesAddForwardMasquerade(driver->iptables,
+ &network->def->ipAddress,
+ &network->def->netmask,
+ network->def->forwardDev,
+ "udp") < 0) {
+ networkReportError(VIR_ERR_SYSTEM_ERROR,
+ _("failed to add iptables rule to enable UDP masquerading
to '%s'"),
+ network->def->forwardDev ?
network->def->forwardDev : NULL);
goto masqerr4;
}
/* TCP with a source port restriction */
- if ((err = iptablesAddForwardMasquerade(driver->iptables,
- &network->def->ipAddress,
- &network->def->netmask,
- network->def->forwardDev,
- "tcp"))) {
- virReportSystemError(err,
- _("failed to add iptables rule to enable TCP
masquerading to '%s'"),
- network->def->forwardDev ?
network->def->forwardDev : NULL);
+ if (iptablesAddForwardMasquerade(driver->iptables,
+ &network->def->ipAddress,
+ &network->def->netmask,
+ network->def->forwardDev,
+ "tcp") < 0) {
+ networkReportError(VIR_ERR_SYSTEM_ERROR,
+ _("failed to add iptables rule to enable TCP masquerading
to '%s'"),
+ network->def->forwardDev ?
network->def->forwardDev : NULL);
goto masqerr5;
}
@@ -702,28 +702,28 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver,
static int
networkAddRoutingIptablesRules(struct network_driver *driver,
virNetworkObjPtr network) {
- int err;
+
/* allow routing packets from the bridge interface */
- if ((err = iptablesAddForwardAllowOut(driver->iptables,
- &network->def->ipAddress,
- &network->def->netmask,
- network->def->bridge,
- network->def->forwardDev))) {
- virReportSystemError(err,
- _("failed to add iptables rule to allow routing from
'%s'"),
- network->def->bridge);
+ if (iptablesAddForwardAllowOut(driver->iptables,
+ &network->def->ipAddress,
+ &network->def->netmask,
+ network->def->bridge,
+ network->def->forwardDev) < 0) {
+ networkReportError(VIR_ERR_SYSTEM_ERROR,
+ _("failed to add iptables rule to allow routing from
'%s'"),
+ network->def->bridge);
goto routeerr1;
}
/* allow routing packets to the bridge interface */
- if ((err = iptablesAddForwardAllowIn(driver->iptables,
- &network->def->ipAddress,
- &network->def->netmask,
- network->def->bridge,
- network->def->forwardDev))) {
- virReportSystemError(err,
- _("failed to add iptables rule to allow routing to
'%s'"),
- network->def->bridge);
+ if (iptablesAddForwardAllowIn(driver->iptables,
+ &network->def->ipAddress,
+ &network->def->netmask,
+ network->def->bridge,
+ network->def->forwardDev) < 0) {
+ networkReportError(VIR_ERR_SYSTEM_ERROR,
+ _("failed to add iptables rule to allow routing to
'%s'"),
+ network->def->bridge);
goto routeerr2;
}
@@ -743,69 +743,68 @@ networkAddRoutingIptablesRules(struct network_driver *driver,
static int
networkAddIptablesRules(struct network_driver *driver,
virNetworkObjPtr network) {
- int err;
/* allow DHCP requests through to dnsmasq */
- if ((err = iptablesAddTcpInput(driver->iptables, network->def->bridge, 67)))
{
- virReportSystemError(err,
- _("failed to add iptables rule to allow DHCP requests
from '%s'"),
- network->def->bridge);
+ if (iptablesAddTcpInput(driver->iptables, network->def->bridge, 67) < 0)
{
+ networkReportError(VIR_ERR_SYSTEM_ERROR,
+ _("failed to add iptables rule to allow DHCP requests
from '%s'"),
+ network->def->bridge);
goto err1;
}
- if ((err = iptablesAddUdpInput(driver->iptables, network->def->bridge, 67)))
{
- virReportSystemError(err,
- _("failed to add iptables rule to allow DHCP requests
from '%s'"),
- network->def->bridge);
+ if (iptablesAddUdpInput(driver->iptables, network->def->bridge, 67) < 0)
{
+ networkReportError(VIR_ERR_SYSTEM_ERROR,
+ _("failed to add iptables rule to allow DHCP requests
from '%s'"),
+ network->def->bridge);
goto err2;
}
/* allow DNS requests through to dnsmasq */
- if ((err = iptablesAddTcpInput(driver->iptables, network->def->bridge, 53)))
{
- virReportSystemError(err,
- _("failed to add iptables rule to allow DNS requests
from '%s'"),
- network->def->bridge);
+ if (iptablesAddTcpInput(driver->iptables, network->def->bridge, 53) < 0)
{
+ networkReportError(VIR_ERR_SYSTEM_ERROR,
+ _("failed to add iptables rule to allow DNS requests from
'%s'"),
+ network->def->bridge);
goto err3;
}
- if ((err = iptablesAddUdpInput(driver->iptables, network->def->bridge, 53)))
{
- virReportSystemError(err,
- _("failed to add iptables rule to allow DNS requests
from '%s'"),
- network->def->bridge);
+ if (iptablesAddUdpInput(driver->iptables, network->def->bridge, 53) < 0)
{
+ networkReportError(VIR_ERR_SYSTEM_ERROR,
+ _("failed to add iptables rule to allow DNS requests from
'%s'"),
+ network->def->bridge);
goto err4;
}
/* allow TFTP requests through to dnsmasq */
if (network->def->tftproot &&
- (err = iptablesAddUdpInput(driver->iptables, network->def->bridge, 69)))
{
- virReportSystemError(err,
- _("failed to add iptables rule to allow TFTP requests
from '%s'"),
- network->def->bridge);
+ iptablesAddUdpInput(driver->iptables, network->def->bridge, 69) < 0)
{
+ networkReportError(VIR_ERR_SYSTEM_ERROR,
+ _("failed to add iptables rule to allow TFTP requests
from '%s'"),
+ network->def->bridge);
goto err4tftp;
}
/* Catch all rules to block forwarding to/from bridges */
- if ((err = iptablesAddForwardRejectOut(driver->iptables,
network->def->bridge))) {
- virReportSystemError(err,
- _("failed to add iptables rule to block outbound
traffic from '%s'"),
- network->def->bridge);
+ if (iptablesAddForwardRejectOut(driver->iptables, network->def->bridge) <
0) {
+ networkReportError(VIR_ERR_SYSTEM_ERROR,
+ _("failed to add iptables rule to block outbound traffic
from '%s'"),
+ network->def->bridge);
goto err5;
}
- if ((err = iptablesAddForwardRejectIn(driver->iptables,
network->def->bridge))) {
- virReportSystemError(err,
- _("failed to add iptables rule to block inbound traffic
to '%s'"),
- network->def->bridge);
+ if (iptablesAddForwardRejectIn(driver->iptables, network->def->bridge) <
0) {
+ networkReportError(VIR_ERR_SYSTEM_ERROR,
+ _("failed to add iptables rule to block inbound traffic
to '%s'"),
+ network->def->bridge);
goto err6;
}
/* Allow traffic between guests on the same bridge */
- if ((err = iptablesAddForwardAllowCross(driver->iptables,
network->def->bridge))) {
- virReportSystemError(err,
- _("failed to add iptables rule to allow cross bridge
traffic on '%s'"),
- network->def->bridge);
+ if (iptablesAddForwardAllowCross(driver->iptables, network->def->bridge)
< 0) {
+ networkReportError(VIR_ERR_SYSTEM_ERROR,
+ _("failed to add iptables rule to allow cross bridge
traffic on '%s'"),
+ network->def->bridge);
goto err7;
}
@@ -828,7 +827,7 @@ networkAddIptablesRules(struct network_driver *driver,
if ((VIR_SOCKET_HAS_ADDR(&network->def->ipAddress) ||
network->def->nranges) &&
(iptablesAddOutputFixUdpChecksum(driver->iptables,
- network->def->bridge, 68) != 0)) {
+ network->def->bridge, 68) < 0)) {
VIR_WARN("Could not add rule to fixup DHCP response checksums "
"on network '%s'.", network->def->name);
VIR_WARN0("May need to update iptables package & kernel to support
CHECKSUM rule.");
--
1.7.3.4
5:13 p.m.
New subject: [libvirt] [PATCHv2 03/13] Fix logging of failed iptables commands
On 12/22/2010 11:58 AM, Laine Stump wrote:
The functions in iptables.c all return -1 on failure, but all their
callers (which all happen to be in bridge_driver.c) assume that they
are returning an errno, and the logging is done accordingly. This
patch fixes all the error checking and logging to assume < 0 is an
error, and nothing else.
ACK from v1 stands.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
12:58 p.m.
New subject: [libvirt] [PATCHv2 04/13] Consistently return 0 on success, -1 on failure in bridge_driver.c
Some functions in this file were returning 1 on success and 0 on
failure, and others were returning 0 on success and -1 on
failure. Switch them all to return the libvirt-preferred 0/-1.
---
No changes from V1.
src/network/bridge_driver.c | 28 ++++++++++++++--------------
1 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 81a58a9..23c39e2 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -373,7 +373,7 @@ networkSaveDnsmasqHostsfile(virNetworkObjPtr network,
unsigned int i;
if (! force && virFileExists(dctx->hostsfile->path))
- return 1;
+ return 0;
for (i = 0 ; i < network->def->nhosts ; i++) {
virNetworkDHCPHostDefPtr host = &(network->def->hosts[i]);
@@ -382,9 +382,9 @@ networkSaveDnsmasqHostsfile(virNetworkObjPtr network,
}
if (dnsmasqSave(dctx) < 0)
- return 0;
+ return -1;
- return 1;
+ return 0;
}
@@ -487,7 +487,7 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network,
goto cleanup;
}
- if (networkSaveDnsmasqHostsfile(network, dctx, false)) {
+ if (networkSaveDnsmasqHostsfile(network, dctx, false) < 0) {
virCommandAddArgPair(cmd, "--dhcp-hostsfile",
dctx->hostsfile->path);
}
@@ -669,7 +669,7 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver,
goto masqerr5;
}
- return 1;
+ return 0;
masqerr5:
iptablesRemoveForwardMasquerade(driver->iptables,
@@ -696,7 +696,7 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver,
network->def->bridge,
network->def->forwardDev);
masqerr1:
- return 0;
+ return -1;
}
static int
@@ -727,7 +727,7 @@ networkAddRoutingIptablesRules(struct network_driver *driver,
goto routeerr2;
}
- return 1;
+ return 0;
routeerr2:
@@ -737,7 +737,7 @@ networkAddRoutingIptablesRules(struct network_driver *driver,
network->def->bridge,
network->def->forwardDev);
routeerr1:
- return 0;
+ return -1;
}
static int
@@ -811,11 +811,11 @@ networkAddIptablesRules(struct network_driver *driver,
/* If masquerading is enabled, set up the rules*/
if (network->def->forwardType == VIR_NETWORK_FORWARD_NAT &&
- !networkAddMasqueradingIptablesRules(driver, network))
+ networkAddMasqueradingIptablesRules(driver, network) < 0)
goto err8;
/* else if routing is enabled, set up the rules*/
else if (network->def->forwardType == VIR_NETWORK_FORWARD_ROUTE &&
- !networkAddRoutingIptablesRules(driver, network))
+ networkAddRoutingIptablesRules(driver, network) < 0)
goto err8;
/* If we are doing local DHCP service on this network, attempt to
@@ -833,7 +833,7 @@ networkAddIptablesRules(struct network_driver *driver,
VIR_WARN0("May need to update iptables package & kernel to support
CHECKSUM rule.");
}
- return 1;
+ return 0;
err8:
iptablesRemoveForwardAllowCross(driver->iptables,
@@ -857,7 +857,7 @@ networkAddIptablesRules(struct network_driver *driver,
err2:
iptablesRemoveTcpInput(driver->iptables, network->def->bridge, 67);
err1:
- return 0;
+ return -1;
}
static void
@@ -926,7 +926,7 @@ networkReloadIptablesRules(struct network_driver *driver)
if (virNetworkObjIsActive(driver->networks.objs[i])) {
networkRemoveIptablesRules(driver, driver->networks.objs[i]);
- if (!networkAddIptablesRules(driver, driver->networks.objs[i])) {
+ if (networkAddIptablesRules(driver, driver->networks.objs[i]) < 0) {
/* failed to add but already logged */
}
}
@@ -1141,7 +1141,7 @@ static int networkStartNetworkDaemon(struct network_driver *driver,
goto err_delbr;
}
- if (!networkAddIptablesRules(driver, network))
+ if (networkAddIptablesRules(driver, network) < 0)
goto err_delbr1;
if (network->def->forwardType != VIR_NETWORK_FORWARD_NONE &&
--
1.7.3.4
5:13 p.m.
New subject: [libvirt] [PATCHv2 04/13] Consistently return 0 on success, -1 on failure in bridge_driver.c
On 12/22/2010 11:58 AM, Laine Stump wrote:
Some functions in this file were returning 1 on success and 0 on
failure, and others were returning 0 on success and -1 on
failure. Switch them all to return the libvirt-preferred 0/-1.
ACK.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
12:58 p.m.
New subject: [libvirt] [PATCHv2 05/13] Pass prefix rather than netmask into iptables functions
IPv6 will use prefix exclusively, and IPv4 will also optionally be
able to use it, and the iptables functions really need a prefix
anyway, so use the new virNetworkDefPrefix() function to send prefixes
into iptables functions instead of netmasks.
Also, in a couple places where a netmask is actually needed, use the
new private API function for it rather than getting it directly. This
will allow for cases where no netmask or prefix is specified (it
returns the default for the current class of network.)
---
V2 Changes:
* Any prefix arg is now unsigned int rather than int.
src/network/bridge_driver.c | 91 +++++++++++++++++++++++++++++++------------
src/util/iptables.c | 63 +++++++++++++++---------------
src/util/iptables.h | 16 ++++----
3 files changed, 105 insertions(+), 65 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 23c39e2..41c8478 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -585,11 +585,19 @@ cleanup:
static int
networkAddMasqueradingIptablesRules(struct network_driver *driver,
virNetworkObjPtr network) {
+ int prefix = virNetworkDefPrefix(network->def);
+
+ if (prefix < 0) {
+ networkReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Invalid prefix or netmask for '%s'"),
+ network->def->bridge);
+ goto masqerr1;
+ }
/* allow forwarding packets from the bridge interface */
if (iptablesAddForwardAllowOut(driver->iptables,
&network->def->ipAddress,
- &network->def->netmask,
+ prefix,
network->def->bridge,
network->def->forwardDev) < 0) {
networkReportError(VIR_ERR_SYSTEM_ERROR,
@@ -601,7 +609,7 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver,
/* allow forwarding packets to the bridge interface if they are part of an existing
connection */
if (iptablesAddForwardAllowRelatedIn(driver->iptables,
&network->def->ipAddress,
- &network->def->netmask,
+ prefix,
network->def->bridge,
network->def->forwardDev) < 0) {
networkReportError(VIR_ERR_SYSTEM_ERROR,
@@ -636,7 +644,7 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver,
/* First the generic masquerade rule for other protocols */
if (iptablesAddForwardMasquerade(driver->iptables,
&network->def->ipAddress,
- &network->def->netmask,
+ prefix,
network->def->forwardDev,
NULL) < 0) {
networkReportError(VIR_ERR_SYSTEM_ERROR,
@@ -648,7 +656,7 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver,
/* UDP with a source port restriction */
if (iptablesAddForwardMasquerade(driver->iptables,
&network->def->ipAddress,
- &network->def->netmask,
+ prefix,
network->def->forwardDev,
"udp") < 0) {
networkReportError(VIR_ERR_SYSTEM_ERROR,
@@ -660,7 +668,7 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver,
/* TCP with a source port restriction */
if (iptablesAddForwardMasquerade(driver->iptables,
&network->def->ipAddress,
- &network->def->netmask,
+ prefix,
network->def->forwardDev,
"tcp") < 0) {
networkReportError(VIR_ERR_SYSTEM_ERROR,
@@ -674,25 +682,25 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver,
masqerr5:
iptablesRemoveForwardMasquerade(driver->iptables,
&network->def->ipAddress,
- &network->def->netmask,
+ prefix,
network->def->forwardDev,
"udp");
masqerr4:
iptablesRemoveForwardMasquerade(driver->iptables,
&network->def->ipAddress,
- &network->def->netmask,
+ prefix,
network->def->forwardDev,
NULL);
masqerr3:
iptablesRemoveForwardAllowRelatedIn(driver->iptables,
&network->def->ipAddress,
- &network->def->netmask,
+ prefix,
network->def->bridge,
network->def->forwardDev);
masqerr2:
iptablesRemoveForwardAllowOut(driver->iptables,
&network->def->ipAddress,
- &network->def->netmask,
+ prefix,
network->def->bridge,
network->def->forwardDev);
masqerr1:
@@ -702,11 +710,19 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver,
static int
networkAddRoutingIptablesRules(struct network_driver *driver,
virNetworkObjPtr network) {
+ int prefix = virNetworkDefPrefix(network->def);
+
+ if (prefix < 0) {
+ networkReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Invalid prefix or netmask for '%s'"),
+ network->def->bridge);
+ goto routeerr1;
+ }
/* allow routing packets from the bridge interface */
if (iptablesAddForwardAllowOut(driver->iptables,
&network->def->ipAddress,
- &network->def->netmask,
+ prefix,
network->def->bridge,
network->def->forwardDev) < 0) {
networkReportError(VIR_ERR_SYSTEM_ERROR,
@@ -718,7 +734,7 @@ networkAddRoutingIptablesRules(struct network_driver *driver,
/* allow routing packets to the bridge interface */
if (iptablesAddForwardAllowIn(driver->iptables,
&network->def->ipAddress,
- &network->def->netmask,
+ prefix,
network->def->bridge,
network->def->forwardDev) < 0) {
networkReportError(VIR_ERR_SYSTEM_ERROR,
@@ -733,7 +749,7 @@ networkAddRoutingIptablesRules(struct network_driver *driver,
routeerr2:
iptablesRemoveForwardAllowOut(driver->iptables,
&network->def->ipAddress,
- &network->def->netmask,
+ prefix,
network->def->bridge,
network->def->forwardDev);
routeerr1:
@@ -869,40 +885,50 @@ networkRemoveIptablesRules(struct network_driver *driver,
network->def->bridge, 68);
}
if (network->def->forwardType != VIR_NETWORK_FORWARD_NONE) {
+ int prefix = virNetworkDefPrefix(network->def);
+
+ if (prefix < 0) {
+ networkReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Invalid prefix or netmask for
'%s'"),
+ network->def->bridge);
+ goto error;
+ }
+
if (network->def->forwardType == VIR_NETWORK_FORWARD_NAT) {
iptablesRemoveForwardMasquerade(driver->iptables,
&network->def->ipAddress,
- &network->def->netmask,
+ prefix,
network->def->forwardDev,
"tcp");
iptablesRemoveForwardMasquerade(driver->iptables,
&network->def->ipAddress,
- &network->def->netmask,
+ prefix,
network->def->forwardDev,
"udp");
iptablesRemoveForwardMasquerade(driver->iptables,
&network->def->ipAddress,
- &network->def->netmask,
+ prefix,
network->def->forwardDev,
NULL);
iptablesRemoveForwardAllowRelatedIn(driver->iptables,
&network->def->ipAddress,
- &network->def->netmask,
+ prefix,
network->def->bridge,
network->def->forwardDev);
} else if (network->def->forwardType == VIR_NETWORK_FORWARD_ROUTE)
iptablesRemoveForwardAllowIn(driver->iptables,
&network->def->ipAddress,
- &network->def->netmask,
+ prefix,
network->def->bridge,
network->def->forwardDev);
iptablesRemoveForwardAllowOut(driver->iptables,
&network->def->ipAddress,
- &network->def->netmask,
+ prefix,
network->def->bridge,
network->def->forwardDev);
}
+error:
iptablesRemoveForwardAllowCross(driver->iptables, network->def->bridge);
iptablesRemoveForwardRejectIn(driver->iptables, network->def->bridge);
iptablesRemoveForwardRejectOut(driver->iptables, network->def->bridge);
@@ -1006,17 +1032,23 @@ static int networkCheckRouteCollision(virNetworkObjPtr network)
{
int ret = -1, len;
unsigned int net_dest;
+ virSocketAddr netmask;
char *cur, *buf = NULL;
enum {MAX_ROUTE_SIZE = 1024*64};
- if (!VIR_SOCKET_IS_FAMILY(&network->def->ipAddress, AF_INET) ||
- !VIR_SOCKET_IS_FAMILY(&network->def->netmask, AF_INET)) {
+ if (!VIR_SOCKET_IS_FAMILY(&network->def->ipAddress, AF_INET)) {
/* Only support collision check for IPv4 */
return 0;
}
+ if (virNetworkDefNetmask(network->def, &netmask) < 0) {
+ networkReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to get netmask of '%s'"),
+ network->def->bridge);
+ }
+
net_dest = (network->def->ipAddress.data.inet4.sin_addr.s_addr &
- network->def->netmask.data.inet4.sin_addr.s_addr);
+ netmask.data.inet4.sin_addr.s_addr);
/* Read whole routing table into memory */
if ((len = virFileReadAll(PROC_NET_ROUTE, MAX_ROUTE_SIZE, &buf)) < 0)
@@ -1069,7 +1101,7 @@ static int networkCheckRouteCollision(virNetworkObjPtr network)
addr_val &= mask_val;
if ((net_dest == addr_val) &&
- (network->def->netmask.data.inet4.sin_addr.s_addr == mask_val)) {
+ (netmask.data.inet4.sin_addr.s_addr == mask_val)) {
networkReportError(VIR_ERR_INTERNAL_ERROR,
_("Network is already in use by interface %s"),
iface);
@@ -1125,9 +1157,18 @@ static int networkStartNetworkDaemon(struct network_driver
*driver,
goto err_delbr;
}
- if (VIR_SOCKET_HAS_ADDR(&network->def->netmask) &&
- (err = brSetInetNetmask(driver->brctl, network->def->bridge,
- &network->def->netmask))) {
+ virSocketAddr netmask;
+
+ if (virNetworkDefNetmask(network->def, &netmask) < 0) {
+
+ networkReportError(VIR_ERR_INTERNAL_ERROR,
+ _("bridge '%s' has an invalid netmask or IP
address"),
+ network->def->bridge);
+ goto err_delbr;
+ }
+
+ if ((err = brSetInetNetmask(driver->brctl, network->def->bridge,
+ &netmask))) {
virReportSystemError(err,
_("cannot set netmask on bridge '%s'"),
network->def->bridge);
diff --git a/src/util/iptables.c b/src/util/iptables.c
index effd81b..f7b692c 100644
--- a/src/util/iptables.c
+++ b/src/util/iptables.c
@@ -276,25 +276,24 @@ iptablesRemoveUdpInput(iptablesContext *ctx,
static char *iptablesFormatNetwork(virSocketAddr *netaddr,
- virSocketAddr *netmask)
+ unsigned int prefix)
{
virSocketAddr network;
- int prefix;
char *netstr;
char *ret;
- if (!VIR_SOCKET_IS_FAMILY(netaddr, AF_INET) ||
- !VIR_SOCKET_IS_FAMILY(netmask, AF_INET)) {
+ if (!VIR_SOCKET_IS_FAMILY(netaddr, AF_INET)) {
iptablesError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("Only IPv4 addresses can be used with iptables"));
return NULL;
}
network = *netaddr;
- network.data.inet4.sin_addr.s_addr &=
- netmask->data.inet4.sin_addr.s_addr;
-
- prefix = virSocketGetNumNetmaskBits(netmask);
+ if (virSocketAddrMaskByPrefix(&network, prefix) < 0) {
+ iptablesError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Failure to mask address"));
+ return NULL;
+ }
netstr = virSocketFormatAddr(&network);
@@ -315,7 +314,7 @@ static char *iptablesFormatNetwork(virSocketAddr *netaddr,
static int
iptablesForwardAllowOut(iptablesContext *ctx,
virSocketAddr *netaddr,
- virSocketAddr *netmask,
+ unsigned int prefix,
const char *iface,
const char *physdev,
int action)
@@ -323,7 +322,7 @@ iptablesForwardAllowOut(iptablesContext *ctx,
int ret;
char *networkstr;
- if (!(networkstr = iptablesFormatNetwork(netaddr, netmask)))
+ if (!(networkstr = iptablesFormatNetwork(netaddr, prefix)))
return -1;
if (physdev && physdev[0]) {
@@ -362,11 +361,11 @@ iptablesForwardAllowOut(iptablesContext *ctx,
int
iptablesAddForwardAllowOut(iptablesContext *ctx,
virSocketAddr *netaddr,
- virSocketAddr *netmask,
+ unsigned int prefix,
const char *iface,
const char *physdev)
{
- return iptablesForwardAllowOut(ctx, netaddr, netmask, iface, physdev, ADD);
+ return iptablesForwardAllowOut(ctx, netaddr, prefix, iface, physdev, ADD);
}
/**
@@ -385,11 +384,11 @@ iptablesAddForwardAllowOut(iptablesContext *ctx,
int
iptablesRemoveForwardAllowOut(iptablesContext *ctx,
virSocketAddr *netaddr,
- virSocketAddr *netmask,
+ unsigned int prefix,
const char *iface,
const char *physdev)
{
- return iptablesForwardAllowOut(ctx, netaddr, netmask, iface, physdev, REMOVE);
+ return iptablesForwardAllowOut(ctx, netaddr, prefix, iface, physdev, REMOVE);
}
@@ -399,7 +398,7 @@ iptablesRemoveForwardAllowOut(iptablesContext *ctx,
static int
iptablesForwardAllowRelatedIn(iptablesContext *ctx,
virSocketAddr *netaddr,
- virSocketAddr *netmask,
+ unsigned int prefix,
const char *iface,
const char *physdev,
int action)
@@ -407,7 +406,7 @@ iptablesForwardAllowRelatedIn(iptablesContext *ctx,
int ret;
char *networkstr;
- if (!(networkstr = iptablesFormatNetwork(netaddr, netmask)))
+ if (!(networkstr = iptablesFormatNetwork(netaddr, prefix)))
return -1;
if (physdev && physdev[0]) {
@@ -450,11 +449,11 @@ iptablesForwardAllowRelatedIn(iptablesContext *ctx,
int
iptablesAddForwardAllowRelatedIn(iptablesContext *ctx,
virSocketAddr *netaddr,
- virSocketAddr *netmask,
+ unsigned int prefix,
const char *iface,
const char *physdev)
{
- return iptablesForwardAllowRelatedIn(ctx, netaddr, netmask, iface, physdev, ADD);
+ return iptablesForwardAllowRelatedIn(ctx, netaddr, prefix, iface, physdev, ADD);
}
/**
@@ -473,11 +472,11 @@ iptablesAddForwardAllowRelatedIn(iptablesContext *ctx,
int
iptablesRemoveForwardAllowRelatedIn(iptablesContext *ctx,
virSocketAddr *netaddr,
- virSocketAddr *netmask,
+ unsigned int prefix,
const char *iface,
const char *physdev)
{
- return iptablesForwardAllowRelatedIn(ctx, netaddr, netmask, iface, physdev, REMOVE);
+ return iptablesForwardAllowRelatedIn(ctx, netaddr, prefix, iface, physdev, REMOVE);
}
/* Allow all traffic destined to the bridge, with a valid network address
@@ -485,7 +484,7 @@ iptablesRemoveForwardAllowRelatedIn(iptablesContext *ctx,
static int
iptablesForwardAllowIn(iptablesContext *ctx,
virSocketAddr *netaddr,
- virSocketAddr *netmask,
+ unsigned int prefix,
const char *iface,
const char *physdev,
int action)
@@ -493,7 +492,7 @@ iptablesForwardAllowIn(iptablesContext *ctx,
int ret;
char *networkstr;
- if (!(networkstr = iptablesFormatNetwork(netaddr, netmask)))
+ if (!(networkstr = iptablesFormatNetwork(netaddr, prefix)))
return -1;
if (physdev && physdev[0]) {
@@ -532,11 +531,11 @@ iptablesForwardAllowIn(iptablesContext *ctx,
int
iptablesAddForwardAllowIn(iptablesContext *ctx,
virSocketAddr *netaddr,
- virSocketAddr *netmask,
+ unsigned int prefix,
const char *iface,
const char *physdev)
{
- return iptablesForwardAllowIn(ctx, netaddr, netmask, iface, physdev, ADD);
+ return iptablesForwardAllowIn(ctx, netaddr, prefix, iface, physdev, ADD);
}
/**
@@ -555,11 +554,11 @@ iptablesAddForwardAllowIn(iptablesContext *ctx,
int
iptablesRemoveForwardAllowIn(iptablesContext *ctx,
virSocketAddr *netaddr,
- virSocketAddr *netmask,
+ unsigned int prefix,
const char *iface,
const char *physdev)
{
- return iptablesForwardAllowIn(ctx, netaddr, netmask, iface, physdev, REMOVE);
+ return iptablesForwardAllowIn(ctx, netaddr, prefix, iface, physdev, REMOVE);
}
@@ -722,7 +721,7 @@ iptablesRemoveForwardRejectIn(iptablesContext *ctx,
static int
iptablesForwardMasquerade(iptablesContext *ctx,
virSocketAddr *netaddr,
- virSocketAddr *netmask,
+ unsigned int prefix,
const char *physdev,
const char *protocol,
int action)
@@ -730,7 +729,7 @@ iptablesForwardMasquerade(iptablesContext *ctx,
int ret;
char *networkstr;
- if (!(networkstr = iptablesFormatNetwork(netaddr, netmask)))
+ if (!(networkstr = iptablesFormatNetwork(netaddr, prefix)))
return -1;
if (protocol && protocol[0]) {
@@ -792,11 +791,11 @@ iptablesForwardMasquerade(iptablesContext *ctx,
int
iptablesAddForwardMasquerade(iptablesContext *ctx,
virSocketAddr *netaddr,
- virSocketAddr *netmask,
+ unsigned int prefix,
const char *physdev,
const char *protocol)
{
- return iptablesForwardMasquerade(ctx, netaddr, netmask, physdev, protocol, ADD);
+ return iptablesForwardMasquerade(ctx, netaddr, prefix, physdev, protocol, ADD);
}
/**
@@ -815,11 +814,11 @@ iptablesAddForwardMasquerade(iptablesContext *ctx,
int
iptablesRemoveForwardMasquerade(iptablesContext *ctx,
virSocketAddr *netaddr,
- virSocketAddr *netmask,
+ unsigned int prefix,
const char *physdev,
const char *protocol)
{
- return iptablesForwardMasquerade(ctx, netaddr, netmask, physdev, protocol, REMOVE);
+ return iptablesForwardMasquerade(ctx, netaddr, prefix, physdev, protocol, REMOVE);
}
diff --git a/src/util/iptables.h b/src/util/iptables.h
index ed843ec..982acf1 100644
--- a/src/util/iptables.h
+++ b/src/util/iptables.h
@@ -45,34 +45,34 @@ int iptablesRemoveUdpInput (iptablesContext
*ctx,
int iptablesAddForwardAllowOut (iptablesContext *ctx,
virSocketAddr *netaddr,
- virSocketAddr *netmask,
+ unsigned int prefix,
const char *iface,
const char *physdev);
int iptablesRemoveForwardAllowOut (iptablesContext *ctx,
virSocketAddr *netaddr,
- virSocketAddr *netmask,
+ unsigned int prefix,
const char *iface,
const char *physdev);
int iptablesAddForwardAllowRelatedIn(iptablesContext *ctx,
virSocketAddr *netaddr,
- virSocketAddr *netmask,
+ unsigned int prefix,
const char *iface,
const char *physdev);
int iptablesRemoveForwardAllowRelatedIn(iptablesContext *ctx,
virSocketAddr *netaddr,
- virSocketAddr *netmask,
+ unsigned int prefix,
const char *iface,
const char *physdev);
int iptablesAddForwardAllowIn (iptablesContext *ctx,
virSocketAddr *netaddr,
- virSocketAddr *netmask,
+ unsigned int prefix,
const char *iface,
const char *physdev);
int iptablesRemoveForwardAllowIn (iptablesContext *ctx,
virSocketAddr *netaddr,
- virSocketAddr *netmask,
+ unsigned int prefix,
const char *iface,
const char *physdev);
@@ -93,12 +93,12 @@ int iptablesRemoveForwardRejectIn (iptablesContext
*ctx,
int iptablesAddForwardMasquerade (iptablesContext *ctx,
virSocketAddr *netaddr,
- virSocketAddr *netmask,
+ unsigned int prefix,
const char *physdev,
const char *protocol);
int iptablesRemoveForwardMasquerade (iptablesContext *ctx,
virSocketAddr *netaddr,
- virSocketAddr *netmask,
+ unsigned int prefix,
const char *physdev,
const char *protocol);
int iptablesAddOutputFixUdpChecksum (iptablesContext *ctx,
--
1.7.3.4
5:19 p.m.
New subject: [libvirt] [PATCHv2 05/13] Pass prefix rather than netmask into iptables functions
On 12/22/2010 11:58 AM, Laine Stump wrote:
IPv6 will use prefix exclusively, and IPv4 will also optionally be
able to use it, and the iptables functions really need a prefix
anyway, so use the new virNetworkDefPrefix() function to send prefixes
into iptables functions instead of netmasks.
Also, in a couple places where a netmask is actually needed, use the
new private API function for it rather than getting it directly. This
will allow for cases where no netmask or prefix is specified (it
returns the default for the current class of network.)
ACK.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
12:58 p.m.
New subject: [libvirt] [PATCHv2 06/13] Make virtual network netmasks optional
When a netmask isn't specified for an IPv4 address, one can be implied
based on what network class range the address is in. The
virNetworkDefPrefix function does this for us, so netmask isn't
required.
---
V2 Changes:
* Any prefix arg is now unsigned int rather than int.
src/conf/network_conf.c | 30 ++++++++++++++++--------------
1 files changed, 16 insertions(+), 14 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 427b338..b17c708 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -470,19 +470,14 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
def->delay = 0;
ipAddress = virXPathString("string(./ip[1]/@address)", ctxt);
- netmask = virXPathString("string(./ip[1]/@netmask)", ctxt);
- if (ipAddress &&
- netmask) {
+ if (ipAddress) {
xmlNodePtr ip;
if (virSocketParseAddr(ipAddress, &def->ipAddress, AF_UNSPEC) < 0)
goto error;
- if (virSocketParseAddr(netmask, &def->netmask, AF_UNSPEC) < 0)
- goto error;
/* XXX someday we want IPv6, so will need to relax this */
- if (!VIR_SOCKET_IS_FAMILY(&def->ipAddress, AF_INET) ||
- !VIR_SOCKET_IS_FAMILY(&def->netmask, AF_INET)) {
+ if (!VIR_SOCKET_IS_FAMILY(&def->ipAddress, AF_INET)) {
virNetworkReportError(VIR_ERR_CONFIG_UNSUPPORTED,
"%s", _("Only IPv4 addresses are
supported"));
goto error;
@@ -493,18 +488,25 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
goto error;
}
VIR_FREE(ipAddress);
- VIR_FREE(netmask);
+ netmask = virXPathString("string(./ip[1]/@netmask)", ctxt);
+ if (netmask) {
- /* IPv4 forwarding setup */
- if (virXPathBoolean("count(./forward) > 0", ctxt)) {
- if (def->ipAddress.data.sa.sa_family != AF_INET ||
- def->netmask.data.sa.sa_family != AF_INET) {
- virNetworkReportError(VIR_ERR_INTERNAL_ERROR,
- "%s", _("Forwarding requested, but no
IPv4 address/netmask provided"));
+ if (virSocketParseAddr(netmask, &def->netmask, AF_UNSPEC) < 0)
+ goto error;
+
+ /* XXX someday we want IPv6, so will need to relax this */
+ if (!VIR_SOCKET_IS_FAMILY(&def->netmask, AF_INET)) {
+ virNetworkReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ "%s", _("Only IPv4 addresses are
supported"));
goto error;
}
+ }
+ VIR_FREE(netmask);
+
+ /* IPv4 forwarding setup */
+ if (virXPathBoolean("count(./forward) > 0", ctxt)) {
tmp = virXPathString("string(./forward[1]/@mode)", ctxt);
if (tmp) {
if ((def->forwardType = virNetworkForwardTypeFromString(tmp)) < 0) {
--
1.7.3.4
5:23 p.m.
New subject: [libvirt] [PATCHv2 06/13] Make virtual network netmasks optional
On 12/22/2010 11:58 AM, Laine Stump wrote:
When a netmask isn't specified for an IPv4 address, one can be
implied
based on what network class range the address is in. The
virNetworkDefPrefix function does this for us, so netmask isn't
required.
ACK.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
12:58 p.m.
New subject: [libvirt] [PATCHv2 07/13] Replace brSetInetAddress/brSetInetNetmask with brAddInetAddress
brSetInetAddress can only set a single IP address on the bridge, and
uses a method (ioctl(SIOCSETIFADDR)) that only works for IPv4. Replace
it and brSetInetNetmask with a single function that uses the external
"ip addr add" command to add an address/prefix to the interface - this
supports IPv6, and allows adding multiple addresses to the interface.
Although it isn't currently used in the code, we also add a
brDelInetAddress for completeness' sake.
Also, while we're modifying bridge.c, we change brSetForwardDelay and
brSetEnableSTP to use the new virCommand API rather than the
deprecated virRun, and also log an error message in bridge_driver.c if
either of those fail (previously the failure would be completely
silent).
NB: it's a bit bothersome that there is no difference in error
reporting between being unable to run one of these commands, and the
command returning a non-0 exit status, but there is no precedent in
bridge.c for logging error messages locally rather than pushing them
up the call chain, and what's pushed up is only 'success' or
'failure'; no exit codes. Perhaps a non-0 exit could be passed up
directly as the return, other errors could return -1, and callers
could check for ret != 0 rather than ret < 0?
---
V2 Changes:
* Any prefix arg is now unsigned int rather than int.
configure.ac | 3 +
src/libvirt_bridge.syms | 3 +-
src/network/bridge_driver.c | 50 ++++++++-------
src/util/bridge.c | 141 +++++++++++++++++++++++++------------------
src/util/bridge.h | 10 ++-
5 files changed, 118 insertions(+), 89 deletions(-)
diff --git a/configure.ac b/configure.ac
index 73c1176..4db613a 100644
--- a/configure.ac
+++ b/configure.ac
@@ -306,6 +306,9 @@ if test x"$with_rhel5_api" = x"yes"; then
AC_DEFINE([WITH_RHEL5_API], [1], [whether building for the RHEL-5 API])
fi
+AC_PATH_PROG([IP_PATH], [ip], /sbin/ip, [/usr/sbin:$PATH])
+AC_DEFINE_UNQUOTED([IP_PATH], "$IP_PATH", [path to ip binary])
+
AC_PATH_PROG([IPTABLES_PATH], [iptables], /sbin/iptables, [/usr/sbin:$PATH])
AC_DEFINE_UNQUOTED([IPTABLES_PATH], "$IPTABLES_PATH", [path to iptables
binary])
diff --git a/src/libvirt_bridge.syms b/src/libvirt_bridge.syms
index 2658291..c3773bd 100644
--- a/src/libvirt_bridge.syms
+++ b/src/libvirt_bridge.syms
@@ -6,15 +6,16 @@
# bridge.h
brAddBridge;
+brAddInetAddress;
brAddInterface;
brAddTap;
brDeleteTap;
brDeleteBridge;
+brDelInetAddress;
brHasBridge;
brInit;
brSetEnableSTP;
brSetForwardDelay;
-brSetInetAddress;
brSetInetNetmask;
brSetInterfaceUp;
brShutdown;
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 41c8478..ba9ff96 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -1142,37 +1142,39 @@ static int networkStartNetworkDaemon(struct network_driver
*driver,
if (networkDisableIPV6(network) < 0)
goto err_delbr;
- if (brSetForwardDelay(driver->brctl, network->def->bridge,
network->def->delay) < 0)
- goto err_delbr;
-
- if (brSetEnableSTP(driver->brctl, network->def->bridge,
network->def->stp ? 1 : 0) < 0)
- goto err_delbr;
-
- if (VIR_SOCKET_HAS_ADDR(&network->def->ipAddress) &&
- (err = brSetInetAddress(driver->brctl, network->def->bridge,
- &network->def->ipAddress))) {
- virReportSystemError(err,
- _("cannot set IP address on bridge
'%s'"),
- network->def->bridge);
+ if ((err = brSetForwardDelay(driver->brctl, network->def->bridge,
+ network->def->delay))) {
+ networkReportError(VIR_ERR_INTERNAL_ERROR,
+ _("cannot set forward delay on bridge
'%s'"),
+ network->def->bridge);
goto err_delbr;
}
- virSocketAddr netmask;
-
- if (virNetworkDefNetmask(network->def, &netmask) < 0) {
-
+ if ((err = brSetEnableSTP(driver->brctl, network->def->bridge,
+ network->def->stp ? 1 : 0))) {
networkReportError(VIR_ERR_INTERNAL_ERROR,
- _("bridge '%s' has an invalid netmask or IP
address"),
- network->def->bridge);
+ _("cannot set STP '%s' on bridge
'%s'"),
+ network->def->stp ? "on" : "off",
network->def->bridge);
goto err_delbr;
}
- if ((err = brSetInetNetmask(driver->brctl, network->def->bridge,
- &netmask))) {
- virReportSystemError(err,
- _("cannot set netmask on bridge '%s'"),
- network->def->bridge);
- goto err_delbr;
+ if (VIR_SOCKET_HAS_ADDR(&network->def->ipAddress)) {
+ int prefix = virNetworkDefPrefix(network->def);
+
+ if (prefix < 0) {
+ networkReportError(VIR_ERR_INTERNAL_ERROR,
+ _("bridge '%s' has an invalid netmask or IP
address"),
+ network->def->bridge);
+ goto err_delbr;
+ }
+
+ if ((err = brAddInetAddress(driver->brctl, network->def->bridge,
+ &network->def->ipAddress, prefix))) {
+ networkReportError(VIR_ERR_INTERNAL_ERROR,
+ _("cannot set IP address on bridge
'%s'"),
+ network->def->bridge);
+ goto err_delbr;
+ }
}
if ((err = brSetInterfaceUp(driver->brctl, network->def->bridge, 1))) {
diff --git a/src/util/bridge.c b/src/util/bridge.c
index 97cf73f..dcd3f39 100644
--- a/src/util/bridge.c
+++ b/src/util/bridge.c
@@ -46,6 +46,7 @@
# include <net/if_arp.h> /* ARPHRD_ETHER */
# include "internal.h"
+# include "command.h"
# include "memory.h"
# include "util.h"
# include "logging.h"
@@ -655,73 +656,84 @@ brGetInterfaceUp(brControl *ctl,
return 0;
}
-static int
-brSetInetAddr(brControl *ctl,
- const char *ifname,
- int cmd,
- virSocketAddr *addr)
-{
- struct ifreq ifr;
-
- if (!ctl || !ctl->fd || !ifname || !addr)
- return EINVAL;
-
- memset(&ifr, 0, sizeof(struct ifreq));
-
- if (virStrcpyStatic(ifr.ifr_name, ifname) == NULL)
- return EINVAL;
-
- if (!VIR_SOCKET_IS_FAMILY(addr, AF_INET))
- return EINVAL;
-
- ifr.ifr_addr = addr->data.sa;
-
- if (ioctl(ctl->fd, cmd, &ifr) < 0)
- return errno;
-
- return 0;
-}
-
/**
- * brSetInetAddress:
+ * brAddInetAddress:
* @ctl: bridge control pointer
* @ifname: the interface name
- * @addr: the string representation of the IP address
+ * @addr: the IP address (IPv4 or IPv6)
+ * @prefix: number of 1 bits in the netmask
*
- * Function to bind the interface to an IP address, it should handle
- * IPV4 and IPv6. The string for addr would be of the form
- * "ddd.ddd.ddd.ddd" assuming the common IPv4 format.
+ * Add an IP address to an interface. This function *does not* remove
+ * any previously added IP addresses - that must be done separately with
+ * brDelInetAddress.
*
- * Returns 0 in case of success or an errno code in case of failure.
+ * Returns 0 in case of success or -1 in case of error.
*/
int
-brSetInetAddress(brControl *ctl,
+brAddInetAddress(brControl *ctl ATTRIBUTE_UNUSED,
const char *ifname,
- virSocketAddr *addr)
+ virSocketAddr *addr,
+ unsigned int prefix)
{
- return brSetInetAddr(ctl, ifname, SIOCSIFADDR, addr);
+ virCommandPtr cmd;
+ char *addrstr;
+ int ret = -1;
+
+ if (!(addrstr = virSocketFormatAddr(addr)))
+ goto cleanup;
+ cmd = virCommandNew(IP_PATH);
+ virCommandAddArgList(cmd, "addr", "add", NULL);
+ virCommandAddArgFormat(cmd, "%s/%u", addrstr, prefix);
+ virCommandAddArgList(cmd, "dev", ifname, NULL);
+
+ if (virCommandRun(cmd, NULL) < 0)
+ goto cleanup;
+
+ ret = 0;
+cleanup:
+ VIR_FREE(addrstr);
+ virCommandFree(cmd);
+ return ret;
}
/**
- * brSetInetNetmask:
+ * brDelInetAddress:
* @ctl: bridge control pointer
* @ifname: the interface name
- * @addr: the string representation of the netmask
+ * @addr: the IP address (IPv4 or IPv6)
+ * @prefix: number of 1 bits in the netmask
*
- * Function to set the netmask of an interface, it should handle
- * IPV4 and IPv6 forms. The string for addr would be of the form
- * "ddd.ddd.ddd.ddd" assuming the common IPv4 format.
+ * Delete an IP address from an interface.
*
- * Returns 0 in case of success or an errno code in case of failure.
+ * Returns 0 in case of success or -1 in case of error.
*/
int
-brSetInetNetmask(brControl *ctl,
+brDelInetAddress(brControl *ctl ATTRIBUTE_UNUSED,
const char *ifname,
- virSocketAddr *addr)
+ virSocketAddr *addr,
+ unsigned int prefix)
{
- return brSetInetAddr(ctl, ifname, SIOCSIFNETMASK, addr);
+ virCommandPtr cmd;
+ char *addrstr;
+ int ret = -1;
+
+ if (!(addrstr = virSocketFormatAddr(addr)))
+ goto cleanup;
+ cmd = virCommandNew(IP_PATH);
+ virCommandAddArgList(cmd, "addr", "del", NULL);
+ virCommandAddArgFormat(cmd, "%s/%u", addrstr, prefix);
+ virCommandAddArgList(cmd, "dev", ifname, NULL);
+
+ if (virCommandRun(cmd, NULL) < 0)
+ goto cleanup;
+
+ ret = 0;
+cleanup:
+ VIR_FREE(addrstr);
+ virCommandFree(cmd);
+ return ret;
}
/**
@@ -740,17 +752,20 @@ brSetForwardDelay(brControl *ctl ATTRIBUTE_UNUSED,
const char *bridge,
int delay)
{
- char delayStr[30];
- const char *const progargv[] = {
- BRCTL, "setfd", bridge, delayStr, NULL
- };
+ virCommandPtr cmd;
+ int ret = -1;
- snprintf(delayStr, sizeof(delayStr), "%d", delay);
+ cmd = virCommandNew(BRCTL);
+ virCommandAddArgList(cmd, "setfd", bridge, NULL);
+ virCommandAddArgFormat(cmd, "%d", delay);
- if (virRun(progargv, NULL) < 0)
- return -1;
+ if (virCommandRun(cmd, NULL) < 0)
+ goto cleanup;
- return 0;
+ ret = 0;
+cleanup:
+ virCommandFree(cmd);
+ return ret;
}
/**
@@ -769,15 +784,21 @@ brSetEnableSTP(brControl *ctl ATTRIBUTE_UNUSED,
const char *bridge,
int enable)
{
- const char *setting = enable ? "on" : "off";
- const char *const progargv[] = {
- BRCTL, "stp", bridge, setting, NULL
- };
+ virCommandPtr cmd;
+ int ret = -1;
- if (virRun(progargv, NULL) < 0)
- return -1;
+ cmd = virCommandNew(BRCTL);
+ virCommandAddArgList(cmd, "stp", bridge,
+ enable ? "on" : "off",
+ NULL);
- return 0;
+ if (virCommandRun(cmd, NULL) < 0)
+ goto cleanup;
+
+ ret = 0;
+cleanup:
+ virCommandFree(cmd);
+ return ret;
}
#endif /* WITH_BRIDGE */
diff --git a/src/util/bridge.h b/src/util/bridge.h
index 3ef38d9..e8e7eca 100644
--- a/src/util/bridge.h
+++ b/src/util/bridge.h
@@ -83,12 +83,14 @@ int brGetInterfaceUp (brControl *ctl,
const char *ifname,
int *up);
-int brSetInetAddress (brControl *ctl,
+int brAddInetAddress (brControl *ctl,
const char *ifname,
- virSocketAddr *addr);
-int brSetInetNetmask (brControl *ctl,
+ virSocketAddr *addr,
+ unsigned int prefix);
+int brDelInetAddress (brControl *ctl,
const char *ifname,
- virSocketAddr *addr);
+ virSocketAddr *addr,
+ unsigned int prefix);
int brSetForwardDelay (brControl *ctl,
const char *bridge,
--
1.7.3.4
5:32 p.m.
New subject: [libvirt] [PATCHv2 07/13] Replace brSetInetAddress/brSetInetNetmask with brAddInetAddress
On 12/22/2010 11:58 AM, Laine Stump wrote:
brSetInetAddress can only set a single IP address on the bridge, and
uses a method (ioctl(SIOCSETIFADDR)) that only works for IPv4. Replace
it and brSetInetNetmask with a single function that uses the external
"ip addr add" command to add an address/prefix to the interface - this
supports IPv6, and allows adding multiple addresses to the interface.
Although it isn't currently used in the code, we also add a
brDelInetAddress for completeness' sake.
Also, while we're modifying bridge.c, we change brSetForwardDelay and
brSetEnableSTP to use the new virCommand API rather than the
deprecated virRun, and also log an error message in bridge_driver.c if
either of those fail (previously the failure would be completely
silent).
NB: it's a bit bothersome that there is no difference in error
reporting between being unable to run one of these commands, and the
command returning a non-0 exit status, but there is no precedent in
bridge.c for logging error messages locally rather than pushing them
up the call chain, and what's pushed up is only 'success' or
'failure'; no exit codes. Perhaps a non-0 exit could be passed up
directly as the return, other errors could return -1, and callers
could check for ret != 0 rather than ret < 0?
Food for thought, but doesn't impact the quality of this patch.
+ if (VIR_SOCKET_HAS_ADDR(&network->def->ipAddress)) {
+ int prefix = virNetworkDefPrefix(network->def);
+
+ if (prefix < 0) {
+ networkReportError(VIR_ERR_INTERNAL_ERROR,
+ _("bridge '%s' has an invalid netmask or IP
address"),
As long as you're touching this, collapse the two spaces in the message
to one.
ACK with that nit fixed.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
12:58 p.m.
New subject: [libvirt] [PATCHv2 08/13] make the <dhcp> element optional in network.rng
In practice this has always been optional, but the RNG has shown it as
mandatory, and since all the examples for make check had it, it was
never noticed. One of the existing test cases has been changed to
check for this.
I also noticed that the dhcp/host/ip was still defined as <text/>,
but should really be <ref name='ipv4-addr'/>
---
No changes from V1.
docs/schemas/network.rng | 52 ++++++++++++++-------------
tests/networkxml2xmlin/routed-network.xml | 3 --
tests/networkxml2xmlout/routed-network.xml | 3 --
3 files changed, 27 insertions(+), 31 deletions(-)
diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng
index 1daa30e..ac13af2 100644
--- a/docs/schemas/network.rng
+++ b/docs/schemas/network.rng
@@ -95,31 +95,33 @@
<attribute name="root"><text/></attribute>
</element>
</optional>
- <!-- Define the range(s) of IP addresses that the DHCP
- server should hand out -->
- <element name="dhcp">
- <zeroOrMore>
- <element name="range">
- <attribute name="start"><ref
name="ipv4-addr"/></attribute>
- <attribute name="end"><ref
name="ipv4-addr"/></attribute>
- </element>
- </zeroOrMore>
- <zeroOrMore>
- <element name="host">
- <attribute name="mac"><ref
name="mac-addr"/></attribute>
- <attribute name="name"><text/></attribute>
- <attribute name="ip"><text/></attribute>
- </element>
- </zeroOrMore>
- <optional>
- <element name="bootp">
- <attribute name="file"><text/></attribute>
- <optional>
- <attribute
name="server"><text/></attribute>
- </optional>
- </element>
- </optional>
- </element>
+ <optional>
+ <!-- Define the range(s) of IP addresses that the DHCP
+ server should hand out -->
+ <element name="dhcp">
+ <zeroOrMore>
+ <element name="range">
+ <attribute name="start"><ref
name="ipv4-addr"/></attribute>
+ <attribute name="end"><ref
name="ipv4-addr"/></attribute>
+ </element>
+ </zeroOrMore>
+ <zeroOrMore>
+ <element name="host">
+ <attribute name="mac"><ref
name="mac-addr"/></attribute>
+ <attribute
name="name"><text/></attribute>
+ <attribute name="ip"><ref
name="ipv4-addr"/></attribute>
+ </element>
+ </zeroOrMore>
+ <optional>
+ <element name="bootp">
+ <attribute
name="file"><text/></attribute>
+ <optional>
+ <attribute
name="server"><text/></attribute>
+ </optional>
+ </element>
+ </optional>
+ </element>
+ </optional>
</element>
</optional>
</interleave>
diff --git a/tests/networkxml2xmlin/routed-network.xml
b/tests/networkxml2xmlin/routed-network.xml
index 824ad75..6634ee8 100644
--- a/tests/networkxml2xmlin/routed-network.xml
+++ b/tests/networkxml2xmlin/routed-network.xml
@@ -4,8 +4,5 @@
<bridge name="virbr1" />
<forward mode="route" dev="eth1"/>
<ip address="192.168.122.1" netmask="255.255.255.0">
- <dhcp>
- <range start="192.168.122.2" end="192.168.122.254" />
- </dhcp>
</ip>
</network>
diff --git a/tests/networkxml2xmlout/routed-network.xml
b/tests/networkxml2xmlout/routed-network.xml
index fa36c08..8f11166 100644
--- a/tests/networkxml2xmlout/routed-network.xml
+++ b/tests/networkxml2xmlout/routed-network.xml
@@ -4,8 +4,5 @@
<forward dev='eth1' mode='route'/>
<bridge name='virbr1' stp='on' delay='0' />
<ip address='192.168.122.1' netmask='255.255.255.0'>
- <dhcp>
- <range start='192.168.122.2' end='192.168.122.254' />
- </dhcp>
</ip>
</network>
--
1.7.3.4
5:38 p.m.
New subject: [libvirt] [PATCHv2 08/13] make the <dhcp> element optional in network.rng
On 12/22/2010 11:58 AM, Laine Stump wrote:
In practice this has always been optional, but the RNG has shown it
as
mandatory, and since all the examples for make check had it, it was
never noticed. One of the existing test cases has been changed to
check for this.
I also noticed that the dhcp/host/ip was still defined as <text/>,
but should really be <ref name='ipv4-addr'/>
ACK.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
12:58 p.m.
New subject: [libvirt] [PATCHv2 09/13] Change virtual network XML parsing/formatting to support IPv6
This commit adds support for IPv6 parsing and formatting to the
virtual network XML parser, including moving around data definitions
to allow for multiple <ip> elements on a single network, but only
changes the consumers of this API to accomodate for the changes in
API/structure, not to add any actual IPv6 functionality. That will
come in a later patch - this patch attempts to maintain the same final
functionality in both drivers that use the network XML parser - vbox
and "bridge" (the Linux bridge-based driver used by the qemu
hypervisor driver).
* src/libvirt_private.syms: Add new private API functions.
* src/conf/network_conf.[ch]: Change C data structure and
parsing/formatting.
* src/network/bridge_driver.c: Update to use new parser/formatter.
* src/vbox/vbox_tmpl.c: update to use new parser/formatter
* docs/schemas/network.rng: changes to the schema -
* there can now be more than one <ip> element.
* ip address is now an ip-addr (ipv4 or ipv6) rather than ipv4-addr
* new optional "prefix" attribute that can be used in place of
"netmask"
* new optional "family" attribute - "ipv4" or "ipv6"
(will default to ipv4)
* define data types for the above
* tests/networkxml2xml(in|out)/nat-network.xml: add multiple <ip> elements
(including IPv6) to a single network definition to verify they are being
correctly parsed and formatted.
---
V2 Changes:
* prefix in virNetworkIpDef struct is now unsigned int rather than int.
* caught a memory leak in an error patch of virNetworkIPParseXML().
docs/schemas/network.rng | 41 +++-
src/conf/network_conf.c | 453 +++++++++++++++++++++----------
src/conf/network_conf.h | 50 +++-
src/libvirt_private.syms | 5 +-
src/network/bridge_driver.c | 248 ++++++++++--------
src/vbox/vbox_tmpl.c | 81 ++++--
tests/networkxml2xmlin/nat-network.xml | 8 +
tests/networkxml2xmlout/nat-network.xml | 8 +
8 files changed, 590 insertions(+), 304 deletions(-)
diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng
index ac13af2..bc34ddc 100644
--- a/docs/schemas/network.rng
+++ b/docs/schemas/network.rng
@@ -80,15 +80,21 @@
</optional>
<!-- <ip> element -->
- <optional>
+ <zeroOrMore>
<!-- The IP element sets up NAT'ing and an optional DHCP server
local to the host. -->
<element name="ip">
<optional>
- <attribute name="address"><ref
name="ipv4-addr"/></attribute>
+ <attribute name="address"><ref
name="ip-addr"/></attribute>
+ </optional>
+ <optional>
+ <choice>
+ <attribute name="netmask"><ref
name="ipv4-addr"/></attribute>
+ <attribute name="prefix"><ref
name="ip-prefix"/></attribute>
+ </choice>
</optional>
<optional>
- <attribute name="netmask"><ref
name="ipv4-addr"/></attribute>
+ <attribute name="family"><ref
name="addr-family"/></attribute>
</optional>
<optional>
<element name="tftp">
@@ -123,7 +129,7 @@
</element>
</optional>
</element>
- </optional>
+ </zeroOrMore>
</interleave>
</element>
</define>
@@ -135,6 +141,33 @@
</data>
</define>
+ <!-- Based on http://blog.mes-stats.fr/2008/10/09/regex-ipv4-et-ipv6 -->
+ <define name='ipv6-addr'>
+ <data type='string'>
+ <!-- To understand this better, take apart the toplevel '|'s -->
+ <param
name="pattern">(([0-9A-Fa-f]{1,4}:){7}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){6}:[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){5}:([0-9A-Fa-f]{1,4}:)?[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){4}:([0-9A-Fa-f]{1,4}:){0,2}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){3}:([0-9A-Fa-f]{1,4}:){0,3}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){2}:([0-9A-Fa-f]{1,4}:){0,4}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){6}((((25[0-5])|(1[0-9]{2})|(2[0-4][0-9])|([0-9]{1,2})))\.){3}(((25[0-5])|(1[0-9]{2})|(2[0-4][0-9])|([0-9]{1,2}))))|(([0-9A-Fa-f]{1,4}:){0,5}:((((25[0-5])|(1[0-9]{2})|(2[0-4][0-9])|([0-9]{1,2})))\.){3}(((25[0-5])|(1[0-9]{2})|(2[0-4][0-9])|([0-9]{1,2}))))|(::([0-9A-Fa-f]{1,4}:){0,5}((((25[0-5])|(1[0-9]{2})|(2[0-4][0-9])|([0-9]{1,2})))\.){3}(((25[0-5])|(1[0-9]{2})|(2[0-4][0-9])|([0-9]{1,2}))))|([0-9A-Fa-f]{1,4}::([0-9A-Fa-f]{1,4}:){0,5}[0-9A-Fa-f]{1,4})|(::([0-9A-Fa-f]{1,4}:){0,6}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){1,7}:)</param>
+ </data>
+ </define>
+
+ <define name='ip-addr'>
+ <choice>
+ <ref name='ipv4-addr'/>
+ <ref name='ipv6-addr'/>
+ </choice>
+ </define>
+
+ <define name='ip-prefix'>
+ <data type='unsignedInt'>
+ <param name="maxInclusive">128</param>
+ </data>
+ </define>
+
+ <define name='addr-family'>
+ <data type='string'>
+ <param name="pattern">(ipv4)|(ipv6)</param>
+ </data>
+ </define>
+
<!-- a 6 byte MAC address in ASCII-hex format, eg "12:34:56:78:9A:BC"
-->
<define name='mac-addr'>
<data type='string'>
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index b17c708..86442ce 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -87,9 +87,26 @@ virNetworkObjPtr virNetworkFindByName(const virNetworkObjListPtr nets,
}
+static void virNetworkIpDefClear(virNetworkIpDefPtr def)
+{
+ int ii;
+
+ VIR_FREE(def->family);
+ VIR_FREE(def->ranges);
+
+ for (ii = 0 ; ii < def->nhosts && def->hosts ; ii++) {
+ VIR_FREE(def->hosts[ii].mac);
+ VIR_FREE(def->hosts[ii].name);
+ }
+
+ VIR_FREE(def->hosts);
+ VIR_FREE(def->tftproot);
+ VIR_FREE(def->bootfile);
+}
+
void virNetworkDefFree(virNetworkDefPtr def)
{
- int i;
+ int ii;
if (!def)
return;
@@ -99,16 +116,10 @@ void virNetworkDefFree(virNetworkDefPtr def)
VIR_FREE(def->forwardDev);
VIR_FREE(def->domain);
- VIR_FREE(def->ranges);
-
- for (i = 0 ; i < def->nhosts && def->hosts ; i++) {
- VIR_FREE(def->hosts[i].mac);
- VIR_FREE(def->hosts[i].name);
+ for (ii = 0 ; ii < def->nips && def->ips ; ii++) {
+ virNetworkIpDefClear(&def->ips[ii]);
}
- VIR_FREE(def->hosts);
-
- VIR_FREE(def->tftproot);
- VIR_FREE(def->bootfile);
+ VIR_FREE(def->ips);
VIR_FREE(def);
}
@@ -207,21 +218,48 @@ void virNetworkRemoveInactive(virNetworkObjListPtr nets,
}
}
+/* return ips[index], or NULL if there aren't enough ips */
+virNetworkIpDefPtr
+virNetworkDefGetIpByIndex(const virNetworkDefPtr def,
+ int family, int n)
+{
+ int ii;
+
+ if (!def->ips || n >= def->nips)
+ return NULL;
+
+ if (family == AF_UNSPEC) {
+ return &def->ips[n];
+ }
+
+ /* find the nth ip of type "family" */
+ for (ii = 0; ii < def->nips; ii++) {
+ if (VIR_SOCKET_IS_FAMILY(&def->ips[ii].address, family)
+ && (n-- <= 0)) {
+ return &def->ips[ii];
+ }
+ }
+ /* failed to find enough of the right family */
+ return NULL;
+}
+
/* return number of 1 bits in netmask for the network's ipAddress,
* or -1 on error
*/
-int virNetworkDefPrefix(const virNetworkDefPtr def)
+int virNetworkIpDefPrefix(const virNetworkIpDefPtr def)
{
- if (VIR_SOCKET_HAS_ADDR(&def->netmask)) {
+ if (def->prefix > 0) {
+ return def->prefix;
+ } else if (VIR_SOCKET_HAS_ADDR(&def->netmask)) {
return virSocketGetNumNetmaskBits(&def->netmask);
- } else if (VIR_SOCKET_IS_FAMILY(&def->ipAddress, AF_INET)) {
+ } else if (VIR_SOCKET_IS_FAMILY(&def->address, AF_INET)) {
/* Return the natural prefix for the network's ip address.
* On Linux we could use the IN_CLASSx() macros, but those
* aren't guaranteed on all platforms, so we just deal with
* the bits ourselves.
*/
const unsigned char *octets
- = (const unsigned char
*)(&def->ipAddress.data.inet4.sin_addr.s_addr);
+ = (const unsigned char *)(&def->address.data.inet4.sin_addr.s_addr);
if ((octets[0] & 0x80) == 0) {
/* Class A network */
return 8;
@@ -233,6 +271,8 @@ int virNetworkDefPrefix(const virNetworkDefPtr def)
return 24;
}
return -1;
+ } else if (VIR_SOCKET_IS_FAMILY(&def->address, AF_INET6)) {
+ return 64;
}
return -1;
}
@@ -241,22 +281,23 @@ int virNetworkDefPrefix(const virNetworkDefPtr def)
* definition, based on either the definition's netmask, or its
* prefix. Return -1 on error (and set the netmask family to AF_UNSPEC)
*/
-int virNetworkDefNetmask(const virNetworkDefPtr def,
- virSocketAddrPtr netmask)
+int virNetworkIpDefNetmask(const virNetworkIpDefPtr def,
+ virSocketAddrPtr netmask)
{
if (VIR_SOCKET_IS_FAMILY(&def->netmask, AF_INET)) {
*netmask = def->netmask;
return 0;
}
- return virSocketAddrPrefixToNetmask(virNetworkDefPrefix(def), netmask,
- VIR_SOCKET_FAMILY(&def->ipAddress));
+ return virSocketAddrPrefixToNetmask(virNetworkIpDefPrefix(def), netmask,
+ VIR_SOCKET_FAMILY(&def->address));
}
static int
-virNetworkDHCPRangeDefParseXML(virNetworkDefPtr def,
- xmlNodePtr node) {
+virNetworkDHCPRangeDefParseXML(virNetworkIpDefPtr def,
+ xmlNodePtr node)
+{
xmlNodePtr cur;
@@ -390,33 +431,147 @@ virNetworkDHCPRangeDefParseXML(virNetworkDefPtr def,
}
static int
-virNetworkIPParseXML(virNetworkDefPtr def,
- xmlNodePtr node) {
- xmlNodePtr cur;
+virNetworkIPParseXML(const char *networkName,
+ virNetworkIpDefPtr def,
+ xmlNodePtr node,
+ xmlXPathContextPtr ctxt)
+{
+ /*
+ * virNetworkIpDef object is already allocated as part of an array.
+ * On failure clear it out, but don't free it.
+ */
- cur = node->children;
- while (cur != NULL) {
- if (cur->type == XML_ELEMENT_NODE &&
- xmlStrEqual(cur->name, BAD_CAST "dhcp")) {
- int result = virNetworkDHCPRangeDefParseXML(def, cur);
- if (result)
- return result;
+ xmlNodePtr cur, save;
+ char *address = NULL, *netmask = NULL;
+ unsigned long prefix;
+ int result = -1;
+
+ save = ctxt->node;
+ ctxt->node = node;
+
+ /* grab raw data from XML */
+ def->family = virXPathString("string(./@family)", ctxt);
+ address = virXPathString("string(./@address)", ctxt);
+ if (virXPathULong("string(./@prefix)", ctxt, &prefix) < 0)
+ def->prefix = 0;
+ else
+ def->prefix = prefix;
+
+ netmask = virXPathString("string(./@netmask)", ctxt);
+
+ if (address) {
+ if (virSocketParseAddr(address, &def->address, AF_UNSPEC) < 0) {
+ virNetworkReportError(VIR_ERR_XML_ERROR,
+ _("Bad address '%s' in definition of
network '%s'"),
+ address, networkName);
+ goto error;
+ }
- } else if (cur->type == XML_ELEMENT_NODE &&
- xmlStrEqual(cur->name, BAD_CAST "tftp")) {
- char *root;
+ }
- if (!(root = virXMLPropString(cur, "root"))) {
- cur = cur->next;
- continue;
+ /* validate family vs. address */
+ if (def->family == NULL) {
+ if (!(VIR_SOCKET_IS_FAMILY(&def->address, AF_INET) ||
+ VIR_SOCKET_IS_FAMILY(&def->address, AF_UNSPEC))) {
+ virNetworkReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("no family specified for non-IPv4 address
address '%s' in network '%s'"),
+ address, networkName);
+ goto error;
+ }
+ } else if (STREQ(def->family, "ipv4")) {
+ if (!VIR_SOCKET_IS_FAMILY(&def->address, AF_INET)) {
+ virNetworkReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("family 'ipv4' specified for non-IPv4
address '%s' in network '%s'"),
+ address, networkName);
+ goto error;
+ }
+ } else if (STREQ(def->family, "ipv6")) {
+ if (!VIR_SOCKET_IS_FAMILY(&def->address, AF_INET6)) {
+ virNetworkReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("family 'ipv6' specified for non-IPv6
address '%s' in network '%s'"),
+ address, networkName);
+ goto error;
+ }
+ } else {
+ virNetworkReportError(VIR_ERR_XML_ERROR,
+ _("Unrecognized family '%s' in definition of
network '%s'"),
+ def->family, networkName);
+ goto error;
+ }
+
+ /* parse/validate netmask */
+ if (netmask) {
+ if (address == NULL) {
+ /* netmask is meaningless without an address */
+ virNetworkReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("netmask specified without address in network
'%s'"),
+ networkName);
+ goto error;
+ }
+
+ if (!VIR_SOCKET_IS_FAMILY(&def->address, AF_INET)) {
+ virNetworkReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("netmask not supported for address '%s'
in network '%s' (IPv4 only)"),
+ address, networkName);
+ goto error;
+ }
+
+ if (def->prefix > 0) {
+ /* can't have both netmask and prefix at the same time */
+ virNetworkReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("network '%s' has prefix='%u'
but no address"),
+ networkName, def->prefix);
+ goto error;
+ }
+
+ if (virSocketParseAddr(netmask, &def->netmask, AF_UNSPEC) < 0)
+ goto error;
+
+ if (!VIR_SOCKET_IS_FAMILY(&def->netmask, AF_INET)) {
+ virNetworkReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("network '%s' has invalid netmask
'%s' for address '%s' (both must be IPv4)"),
+ networkName, netmask, address);
+ goto error;
+ }
+ }
+
+ if (VIR_SOCKET_IS_FAMILY(&def->address, AF_INET)) {
+ /* parse IPv4-related info */
+ cur = node->children;
+ while (cur != NULL) {
+ if (cur->type == XML_ELEMENT_NODE &&
+ xmlStrEqual(cur->name, BAD_CAST "dhcp")) {
+ result = virNetworkDHCPRangeDefParseXML(def, cur);
+ if (result)
+ goto error;
+
+ } else if (cur->type == XML_ELEMENT_NODE &&
+ xmlStrEqual(cur->name, BAD_CAST "tftp")) {
+ char *root;
+
+ if (!(root = virXMLPropString(cur, "root"))) {
+ cur = cur->next;
+ continue;
+ }
+
+ def->tftproot = (char *)root;
}
- def->tftproot = root;
+ cur = cur->next;
}
+ }
- cur = cur->next;
+ result = 0;
+
+error:
+ if (result < 0) {
+ virNetworkIpDefClear(def);
}
- return 0;
+ VIR_FREE(address);
+ VIR_FREE(netmask);
+
+ ctxt->node = save;
+ return result;
}
static virNetworkDefPtr
@@ -424,8 +579,8 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
{
virNetworkDefPtr def;
char *tmp;
- char *ipAddress;
- char *netmask;
+ xmlNodePtr *ipNodes = NULL;
+ int nIps;
if (VIR_ALLOC(def) < 0) {
virReportOOMError();
@@ -469,44 +624,32 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
if (virXPathULong("string(./bridge[1]/@delay)", ctxt, &def->delay)
< 0)
def->delay = 0;
- ipAddress = virXPathString("string(./ip[1]/@address)", ctxt);
- if (ipAddress) {
- xmlNodePtr ip;
-
- if (virSocketParseAddr(ipAddress, &def->ipAddress, AF_UNSPEC) < 0)
- goto error;
+ nIps = virXPathNodeSet("./ip", ctxt, &ipNodes);
+ if (nIps > 0) {
+ int ii;
- /* XXX someday we want IPv6, so will need to relax this */
- if (!VIR_SOCKET_IS_FAMILY(&def->ipAddress, AF_INET)) {
- virNetworkReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- "%s", _("Only IPv4 addresses are
supported"));
+ /* allocate array to hold all the addrs */
+ if (VIR_ALLOC_N(def->ips, nIps) < 0) {
+ virReportOOMError();
goto error;
}
-
- if ((ip = virXPathNode("./ip[1]", ctxt)) &&
- virNetworkIPParseXML(def, ip) < 0)
- goto error;
- }
- VIR_FREE(ipAddress);
-
- netmask = virXPathString("string(./ip[1]/@netmask)", ctxt);
- if (netmask) {
-
- if (virSocketParseAddr(netmask, &def->netmask, AF_UNSPEC) < 0)
- goto error;
-
- /* XXX someday we want IPv6, so will need to relax this */
- if (!VIR_SOCKET_IS_FAMILY(&def->netmask, AF_INET)) {
- virNetworkReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- "%s", _("Only IPv4 addresses are
supported"));
- goto error;
+ /* parse each addr */
+ for (ii = 0; ii < nIps; ii++) {
+ int ret = virNetworkIPParseXML(def->name, &def->ips[ii],
+ ipNodes[ii], ctxt);
+ if (ret < 0)
+ goto error;
+ def->nips++;
}
}
- VIR_FREE(netmask);
-
/* IPv4 forwarding setup */
if (virXPathBoolean("count(./forward) > 0", ctxt)) {
+ if (def->nips == 0) {
+ virNetworkReportError(VIR_ERR_INTERNAL_ERROR,
+ "%s", _("Forwarding requested, but no IP
address provided"));
+ goto error;
+ }
tmp = virXPathString("string(./forward[1]/@mode)", ctxt);
if (tmp) {
if ((def->forwardType = virNetworkForwardTypeFromString(tmp)) < 0) {
@@ -585,11 +728,101 @@ cleanup:
return def;
}
+static int
+virNetworkIpDefFormat(virBufferPtr buf,
+ const virNetworkIpDefPtr def)
+{
+ int result = -1;
+
+ virBufferAddLit(buf, " <ip");
+
+ if (def->family) {
+ virBufferVSprintf(buf, " family='%s'", def->family);
+ }
+ if (VIR_SOCKET_HAS_ADDR(&def->address)) {
+ char *addr = virSocketFormatAddr(&def->address);
+ if (!addr)
+ goto error;
+ virBufferVSprintf(buf, " address='%s'", addr);
+ VIR_FREE(addr);
+ }
+ if (VIR_SOCKET_HAS_ADDR(&def->netmask)) {
+ char *addr = virSocketFormatAddr(&def->netmask);
+ if (!addr)
+ goto error;
+ virBufferVSprintf(buf, " netmask='%s'", addr);
+ VIR_FREE(addr);
+ }
+ if (def->prefix > 0) {
+ virBufferVSprintf(buf," prefix='%u'", def->prefix);
+ }
+ virBufferAddLit(buf, ">\n");
+
+ if (def->tftproot) {
+ virBufferEscapeString(buf, " <tftp root='%s' />\n",
+ def->tftproot);
+ }
+ if ((def->nranges || def->nhosts)) {
+ int ii;
+ virBufferAddLit(buf, " <dhcp>\n");
+ for (ii = 0 ; ii < def->nranges ; ii++) {
+ char *saddr = virSocketFormatAddr(&def->ranges[ii].start);
+ if (!saddr)
+ goto error;
+ char *eaddr = virSocketFormatAddr(&def->ranges[ii].end);
+ if (!eaddr) {
+ VIR_FREE(saddr);
+ goto error;
+ }
+ virBufferVSprintf(buf, " <range start='%s'
end='%s' />\n",
+ saddr, eaddr);
+ VIR_FREE(saddr);
+ VIR_FREE(eaddr);
+ }
+ for (ii = 0 ; ii < def->nhosts ; ii++) {
+ virBufferAddLit(buf, " <host ");
+ if (def->hosts[ii].mac)
+ virBufferVSprintf(buf, "mac='%s' ",
def->hosts[ii].mac);
+ if (def->hosts[ii].name)
+ virBufferVSprintf(buf, "name='%s' ",
def->hosts[ii].name);
+ if (VIR_SOCKET_HAS_ADDR(&def->hosts[ii].ip)) {
+ char *ipaddr = virSocketFormatAddr(&def->hosts[ii].ip);
+ if (!ipaddr)
+ goto error;
+ virBufferVSprintf(buf, "ip='%s' ", ipaddr);
+ VIR_FREE(ipaddr);
+ }
+ virBufferAddLit(buf, "/>\n");
+ }
+ if (def->bootfile) {
+ virBufferEscapeString(buf, " <bootp file='%s' ",
+ def->bootfile);
+ if (VIR_SOCKET_HAS_ADDR(&def->bootserver)) {
+ char *ipaddr = virSocketFormatAddr(&def->bootserver);
+ if (!ipaddr)
+ goto error;
+ virBufferEscapeString(buf, "server='%s' ", ipaddr);
+ VIR_FREE(ipaddr);
+ }
+ virBufferAddLit(buf, "/>\n");
+ }
+
+ virBufferAddLit(buf, " </dhcp>\n");
+ }
+
+ virBufferAddLit(buf, " </ip>\n");
+
+ result = 0;
+error:
+ return result;
+}
+
char *virNetworkDefFormat(const virNetworkDefPtr def)
{
virBuffer buf = VIR_BUFFER_INITIALIZER;
unsigned char *uuid;
char uuidstr[VIR_UUID_STRING_BUFLEN];
+ int ii;
virBufferAddLit(&buf, "<network>\n");
virBufferEscapeString(&buf, " <name>%s</name>\n",
def->name);
@@ -621,81 +854,9 @@ char *virNetworkDefFormat(const virNetworkDefPtr def)
if (def->domain)
virBufferVSprintf(&buf, " <domain name='%s'/>\n",
def->domain);
- if (VIR_SOCKET_HAS_ADDR(&def->ipAddress) ||
- VIR_SOCKET_HAS_ADDR(&def->netmask)) {
- virBufferAddLit(&buf, " <ip");
-
- if (VIR_SOCKET_HAS_ADDR(&def->ipAddress)) {
- char *addr = virSocketFormatAddr(&def->ipAddress);
- if (!addr)
- goto error;
- virBufferVSprintf(&buf, " address='%s'", addr);
- VIR_FREE(addr);
- }
-
- if (VIR_SOCKET_HAS_ADDR(&def->netmask)) {
- char *addr = virSocketFormatAddr(&def->netmask);
- if (!addr)
- goto error;
- virBufferVSprintf(&buf, " netmask='%s'", addr);
- VIR_FREE(addr);
- }
-
- virBufferAddLit(&buf, ">\n");
-
- if (def->tftproot) {
- virBufferEscapeString(&buf, " <tftp root='%s'
/>\n",
- def->tftproot);
- }
- if ((def->nranges || def->nhosts)) {
- int i;
- virBufferAddLit(&buf, " <dhcp>\n");
- for (i = 0 ; i < def->nranges ; i++) {
- char *saddr = virSocketFormatAddr(&def->ranges[i].start);
- if (!saddr)
- goto error;
- char *eaddr = virSocketFormatAddr(&def->ranges[i].end);
- if (!eaddr) {
- VIR_FREE(saddr);
- goto error;
- }
- virBufferVSprintf(&buf, " <range start='%s'
end='%s' />\n",
- saddr, eaddr);
- VIR_FREE(saddr);
- VIR_FREE(eaddr);
- }
- for (i = 0 ; i < def->nhosts ; i++) {
- virBufferAddLit(&buf, " <host ");
- if (def->hosts[i].mac)
- virBufferVSprintf(&buf, "mac='%s' ",
def->hosts[i].mac);
- if (def->hosts[i].name)
- virBufferVSprintf(&buf, "name='%s' ",
def->hosts[i].name);
- if (VIR_SOCKET_HAS_ADDR(&def->hosts[i].ip)) {
- char *ipaddr = virSocketFormatAddr(&def->hosts[i].ip);
- if (!ipaddr)
- goto error;
- virBufferVSprintf(&buf, "ip='%s' ", ipaddr);
- VIR_FREE(ipaddr);
- }
- virBufferAddLit(&buf, "/>\n");
- }
- if (def->bootfile) {
- virBufferEscapeString(&buf, " <bootp file='%s'
",
- def->bootfile);
- if (VIR_SOCKET_HAS_ADDR(&def->bootserver)) {
- char *ipaddr = virSocketFormatAddr(&def->bootserver);
- if (!ipaddr)
- goto error;
- virBufferEscapeString(&buf, "server='%s' ",
ipaddr);
- VIR_FREE(ipaddr);
- }
- virBufferAddLit(&buf, "/>\n");
- }
-
- virBufferAddLit(&buf, " </dhcp>\n");
- }
-
- virBufferAddLit(&buf, " </ip>\n");
+ for (ii = 0; ii < def->nips; ii++) {
+ if (virNetworkIpDefFormat(&buf, &def->ips[ii]) < 0)
+ goto error;
}
virBufferAddLit(&buf, "</network>\n");
diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
index a922d28..a51794d 100644
--- a/src/conf/network_conf.h
+++ b/src/conf/network_conf.h
@@ -56,6 +56,33 @@ struct _virNetworkDHCPHostDef {
virSocketAddr ip;
};
+typedef struct _virNetworkIpDef virNetworkIpDef;
+typedef virNetworkIpDef *virNetworkIpDefPtr;
+struct _virNetworkIpDef {
+ char *family; /* ipv4 or ipv6 - default is ipv4 */
+ virSocketAddr address; /* Bridge IP address */
+
+ /* The first two items below are taken directly from the XML (one
+ * or the other is given, but not both) and the 3rd is derived
+ * from the first two. When formatting XML, always use netMasktStr
+ * if it's non-NULL, but when using netmask in other code, use
+ * netmask, as it will automatically take into account prefix or
+ * natural netmasks (when no netmask or prefix is specified).
+ */
+ unsigned int prefix; /* ipv6 - only prefix allowed */
+ virSocketAddr netmask; /* ipv4 - either netmask or prefix specified */
+
+ unsigned int nranges; /* Zero or more dhcp ranges */
+ virNetworkDHCPRangeDefPtr ranges;
+
+ unsigned int nhosts; /* Zero or more dhcp hosts */
+ virNetworkDHCPHostDefPtr hosts;
+
+ char *tftproot;
+ char *bootfile;
+ virSocketAddr bootserver;
+ };
+
typedef struct _virNetworkDef virNetworkDef;
typedef virNetworkDef *virNetworkDefPtr;
struct _virNetworkDef {
@@ -70,18 +97,8 @@ struct _virNetworkDef {
int forwardType; /* One of virNetworkForwardType constants */
char *forwardDev; /* Destination device for forwarding */
- virSocketAddr ipAddress; /* Bridge IP address */
- virSocketAddr netmask;
-
- unsigned int nranges; /* Zero or more dhcp ranges */
- virNetworkDHCPRangeDefPtr ranges;
-
- unsigned int nhosts; /* Zero or more dhcp hosts */
- virNetworkDHCPHostDefPtr hosts;
-
- char *tftproot;
- char *bootfile;
- virSocketAddr bootserver;
+ int nips;
+ virNetworkIpDefPtr ips; /* ptr to array of IP addresses on this network */
};
typedef struct _virNetworkObj virNetworkObj;
@@ -133,9 +150,12 @@ virNetworkDefPtr virNetworkDefParseNode(xmlDocPtr xml,
char *virNetworkDefFormat(const virNetworkDefPtr def);
-int virNetworkDefPrefix(const virNetworkDefPtr def);
-int virNetworkDefNetmask(const virNetworkDefPtr def,
- virSocketAddrPtr netmask);
+virNetworkIpDefPtr
+virNetworkDefGetIpByIndex(const virNetworkDefPtr def,
+ int family, int n);
+int virNetworkIpDefPrefix(const virNetworkIpDefPtr def);
+int virNetworkIpDefNetmask(const virNetworkIpDefPtr def,
+ virSocketAddrPtr netmask);
int virNetworkSaveXML(const char *configDir,
virNetworkDefPtr def,
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index d9ba7b1..6cb8270 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -578,15 +578,16 @@ virNetworkAssignDef;
virNetworkConfigFile;
virNetworkDefFormat;
virNetworkDefFree;
-virNetworkDefNetmask;
+virNetworkDefGetIpByIndex;
virNetworkDefParseFile;
virNetworkDefParseNode;
virNetworkDefParseString;
-virNetworkDefPrefix;
virNetworkDeleteConfig;
virNetworkFindByName;
virNetworkFindByUUID;
virNetworkLoadAllConfigs;
+virNetworkIpDefNetmask;
+virNetworkIpDefPrefix;
virNetworkObjIsDuplicate;
virNetworkObjListFree;
virNetworkObjLock;
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index ba9ff96..0f8b050 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -145,8 +145,7 @@ networkFindActiveConfigs(struct network_driver *driver) {
obj->active = 1;
/* Finally try and read dnsmasq pid if any */
- if ((VIR_SOCKET_HAS_ADDR(&obj->def->ipAddress) ||
- obj->def->nranges) &&
+ if (obj->def->ips && (obj->def->nips > 0) &&
virFileReadPid(NETWORK_PID_DIR, obj->def->name,
&obj->dnsmasqPid) == 0) {
@@ -366,7 +365,7 @@ networkShutdown(void) {
static int
-networkSaveDnsmasqHostsfile(virNetworkObjPtr network,
+networkSaveDnsmasqHostsfile(virNetworkIpDefPtr ipdef,
dnsmasqContext *dctx,
bool force)
{
@@ -375,8 +374,8 @@ networkSaveDnsmasqHostsfile(virNetworkObjPtr network,
if (! force && virFileExists(dctx->hostsfile->path))
return 0;
- for (i = 0 ; i < network->def->nhosts ; i++) {
- virNetworkDHCPHostDefPtr host = &(network->def->hosts[i]);
+ for (i = 0; i < ipdef->nhosts; i++) {
+ virNetworkDHCPHostDefPtr host = &(ipdef->hosts[i]);
if ((host->mac) && VIR_SOCKET_HAS_ADDR(&host->ip))
dnsmasqAddDhcpHost(dctx, host->mac, &host->ip, host->name);
}
@@ -390,13 +389,14 @@ networkSaveDnsmasqHostsfile(virNetworkObjPtr network,
static int
networkBuildDnsmasqArgv(virNetworkObjPtr network,
+ virNetworkIpDefPtr ipdef,
const char *pidfile,
virCommandPtr cmd) {
int r, ret = -1;
int nbleases = 0;
char *bridgeaddr;
- if (!(bridgeaddr = virSocketFormatAddr(&network->def->ipAddress)))
+ if (!(bridgeaddr = virSocketFormatAddr(&ipdef->address)))
goto cleanup;
/*
* NB, be careful about syntax for dnsmasq options in long format.
@@ -438,18 +438,18 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network,
* clearly not practical
*
* virCommandAddArg(cmd, "--interface");
- * virCommandAddArg(cmd, network->def->bridge);
+ * virCommandAddArg(cmd, ipdef->bridge);
*/
virCommandAddArgList(cmd,
"--listen-address", bridgeaddr,
"--except-interface", "lo",
NULL);
- for (r = 0 ; r < network->def->nranges ; r++) {
- char *saddr = virSocketFormatAddr(&network->def->ranges[r].start);
+ for (r = 0 ; r < ipdef->nranges ; r++) {
+ char *saddr = virSocketFormatAddr(&ipdef->ranges[r].start);
if (!saddr)
goto cleanup;
- char *eaddr = virSocketFormatAddr(&network->def->ranges[r].end);
+ char *eaddr = virSocketFormatAddr(&ipdef->ranges[r].end);
if (!eaddr) {
VIR_FREE(saddr);
goto cleanup;
@@ -458,8 +458,8 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network,
virCommandAddArgFormat(cmd, "%s,%s", saddr, eaddr);
VIR_FREE(saddr);
VIR_FREE(eaddr);
- nbleases += virSocketGetRange(&network->def->ranges[r].start,
- &network->def->ranges[r].end);
+ nbleases += virSocketGetRange(&ipdef->ranges[r].start,
+ &ipdef->ranges[r].end);
}
/*
@@ -467,19 +467,19 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network,
* we have to add a special --dhcp-range option to enable the service in
* dnsmasq.
*/
- if (!network->def->nranges && network->def->nhosts) {
+ if (!ipdef->nranges && ipdef->nhosts) {
virCommandAddArg(cmd, "--dhcp-range");
virCommandAddArgFormat(cmd, "%s,static", bridgeaddr);
}
- if (network->def->nranges > 0) {
+ if (ipdef->nranges > 0) {
virCommandAddArgFormat(cmd, "--dhcp-lease-max=%d", nbleases);
}
- if (network->def->nranges || network->def->nhosts)
+ if (ipdef->nranges || ipdef->nhosts)
virCommandAddArg(cmd, "--dhcp-no-override");
- if (network->def->nhosts > 0) {
+ if (ipdef->nhosts > 0) {
dnsmasqContext *dctx = dnsmasqContextNew(network->def->name,
DNSMASQ_STATE_DIR);
if (dctx == NULL) {
@@ -487,31 +487,30 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network,
goto cleanup;
}
- if (networkSaveDnsmasqHostsfile(network, dctx, false) < 0) {
+ if (networkSaveDnsmasqHostsfile(ipdef, dctx, false) < 0) {
virCommandAddArgPair(cmd, "--dhcp-hostsfile",
dctx->hostsfile->path);
}
dnsmasqContextFree(dctx);
}
- if (network->def->tftproot) {
+ if (ipdef->tftproot) {
virCommandAddArgList(cmd, "--enable-tftp",
- "--tftp-root", network->def->tftproot,
+ "--tftp-root", ipdef->tftproot,
NULL);
}
- if (network->def->bootfile) {
-
+ if (ipdef->bootfile) {
virCommandAddArg(cmd, "--dhcp-boot");
- if (VIR_SOCKET_HAS_ADDR(&network->def->bootserver)) {
- char *bootserver = virSocketFormatAddr(&network->def->bootserver);
+ if (VIR_SOCKET_HAS_ADDR(&ipdef->bootserver)) {
+ char *bootserver = virSocketFormatAddr(&ipdef->bootserver);
if (!bootserver)
goto cleanup;
virCommandAddArgFormat(cmd, "%s%s%s",
- network->def->bootfile, ",,",
bootserver);
+ ipdef->bootfile, ",,", bootserver);
VIR_FREE(bootserver);
} else {
- virCommandAddArg(cmd, network->def->bootfile);
+ virCommandAddArg(cmd, ipdef->bootfile);
}
}
@@ -521,9 +520,9 @@ cleanup:
return ret;
}
-
static int
-dhcpStartDhcpDaemon(virNetworkObjPtr network)
+dhcpStartDhcpDaemon(virNetworkObjPtr network,
+ virNetworkIpDefPtr ipdef)
{
virCommandPtr cmd = NULL;
char *pidfile = NULL;
@@ -531,7 +530,7 @@ dhcpStartDhcpDaemon(virNetworkObjPtr network)
network->dnsmasqPid = -1;
- if (!VIR_SOCKET_IS_FAMILY(&network->def->ipAddress, AF_INET)) {
+ if (!VIR_SOCKET_IS_FAMILY(&ipdef->address, AF_INET)) {
networkReportError(VIR_ERR_INTERNAL_ERROR,
"%s", _("cannot start dhcp daemon without IPv4
address for server"));
goto cleanup;
@@ -556,7 +555,7 @@ dhcpStartDhcpDaemon(virNetworkObjPtr network)
}
cmd = virCommandNew(DNSMASQ);
- if (networkBuildDnsmasqArgv(network, pidfile, cmd) < 0) {
+ if (networkBuildDnsmasqArgv(network, ipdef, pidfile, cmd) < 0) {
goto cleanup;
}
@@ -584,8 +583,10 @@ cleanup:
static int
networkAddMasqueradingIptablesRules(struct network_driver *driver,
- virNetworkObjPtr network) {
- int prefix = virNetworkDefPrefix(network->def);
+ virNetworkObjPtr network,
+ virNetworkIpDefPtr ipdef)
+{
+ int prefix = virNetworkIpDefPrefix(ipdef);
if (prefix < 0) {
networkReportError(VIR_ERR_INTERNAL_ERROR,
@@ -596,7 +597,7 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver,
/* allow forwarding packets from the bridge interface */
if (iptablesAddForwardAllowOut(driver->iptables,
- &network->def->ipAddress,
+ &ipdef->address,
prefix,
network->def->bridge,
network->def->forwardDev) < 0) {
@@ -608,7 +609,7 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver,
/* allow forwarding packets to the bridge interface if they are part of an existing
connection */
if (iptablesAddForwardAllowRelatedIn(driver->iptables,
- &network->def->ipAddress,
+ &ipdef->address,
prefix,
network->def->bridge,
network->def->forwardDev) < 0) {
@@ -643,7 +644,7 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver,
/* First the generic masquerade rule for other protocols */
if (iptablesAddForwardMasquerade(driver->iptables,
- &network->def->ipAddress,
+ &ipdef->address,
prefix,
network->def->forwardDev,
NULL) < 0) {
@@ -655,7 +656,7 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver,
/* UDP with a source port restriction */
if (iptablesAddForwardMasquerade(driver->iptables,
- &network->def->ipAddress,
+ &ipdef->address,
prefix,
network->def->forwardDev,
"udp") < 0) {
@@ -667,7 +668,7 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver,
/* TCP with a source port restriction */
if (iptablesAddForwardMasquerade(driver->iptables,
- &network->def->ipAddress,
+ &ipdef->address,
prefix,
network->def->forwardDev,
"tcp") < 0) {
@@ -681,25 +682,25 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver,
masqerr5:
iptablesRemoveForwardMasquerade(driver->iptables,
- &network->def->ipAddress,
+ &ipdef->address,
prefix,
network->def->forwardDev,
"udp");
masqerr4:
iptablesRemoveForwardMasquerade(driver->iptables,
- &network->def->ipAddress,
+ &ipdef->address,
prefix,
network->def->forwardDev,
NULL);
masqerr3:
iptablesRemoveForwardAllowRelatedIn(driver->iptables,
- &network->def->ipAddress,
+ &ipdef->address,
prefix,
network->def->bridge,
network->def->forwardDev);
masqerr2:
iptablesRemoveForwardAllowOut(driver->iptables,
- &network->def->ipAddress,
+ &ipdef->address,
prefix,
network->def->bridge,
network->def->forwardDev);
@@ -709,8 +710,9 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver,
static int
networkAddRoutingIptablesRules(struct network_driver *driver,
- virNetworkObjPtr network) {
- int prefix = virNetworkDefPrefix(network->def);
+ virNetworkObjPtr network,
+ virNetworkIpDefPtr ipdef) {
+ int prefix = virNetworkIpDefPrefix(ipdef);
if (prefix < 0) {
networkReportError(VIR_ERR_INTERNAL_ERROR,
@@ -721,7 +723,7 @@ networkAddRoutingIptablesRules(struct network_driver *driver,
/* allow routing packets from the bridge interface */
if (iptablesAddForwardAllowOut(driver->iptables,
- &network->def->ipAddress,
+ &ipdef->address,
prefix,
network->def->bridge,
network->def->forwardDev) < 0) {
@@ -733,7 +735,7 @@ networkAddRoutingIptablesRules(struct network_driver *driver,
/* allow routing packets to the bridge interface */
if (iptablesAddForwardAllowIn(driver->iptables,
- &network->def->ipAddress,
+ &ipdef->address,
prefix,
network->def->bridge,
network->def->forwardDev) < 0) {
@@ -748,7 +750,7 @@ networkAddRoutingIptablesRules(struct network_driver *driver,
routeerr2:
iptablesRemoveForwardAllowOut(driver->iptables,
- &network->def->ipAddress,
+ &ipdef->address,
prefix,
network->def->bridge,
network->def->forwardDev);
@@ -758,7 +760,8 @@ networkAddRoutingIptablesRules(struct network_driver *driver,
static int
networkAddIptablesRules(struct network_driver *driver,
- virNetworkObjPtr network) {
+ virNetworkObjPtr network,
+ virNetworkIpDefPtr ipdef) {
/* allow DHCP requests through to dnsmasq */
if (iptablesAddTcpInput(driver->iptables, network->def->bridge, 67) < 0)
{
@@ -791,7 +794,7 @@ networkAddIptablesRules(struct network_driver *driver,
}
/* allow TFTP requests through to dnsmasq */
- if (network->def->tftproot &&
+ if (ipdef && ipdef->tftproot &&
iptablesAddUdpInput(driver->iptables, network->def->bridge, 69) < 0)
{
networkReportError(VIR_ERR_SYSTEM_ERROR,
_("failed to add iptables rule to allow TFTP requests
from '%s'"),
@@ -824,29 +827,30 @@ networkAddIptablesRules(struct network_driver *driver,
goto err7;
}
-
- /* If masquerading is enabled, set up the rules*/
- if (network->def->forwardType == VIR_NETWORK_FORWARD_NAT &&
- networkAddMasqueradingIptablesRules(driver, network) < 0)
- goto err8;
- /* else if routing is enabled, set up the rules*/
- else if (network->def->forwardType == VIR_NETWORK_FORWARD_ROUTE &&
- networkAddRoutingIptablesRules(driver, network) < 0)
- goto err8;
-
- /* If we are doing local DHCP service on this network, attempt to
- * add a rule that will fixup the checksum of DHCP response
- * packets back to the guests (but report failure without
- * aborting, since not all iptables implementations support it).
- */
-
- if ((VIR_SOCKET_HAS_ADDR(&network->def->ipAddress) ||
- network->def->nranges) &&
- (iptablesAddOutputFixUdpChecksum(driver->iptables,
- network->def->bridge, 68) < 0)) {
- VIR_WARN("Could not add rule to fixup DHCP response checksums "
- "on network '%s'.", network->def->name);
- VIR_WARN0("May need to update iptables package & kernel to support
CHECKSUM rule.");
+ if (ipdef) {
+ /* If masquerading is enabled, set up the rules*/
+ if (network->def->forwardType == VIR_NETWORK_FORWARD_NAT &&
+ networkAddMasqueradingIptablesRules(driver, network, ipdef) < 0)
+ goto err8;
+ /* else if routing is enabled, set up the rules*/
+ else if (network->def->forwardType == VIR_NETWORK_FORWARD_ROUTE &&
+ networkAddRoutingIptablesRules(driver, network, ipdef) < 0)
+ goto err8;
+
+ /* If we are doing local DHCP service on this network, attempt to
+ * add a rule that will fixup the checksum of DHCP response
+ * packets back to the guests (but report failure without
+ * aborting, since not all iptables implementations support it).
+ */
+
+ if (ipdef && (VIR_SOCKET_IS_FAMILY(&ipdef->address, AF_INET) ||
+ ipdef->nranges) &&
+ (iptablesAddOutputFixUdpChecksum(driver->iptables,
+ network->def->bridge, 68) < 0)) {
+ VIR_WARN("Could not add rule to fixup DHCP response checksums "
+ "on network '%s'.", network->def->name);
+ VIR_WARN0("May need to update iptables package & kernel to support
CHECKSUM rule.");
+ }
}
return 0;
@@ -861,7 +865,7 @@ networkAddIptablesRules(struct network_driver *driver,
iptablesRemoveForwardRejectOut(driver->iptables,
network->def->bridge);
err5:
- if (network->def->tftproot) {
+ if (ipdef && ipdef->tftproot) {
iptablesRemoveUdpInput(driver->iptables, network->def->bridge, 69);
}
err4tftp:
@@ -878,14 +882,16 @@ networkAddIptablesRules(struct network_driver *driver,
static void
networkRemoveIptablesRules(struct network_driver *driver,
- virNetworkObjPtr network) {
- if (VIR_SOCKET_HAS_ADDR(&network->def->ipAddress) ||
- network->def->nranges) {
+ virNetworkObjPtr network,
+ virNetworkIpDefPtr ipdef) {
+
+ if (ipdef && (VIR_SOCKET_HAS_ADDR(&ipdef->address) ||
+ ipdef->nranges)) {
iptablesRemoveOutputFixUdpChecksum(driver->iptables,
network->def->bridge, 68);
}
- if (network->def->forwardType != VIR_NETWORK_FORWARD_NONE) {
- int prefix = virNetworkDefPrefix(network->def);
+ if (ipdef && network->def->forwardType != VIR_NETWORK_FORWARD_NONE) {
+ int prefix = virNetworkIpDefPrefix(ipdef);
if (prefix < 0) {
networkReportError(VIR_ERR_INTERNAL_ERROR,
@@ -896,34 +902,35 @@ networkRemoveIptablesRules(struct network_driver *driver,
if (network->def->forwardType == VIR_NETWORK_FORWARD_NAT) {
iptablesRemoveForwardMasquerade(driver->iptables,
- &network->def->ipAddress,
+ &ipdef->address,
prefix,
network->def->forwardDev,
"tcp");
iptablesRemoveForwardMasquerade(driver->iptables,
- &network->def->ipAddress,
+ &ipdef->address,
prefix,
network->def->forwardDev,
"udp");
iptablesRemoveForwardMasquerade(driver->iptables,
- &network->def->ipAddress,
+ &ipdef->address,
prefix,
network->def->forwardDev,
NULL);
iptablesRemoveForwardAllowRelatedIn(driver->iptables,
- &network->def->ipAddress,
+ &ipdef->address,
prefix,
network->def->bridge,
network->def->forwardDev);
- } else if (network->def->forwardType == VIR_NETWORK_FORWARD_ROUTE)
+ } else if (network->def->forwardType == VIR_NETWORK_FORWARD_ROUTE) {
iptablesRemoveForwardAllowIn(driver->iptables,
- &network->def->ipAddress,
+ &ipdef->address,
prefix,
network->def->bridge,
network->def->forwardDev);
+ }
iptablesRemoveForwardAllowOut(driver->iptables,
- &network->def->ipAddress,
+ &ipdef->address,
prefix,
network->def->bridge,
network->def->forwardDev);
@@ -932,7 +939,7 @@ error:
iptablesRemoveForwardAllowCross(driver->iptables, network->def->bridge);
iptablesRemoveForwardRejectIn(driver->iptables, network->def->bridge);
iptablesRemoveForwardRejectOut(driver->iptables, network->def->bridge);
- if (network->def->tftproot)
+ if (ipdef && ipdef->tftproot)
iptablesRemoveUdpInput(driver->iptables, network->def->bridge, 69);
iptablesRemoveUdpInput(driver->iptables, network->def->bridge, 53);
iptablesRemoveTcpInput(driver->iptables, network->def->bridge, 53);
@@ -951,10 +958,18 @@ networkReloadIptablesRules(struct network_driver *driver)
virNetworkObjLock(driver->networks.objs[i]);
if (virNetworkObjIsActive(driver->networks.objs[i])) {
- networkRemoveIptablesRules(driver, driver->networks.objs[i]);
- if (networkAddIptablesRules(driver, driver->networks.objs[i]) < 0) {
- /* failed to add but already logged */
- }
+ virNetworkIpDefPtr ipv4def;
+
+ /* Find the one allowed IPv4 ip address in the definition */
+ /* Even if none is found, we still call the functions below */
+ ipv4def = virNetworkDefGetIpByIndex(driver->networks.objs[i]->def,
+ AF_INET, 0);
+ networkRemoveIptablesRules(driver, driver->networks.objs[i],
+ ipv4def);
+ if (networkAddIptablesRules(driver, driver->networks.objs[i],
+ ipv4def) < 0) {
+ /* failed to add but already logged */
+ }
}
virNetworkObjUnlock(driver->networks.objs[i]);
@@ -1028,7 +1043,8 @@ cleanup:
* other scenarios where we can ruin host network connectivity.
* XXX: Using a proper library is preferred over parsing /proc
*/
-static int networkCheckRouteCollision(virNetworkObjPtr network)
+static int networkCheckRouteCollision(virNetworkObjPtr network,
+ virNetworkIpDefPtr ipdef)
{
int ret = -1, len;
unsigned int net_dest;
@@ -1036,18 +1052,18 @@ static int networkCheckRouteCollision(virNetworkObjPtr network)
char *cur, *buf = NULL;
enum {MAX_ROUTE_SIZE = 1024*64};
- if (!VIR_SOCKET_IS_FAMILY(&network->def->ipAddress, AF_INET)) {
+ if (!VIR_SOCKET_IS_FAMILY(&ipdef->address, AF_INET)) {
/* Only support collision check for IPv4 */
return 0;
}
- if (virNetworkDefNetmask(network->def, &netmask) < 0) {
+ if (virNetworkIpDefNetmask(ipdef, &netmask) < 0) {
networkReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to get netmask of '%s'"),
network->def->bridge);
}
- net_dest = (network->def->ipAddress.data.inet4.sin_addr.s_addr &
+ net_dest = (ipdef->address.data.inet4.sin_addr.s_addr &
netmask.data.inet4.sin_addr.s_addr);
/* Read whole routing table into memory */
@@ -1121,6 +1137,7 @@ static int networkStartNetworkDaemon(struct network_driver *driver,
{
int err;
virErrorPtr save_err;
+ virNetworkIpDefPtr ipv4def;
if (virNetworkObjIsActive(network)) {
networkReportError(VIR_ERR_INTERNAL_ERROR,
@@ -1128,8 +1145,11 @@ static int networkStartNetworkDaemon(struct network_driver
*driver,
return -1;
}
+ /* find the one allowed IPv4 ip address in the definition */
+ ipv4def = virNetworkDefGetIpByIndex(network->def, AF_INET, 0);
+
/* Check to see if network collides with an existing route */
- if (networkCheckRouteCollision(network) < 0)
+ if (ipv4def && networkCheckRouteCollision(network, ipv4def) < 0)
return -1;
if ((err = brAddBridge(driver->brctl, network->def->bridge))) {
@@ -1158,8 +1178,8 @@ static int networkStartNetworkDaemon(struct network_driver *driver,
goto err_delbr;
}
- if (VIR_SOCKET_HAS_ADDR(&network->def->ipAddress)) {
- int prefix = virNetworkDefPrefix(network->def);
+ if (ipv4def) {
+ int prefix = virNetworkIpDefPrefix(ipv4def);
if (prefix < 0) {
networkReportError(VIR_ERR_INTERNAL_ERROR,
@@ -1169,7 +1189,7 @@ static int networkStartNetworkDaemon(struct network_driver *driver,
}
if ((err = brAddInetAddress(driver->brctl, network->def->bridge,
- &network->def->ipAddress, prefix))) {
+ &ipv4def->address, prefix))) {
networkReportError(VIR_ERR_INTERNAL_ERROR,
_("cannot set IP address on bridge
'%s'"),
network->def->bridge);
@@ -1184,7 +1204,7 @@ static int networkStartNetworkDaemon(struct network_driver *driver,
goto err_delbr;
}
- if (networkAddIptablesRules(driver, network) < 0)
+ if (ipv4def && networkAddIptablesRules(driver, network, ipv4def) < 0)
goto err_delbr1;
if (network->def->forwardType != VIR_NETWORK_FORWARD_NONE &&
@@ -1194,12 +1214,13 @@ static int networkStartNetworkDaemon(struct network_driver
*driver,
goto err_delbr2;
}
- if ((VIR_SOCKET_HAS_ADDR(&network->def->ipAddress) ||
- network->def->nranges) &&
- dhcpStartDhcpDaemon(network) < 0)
+ /*
+ * Start the dhcp daemon for the 1st (and only supported) ipv4
+ * address.
+ */
+ if (ipv4def && dhcpStartDhcpDaemon(network, ipv4def) < 0)
goto err_delbr2;
-
/* Persist the live configuration now we have bridge info */
if (virNetworkSaveConfig(NETWORK_STATE_DIR, network->def) < 0) {
goto err_kill;
@@ -1217,7 +1238,8 @@ static int networkStartNetworkDaemon(struct network_driver *driver,
err_delbr2:
save_err = virSaveLastError();
- networkRemoveIptablesRules(driver, network);
+ if (ipv4def)
+ networkRemoveIptablesRules(driver, network, ipv4def);
if (save_err) {
virSetError(save_err);
virFreeError(save_err);
@@ -1246,6 +1268,7 @@ static int networkShutdownNetworkDaemon(struct network_driver
*driver,
{
int err;
char *stateFile;
+ virNetworkIpDefPtr ipv4def;
VIR_INFO(_("Shutting down network '%s'"),
network->def->name);
@@ -1262,7 +1285,10 @@ static int networkShutdownNetworkDaemon(struct network_driver
*driver,
if (network->dnsmasqPid > 0)
kill(network->dnsmasqPid, SIGTERM);
- networkRemoveIptablesRules(driver, network);
+ /* find the one allowed IPv4 ip address in the definition */
+ ipv4def = virNetworkDefGetIpByIndex(network->def, AF_INET, 0);
+ if (ipv4def)
+ networkRemoveIptablesRules(driver, network, ipv4def);
char ebuf[1024];
if ((err = brSetInterfaceUp(driver->brctl, network->def->bridge, 0))) {
@@ -1526,6 +1552,7 @@ cleanup:
static virNetworkPtr networkDefine(virConnectPtr conn, const char *xml) {
struct network_driver *driver = conn->networkPrivateData;
+ virNetworkIpDefPtr ipv4def;
virNetworkDefPtr def;
virNetworkObjPtr network = NULL;
virNetworkPtr ret = NULL;
@@ -1556,12 +1583,15 @@ static virNetworkPtr networkDefine(virConnectPtr conn, const char
*xml) {
goto cleanup;
}
- if (network->def->nhosts > 0) {
+ /* we only support dhcp on one IPv4 address per defined network */
+ ipv4def = virNetworkDefGetIpByIndex(network->def, AF_INET, 0);
+
+ if (ipv4def && ipv4def->nhosts > 0) {
dnsmasqContext *dctx = dnsmasqContextNew(network->def->name,
DNSMASQ_STATE_DIR);
if (dctx == NULL)
goto cleanup;
- networkSaveDnsmasqHostsfile(network, dctx, true);
+ networkSaveDnsmasqHostsfile(ipv4def, dctx, true);
dnsmasqContextFree(dctx);
}
@@ -1577,7 +1607,8 @@ cleanup:
static int networkUndefine(virNetworkPtr net) {
struct network_driver *driver = net->conn->networkPrivateData;
- virNetworkObjPtr network = NULL;
+ virNetworkObjPtr network;
+ virNetworkIpDefPtr ipv4def;
int ret = -1;
networkDriverLock(driver);
@@ -1600,7 +1631,10 @@ static int networkUndefine(virNetworkPtr net) {
network) < 0)
goto cleanup;
- if (network->def->nhosts > 0) {
+ /* find the one allowed IPv4 ip address in the definition */
+ ipv4def = virNetworkDefGetIpByIndex(network->def, AF_INET, 0);
+
+ if (ipv4def && ipv4def->nhosts > 0) {
dnsmasqContext *dctx = dnsmasqContextNew(network->def->name,
DNSMASQ_STATE_DIR);
if (dctx == NULL)
goto cleanup;
diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
index 728c501..03defb5 100644
--- a/src/vbox/vbox_tmpl.c
+++ b/src/vbox/vbox_tmpl.c
@@ -7038,9 +7038,23 @@ static virNetworkPtr vboxNetworkDefineCreateXML(virConnectPtr conn,
const char *
IHostNetworkInterface *networkInterface = NULL;
virNetworkDefPtr def = virNetworkDefParseString(xml);
+ virNetworkIpDefPtr ipdef;
+ virSocketAddr netmask;
if ( (!def)
- || (def->forwardType != VIR_NETWORK_FORWARD_NONE))
+ || (def->forwardType != VIR_NETWORK_FORWARD_NONE)
+ || (def->nips == 0 || !def->ips))
+ goto cleanup;
+
+ /* Look for the first IPv4 IP address definition and use that.
+ * If there weren't any IPv4 addresses, ignore the network (since it's
+ * required below to have an IPv4 address)
+ */
+ ipdef = virNetworkDefGetIpByIndex(def, AF_INET, 0);
+ if (!ipdef)
+ goto cleanup;
+
+ if (virNetworkIpDefNetmask(ipdef, &netmask) < 0)
goto cleanup;
/* the current limitation of hostonly network is that you can't
@@ -7096,9 +7110,9 @@ static virNetworkPtr vboxNetworkDefineCreateXML(virConnectPtr conn,
const char *
/* Currently support only one dhcp server per network
* with contigious address space from start to end
*/
- if ((def->nranges >= 1) &&
- VIR_SOCKET_HAS_ADDR(&def->ranges[0].start) &&
- VIR_SOCKET_HAS_ADDR(&def->ranges[0].end)) {
+ if ((ipdef->nranges >= 1) &&
+ VIR_SOCKET_HAS_ADDR(&ipdef->ranges[0].start) &&
+ VIR_SOCKET_HAS_ADDR(&ipdef->ranges[0].end)) {
IDHCPServer *dhcpServer = NULL;
data->vboxObj->vtbl->FindDHCPServerByNetworkName(data->vboxObj,
@@ -7118,10 +7132,10 @@ static virNetworkPtr vboxNetworkDefineCreateXML(virConnectPtr
conn, const char *
PRUnichar *toIPAddressUtf16 = NULL;
PRUnichar *trunkTypeUtf16 = NULL;
- ipAddressUtf16 = vboxSocketFormatAddrUtf16(data,
&def->ipAddress);
- networkMaskUtf16 = vboxSocketFormatAddrUtf16(data,
&def->netmask);
- fromIPAddressUtf16 = vboxSocketFormatAddrUtf16(data,
&def->ranges[0].start);
- toIPAddressUtf16 = vboxSocketFormatAddrUtf16(data,
&def->ranges[0].end);
+ ipAddressUtf16 = vboxSocketFormatAddrUtf16(data,
&ipdef->address);
+ networkMaskUtf16 = vboxSocketFormatAddrUtf16(data, &netmask);
+ fromIPAddressUtf16 = vboxSocketFormatAddrUtf16(data,
&ipdef->ranges[0].start);
+ toIPAddressUtf16 = vboxSocketFormatAddrUtf16(data,
&ipdef->ranges[0].end);
if (ipAddressUtf16 == NULL || networkMaskUtf16 == NULL ||
fromIPAddressUtf16 == NULL || toIPAddressUtf16 == NULL) {
@@ -7158,13 +7172,13 @@ static virNetworkPtr vboxNetworkDefineCreateXML(virConnectPtr
conn, const char *
}
}
- if ((def->nhosts >= 1) &&
- VIR_SOCKET_HAS_ADDR(&def->hosts[0].ip)) {
+ if ((ipdef->nhosts >= 1) &&
+ VIR_SOCKET_HAS_ADDR(&ipdef->hosts[0].ip)) {
PRUnichar *ipAddressUtf16 = NULL;
PRUnichar *networkMaskUtf16 = NULL;
- ipAddressUtf16 = vboxSocketFormatAddrUtf16(data, &def->hosts[0].ip);
- networkMaskUtf16 = vboxSocketFormatAddrUtf16(data, &def->netmask);
+ ipAddressUtf16 = vboxSocketFormatAddrUtf16(data,
&ipdef->hosts[0].ip);
+ networkMaskUtf16 = vboxSocketFormatAddrUtf16(data, &netmask);
if (ipAddressUtf16 == NULL || networkMaskUtf16 == NULL) {
VBOX_UTF16_FREE(ipAddressUtf16);
@@ -7385,12 +7399,19 @@ static int vboxNetworkDestroy(virNetworkPtr network) {
static char *vboxNetworkDumpXML(virNetworkPtr network, int flags ATTRIBUTE_UNUSED) {
VBOX_OBJECT_HOST_CHECK(network->conn, char *, NULL);
virNetworkDefPtr def = NULL;
+ virNetworkIpDefPtr ipdef = NULL;
char *networkNameUtf8 = NULL;
if (VIR_ALLOC(def) < 0) {
virReportOOMError();
goto cleanup;
}
+ if (VIR_ALLOC(ipdef) < 0) {
+ virReportOOMError();
+ goto cleanup;
+ }
+ def->ips = ipdef;
+ def->nips = 1;
if (virAsprintf(&networkNameUtf8, "HostInterfaceNetworking-%s",
network->name) < 0) {
virReportOOMError();
@@ -7427,8 +7448,8 @@ static char *vboxNetworkDumpXML(virNetworkPtr network, int flags
ATTRIBUTE_UNUSE
networkNameUtf16,
&dhcpServer);
if (dhcpServer) {
- def->nranges = 1;
- if (VIR_ALLOC_N(def->ranges, def->nranges) >=0 ) {
+ ipdef->nranges = 1;
+ if (VIR_ALLOC_N(ipdef->ranges, ipdef->nranges) >=0 ) {
PRUnichar *ipAddressUtf16 = NULL;
PRUnichar *networkMaskUtf16 = NULL;
PRUnichar *fromIPAddressUtf16 = NULL;
@@ -7443,13 +7464,13 @@ static char *vboxNetworkDumpXML(virNetworkPtr network, int flags
ATTRIBUTE_UNUSE
* with contigious address space from start to end
*/
if (vboxSocketParseAddrUtf16(data, ipAddressUtf16,
- &def->ipAddress) < 0 ||
+ &ipdef->address) < 0 ||
vboxSocketParseAddrUtf16(data, networkMaskUtf16,
- &def->netmask) < 0 ||
+ &ipdef->netmask) < 0 ||
vboxSocketParseAddrUtf16(data, fromIPAddressUtf16,
- &def->ranges[0].start) < 0
||
+ &ipdef->ranges[0].start) <
0 ||
vboxSocketParseAddrUtf16(data, toIPAddressUtf16,
- &def->ranges[0].end) < 0)
{
+ &ipdef->ranges[0].end) <
0) {
errorOccurred = true;
}
@@ -7462,16 +7483,16 @@ static char *vboxNetworkDumpXML(virNetworkPtr network, int flags
ATTRIBUTE_UNUSE
goto cleanup;
}
} else {
- def->nranges = 0;
+ ipdef->nranges = 0;
virReportOOMError();
}
- def->nhosts = 1;
- if (VIR_ALLOC_N(def->hosts, def->nhosts) >=0 ) {
- def->hosts[0].name = strdup(network->name);
- if (def->hosts[0].name == NULL) {
- VIR_FREE(def->hosts);
- def->nhosts = 0;
+ ipdef->nhosts = 1;
+ if (VIR_ALLOC_N(ipdef->hosts, ipdef->nhosts) >=0 ) {
+ ipdef->hosts[0].name = strdup(network->name);
+ if (ipdef->hosts[0].name == NULL) {
+ VIR_FREE(ipdef->hosts);
+ ipdef->nhosts = 0;
virReportOOMError();
} else {
PRUnichar *macAddressUtf16 = NULL;
@@ -7481,10 +7502,10 @@ static char *vboxNetworkDumpXML(virNetworkPtr network, int flags
ATTRIBUTE_UNUSE
networkInterface->vtbl->GetHardwareAddress(networkInterface, &macAddressUtf16);
networkInterface->vtbl->GetIPAddress(networkInterface,
&ipAddressUtf16);
- VBOX_UTF16_TO_UTF8(macAddressUtf16,
&def->hosts[0].mac);
+ VBOX_UTF16_TO_UTF8(macAddressUtf16,
&ipdef->hosts[0].mac);
if (vboxSocketParseAddrUtf16(data, ipAddressUtf16,
- &def->hosts[0].ip) <
0) {
+ &ipdef->hosts[0].ip) <
0) {
errorOccurred = true;
}
@@ -7496,7 +7517,7 @@ static char *vboxNetworkDumpXML(virNetworkPtr network, int flags
ATTRIBUTE_UNUSE
}
}
} else {
- def->nhosts = 0;
+ ipdef->nhosts = 0;
}
VBOX_RELEASE(dhcpServer);
@@ -7509,9 +7530,9 @@ static char *vboxNetworkDumpXML(virNetworkPtr network, int flags
ATTRIBUTE_UNUSE
networkInterface->vtbl->GetIPAddress(networkInterface,
&ipAddressUtf16);
if (vboxSocketParseAddrUtf16(data, networkMaskUtf16,
- &def->netmask) < 0 ||
+ &ipdef->netmask) < 0 ||
vboxSocketParseAddrUtf16(data, ipAddressUtf16,
- &def->ipAddress) < 0) {
+ &ipdef->address) < 0) {
errorOccurred = true;
}
diff --git a/tests/networkxml2xmlin/nat-network.xml
b/tests/networkxml2xmlin/nat-network.xml
index 93ab186..23f7fcb 100644
--- a/tests/networkxml2xmlin/nat-network.xml
+++ b/tests/networkxml2xmlin/nat-network.xml
@@ -10,4 +10,12 @@
<host mac="00:16:3e:3e:a9:1a" name="b.example.com"
ip="192.168.122.11" />
</dhcp>
</ip>
+ <ip family="ipv4" address="192.168.123.1"
netmask="255.255.255.0">
+ </ip>
+ <ip family="ipv6" address="2001:db8:ac10:fe01::1"
prefix="64">
+ </ip>
+ <ip family="ipv6" address="2001:db8:ac10:fd01::1"
prefix="64">
+ </ip>
+ <ip family="ipv4" address="10.24.10.1">
+ </ip>
</network>
diff --git a/tests/networkxml2xmlout/nat-network.xml
b/tests/networkxml2xmlout/nat-network.xml
index 036d4fb..eb71d9e 100644
--- a/tests/networkxml2xmlout/nat-network.xml
+++ b/tests/networkxml2xmlout/nat-network.xml
@@ -10,4 +10,12 @@
<host mac='00:16:3e:3e:a9:1a' name='b.example.com'
ip='192.168.122.11' />
</dhcp>
</ip>
+ <ip family='ipv4' address='192.168.123.1'
netmask='255.255.255.0'>
+ </ip>
+ <ip family='ipv6' address='2001:db8:ac10:fe01::1'
prefix='64'>
+ </ip>
+ <ip family='ipv6' address='2001:db8:ac10:fd01::1'
prefix='64'>
+ </ip>
+ <ip family='ipv4' address='10.24.10.1'>
+ </ip>
</network>
--
1.7.3.4
6:16 p.m.
New subject: [libvirt] [PATCHv2 09/13] Change virtual network XML parsing/formatting to support IPv6
On 12/22/2010 11:58 AM, Laine Stump wrote:
This commit adds support for IPv6 parsing and formatting to the
virtual network XML parser, including moving around data definitions
to allow for multiple <ip> elements on a single network, but only
changes the consumers of this API to accomodate for the changes in
s/accomodate/accommodate/
API/structure, not to add any actual IPv6 functionality. That will
come in a later patch - this patch attempts to maintain the same final
functionality in both drivers that use the network XML parser - vbox
and "bridge" (the Linux bridge-based driver used by the qemu
hypervisor driver).
* src/libvirt_private.syms: Add new private API functions.
* src/conf/network_conf.[ch]: Change C data structure and
parsing/formatting.
* src/network/bridge_driver.c: Update to use new parser/formatter.
* src/vbox/vbox_tmpl.c: update to use new parser/formatter
* docs/schemas/network.rng: changes to the schema -
* there can now be more than one <ip> element.
* ip address is now an ip-addr (ipv4 or ipv6) rather than ipv4-addr
* new optional "prefix" attribute that can be used in place of
"netmask"
* new optional "family" attribute - "ipv4" or "ipv6"
(will default to ipv4)
* define data types for the above
* tests/networkxml2xml(in|out)/nat-network.xml: add multiple <ip> elements
(including IPv6) to a single network definition to verify they are being
correctly parsed and formatted.
+ <!-- Based on
http://blog.mes-stats.fr/2008/10/09/regex-ipv4-et-ipv6 -->
+ <define name='ipv6-addr'>
+ <data type='string'>
+ <!-- To understand this better, take apart the toplevel '|'s -->
+ <param name="pattern">
(([0-9A-Fa-f]{1,4}:){7}[0-9A-Fa-f]{1,4})
|(([0-9A-Fa-f]{1,4}:){6}:[0-9A-Fa-f]{1,4})
|(([0-9A-Fa-f]{1,4}:){5}:([0-9A-Fa-f]{1,4}:)?[0-9A-Fa-f]{1,4})
|(([0-9A-Fa-f]{1,4}:){4}:([0-9A-Fa-f]{1,4}:){0,2}[0-9A-Fa-f]{1,4})
|(([0-9A-Fa-f]{1,4}:){3}:([0-9A-Fa-f]{1,4}:){0,3}[0-9A-Fa-f]{1,4})
|(([0-9A-Fa-f]{1,4}:){2}:([0-9A-Fa-f]{1,4}:){0,4}[0-9A-Fa-f]{1,4})
|(([0-9A-Fa-f]{1,4}:){6}((((25[0-5])|(1[0-9]{2})|(2[0-4][0-9])|([0-9]{1,2})))\.){3}(((25[0-5])|(1[0-9]{2})|(2[0-4][0-9])|([0-9]{1,2}))))
This doesn't properly represent the dotted IPv4 suffix (it allows
abcd::00.00.00.00). This should reuse the IPv4 pattern (swap 200 to
occur in the pattern before 100, and avoid double 0):
(((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9]))\.){3}((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9]))
|(([0-9A-Fa-f]{1,4}:){0,5}:((((25[0-5])|(1[0-9]{2})|(2[0-4][0-9])|([0-9]{1,2})))\.){3}(((25[0-5])|(1[0-9]{2})|(2[0-4][0-9])|([0-9]{1,2}))))
same problem with the IPv4 portion
|(::([0-9A-Fa-f]{1,4}:){0,5}((((25[0-5])|(1[0-9]{2})|(2[0-4][0-9])|([0-9]{1,2})))\.){3}(((25[0-5])|(1[0-9]{2})|(2[0-4][0-9])|([0-9]{1,2}))))
and again
|([0-9A-Fa-f]{1,4}::([0-9A-Fa-f]{1,4}:){0,5}[0-9A-Fa-f]{1,4})
|(::([0-9A-Fa-f]{1,4}:){0,6}[0-9A-Fa-f]{1,4})
|(([0-9A-Fa-f]{1,4}:){1,7}:)
Wow - that's quite the strict regexp. I probably would have copped out
and done the much shorter
[:0-9a-fA-F.]+
and filter out the non-IPv6 strings later, but since you already have
the regex, we might as well keep it.
void virNetworkDefFree(virNetworkDefPtr def)
{
- int i;
+ int ii;
This rename looks funny.
+ /* parse/validate netmask */
+ if (netmask) {
+ if (address == NULL) {
+ /* netmask is meaningless without an address */
+ virNetworkReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("netmask specified without address in network
'%s'"),
+ networkName);
+ goto error;
+ }
+
+ if (!VIR_SOCKET_IS_FAMILY(&def->address, AF_INET)) {
+ virNetworkReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("netmask not supported for address '%s'
in network '%s' (IPv4 only)"),
+ address, networkName);
+ goto error;
+ }
+
+ if (def->prefix > 0) {
+ /* can't have both netmask and prefix at the same time */
+ virNetworkReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("network '%s' has prefix='%u'
but no address"),
+ networkName, def->prefix);
Should this message be "network '%s' cannot have both prefix='%u' and
a
netmask"?
diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
index a922d28..a51794d 100644
--- a/src/conf/network_conf.h
+++ b/src/conf/network_conf.h
@@ -56,6 +56,33 @@ struct _virNetworkDHCPHostDef {
virSocketAddr ip;
};
+typedef struct _virNetworkIpDef virNetworkIpDef;
+typedef virNetworkIpDef *virNetworkIpDefPtr;
+struct _virNetworkIpDef {
+ char *family; /* ipv4 or ipv6 - default is ipv4 */
+ virSocketAddr address; /* Bridge IP address */
+
+ /* The first two items below are taken directly from the XML (one
+ * or the other is given, but not both) and the 3rd is derived
+ * from the first two. When formatting XML, always use netMasktStr
Typo in netMasktStr?
+ int nips;
s/int/size_t/
+ virNetworkIpDefPtr ips; /* ptr to array of IP addresses on this
network */
};
typedef struct _virNetworkObj virNetworkObj;
+virNetworkIpDefPtr
+virNetworkDefGetIpByIndex(const virNetworkDefPtr def,
+ int family, int n);
Should n be size_t?
virNetworkFindByUUID;
virNetworkLoadAllConfigs;
+virNetworkIpDefNetmask;
+virNetworkIpDefPrefix;
Sorting.
Close call as to whether I pointed out enough things to warrant a v3, or
if you should just fix them.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
2:31 a.m.
New subject: [libvirt] [PATCHv2 09/13] Change virtual network XML parsing/formatting to support IPv6
On 12/22/2010 07:16 PM, Eric Blake wrote:
On 12/22/2010 11:58 AM, Laine Stump wrote:
> This commit adds support for IPv6 parsing and formatting to the
> virtual network XML parser, including moving around data definitions
> to allow for multiple<ip> elements on a single network, but only
> changes the consumers of this API to accomodate for the changes in
s/accomodate/accommodate/
Done.
> API/structure, not to add any actual IPv6 functionality. That
will
> come in a later patch - this patch attempts to maintain the same final
> functionality in both drivers that use the network XML parser - vbox
> and "bridge" (the Linux bridge-based driver used by the qemu
> hypervisor driver).
>
> * src/libvirt_private.syms: Add new private API functions.
> * src/conf/network_conf.[ch]: Change C data structure and
> parsing/formatting.
> * src/network/bridge_driver.c: Update to use new parser/formatter.
> * src/vbox/vbox_tmpl.c: update to use new parser/formatter
> * docs/schemas/network.rng: changes to the schema -
> * there can now be more than one<ip> element.
> * ip address is now an ip-addr (ipv4 or ipv6) rather than ipv4-addr
> * new optional "prefix" attribute that can be used in place of
"netmask"
> * new optional "family" attribute - "ipv4" or
"ipv6"
> (will default to ipv4)
> * define data types for the above
> * tests/networkxml2xml(in|out)/nat-network.xml: add multiple<ip> elements
> (including IPv6) to a single network definition to verify they are being
> correctly parsed and formatted.
> +<!-- Based on http://blog.mes-stats.fr/2008/10/09/regex-ipv4-et-ipv6 -->
> +<define name='ipv6-addr'>
> +<data type='string'>
> +<!-- To understand this better, take apart the toplevel '|'s -->
> +<param name="pattern">
(([0-9A-Fa-f]{1,4}:){7}[0-9A-Fa-f]{1,4})
|(([0-9A-Fa-f]{1,4}:){6}:[0-9A-Fa-f]{1,4})
|(([0-9A-Fa-f]{1,4}:){5}:([0-9A-Fa-f]{1,4}:)?[0-9A-Fa-f]{1,4})
|(([0-9A-Fa-f]{1,4}:){4}:([0-9A-Fa-f]{1,4}:){0,2}[0-9A-Fa-f]{1,4})
|(([0-9A-Fa-f]{1,4}:){3}:([0-9A-Fa-f]{1,4}:){0,3}[0-9A-Fa-f]{1,4})
|(([0-9A-Fa-f]{1,4}:){2}:([0-9A-Fa-f]{1,4}:){0,4}[0-9A-Fa-f]{1,4})
|(([0-9A-Fa-f]{1,4}:){6}((((25[0-5])|(1[0-9]{2})|(2[0-4][0-9])|([0-9]{1,2})))\.){3}(((25[0-5])|(1[0-9]{2})|(2[0-4][0-9])|([0-9]{1,2}))))
This doesn't properly represent the dotted IPv4 suffix (it allows
abcd::00.00.00.00). This should reuse the IPv4 pattern (swap 200 to
occur in the pattern before 100, and avoid double 0):
(((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9]))\.){3}((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9]))
|(([0-9A-Fa-f]{1,4}:){0,5}:((((25[0-5])|(1[0-9]{2})|(2[0-4][0-9])|([0-9]{1,2})))\.){3}(((25[0-5])|(1[0-9]{2})|(2[0-4][0-9])|([0-9]{1,2}))))
same problem with the IPv4 portion
|(::([0-9A-Fa-f]{1,4}:){0,5}((((25[0-5])|(1[0-9]{2})|(2[0-4][0-9])|([0-9]{1,2})))\.){3}(((25[0-5])|(1[0-9]{2})|(2[0-4][0-9])|([0-9]{1,2}))))
and again
|([0-9A-Fa-f]{1,4}::([0-9A-Fa-f]{1,4}:){0,5}[0-9A-Fa-f]{1,4})
|(::([0-9A-Fa-f]{1,4}:){0,6}[0-9A-Fa-f]{1,4})
|(([0-9A-Fa-f]{1,4}:){1,7}:)
Wow - that's quite the strict regexp. I probably would have copped out
and done the much shorter
[:0-9a-fA-F.]+
Wow. Great analysis of a strict regexp! :-) I would have copped out too,
but already had a regexp written by David Lutterkort for netcf, so I
figured I'd take the "easy" way out and re-use it.
I've changed the IPv6 regexp to replicate the fixed IPv4 regexp in each
of the places you point out.
and filter out the non-IPv6 strings later, but since you already
have
the regex, we might as well keep it.
> void virNetworkDefFree(virNetworkDefPtr def)
> {
> - int i;
> + int ii;
This rename looks funny.
It's not exactly a rename - I removed the original use (for nhosts), and
put in my own new loop. I prefer to use "ii" as an anonymous loop
variable rather than "i" because it makes it easier to search for (no
false positives).
> + /* parse/validate netmask */
> + if (netmask) {
> + if (address == NULL) {
> + /* netmask is meaningless without an address */
> + virNetworkReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("netmask specified without address in
network '%s'"),
> + networkName);
> + goto error;
> + }
> +
> + if (!VIR_SOCKET_IS_FAMILY(&def->address, AF_INET)) {
> + virNetworkReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("netmask not supported for address
'%s' in network '%s' (IPv4 only)"),
> + address, networkName);
> + goto error;
> + }
> +
> + if (def->prefix> 0) {
> + /* can't have both netmask and prefix at the same time */
> + virNetworkReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("network '%s' has
prefix='%u' but no address"),
> + networkName, def->prefix);
Should this message be "network '%s' cannot have both prefix='%u'
and a
netmask"?
Yes. Fixed.
> diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
> index a922d28..a51794d 100644
> --- a/src/conf/network_conf.h
> +++ b/src/conf/network_conf.h
> @@ -56,6 +56,33 @@ struct _virNetworkDHCPHostDef {
> virSocketAddr ip;
> };
>
> +typedef struct _virNetworkIpDef virNetworkIpDef;
> +typedef virNetworkIpDef *virNetworkIpDefPtr;
> +struct _virNetworkIpDef {
> + char *family; /* ipv4 or ipv6 - default is ipv4 */
> + virSocketAddr address; /* Bridge IP address */
> +
> + /* The first two items below are taken directly from the XML (one
> + * or the other is given, but not both) and the 3rd is derived
> + * from the first two. When formatting XML, always use netMasktStr
Typo in netMasktStr?
That entire comment was out of date - I had originally stored the
netmask as a string in addition to the virSocketAddr for some silly
reason. I've changed the comment to just point out that either prefix or
netmask will be valid (guaranteed by parser), and that the API functions
should be used rather than directly accessing the values.
> + int nips;
s/int/size_t/
Okay, I've changed it. I'll point out that the great majority of the
"n<thing>s" variables in *_conf.h are defined as int (and some more as
unsigned int). Should these all be standardized at some point?
> + virNetworkIpDefPtr ips; /* ptr to array of IP addresses on
this network */
> };
>
> typedef struct _virNetworkObj virNetworkObj;
> +virNetworkIpDefPtr
> +virNetworkDefGetIpByIndex(const virNetworkDefPtr def,
> + int family, int n);
Should n be size_t?
Sure, why not.
> virNetworkFindByUUID;
> virNetworkLoadAllConfigs;
> +virNetworkIpDefNetmask;
> +virNetworkIpDefPrefix;
Sorting.
Oops.
Close call as to whether I pointed out enough things to warrant a v3,
or
if you should just fix them.
I was going to attach a delta diff, but realized after the fact that I
didn't know how to get a diff between an old and new version of a commit
once I'd rebased. Instead, I'm pasting the new regexp below for you to
review; that's the only significant change. The others have all been
squashed in as well.
I've also incorporated all the fixes you suggested in your reviews of
01-08, but will wait to reply to those messages with a "pushed with
fixes" once the whole set is ACKed and I push them all.
Thanks again for the very thorough and timely reviews. I know this isn't
the easiest time of year to take on extra tasks!
************************
The new IPv6 regexp (constructed by first de-constructing the old
regexp, then removing the IPv4 sub-expressions and replacing them with
the fixed version from ipv4-addr, and finally concatenating everything
back together):
<param
name="pattern">(([0-9A-Fa-f]{1,4}:){7}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){6}:[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){5}:([0-9A-Fa-f]{1,4}:)?[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){4}:([0-9A-Fa-f]{1,4}:){0,2}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){3}:([0-9A-Fa-f]{1,4}:){0,3}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){2}:([0-9A-Fa-f]{1,4}:){0,4}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){6}(((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9]))\.){3}((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9])))|(([0-9A-Fa-f]{1,4}:){0,5}:(((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9]))\.){3}((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9])))|(::([0-9A-Fa-f]{1,4}:){0,5}(((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9]))\.){3}((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9])))|([0-9A-Fa-f]{1,4}::([0-9A-Fa-f]{1,4}:){0,5}[0-9A-Fa-f]{1,4})|(::([0-9A-Fa-f]{1,4}:){0,6}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){1,7}:)</param>
8:23 a.m.
New subject: [libvirt] [PATCHv2 09/13] Change virtual network XML parsing/formatting to support IPv6
On Thu, Dec 23, 2010 at 09:31, Laine Stump <laine(a)laine.org> wrote:
The new IPv6 regexp (constructed by first de-constructing the old
regexp,
then removing the IPv4 sub-expressions and replacing them with the fixed
version from ipv4-addr, and finally concatenating everything back together):
<param
name="pattern">(([0-9A-Fa-f]{1,4}:){7}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){6}:[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){5}:([0-9A-Fa-f]{1,4}:)?[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){4}:([0-9A-Fa-f]{1,4}:){0,2}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){3}:([0-9A-Fa-f]{1,4}:){0,3}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){2}:([0-9A-Fa-f]{1,4}:){0,4}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){6}(((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9]))\.){3}((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9])))|(([0-9A-Fa-f]{1,4}:){0,5}:(((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9]))\.){3}((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9])))|(::([0-9A-Fa-f]{1,4}:){0,5}(((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9]))\.){3}((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9])))|([0-9A-Fa-f]{1,4}::([0-9A-Fa-f]{1,4}:){0,5}[0-9A-Fa-f]{1,4})|(::([0-9A-Fa-f]{1,4}:){0,6}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){1,7}:)</param>
which matches incorrect address:
:10.0.0.1
more reading: http://crisp.tweakblogs.net/blog/3049/ipv6-validation-more-caveats.html
--
Pawel
8:50 a.m.
New subject: [libvirt] [PATCHv2 09/13] Change virtual network XML parsing/formatting to support IPv6
On 12/23/2010 09:23 AM, Paweł Krześniak wrote:
On Thu, Dec 23, 2010 at 09:31, Laine Stump<laine(a)laine.org>
wrote:
> The new IPv6 regexp (constructed by first de-constructing the old regexp,
> then removing the IPv4 sub-expressions and replacing them with the fixed
> version from ipv4-addr, and finally concatenating everything back together):
>
> <param
>
name="pattern">(([0-9A-Fa-f]{1,4}:){7}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){6}:[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){5}:([0-9A-Fa-f]{1,4}:)?[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){4}:([0-9A-Fa-f]{1,4}:){0,2}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){3}:([0-9A-Fa-f]{1,4}:){0,3}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){2}:([0-9A-Fa-f]{1,4}:){0,4}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){6}(((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9]))\.){3}((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9])))|(([0-9A-Fa-f]{1,4}:){0,5}:(((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9]))\.){3}((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9])))|(::([0-9A-Fa-f]{1,4}:){0,5}(((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9]))\.){3}((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9])))|([0-9A-Fa-f]{1,4}::([0-9A-Fa-f]{1,4}:){0,5}[0-9A-Fa-f]{1,4})|(::([0-9A-Fa-f]{1,4}:){0,6}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){1,7}:)</param>
which matches incorrect address:
:10.0.0.1
more reading: http://crisp.tweakblogs.net/blog/3049/ipv6-validation-more-caveats.html
Yikes!
I'd like to get this 100% correct, but since the rng is only actually
used by the test programs (and as a guide for users of the API), and not
used by the library itself, I'm inclined to call it good enough for now,
and put a note on my to-do list to fix it up later :-)
Thanks for the pointer to the article. That will be useful to have for
reference when I go back to fix it.
11:09 a.m.
New subject: [libvirt] [PATCHv2 09/13] Change virtual network XML parsing/formatting to support IPv6
On 12/23/2010 01:31 AM, Laine Stump wrote:
>> + int nips;
> s/int/size_t/
Okay, I've changed it. I'll point out that the great majority of the
"n<thing>s" variables in *_conf.h are defined as int (and some more as
unsigned int). Should these all be standardized at some point?
Probably, but not high priority.
I was going to attach a delta diff, but realized after the fact that
I
didn't know how to get a diff between an old and new version of a commit
once I'd rebased. Instead, I'm pasting the new regexp below for you to
review; that's the only significant change. The others have all been
squashed in as well.
Fair enough.
Agree to ACK for now, and we can further fix the regex later (to fix the
noted problem with accepting :1.2.3.4 - too loose rng is not a
show-stopper for actually using valid IPv6 in XML) [hmm - can you tell
that this is a big enough series that I'd like to get it in before 0.8.7
to widen the test coverage?].
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
12:22 p.m.
New subject: [libvirt] [PATCHv2 09/13] Change virtual network XML parsing/formatting to support IPv6
On 12/23/2010 12:09 PM, Eric Blake wrote:
On 12/23/2010 01:31 AM, Laine Stump wrote:
>>> + int nips;
>> s/int/size_t/
> Okay, I've changed it. I'll point out that the great majority of the
> "n<thing>s" variables in *_conf.h are defined as int (and some more
as
> unsigned int). Should these all be standardized at some point?
Probably, but not high priority.
> I was going to attach a delta diff, but realized after the fact that I
> didn't know how to get a diff between an old and new version of a commit
> once I'd rebased. Instead, I'm pasting the new regexp below for you to
> review; that's the only significant change. The others have all been
> squashed in as well.
Fair enough.
Agree to ACK for now, and we can further fix the regex later (to fix the
noted problem with accepting :1.2.3.4 - too loose rng is not a
show-stopper for actually using valid IPv6 in XML) [hmm - can you tell
that this is a big enough series that I'd like to get it in before 0.8.7
to widen the test coverage?].
Yes!
Me too.
+1
AOL++
<Whatever other method exists to empatically agree with you>
12:58 p.m.
New subject: [libvirt] [PATCHv2 10/13] Support multiple IP addresses on one network in bridge_driver.c
This patch reorganizes the code in bridge_driver.c to account for the
concept of a single network with multiple IP addresses, without adding
in the extra variable of IPv6. A small bit of code has been
temporarily added that checks all given addresses to verify they are
IPv4 - this will be removed when full IPv6 support is turned on.
---
No changes from V1.
src/network/bridge_driver.c | 603 ++++++++++++++++++++++++++----------------
1 files changed, 373 insertions(+), 230 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 0f8b050..03afe6c 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -521,20 +521,25 @@ cleanup:
}
static int
-dhcpStartDhcpDaemon(virNetworkObjPtr network,
- virNetworkIpDefPtr ipdef)
+networkStartDhcpDaemon(virNetworkObjPtr network)
{
virCommandPtr cmd = NULL;
char *pidfile = NULL;
- int ret = -1, err;
+ int ret = -1, err, ii;
+ virNetworkIpDefPtr ipdef;
network->dnsmasqPid = -1;
- if (!VIR_SOCKET_IS_FAMILY(&ipdef->address, AF_INET)) {
- networkReportError(VIR_ERR_INTERNAL_ERROR,
- "%s", _("cannot start dhcp daemon without IPv4
address for server"));
- goto cleanup;
+ /* Look for first IPv4 address that has dhcp defined. */
+ /* We support dhcp config on 1 IPv4 interface only. */
+ for (ii = 0;
+ (ipdef = virNetworkDefGetIpByIndex(network->def, AF_INET, ii));
+ ii++) {
+ if (ipdef->nranges || ipdef->nhosts)
+ break;
}
+ if (!ipdef)
+ return 0;
if ((err = virFileMakePath(NETWORK_PID_DIR)) != 0) {
virReportSystemError(err,
@@ -583,8 +588,8 @@ cleanup:
static int
networkAddMasqueradingIptablesRules(struct network_driver *driver,
- virNetworkObjPtr network,
- virNetworkIpDefPtr ipdef)
+ virNetworkObjPtr network,
+ virNetworkIpDefPtr ipdef)
{
int prefix = virNetworkIpDefPrefix(ipdef);
@@ -607,7 +612,9 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver,
goto masqerr1;
}
- /* allow forwarding packets to the bridge interface if they are part of an existing
connection */
+ /* allow forwarding packets to the bridge interface if they are
+ * part of an existing connection
+ */
if (iptablesAddForwardAllowRelatedIn(driver->iptables,
&ipdef->address,
prefix,
@@ -708,10 +715,48 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver,
return -1;
}
+static void
+networkRemoveMasqueradingIptablesRules(struct network_driver *driver,
+ virNetworkObjPtr network,
+ virNetworkIpDefPtr ipdef)
+{
+ int prefix = virNetworkIpDefPrefix(ipdef);
+
+ if (prefix >= 0) {
+ iptablesRemoveForwardMasquerade(driver->iptables,
+ &ipdef->address,
+ prefix,
+ network->def->forwardDev,
+ "tcp");
+ iptablesRemoveForwardMasquerade(driver->iptables,
+ &ipdef->address,
+ prefix,
+ network->def->forwardDev,
+ "udp");
+ iptablesRemoveForwardMasquerade(driver->iptables,
+ &ipdef->address,
+ prefix,
+ network->def->forwardDev,
+ NULL);
+
+ iptablesRemoveForwardAllowRelatedIn(driver->iptables,
+ &ipdef->address,
+ prefix,
+ network->def->bridge,
+ network->def->forwardDev);
+ iptablesRemoveForwardAllowOut(driver->iptables,
+ &ipdef->address,
+ prefix,
+ network->def->bridge,
+ network->def->forwardDev);
+ }
+}
+
static int
networkAddRoutingIptablesRules(struct network_driver *driver,
virNetworkObjPtr network,
- virNetworkIpDefPtr ipdef) {
+ virNetworkIpDefPtr ipdef)
+{
int prefix = virNetworkIpDefPrefix(ipdef);
if (prefix < 0) {
@@ -747,23 +792,56 @@ networkAddRoutingIptablesRules(struct network_driver *driver,
return 0;
-
- routeerr2:
+routeerr2:
iptablesRemoveForwardAllowOut(driver->iptables,
&ipdef->address,
prefix,
network->def->bridge,
network->def->forwardDev);
- routeerr1:
+routeerr1:
return -1;
}
+static void
+networkRemoveRoutingIptablesRules(struct network_driver *driver,
+ virNetworkObjPtr network,
+ virNetworkIpDefPtr ipdef)
+{
+ int prefix = virNetworkIpDefPrefix(ipdef);
+
+ if (prefix >= 0) {
+ iptablesRemoveForwardAllowIn(driver->iptables,
+ &ipdef->address,
+ prefix,
+ network->def->bridge,
+ network->def->forwardDev);
+
+ iptablesRemoveForwardAllowOut(driver->iptables,
+ &ipdef->address,
+ prefix,
+ network->def->bridge,
+ network->def->forwardDev);
+ }
+}
+
static int
-networkAddIptablesRules(struct network_driver *driver,
- virNetworkObjPtr network,
- virNetworkIpDefPtr ipdef) {
+networkAddGeneralIptablesRules(struct network_driver *driver,
+ virNetworkObjPtr network)
+{
+ int ii;
+ virNetworkIpDefPtr ipv4def;
+
+ /* First look for first IPv4 address that has dhcp or tftpboot defined. */
+ /* We support dhcp config on 1 IPv4 interface only. */
+ for (ii = 0;
+ (ipv4def = virNetworkDefGetIpByIndex(network->def, AF_INET, ii));
+ ii++) {
+ if (ipv4def->nranges || ipv4def->nhosts || ipv4def->tftproot)
+ break;
+ }
/* allow DHCP requests through to dnsmasq */
+
if (iptablesAddTcpInput(driver->iptables, network->def->bridge, 67) < 0)
{
networkReportError(VIR_ERR_SYSTEM_ERROR,
_("failed to add iptables rule to allow DHCP requests
from '%s'"),
@@ -778,6 +856,20 @@ networkAddIptablesRules(struct network_driver *driver,
goto err2;
}
+ /* If we are doing local DHCP service on this network, attempt to
+ * add a rule that will fixup the checksum of DHCP response
+ * packets back to the guests (but report failure without
+ * aborting, since not all iptables implementations support it).
+ */
+
+ if (ipv4def && (ipv4def->nranges || ipv4def->nhosts) &&
+ (iptablesAddOutputFixUdpChecksum(driver->iptables,
+ network->def->bridge, 68) < 0)) {
+ VIR_WARN("Could not add rule to fixup DHCP response checksums "
+ "on network '%s'.", network->def->name);
+ VIR_WARN0("May need to update iptables package & kernel to support
CHECKSUM rule.");
+ }
+
/* allow DNS requests through to dnsmasq */
if (iptablesAddTcpInput(driver->iptables, network->def->bridge, 53) < 0)
{
networkReportError(VIR_ERR_SYSTEM_ERROR,
@@ -793,30 +885,29 @@ networkAddIptablesRules(struct network_driver *driver,
goto err4;
}
- /* allow TFTP requests through to dnsmasq */
- if (ipdef && ipdef->tftproot &&
+ /* allow TFTP requests through to dnsmasq if necessary */
+ if (ipv4def && ipv4def->tftproot &&
iptablesAddUdpInput(driver->iptables, network->def->bridge, 69) < 0)
{
networkReportError(VIR_ERR_SYSTEM_ERROR,
_("failed to add iptables rule to allow TFTP requests
from '%s'"),
network->def->bridge);
- goto err4tftp;
+ goto err5;
}
-
/* Catch all rules to block forwarding to/from bridges */
if (iptablesAddForwardRejectOut(driver->iptables, network->def->bridge) <
0) {
networkReportError(VIR_ERR_SYSTEM_ERROR,
_("failed to add iptables rule to block outbound traffic
from '%s'"),
network->def->bridge);
- goto err5;
+ goto err6;
}
if (iptablesAddForwardRejectIn(driver->iptables, network->def->bridge) <
0) {
networkReportError(VIR_ERR_SYSTEM_ERROR,
_("failed to add iptables rule to block inbound traffic
to '%s'"),
network->def->bridge);
- goto err6;
+ goto err7;
}
/* Allow traffic between guests on the same bridge */
@@ -824,129 +915,139 @@ networkAddIptablesRules(struct network_driver *driver,
networkReportError(VIR_ERR_SYSTEM_ERROR,
_("failed to add iptables rule to allow cross bridge
traffic on '%s'"),
network->def->bridge);
- goto err7;
- }
-
- if (ipdef) {
- /* If masquerading is enabled, set up the rules*/
- if (network->def->forwardType == VIR_NETWORK_FORWARD_NAT &&
- networkAddMasqueradingIptablesRules(driver, network, ipdef) < 0)
- goto err8;
- /* else if routing is enabled, set up the rules*/
- else if (network->def->forwardType == VIR_NETWORK_FORWARD_ROUTE &&
- networkAddRoutingIptablesRules(driver, network, ipdef) < 0)
- goto err8;
-
- /* If we are doing local DHCP service on this network, attempt to
- * add a rule that will fixup the checksum of DHCP response
- * packets back to the guests (but report failure without
- * aborting, since not all iptables implementations support it).
- */
-
- if (ipdef && (VIR_SOCKET_IS_FAMILY(&ipdef->address, AF_INET) ||
- ipdef->nranges) &&
- (iptablesAddOutputFixUdpChecksum(driver->iptables,
- network->def->bridge, 68) < 0)) {
- VIR_WARN("Could not add rule to fixup DHCP response checksums "
- "on network '%s'.", network->def->name);
- VIR_WARN0("May need to update iptables package & kernel to support
CHECKSUM rule.");
- }
+ goto err8;
}
return 0;
- err8:
- iptablesRemoveForwardAllowCross(driver->iptables,
- network->def->bridge);
- err7:
- iptablesRemoveForwardRejectIn(driver->iptables,
- network->def->bridge);
- err6:
- iptablesRemoveForwardRejectOut(driver->iptables,
- network->def->bridge);
- err5:
- if (ipdef && ipdef->tftproot) {
+ /* unwind in reverse order from the point of failure */
+err8:
+ iptablesRemoveForwardRejectIn(driver->iptables, network->def->bridge);
+err7:
+ iptablesRemoveForwardRejectOut(driver->iptables, network->def->bridge);
+err6:
+ if (ipv4def && ipv4def->tftproot) {
iptablesRemoveUdpInput(driver->iptables, network->def->bridge, 69);
}
- err4tftp:
+err5:
iptablesRemoveUdpInput(driver->iptables, network->def->bridge, 53);
- err4:
+err4:
iptablesRemoveTcpInput(driver->iptables, network->def->bridge, 53);
- err3:
+err3:
iptablesRemoveUdpInput(driver->iptables, network->def->bridge, 67);
- err2:
+err2:
iptablesRemoveTcpInput(driver->iptables, network->def->bridge, 67);
- err1:
+err1:
return -1;
}
static void
-networkRemoveIptablesRules(struct network_driver *driver,
- virNetworkObjPtr network,
- virNetworkIpDefPtr ipdef) {
+networkRemoveGeneralIptablesRules(struct network_driver *driver,
+ virNetworkObjPtr network)
+{
+ int ii;
+ virNetworkIpDefPtr ipv4def;
- if (ipdef && (VIR_SOCKET_HAS_ADDR(&ipdef->address) ||
- ipdef->nranges)) {
- iptablesRemoveOutputFixUdpChecksum(driver->iptables,
- network->def->bridge, 68);
+ for (ii = 0;
+ (ipv4def = virNetworkDefGetIpByIndex(network->def, AF_INET, ii));
+ ii++) {
+ if (ipv4def->nranges || ipv4def->nhosts || ipv4def->tftproot)
+ break;
}
- if (ipdef && network->def->forwardType != VIR_NETWORK_FORWARD_NONE) {
- int prefix = virNetworkIpDefPrefix(ipdef);
-
- if (prefix < 0) {
- networkReportError(VIR_ERR_INTERNAL_ERROR,
- _("Invalid prefix or netmask for
'%s'"),
- network->def->bridge);
- goto error;
- }
-
- if (network->def->forwardType == VIR_NETWORK_FORWARD_NAT) {
- iptablesRemoveForwardMasquerade(driver->iptables,
- &ipdef->address,
- prefix,
- network->def->forwardDev,
- "tcp");
- iptablesRemoveForwardMasquerade(driver->iptables,
- &ipdef->address,
- prefix,
- network->def->forwardDev,
- "udp");
- iptablesRemoveForwardMasquerade(driver->iptables,
- &ipdef->address,
- prefix,
- network->def->forwardDev,
- NULL);
- iptablesRemoveForwardAllowRelatedIn(driver->iptables,
- &ipdef->address,
- prefix,
- network->def->bridge,
- network->def->forwardDev);
- } else if (network->def->forwardType == VIR_NETWORK_FORWARD_ROUTE) {
- iptablesRemoveForwardAllowIn(driver->iptables,
- &ipdef->address,
- prefix,
- network->def->bridge,
- network->def->forwardDev);
- }
- iptablesRemoveForwardAllowOut(driver->iptables,
- &ipdef->address,
- prefix,
- network->def->bridge,
- network->def->forwardDev);
- }
-error:
iptablesRemoveForwardAllowCross(driver->iptables, network->def->bridge);
iptablesRemoveForwardRejectIn(driver->iptables, network->def->bridge);
iptablesRemoveForwardRejectOut(driver->iptables, network->def->bridge);
- if (ipdef && ipdef->tftproot)
+ if (ipv4def && ipv4def->tftproot) {
iptablesRemoveUdpInput(driver->iptables, network->def->bridge, 69);
+ }
iptablesRemoveUdpInput(driver->iptables, network->def->bridge, 53);
iptablesRemoveTcpInput(driver->iptables, network->def->bridge, 53);
+ if (ipv4def && (ipv4def->nranges || ipv4def->nhosts)) {
+ iptablesRemoveOutputFixUdpChecksum(driver->iptables,
+ network->def->bridge, 68);
+ }
iptablesRemoveUdpInput(driver->iptables, network->def->bridge, 67);
iptablesRemoveTcpInput(driver->iptables, network->def->bridge, 67);
}
+static int
+networkAddIpSpecificIptablesRules(struct network_driver *driver,
+ virNetworkObjPtr network,
+ virNetworkIpDefPtr ipdef)
+{
+ if (network->def->forwardType == VIR_NETWORK_FORWARD_NAT &&
+ networkAddMasqueradingIptablesRules(driver, network, ipdef) < 0)
+ return -1;
+ else if (network->def->forwardType == VIR_NETWORK_FORWARD_ROUTE &&
+ networkAddRoutingIptablesRules(driver, network, ipdef) < 0)
+ return -1;
+
+ return 0;
+}
+
+static void
+networkRemoveIpSpecificIptablesRules(struct network_driver *driver,
+ virNetworkObjPtr network,
+ virNetworkIpDefPtr ipdef)
+{
+ if (network->def->forwardType == VIR_NETWORK_FORWARD_NAT)
+ networkRemoveMasqueradingIptablesRules(driver, network, ipdef);
+ else if (network->def->forwardType == VIR_NETWORK_FORWARD_ROUTE)
+ networkRemoveRoutingIptablesRules(driver, network, ipdef);
+}
+
+/* Add all rules for all ip addresses (and general rules) on a network */
+static int
+networkAddIptablesRules(struct network_driver *driver,
+ virNetworkObjPtr network)
+{
+ int ii;
+ virNetworkIpDefPtr ipdef;
+
+ /* Add "once per network" rules */
+ if (networkAddGeneralIptablesRules(driver, network) < 0)
+ return -1;
+
+ for (ii = 0;
+ (ipdef = virNetworkDefGetIpByIndex(network->def, AF_UNSPEC, ii));
+ ii++) {
+ /* Add address-specific iptables rules */
+ if (networkAddIpSpecificIptablesRules(driver, network, ipdef) < 0) {
+ goto err;
+ }
+ }
+ return 0;
+
+err:
+ /* The final failed call to networkAddIpSpecificIptablesRules will
+ * have removed any rules it created, but we need to remove those
+ * added for previous IP addresses.
+ */
+ while ((--ii >= 0) &&
+ (ipdef = virNetworkDefGetIpByIndex(network->def, AF_UNSPEC, ii))) {
+ networkRemoveIpSpecificIptablesRules(driver, network, ipdef);
+ }
+ networkRemoveGeneralIptablesRules(driver, network);
+ return -1;
+}
+
+/* Remove all rules for all ip addresses (and general rules) on a network */
+static void
+networkRemoveIptablesRules(struct network_driver *driver,
+ virNetworkObjPtr network)
+{
+ int ii;
+ virNetworkIpDefPtr ipdef;
+
+ for (ii = 0;
+ (ipdef = virNetworkDefGetIpByIndex(network->def, AF_UNSPEC, ii));
+ ii++) {
+ networkRemoveIpSpecificIptablesRules(driver, network, ipdef);
+ }
+ networkRemoveGeneralIptablesRules(driver, network);
+}
+
static void
networkReloadIptablesRules(struct network_driver *driver)
{
@@ -956,22 +1057,12 @@ networkReloadIptablesRules(struct network_driver *driver)
for (i = 0 ; i < driver->networks.count ; i++) {
virNetworkObjLock(driver->networks.objs[i]);
-
if (virNetworkObjIsActive(driver->networks.objs[i])) {
- virNetworkIpDefPtr ipv4def;
-
- /* Find the one allowed IPv4 ip address in the definition */
- /* Even if none is found, we still call the functions below */
- ipv4def = virNetworkDefGetIpByIndex(driver->networks.objs[i]->def,
- AF_INET, 0);
- networkRemoveIptablesRules(driver, driver->networks.objs[i],
- ipv4def);
- if (networkAddIptablesRules(driver, driver->networks.objs[i],
- ipv4def) < 0) {
- /* failed to add but already logged */
- }
+ networkRemoveIptablesRules(driver, driver->networks.objs[i]);
+ if (networkAddIptablesRules(driver, driver->networks.objs[i]) < 0) {
+ /* failed to add but already logged */
+ }
}
-
virNetworkObjUnlock(driver->networks.objs[i]);
}
}
@@ -1043,32 +1134,16 @@ cleanup:
* other scenarios where we can ruin host network connectivity.
* XXX: Using a proper library is preferred over parsing /proc
*/
-static int networkCheckRouteCollision(virNetworkObjPtr network,
- virNetworkIpDefPtr ipdef)
+static int
+networkCheckRouteCollision(virNetworkObjPtr network)
{
- int ret = -1, len;
- unsigned int net_dest;
- virSocketAddr netmask;
+ int ret = 0, len;
char *cur, *buf = NULL;
enum {MAX_ROUTE_SIZE = 1024*64};
- if (!VIR_SOCKET_IS_FAMILY(&ipdef->address, AF_INET)) {
- /* Only support collision check for IPv4 */
- return 0;
- }
-
- if (virNetworkIpDefNetmask(ipdef, &netmask) < 0) {
- networkReportError(VIR_ERR_INTERNAL_ERROR,
- _("Failed to get netmask of '%s'"),
- network->def->bridge);
- }
-
- net_dest = (ipdef->address.data.inet4.sin_addr.s_addr &
- netmask.data.inet4.sin_addr.s_addr);
-
/* Read whole routing table into memory */
if ((len = virFileReadAll(PROC_NET_ROUTE, MAX_ROUTE_SIZE, &buf)) < 0)
- goto error;
+ goto out;
/* Dropping the last character shouldn't hurt */
if (len > 0)
@@ -1087,7 +1162,8 @@ static int networkCheckRouteCollision(virNetworkObjPtr network,
while (cur) {
char iface[17], dest[128], mask[128];
unsigned int addr_val, mask_val;
- int num;
+ virNetworkIpDefPtr ipdef;
+ int num, ii;
/* NUL-terminate the line, so sscanf doesn't go beyond a newline. */
char *nl = strchr(cur, '\n');
@@ -1116,28 +1192,70 @@ static int networkCheckRouteCollision(virNetworkObjPtr network,
addr_val &= mask_val;
- if ((net_dest == addr_val) &&
- (netmask.data.inet4.sin_addr.s_addr == mask_val)) {
- networkReportError(VIR_ERR_INTERNAL_ERROR,
- _("Network is already in use by interface %s"),
- iface);
- goto error;
+ for (ii = 0;
+ (ipdef = virNetworkDefGetIpByIndex(network->def, AF_INET, ii));
+ ii++) {
+
+ unsigned int net_dest;
+ virSocketAddr netmask;
+
+ if (virNetworkIpDefNetmask(ipdef, &netmask) < 0) {
+ VIR_WARN("Failed to get netmask of '%s'",
+ network->def->bridge);
+ continue;
+ }
+
+ net_dest = (ipdef->address.data.inet4.sin_addr.s_addr &
+ netmask.data.inet4.sin_addr.s_addr);
+
+ if ((net_dest == addr_val) &&
+ (netmask.data.inet4.sin_addr.s_addr == mask_val)) {
+ networkReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Network is already in use by interface
%s"),
+ iface);
+ ret = -1;
+ goto out;
+ }
}
}
out:
- ret = 0;
-error:
VIR_FREE(buf);
return ret;
}
-static int networkStartNetworkDaemon(struct network_driver *driver,
- virNetworkObjPtr network)
+static int
+networkAddAddrToBridge(struct network_driver *driver,
+ virNetworkObjPtr network,
+ virNetworkIpDefPtr ipdef)
{
- int err;
- virErrorPtr save_err;
- virNetworkIpDefPtr ipv4def;
+ int prefix = virNetworkIpDefPrefix(ipdef);
+
+ if (prefix < 0) {
+ networkReportError(VIR_ERR_INTERNAL_ERROR,
+ _("bridge '%s' has an invalid netmask or IP
address"),
+ network->def->bridge);
+ return -1;
+ }
+
+ if (brAddInetAddress(driver->brctl, network->def->bridge,
+ &ipdef->address, prefix) < 0) {
+ networkReportError(VIR_ERR_INTERNAL_ERROR,
+ _("cannot set IP address on bridge '%s'"),
+ network->def->bridge);
+ return -1;
+ }
+
+ return 0;
+}
+
+static int
+networkStartNetworkDaemon(struct network_driver *driver,
+ virNetworkObjPtr network)
+{
+ int ii, err, v4present = 0;
+ virErrorPtr save_err = NULL;
+ virNetworkIpDefPtr ipdef;
if (virNetworkObjIsActive(network)) {
networkReportError(VIR_ERR_INTERNAL_ERROR,
@@ -1145,13 +1263,11 @@ static int networkStartNetworkDaemon(struct network_driver
*driver,
return -1;
}
- /* find the one allowed IPv4 ip address in the definition */
- ipv4def = virNetworkDefGetIpByIndex(network->def, AF_INET, 0);
-
- /* Check to see if network collides with an existing route */
- if (ipv4def && networkCheckRouteCollision(network, ipv4def) < 0)
+ /* Check to see if any network IP collides with an existing route */
+ if (networkCheckRouteCollision(network) < 0)
return -1;
+ /* Create and configure the bridge device */
if ((err = brAddBridge(driver->brctl, network->def->bridge))) {
virReportSystemError(err,
_("cannot create bridge '%s'"),
@@ -1159,15 +1275,13 @@ static int networkStartNetworkDaemon(struct network_driver
*driver,
return -1;
}
- if (networkDisableIPV6(network) < 0)
- goto err_delbr;
-
+ /* Set bridge options */
if ((err = brSetForwardDelay(driver->brctl, network->def->bridge,
network->def->delay))) {
networkReportError(VIR_ERR_INTERNAL_ERROR,
_("cannot set forward delay on bridge
'%s'"),
network->def->bridge);
- goto err_delbr;
+ goto err1;
}
if ((err = brSetEnableSTP(driver->brctl, network->def->bridge,
@@ -1175,90 +1289,95 @@ static int networkStartNetworkDaemon(struct network_driver
*driver,
networkReportError(VIR_ERR_INTERNAL_ERROR,
_("cannot set STP '%s' on bridge
'%s'"),
network->def->stp ? "on" : "off",
network->def->bridge);
- goto err_delbr;
+ goto err1;
}
- if (ipv4def) {
- int prefix = virNetworkIpDefPrefix(ipv4def);
+ /* Disable IPv6 on the bridge */
+ if (networkDisableIPV6(network) < 0)
+ goto err1;
- if (prefix < 0) {
- networkReportError(VIR_ERR_INTERNAL_ERROR,
- _("bridge '%s' has an invalid netmask or IP
address"),
- network->def->bridge);
- goto err_delbr;
- }
+ /* Add "once per network" rules */
+ if (networkAddIptablesRules(driver, network) < 0)
+ goto err1;
+
+ for (ii = 0;
+ (ipdef = virNetworkDefGetIpByIndex(network->def, AF_UNSPEC, ii));
+ ii++) {
+ if (VIR_SOCKET_IS_FAMILY(&ipdef->address, AF_INET))
+ v4present = 1;
- if ((err = brAddInetAddress(driver->brctl, network->def->bridge,
- &ipv4def->address, prefix))) {
- networkReportError(VIR_ERR_INTERNAL_ERROR,
- _("cannot set IP address on bridge
'%s'"),
- network->def->bridge);
- goto err_delbr;
+ /* Add the IP address/netmask to the bridge */
+ if (networkAddAddrToBridge(driver, network, ipdef) < 0) {
+ goto err2;
}
}
+ /* Bring up the bridge interface */
if ((err = brSetInterfaceUp(driver->brctl, network->def->bridge, 1))) {
virReportSystemError(err,
_("failed to bring the bridge '%s' up"),
network->def->bridge);
- goto err_delbr;
+ goto err2;
}
- if (ipv4def && networkAddIptablesRules(driver, network, ipv4def) < 0)
- goto err_delbr1;
-
+ /* If forwardType != NONE, turn on global IP forwarding */
if (network->def->forwardType != VIR_NETWORK_FORWARD_NONE &&
networkEnableIpForwarding() < 0) {
virReportSystemError(errno, "%s",
_("failed to enable IP forwarding"));
- goto err_delbr2;
+ goto err3;
}
- /*
- * Start the dhcp daemon for the 1st (and only supported) ipv4
- * address.
- */
- if (ipv4def && dhcpStartDhcpDaemon(network, ipv4def) < 0)
- goto err_delbr2;
+
+ /* start dnsmasq if there are any IPv4 addresses */
+ if (v4present && networkStartDhcpDaemon(network) < 0)
+ goto err3;
/* Persist the live configuration now we have bridge info */
if (virNetworkSaveConfig(NETWORK_STATE_DIR, network->def) < 0) {
- goto err_kill;
+ goto err4;
}
network->active = 1;
return 0;
- err_kill:
+ err4:
+ if (!save_err)
+ save_err = virSaveLastError();
+
if (network->dnsmasqPid > 0) {
kill(network->dnsmasqPid, SIGTERM);
network->dnsmasqPid = -1;
}
- err_delbr2:
- save_err = virSaveLastError();
- if (ipv4def)
- networkRemoveIptablesRules(driver, network, ipv4def);
- if (save_err) {
- virSetError(save_err);
- virFreeError(save_err);
- }
-
- err_delbr1:
+ err3:
+ if (!save_err)
+ save_err = virSaveLastError();
if ((err = brSetInterfaceUp(driver->brctl, network->def->bridge, 0))) {
char ebuf[1024];
VIR_WARN("Failed to bring down bridge '%s' : %s",
network->def->bridge, virStrerror(err, ebuf, sizeof ebuf));
}
- err_delbr:
+ err2:
+ if (!save_err)
+ save_err = virSaveLastError();
+ networkRemoveIptablesRules(driver, network);
+
+ err1:
+ if (!save_err)
+ save_err = virSaveLastError();
if ((err = brDeleteBridge(driver->brctl, network->def->bridge))) {
char ebuf[1024];
VIR_WARN("Failed to delete bridge '%s' : %s",
network->def->bridge, virStrerror(err, ebuf, sizeof ebuf));
}
+ if (save_err) {
+ virSetError(save_err);
+ virFreeError(save_err);
+ }
return -1;
}
@@ -1268,7 +1387,6 @@ static int networkShutdownNetworkDaemon(struct network_driver
*driver,
{
int err;
char *stateFile;
- virNetworkIpDefPtr ipv4def;
VIR_INFO(_("Shutting down network '%s'"),
network->def->name);
@@ -1285,17 +1403,14 @@ static int networkShutdownNetworkDaemon(struct network_driver
*driver,
if (network->dnsmasqPid > 0)
kill(network->dnsmasqPid, SIGTERM);
- /* find the one allowed IPv4 ip address in the definition */
- ipv4def = virNetworkDefGetIpByIndex(network->def, AF_INET, 0);
- if (ipv4def)
- networkRemoveIptablesRules(driver, network, ipv4def);
-
char ebuf[1024];
if ((err = brSetInterfaceUp(driver->brctl, network->def->bridge, 0))) {
VIR_WARN("Failed to bring down bridge '%s' : %s",
network->def->bridge, virStrerror(err, ebuf, sizeof ebuf));
}
+ networkRemoveIptablesRules(driver, network);
+
if ((err = brDeleteBridge(driver->brctl, network->def->bridge))) {
VIR_WARN("Failed to delete bridge '%s' : %s",
network->def->bridge, virStrerror(err, ebuf, sizeof ebuf));
@@ -1552,10 +1667,11 @@ cleanup:
static virNetworkPtr networkDefine(virConnectPtr conn, const char *xml) {
struct network_driver *driver = conn->networkPrivateData;
- virNetworkIpDefPtr ipv4def;
+ virNetworkIpDefPtr ipdef, ipv4def = NULL;
virNetworkDefPtr def;
virNetworkObjPtr network = NULL;
virNetworkPtr ret = NULL;
+ int ii;
networkDriverLock(driver);
@@ -1583,10 +1699,33 @@ static virNetworkPtr networkDefine(virConnectPtr conn, const char
*xml) {
goto cleanup;
}
- /* we only support dhcp on one IPv4 address per defined network */
- ipv4def = virNetworkDefGetIpByIndex(network->def, AF_INET, 0);
+ /* we only support dhcp on one IPv4 address per defined network, and currently
+ * don't support IPv6.
+ */
+ for (ii = 0;
+ (ipdef = virNetworkDefGetIpByIndex(network->def, AF_UNSPEC, ii));
+ ii++) {
+ if (VIR_SOCKET_IS_FAMILY(&ipdef->address, AF_INET)) {
+ if (ipdef->nranges || ipdef->nhosts) {
+ if (ipv4def) {
+ networkReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ "%s", _("Multiple dhcp sections
found. dhcp is supported only for a single IPv4 address on each network"));
+ goto cleanup;
+ } else {
+ ipv4def = ipdef;
+ }
+ }
+ } else {
+ /* we currently only support IPv4 */
+ networkReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unsupported address family '%s' (%d) in
network definition"),
+ ipdef->family ? ipdef->family :
"unspecified",
+ VIR_SOCKET_FAMILY(&ipdef->address));
+ goto cleanup;
- if (ipv4def && ipv4def->nhosts > 0) {
+ }
+ }
+ if (ipv4def) {
dnsmasqContext *dctx = dnsmasqContextNew(network->def->name,
DNSMASQ_STATE_DIR);
if (dctx == NULL)
goto cleanup;
@@ -1609,7 +1748,7 @@ static int networkUndefine(virNetworkPtr net) {
struct network_driver *driver = net->conn->networkPrivateData;
virNetworkObjPtr network;
virNetworkIpDefPtr ipv4def;
- int ret = -1;
+ int ret = -1, ii;
networkDriverLock(driver);
@@ -1631,10 +1770,14 @@ static int networkUndefine(virNetworkPtr net) {
network) < 0)
goto cleanup;
- /* find the one allowed IPv4 ip address in the definition */
- ipv4def = virNetworkDefGetIpByIndex(network->def, AF_INET, 0);
-
- if (ipv4def && ipv4def->nhosts > 0) {
+ /* we only support dhcp on one IPv4 address per defined network */
+ for (ii = 0;
+ (ipv4def = virNetworkDefGetIpByIndex(network->def, AF_INET, ii));
+ ii++) {
+ if (ipv4def->nranges || ipv4def->nhosts)
+ break;
+ }
+ if (ipv4def) {
dnsmasqContext *dctx = dnsmasqContextNew(network->def->name,
DNSMASQ_STATE_DIR);
if (dctx == NULL)
goto cleanup;
--
1.7.3.4
11:26 a.m.
New subject: [libvirt] [PATCHv2 10/13] Support multiple IP addresses on one network in bridge_driver.c
On 12/22/2010 11:58 AM, Laine Stump wrote:
This patch reorganizes the code in bridge_driver.c to account for
the
concept of a single network with multiple IP addresses, without adding
in the extra variable of IPv6. A small bit of code has been
temporarily added that checks all given addresses to verify they are
IPv4 - this will be removed when full IPv6 support is turned on.
@@ -747,23 +792,56 @@ networkAddRoutingIptablesRules(struct
network_driver *driver,
return 0;
-
- routeerr2:
+routeerr2:
Interesting mix of formatting changes as well as refactoring, but not
enough of an issue to insist on splitting this into two patches.
+static int
+networkAddIpSpecificIptablesRules(struct network_driver *driver,
+ virNetworkObjPtr network,
+ virNetworkIpDefPtr ipdef)
+{
+ if (network->def->forwardType == VIR_NETWORK_FORWARD_NAT &&
+ networkAddMasqueradingIptablesRules(driver, network, ipdef) < 0)
+ return -1;
+ else if (network->def->forwardType == VIR_NETWORK_FORWARD_ROUTE &&
Technically, the else is not necessary since the previous condition
returns. But it doesn't bother me to keep it in place.
ACK.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
12:51 p.m.
New subject: [libvirt] [PATCHv2 10/13] Support multiple IP addresses on one network in bridge_driver.c
On 12/23/2010 12:26 PM, Eric Blake wrote:
On 12/22/2010 11:58 AM, Laine Stump wrote:
> This patch reorganizes the code in bridge_driver.c to account for the
> concept of a single network with multiple IP addresses, without adding
> in the extra variable of IPv6. A small bit of code has been
> temporarily added that checks all given addresses to verify they are
> IPv4 - this will be removed when full IPv6 support is turned on.
> @@ -747,23 +792,56 @@ networkAddRoutingIptablesRules(struct network_driver *driver,
>
> return 0;
>
> -
> - routeerr2:
> +routeerr2:
Interesting mix of formatting changes as well as refactoring, but not
enough of an issue to insist on splitting this into two patches.
Many of those are due to ctl-X ctl-Q in emacs after doing a block copy.
Most of these functions I approached by rewriting them "from scratch",
using chunks of the existing functions as a model, in some cases copying
them, and in other cases rewriting from scratch.
> +static int
> +networkAddIpSpecificIptablesRules(struct network_driver *driver,
> + virNetworkObjPtr network,
> + virNetworkIpDefPtr ipdef)
> +{
> + if (network->def->forwardType == VIR_NETWORK_FORWARD_NAT&&
> + networkAddMasqueradingIptablesRules(driver, network, ipdef)< 0)
> + return -1;
> + else if (network->def->forwardType == VIR_NETWORK_FORWARD_ROUTE&&
Technically, the else is not necessary since the previous condition
returns. But it doesn't bother me to keep it in place.
I'll remove it anyway (of course it's going to change in a subsequent
patch ;-)
ACK.
12:58 p.m.
New subject: [libvirt] [PATCHv2 11/13] Update iptables.c to also support ip6tables.
All of the iptables functions eventually call down to a single
bottom-level function, and fortunately, ip6tables syntax (for all the
args that we use) is identical to iptables format (except the
addresses), so all we need to do is:
1) Get an address family down to the lowest level function in each
case, either implied through an address, or explicitly when no
address is in the parameter list, and
2) At the lowest level, just decide whether to call "iptables" or
"ip6tables" based on the family.
The location of the ip6tables binary is determined at build time by
autoconf. If a particular target system happens to not have ip6tables
installed, any attempts to run it will generate an error, but that
won't happen unless someone tries to define an IPv6 address for a
network. This is identical behavior to IPv4 addresses and iptables.
---
No changes from V1.
configure.ac | 3 ++
src/network/bridge_driver.c | 54 ++++++++++++++++++-------------
src/util/iptables.c | 75 +++++++++++++++++++++++++++++++-----------
src/util/iptables.h | 10 ++++++
4 files changed, 99 insertions(+), 43 deletions(-)
diff --git a/configure.ac b/configure.ac
index 4db613a..76652f4 100644
--- a/configure.ac
+++ b/configure.ac
@@ -312,6 +312,9 @@ AC_DEFINE_UNQUOTED([IP_PATH], "$IP_PATH", [path to ip
binary])
AC_PATH_PROG([IPTABLES_PATH], [iptables], /sbin/iptables, [/usr/sbin:$PATH])
AC_DEFINE_UNQUOTED([IPTABLES_PATH], "$IPTABLES_PATH", [path to iptables
binary])
+AC_PATH_PROG([IP6TABLES_PATH], [ip6tables], /sbin/ip6tables, [/usr/sbin:$PATH])
+AC_DEFINE_UNQUOTED([IP6TABLES_PATH], "$IP6TABLES_PATH", [path to ip6tables
binary])
+
AC_PATH_PROG([EBTABLES_PATH], [ebtables], /sbin/ebtables, [/usr/sbin:$PATH])
AC_DEFINE_UNQUOTED([EBTABLES_PATH], "$EBTABLES_PATH", [path to ebtables
binary])
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 03afe6c..e0d5aaf 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -842,14 +842,16 @@ networkAddGeneralIptablesRules(struct network_driver *driver,
/* allow DHCP requests through to dnsmasq */
- if (iptablesAddTcpInput(driver->iptables, network->def->bridge, 67) < 0)
{
+ if (iptablesAddTcpInput(driver->iptables, AF_INET,
+ network->def->bridge, 67) < 0) {
networkReportError(VIR_ERR_SYSTEM_ERROR,
_("failed to add iptables rule to allow DHCP requests
from '%s'"),
network->def->bridge);
goto err1;
}
- if (iptablesAddUdpInput(driver->iptables, network->def->bridge, 67) < 0)
{
+ if (iptablesAddUdpInput(driver->iptables, AF_INET,
+ network->def->bridge, 67) < 0) {
networkReportError(VIR_ERR_SYSTEM_ERROR,
_("failed to add iptables rule to allow DHCP requests
from '%s'"),
network->def->bridge);
@@ -871,14 +873,16 @@ networkAddGeneralIptablesRules(struct network_driver *driver,
}
/* allow DNS requests through to dnsmasq */
- if (iptablesAddTcpInput(driver->iptables, network->def->bridge, 53) < 0)
{
+ if (iptablesAddTcpInput(driver->iptables, AF_INET,
+ network->def->bridge, 53) < 0) {
networkReportError(VIR_ERR_SYSTEM_ERROR,
_("failed to add iptables rule to allow DNS requests from
'%s'"),
network->def->bridge);
goto err3;
}
- if (iptablesAddUdpInput(driver->iptables, network->def->bridge, 53) < 0)
{
+ if (iptablesAddUdpInput(driver->iptables, AF_INET,
+ network->def->bridge, 53) < 0) {
networkReportError(VIR_ERR_SYSTEM_ERROR,
_("failed to add iptables rule to allow DNS requests from
'%s'"),
network->def->bridge);
@@ -887,7 +891,8 @@ networkAddGeneralIptablesRules(struct network_driver *driver,
/* allow TFTP requests through to dnsmasq if necessary */
if (ipv4def && ipv4def->tftproot &&
- iptablesAddUdpInput(driver->iptables, network->def->bridge, 69) < 0)
{
+ iptablesAddUdpInput(driver->iptables, AF_INET,
+ network->def->bridge, 69) < 0) {
networkReportError(VIR_ERR_SYSTEM_ERROR,
_("failed to add iptables rule to allow TFTP requests
from '%s'"),
network->def->bridge);
@@ -896,14 +901,16 @@ networkAddGeneralIptablesRules(struct network_driver *driver,
/* Catch all rules to block forwarding to/from bridges */
- if (iptablesAddForwardRejectOut(driver->iptables, network->def->bridge) <
0) {
+ if (iptablesAddForwardRejectOut(driver->iptables, AF_INET,
+ network->def->bridge) < 0) {
networkReportError(VIR_ERR_SYSTEM_ERROR,
_("failed to add iptables rule to block outbound traffic
from '%s'"),
network->def->bridge);
goto err6;
}
- if (iptablesAddForwardRejectIn(driver->iptables, network->def->bridge) <
0) {
+ if (iptablesAddForwardRejectIn(driver->iptables, AF_INET,
+ network->def->bridge) < 0) {
networkReportError(VIR_ERR_SYSTEM_ERROR,
_("failed to add iptables rule to block inbound traffic
to '%s'"),
network->def->bridge);
@@ -911,7 +918,8 @@ networkAddGeneralIptablesRules(struct network_driver *driver,
}
/* Allow traffic between guests on the same bridge */
- if (iptablesAddForwardAllowCross(driver->iptables, network->def->bridge)
< 0) {
+ if (iptablesAddForwardAllowCross(driver->iptables, AF_INET,
+ network->def->bridge) < 0) {
networkReportError(VIR_ERR_SYSTEM_ERROR,
_("failed to add iptables rule to allow cross bridge
traffic on '%s'"),
network->def->bridge);
@@ -922,21 +930,21 @@ networkAddGeneralIptablesRules(struct network_driver *driver,
/* unwind in reverse order from the point of failure */
err8:
- iptablesRemoveForwardRejectIn(driver->iptables, network->def->bridge);
+ iptablesRemoveForwardRejectIn(driver->iptables, AF_INET,
network->def->bridge);
err7:
- iptablesRemoveForwardRejectOut(driver->iptables, network->def->bridge);
+ iptablesRemoveForwardRejectOut(driver->iptables, AF_INET,
network->def->bridge);
err6:
if (ipv4def && ipv4def->tftproot) {
- iptablesRemoveUdpInput(driver->iptables, network->def->bridge, 69);
+ iptablesRemoveUdpInput(driver->iptables, AF_INET, network->def->bridge,
69);
}
err5:
- iptablesRemoveUdpInput(driver->iptables, network->def->bridge, 53);
+ iptablesRemoveUdpInput(driver->iptables, AF_INET, network->def->bridge,
53);
err4:
- iptablesRemoveTcpInput(driver->iptables, network->def->bridge, 53);
+ iptablesRemoveTcpInput(driver->iptables, AF_INET, network->def->bridge,
53);
err3:
- iptablesRemoveUdpInput(driver->iptables, network->def->bridge, 67);
+ iptablesRemoveUdpInput(driver->iptables, AF_INET, network->def->bridge,
67);
err2:
- iptablesRemoveTcpInput(driver->iptables, network->def->bridge, 67);
+ iptablesRemoveTcpInput(driver->iptables, AF_INET, network->def->bridge,
67);
err1:
return -1;
}
@@ -955,20 +963,20 @@ networkRemoveGeneralIptablesRules(struct network_driver *driver,
break;
}
- iptablesRemoveForwardAllowCross(driver->iptables, network->def->bridge);
- iptablesRemoveForwardRejectIn(driver->iptables, network->def->bridge);
- iptablesRemoveForwardRejectOut(driver->iptables, network->def->bridge);
+ iptablesRemoveForwardAllowCross(driver->iptables, AF_INET,
network->def->bridge);
+ iptablesRemoveForwardRejectIn(driver->iptables, AF_INET,
network->def->bridge);
+ iptablesRemoveForwardRejectOut(driver->iptables, AF_INET,
network->def->bridge);
if (ipv4def && ipv4def->tftproot) {
- iptablesRemoveUdpInput(driver->iptables, network->def->bridge, 69);
+ iptablesRemoveUdpInput(driver->iptables, AF_INET, network->def->bridge,
69);
}
- iptablesRemoveUdpInput(driver->iptables, network->def->bridge, 53);
- iptablesRemoveTcpInput(driver->iptables, network->def->bridge, 53);
+ iptablesRemoveUdpInput(driver->iptables, AF_INET, network->def->bridge,
53);
+ iptablesRemoveTcpInput(driver->iptables, AF_INET, network->def->bridge,
53);
if (ipv4def && (ipv4def->nranges || ipv4def->nhosts)) {
iptablesRemoveOutputFixUdpChecksum(driver->iptables,
network->def->bridge, 68);
}
- iptablesRemoveUdpInput(driver->iptables, network->def->bridge, 67);
- iptablesRemoveTcpInput(driver->iptables, network->def->bridge, 67);
+ iptablesRemoveUdpInput(driver->iptables, AF_INET, network->def->bridge,
67);
+ iptablesRemoveTcpInput(driver->iptables, AF_INET, network->def->bridge,
67);
}
static int
diff --git a/src/util/iptables.c b/src/util/iptables.c
index f7b692c..647e7ae 100644
--- a/src/util/iptables.c
+++ b/src/util/iptables.c
@@ -99,14 +99,17 @@ iptRulesNew(const char *table,
}
static int ATTRIBUTE_SENTINEL
-iptablesAddRemoveRule(iptRules *rules, int action, const char *arg, ...)
+iptablesAddRemoveRule(iptRules *rules, int family, int action,
+ const char *arg, ...)
{
va_list args;
int ret;
virCommandPtr cmd;
const char *s;
- cmd = virCommandNew(IPTABLES_PATH);
+ cmd = virCommandNew((family == AF_INET6)
+ ? IP6TABLES_PATH : IPTABLES_PATH);
+
virCommandAddArgList(cmd, "--table", rules->table,
action == ADD ? "--insert" : "--delete",
rules->chain, arg, NULL);
@@ -177,6 +180,7 @@ iptablesContextFree(iptablesContext *ctx)
static int
iptablesInput(iptablesContext *ctx,
+ int family,
const char *iface,
int port,
int action,
@@ -188,6 +192,7 @@ iptablesInput(iptablesContext *ctx,
portstr[sizeof(portstr) - 1] = '\0';
return iptablesAddRemoveRule(ctx->input_filter,
+ family,
action,
"--in-interface", iface,
"--protocol", tcp ? "tcp" :
"udp",
@@ -210,10 +215,11 @@ iptablesInput(iptablesContext *ctx,
int
iptablesAddTcpInput(iptablesContext *ctx,
+ int family,
const char *iface,
int port)
{
- return iptablesInput(ctx, iface, port, ADD, 1);
+ return iptablesInput(ctx, family, iface, port, ADD, 1);
}
/**
@@ -229,10 +235,11 @@ iptablesAddTcpInput(iptablesContext *ctx,
*/
int
iptablesRemoveTcpInput(iptablesContext *ctx,
+ int family,
const char *iface,
int port)
{
- return iptablesInput(ctx, iface, port, REMOVE, 1);
+ return iptablesInput(ctx, family, iface, port, REMOVE, 1);
}
/**
@@ -249,10 +256,11 @@ iptablesRemoveTcpInput(iptablesContext *ctx,
int
iptablesAddUdpInput(iptablesContext *ctx,
+ int family,
const char *iface,
int port)
{
- return iptablesInput(ctx, iface, port, ADD, 0);
+ return iptablesInput(ctx, family, iface, port, ADD, 0);
}
/**
@@ -268,10 +276,11 @@ iptablesAddUdpInput(iptablesContext *ctx,
*/
int
iptablesRemoveUdpInput(iptablesContext *ctx,
+ int family,
const char *iface,
int port)
{
- return iptablesInput(ctx, iface, port, REMOVE, 0);
+ return iptablesInput(ctx, family, iface, port, REMOVE, 0);
}
@@ -282,9 +291,10 @@ static char *iptablesFormatNetwork(virSocketAddr *netaddr,
char *netstr;
char *ret;
- if (!VIR_SOCKET_IS_FAMILY(netaddr, AF_INET)) {
+ if (!(VIR_SOCKET_IS_FAMILY(netaddr, AF_INET) ||
+ VIR_SOCKET_IS_FAMILY(netaddr, AF_INET6))) {
iptablesError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("Only IPv4 addresses can be used with iptables"));
+ _("Only IPv4 or IPv6 addresses can be used with
iptables"));
return NULL;
}
@@ -327,6 +337,7 @@ iptablesForwardAllowOut(iptablesContext *ctx,
if (physdev && physdev[0]) {
ret = iptablesAddRemoveRule(ctx->forward_filter,
+ VIR_SOCKET_FAMILY(netaddr),
action,
"--source", networkstr,
"--in-interface", iface,
@@ -335,6 +346,7 @@ iptablesForwardAllowOut(iptablesContext *ctx,
NULL);
} else {
ret = iptablesAddRemoveRule(ctx->forward_filter,
+ VIR_SOCKET_FAMILY(netaddr),
action,
"--source", networkstr,
"--in-interface", iface,
@@ -411,6 +423,7 @@ iptablesForwardAllowRelatedIn(iptablesContext *ctx,
if (physdev && physdev[0]) {
ret = iptablesAddRemoveRule(ctx->forward_filter,
+ VIR_SOCKET_FAMILY(netaddr),
action,
"--destination", networkstr,
"--in-interface", physdev,
@@ -421,6 +434,7 @@ iptablesForwardAllowRelatedIn(iptablesContext *ctx,
NULL);
} else {
ret = iptablesAddRemoveRule(ctx->forward_filter,
+ VIR_SOCKET_FAMILY(netaddr),
action,
"--destination", networkstr,
"--out-interface", iface,
@@ -497,6 +511,7 @@ iptablesForwardAllowIn(iptablesContext *ctx,
if (physdev && physdev[0]) {
ret = iptablesAddRemoveRule(ctx->forward_filter,
+ VIR_SOCKET_FAMILY(netaddr),
action,
"--destination", networkstr,
"--in-interface", physdev,
@@ -505,6 +520,7 @@ iptablesForwardAllowIn(iptablesContext *ctx,
NULL);
} else {
ret = iptablesAddRemoveRule(ctx->forward_filter,
+ VIR_SOCKET_FAMILY(netaddr),
action,
"--destination", networkstr,
"--out-interface", iface,
@@ -567,10 +583,12 @@ iptablesRemoveForwardAllowIn(iptablesContext *ctx,
*/
static int
iptablesForwardAllowCross(iptablesContext *ctx,
+ int family,
const char *iface,
int action)
{
return iptablesAddRemoveRule(ctx->forward_filter,
+ family,
action,
"--in-interface", iface,
"--out-interface", iface,
@@ -591,8 +609,10 @@ iptablesForwardAllowCross(iptablesContext *ctx,
*/
int
iptablesAddForwardAllowCross(iptablesContext *ctx,
- const char *iface) {
- return iptablesForwardAllowCross(ctx, iface, ADD);
+ int family,
+ const char *iface)
+{
+ return iptablesForwardAllowCross(ctx, family, iface, ADD);
}
/**
@@ -608,8 +628,10 @@ iptablesAddForwardAllowCross(iptablesContext *ctx,
*/
int
iptablesRemoveForwardAllowCross(iptablesContext *ctx,
- const char *iface) {
- return iptablesForwardAllowCross(ctx, iface, REMOVE);
+ int family,
+ const char *iface)
+{
+ return iptablesForwardAllowCross(ctx, family, iface, REMOVE);
}
@@ -618,14 +640,16 @@ iptablesRemoveForwardAllowCross(iptablesContext *ctx,
*/
static int
iptablesForwardRejectOut(iptablesContext *ctx,
+ int family,
const char *iface,
int action)
{
return iptablesAddRemoveRule(ctx->forward_filter,
- action,
- "--in-interface", iface,
- "--jump", "REJECT",
- NULL);
+ family,
+ action,
+ "--in-interface", iface,
+ "--jump", "REJECT",
+ NULL);
}
/**
@@ -640,9 +664,10 @@ iptablesForwardRejectOut(iptablesContext *ctx,
*/
int
iptablesAddForwardRejectOut(iptablesContext *ctx,
+ int family,
const char *iface)
{
- return iptablesForwardRejectOut(ctx, iface, ADD);
+ return iptablesForwardRejectOut(ctx, family, iface, ADD);
}
/**
@@ -657,9 +682,10 @@ iptablesAddForwardRejectOut(iptablesContext *ctx,
*/
int
iptablesRemoveForwardRejectOut(iptablesContext *ctx,
+ int family,
const char *iface)
{
- return iptablesForwardRejectOut(ctx, iface, REMOVE);
+ return iptablesForwardRejectOut(ctx, family, iface, REMOVE);
}
@@ -670,10 +696,12 @@ iptablesRemoveForwardRejectOut(iptablesContext *ctx,
*/
static int
iptablesForwardRejectIn(iptablesContext *ctx,
+ int family,
const char *iface,
int action)
{
return iptablesAddRemoveRule(ctx->forward_filter,
+ family,
action,
"--out-interface", iface,
"--jump", "REJECT",
@@ -692,9 +720,10 @@ iptablesForwardRejectIn(iptablesContext *ctx,
*/
int
iptablesAddForwardRejectIn(iptablesContext *ctx,
+ int family,
const char *iface)
{
- return iptablesForwardRejectIn(ctx, iface, ADD);
+ return iptablesForwardRejectIn(ctx, family, iface, ADD);
}
/**
@@ -709,9 +738,10 @@ iptablesAddForwardRejectIn(iptablesContext *ctx,
*/
int
iptablesRemoveForwardRejectIn(iptablesContext *ctx,
+ int family,
const char *iface)
{
- return iptablesForwardRejectIn(ctx, iface, REMOVE);
+ return iptablesForwardRejectIn(ctx, family, iface, REMOVE);
}
@@ -735,6 +765,7 @@ iptablesForwardMasquerade(iptablesContext *ctx,
if (protocol && protocol[0]) {
if (physdev && physdev[0]) {
ret = iptablesAddRemoveRule(ctx->nat_postrouting,
+ VIR_SOCKET_FAMILY(netaddr),
action,
"--source", networkstr,
"-p", protocol,
@@ -745,6 +776,7 @@ iptablesForwardMasquerade(iptablesContext *ctx,
NULL);
} else {
ret = iptablesAddRemoveRule(ctx->nat_postrouting,
+ VIR_SOCKET_FAMILY(netaddr),
action,
"--source", networkstr,
"-p", protocol,
@@ -756,6 +788,7 @@ iptablesForwardMasquerade(iptablesContext *ctx,
} else {
if (physdev && physdev[0]) {
ret = iptablesAddRemoveRule(ctx->nat_postrouting,
+ VIR_SOCKET_FAMILY(netaddr),
action,
"--source", networkstr,
"!", "--destination",
networkstr,
@@ -764,6 +797,7 @@ iptablesForwardMasquerade(iptablesContext *ctx,
NULL);
} else {
ret = iptablesAddRemoveRule(ctx->nat_postrouting,
+ VIR_SOCKET_FAMILY(netaddr),
action,
"--source", networkstr,
"!", "--destination",
networkstr,
@@ -834,6 +868,7 @@ iptablesOutputFixUdpChecksum(iptablesContext *ctx,
portstr[sizeof(portstr) - 1] = '\0';
return iptablesAddRemoveRule(ctx->mangle_postrouting,
+ AF_INET,
action,
"--out-interface", iface,
"--protocol", "udp",
diff --git a/src/util/iptables.h b/src/util/iptables.h
index 982acf1..572d612 100644
--- a/src/util/iptables.h
+++ b/src/util/iptables.h
@@ -30,16 +30,20 @@ iptablesContext *iptablesContextNew (void);
void iptablesContextFree (iptablesContext *ctx);
int iptablesAddTcpInput (iptablesContext *ctx,
+ int family,
const char *iface,
int port);
int iptablesRemoveTcpInput (iptablesContext *ctx,
+ int family,
const char *iface,
int port);
int iptablesAddUdpInput (iptablesContext *ctx,
+ int family,
const char *iface,
int port);
int iptablesRemoveUdpInput (iptablesContext *ctx,
+ int family,
const char *iface,
int port);
@@ -77,18 +81,24 @@ int iptablesRemoveForwardAllowIn (iptablesContext
*ctx,
const char *physdev);
int iptablesAddForwardAllowCross (iptablesContext *ctx,
+ int family,
const char *iface);
int iptablesRemoveForwardAllowCross (iptablesContext *ctx,
+ int family,
const char *iface);
int iptablesAddForwardRejectOut (iptablesContext *ctx,
+ int family,
const char *iface);
int iptablesRemoveForwardRejectOut (iptablesContext *ctx,
+ int family,
const char *iface);
int iptablesAddForwardRejectIn (iptablesContext *ctx,
+ int family,
const char *iface);
int iptablesRemoveForwardRejectIn (iptablesContext *ctx,
+ int family,
const char *iface);
int iptablesAddForwardMasquerade (iptablesContext *ctx,
--
1.7.3.4
11:30 a.m.
New subject: [libvirt] [PATCHv2 11/13] Update iptables.c to also support ip6tables.
On 12/22/2010 11:58 AM, Laine Stump wrote:
All of the iptables functions eventually call down to a single
bottom-level function, and fortunately, ip6tables syntax (for all the
args that we use) is identical to iptables format (except the
addresses), so all we need to do is:
1) Get an address family down to the lowest level function in each
case, either implied through an address, or explicitly when no
address is in the parameter list, and
2) At the lowest level, just decide whether to call "iptables" or
"ip6tables" based on the family.
The location of the ip6tables binary is determined at build time by
autoconf. If a particular target system happens to not have ip6tables
installed, any attempts to run it will generate an error, but that
won't happen unless someone tries to define an IPv6 address for a
network. This is identical behavior to IPv4 addresses and iptables.
err6:
if (ipv4def && ipv4def->tftproot) {
- iptablesRemoveUdpInput(driver->iptables, network->def->bridge, 69);
+ iptablesRemoveUdpInput(driver->iptables, AF_INET, network->def->bridge,
69);
}
Indentation.
ACK with that nit fixed.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
12:58 p.m.
New subject: [libvirt] [PATCHv2 12/13] Turn on IPv6 support in the bridge_driver.c virtual network driver
At this point everything is already in place to make IPv6 happen, we just
need to add a few rules, remove some checks for IPv4-only, and document
the changes to the XML on the website.
---
No changes from V1.
docs/formatnetwork.html.in | 35 +++++++--
src/network/bridge_driver.c | 186 ++++++++++++++++++++++++++++++++----------
2 files changed, 169 insertions(+), 52 deletions(-)
diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in
index 7a24518..b1b0485 100644
--- a/docs/formatnetwork.html.in
+++ b/docs/formatnetwork.html.in
@@ -85,8 +85,13 @@
</dd>
<dt><code>forward</code></dt>
<dd>Inclusion of the <code>forward</code> element indicates that
- the virtual network is to be connected to the physical LAN. If
- no attributes are set, NAT forwarding will be used for connectivity.
+ the virtual network is to be connected to the physical
+ LAN. the <code>mode</code> attribute determines the method of
+ forwarding; possible selections are 'nat' and 'route'. If mode
+ is not specified, NAT forwarding will be used for
+ connectivity. If a network has any IPv6 addresses defined,
+ even if <code>mode</code> is given as 'nat', the IPv6
traffic
+ will be forwarded using routing, since IPv6 has no concept of NAT.
Firewall rules will allow forwarding to any other network device whether
ethernet, wireless, dialup, or VPN. If the <code>dev</code>
attribute
is set, the firewall rules will restrict forwarding to the named
@@ -118,21 +123,37 @@
<dl>
<dt><code>ip</code></dt>
<dd>The <code>address</code> attribute defines an IPv4 address
in
- dotted-decimal format, that will be configured on the bridge
+ dotted-decimal format, or an IPv6 address in standard
+ colon-separated hexadecimal format, that will be configured on
+ the bridge
device associated with the virtual network. To the guests this
- address will be their default route. The <code>netmask</code>
+ address will be their default route. For IPv4 addresses, the
<code>netmask</code>
attribute defines the significant bits of the network address,
- again specified in dotted-decimal format. <span
class="since">Since 0.3.0</span>
+ again specified in dotted-decimal format. For IPv6 addresses,
+ and as an alternate method for IPv4 addresses, you can specify
+ the significant bits of the network address with the
<code>prefix</code>
+ attribute, which is an integer (for example,
<code>netmask='255.255.255.0'</code>
+ could also be given as <code>prefix='24'</code>. The
<code>family</code>
+ attribute is used to specify the type of address - 'ipv4' or
'ipv6'; if no
+ <code>family</code> is given, 'ipv4' is assumed. A network
can have more than
+ one of each family of address defined, but only a single address can have a
+ <code>dhcp</code> or <code>tftp</code> element. <span
class="since">Since 0.3.0;
+ IPv6, multiple addresses on a single network, <code>family</code>,
and
+ <code>prefix</code> since 0.8.7</span>
</dd><dt><code>tftp</code></dt><dd>Immediately
within
the <code>ip</code> element there is an optional
<code>tftp</code>
element. The presence of this element and of its attribute
<code>root</code> enables TFTP services. The attribute specifies
- the path to the root directory served via TFTP.
+ the path to the root directory served via TFTP. <code>tftp</code> is
not
+ supported for IPv6 addresses, can only be specified on a single IPv4 address
+ per network.
<span class="since">Since 0.7.1</span>
</dd><dt><code>dhcp</code></dt><dd>Also within
the <code>ip</code> element there is an
optional <code>dhcp</code> element. The presence of this element
enables DHCP services on the virtual network. It will further
- contain one or more <code>range</code> elements.
+ contain one or more <code>range</code> elements. The
+ <code>dhcp</code> element is not supported for IPv6, and
+ is only supported on a single IP address per network for IPv4.
<span class="since">Since 0.3.0</span>
</dd>
<dt><code>range</code></dt>
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index e0d5aaf..d3adc32 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -824,6 +824,65 @@ networkRemoveRoutingIptablesRules(struct network_driver *driver,
}
}
+/* Add all once/network rules required for IPv6 (if any IPv6 addresses are defined) */
+static int
+networkAddGeneralIp6tablesRules(struct network_driver *driver,
+ virNetworkObjPtr network)
+{
+
+ if (!virNetworkDefGetIpByIndex(network->def, AF_INET6, 0))
+ return 0;
+
+ /* Catch all rules to block forwarding to/from bridges */
+
+ if (iptablesAddForwardRejectOut(driver->iptables, AF_INET6,
+ network->def->bridge) < 0) {
+ networkReportError(VIR_ERR_SYSTEM_ERROR,
+ _("failed to add ip6tables rule to block outbound traffic
from '%s'"),
+ network->def->bridge);
+ goto err1;
+ }
+
+ if (iptablesAddForwardRejectIn(driver->iptables, AF_INET6,
+ network->def->bridge) < 0) {
+ networkReportError(VIR_ERR_SYSTEM_ERROR,
+ _("failed to add ip6tables rule to block inbound traffic
to '%s'"),
+ network->def->bridge);
+ goto err2;
+ }
+
+ /* Allow traffic between guests on the same bridge */
+ if (iptablesAddForwardAllowCross(driver->iptables, AF_INET6,
+ network->def->bridge) < 0) {
+ networkReportError(VIR_ERR_SYSTEM_ERROR,
+ _("failed to add ip6tables rule to allow cross bridge
traffic on '%s'"),
+ network->def->bridge);
+ goto err3;
+ }
+
+ return 0;
+
+ /* unwind in reverse order from the point of failure */
+err3:
+ iptablesRemoveForwardRejectIn(driver->iptables, AF_INET6,
network->def->bridge);
+err2:
+ iptablesRemoveForwardRejectOut(driver->iptables, AF_INET6,
network->def->bridge);
+err1:
+ return -1;
+}
+
+static void
+networkRemoveGeneralIp6tablesRules(struct network_driver *driver,
+ virNetworkObjPtr network)
+{
+ if (!virNetworkDefGetIpByIndex(network->def, AF_INET6, 0))
+ return;
+
+ iptablesRemoveForwardAllowCross(driver->iptables, AF_INET6,
network->def->bridge);
+ iptablesRemoveForwardRejectIn(driver->iptables, AF_INET6,
network->def->bridge);
+ iptablesRemoveForwardRejectOut(driver->iptables, AF_INET6,
network->def->bridge);
+}
+
static int
networkAddGeneralIptablesRules(struct network_driver *driver,
virNetworkObjPtr network)
@@ -926,6 +985,11 @@ networkAddGeneralIptablesRules(struct network_driver *driver,
goto err8;
}
+ /* add IPv6 general rules, if needed */
+ if (networkAddGeneralIp6tablesRules(driver, network) < 0) {
+ goto err8;
+ }
+
return 0;
/* unwind in reverse order from the point of failure */
@@ -956,6 +1020,8 @@ networkRemoveGeneralIptablesRules(struct network_driver *driver,
int ii;
virNetworkIpDefPtr ipv4def;
+ networkRemoveGeneralIp6tablesRules(driver, network);
+
for (ii = 0;
(ipv4def = virNetworkDefGetIpByIndex(network->def, AF_INET, ii));
ii++) {
@@ -984,12 +1050,18 @@ networkAddIpSpecificIptablesRules(struct network_driver *driver,
virNetworkObjPtr network,
virNetworkIpDefPtr ipdef)
{
- if (network->def->forwardType == VIR_NETWORK_FORWARD_NAT &&
- networkAddMasqueradingIptablesRules(driver, network, ipdef) < 0)
- return -1;
- else if (network->def->forwardType == VIR_NETWORK_FORWARD_ROUTE &&
- networkAddRoutingIptablesRules(driver, network, ipdef) < 0)
- return -1;
+ /* NB: in the case of IPv6, routing rules are added when the
+ * forward mode is NAT. This is because IPv6 has no NAT.
+ */
+
+ if (network->def->forwardType == VIR_NETWORK_FORWARD_NAT) {
+ if (VIR_SOCKET_IS_FAMILY(&ipdef->address, AF_INET))
+ return networkAddMasqueradingIptablesRules(driver, network, ipdef);
+ else if (VIR_SOCKET_IS_FAMILY(&ipdef->address, AF_INET6))
+ return networkAddRoutingIptablesRules(driver, network, ipdef);
+ } else if (network->def->forwardType == VIR_NETWORK_FORWARD_ROUTE) {
+ return networkAddRoutingIptablesRules(driver, network, ipdef);
+ }
return 0;
}
@@ -999,10 +1071,14 @@ networkRemoveIpSpecificIptablesRules(struct network_driver
*driver,
virNetworkObjPtr network,
virNetworkIpDefPtr ipdef)
{
- if (network->def->forwardType == VIR_NETWORK_FORWARD_NAT)
- networkRemoveMasqueradingIptablesRules(driver, network, ipdef);
- else if (network->def->forwardType == VIR_NETWORK_FORWARD_ROUTE)
+ if (network->def->forwardType == VIR_NETWORK_FORWARD_NAT) {
+ if (VIR_SOCKET_IS_FAMILY(&ipdef->address, AF_INET))
+ networkRemoveMasqueradingIptablesRules(driver, network, ipdef);
+ else if (VIR_SOCKET_IS_FAMILY(&ipdef->address, AF_INET6))
+ networkRemoveRoutingIptablesRules(driver, network, ipdef);
+ } else if (network->def->forwardType == VIR_NETWORK_FORWARD_ROUTE) {
networkRemoveRoutingIptablesRules(driver, network, ipdef);
+ }
}
/* Add all rules for all ip addresses (and general rules) on a network */
@@ -1084,30 +1160,46 @@ networkEnableIpForwarding(void)
#define SYSCTL_PATH "/proc/sys"
-static int networkDisableIPV6(virNetworkObjPtr network)
+static int
+networkSetIPv6Sysctls(virNetworkObjPtr network)
{
char *field = NULL;
int ret = -1;
- if (virAsprintf(&field, SYSCTL_PATH "/net/ipv6/conf/%s/disable_ipv6",
network->def->bridge) < 0) {
- virReportOOMError();
- goto cleanup;
- }
+ if (!virNetworkDefGetIpByIndex(network->def, AF_INET6, 0)) {
+ /* Only set disable_ipv6 if there are no ipv6 addresses defined for
+ * the network.
+ */
+ if (virAsprintf(&field, SYSCTL_PATH
"/net/ipv6/conf/%s/disable_ipv6",
+ network->def->bridge) < 0) {
+ virReportOOMError();
+ goto cleanup;
+ }
- if (access(field, W_OK) < 0 && errno == ENOENT) {
- VIR_DEBUG("ipv6 appears to already be disabled on %s",
network->def->bridge);
- ret = 0;
- goto cleanup;
- }
+ if (access(field, W_OK) < 0 && errno == ENOENT) {
+ VIR_DEBUG("ipv6 appears to already be disabled on %s",
+ network->def->bridge);
+ ret = 0;
+ goto cleanup;
+ }
- if (virFileWriteStr(field, "1", 0) < 0) {
- virReportSystemError(errno,
- _("cannot enable %s"), field);
- goto cleanup;
+ if (virFileWriteStr(field, "1", 0) < 0) {
+ virReportSystemError(errno,
+ _("cannot enable %s"), field);
+ goto cleanup;
+ }
+ VIR_FREE(field);
}
- VIR_FREE(field);
- if (virAsprintf(&field, SYSCTL_PATH "/net/ipv6/conf/%s/accept_ra",
network->def->bridge) < 0) {
+ /* The rest of the ipv6 sysctl tunables should always be set,
+ * whether or not we're using ipv6 on this bridge.
+ */
+
+ /* Prevent guests from hijacking the host network by sending out
+ * their own router advertisements.
+ */
+ if (virAsprintf(&field, SYSCTL_PATH "/net/ipv6/conf/%s/accept_ra",
+ network->def->bridge) < 0) {
virReportOOMError();
goto cleanup;
}
@@ -1119,7 +1211,11 @@ static int networkDisableIPV6(virNetworkObjPtr network)
}
VIR_FREE(field);
- if (virAsprintf(&field, SYSCTL_PATH "/net/ipv6/conf/%s/autoconf",
network->def->bridge) < 0) {
+ /* All interfaces used as a gateway (which is what this is, by
+ * definition), must always have autoconf=0.
+ */
+ if (virAsprintf(&field, SYSCTL_PATH "/net/ipv6/conf/%s/autoconf",
+ network->def->bridge) < 0) {
virReportOOMError();
goto cleanup;
}
@@ -1261,7 +1357,7 @@ static int
networkStartNetworkDaemon(struct network_driver *driver,
virNetworkObjPtr network)
{
- int ii, err, v4present = 0;
+ int ii, err, v4present = 0, v6present = 0;
virErrorPtr save_err = NULL;
virNetworkIpDefPtr ipdef;
@@ -1300,8 +1396,10 @@ networkStartNetworkDaemon(struct network_driver *driver,
goto err1;
}
- /* Disable IPv6 on the bridge */
- if (networkDisableIPV6(network) < 0)
+ /* Disable IPv6 on the bridge if there are no IPv6 addresses
+ * defined, and set other IPv6 sysctl tunables appropriately.
+ */
+ if (networkSetIPv6Sysctls(network) < 0)
goto err1;
/* Add "once per network" rules */
@@ -1313,6 +1411,8 @@ networkStartNetworkDaemon(struct network_driver *driver,
ii++) {
if (VIR_SOCKET_IS_FAMILY(&ipdef->address, AF_INET))
v4present = 1;
+ if (VIR_SOCKET_IS_FAMILY(&ipdef->address, AF_INET6))
+ v6present = 1;
/* Add the IP address/netmask to the bridge */
if (networkAddAddrToBridge(driver, network, ipdef) < 0) {
@@ -1707,9 +1807,7 @@ static virNetworkPtr networkDefine(virConnectPtr conn, const char
*xml) {
goto cleanup;
}
- /* we only support dhcp on one IPv4 address per defined network, and currently
- * don't support IPv6.
- */
+ /* We only support dhcp on one IPv4 address per defined network */
for (ii = 0;
(ipdef = virNetworkDefGetIpByIndex(network->def, AF_UNSPEC, ii));
ii++) {
@@ -1723,14 +1821,6 @@ static virNetworkPtr networkDefine(virConnectPtr conn, const char
*xml) {
ipv4def = ipdef;
}
}
- } else {
- /* we currently only support IPv4 */
- networkReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("unsupported address family '%s' (%d) in
network definition"),
- ipdef->family ? ipdef->family :
"unspecified",
- VIR_SOCKET_FAMILY(&ipdef->address));
- goto cleanup;
-
}
}
if (ipv4def) {
@@ -1755,7 +1845,8 @@ cleanup:
static int networkUndefine(virNetworkPtr net) {
struct network_driver *driver = net->conn->networkPrivateData;
virNetworkObjPtr network;
- virNetworkIpDefPtr ipv4def;
+ virNetworkIpDefPtr ipdef;
+ int v4present = 0, v6present = 0;
int ret = -1, ii;
networkDriverLock(driver);
@@ -1780,12 +1871,17 @@ static int networkUndefine(virNetworkPtr net) {
/* we only support dhcp on one IPv4 address per defined network */
for (ii = 0;
- (ipv4def = virNetworkDefGetIpByIndex(network->def, AF_INET, ii));
+ (ipdef = virNetworkDefGetIpByIndex(network->def, AF_UNSPEC, ii));
ii++) {
- if (ipv4def->nranges || ipv4def->nhosts)
- break;
+ if (VIR_SOCKET_IS_FAMILY(&ipdef->address, AF_INET)) {
+ if (ipdef->nranges || ipdef->nhosts)
+ v4present = 1;
+ } else if (VIR_SOCKET_IS_FAMILY(&ipdef->address, AF_INET6)) {
+ v6present = 1;
+ }
}
- if (ipv4def) {
+
+ if (v4present) {
dnsmasqContext *dctx = dnsmasqContextNew(network->def->name,
DNSMASQ_STATE_DIR);
if (dctx == NULL)
goto cleanup;
--
1.7.3.4
11:41 a.m.
New subject: [libvirt] [PATCHv2 12/13] Turn on IPv6 support in the bridge_driver.c virtual network driver
On 12/22/2010 11:58 AM, Laine Stump wrote:
At this point everything is already in place to make IPv6 happen, we
just
need to add a few rules, remove some checks for IPv4-only, and document
the changes to the XML on the website.
---
No changes from V1.
docs/formatnetwork.html.in | 35 +++++++--
Yeah - a patch series with documentation updates!
static int
networkAddGeneralIptablesRules(struct network_driver *driver,
virNetworkObjPtr network)
@@ -926,6 +985,11 @@ networkAddGeneralIptablesRules(struct network_driver *driver,
goto err8;
}
+ /* add IPv6 general rules, if needed */
+ if (networkAddGeneralIp6tablesRules(driver, network) < 0) {
+ goto err8;
Should this be err9, with a step that undoes the previous action when
you get past the err8 failure point?
+ if (virAsprintf(&field, SYSCTL_PATH
"/net/ipv6/conf/%s/disable_ipv6",
+ network->def->bridge) < 0) {
...
+ if (virFileWriteStr(field, "1", 0) < 0) {
+ virReportSystemError(errno,
+ _("cannot enable %s"), field);
Misleading message; maybe "cannot write to %s to disable IPv6".
@@ -1755,7 +1845,8 @@ cleanup:
static int networkUndefine(virNetworkPtr net) {
struct network_driver *driver = net->conn->networkPrivateData;
virNetworkObjPtr network;
- virNetworkIpDefPtr ipv4def;
+ virNetworkIpDefPtr ipdef;
+ int v4present = 0, v6present = 0;
s/int/bool/
@@ -1780,12 +1871,17 @@ static int networkUndefine(virNetworkPtr net)
{
/* we only support dhcp on one IPv4 address per defined network */
for (ii = 0;
- (ipv4def = virNetworkDefGetIpByIndex(network->def, AF_INET, ii));
+ (ipdef = virNetworkDefGetIpByIndex(network->def, AF_UNSPEC, ii));
ii++) {
- if (ipv4def->nranges || ipv4def->nhosts)
- break;
+ if (VIR_SOCKET_IS_FAMILY(&ipdef->address, AF_INET)) {
+ if (ipdef->nranges || ipdef->nhosts)
+ v4present = 1;
At first glance, this logic didn't sound right. You only set v4present
if you found a dhcp interface, ignoring other ipv4 interfaces. Then
again,...
+ } else if (VIR_SOCKET_IS_FAMILY(&ipdef->address,
AF_INET6)) {
+ v6present = 1;
+ }
}
- if (ipv4def) {
+
+ if (v4present) {
dnsmasqContext *dctx = dnsmasqContextNew(network->def->name,
DNSMASQ_STATE_DIR);
you only use it to disable dnsmasq rather than all things related to
IPv4, so maybe it would be better to rename the variable to dhcp_present
instead of v4present.
ACK with those nits addressed.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
1:13 p.m.
New subject: [libvirt] [PATCHv2 12/13] Turn on IPv6 support in the bridge_driver.c virtual network driver
On 12/23/2010 12:41 PM, Eric Blake wrote:
On 12/22/2010 11:58 AM, Laine Stump wrote:
> At this point everything is already in place to make IPv6 happen, we just
> need to add a few rules, remove some checks for IPv4-only, and document
> the changes to the XML on the website.
> ---
> No changes from V1.
>
> docs/formatnetwork.html.in | 35 +++++++--
Yeah - a patch series with documentation updates!
Heh. Truthfully I was scared you'd NAK it if I didn't update the docs :-P
> static int
> networkAddGeneralIptablesRules(struct network_driver *driver,
> virNetworkObjPtr network)
> @@ -926,6 +985,11 @@ networkAddGeneralIptablesRules(struct network_driver *driver,
> goto err8;
> }
>
> + /* add IPv6 general rules, if needed */
> + if (networkAddGeneralIp6tablesRules(driver, network)< 0) {
> + goto err8;
Should this be err9, with a step that undoes the previous action when
you get past the err8 failure point?
I had somehow convinced myself not (because
networkAddGeneralIp6tablesRules() undoes itself if it fails), but of
course that logic is wrong - it's the *previous* step that needs
undoing, so you are absolutely correct. I've squashed in the appropriate
call.
> + if (virAsprintf(&field, SYSCTL_PATH
"/net/ipv6/conf/%s/disable_ipv6",
> + network->def->bridge)< 0) {
...
> + if (virFileWriteStr(field, "1", 0)< 0) {
> + virReportSystemError(errno,
> + _("cannot enable %s"), field);
Misleading message; maybe "cannot write to %s to disable IPv6".
Yup.
> @@ -1755,7 +1845,8 @@ cleanup:
> static int networkUndefine(virNetworkPtr net) {
> struct network_driver *driver = net->conn->networkPrivateData;
> virNetworkObjPtr network;
> - virNetworkIpDefPtr ipv4def;
> + virNetworkIpDefPtr ipdef;
> + int v4present = 0, v6present = 0;
s/int/bool/
Okay.
> @@ -1780,12 +1871,17 @@ static int networkUndefine(virNetworkPtr
net) {
>
> /* we only support dhcp on one IPv4 address per defined network */
> for (ii = 0;
> - (ipv4def = virNetworkDefGetIpByIndex(network->def, AF_INET, ii));
> + (ipdef = virNetworkDefGetIpByIndex(network->def, AF_UNSPEC, ii));
> ii++) {
> - if (ipv4def->nranges || ipv4def->nhosts)
> - break;
> + if (VIR_SOCKET_IS_FAMILY(&ipdef->address, AF_INET)) {
> + if (ipdef->nranges || ipdef->nhosts)
> + v4present = 1;
At first glance, this logic didn't sound right. You only set v4present
if you found a dhcp interface, ignoring other ipv4 interfaces. Then
again,...
> + } else if (VIR_SOCKET_IS_FAMILY(&ipdef->address, AF_INET6)) {
> + v6present = 1;
> + }
> }
> - if (ipv4def) {
> +
> + if (v4present) {
> dnsmasqContext *dctx = dnsmasqContextNew(network->def->name,
DNSMASQ_STATE_DIR);
you only use it to disable dnsmasq rather than all things related to
IPv4, so maybe it would be better to rename the variable to dhcp_present
instead of v4present.
Sure. I almost did that originally, and can't say why I didn't (I guess
my left brain liked the symmetry of the names or something).
ACK with those nits addressed.
12:58 p.m.
New subject: [libvirt] [PATCHv2 13/13] Run radvd for virtual networks with IPv6 addresses
Running an instance of the router advertisement daemon (radvd) allows
guests using the virtual network to automatically acquire an IPv6
address and default route. Note that acquiring an address only works
for networks with a prefix length of exactly 64 - radvd is still run
in other circumstances, and still advertises routes, but autoconf will
not work because it requires exactly 64 bits of address info from the
network prefix.
This patch avoids a race condition with the pidfile by manually
daemonizing radvd rather than allowing it to daemonize itself, then
creating our own pidfile (in addition to radvd's own file, which is
unnecessary). This is accomplished by exec'ing it with "--debug 1" in
the commandline, and using virCommand's features to fork, create a
pidfile, and detach from the newly forked process.
---
Changes in V2:
* prevent radvd from daemonizing itself (by adding "--debug 1" to the args),
and do it manually with virCommandDaemonize().
* tell radvd to write a pidfile called "<network>-radvd.pid-bin", which
we
will ignore.
* have virCommand create a pidfile for us, since we can be guaranteed there
will be no race during its creation.
* minor change to he virAsprintf that constructs the configfile name.
* stop the configfile string leak that Eric pointed out.
configure.ac | 4 +
src/conf/network_conf.h | 1 +
src/network/bridge_driver.c | 246 +++++++++++++++++++++++++++++++++++++++----
3 files changed, 230 insertions(+), 21 deletions(-)
diff --git a/configure.ac b/configure.ac
index 76652f4..3e077a8 100644
--- a/configure.ac
+++ b/configure.ac
@@ -135,6 +135,8 @@ dnl detect them, in which case we'll search for the program
dnl along the $PATH at runtime and fail if it's not there.
AC_PATH_PROG([DNSMASQ], [dnsmasq], [dnsmasq],
[/sbin:/usr/sbin:/usr/local/sbin:$PATH])
+AC_PATH_PROG([RADVD], [radvd], [radvd],
+ [/sbin:/usr/sbin:/usr/local/sbin:$PATH])
AC_PATH_PROG([BRCTL], [brctl], [brctl],
[/sbin:/usr/sbin:/usr/local/sbin:$PATH])
AC_PATH_PROG([UDEVADM], [udevadm], [],
@@ -146,6 +148,8 @@ AC_PATH_PROG([MODPROBE], [modprobe], [],
AC_DEFINE_UNQUOTED([DNSMASQ],["$DNSMASQ"],
[Location or name of the dnsmasq program])
+AC_DEFINE_UNQUOTED([RADVD],["$RADVD"],
+ [Location or name of the radvd program])
AC_DEFINE_UNQUOTED([BRCTL],["$BRCTL"],
[Location or name of the brctl program (see bridge-utils)])
if test -n "$UDEVADM"; then
diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
index a51794d..5ad6136 100644
--- a/src/conf/network_conf.h
+++ b/src/conf/network_conf.h
@@ -107,6 +107,7 @@ struct _virNetworkObj {
virMutex lock;
pid_t dnsmasqPid;
+ pid_t radvdPid;
unsigned int active : 1;
unsigned int autostart : 1;
unsigned int persistent : 1;
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index d3adc32..589a962 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -65,6 +65,7 @@
#define NETWORK_STATE_DIR LOCALSTATEDIR "/lib/libvirt/network"
#define DNSMASQ_STATE_DIR LOCALSTATEDIR "/lib/libvirt/dnsmasq"
+#define RADVD_STATE_DIR LOCALSTATEDIR "/lib/libvirt/radvd"
#define VIR_FROM_THIS VIR_FROM_NETWORK
@@ -107,6 +108,25 @@ static void networkReloadIptablesRules(struct network_driver
*driver);
static struct network_driver *driverState = NULL;
+static char *
+networkRadvdPidfileBasename(const char *netname)
+{
+ /* this is simple but we want to be sure it's consistently done */
+ char *pidfilebase;
+
+ virAsprintf(&pidfilebase, "%s-radvd", netname);
+ return pidfilebase;
+}
+
+static char *
+networkRadvdConfigFileName(const char *netname)
+{
+ char *configfile;
+
+ virAsprintf(&configfile, RADVD_STATE_DIR "/%s-radvd.conf",
+ netname);
+ return configfile;
+}
static void
networkFindActiveConfigs(struct network_driver *driver) {
@@ -144,26 +164,43 @@ networkFindActiveConfigs(struct network_driver *driver) {
brHasBridge(driver->brctl, obj->def->bridge) == 0) {
obj->active = 1;
- /* Finally try and read dnsmasq pid if any */
- if (obj->def->ips && (obj->def->nips > 0) &&
- virFileReadPid(NETWORK_PID_DIR, obj->def->name,
- &obj->dnsmasqPid) == 0) {
-
- /* Check its still alive */
- if (kill(obj->dnsmasqPid, 0) != 0)
- obj->dnsmasqPid = -1;
-
-#ifdef __linux__
- char *pidpath;
+ /* Try and read dnsmasq/radvd pids if any */
+ if (obj->def->ips && (obj->def->nips > 0)) {
+ char *pidpath, *radvdpidbase;
+
+ if (virFileReadPid(NETWORK_PID_DIR, obj->def->name,
+ &obj->dnsmasqPid) == 0) {
+ /* Check that it's still alive */
+ if (kill(obj->dnsmasqPid, 0) != 0)
+ obj->dnsmasqPid = -1;
+ if (virAsprintf(&pidpath, "/proc/%d/exe",
obj->dnsmasqPid) < 0) {
+ virReportOOMError();
+ goto cleanup;
+ }
+ if (virFileLinkPointsTo(pidpath, DNSMASQ) == 0)
+ obj->dnsmasqPid = -1;
+ VIR_FREE(pidpath);
+ }
- if (virAsprintf(&pidpath, "/proc/%d/exe",
obj->dnsmasqPid) < 0) {
+ if (!(radvdpidbase = networkRadvdPidfileBasename(obj->def->name)))
{
virReportOOMError();
goto cleanup;
}
- if (virFileLinkPointsTo(pidpath, DNSMASQ) == 0)
- obj->dnsmasqPid = -1;
- VIR_FREE(pidpath);
-#endif
+ if (virFileReadPid(NETWORK_PID_DIR, radvdpidbase,
+ &obj->radvdPid) == 0) {
+ /* Check that it's still alive */
+ if (kill(obj->radvdPid, 0) != 0)
+ obj->radvdPid = -1;
+ if (virAsprintf(&pidpath, "/proc/%d/exe",
obj->radvdPid) < 0) {
+ virReportOOMError();
+ VIR_FREE(radvdpidbase);
+ goto cleanup;
+ }
+ if (virFileLinkPointsTo(pidpath, RADVD) == 0)
+ obj->radvdPid = -1;
+ VIR_FREE(pidpath);
+ }
+ VIR_FREE(radvdpidbase);
}
}
@@ -587,6 +624,136 @@ cleanup:
}
static int
+networkStartRadvd(virNetworkObjPtr network)
+{
+ char *pidfile = NULL;
+ char *radvdpidbase = NULL;
+ virBuffer configbuf = VIR_BUFFER_INITIALIZER;;
+ char *configstr = NULL;
+ char *configfile = NULL;
+ virCommandPtr cmd = NULL;
+ int ret = -1, err, ii;
+ virNetworkIpDefPtr ipdef;
+
+ network->radvdPid = -1;
+
+ if ((err = virFileMakePath(NETWORK_PID_DIR)) != 0) {
+ virReportSystemError(err,
+ _("cannot create directory %s"),
+ NETWORK_PID_DIR);
+ goto cleanup;
+ }
+ if ((err = virFileMakePath(RADVD_STATE_DIR)) != 0) {
+ virReportSystemError(err,
+ _("cannot create directory %s"),
+ RADVD_STATE_DIR);
+ goto cleanup;
+ }
+
+ /* construct pidfile name */
+ if (!(radvdpidbase = networkRadvdPidfileBasename(network->def->name))) {
+ virReportOOMError();
+ goto cleanup;
+ }
+ if (!(pidfile = virFilePid(NETWORK_PID_DIR, radvdpidbase))) {
+ virReportOOMError();
+ goto cleanup;
+ }
+
+ /* create radvd config file appropriate for this network */
+ virBufferVSprintf(&configbuf, "interface %s\n"
+ "{\n"
+ " AdvSendAdvert on;\n"
+ " AdvManagedFlag off;\n"
+ " AdvOtherConfigFlag off;\n"
+ "\n",
+ network->def->bridge);
+ for (ii = 0;
+ (ipdef = virNetworkDefGetIpByIndex(network->def, AF_INET6, ii));
+ ii++) {
+ int prefix;
+ char *netaddr;
+
+ prefix = virNetworkIpDefPrefix(ipdef);
+ if (prefix < 0) {
+ networkReportError(VIR_ERR_INTERNAL_ERROR,
+ _("bridge '%s' has an invalid
prefix"),
+ network->def->bridge);
+ goto cleanup;
+ }
+ if (!(netaddr = virSocketFormatAddr(&ipdef->address)))
+ goto cleanup;
+ virBufferVSprintf(&configbuf,
+ " prefix %s/%d\n"
+ " {\n"
+ " AdvOnLink on;\n"
+ " AdvAutonomous on;\n"
+ " AdvRouterAddr off;\n"
+ " };\n",
+ netaddr, prefix);
+ VIR_FREE(netaddr);
+ }
+
+ virBufferAddLit(&configbuf, "};\n");
+
+ if (virBufferError(&configbuf)) {
+ virReportOOMError();
+ goto cleanup;
+ }
+ if (!(configstr = virBufferContentAndReset(&configbuf))) {
+ virReportOOMError();
+ goto cleanup;
+ }
+
+ /* construct the filename */
+ if (!(configfile = networkRadvdConfigFileName(network->def->name))) {
+ virReportOOMError();
+ goto cleanup;
+ }
+ /* write the file */
+ if (virFileWriteStr(configfile, configstr, 0600) < 0) {
+ virReportSystemError(errno,
+ _("couldn't write radvd config file
'%s'"),
+ configfile);
+ goto cleanup;
+ }
+
+ /* prevent radvd from daemonizing itself with "--debug 1", and use
+ * a dummy pidfile name - virCommand will create the pidfile we
+ * want to use (this is necessary because radvd's internal
+ * daemonization and pidfile creation causes a race, and the
+ * virFileReadPid() below will fail if we use them).
+ * Unfortunately, it isn't possible to tell radvd to not create
+ * its own pidfile, so we just let it do so, with a slightly
+ * different name. Unused, but harmless.
+ */
+ cmd = virCommandNewArgList(RADVD, "--debug", "1",
+ "--config", configfile,
+ "--pidfile", NULL);
+ virCommandAddArgFormat(cmd, "%s-bin", pidfile);
+
+ virCommandSetPidFile(cmd, pidfile);
+ virCommandDaemonize(cmd);
+
+ if (virCommandRun(cmd, NULL) < 0)
+ goto cleanup;
+
+ if (virFileReadPid(NETWORK_PID_DIR, radvdpidbase,
+ &network->radvdPid) < 0)
+ goto cleanup;
+
+ ret = 0;
+cleanup:
+ virCommandFree(cmd);
+ VIR_FREE(configfile);
+ VIR_FREE(configstr);
+ virBufferFreeAndReset(&configbuf);
+ VIR_FREE(radvdpidbase);
+ VIR_FREE(pidfile);
+ return ret;
+}
+
+static int
networkAddMasqueradingIptablesRules(struct network_driver *driver,
virNetworkObjPtr network,
virNetworkIpDefPtr ipdef)
@@ -1153,9 +1320,14 @@ networkReloadIptablesRules(struct network_driver *driver)
/* Enable IP Forwarding. Return 0 for success, -1 for failure. */
static int
-networkEnableIpForwarding(void)
+networkEnableIpForwarding(int enableIPv4, int enableIPv6)
{
- return virFileWriteStr("/proc/sys/net/ipv4/ip_forward", "1\n",
0);
+ int ret = 0;
+ if (enableIPv4)
+ ret = virFileWriteStr("/proc/sys/net/ipv4/ip_forward", "1\n",
0);
+ if (enableIPv6 && ret == 0)
+ ret = virFileWriteStr("/proc/sys/net/ipv6/conf/all/forwarding",
"1\n", 0);
+ return ret;
}
#define SYSCTL_PATH "/proc/sys"
@@ -1430,7 +1602,7 @@ networkStartNetworkDaemon(struct network_driver *driver,
/* If forwardType != NONE, turn on global IP forwarding */
if (network->def->forwardType != VIR_NETWORK_FORWARD_NONE &&
- networkEnableIpForwarding() < 0) {
+ networkEnableIpForwarding(v4present, v6present) < 0) {
virReportSystemError(errno, "%s",
_("failed to enable IP forwarding"));
goto err3;
@@ -1441,15 +1613,28 @@ networkStartNetworkDaemon(struct network_driver *driver,
if (v4present && networkStartDhcpDaemon(network) < 0)
goto err3;
+ /* start radvd if there are any ipv6 addresses */
+ if (v6present && networkStartRadvd(network) < 0)
+ goto err4;
+
/* Persist the live configuration now we have bridge info */
if (virNetworkSaveConfig(NETWORK_STATE_DIR, network->def) < 0) {
- goto err4;
+ goto err5;
}
network->active = 1;
return 0;
+ err5:
+ if (!save_err)
+ save_err = virSaveLastError();
+
+ if (network->radvdPid > 0) {
+ kill(network->radvdPid, SIGTERM);
+ network->radvdPid = -1;
+ }
+
err4:
if (!save_err)
save_err = virSaveLastError();
@@ -1508,6 +1693,9 @@ static int networkShutdownNetworkDaemon(struct network_driver
*driver,
unlink(stateFile);
VIR_FREE(stateFile);
+ if (network->radvdPid > 0)
+ kill(network->radvdPid, SIGTERM);
+
if (network->dnsmasqPid > 0)
kill(network->dnsmasqPid, SIGTERM);
@@ -1528,8 +1716,13 @@ static int networkShutdownNetworkDaemon(struct network_driver
*driver,
if (network->dnsmasqPid > 0 &&
(kill(network->dnsmasqPid, 0) == 0))
kill(network->dnsmasqPid, SIGKILL);
-
network->dnsmasqPid = -1;
+
+ if (network->radvdPid > 0 &&
+ (kill(network->radvdPid, 0) == 0))
+ kill(network->radvdPid, SIGKILL);
+ network->radvdPid = -1;
+
network->active = 0;
if (network->newDef) {
@@ -1890,6 +2083,17 @@ static int networkUndefine(virNetworkPtr net) {
dnsmasqContextFree(dctx);
}
+ if (v6present) {
+ char *configfile = networkRadvdConfigFileName(network->def->name);
+
+ if (!configfile) {
+ virReportOOMError();
+ goto cleanup;
+ }
+ unlink(configfile);
+ VIR_FREE(configfile);
+ }
+
virNetworkRemoveInactive(&driver->networks,
network);
network = NULL;
--
1.7.3.4
11:54 a.m.
New subject: [libvirt] [PATCHv2 13/13] Run radvd for virtual networks with IPv6 addresses
On 12/22/2010 11:58 AM, Laine Stump wrote:
Running an instance of the router advertisement daemon (radvd)
allows
guests using the virtual network to automatically acquire an IPv6
address and default route. Note that acquiring an address only works
for networks with a prefix length of exactly 64 - radvd is still run
in other circumstances, and still advertises routes, but autoconf will
not work because it requires exactly 64 bits of address info from the
network prefix.
This patch avoids a race condition with the pidfile by manually
daemonizing radvd rather than allowing it to daemonize itself, then
creating our own pidfile (in addition to radvd's own file, which is
unnecessary). This is accomplished by exec'ing it with "--debug 1" in
the commandline, and using virCommand's features to fork, create a
pidfile, and detach from the newly forked process.
As a reminder, have you filed a bug report against radvd about their
anti-social daemonizing behavior?
+ cmd = virCommandNewArgList(RADVD, "--debug",
"1",
+ "--config", configfile,
+ "--pidfile", NULL);
+ virCommandAddArgFormat(cmd, "%s-bin", pidfile);
+
+ virCommandSetPidFile(cmd, pidfile);
+ virCommandDaemonize(cmd);
+
+ if (virCommandRun(cmd, NULL) < 0)
+ goto cleanup;
+
+ if (virFileReadPid(NETWORK_PID_DIR, radvdpidbase,
+ &network->radvdPid) < 0)
+ goto cleanup;
+
+ ret = 0;
+cleanup:
+ virCommandFree(cmd);
+ VIR_FREE(configfile);
+ VIR_FREE(configstr);
+ virBufferFreeAndReset(&configbuf);
+ VIR_FREE(radvdpidbase);
+ VIR_FREE(pidfile);
Should we also unlink() pidfile or pidfile-bin at any point?
static int
-networkEnableIpForwarding(void)
+networkEnableIpForwarding(int enableIPv4, int enableIPv6)
s/int/bool/
ACK with those nits addressed.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
1:30 p.m.
New subject: [libvirt] [PATCHv2 13/13] Run radvd for virtual networks with IPv6 addresses
On 12/23/2010 12:54 PM, Eric Blake wrote:
On 12/22/2010 11:58 AM, Laine Stump wrote:
> Running an instance of the router advertisement daemon (radvd) allows
> guests using the virtual network to automatically acquire an IPv6
> address and default route. Note that acquiring an address only works
> for networks with a prefix length of exactly 64 - radvd is still run
> in other circumstances, and still advertises routes, but autoconf will
> not work because it requires exactly 64 bits of address info from the
> network prefix.
>
> This patch avoids a race condition with the pidfile by manually
> daemonizing radvd rather than allowing it to daemonize itself, then
> creating our own pidfile (in addition to radvd's own file, which is
> unnecessary). This is accomplished by exec'ing it with "--debug 1" in
> the commandline, and using virCommand's features to fork, create a
> pidfile, and detach from the newly forked process.
As a reminder, have you filed a bug report against radvd about their
anti-social daemonizing behavior?
Yes, I filed two. One about the race, and one about the inability to
specify "no pidfile". The "no pidfile" one has already been closed as
NOTABUG. The one about the race is still NEW.
https://bugzilla.redhat.com/show_bug.cgi?id=664791
https://bugzilla.redhat.com/show_bug.cgi?id=664783
(I filed them against RHEL6, because I'm not sure where upstream is, but
am sure that the RHEL maintainer does).
> + cmd = virCommandNewArgList(RADVD, "--debug",
"1",
> + "--config", configfile,
> + "--pidfile", NULL);
> + virCommandAddArgFormat(cmd, "%s-bin", pidfile);
> +
> + virCommandSetPidFile(cmd, pidfile);
> + virCommandDaemonize(cmd);
> +
> + if (virCommandRun(cmd, NULL)< 0)
> + goto cleanup;
> +
> + if (virFileReadPid(NETWORK_PID_DIR, radvdpidbase,
> +&network->radvdPid)< 0)
> + goto cleanup;
> +
> + ret = 0;
> +cleanup:
> + virCommandFree(cmd);
> + VIR_FREE(configfile);
> + VIR_FREE(configstr);
> + virBufferFreeAndReset(&configbuf);
> + VIR_FREE(radvdpidbase);
> + VIR_FREE(pidfile);
Should we also unlink() pidfile or pidfile-bin at any point?
we need to unlink pidfile, but radvd seems to guarantee that pidfile-bin
is unlinked when it exits.
I guess we should do it both when we kill radvd, and also when we
undefine the network, since there may have been a situation that killed
radvd without libvirt's knowledge, in which case an extra will be
hanging around.
I'll squash that in as well.
> static int
> -networkEnableIpForwarding(void)
> +networkEnableIpForwarding(int enableIPv4, int enableIPv6)
s/int/bool/
Okay.
ACK with those nits addressed.
3:16 p.m.
I squashed in all of Eric's recommendations and pushed this entire series.
Thanks for the great reviews Eric!
On 12/22/2010 01:57 PM, Laine Stump wrote:
This is a resend of
https://www.redhat.com/archives/libvir-list/2010-December/msg00765.html
incorporating changes due to comments from Eric Blake and Paweł Krześniak.
changes from v1 to v2 are noted in each individual mail
-----
Most of this patchset is setup for patch 09/13, which updates the
network XML parser to support IPv6, and 12/13, which turns on IPv6 in
the bridge driver.
In order to have each patch individually pass make check and
(otherwise function properly in case someone is doing a bisect), there
is a bit of extra code churn (lines that are changed in one patch,
only to be changed again in a later patch); I tried to minimize this
as much as possible.
For for IPv6 to work correctly, target *and build* systems will
now need to have ip6tables and radvd available. The way I added
ip6tables into autoconfigure.ac is identical to existing iptables, and
the way I added radvd is identical to dnsmasq. Unfortunately this
doesn't communicate the requirement downstream in a programmatic
fashion, so I guess we need to make sure that this is adequately
reported in the next set of release notes.
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
9:05 a.m.
On 12/23/2010 04:16 PM, Laine Stump wrote:
I squashed in all of Eric's recommendations and pushed this
entire
series.
The networkApplyTest.sh of the libvirt tck test suite does not pass
anymore. I assume it's due to the changes in this part of the code. I
see the following change on the broadcast address of the interface:
sh ./networkApplyTest.sh --wait --verbose --force
FAIL networkxml2xmlin/tck-testnet-2.xml : ifconfig tck-testbr | grep
':10\.1\.2\.'
1c1
< inet addr:10.1.2.1 Bcast:0.0.0.0 Mask:255.255.255.0
---
inet addr:10.1.2.1 Bcast:10.1.2.255 Mask:255.255.255.0
The Bcast address is now set to 0.0.0.0 and is expected to be 10.1.2.255.
Stefan
Thanks for the great reviews Eric!
On 12/22/2010 01:57 PM, Laine Stump wrote:
> This is a resend of
>
>
> https://www.redhat.com/archives/libvir-list/2010-December/msg00765.html
>
> incorporating changes due to comments from Eric Blake and Paweł
> Krześniak.
>
> changes from v1 to v2 are noted in each individual mail
>
> -----
> Most of this patchset is setup for patch 09/13, which updates the
> network XML parser to support IPv6, and 12/13, which turns on IPv6 in
> the bridge driver.
>
> In order to have each patch individually pass make check and
> (otherwise function properly in case someone is doing a bisect), there
> is a bit of extra code churn (lines that are changed in one patch,
> only to be changed again in a later patch); I tried to minimize this
> as much as possible.
>
> For for IPv6 to work correctly, target *and build* systems will
> now need to have ip6tables and radvd available. The way I added
> ip6tables into autoconfigure.ac is identical to existing iptables, and
> the way I added radvd is identical to dnsmasq. Unfortunately this
> doesn't communicate the requirement downstream in a programmatic
> fashion, so I guess we need to make sure that this is adequately
> reported in the next set of release notes.
>
> --
> libvir-list mailing list
> libvir-list(a)redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
9:04 a.m.
On 12/29/2010 10:05 AM, Stefan Berger wrote:
On 12/23/2010 04:16 PM, Laine Stump wrote:
> I squashed in all of Eric's recommendations and pushed this entire
> series.
The networkApplyTest.sh of the libvirt tck test suite does not pass
anymore. I assume it's due to the changes in this part of the code. I
see the following change on the broadcast address of the interface:
sh ./networkApplyTest.sh --wait --verbose --force
FAIL networkxml2xmlin/tck-testnet-2.xml : ifconfig tck-testbr | grep
':10\.1\.2\.'
1c1
< inet addr:10.1.2.1 Bcast:0.0.0.0 Mask:255.255.255.0
---
> inet addr:10.1.2.1 Bcast:10.1.2.255 Mask:255.255.255.0
The Bcast address is now set to 0.0.0.0 and is expected to be 10.1.2.255.
Thanks for catching this, Stefan! Apparently "ip addr add" doesn't set
the broadcast address unless it's specified, where the old method of
using ioctls must have set it implicitly when the netmask was set.
I'll post a patch for this later today.
5:35 a.m.
On 12/30/2010 10:04 AM, Laine Stump wrote:
On 12/29/2010 10:05 AM, Stefan Berger wrote:
> On 12/23/2010 04:16 PM, Laine Stump wrote:
>> I squashed in all of Eric's recommendations and pushed this entire
>> series.
>
> The networkApplyTest.sh of the libvirt tck test suite does not pass
> anymore. I assume it's due to the changes in this part of the code. I
> see the following change on the broadcast address of the interface:
>
> sh ./networkApplyTest.sh --wait --verbose --force
>
> FAIL networkxml2xmlin/tck-testnet-2.xml : ifconfig tck-testbr | grep
> ':10\.1\.2\.'
> 1c1
> < inet addr:10.1.2.1 Bcast:0.0.0.0 Mask:255.255.255.0
> ---
> > inet addr:10.1.2.1 Bcast:10.1.2.255 Mask:255.255.255.0
>
> The Bcast address is now set to 0.0.0.0 and is expected to be
> 10.1.2.255.
Thanks for catching this, Stefan! Apparently "ip addr add" doesn't set
the broadcast address unless it's specified, where the old method of
using ioctls must have set it implicitly when the netmask was set.
I'll post a patch for this later today.
The fix is included in this series:
https://www.redhat.com/archives/libvir-list/2010-December/msg01075.html
7:08 a.m.
On 12/31/2010 06:35 AM, Laine Stump wrote:
On 12/30/2010 10:04 AM, Laine Stump wrote:
> On 12/29/2010 10:05 AM, Stefan Berger wrote:
>> On 12/23/2010 04:16 PM, Laine Stump wrote:
>>> I squashed in all of Eric's recommendations and pushed this entire
>>> series.
>>
>> The networkApplyTest.sh of the libvirt tck test suite does not pass
>> anymore. I assume it's due to the changes in this part of the code.
>> I see the following change on the broadcast address of the interface:
>>
>> sh ./networkApplyTest.sh --wait --verbose --force
>>
>> FAIL networkxml2xmlin/tck-testnet-2.xml : ifconfig tck-testbr | grep
>> ':10\.1\.2\.'
>> 1c1
>> < inet addr:10.1.2.1 Bcast:0.0.0.0 Mask:255.255.255.0
>> ---
>> > inet addr:10.1.2.1 Bcast:10.1.2.255 Mask:255.255.255.0
>>
>> The Bcast address is now set to 0.0.0.0 and is expected to be
>> 10.1.2.255.
>
>
> Thanks for catching this, Stefan! Apparently "ip addr add" doesn't
> set the broadcast address unless it's specified, where the old method
> of using ioctls must have set it implicitly when the netmask was set.
>
> I'll post a patch for this later today.
The fix is included in this series:
https://www.redhat.com/archives/libvir-list/2010-December/msg01075.html
I applied
them locally and ran the tests successfully.
Stefan
5074
days inactive
5083
days old
39 comments
4 participants
participants (4)
-
Eric Blake -
Laine Stump -
Paweł Krześniak -
Stefan Berger