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

[Daniel P. Berrange]

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.

> @@ -1147,11 +1152,18 @@ static virNetworkPtr networkDefine(virConnectPtr conn, const char *xml) {
>      if (!(def = virNetworkDefParseString(conn, xml)))
>          goto cleanup;
>  
> -    if (def->bridge &&
> -        virNetworkBridgeInUse(&driver->networks, def->bridge)) {
> -        networkReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
> -                           _("bridge name '%s' already in use."), def->bridge);
> -        goto cleanup;
> +    if (def->bridge) {
> +        if (virNetworkBridgeInUse(&driver->networks, def->bridge)) {
> +            networkReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
> +                               _("bridge name '%s' already in use."),
> +                               def->bridge);
> +            goto cleanup;
> +        }
> +    } else {
> +        /* Allocate a bridge name */
> +        if (!(def->bridge = virNetworkAllocateBridge(conn,
> +                                                     &driver->networks)))
> +            goto cleanup;
>      }

This bit is OK from a locking POV, since its at the top level entry
point.

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 :|