On Tue, Mar 24, 2015 at 9:18 PM, Laine Stump <laine@xxxxxxxxx> 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@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").
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.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list