[libvirt] [PATCH 3/2] network: check newDef for used bridge names in addition to def

[Shivaprasad bhat]

On Mon, Apr 27, 2015 at 11:20 PM, Laine Stump <laine at laine.org> wrote:
> On 04/27/2015 05:54 AM, Shivaprasad bhat wrote:
>> On Sat, Apr 25, 2015 at 12:14 AM, Laine Stump <laine at laine.org> wrote:
>>> If someone has updated a network to change its bridge name, but the
>>> network is still active (so that bridge name hasn't taken effect yet),
>>> we still want to disallow another network from taking that new name.
>>> ---
>>> As suggested by Shivaprasad following his tests of patches 1 and 2...
>>>
>>> src/conf/network_conf.c | 15 +++++++++++----
>>>  1 file changed, 11 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
>>> index aa8d6c6..5b734f2 100644
>>> --- a/src/conf/network_conf.c
>>> +++ b/src/conf/network_conf.c
>>> @@ -3270,15 +3270,22 @@ virNetworkBridgeInUseHelper(const void *payload,
>>>                              const void *name ATTRIBUTE_UNUSED,
>>>                              const void *opaque)
>>>  {
>>> -    int ret = 0;
>>> +    int ret;
>>>      virNetworkObjPtr net = (virNetworkObjPtr) payload;
>>>      const struct virNetworkBridgeInUseHelperData *data = opaque;
>>>
>>>      virObjectLock(net);
>>> -    if (net->def->bridge &&
>>> -        STREQ(net->def->bridge, data->bridge) &&
>>> -        !(data->skipname && STREQ(net->def->name, data->skipname)))
>>> +    if (data->skipname &&
>>> +        ((net->def && STREQ(net->def->name, data->skipname)) ||
>>> +         (net->newDef && STREQ(net->newDef->name, data->skipname))))
>> Should the newDef->name be really be checked? The network cant be renamed.
>> Right ?
>
> Right now it can't be renamed, but I recall some discussion about
> possibly making that possible in the future. Adding this check doesn't
> hurt anything now, and may prevent confusion later.
>
> (more thinking about it - right now, we know that newDef->name and
> def->name will always match, so that clause is a NOP. If we ever do
> allow renaming a network, this clause would come into play if someone
> had already edited the network once (so that newDef was populated), then
> wanted to edit it a 2nd time before enacting that edited version; in
> this case we *would* want to allow the same bridge device name as was
> used in newDef, so what's being done here would be proper).
>
> (Actually this has me thinking that maybe the whole idea of skipping a
> particular *name* here is incorrect. Maybe we should be skipping a
> particular *uuid* (since that's the thing that's guaranteed to never
> change).
>
> Anyway, I think for now this patch can stand as-is. Are you convinced?

Thanks for the explanation Laine. Given possible future plans, I agree it makes
sense to have this check for now. ACK