Re: [libvirt] [PATCH v3 2/2] network_conf: check if bridge exists on host for user created bridges

On 03/24/2015 11:12 AM, Laine Stump wrote: > On 03/24/2015 05:59 AM, Shivaprasad G Bhat 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(a)linux.vnet.ibm.com&gt; >> --- >> 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 > 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).