Re: [libvirt] Supporting dynamic bridge names
On Fri, Apr 17, 2009 at 01:12:26PM +0200, Soren Hansen wrote: On Fri, Apr 17, 2009 at 10:45:19AM +0100, Daniel P. Berrange wrote: The problem with this approach is that the bridge name potentially ends up being different every time the virtual network is started. The bridge name needs to be stable, Why? I can't remember ever having to refer to it, and if I did, I can get that information at runtime by parsing the output from virsh net-dumpxml name of the network. What am I not thinking of? :) The non-QEMU drivers I'm afraid. Ah, I see. I've not spent much time with those. Longer term, would it perhaps make sense to let them specify the name of the network they want to use like we do for the QEMU driver? [...] Updated patch: Okay makes sense, I just had to fix a couple of Tabs but it's in CVS, thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] Supporting dynamic bridge names
I'm fairly sure we've discussed this before, but I couldn't find it in
my archives..
A rather long time ago (0.4.0 timeframe, I think) we switched the
default network in Ubuntu to use a bridge whose name was defined as
virbr%d. This was done to be able to actually supply a default,
enabled network without the risk of interfering with an existing bridge
called virbr0 (ignoring the possible implications of unconditionally
using 192.168.122.0/24 (about which, I might add, I've never had /any/
complaints)).
Now, since 0.5.0 (or thereabouts, I think), libvirt doesn't support
this, which
a) makes it rather difficult to achieve our goal of not clashing with
existing bridges named virbr0, and
b) causes some amount of grief for updates.
To overcome this, I've changed virNetworkAllocateBridge like so:
--- libvirt-0.6.1.orig/src/network_conf.c 2009-03-03 09:23:22.0
+0100
+++ libvirt-0.6.1/src/network_conf.c2009-04-16 20:36:43.660996644 +0200
@@ -875,16 +875,20 @@
}
char *virNetworkAllocateBridge(virConnectPtr conn,
- const virNetworkObjListPtr nets)
+ const virNetworkObjListPtr nets,
+ const char *template)
{
int id = 0;
char *newname;
+ if (!template)
+ template = virbr%d;
+
do {
char try[50];
-snprintf(try, sizeof(try), virbr%d, id);
+snprintf(try, sizeof(try), template, id);
if (!virNetworkBridgeInUse(nets, try, NULL)) {
if (!(newname = strdup(try))) {
--- libvirt-0.6.1.orig/src/network_conf.h 2009-03-03 09:23:22.0
+0100
+++ libvirt-0.6.1/src/network_conf.h2009-04-16 15:36:46.975452201 +0200
@@ -174,7 +174,8 @@
const char *skipname);
char *virNetworkAllocateBridge(virConnectPtr conn,
- const virNetworkObjListPtr nets);
+ const virNetworkObjListPtr nets,
+ const char *template);
int virNetworkSetBridgeName(virConnectPtr conn,
const virNetworkObjListPtr nets,
And virNetworkSetBridgeName like so:
--- libvirt-0.6.1.orig/src/network_conf.c 2009-03-03 09:23:22.0
+0100
+++ libvirt-0.6.1/src/network_conf.c2009-04-16 20:36:43.660996644 +0200
@@ -909,7 +913,7 @@
int ret = -1;
-if (def-bridge) {
+if (def-bridge !strstr(def-bridge, %d)) {
if (virNetworkBridgeInUse(nets, def-bridge, def-name)) {
networkReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
_(bridge name '%s' already in use.),
@@ -918,7 +922,7 @@
}
} else {
/* Allocate a bridge name */
-if (!(def-bridge = virNetworkAllocateBridge(conn, nets)))
+if (!(def-bridge = virNetworkAllocateBridge(conn, nets, def-bridge)))
goto error;
}
And finally virNetworkLoadConfig like so:
--- libvirt-0.6.1.orig/src/network_conf.c 2009-03-03 09:23:22.0
+0100
+++ libvirt-0.6.1/src/network_conf.c2009-04-16 20:36:43.660996644 +0200
@@ -747,7 +747,7 @@
/* 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)))
+if (!(def-bridge = virNetworkAllocateBridge(conn, nets, def-bridge)))
goto error;
if (!(net = virNetworkAssignDef(conn, nets, def)))
This is far from perfect, though.
a) As used to be the case for the VNC port, virsh net-dumpxml will
give you /current/ bridge name, so e.g. virsh net-edit will cause you
to hardcode the bridge name if you're not careful to replace the current
name with one that says '%d' somewhere. This can be fixed by e.g. either
adding a template or current attribute on the bridge element in the
network XML (depending on whether you want the name attribute to be
set to the current value or the template value).
b) Even if you /do/ remember to put '%d' in place of the assigned number
during editing, the bridge number will be increased, because
virNetworkBridgeInUse thinks the bridge name is in use. This should be
easy to fix, though.
I'm perfectly willing to work on this, but I'd like to get you guys' gut
feeling on this first.
--
Soren Hansen |
Lead Virtualisation Engineer | Ubuntu Server Team
Canonical Ltd. | http://www.ubuntu.com/
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Supporting dynamic bridge names
On Fri, Apr 17, 2009 at 10:01:38AM +0100, Daniel P. Berrange wrote:
The problem with this approach is that the bridge name potentially
ends up being different every time the virtual network is started. The
bridge name needs to be stable,
Why? I can't remember ever having to refer to it, and if I did, I can
get that information at runtime by parsing the output from virsh
net-dumpxml name of the network. What am I not thinking of? :)
if (!def-bridge !(def-bridge = virNetworkAllocateBridge(conn,
nets)))
To check for %d, as well as NULL, and pass in the template name as your
patch more or less does
Right, with my patch applied virNetworkAllocateBridge takes the
!def-bridge case into account.
I guess that if this is how you want it, the patch should be good as it
is. Here it is in its entirety:
Index: libvirt-0.6.1/src/network_conf.c
===
--- libvirt-0.6.1.orig/src/network_conf.c 2009-04-16 20:49:28.851655724
+0200
+++ libvirt-0.6.1/src/network_conf.c2009-04-17 09:42:33.075084537 +0200
@@ -747,7 +747,7 @@
/* 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)))
+if (!(def-bridge = virNetworkAllocateBridge(conn, nets, def-bridge)))
goto error;
if (!(net = virNetworkAssignDef(conn, nets, def)))
@@ -875,16 +875,20 @@
}
char *virNetworkAllocateBridge(virConnectPtr conn,
- const virNetworkObjListPtr nets)
+ const virNetworkObjListPtr nets,
+ const char *template)
{
int id = 0;
char *newname;
+ if (!template)
+ template = virbr%d;
+
do {
char try[50];
-snprintf(try, sizeof(try), virbr%d, id);
+snprintf(try, sizeof(try), template, id);
if (!virNetworkBridgeInUse(nets, try, NULL)) {
if (!(newname = strdup(try))) {
@@ -909,7 +913,7 @@
int ret = -1;
-if (def-bridge) {
+if (def-bridge !strstr(def-bridge, %d)) {
if (virNetworkBridgeInUse(nets, def-bridge, def-name)) {
networkReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
_(bridge name '%s' already in use.),
@@ -918,7 +922,7 @@
}
} else {
/* Allocate a bridge name */
-if (!(def-bridge = virNetworkAllocateBridge(conn, nets)))
+if (!(def-bridge = virNetworkAllocateBridge(conn, nets, def-bridge)))
goto error;
}
Index: libvirt-0.6.1/src/network_conf.h
===
--- libvirt-0.6.1.orig/src/network_conf.h 2009-04-16 20:49:28.899655820
+0200
+++ libvirt-0.6.1/src/network_conf.h2009-04-17 09:42:33.075084537 +0200
@@ -174,7 +174,8 @@
const char *skipname);
char *virNetworkAllocateBridge(virConnectPtr conn,
- const virNetworkObjListPtr nets);
+ const virNetworkObjListPtr nets,
+ const char *template);
int virNetworkSetBridgeName(virConnectPtr conn,
const virNetworkObjListPtr nets,
--
Soren Hansen |
Lead Virtualisation Engineer | Ubuntu Server Team
Canonical Ltd. | http://www.ubuntu.com/
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Supporting dynamic bridge names
On Fri, Apr 17, 2009 at 11:38:23AM +0200, Soren Hansen wrote:
On Fri, Apr 17, 2009 at 10:01:38AM +0100, Daniel P. Berrange wrote:
The problem with this approach is that the bridge name potentially
ends up being different every time the virtual network is started. The
bridge name needs to be stable,
Why? I can't remember ever having to refer to it, and if I did, I can
get that information at runtime by parsing the output from virsh
net-dumpxml name of the network. What am I not thinking of? :)
The non-QEMU drivers I'm afraid. For those, when we connect a guest to
a virtual network, the underlying guest config includes the name of the
bridge associated with that network. So if the network changes its
bridge name, then all your Xen guests loose network connectivity the
next time around :-(
if (!def-bridge !(def-bridge = virNetworkAllocateBridge(conn,
nets)))
To check for %d, as well as NULL, and pass in the template name as your
patch more or less does
Right, with my patch applied virNetworkAllocateBridge takes the
!def-bridge case into account.
I guess that if this is how you want it, the patch should be good as it
is. Here it is in its entirety:
Index: libvirt-0.6.1/src/network_conf.c
===
--- libvirt-0.6.1.orig/src/network_conf.c 2009-04-16 20:49:28.851655724
+0200
+++ libvirt-0.6.1/src/network_conf.c 2009-04-17 09:42:33.075084537 +0200
@@ -747,7 +747,7 @@
/* 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)))
+if (!(def-bridge = virNetworkAllocateBridge(conn, nets, def-bridge)))
goto error;
I think this would leak memory - the def-bridge string being passed in
is overwritten upon return, if its a non-null string.
if (!(net = virNetworkAssignDef(conn, nets, def)))
@@ -875,16 +875,20 @@
}
char *virNetworkAllocateBridge(virConnectPtr conn,
- const virNetworkObjListPtr nets)
+ const virNetworkObjListPtr nets,
+ const char *template)
{
int id = 0;
char *newname;
+ if (!template)
+ template = virbr%d;
+
do {
char try[50];
-snprintf(try, sizeof(try), virbr%d, id);
+snprintf(try, sizeof(try), template, id);
if (!virNetworkBridgeInUse(nets, try, NULL)) {
if (!(newname = strdup(try))) {
@@ -909,7 +913,7 @@
int ret = -1;
-if (def-bridge) {
+if (def-bridge !strstr(def-bridge, %d)) {
if (virNetworkBridgeInUse(nets, def-bridge, def-name)) {
networkReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
_(bridge name '%s' already in use.),
@@ -918,7 +922,7 @@
}
} else {
/* Allocate a bridge name */
-if (!(def-bridge = virNetworkAllocateBridge(conn, nets)))
+if (!(def-bridge = virNetworkAllocateBridge(conn, nets,
def-bridge)))
goto error;
}
Index: libvirt-0.6.1/src/network_conf.h
===
--- libvirt-0.6.1.orig/src/network_conf.h 2009-04-16 20:49:28.899655820
+0200
+++ libvirt-0.6.1/src/network_conf.h 2009-04-17 09:42:33.075084537 +0200
@@ -174,7 +174,8 @@
const char *skipname);
char *virNetworkAllocateBridge(virConnectPtr conn,
- const virNetworkObjListPtr nets);
+ const virNetworkObjListPtr nets,
+ const char *template);
int virNetworkSetBridgeName(virConnectPtr conn,
const virNetworkObjListPtr nets,
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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Supporting dynamic bridge names
On Fri, Apr 17, 2009 at 10:45:19AM +0100, Daniel P. Berrange wrote:
The problem with this approach is that the bridge name potentially
ends up being different every time the virtual network is started.
The bridge name needs to be stable,
Why? I can't remember ever having to refer to it, and if I did, I can
get that information at runtime by parsing the output from virsh
net-dumpxml name of the network. What am I not thinking of? :)
The non-QEMU drivers I'm afraid.
Ah, I see. I've not spent much time with those. Longer term, would it
perhaps make sense to let them specify the name of the network they want
to use like we do for the QEMU driver?
Index: libvirt-0.6.1/src/network_conf.c
===
--- libvirt-0.6.1.orig/src/network_conf.c 2009-04-16 20:49:28.851655724
+0200
+++ libvirt-0.6.1/src/network_conf.c2009-04-17 09:42:33.075084537
+0200
@@ -747,7 +747,7 @@
/* 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)))
+if (!(def-bridge = virNetworkAllocateBridge(conn, nets, def-bridge)))
goto error;
I think this would leak memory - the def-bridge string being passed in
is overwritten upon return, if its a non-null string.
Good catch.
Updated patch:
Index: libvirt-0.6.1/src/network_conf.c
===
--- libvirt-0.6.1.orig/src/network_conf.c 2009-04-16 20:49:28.851655724
+0200
+++ libvirt-0.6.1/src/network_conf.c2009-04-17 13:11:54.494770980 +0200
@@ -724,6 +724,7 @@
virNetworkDefPtr def = NULL;
virNetworkObjPtr net;
int autostart;
+char *tmp;
if ((configFile = virNetworkConfigFile(conn, configDir, name)) == NULL)
goto error;
@@ -747,7 +748,10 @@
/* 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)))
+if (tmp = virNetworkAllocateBridge(conn, nets, def-bridge)) {
+VIR_FREE(def-bridge);
+def-bridge = tmp;
+} else
goto error;
if (!(net = virNetworkAssignDef(conn, nets, def)))
@@ -875,16 +879,20 @@
}
char *virNetworkAllocateBridge(virConnectPtr conn,
- const virNetworkObjListPtr nets)
+ const virNetworkObjListPtr nets,
+ const char *template)
{
int id = 0;
char *newname;
+ if (!template)
+ template = virbr%d;
+
do {
char try[50];
-snprintf(try, sizeof(try), virbr%d, id);
+snprintf(try, sizeof(try), template, id);
if (!virNetworkBridgeInUse(nets, try, NULL)) {
if (!(newname = strdup(try))) {
@@ -909,7 +917,7 @@
int ret = -1;
-if (def-bridge) {
+if (def-bridge !strstr(def-bridge, %d)) {
if (virNetworkBridgeInUse(nets, def-bridge, def-name)) {
networkReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
_(bridge name '%s' already in use.),
@@ -918,7 +926,7 @@
}
} else {
/* Allocate a bridge name */
-if (!(def-bridge = virNetworkAllocateBridge(conn, nets)))
+if (!(def-bridge = virNetworkAllocateBridge(conn, nets, def-bridge)))
goto error;
}
Index: libvirt-0.6.1/src/network_conf.h
===
--- libvirt-0.6.1.orig/src/network_conf.h 2009-04-16 20:49:28.899655820
+0200
+++ libvirt-0.6.1/src/network_conf.h2009-04-17 09:42:33.075084537 +0200
@@ -174,7 +174,8 @@
const char *skipname);
char *virNetworkAllocateBridge(virConnectPtr conn,
- const virNetworkObjListPtr nets);
+ const virNetworkObjListPtr nets,
+ const char *template);
int virNetworkSetBridgeName(virConnectPtr conn,
const virNetworkObjListPtr nets,
--
Soren Hansen |
Lead Virtualisation Engineer | Ubuntu Server Team
Canonical Ltd. | http://www.ubuntu.com/
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Supporting dynamic bridge names
On Fri, Apr 17, 2009 at 10:46:47AM +0200, Soren Hansen wrote: I'm fairly sure we've discussed this before, but I couldn't find it in my archives.. A rather long time ago (0.4.0 timeframe, I think) we switched the default network in Ubuntu to use a bridge whose name was defined as virbr%d. This was done to be able to actually supply a default, enabled network without the risk of interfering with an existing bridge called virbr0 (ignoring the possible implications of unconditionally using 192.168.122.0/24 (about which, I might add, I've never had /any/ complaints)). Now, since 0.5.0 (or thereabouts, I think), libvirt doesn't support this, which a) makes it rather difficult to achieve our goal of not clashing with existing bridges named virbr0, and b) causes some amount of grief for updates. [snip] This is far from perfect, though. a) As used to be the case for the VNC port, virsh net-dumpxml will give you /current/ bridge name, so e.g. virsh net-edit will cause you to hardcode the bridge name if you're not careful to replace the current name with one that says '%d' somewhere. This can be fixed by e.g. either adding a template or current attribute on the bridge element in the network XML (depending on whether you want the name attribute to be set to the current value or the template value). b) Even if you /do/ remember to put '%d' in place of the assigned number during editing, the bridge number will be increased, because virNetworkBridgeInUse thinks the bridge name is in use. This should be easy to fix, though. The problem with this approach is that the bridge name potentially ends up being different every time the virtual network is started. The bridge name needs to be stable, once allocated at time the virtual network is defined in libvirt. So if we're to accept 'virbr%d' in a XML for a network then at the time it is define, it should be converted into a real bridge name and remain fixed thereafter. This is almost possible already Just need to change the couple of places if (!def-bridge !(def-bridge = virNetworkAllocateBridge(conn, nets))) To check for %d, as well as NULL, and pass in the template name as your patch more or less does 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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
