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@xxxxxxxxxxxxxxxxxx>
>> ---
>> 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).
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list