[libvirt] [PATCH] Account for defined networks when generating bridge names

[Daniel P. Berrange]

On Fri, Feb 27, 2009 at 10:57:35AM -0500, Cole Robinson wrote:
> Daniel P. Berrange wrote:
> > On Mon, Feb 16, 2009 at 06:40:59PM -0500, Cole Robinson wrote:
> >> diff --git a/src/network_driver.c b/src/network_driver.c
> >> index d750565..d83f902 100644
> >> --- a/src/network_driver.c
> >> +++ b/src/network_driver.c
> >> @@ -812,7 +812,12 @@ static int networkStartNetworkDaemon(virConnectPtr conn,
> >>          return -1;
> >>      }
> >>  
> >> -    if ((err = brAddBridge(driver->brctl, &network->def->bridge))) {
> >> +    if (!network->def->bridge &&
> >> +        !(network->def->bridge = virNetworkAllocateBridge(conn,
> >> +                                                          &driver->networks)))
> >> +        return -1;
> >> +
> >> +    if ((err = brAddBridge(driver->brctl, network->def->bridge))) {
> > 
> > This will cause a thread deadlock once you add the locking I describe
> > for virNetworkBridgeInUse() in the previous patch. This is because
> > the current virNetworkObjPtr 'network'  here will be locked, then
> > the function you're calling with then try to lock it again. 
> > 
> > A deep called function like networkStartNetworkDaemon() shouldn't be
> > iterating over all network objects, so this is the wrong place to try
> > and fix this problem.
> > 
> > I'm guessing you're trying to fix up existing defined networks without
> > a bridge here, so IMHO, this is better done at daemon startup, where
> > we load all the configs off disk. This will avoid the locking trouble
> > 
> > Do it in 'networkStartup',m just after the virNetworkLoadAllConfigs
> > call, but before autostart is done.
> > 
> 
> Okay, I rolled these changes and the BridgeInUse changes into one patch
> (along with Jim's suggestions).
> 
> I added a helper function SetBridgeName which basically does:
> 
> if user passed bridge name:
>   verify it doesn't collide
> else:
>   generate bridge name
> 
> We call this in Define and CreateXML. When loading configs from a driver
> restart, we only attempt to generate a bridge: if checking for
> collisions failed, the network wouldn't even be defined, which would
> only cause more pain for users.

ACK, this looks safe now.

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|