[libvirt] [PATCH] network: disallow <bandwidth>/<mac> for bridged/macvtap networks

[Michal Privoznik]

On 24.01.2014 13:18, Laine Stump wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1057321 pointed out that
> we weren't honoring the <bandwidth> element in libvirt networks using
> <forward mode='bridge'/>. In fact, these networks are just a method of
> giving a libvirt network name to an existing Linux host bridge on the
> system, and even if it were technically possible for us to set
> network-wide bandwidth limits for all the taps on a bridge, it's
> probably not a polite thing to do since libvirt is just using a bridge
> that was created by someone else for other purposes. So the proper
> thing is to just log an error when someone tries to put a <bandwidth>
> element in that type of network.
>
> While looking through the network XML documentation and comparing it
> to the networkValidate function, I noticed that we also ignore the
> presence of a mac address in the config, even though we do nothing
> with it in this case either.
>
> This patch updates networkValidate() (which is called any time a
> persistent network is defined, or a transient network created) to log
> an error and fail if it finds either a <bandwidth> or <mac> element
> and the network forward mode is anything except 'route'. 'nat', or
> nothing. (Yes, neither of those elements is acceptable for any macvtap
> mode, nor for a hostdev network).
>
> NB: This does *not* cause failure to start any existing network that
> contains one of those elements, so someone might have erroneously
> defined such a network in the past, and that network will continue to
> function unmodified. I considered it too disruptive to suddenly break
> working configs on the next reboot after a libvirt upgrade.
> ---
>   src/network/bridge_driver.c | 19 ++++++++++++++++++-
>   1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 0b43a67..3b9b58d 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -2407,8 +2407,17 @@ networkValidate(virNetworkDriverStatePtr driver,
>           virNetworkSetBridgeMacAddr(def);
>       } else {
>           /* They are also the only types that currently support setting
> -         * an IP address for the host-side device (bridge)
> +         * a MAC or IP address for the host-side device (bridge), DNS
> +         * configuration, or network-wide bandwidth limits.
>            */
> +        if (def->mac_specified) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                           _("Unsupported <mac> element in network %s "
> +                             "with forward mode='%s'"),
> +                           def->name,
> +                           virNetworkForwardTypeToString(def->forward.type));
> +            return -1;
> +        }
>           if (virNetworkDefGetIpByIndex(def, AF_UNSPEC, 0)) {
>               virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>                              _("Unsupported <ip> element in network %s "
> @@ -2433,6 +2442,14 @@ networkValidate(virNetworkDriverStatePtr driver,
>                              virNetworkForwardTypeToString(def->forward.type));
>               return -1;
>           }
> +        if (def->bandwidth) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                           _("Unsupported network-wide <bandwidth> element "
> +                             "in network %s with forward mode='%s'"),
> +                           def->name,
> +                           virNetworkForwardTypeToString(def->forward.type));
> +            return -1;
> +        }
>       }
>
>       /* We only support dhcp on one IPv4 address and
>

I think think this is exactly the opposite of what I've just pushed :)
I mean:

commit 2996e6be19a13199ded7c2aa21039cca97318e01
Author:     Michal Privoznik <mprivozn at redhat.com>
AuthorDate: Wed Jan 22 18:58:33 2014 +0100
Commit:     Michal Privoznik <mprivozn at redhat.com>
CommitDate: Mon Jan 27 12:11:27 2014 +0100

     networkAllocateActualDevice: Set QoS for bridgeless networks too

     https://bugzilla.redhat.com/show_bug.cgi?id=1055484



In the commit I'm trying to inherit network QoS to the interface that is 
just being created. Yes, it involves some magic but it works. I guess we 
need to agree if we want this approach or mine as they seem to be 
contradictionary.

Michal