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

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