Re: [libvirt] [PATCH 6/9] network: setup bridge devices for promiscLinks='no'

On 11/24/2014 12:48 PM, Laine Stump wrote: > When the bridge device for a network has promiscLinks='no', the intent > is to setup the bridge to opportunistically turn off promiscuous mode > and/or flood mode for as many ports/links on the bridge as possible, > thus improving performance and security. The setup for the bridge > itself is: > > 1) set the "vlan_filtering" property of the bridge device to 1. 2) If > the bridge has a "Dummy" tap device used to set a fixed MAC address on > the bridge, turn off learning and unicast_flood on this tap (this is > needed even though this tap is never IFF_UP, because the kernel > ignores the IFF_UP flag of other devices when using their settings to > automatically decide whether or not to turn off promiscuous mode for > any attached device). > > This is done both for libvirt-created/managed bridges, and for bridges > that are created by the host system config. > > There is no attempt to turn vlan_filtering off when destroying the > network because in the case of a libvirt-created bridge, the bridge is > about to be destroyed anyway, and in the case of a system bridge, if > the other devices attached to the bridge could operate properly before > destroying libvirt's network object, they will continue to operate > properly (this is similar to the way that libvirt will enable > ip_forwarding whenever a routed/natted network is started, but will > never attempt to disable it if they are stopped). > --- > src/network/bridge_driver.c | 53 +++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 53 insertions(+) > > diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c > index bc8da79..f2564c3 100644 > --- a/src/network/bridge_driver.c > +++ b/src/network/bridge_driver.c > @@ -1893,6 +1893,28 @@ networkAddAddrToBridge(virNetworkObjPtr network, > return 0; > } > > + > +static int > +networkStartHandlePromiscLinks(virNetworkObjPtr network, const char *macTapIfName) > +{ > + const char *brname = network->def->bridge; > + > + /* promiscuous mode won't be turned off unless vlan filtering is enabled. */ > + if (brname && > + network->def->promiscLinks == VIR_TRISTATE_BOOL_NO) { > + if (virNetDevBridgeSetVlanFiltering(brname, true) < 0) Could this return failure if we don't have the right kernel? Then do we really want to kill the whole startup processing?

It almost seems as if the failure case here should be some kind of VIR_WARN or VIR_INFO, return 0 and do nothing. I guess what I'm most worried about is the "future" if the decision is to quietly change the default of 'promiscLinks' (or whatever name is used) and we get here from networkStartNetworkBridge which in a libvirtd restart has what effect on something that was running before?

You would be correct in pointing out that for the current design and assumptions this is the right course of action. Someone set promisc=no and there was a failure, so we need to fail too. > + return -1; > + if (macTapIfName) { > + if (virNetDevBridgePortSetLearning(brname, macTapIfName, false) < 0) > + return -1; > + if (virNetDevBridgePortSetUnicastFlood(brname, macTapIfName, false) < 0) > + return -1; Ah - although networkStartNetworkBridge says to restore things on failure, since we'll be deleting the bridge if either of these fail so it doesn't matter.

The BOOL_NO logic could change if you took my suggestion from patch3 ACK, but this is ultimately where changing the default would be affected and perhaps we need to somehow note that.