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 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list