[libvirt] [PATCH] Account for defined networks when generating bridge names
This is the second part of the fix for rhbz 479622. If we are generating a bridge name for a virtual network, don't collide with any bridge name in a defined network. This patch also generates a bridge name at network define time, if none was passed in the xml. The downside to all this is that it won't fix things for existing victims of the bug: if they have 2 networks with the same bridge device in the xml, we can't intelligently remedy the situation. This patch just helps prevent future users getting into that predicament. Thanks, Cole
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 :|
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. Patch below. Thanks, Cole
Cole Robinson wrote: ...
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. ...
Hi Cole, Here's a quick review: One nit below, but what I'd really like is a stand-alone virsh-oriented way to exercise a few of these new code paths (tests), just to make sure we get coverage when running "make check". If you can outline something like that, I'll massage it into a new test.
Generate network bridge names if none passed at define/create time.
...
diff --git a/src/network_conf.c b/src/network_conf.c index 6ad0d01..5de164e 100644 --- a/src/network_conf.c +++ b/src/network_conf.c @@ -43,6 +43,7 @@ #include "buf.h" #include "c-ctype.h"
+#define MAX_BRIDGE_ID 256 ... +char *virNetworkAllocateBridge(virConnectPtr conn, + const virNetworkObjListPtr nets) +{ + + int id = 0; + char *newname; + + do { + char try[50]; + + snprintf(try, sizeof(try), "virbr%d", id); + + if (!virNetworkBridgeInUse(nets, try, NULL)) { + if (!(newname = strdup(try))) { + virReportOOMError(conn); + return NULL; + } + return newname; + } + + id++; + } while (id < MAX_BRIDGE_ID);
This should probably be <= MAX_BRIDGE_ID, or change MAX_BRIDGE_ID to 255, .. so that the diagnostic below makes sense.
+ virNetworkReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Bridge generation exceeded max id %d"), + MAX_BRIDGE_ID); + return NULL; +}
Jim Meyering wrote:
Cole Robinson wrote: ...
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. ...
Hi Cole,
Here's a quick review: One nit below, but what I'd really like is a stand-alone virsh-oriented way to exercise a few of these new code paths (tests), just to make sure we get coverage when running "make check". If you can outline something like that, I'll massage it into a new test.
There's a few pieces: Define a virtual network without a 'bridge=' attribute, then do something like 'virsh net-dumpxml netname | grep virbr'. Previously nothing would be shown, now it should match the generated bridge name. Define a virtual network with an already in use bridge (simplest to use would be virbr0 since the 'default' network uses it.) Now it will rightfully error, previously no complaints. The same for 'virsh create' (though this would fail previously if the colliding network was running, but with an unclear error message). Unfortunately none of this will work for the test driver, since all these changes were specific to the actual network driver. It would have been nice to do the collision detections in the xml parsing routines but I think it breaks the API separation too much.
Generate network bridge names if none passed at define/create time.
...
diff --git a/src/network_conf.c b/src/network_conf.c index 6ad0d01..5de164e 100644 --- a/src/network_conf.c +++ b/src/network_conf.c @@ -43,6 +43,7 @@ #include "buf.h" #include "c-ctype.h"
+#define MAX_BRIDGE_ID 256 ... +char *virNetworkAllocateBridge(virConnectPtr conn, + const virNetworkObjListPtr nets) +{ + + int id = 0; + char *newname; + + do { + char try[50]; + + snprintf(try, sizeof(try), "virbr%d", id); + + if (!virNetworkBridgeInUse(nets, try, NULL)) { + if (!(newname = strdup(try))) { + virReportOOMError(conn); + return NULL; + } + return newname; + } + + id++; + } while (id < MAX_BRIDGE_ID);
This should probably be <= MAX_BRIDGE_ID, or change MAX_BRIDGE_ID to 255, .. so that the diagnostic below makes sense.
Ahh! I forgot about this change and already committed the patch. I'll commit this as an incremental fix. Thanks for the review. - Cole
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 :|
Cole Robinson <crobinso@redhat.com> wrote:
This is the second part of the fix for rhbz 479622. If we are generating a bridge name for a virtual network, don't collide with any bridge name in a defined network. This patch also generates a bridge name at network define time, if none was passed in the xml.
The downside to all this is that it won't fix things for existing victims of the bug: if they have 2 networks with the same bridge device in the xml, we can't intelligently remedy the situation. This patch just helps prevent future users getting into that predicament.
Thanks, Cole
diff --git a/src/bridge.c b/src/bridge.c index fc11429..4446a95 100644 --- a/src/bridge.c +++ b/src/bridge.c @@ -49,7 +49,7 @@ #include "util.h" #include "logging.h"
-#define MAX_BRIDGE_ID 256 +#define MAX_TAP_ID 256
#define JIFFIES_TO_MS(j) (((j)*1000)/HZ) #define MS_TO_JIFFIES(ms) (((ms)*HZ)/1000) @@ -127,32 +127,13 @@ brShutdown(brControl *ctl) #ifdef SIOCBRADDBR int brAddBridge(brControl *ctl, - char **name) + char *name)
Hi Cole, Presuming this part will stay, as you rework the patch, please make that "const". and maybe save a line of vertical space: brAddBridge(brControl *ctl, const char *name)
participants (3)
-
Cole Robinson -
Daniel P. Berrange -
Jim Meyering