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
Generate network bridge names if none passed at define/create time.
diff --git a/src/bridge.c b/src/bridge.c
index 668dcf0..46dc407 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)
+ const char *name)
{
if (!ctl || !ctl->fd || !name)
return EINVAL;
- if (*name) {
- if (ioctl(ctl->fd, SIOCBRADDBR, *name) == 0)
- return 0;
- } else {
- int id = 0;
- do {
- char try[50];
-
- snprintf(try, sizeof(try), "virbr%d", id);
-
- if (ioctl(ctl->fd, SIOCBRADDBR, try) == 0) {
- if (!(*name = strdup(try))) {
- ioctl(ctl->fd, SIOCBRDELBR, name);
- return ENOMEM;
- }
- return 0;
- }
-
- id++;
- } while (id < MAX_BRIDGE_ID);
- }
+ if (ioctl(ctl->fd, SIOCBRADDBR, name) == 0)
+ return 0;
return errno;
}
@@ -547,7 +528,7 @@ brAddTap(brControl *ctl,
}
id++;
- } while (subst && id <= MAX_BRIDGE_ID);
+ } while (subst && id <= MAX_TAP_ID);
error:
close(fd);
diff --git a/src/bridge.h b/src/bridge.h
index f37ab72..e06ff41 100644
--- a/src/bridge.h
+++ b/src/bridge.h
@@ -47,7 +47,7 @@ int brInit (brControl **ctl);
void brShutdown (brControl *ctl);
int brAddBridge (brControl *ctl,
- char **name);
+ const char *name);
int brDeleteBridge (brControl *ctl,
const char *name);
int brHasBridge (brControl *ctl,
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index c38e142..b8cca00 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -206,6 +206,7 @@ virNetworkObjListFree;
virNetworkDefParseNode;
virNetworkRemoveInactive;
virNetworkSaveConfig;
+virNetworkSetBridgeName;
virNetworkObjLock;
virNetworkObjUnlock;
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
#define VIR_FROM_THIS VIR_FROM_NETWORK
VIR_ENUM_DECL(virNetworkForward)
@@ -743,6 +744,12 @@ virNetworkObjPtr virNetworkLoadConfig(virConnectPtr conn,
goto error;
}
+ /* Generate a bridge if none is found, but don't check for collisions
+ * if a bridge is hardcoded, so the network is at least defined
+ */
+ if (!def->bridge && !(def->bridge = virNetworkAllocateBridge(conn, nets)))
+ goto error;
+
if (!(net = virNetworkAssignDef(conn, nets, def)))
goto error;
@@ -848,6 +855,77 @@ char *virNetworkConfigFile(virConnectPtr conn,
return ret;
}
+int virNetworkBridgeInUse(const virNetworkObjListPtr nets,
+ const char *bridge,
+ const char *skipname)
+{
+ unsigned int i;
+ unsigned int ret = 0;
+
+ for (i = 0 ; i < nets->count ; i++) {
+ virNetworkObjLock(nets->objs[i]);
+ if (nets->objs[i]->def->bridge &&
+ STREQ(nets->objs[i]->def->bridge, bridge) &&
+ !(skipname && STREQ(nets->objs[i]->def->name, skipname)))
+ ret = 1;
+ virNetworkObjUnlock(nets->objs[i]);
+ }
+
+ return ret;
+}
+
+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);
+
+ virNetworkReportError(conn, VIR_ERR_INTERNAL_ERROR,
+ _("Bridge generation exceeded max id %d"),
+ MAX_BRIDGE_ID);
+ return NULL;
+}
+
+int virNetworkSetBridgeName(virConnectPtr conn,
+ const virNetworkObjListPtr nets,
+ virNetworkDefPtr def) {
+
+ int ret = -1;
+
+ if (def->bridge) {
+ if (virNetworkBridgeInUse(nets, def->bridge, def->name)) {
+ networkReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
+ _("bridge name '%s' already in use."),
+ def->bridge);
+ goto error;
+ }
+ } else {
+ /* Allocate a bridge name */
+ if (!(def->bridge = virNetworkAllocateBridge(conn, nets)))
+ goto error;
+ }
+
+ ret = 0;
+error:
+ return ret;
+}
void virNetworkObjLock(virNetworkObjPtr obj)
{
diff --git a/src/network_conf.h b/src/network_conf.h
index 94a1748..7e36e68 100644
--- a/src/network_conf.h
+++ b/src/network_conf.h
@@ -107,6 +107,10 @@ virNetworkIsActive(const virNetworkObjPtr net)
return net->active;
}
+#define networkReportError(conn, dom, net, code, fmt...) \
+ virReportErrorHelper(conn, VIR_FROM_QEMU, code, __FILE__, \
+ __FUNCTION__, __LINE__, fmt)
+
virNetworkObjPtr virNetworkFindByUUID(const virNetworkObjListPtr nets,
const unsigned char *uuid);
@@ -165,6 +169,16 @@ char *virNetworkConfigFile(virConnectPtr conn,
const char *dir,
const char *name);
+int virNetworkBridgeInUse(const virNetworkObjListPtr nets,
+ const char *bridge,
+ const char *skipname);
+
+char *virNetworkAllocateBridge(virConnectPtr conn,
+ const virNetworkObjListPtr nets);
+
+int virNetworkSetBridgeName(virConnectPtr conn,
+ const virNetworkObjListPtr nets,
+ virNetworkDefPtr def);
void virNetworkObjLock(virNetworkObjPtr obj);
void virNetworkObjUnlock(virNetworkObjPtr obj);
diff --git a/src/network_driver.c b/src/network_driver.c
index 4b9c666..a17a769 100644
--- a/src/network_driver.c
+++ b/src/network_driver.c
@@ -91,11 +91,6 @@ static int networkShutdown(void);
#define networkLog(level, msg...) fprintf(stderr, msg)
-#define networkReportError(conn, dom, net, code, fmt...) \
- virReportErrorHelper(conn, VIR_FROM_QEMU, code, __FILE__, \
- __FUNCTION__, __LINE__, fmt)
-
-
static int networkStartNetworkDaemon(virConnectPtr conn,
struct network_driver *driver,
virNetworkObjPtr network);
@@ -812,7 +807,7 @@ static int networkStartNetworkDaemon(virConnectPtr conn,
return -1;
}
- if ((err = brAddBridge(driver->brctl, &network->def->bridge))) {
+ if ((err = brAddBridge(driver->brctl, network->def->bridge))) {
virReportSystemError(conn, err,
_("cannot create bridge '%s'"),
network->def->bridge);
@@ -1113,6 +1108,9 @@ static virNetworkPtr networkCreate(virConnectPtr conn, const char *xml) {
if (!(def = virNetworkDefParseString(conn, xml)))
goto cleanup;
+ if (virNetworkSetBridgeName(conn, &driver->networks, def))
+ goto cleanup;
+
if (!(network = virNetworkAssignDef(conn,
&driver->networks,
def)))
@@ -1147,6 +1145,9 @@ static virNetworkPtr networkDefine(virConnectPtr conn, const char *xml) {
if (!(def = virNetworkDefParseString(conn, xml)))
goto cleanup;
+ if (virNetworkSetBridgeName(conn, &driver->networks, def))
+ goto cleanup;
+
if (!(network = virNetworkAssignDef(conn,
&driver->networks,
def)))
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list