[libvirt] [PATCHv2 6/9] network: setup bridge devices for fdb='managed'

[John Ferlan]

On 12/02/2014 12:08 PM, Laine Stump wrote:
> When the bridge device for a network has fdb='managed' the intent is
> to configure 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 required 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).
> ---
> 
> Changes from V1: only in attribute/value names
> 
>  src/network/bridge_driver.c | 52 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 52 insertions(+)
> 

ACK still applies given naming
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 3299cd6..9b3dc58 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -1912,6 +1912,27 @@ networkAddAddrToBridge(virNetworkObjPtr network,
>      return 0;
>  }
>  
> +
> +static int
> +networkStartHandleFDBMode(virNetworkObjPtr network, const char *macTapIfName)
> +{
> +    const char *brname = network->def->bridge;
> +
> +    if (brname &&
> +        network->def->fdb == VIR_NETWORK_BRIDGE_FDB_MANAGED) {
> +        if (virNetDevBridgeSetVlanFiltering(brname, true) < 0)
> +            return -1;

Given this particular path where we're being managed, we set
vlan_filtering, but macTapIfName == NULL (for who knows what reason),
then we have the condition where we've set vlan_filtering, but not
learning/flood. Thus, it seems plausible that a filterVLAN='yes|no'
(from my comments in .0) might be possible.

So, if that's not a desirable state, then perhaps macTapIfName needs to
be in the outer if, leaving:

if (virNetDevBridgeSetVlanFiltering(brname, true) < 0 ||
    virNetDevBridgePortSetLearning(brname, macTapIfName, false) < 0 ||
    virNetDevBridgePortSetUnicastFlood(brname, macTapIfName, false) < 0)
    return -1;

If you were to use the .0 suggestion to have filterVLAN, then you'd have
to decide whether forcing filterVLAN on even though it was perhaps set
to 'no' in the XML would be something that 'should' be done...


ACK with this resolved and given current names....

John

> +        if (macTapIfName) {
> +            if (virNetDevBridgePortSetLearning(brname, macTapIfName, false) < 0)
> +                return -1;
> +            if (virNetDevBridgePortSetUnicastFlood(brname, macTapIfName, false) < 0)
> +                return -1;
> +        }
> +    }
> +    return 0;
> +}
> +
> +
>  /* add an IP (static) route to a bridge */
>  static int
>  networkAddRouteToBridge(virNetworkObjPtr network,
> @@ -2038,6 +2059,9 @@ networkStartNetworkVirtual(virNetworkObjPtr network)
>              goto err2;
>      }
>  
> +    if (networkStartHandleFDBMode(network, macTapIfName) < 0)
> +        goto err2;
> +
>      /* Bring up the bridge interface */
>      if (virNetDevSetOnline(network->def->bridge, 1) < 0)
>          goto err2;
> @@ -2182,6 +2206,27 @@ static int networkShutdownNetworkVirtual(virNetworkObjPtr network)
>  }
>  
>  
> +static int
> +networkStartNetworkBridge(virNetworkObjPtr network)
> +{
> +    /* put anything here that needs to be done each time a network of
> +     * type BRIDGE, is started. On failure, undo anything you've done,
> +     * and return -1. On success return 0.
> +     */
> +    return networkStartHandleFDBMode(network, NULL);
> +}
> +
> +static int
> +networkShutdownNetworkBridge(virNetworkObjPtr network ATTRIBUTE_UNUSED)
> +{
> +    /* put anything here that needs to be done each time a network of
> +     * type BRIDGE is shutdown. On failure, undo anything you've done,
> +     * and return -1. On success return 0.
> +     */
> +    return 0;
> +}
> +
> +
>  /* networkCreateInterfacePool:
>   * @netdef: the original NetDef from the network
>   *
> @@ -2345,6 +2390,10 @@ networkStartNetwork(virNetworkObjPtr network)
>          break;
>  
>      case VIR_NETWORK_FORWARD_BRIDGE:
> +       if (networkStartNetworkBridge(network) < 0)
> +          goto cleanup;
> +       break;
> +
>      case VIR_NETWORK_FORWARD_PRIVATE:
>      case VIR_NETWORK_FORWARD_VEPA:
>      case VIR_NETWORK_FORWARD_PASSTHROUGH:
> @@ -2411,6 +2460,9 @@ static int networkShutdownNetwork(virNetworkObjPtr network)
>          break;
>  
>      case VIR_NETWORK_FORWARD_BRIDGE:
> +        ret = networkShutdownNetworkBridge(network);
> +        break;
> +
>      case VIR_NETWORK_FORWARD_PRIVATE:
>      case VIR_NETWORK_FORWARD_VEPA:
>      case VIR_NETWORK_FORWARD_PASSTHROUGH:
>