Re: [libvirt] [PATCH 3/2] network: check newDef for used bridge names in addition to def
On 04/27/2015 05:54 AM, Shivaprasad bhat wrote:
On Sat, Apr 25, 2015 at 12:14 AM, Laine Stump <laine(a)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?