5:40 p.m.
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
5:32 a.m.
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 :|
9:57 a.m.
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
8:41 a.m.
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;
+}
11:50 a.m.
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
11:23 a.m.
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 :|
3:12 a.m.
Cole Robinson <crobinso(a)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)
5743
days inactive
5757
days old
6 comments
3 participants
participants (3)
-
Cole Robinson -
Daniel P. Berrange -
Jim Meyering