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

[Michal Privoznik]

On 30.01.2014 11:26, Laine Stump wrote:
> On 01/27/2014 06:08 PM, Michal Privoznik wrote:
>> 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 :)
>
> Indeed :-) I really should get myself Cc'ed on more bugzilla
> copmonents/products so that I notice these things sooner.
>
>> 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.
>
> Since you've reverted yours, should I push this?
>
> After that, we may want to talk about 1) supporting use of the "dev"
> attribute in <forward> to name a *single* forwarding interface, and
> applying a network's <bandwidth> to that interface (while still failing
> in other cases), and 2) maybe supporting Open vSwitch in a more thorough
> manner so that projects like ovirt can use it to create intermediate
> bridges and manage their bandwidth via libvirt <network> xml.
>

Yes. Please do push your patch. ACK.

Michal