11:25 a.m.
This is a second step for making bridge_driver more portable.
This one splits up bridge driver into platform specific and general
parts. Platform specifc parts are mostly firewalling stuff.
So the support for new platforms should be added by implementing
bridge_driver_$platform.c
However, you might notice I didn't move some ports of the
platform specific code. E.g. I left networkEnableIpForwarding
in bridge_driver.c because it feels like the implementation for
e.g. all BSDs would be the same (while firewalling stuff would
be different on BSDs) to avoid code copy/paste.
Roman Bogorodskiy (1):
Split up platfrom specifics from bridge driver
po/POTFILES.in | 1 +
src/Makefile.am | 5 +-
src/network/bridge_driver.c | 729 +---------------------------------
src/network/bridge_driver_linux.c | 709 +++++++++++++++++++++++++++++++++
src/network/bridge_driver_noop.c | 80 ++++
src/network/bridge_driver_platform.c | 32 ++
src/network/bridge_driver_platform.h | 77 ++++
7 files changed, 915 insertions(+), 718 deletions(-)
create mode 100644 src/network/bridge_driver_linux.c
create mode 100644 src/network/bridge_driver_noop.c
create mode 100644 src/network/bridge_driver_platform.c
create mode 100644 src/network/bridge_driver_platform.h
--
1.7.9.5
11:25 a.m.
* Functions were renamed: s/Iptables/Firewall/
to make names more general.
* Platform specific things (e.g. firewalling and route
collision checks) were moved into bridge_driver_platform
* Created two platform specific implementations:
- bridge_driver_linux: Linux implementation using iptables,
it's actually the code moved from bridge_driver.c
- bridge_driver_noop: dump implementation that does nothing
---
po/POTFILES.in | 1 +
src/Makefile.am | 5 +-
src/network/bridge_driver.c | 729 +---------------------------------
src/network/bridge_driver_linux.c | 709 +++++++++++++++++++++++++++++++++
src/network/bridge_driver_noop.c | 80 ++++
src/network/bridge_driver_platform.c | 32 ++
src/network/bridge_driver_platform.h | 77 ++++
7 files changed, 915 insertions(+), 718 deletions(-)
create mode 100644 src/network/bridge_driver_linux.c
create mode 100644 src/network/bridge_driver_noop.c
create mode 100644 src/network/bridge_driver_platform.c
create mode 100644 src/network/bridge_driver_platform.h
diff --git a/po/POTFILES.in b/po/POTFILES.in
index af7fd7f..b833110 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -69,6 +69,7 @@ src/lxc/lxc_process.c
src/libxl/libxl_driver.c
src/libxl/libxl_conf.c
src/network/bridge_driver.c
+src/network/bridge_driver_linux.c
src/node_device/node_device_driver.c
src/node_device/node_device_hal.c
src/node_device/node_device_linux_sysfs.c
diff --git a/src/Makefile.am b/src/Makefile.am
index d9e703f..a8b47ed 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -722,8 +722,9 @@ PARALLELS_DRIVER_SOURCES = \
parallels/parallels_storage.c \
parallels/parallels_network.c
-NETWORK_DRIVER_SOURCES = \
- network/bridge_driver.h network/bridge_driver.c
+NETWORK_DRIVER_SOURCES = \
+ network/bridge_driver.h network/bridge_driver.c \
+ network/bridge_driver_platform.h network/bridge_driver_platform.c
INTERFACE_DRIVER_SOURCES =
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index d7e90ac..0b843b6 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -45,6 +45,7 @@
#include "virerror.h"
#include "datatypes.h"
#include "bridge_driver.h"
+#include "bridge_driver_platform.h"
#include "network_conf.h"
#include "device_conf.h"
#include "driver.h"
@@ -69,22 +70,6 @@
#define VIR_FROM_THIS VIR_FROM_NETWORK
-/* Main driver state */
-struct network_driver {
- virMutex lock;
-
- virNetworkObjList networks;
-
- char *networkConfigDir;
- char *networkAutostartDir;
- char *stateDir;
- char *pidDir;
- char *dnsmasqStateDir;
- char *radvdStateDir;
- dnsmasqCapsPtr dnsmasqCaps;
-};
-
-
static void networkDriverLock(struct network_driver *driver)
{
virMutexLock(&driver->lock);
@@ -114,7 +99,7 @@ static int networkStartNetworkExternal(struct network_driver *driver,
static int networkShutdownNetworkExternal(struct network_driver *driver,
virNetworkObjPtr network);
-static void networkReloadIptablesRules(struct network_driver *driver);
+static void networkReloadFirewallRules(struct network_driver *driver);
static void networkRefreshDaemons(struct network_driver *driver);
static int networkPlugBandwidth(virNetworkObjPtr net,
@@ -341,7 +326,7 @@ firewalld_dbus_filter_bridge(DBusConnection *connection
ATTRIBUTE_UNUSED,
"Reloaded"))
{
VIR_DEBUG("Reload in bridge_driver because of firewalld.");
- networkReloadIptablesRules(_driverState);
+ networkReloadFirewallRules(_driverState);
}
return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
@@ -432,7 +417,7 @@ networkStateInitialize(bool privileged,
goto error;
networkFindActiveConfigs(driverState);
- networkReloadIptablesRules(driverState);
+ networkReloadFirewallRules(driverState);
networkRefreshDaemons(driverState);
networkAutostartConfigs(driverState);
@@ -496,7 +481,7 @@ networkStateReload(void) {
virNetworkLoadAllConfigs(&driverState->networks,
driverState->networkConfigDir,
driverState->networkAutostartDir);
- networkReloadIptablesRules(driverState);
+ networkReloadFirewallRules(driverState);
networkRefreshDaemons(driverState);
networkAutostartConfigs(driverState);
networkDriverUnlock(driverState);
@@ -1535,599 +1520,8 @@ networkRefreshDaemons(struct network_driver *driver)
}
}
-static int
-networkAddMasqueradingIptablesRules(virNetworkObjPtr network,
- virNetworkIpDefPtr ipdef)
-{
- int prefix = virNetworkIpDefPrefix(ipdef);
- const char *forwardIf = virNetworkDefForwardIf(network->def, 0);
-
- if (prefix < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Invalid prefix or netmask for '%s'"),
- network->def->bridge);
- goto masqerr1;
- }
-
- /* allow forwarding packets from the bridge interface */
- if (iptablesAddForwardAllowOut(&ipdef->address,
- prefix,
- network->def->bridge,
- forwardIf) < 0) {
- virReportError(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 (iptablesAddForwardAllowRelatedIn(&ipdef->address,
- prefix,
- network->def->bridge,
- forwardIf) < 0) {
- virReportError(VIR_ERR_SYSTEM_ERROR,
- _("failed to add iptables rule to allow forwarding to
'%s'"),
- network->def->bridge);
- goto masqerr2;
- }
-
- /*
- * Enable masquerading.
- *
- * We need to end up with 3 rules in the table in this order
- *
- * 1. protocol=tcp with sport mapping restriction
- * 2. protocol=udp with sport mapping restriction
- * 3. generic any protocol
- *
- * The sport mappings are required, because default IPtables
- * MASQUERADE maintain port numbers unchanged where possible.
- *
- * NFS can be configured to only "trust" port numbers < 1023.
- *
- * Guests using NAT thus need to be prevented from having port
- * numbers < 1023, otherwise they can bypass the NFS "security"
- * check on the source port number.
- *
- * Since we use '--insert' to add rules to the header of the
- * chain, we actually need to add them in the reverse of the
- * order just mentioned !
- */
-
- /* First the generic masquerade rule for other protocols */
- if (iptablesAddForwardMasquerade(&ipdef->address,
- prefix,
- forwardIf,
- &network->def->forward.addr,
- &network->def->forward.port,
- NULL) < 0) {
- if (forwardIf)
- virReportError(VIR_ERR_SYSTEM_ERROR,
- _("failed to add iptables rule to enable masquerading to
%s"),
- forwardIf);
- else
- virReportError(VIR_ERR_SYSTEM_ERROR, "%s",
- _("failed to add iptables rule to enable
masquerading"));
- goto masqerr3;
- }
-
- /* UDP with a source port restriction */
- if (iptablesAddForwardMasquerade(&ipdef->address,
- prefix,
- forwardIf,
- &network->def->forward.addr,
- &network->def->forward.port,
- "udp") < 0) {
- if (forwardIf)
- virReportError(VIR_ERR_SYSTEM_ERROR,
- _("failed to add iptables rule to enable UDP masquerading
to %s"),
- forwardIf);
- else
- virReportError(VIR_ERR_SYSTEM_ERROR, "%s",
- _("failed to add iptables rule to enable UDP
masquerading"));
- goto masqerr4;
- }
-
- /* TCP with a source port restriction */
- if (iptablesAddForwardMasquerade(&ipdef->address,
- prefix,
- forwardIf,
- &network->def->forward.addr,
- &network->def->forward.port,
- "tcp") < 0) {
- if (forwardIf)
- virReportError(VIR_ERR_SYSTEM_ERROR,
- _("failed to add iptables rule to enable TCP masquerading
to %s"),
- forwardIf);
- else
- virReportError(VIR_ERR_SYSTEM_ERROR, "%s",
- _("failed to add iptables rule to enable TCP
masquerading"));
- goto masqerr5;
- }
-
- return 0;
-
- masqerr5:
- iptablesRemoveForwardMasquerade(&ipdef->address,
- prefix,
- forwardIf,
- &network->def->forward.addr,
- &network->def->forward.port,
- "udp");
- masqerr4:
- iptablesRemoveForwardMasquerade(&ipdef->address,
- prefix,
- forwardIf,
- &network->def->forward.addr,
- &network->def->forward.port,
- NULL);
- masqerr3:
- iptablesRemoveForwardAllowRelatedIn(&ipdef->address,
- prefix,
- network->def->bridge,
- forwardIf);
- masqerr2:
- iptablesRemoveForwardAllowOut(&ipdef->address,
- prefix,
- network->def->bridge,
- forwardIf);
- masqerr1:
- return -1;
-}
-
static void
-networkRemoveMasqueradingIptablesRules(virNetworkObjPtr network,
- virNetworkIpDefPtr ipdef)
-{
- int prefix = virNetworkIpDefPrefix(ipdef);
- const char *forwardIf = virNetworkDefForwardIf(network->def, 0);
-
- if (prefix >= 0) {
- iptablesRemoveForwardMasquerade(&ipdef->address,
- prefix,
- forwardIf,
- &network->def->forward.addr,
- &network->def->forward.port,
- "tcp");
- iptablesRemoveForwardMasquerade(&ipdef->address,
- prefix,
- forwardIf,
- &network->def->forward.addr,
- &network->def->forward.port,
- "udp");
- iptablesRemoveForwardMasquerade(&ipdef->address,
- prefix,
- forwardIf,
- &network->def->forward.addr,
- &network->def->forward.port,
- NULL);
-
- iptablesRemoveForwardAllowRelatedIn(&ipdef->address,
- prefix,
- network->def->bridge,
- forwardIf);
- iptablesRemoveForwardAllowOut(&ipdef->address,
- prefix,
- network->def->bridge,
- forwardIf);
- }
-}
-
-static int
-networkAddRoutingIptablesRules(virNetworkObjPtr network,
- virNetworkIpDefPtr ipdef)
-{
- int prefix = virNetworkIpDefPrefix(ipdef);
- const char *forwardIf = virNetworkDefForwardIf(network->def, 0);
-
- if (prefix < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Invalid prefix or netmask for '%s'"),
- network->def->bridge);
- goto routeerr1;
- }
-
- /* allow routing packets from the bridge interface */
- if (iptablesAddForwardAllowOut(&ipdef->address,
- prefix,
- network->def->bridge,
- forwardIf) < 0) {
- virReportError(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 (iptablesAddForwardAllowIn(&ipdef->address,
- prefix,
- network->def->bridge,
- forwardIf) < 0) {
- virReportError(VIR_ERR_SYSTEM_ERROR,
- _("failed to add iptables rule to allow routing to
'%s'"),
- network->def->bridge);
- goto routeerr2;
- }
-
- return 0;
-
-routeerr2:
- iptablesRemoveForwardAllowOut(&ipdef->address,
- prefix,
- network->def->bridge,
- forwardIf);
-routeerr1:
- return -1;
-}
-
-static void
-networkRemoveRoutingIptablesRules(virNetworkObjPtr network,
- virNetworkIpDefPtr ipdef)
-{
- int prefix = virNetworkIpDefPrefix(ipdef);
- const char *forwardIf = virNetworkDefForwardIf(network->def, 0);
-
- if (prefix >= 0) {
- iptablesRemoveForwardAllowIn(&ipdef->address,
- prefix,
- network->def->bridge,
- forwardIf);
-
- iptablesRemoveForwardAllowOut(&ipdef->address,
- prefix,
- network->def->bridge,
- forwardIf);
- }
-}
-
-/* Add all once/network rules required for IPv6.
- * If no IPv6 addresses are defined and <network ipv6='yes'> is
- * specified, then allow IPv6 commuinications between virtual systems.
- * If any IPv6 addresses are defined, then add the rules for regular operation.
- */
-static int
-networkAddGeneralIp6tablesRules(virNetworkObjPtr network)
-{
-
- if (!virNetworkDefGetIpByIndex(network->def, AF_INET6, 0) &&
- !network->def->ipv6nogw) {
- return 0;
- }
-
- /* Catch all rules to block forwarding to/from bridges */
-
- if (iptablesAddForwardRejectOut(AF_INET6, network->def->bridge) < 0) {
- virReportError(VIR_ERR_SYSTEM_ERROR,
- _("failed to add ip6tables rule to block outbound traffic
from '%s'"),
- network->def->bridge);
- goto err1;
- }
-
- if (iptablesAddForwardRejectIn(AF_INET6, network->def->bridge) < 0) {
- virReportError(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(AF_INET6, network->def->bridge) < 0) {
- virReportError(VIR_ERR_SYSTEM_ERROR,
- _("failed to add ip6tables rule to allow cross bridge traffic
on '%s'"),
- network->def->bridge);
- goto err3;
- }
-
- /* if no IPv6 addresses are defined, we are done. */
- if (!virNetworkDefGetIpByIndex(network->def, AF_INET6, 0))
- return 0;
-
- /* allow DNS over IPv6 */
- if (iptablesAddTcpInput(AF_INET6, network->def->bridge, 53) < 0) {
- virReportError(VIR_ERR_SYSTEM_ERROR,
- _("failed to add ip6tables rule to allow DNS requests from
'%s'"),
- network->def->bridge);
- goto err4;
- }
-
- if (iptablesAddUdpInput(AF_INET6, network->def->bridge, 53) < 0) {
- virReportError(VIR_ERR_SYSTEM_ERROR,
- _("failed to add ip6tables rule to allow DNS requests from
'%s'"),
- network->def->bridge);
- goto err5;
- }
-
- if (iptablesAddUdpInput(AF_INET6, network->def->bridge, 547) < 0) {
- virReportError(VIR_ERR_SYSTEM_ERROR,
- _("failed to add ip6tables rule to allow DHCP6 requests from
'%s'"),
- network->def->bridge);
- goto err6;
- }
-
- return 0;
-
- /* unwind in reverse order from the point of failure */
-err6:
- iptablesRemoveUdpInput(AF_INET6, network->def->bridge, 53);
-err5:
- iptablesRemoveTcpInput(AF_INET6, network->def->bridge, 53);
-err4:
- iptablesRemoveForwardAllowCross(AF_INET6, network->def->bridge);
-err3:
- iptablesRemoveForwardRejectIn(AF_INET6, network->def->bridge);
-err2:
- iptablesRemoveForwardRejectOut(AF_INET6, network->def->bridge);
-err1:
- return -1;
-}
-
-static void
-networkRemoveGeneralIp6tablesRules(virNetworkObjPtr network)
-{
- if (!virNetworkDefGetIpByIndex(network->def, AF_INET6, 0) &&
- !network->def->ipv6nogw) {
- return;
- }
- if (virNetworkDefGetIpByIndex(network->def, AF_INET6, 0)) {
- iptablesRemoveUdpInput(AF_INET6, network->def->bridge, 547);
- iptablesRemoveUdpInput(AF_INET6, network->def->bridge, 53);
- iptablesRemoveTcpInput(AF_INET6, network->def->bridge, 53);
- }
-
- /* the following rules are there if no IPv6 address has been defined
- * but network->def->ipv6nogw == true
- */
- iptablesRemoveForwardAllowCross(AF_INET6, network->def->bridge);
- iptablesRemoveForwardRejectIn(AF_INET6, network->def->bridge);
- iptablesRemoveForwardRejectOut(AF_INET6, network->def->bridge);
-}
-
-static int
-networkAddGeneralIptablesRules(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(AF_INET, network->def->bridge, 67) < 0) {
- virReportError(VIR_ERR_SYSTEM_ERROR,
- _("failed to add iptables rule to allow DHCP requests from
'%s'"),
- network->def->bridge);
- goto err1;
- }
-
- if (iptablesAddUdpInput(AF_INET, network->def->bridge, 67) < 0) {
- virReportError(VIR_ERR_SYSTEM_ERROR,
- _("failed to add iptables rule to allow DHCP requests from
'%s'"),
- network->def->bridge);
- 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(network->def->bridge, 68) < 0)) {
- VIR_WARN("Could not add rule to fixup DHCP response checksums "
- "on network '%s'.", network->def->name);
- VIR_WARN("May need to update iptables package & kernel to support
CHECKSUM rule.");
- }
-
- /* allow DNS requests through to dnsmasq */
- if (iptablesAddTcpInput(AF_INET, network->def->bridge, 53) < 0) {
- virReportError(VIR_ERR_SYSTEM_ERROR,
- _("failed to add iptables rule to allow DNS requests from
'%s'"),
- network->def->bridge);
- goto err3;
- }
-
- if (iptablesAddUdpInput(AF_INET, network->def->bridge, 53) < 0) {
- virReportError(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 necessary */
- if (ipv4def && ipv4def->tftproot &&
- iptablesAddUdpInput(AF_INET, network->def->bridge, 69) < 0) {
- virReportError(VIR_ERR_SYSTEM_ERROR,
- _("failed to add iptables rule to allow TFTP requests from
'%s'"),
- network->def->bridge);
- goto err5;
- }
-
- /* Catch all rules to block forwarding to/from bridges */
-
- if (iptablesAddForwardRejectOut(AF_INET, network->def->bridge) < 0) {
- virReportError(VIR_ERR_SYSTEM_ERROR,
- _("failed to add iptables rule to block outbound traffic from
'%s'"),
- network->def->bridge);
- goto err6;
- }
-
- if (iptablesAddForwardRejectIn(AF_INET, network->def->bridge) < 0) {
- virReportError(VIR_ERR_SYSTEM_ERROR,
- _("failed to add iptables rule to block inbound traffic to
'%s'"),
- network->def->bridge);
- goto err7;
- }
-
- /* Allow traffic between guests on the same bridge */
- if (iptablesAddForwardAllowCross(AF_INET, network->def->bridge) < 0) {
- virReportError(VIR_ERR_SYSTEM_ERROR,
- _("failed to add iptables rule to allow cross bridge traffic
on '%s'"),
- network->def->bridge);
- goto err8;
- }
-
- /* add IPv6 general rules, if needed */
- if (networkAddGeneralIp6tablesRules(network) < 0) {
- goto err9;
- }
-
- return 0;
-
- /* unwind in reverse order from the point of failure */
-err9:
- iptablesRemoveForwardAllowCross(AF_INET, network->def->bridge);
-err8:
- iptablesRemoveForwardRejectIn(AF_INET, network->def->bridge);
-err7:
- iptablesRemoveForwardRejectOut(AF_INET, network->def->bridge);
-err6:
- if (ipv4def && ipv4def->tftproot) {
- iptablesRemoveUdpInput(AF_INET, network->def->bridge, 69);
- }
-err5:
- iptablesRemoveUdpInput(AF_INET, network->def->bridge, 53);
-err4:
- iptablesRemoveTcpInput(AF_INET, network->def->bridge, 53);
-err3:
- iptablesRemoveUdpInput(AF_INET, network->def->bridge, 67);
-err2:
- iptablesRemoveTcpInput(AF_INET, network->def->bridge, 67);
-err1:
- return -1;
-}
-
-static void
-networkRemoveGeneralIptablesRules(virNetworkObjPtr network)
-{
- int ii;
- virNetworkIpDefPtr ipv4def;
-
- networkRemoveGeneralIp6tablesRules(network);
-
- for (ii = 0;
- (ipv4def = virNetworkDefGetIpByIndex(network->def, AF_INET, ii));
- ii++) {
- if (ipv4def->nranges || ipv4def->nhosts || ipv4def->tftproot)
- break;
- }
-
- iptablesRemoveForwardAllowCross(AF_INET, network->def->bridge);
- iptablesRemoveForwardRejectIn(AF_INET, network->def->bridge);
- iptablesRemoveForwardRejectOut(AF_INET, network->def->bridge);
- if (ipv4def && ipv4def->tftproot) {
- iptablesRemoveUdpInput(AF_INET, network->def->bridge, 69);
- }
- iptablesRemoveUdpInput(AF_INET, network->def->bridge, 53);
- iptablesRemoveTcpInput(AF_INET, network->def->bridge, 53);
- if (ipv4def && (ipv4def->nranges || ipv4def->nhosts)) {
- iptablesRemoveOutputFixUdpChecksum(network->def->bridge, 68);
- }
- iptablesRemoveUdpInput(AF_INET, network->def->bridge, 67);
- iptablesRemoveTcpInput(AF_INET, network->def->bridge, 67);
-}
-
-static int
-networkAddIpSpecificIptablesRules(virNetworkObjPtr network,
- virNetworkIpDefPtr ipdef)
-{
- /* 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->forward.type == VIR_NETWORK_FORWARD_NAT) {
- if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET))
- return networkAddMasqueradingIptablesRules(network, ipdef);
- else if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6))
- return networkAddRoutingIptablesRules(network, ipdef);
- } else if (network->def->forward.type == VIR_NETWORK_FORWARD_ROUTE) {
- return networkAddRoutingIptablesRules(network, ipdef);
- }
- return 0;
-}
-
-static void
-networkRemoveIpSpecificIptablesRules(virNetworkObjPtr network,
- virNetworkIpDefPtr ipdef)
-{
- if (network->def->forward.type == VIR_NETWORK_FORWARD_NAT) {
- if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET))
- networkRemoveMasqueradingIptablesRules(network, ipdef);
- else if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6))
- networkRemoveRoutingIptablesRules(network, ipdef);
- } else if (network->def->forward.type == VIR_NETWORK_FORWARD_ROUTE) {
- networkRemoveRoutingIptablesRules(network, ipdef);
- }
-}
-
-/* Add all rules for all ip addresses (and general rules) on a network */
-static int
-networkAddIptablesRules(virNetworkObjPtr network)
-{
- int ii;
- virNetworkIpDefPtr ipdef;
- virErrorPtr orig_error;
-
- /* Add "once per network" rules */
- if (networkAddGeneralIptablesRules(network) < 0)
- return -1;
-
- for (ii = 0;
- (ipdef = virNetworkDefGetIpByIndex(network->def, AF_UNSPEC, ii));
- ii++) {
- /* Add address-specific iptables rules */
- if (networkAddIpSpecificIptablesRules(network, ipdef) < 0) {
- goto err;
- }
- }
- return 0;
-
-err:
- /* store the previous error message before attempting removal of rules */
- orig_error = virSaveLastError();
-
- /* 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(network, ipdef);
- }
- networkRemoveGeneralIptablesRules(network);
-
- /* return the original error */
- virSetError(orig_error);
- virFreeError(orig_error);
- return -1;
-}
-
-/* Remove all rules for all ip addresses (and general rules) on a network */
-static void
-networkRemoveIptablesRules(virNetworkObjPtr network)
-{
- int ii;
- virNetworkIpDefPtr ipdef;
-
- for (ii = 0;
- (ipdef = virNetworkDefGetIpByIndex(network->def, AF_UNSPEC, ii));
- ii++) {
- networkRemoveIpSpecificIptablesRules(network, ipdef);
- }
- networkRemoveGeneralIptablesRules(network);
-}
-
-static void
-networkReloadIptablesRules(struct network_driver *driver)
+networkReloadFirewallRules(struct network_driver *driver)
{
unsigned int i;
@@ -2144,8 +1538,8 @@ networkReloadIptablesRules(struct network_driver *driver)
/* Only the three L3 network types that are configured by libvirt
* need to have iptables rules reloaded.
*/
- networkRemoveIptablesRules(network);
- if (networkAddIptablesRules(network) < 0) {
+ networkRemoveFirewallRules(network);
+ if (networkAddFirewallRules(network) < 0) {
/* failed to add but already logged */
}
}
@@ -2240,103 +1634,6 @@ cleanup:
return ret;
}
-#define PROC_NET_ROUTE "/proc/net/route"
-
-/* XXX: This function can be a lot more exhaustive, there are certainly
- * other scenarios where we can ruin host network connectivity.
- * XXX: Using a proper library is preferred over parsing /proc
- */
-static int
-networkCheckRouteCollision(virNetworkObjPtr network)
-{
- int ret = 0, len;
- char *cur, *buf = NULL;
- /* allow for up to 100000 routes (each line is 128 bytes) */
- enum {MAX_ROUTE_SIZE = 128*100000};
-
- /* Read whole routing table into memory */
- if ((len = virFileReadAll(PROC_NET_ROUTE, MAX_ROUTE_SIZE, &buf)) < 0)
- goto out;
-
- /* Dropping the last character shouldn't hurt */
- if (len > 0)
- buf[len-1] = '\0';
-
- VIR_DEBUG("%s output:\n%s", PROC_NET_ROUTE, buf);
-
- if (!STRPREFIX(buf, "Iface"))
- goto out;
-
- /* First line is just headings, skip it */
- cur = strchr(buf, '\n');
- if (cur)
- cur++;
-
- while (cur) {
- char iface[17], dest[128], mask[128];
- unsigned int addr_val, mask_val;
- virNetworkIpDefPtr ipdef;
- int num, ii;
-
- /* NUL-terminate the line, so sscanf doesn't go beyond a newline. */
- char *nl = strchr(cur, '\n');
- if (nl) {
- *nl++ = '\0';
- }
-
- num = sscanf(cur, "%16s %127s %*s %*s %*s %*s %*s %127s",
- iface, dest, mask);
- cur = nl;
-
- if (num != 3) {
- VIR_DEBUG("Failed to parse %s", PROC_NET_ROUTE);
- continue;
- }
-
- if (virStrToLong_ui(dest, NULL, 16, &addr_val) < 0) {
- VIR_DEBUG("Failed to convert network address %s to uint", dest);
- continue;
- }
-
- if (virStrToLong_ui(mask, NULL, 16, &mask_val) < 0) {
- VIR_DEBUG("Failed to convert network mask %s to uint", mask);
- continue;
- }
-
- addr_val &= mask_val;
-
- 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)) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Network is already in use by interface %s"),
- iface);
- ret = -1;
- goto out;
- }
- }
- }
-
-out:
- VIR_FREE(buf);
- return ret;
-}
-
/* add an IP address to a bridge */
static int
networkAddAddrToBridge(virNetworkObjPtr network,
@@ -2471,7 +1768,7 @@ networkStartNetworkVirtual(struct network_driver *driver,
goto err1;
/* Add "once per network" rules */
- if (networkAddIptablesRules(network) < 0)
+ if (networkAddFirewallRules(network) < 0)
goto err1;
for (ii = 0;
@@ -2564,7 +1861,7 @@ networkStartNetworkVirtual(struct network_driver *driver,
err2:
if (!save_err)
save_err = virSaveLastError();
- networkRemoveIptablesRules(network);
+ networkRemoveFirewallRules(network);
err1:
if (!save_err)
@@ -2622,7 +1919,7 @@ static int networkShutdownNetworkVirtual(struct network_driver
*driver ATTRIBUTE
ignore_value(virNetDevSetOnline(network->def->bridge, 0));
- networkRemoveIptablesRules(network);
+ networkRemoveFirewallRules(network);
ignore_value(virNetDevBridgeDelete(network->def->bridge));
@@ -3445,8 +2742,8 @@ networkUpdate(virNetworkPtr net,
network->def->forward.type == VIR_NETWORK_FORWARD_NAT ||
network->def->forward.type == VIR_NETWORK_FORWARD_ROUTE)) {
/* these could affect the iptables rules */
- networkRemoveIptablesRules(network);
- if (networkAddIptablesRules(network) < 0)
+ networkRemoveFirewallRules(network);
+ if (networkAddFirewallRules(network) < 0)
goto cleanup;
}
diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c
new file mode 100644
index 0000000..efa3d0a
--- /dev/null
+++ b/src/network/bridge_driver_linux.c
@@ -0,0 +1,709 @@
+/*
+ * bridge_driver.c: core driver methods for managing network
+ *
+ * Copyright (C) 2006-2013 Red Hat, Inc.
+ * Copyright (C) 2006 Daniel P. Berrange
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library. If not, see
+ * <http://www.gnu.org/licenses/>;.
+ *
+ * Author: Daniel P. Berrange <berrange(a)redhat.com>
+ */
+
+#include <config.h>
+
+#include "viralloc.h"
+#include "virfile.h"
+#include "viriptables.h"
+#include "virstring.h"
+
+#define VIR_FROM_THIS VIR_FROM_NONE
+
+#define PROC_NET_ROUTE "/proc/net/route"
+
+/* XXX: This function can be a lot more exhaustive, there are certainly
+ * other scenarios where we can ruin host network connectivity.
+ * XXX: Using a proper library is preferred over parsing /proc
+ */
+int networkCheckRouteCollision(virNetworkObjPtr network)
+{
+ int ret = 0, len;
+ char *cur, *buf = NULL;
+ /* allow for up to 100000 routes (each line is 128 bytes) */
+ enum {MAX_ROUTE_SIZE = 128*100000};
+
+ /* Read whole routing table into memory */
+ if ((len = virFileReadAll(PROC_NET_ROUTE, MAX_ROUTE_SIZE, &buf)) < 0)
+ goto out;
+
+ /* Dropping the last character shouldn't hurt */
+ if (len > 0)
+ buf[len-1] = '\0';
+
+ VIR_DEBUG("%s output:\n%s", PROC_NET_ROUTE, buf);
+
+ if (!STRPREFIX(buf, "Iface"))
+ goto out;
+
+ /* First line is just headings, skip it */
+ cur = strchr(buf, '\n');
+ if (cur)
+ cur++;
+
+ while (cur) {
+ char iface[17], dest[128], mask[128];
+ unsigned int addr_val, mask_val;
+ virNetworkIpDefPtr ipdef;
+ int num, ii;
+
+ /* NUL-terminate the line, so sscanf doesn't go beyond a newline. */
+ char *nl = strchr(cur, '\n');
+ if (nl) {
+ *nl++ = '\0';
+ }
+
+ num = sscanf(cur, "%16s %127s %*s %*s %*s %*s %*s %127s",
+ iface, dest, mask);
+ cur = nl;
+
+ if (num != 3) {
+ VIR_DEBUG("Failed to parse %s", PROC_NET_ROUTE);
+ continue;
+ }
+
+ if (virStrToLong_ui(dest, NULL, 16, &addr_val) < 0) {
+ VIR_DEBUG("Failed to convert network address %s to uint", dest);
+ continue;
+ }
+
+ if (virStrToLong_ui(mask, NULL, 16, &mask_val) < 0) {
+ VIR_DEBUG("Failed to convert network mask %s to uint", mask);
+ continue;
+ }
+
+ addr_val &= mask_val;
+
+ 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)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Network is already in use by interface %s"),
+ iface);
+ ret = -1;
+ goto out;
+ }
+ }
+ }
+
+out:
+ VIR_FREE(buf);
+ return ret;
+}
+
+
+int networkAddMasqueradingFirewallRules(virNetworkObjPtr network,
+ virNetworkIpDefPtr ipdef)
+{
+ int prefix = virNetworkIpDefPrefix(ipdef);
+ const char *forwardIf = virNetworkDefForwardIf(network->def, 0);
+
+ if (prefix < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Invalid prefix or netmask for '%s'"),
+ network->def->bridge);
+ goto masqerr1;
+ }
+
+ /* allow forwarding packets from the bridge interface */
+ if (iptablesAddForwardAllowOut(&ipdef->address,
+ prefix,
+ network->def->bridge,
+ forwardIf) < 0) {
+ virReportError(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 (iptablesAddForwardAllowRelatedIn(&ipdef->address,
+ prefix,
+ network->def->bridge,
+ forwardIf) < 0) {
+ virReportError(VIR_ERR_SYSTEM_ERROR,
+ _("failed to add iptables rule to allow forwarding to
'%s'"),
+ network->def->bridge);
+ goto masqerr2;
+ }
+
+ /*
+ * Enable masquerading.
+ *
+ * We need to end up with 3 rules in the table in this order
+ *
+ * 1. protocol=tcp with sport mapping restriction
+ * 2. protocol=udp with sport mapping restriction
+ * 3. generic any protocol
+ *
+ * The sport mappings are required, because default IPtables
+ * MASQUERADE maintain port numbers unchanged where possible.
+ *
+ * NFS can be configured to only "trust" port numbers < 1023.
+ *
+ * Guests using NAT thus need to be prevented from having port
+ * numbers < 1023, otherwise they can bypass the NFS "security"
+ * check on the source port number.
+ *
+ * Since we use '--insert' to add rules to the header of the
+ * chain, we actually need to add them in the reverse of the
+ * order just mentioned !
+ */
+
+ /* First the generic masquerade rule for other protocols */
+ if (iptablesAddForwardMasquerade(&ipdef->address,
+ prefix,
+ forwardIf,
+ &network->def->forward.addr,
+ &network->def->forward.port,
+ NULL) < 0) {
+ if (forwardIf)
+ virReportError(VIR_ERR_SYSTEM_ERROR,
+ _("failed to add iptables rule to enable masquerading to
%s"),
+ forwardIf);
+ else
+ virReportError(VIR_ERR_SYSTEM_ERROR, "%s",
+ _("failed to add iptables rule to enable
masquerading"));
+ goto masqerr3;
+ }
+
+ /* UDP with a source port restriction */
+ if (iptablesAddForwardMasquerade(&ipdef->address,
+ prefix,
+ forwardIf,
+ &network->def->forward.addr,
+ &network->def->forward.port,
+ "udp") < 0) {
+ if (forwardIf)
+ virReportError(VIR_ERR_SYSTEM_ERROR,
+ _("failed to add iptables rule to enable UDP masquerading
to %s"),
+ forwardIf);
+ else
+ virReportError(VIR_ERR_SYSTEM_ERROR, "%s",
+ _("failed to add iptables rule to enable UDP
masquerading"));
+ goto masqerr4;
+ }
+
+ /* TCP with a source port restriction */
+ if (iptablesAddForwardMasquerade(&ipdef->address,
+ prefix,
+ forwardIf,
+ &network->def->forward.addr,
+ &network->def->forward.port,
+ "tcp") < 0) {
+ if (forwardIf)
+ virReportError(VIR_ERR_SYSTEM_ERROR,
+ _("failed to add iptables rule to enable TCP masquerading
to %s"),
+ forwardIf);
+ else
+ virReportError(VIR_ERR_SYSTEM_ERROR, "%s",
+ _("failed to add iptables rule to enable TCP
masquerading"));
+ goto masqerr5;
+ }
+
+ return 0;
+
+ masqerr5:
+ iptablesRemoveForwardMasquerade(&ipdef->address,
+ prefix,
+ forwardIf,
+ &network->def->forward.addr,
+ &network->def->forward.port,
+ "udp");
+ masqerr4:
+ iptablesRemoveForwardMasquerade(&ipdef->address,
+ prefix,
+ forwardIf,
+ &network->def->forward.addr,
+ &network->def->forward.port,
+ NULL);
+ masqerr3:
+ iptablesRemoveForwardAllowRelatedIn(&ipdef->address,
+ prefix,
+ network->def->bridge,
+ forwardIf);
+ masqerr2:
+ iptablesRemoveForwardAllowOut(&ipdef->address,
+ prefix,
+ network->def->bridge,
+ forwardIf);
+ masqerr1:
+ return -1;
+}
+
+void networkRemoveMasqueradingFirewallRules(virNetworkObjPtr network,
+ virNetworkIpDefPtr ipdef)
+{
+ int prefix = virNetworkIpDefPrefix(ipdef);
+ const char *forwardIf = virNetworkDefForwardIf(network->def, 0);
+
+ if (prefix >= 0) {
+ iptablesRemoveForwardMasquerade(&ipdef->address,
+ prefix,
+ forwardIf,
+ &network->def->forward.addr,
+ &network->def->forward.port,
+ "tcp");
+ iptablesRemoveForwardMasquerade(&ipdef->address,
+ prefix,
+ forwardIf,
+ &network->def->forward.addr,
+ &network->def->forward.port,
+ "udp");
+ iptablesRemoveForwardMasquerade(&ipdef->address,
+ prefix,
+ forwardIf,
+ &network->def->forward.addr,
+ &network->def->forward.port,
+ NULL);
+
+ iptablesRemoveForwardAllowRelatedIn(&ipdef->address,
+ prefix,
+ network->def->bridge,
+ forwardIf);
+ iptablesRemoveForwardAllowOut(&ipdef->address,
+ prefix,
+ network->def->bridge,
+ forwardIf);
+ }
+}
+
+int networkAddRoutingFirewallRules(virNetworkObjPtr network,
+ virNetworkIpDefPtr ipdef)
+{
+ int prefix = virNetworkIpDefPrefix(ipdef);
+ const char *forwardIf = virNetworkDefForwardIf(network->def, 0);
+
+ if (prefix < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Invalid prefix or netmask for '%s'"),
+ network->def->bridge);
+ goto routeerr1;
+ }
+
+ /* allow routing packets from the bridge interface */
+ if (iptablesAddForwardAllowOut(&ipdef->address,
+ prefix,
+ network->def->bridge,
+ forwardIf) < 0) {
+ virReportError(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 (iptablesAddForwardAllowIn(&ipdef->address,
+ prefix,
+ network->def->bridge,
+ forwardIf) < 0) {
+ virReportError(VIR_ERR_SYSTEM_ERROR,
+ _("failed to add iptables rule to allow routing to
'%s'"),
+ network->def->bridge);
+ goto routeerr2;
+ }
+
+ return 0;
+
+routeerr2:
+ iptablesRemoveForwardAllowOut(&ipdef->address,
+ prefix,
+ network->def->bridge,
+ forwardIf);
+routeerr1:
+ return -1;
+}
+
+void networkRemoveRoutingFirewallRules(virNetworkObjPtr network,
+ virNetworkIpDefPtr ipdef)
+{
+ int prefix = virNetworkIpDefPrefix(ipdef);
+ const char *forwardIf = virNetworkDefForwardIf(network->def, 0);
+
+ if (prefix >= 0) {
+ iptablesRemoveForwardAllowIn(&ipdef->address,
+ prefix,
+ network->def->bridge,
+ forwardIf);
+
+ iptablesRemoveForwardAllowOut(&ipdef->address,
+ prefix,
+ network->def->bridge,
+ forwardIf);
+ }
+}
+
+/* Add all once/network rules required for IPv6.
+ * If no IPv6 addresses are defined and <network ipv6='yes'> is
+ * specified, then allow IPv6 commuinications between virtual systems.
+ * If any IPv6 addresses are defined, then add the rules for regular operation.
+ */
+static int
+networkAddGeneralIp6tablesRules(virNetworkObjPtr network)
+{
+
+ if (!virNetworkDefGetIpByIndex(network->def, AF_INET6, 0) &&
+ !network->def->ipv6nogw) {
+ return 0;
+ }
+
+ /* Catch all rules to block forwarding to/from bridges */
+
+ if (iptablesAddForwardRejectOut(AF_INET6, network->def->bridge) < 0) {
+ virReportError(VIR_ERR_SYSTEM_ERROR,
+ _("failed to add ip6tables rule to block outbound traffic
from '%s'"),
+ network->def->bridge);
+ goto err1;
+ }
+
+ if (iptablesAddForwardRejectIn(AF_INET6, network->def->bridge) < 0) {
+ virReportError(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(AF_INET6, network->def->bridge) < 0) {
+ virReportError(VIR_ERR_SYSTEM_ERROR,
+ _("failed to add ip6tables rule to allow cross bridge traffic
on '%s'"),
+ network->def->bridge);
+ goto err3;
+ }
+
+ /* if no IPv6 addresses are defined, we are done. */
+ if (!virNetworkDefGetIpByIndex(network->def, AF_INET6, 0))
+ return 0;
+
+ /* allow DNS over IPv6 */
+ if (iptablesAddTcpInput(AF_INET6, network->def->bridge, 53) < 0) {
+ virReportError(VIR_ERR_SYSTEM_ERROR,
+ _("failed to add ip6tables rule to allow DNS requests from
'%s'"),
+ network->def->bridge);
+ goto err4;
+ }
+
+ if (iptablesAddUdpInput(AF_INET6, network->def->bridge, 53) < 0) {
+ virReportError(VIR_ERR_SYSTEM_ERROR,
+ _("failed to add ip6tables rule to allow DNS requests from
'%s'"),
+ network->def->bridge);
+ goto err5;
+ }
+
+ if (iptablesAddUdpInput(AF_INET6, network->def->bridge, 547) < 0) {
+ virReportError(VIR_ERR_SYSTEM_ERROR,
+ _("failed to add ip6tables rule to allow DHCP6 requests from
'%s'"),
+ network->def->bridge);
+ goto err6;
+ }
+
+ return 0;
+
+ /* unwind in reverse order from the point of failure */
+err6:
+ iptablesRemoveUdpInput(AF_INET6, network->def->bridge, 53);
+err5:
+ iptablesRemoveTcpInput(AF_INET6, network->def->bridge, 53);
+err4:
+ iptablesRemoveForwardAllowCross(AF_INET6, network->def->bridge);
+err3:
+ iptablesRemoveForwardRejectIn(AF_INET6, network->def->bridge);
+err2:
+ iptablesRemoveForwardRejectOut(AF_INET6, network->def->bridge);
+err1:
+ return -1;
+}
+
+static void
+networkRemoveGeneralIp6tablesRules(virNetworkObjPtr network)
+{
+ if (!virNetworkDefGetIpByIndex(network->def, AF_INET6, 0) &&
+ !network->def->ipv6nogw) {
+ return;
+ }
+ if (virNetworkDefGetIpByIndex(network->def, AF_INET6, 0)) {
+ iptablesRemoveUdpInput(AF_INET6, network->def->bridge, 547);
+ iptablesRemoveUdpInput(AF_INET6, network->def->bridge, 53);
+ iptablesRemoveTcpInput(AF_INET6, network->def->bridge, 53);
+ }
+
+ /* the following rules are there if no IPv6 address has been defined
+ * but network->def->ipv6nogw == true
+ */
+ iptablesRemoveForwardAllowCross(AF_INET6, network->def->bridge);
+ iptablesRemoveForwardRejectIn(AF_INET6, network->def->bridge);
+ iptablesRemoveForwardRejectOut(AF_INET6, network->def->bridge);
+}
+
+int networkAddGeneralFirewallRules(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(AF_INET, network->def->bridge, 67) < 0) {
+ virReportError(VIR_ERR_SYSTEM_ERROR,
+ _("failed to add iptables rule to allow DHCP requests from
'%s'"),
+ network->def->bridge);
+ goto err1;
+ }
+
+ if (iptablesAddUdpInput(AF_INET, network->def->bridge, 67) < 0) {
+ virReportError(VIR_ERR_SYSTEM_ERROR,
+ _("failed to add iptables rule to allow DHCP requests from
'%s'"),
+ network->def->bridge);
+ 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(network->def->bridge, 68) < 0)) {
+ VIR_WARN("Could not add rule to fixup DHCP response checksums "
+ "on network '%s'.", network->def->name);
+ VIR_WARN("May need to update iptables package & kernel to support
CHECKSUM rule.");
+ }
+
+ /* allow DNS requests through to dnsmasq */
+ if (iptablesAddTcpInput(AF_INET, network->def->bridge, 53) < 0) {
+ virReportError(VIR_ERR_SYSTEM_ERROR,
+ _("failed to add iptables rule to allow DNS requests from
'%s'"),
+ network->def->bridge);
+ goto err3;
+ }
+
+ if (iptablesAddUdpInput(AF_INET, network->def->bridge, 53) < 0) {
+ virReportError(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 necessary */
+ if (ipv4def && ipv4def->tftproot &&
+ iptablesAddUdpInput(AF_INET, network->def->bridge, 69) < 0) {
+ virReportError(VIR_ERR_SYSTEM_ERROR,
+ _("failed to add iptables rule to allow TFTP requests from
'%s'"),
+ network->def->bridge);
+ goto err5;
+ }
+
+ /* Catch all rules to block forwarding to/from bridges */
+
+ if (iptablesAddForwardRejectOut(AF_INET, network->def->bridge) < 0) {
+ virReportError(VIR_ERR_SYSTEM_ERROR,
+ _("failed to add iptables rule to block outbound traffic from
'%s'"),
+ network->def->bridge);
+ goto err6;
+ }
+
+ if (iptablesAddForwardRejectIn(AF_INET, network->def->bridge) < 0) {
+ virReportError(VIR_ERR_SYSTEM_ERROR,
+ _("failed to add iptables rule to block inbound traffic to
'%s'"),
+ network->def->bridge);
+ goto err7;
+ }
+
+ /* Allow traffic between guests on the same bridge */
+ if (iptablesAddForwardAllowCross(AF_INET, network->def->bridge) < 0) {
+ virReportError(VIR_ERR_SYSTEM_ERROR,
+ _("failed to add iptables rule to allow cross bridge traffic
on '%s'"),
+ network->def->bridge);
+ goto err8;
+ }
+
+ /* add IPv6 general rules, if needed */
+ if (networkAddGeneralIp6tablesRules(network) < 0) {
+ goto err9;
+ }
+
+ return 0;
+
+ /* unwind in reverse order from the point of failure */
+err9:
+ iptablesRemoveForwardAllowCross(AF_INET, network->def->bridge);
+err8:
+ iptablesRemoveForwardRejectIn(AF_INET, network->def->bridge);
+err7:
+ iptablesRemoveForwardRejectOut(AF_INET, network->def->bridge);
+err6:
+ if (ipv4def && ipv4def->tftproot) {
+ iptablesRemoveUdpInput(AF_INET, network->def->bridge, 69);
+ }
+err5:
+ iptablesRemoveUdpInput(AF_INET, network->def->bridge, 53);
+err4:
+ iptablesRemoveTcpInput(AF_INET, network->def->bridge, 53);
+err3:
+ iptablesRemoveUdpInput(AF_INET, network->def->bridge, 67);
+err2:
+ iptablesRemoveTcpInput(AF_INET, network->def->bridge, 67);
+err1:
+ return -1;
+}
+
+void networkRemoveGeneralFirewallRules(virNetworkObjPtr network)
+{
+ int ii;
+ virNetworkIpDefPtr ipv4def;
+
+ networkRemoveGeneralIp6tablesRules(network);
+
+ for (ii = 0;
+ (ipv4def = virNetworkDefGetIpByIndex(network->def, AF_INET, ii));
+ ii++) {
+ if (ipv4def->nranges || ipv4def->nhosts || ipv4def->tftproot)
+ break;
+ }
+
+ iptablesRemoveForwardAllowCross(AF_INET, network->def->bridge);
+ iptablesRemoveForwardRejectIn(AF_INET, network->def->bridge);
+ iptablesRemoveForwardRejectOut(AF_INET, network->def->bridge);
+ if (ipv4def && ipv4def->tftproot) {
+ iptablesRemoveUdpInput(AF_INET, network->def->bridge, 69);
+ }
+ iptablesRemoveUdpInput(AF_INET, network->def->bridge, 53);
+ iptablesRemoveTcpInput(AF_INET, network->def->bridge, 53);
+ if (ipv4def && (ipv4def->nranges || ipv4def->nhosts)) {
+ iptablesRemoveOutputFixUdpChecksum(network->def->bridge, 68);
+ }
+ iptablesRemoveUdpInput(AF_INET, network->def->bridge, 67);
+ iptablesRemoveTcpInput(AF_INET, network->def->bridge, 67);
+}
+
+int networkAddIpSpecificFirewallRules(virNetworkObjPtr network,
+ virNetworkIpDefPtr ipdef)
+{
+ /* 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->forward.type == VIR_NETWORK_FORWARD_NAT) {
+ if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET))
+ return networkAddMasqueradingFirewallRules(network, ipdef);
+ else if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6))
+ return networkAddRoutingFirewallRules(network, ipdef);
+ } else if (network->def->forward.type == VIR_NETWORK_FORWARD_ROUTE) {
+ return networkAddRoutingFirewallRules(network, ipdef);
+ }
+ return 0;
+}
+
+void networkRemoveIpSpecificFirewallRules(virNetworkObjPtr network,
+ virNetworkIpDefPtr ipdef)
+{
+ if (network->def->forward.type == VIR_NETWORK_FORWARD_NAT) {
+ if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET))
+ networkRemoveMasqueradingFirewallRules(network, ipdef);
+ else if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6))
+ networkRemoveRoutingFirewallRules(network, ipdef);
+ } else if (network->def->forward.type == VIR_NETWORK_FORWARD_ROUTE) {
+ networkRemoveRoutingFirewallRules(network, ipdef);
+ }
+}
+
+/* Add all rules for all ip addresses (and general rules) on a network */
+int networkAddFirewallRules(virNetworkObjPtr network)
+{
+ int ii;
+ virNetworkIpDefPtr ipdef;
+ virErrorPtr orig_error;
+
+ /* Add "once per network" rules */
+ if (networkAddGeneralFirewallRules(network) < 0)
+ return -1;
+
+ for (ii = 0;
+ (ipdef = virNetworkDefGetIpByIndex(network->def, AF_UNSPEC, ii));
+ ii++) {
+ /* Add address-specific iptables rules */
+ if (networkAddIpSpecificFirewallRules(network, ipdef) < 0) {
+ goto err;
+ }
+ }
+ return 0;
+
+err:
+ /* store the previous error message before attempting removal of rules */
+ orig_error = virSaveLastError();
+
+ /* The final failed call to networkAddIpSpecificFirewallRules 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))) {
+ networkRemoveIpSpecificFirewallRules(network, ipdef);
+ }
+ networkRemoveGeneralFirewallRules(network);
+
+ /* return the original error */
+ virSetError(orig_error);
+ virFreeError(orig_error);
+ return -1;
+}
+
+/* Remove all rules for all ip addresses (and general rules) on a network */
+void networkRemoveFirewallRules(virNetworkObjPtr network)
+{
+ int ii;
+ virNetworkIpDefPtr ipdef;
+
+ for (ii = 0;
+ (ipdef = virNetworkDefGetIpByIndex(network->def, AF_UNSPEC, ii));
+ ii++) {
+ networkRemoveIpSpecificFirewallRules(network, ipdef);
+ }
+ networkRemoveGeneralFirewallRules(network);
+}
diff --git a/src/network/bridge_driver_noop.c b/src/network/bridge_driver_noop.c
new file mode 100644
index 0000000..90e6b12
--- /dev/null
+++ b/src/network/bridge_driver_noop.c
@@ -0,0 +1,80 @@
+/*
+ * bridge_driver.c: core driver methods for managing network
+ *
+ * Copyright (C) 2006-2013 Red Hat, Inc.
+ * Copyright (C) 2006 Daniel P. Berrange
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library. If not, see
+ * <http://www.gnu.org/licenses/>;.
+ *
+ * Author: Daniel P. Berrange <berrange(a)redhat.com>
+ */
+
+#include <config.h>
+
+int networkCheckRouteCollision(virNetworkObjPtr network)
+{
+ return 0;
+}
+
+int networkAddMasqueradingFirewallRules(virNetworkObjPtr network ATTRIBUTE_UNUSED,
+ virNetworkIpDefPtr ipdef ATTRIBUTE_UNUSED)
+{
+ return 0;
+}
+
+void networkRemoveMasqueradingFirewallRules(virNetworkObjPtr network ATTRIBUTE_UNUSED,
+ virNetworkIpDefPtr ipdef ATTRIBUTE_UNUSED)
+{
+}
+
+int networkAddRoutingFirewallRules(virNetworkObjPtr network ATTRIBUTE_UNUSED,
+ virNetworkIpDefPtr ipdef ATTRIBUTE_UNUSED)
+{
+ return 0;
+}
+
+void networkRemoveRoutingFirewallRules(virNetworkObjPtr network ATTRIBUTE_UNUSED,
+ virNetworkIpDefPtr ipdef ATTRIBUTE_UNUSED)
+{
+}
+
+int networkAddGeneralFirewallRules(virNetworkObjPtr network ATTRIBUTE_UNUSED)
+{
+ return 0;
+}
+
+void networkRemoveGeneralFirewallRules(virNetworkObjPtr network ATTRIBUTE_UNUSED)
+{
+}
+
+int networkAddIpSpecificFirewallRules(virNetworkObjPtr network ATTRIBUTE_UNUSED,
+ virNetworkIpDefPtr ipdef ATTRIBUTE_UNUSED)
+{
+ return 0;
+}
+
+void networkRemoveIpSpecificFirewallRules(virNetworkObjPtr network ATTRIBUTE_UNUSED,
+ virNetworkIpDefPtr ipdef ATTRIBUTE_UNUSED)
+{
+}
+
+int networkAddFirewallRules(virNetworkObjPtr network ATTRIBUTE_UNUSED)
+{
+ return 0;
+}
+
+void networkRemoveFirewallRules(virNetworkObjPtr network ATTRIBUTE_UNUSED)
+{
+}
diff --git a/src/network/bridge_driver_platform.c b/src/network/bridge_driver_platform.c
new file mode 100644
index 0000000..65a2ffd
--- /dev/null
+++ b/src/network/bridge_driver_platform.c
@@ -0,0 +1,32 @@
+/*
+ * bridge_driver.c: core driver methods for managing network
+ *
+ * Copyright (C) 2006-2013 Red Hat, Inc.
+ * Copyright (C) 2006 Daniel P. Berrange
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library. If not, see
+ * <http://www.gnu.org/licenses/>;.
+ *
+ * Author: Daniel P. Berrange <berrange(a)redhat.com>
+ */
+
+#include <config.h>
+
+#include "bridge_driver_platform.h"
+
+#if defined(__linux__)
+# include "bridge_driver_linux.c"
+#else
+# include "bridge_driver_noop.c"
+#endif
diff --git a/src/network/bridge_driver_platform.h b/src/network/bridge_driver_platform.h
new file mode 100644
index 0000000..bab507f
--- /dev/null
+++ b/src/network/bridge_driver_platform.h
@@ -0,0 +1,77 @@
+/*
+ * bridge_driver.c: core driver methods for managing network
+ *
+ * Copyright (C) 2006-2013 Red Hat, Inc.
+ * Copyright (C) 2006 Daniel P. Berrange
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library. If not, see
+ * <http://www.gnu.org/licenses/>;.
+ *
+ * Author: Daniel P. Berrange <berrange(a)redhat.com>
+ */
+
+#ifndef __VIR_BRIDGE_DRIVER_PLATFORM_H__
+# define __VIR_BRIDGE_DRIVER_PLATFORM_H__
+
+# include "internal.h"
+# include "virlog.h"
+# include "virthread.h"
+# include "virdnsmasq.h"
+# include "network_conf.h"
+
+/* Main driver state */
+struct network_driver {
+ virMutex lock;
+
+ virNetworkObjList networks;
+
+ char *networkConfigDir;
+ char *networkAutostartDir;
+ char *stateDir;
+ char *pidDir;
+ char *dnsmasqStateDir;
+ char *radvdStateDir;
+ dnsmasqCapsPtr dnsmasqCaps;
+};
+
+
+int networkCheckRouteCollision(virNetworkObjPtr network);
+
+int networkAddMasqueradingFirewallRules(virNetworkObjPtr network,
+ virNetworkIpDefPtr ipdef);
+
+void networkRemoveMasqueradingFirewallRules(virNetworkObjPtr network,
+ virNetworkIpDefPtr ipdef);
+
+int networkAddRoutingFirewallRules(virNetworkObjPtr network,
+ virNetworkIpDefPtr ipdef);
+
+void networkRemoveRoutingFirewallRules(virNetworkObjPtr network,
+ virNetworkIpDefPtr ipdef);
+
+int networkAddGeneralFirewallRules(virNetworkObjPtr network);
+
+void networkRemoveGeneralFirewallRules(virNetworkObjPtr network);
+
+int networkAddIpSpecificFirewallRules(virNetworkObjPtr network,
+ virNetworkIpDefPtr ipdef);
+
+void networkRemoveIpSpecificFirewallRules(virNetworkObjPtr network,
+ virNetworkIpDefPtr ipdef);
+
+int networkAddFirewallRules(virNetworkObjPtr network);
+
+void networkRemoveFirewallRules(virNetworkObjPtr network);
+
+#endif /* __VIR_BRIDGE_DRIVER_PLATFORM_H__ */
--
1.7.9.5
7:53 a.m.
On Thu, Jul 04, 2013 at 08:25:32PM +0400, Roman Bogorodskiy wrote:
* Functions were renamed: s/Iptables/Firewall/
to make names more general.
* Platform specific things (e.g. firewalling and route
collision checks) were moved into bridge_driver_platform
* Created two platform specific implementations:
- bridge_driver_linux: Linux implementation using iptables,
it's actually the code moved from bridge_driver.c
- bridge_driver_noop: dump implementation that does nothing
---
po/POTFILES.in | 1 +
src/Makefile.am | 5 +-
src/network/bridge_driver.c | 729 +---------------------------------
src/network/bridge_driver_linux.c | 709 +++++++++++++++++++++++++++++++++
src/network/bridge_driver_noop.c | 80 ++++
src/network/bridge_driver_platform.c | 32 ++
src/network/bridge_driver_platform.h | 77 ++++
7 files changed, 915 insertions(+), 718 deletions(-)
create mode 100644 src/network/bridge_driver_linux.c
create mode 100644 src/network/bridge_driver_noop.c
create mode 100644 src/network/bridge_driver_platform.c
create mode 100644 src/network/bridge_driver_platform.h
ACK, this is pretty much the approach we discussed on IRC.
Will let Eric look at it before pushing though, in case he
disagrees with this approach.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
6:41 p.m.
On 07/04/2013 10:25 AM, Roman Bogorodskiy wrote:
* Functions were renamed: s/Iptables/Firewall/
to make names more general.
* Platform specific things (e.g. firewalling and route
collision checks) were moved into bridge_driver_platform
* Created two platform specific implementations:
- bridge_driver_linux: Linux implementation using iptables,
it's actually the code moved from bridge_driver.c
- bridge_driver_noop: dump implementation that does nothing
I've let review sit for so long that it no longer applies, and needs a
rebasing; but I'll at least skim the patch looking for potential issues
to be aware of during the rebase...
---
po/POTFILES.in | 1 +
src/Makefile.am | 5 +-
src/network/bridge_driver.c | 729 +---------------------------------
src/network/bridge_driver_linux.c | 709 +++++++++++++++++++++++++++++++++
src/network/bridge_driver_noop.c | 80 ++++
src/network/bridge_driver_platform.c | 32 ++
src/network/bridge_driver_platform.h | 77 ++++
7 files changed, 915 insertions(+), 718 deletions(-)
create mode 100644 src/network/bridge_driver_linux.c
create mode 100644 src/network/bridge_driver_noop.c
create mode 100644 src/network/bridge_driver_platform.c
create mode 100644 src/network/bridge_driver_platform.h
If you generate the email with 'git send-email/format-patch
-O/path/to/file', then you can use a file that prioritizes .h files to
come first in the diff listing; this can sometimes help in interface
reviews.
-/* Main driver state */
-struct network_driver {
You moved this struct out of a .c file into driver_platform.h; as a
result, this poor choice of naming will potentially cause problems to
more files that include the .h. Can you please do a separate cleanup
pre-patch that s/network_driver/virNetworkDriver/, and uses a typedef so
that you don't have to repeat 'struct' everywhere?
- virMutex lock;
-
- virNetworkObjList networks;
Also, it might be worth a patch to make this struct inherit from
virObjectLockable, instead of open-coding the locking ourselves. But
that's separate from your goal of splitting the files, so it's not a
prerequisite.
@@ -114,7 +99,7 @@ static int networkStartNetworkExternal(struct
network_driver *driver,
static int networkShutdownNetworkExternal(struct network_driver *driver,
virNetworkObjPtr network);
-static void networkReloadIptablesRules(struct network_driver *driver);
+static void networkReloadFirewallRules(struct network_driver *driver);
For ease of review, it might be worth splitting the function renames
into a separate patch from the code motion.
+++ b/src/network/bridge_driver_platform.c
@@ -0,0 +1,32 @@
+#include "bridge_driver_platform.h"
+
+#if defined(__linux__)
+# include "bridge_driver_linux.c"
+#else
+# include "bridge_driver_noop.c"
One .c including another is not typical, but we've done it before for
virthread.c; seems like a reasonable way to split things, as long as all
branches of the file declare the same functions.
The other alternative for splitting things is the way we've done it in
src/security - the public interface is in virSecurityManager, and
basically forwards all calls into a private interface, where each
backend registers a struct of callback pointers. That method is a bit
more robust - you can add a callback to one driver without having to
remember to add it to all drivers simultaneously. With your approach,
if we add a new function in bridge_driver_linux.c, we MUST remember to
add the same function in bridge_driver_noop.c, or it will break
compilation on non-Linux, but developers that don't use Linux will be
unaware of the breakage. With the callback approach, not adding a
callback to bridge_driver_noop.c is not fatal; the manager wrapper would
simply know to do nothing for a NULL callback.
So, although I like the split, I can't help but wonder if your rebase
should take the road of adjusting things to use a callback struct,
rather than requiring matching implementations across multiple files.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
4:23 a.m.
On Wed, Jul 17, 2013 at 05:41:56PM -0600, Eric Blake wrote:
> +++ b/src/network/bridge_driver_platform.c
> @@ -0,0 +1,32 @@
> +#include "bridge_driver_platform.h"
> +
> +#if defined(__linux__)
> +# include "bridge_driver_linux.c"
> +#else
> +# include "bridge_driver_noop.c"
One .c including another is not typical, but we've done it before for
virthread.c; seems like a reasonable way to split things, as long as all
branches of the file declare the same functions.
The other alternative for splitting things is the way we've done it in
src/security - the public interface is in virSecurityManager, and
basically forwards all calls into a private interface, where each
backend registers a struct of callback pointers. That method is a bit
more robust - you can add a callback to one driver without having to
remember to add it to all drivers simultaneously. With your approach,
if we add a new function in bridge_driver_linux.c, we MUST remember to
add the same function in bridge_driver_noop.c, or it will break
compilation on non-Linux, but developers that don't use Linux will be
unaware of the breakage. With the callback approach, not adding a
callback to bridge_driver_noop.c is not fatal; the manager wrapper would
simply know to do nothing for a NULL callback.
So, although I like the split, I can't help but wonder if your rebase
should take the road of adjusting things to use a callback struct,
rather than requiring matching implementations across multiple files.
I think the callback struct approach is useful for the case where you
have multiple possible implementations compiled in at once and we need
to choose them dynamically. Here though, we're making the choice at
compile time, so I think callback + dynamic dispatch is somewhat overkill.
Following of the virthread approach of #include'ing .c file was my
suggestion to Roman for how to structure this.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
8:07 a.m.
On 07/18/2013 03:23 AM, Daniel P. Berrange wrote:
On Wed, Jul 17, 2013 at 05:41:56PM -0600, Eric Blake wrote:
> So, although I like the split, I can't help but wonder if your rebase
> should take the road of adjusting things to use a callback struct,
> rather than requiring matching implementations across multiple files.
I think the callback struct approach is useful for the case where you
have multiple possible implementations compiled in at once and we need
to choose them dynamically. Here though, we're making the choice at
compile time, so I think callback + dynamic dispatch is somewhat overkill.
Following of the virthread approach of #include'ing .c file was my
suggestion to Roman for how to structure this.
Fair enough. It just means that we don't have the compiler helping us
remember to add both functions at once, so it raises the probability
that someone will add a Linux-only function and forget to add the noop
counterpart. But as long as we detect it before a release (that's what
release candidate testing is for, right?) we should be okay.
Roman, I'll accept your code with the .c inclusion, so you don't have to
do the extra work of a dynamic dispatch rewrite; now it's just waiting
for a rebased patch.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
6:32 a.m.
Eric Blake wrote:
On 07/18/2013 03:23 AM, Daniel P. Berrange wrote:
> On Wed, Jul 17, 2013 at 05:41:56PM -0600, Eric Blake wrote:
>> So, although I like the split, I can't help but wonder if your rebase
>> should take the road of adjusting things to use a callback struct,
>> rather than requiring matching implementations across multiple files.
>
> I think the callback struct approach is useful for the case where you
> have multiple possible implementations compiled in at once and we need
> to choose them dynamically. Here though, we're making the choice at
> compile time, so I think callback + dynamic dispatch is somewhat overkill.
>
> Following of the virthread approach of #include'ing .c file was my
> suggestion to Roman for how to structure this.
Fair enough. It just means that we don't have the compiler helping us
remember to add both functions at once, so it raises the probability
that someone will add a Linux-only function and forget to add the noop
counterpart. But as long as we detect it before a release (that's what
release candidate testing is for, right?) we should be okay.
Roman, I'll accept your code with the .c inclusion, so you don't have to
do the extra work of a dynamic dispatch rewrite; now it's just waiting
for a rebased patch.
That sounds good, thanks.
I have started with renaming of 'struct network_driver' (patch is
already on the list). When done with this patch, I'll do 'Iptables' ->
'Firewall' rename and then finally will rebase this split patch.
Roman Bogorodskiy
6:26 p.m.
On 07/04/2013 10:25 AM, Roman Bogorodskiy wrote:
This is a second step for making bridge_driver more portable.
This finally percolated to the top of my todo list; I'll have a review
shortly.
This one splits up bridge driver into platform specific and general
parts. Platform specifc parts are mostly firewalling stuff.
So the support for new platforms should be added by implementing
bridge_driver_$platform.c
However, you might notice I didn't move some ports of the
platform specific code. E.g. I left networkEnableIpForwarding
in bridge_driver.c because it feels like the implementation for
e.g. all BSDs would be the same (while firewalling stuff would
be different on BSDs) to avoid code copy/paste.
Roman Bogorodskiy (1):
Split up platfrom specifics from bridge driver
po/POTFILES.in | 1 +
src/Makefile.am | 5 +-
src/network/bridge_driver.c | 729 +---------------------------------
src/network/bridge_driver_linux.c | 709 +++++++++++++++++++++++++++++++++
src/network/bridge_driver_noop.c | 80 ++++
src/network/bridge_driver_platform.c | 32 ++
src/network/bridge_driver_platform.h | 77 ++++
7 files changed, 915 insertions(+), 718 deletions(-)
Looks big, but the bulk of it is code motion, so it should be reasonable.
create mode 100644 src/network/bridge_driver_linux.c
create mode 100644 src/network/bridge_driver_noop.c
create mode 100644 src/network/bridge_driver_platform.c
create mode 100644 src/network/bridge_driver_platform.h
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
4141
days inactive
4159
days old
7 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Roman Bogorodskiy