Re: [libvirt] [PATCH 1/4] network: don't refresh iptables rules on networks without them
On 09/21/2012 04:04 PM, Eric Blake wrote:
On 09/21/2012 01:46 PM, Laine Stump wrote:
> The bridge driver implementation of virNetworkUpdate() removes and
> re-adds iptables rules any time a network has an <ip>, <forward>, or
> <forward>/<interface> elements updated. There are some types of
> networks that have those elements and yet have no iptables rules
> associated with them, and unfortunately the functions that remove/add
> iptables rules don't check the type of network before attempting to
> remove/add the rules.
>
> Under normal circumstances I would refactor the lower level functions
> to be more robust, but to avoid code churn as much as possible, I've
> just added extra checks directly to networkUpdate().
> ---
> src/network/bridge_driver.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index fce1739..6e260f7 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -2945,9 +2945,12 @@ networkUpdate(virNetworkPtr net,
> goto cleanup;
> }
>
> - if (section == VIR_NETWORK_SECTION_IP ||
> - section == VIR_NETWORK_SECTION_FORWARD ||
> - section == VIR_NETWORK_SECTION_FORWARD_INTERFACE) {
> + if ((section == VIR_NETWORK_SECTION_IP ||
> + section == VIR_NETWORK_SECTION_FORWARD ||
> + section == VIR_NETWORK_SECTION_FORWARD_INTERFACE) &&
> + (network->def->forwardType == VIR_NETWORK_FORWARD_NONE ||
Is network->def->forwardType something that can be changed by
networkUpdate()?
Currently it's locked down, because the backend that can do that hasn't
been implemented yet.
If so,
> + network->def->forwardType == VIR_NETWORK_FORWARD_NAT ||
> + network->def->forwardType == VIR_NETWORK_FORWARD_ROUTE)) {
> /* these could affect the iptables rules */
> networkRemoveIptablesRules(driver, network);
> if (networkAddIptablesRules(driver, network) < 0)
then it seems like you'd have to check the old type before
networkRemoveIptablesRules, and the new type before
networkAddIptablesRules.
Beyond that, we would have to keep the original networkdef in place
until after networkRemoveiptablesRules was finished, since it uses
things from there. And not just when moving from one type of network to
another, but also when adding/removing IP addresses.
But if the forward type is always locked down
once the network is started (where changing types requires destroying
and restarting the network, rather than an on-the-fly update), then this
makes sense. Conditional ACK.
Currently it is locked down. As a matter of fact, the section that is
triggering this code is the update of <forward>/<interface>, which is
almost never useful unless you're in one of the modes that doesn't setup
iptable rules anyway. There's just a seldom-used corner case where the
dev attribute of <forward> could also be used to limit egress traffic to
a single physical interface, and if that was being used, the likelyhood
they would want to change it live is much lower than the chance that
someone would want to add another <interface> to a pool used for one of
the "non-iptables-using" modes.
So, the conclusion is that I think it's safe for now to not worry about
that, but when I add support for updating <ip> and <forward>, then we'll
need to deal with it.