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

Shivaprasad bhat shivaprasadbhat at gmail.com
Wed Mar 25 13:18:44 UTC 2015


On Tue, Mar 24, 2015 at 9:18 PM, Laine Stump <laine at laine.org> wrote:
> 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 at 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
>> 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").

Ok. Will do as suggested in the next version.

>
> 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).

Hmm.. I too am not sure whats best here.

Is it better to move the function call virSetBridgeName
from virNetworkLoadConfig() to networkAutostartConfig() ? and
virNetworkSetBridgeName, virNetworkBridgeInUse, virNetworkAllocateBridge
and virNetworkBridgeInUseHelper function definitions to bridge_driver.c ?
That way the calls will be from the driver only instead of conf.




More information about the libvir-list mailing list