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

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.