On 03/24/2015 11:12 AM, Laine Stump wrote:
virNetworkBridgeInUse() doesn't check if the bridge is manually created outside of libvirt. Check if the bridge actually exist on host using the virNetDevExists().
Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com> --- src/conf/network_conf.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index d7c5dec..c3ae2e4 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -3227,13 +3227,22 @@ virNetworkBridgeInUseHelper(const void *payload, int ret = 0; virNetworkObjPtr net = (virNetworkObjPtr) payload; const struct virNetworkBridgeInUseHelperData *data = opaque; + bool defined_bridge = false;
virObjectLock(net); if (net->def->bridge && - STREQ(net->def->bridge, data->bridge) && - !(data->skipname && STREQ(net->def->name, data->skipname))) - ret = 1; + STREQ(net->def->bridge, data->bridge)) { + defined_bridge = true; + if (!(data->skipname && STREQ(net->def->name, data->skipname))) + ret = 1; + } + virObjectUnlock(net); + + /* Bridge might have been created by a user manually outside libvirt */ + if (!defined_bridge && !ret) + ret = virNetDevExists(data->bridge) ? 1 : 0; + return ret; } This function is intended to be called once for each defined network on
On 03/24/2015 05:59 AM, Shivaprasad G Bhat wrote: the host, with data->bridge being the same each time, but net->def->bridge being different, so adding the check for data->bridge existence here may work, but it's a bit convoluted.
Instead, you should leave this function alone, and just add the extra check directly in the function virNetworkBridgeInUse() (either before locking, or after unlocking "nets").
You know, there's another problem with this - adding this call to virNetDevExists() would be the first case of anything in the conf directory (which is supposed to be platform-independent parsing/formatting of XML and maintenance of lists of config objects) that calls a virNetDev*() function which is dependent on the current platform (and having root privileges). For the current uses of virNetworkBridgeInUse() it ends up not being a problem, because the only caller of virNetworkBridgeInUse() will already be verified as running on a platform that supports the virNetDev* functions and having root privileges (and/or certain to fail if we made this call on return), but it still leaves a bad taste in my mouth to be calling a device ioctl from a function in the conf directory. The trouble of course is that doing it differently turns this into a much bigger problem - as it is network_conf.c mostly isolates the network driver (bridge_driver.c) from the idea that a network could be defined without a bridge name specified, or that there might be a conflicting bridge name, by calling virNetworkSetBridgeName() which is in network_conf.c (it also calls virNetworkSetBridgeName() on its own when loading the network configs from disk, thus not even giving the network driver a chance to intervene). So I don't want to be the one to NACK based on this cross pollination, but thought I should point it out in case anyone with a stronger opinion did (and even if not, so that we see that even if we accept a patch like the current one to fix the bug now, we may still want to plan a different fix in the long run. Or maybe not - maybe I'm being too pedantic).