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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]