12:55 p.m.
This patch addresses:
https://bugzilla.redhat.com/show_bug.cgi?id=694382
In order to give each libvirt-created bridge a fixed MAC address,
commit 5754dbd56d4738112a86776c09e810e32f7c3224, added code to create
a dummy tap device with guaranteed lowest MAC address and attach it to
the bridge. This tap device was given the name "${bridgename}-nic".
However, an interface device name must be IFNAMSIZ (15) characters or
less, so a valid ${bridgename} such as "verylongname123" (15
characters) would lead to an invalid tap device name
("verylongname123-nic" - 19 characters), and that in turn led to a
failure to bring up the network.
The solution is to shorten the part of the original name used to
generate the tap device name. However, simply truncating it is
insufficient, because the last few characters of an interface name are
often a number used to indicate one of a list of several similar
devices (for example, "verylongname123", "verylongname124", etc) and
simple truncation would lead to duplicate names. So instead we take
the first 8 characters of $bridgename ("verylong" in the example), add
on the final 3 bytes ("123"), then add "-nic" (so
"verylong123-nic").
Not pretty, but it is much more likely to generate a unique name, and
is reproducible (unlike, say, a random number).
---
src/network/bridge_driver.c | 14 +++++++++++++-
1 files changed, 13 insertions(+), 1 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index ea2bfd4..ef89bf5 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -143,7 +143,19 @@ networkBridgeDummyNicName(const char *brname)
{
char *nicname;
- virAsprintf(&nicname, "%s-nic", brname);
+ if (strlen(brname) > (IFNAMSIZ - 5)) {
+ /* because the length of an ifname is limited to IFNAMSIZ-1
+ * (usually 15), and we're adding 4 more characters, we must
+ * truncate the original name to 11 to fit. In order to catch
+ * a possible numeric ending (eg virbr0, virbr1, etc), we grab
+ * the first 8 and last 3 characters of the string.
+ */
+ virAsprintf(&nicname, "%.*s%s-nic",
+ IFNAMSIZ - 8, /* space for last 3 chars + "-nic" + NULL
*/
+ brname, brname + strlen(brname) - 3);
+ } else {
+ virAsprintf(&nicname, "%s-nic", brname);
+ }
return nicname;
}
--
1.7.3.4
2:24 p.m.
New subject: [libvirt] [PATCH] network: truncate bridges' dummy tap device names to IFNAMSIZ (15) chars
On 04/13/2011 11:55 AM, Laine Stump wrote:
The solution is to shorten the part of the original name used to
generate the tap device name. However, simply truncating it is
insufficient, because the last few characters of an interface name are
often a number used to indicate one of a list of several similar
devices (for example, "verylongname123", "verylongname124", etc) and
simple truncation would lead to duplicate names. So instead we take
the first 8 characters of $bridgename ("verylong" in the example), add
on the final 3 bytes ("123"), then add "-nic" (so
"verylong123-nic").
Not pretty, but it is much more likely to generate a unique name, and
is reproducible (unlike, say, a random number).
Should we also minimize the truncation by adding just "-n" instead of
"-nic", so that there are fewer user strings being butchered? Or would
that cause problems for existing users that already have bridge-nic and
would now also have bridge-n?
- virAsprintf(&nicname, "%s-nic", brname);
+ if (strlen(brname) > (IFNAMSIZ - 5)) {
+ /* because the length of an ifname is limited to IFNAMSIZ-1
+ * (usually 15), and we're adding 4 more characters, we must
+ * truncate the original name to 11 to fit. In order to catch
+ * a possible numeric ending (eg virbr0, virbr1, etc), we grab
+ * the first 8 and last 3 characters of the string.
+ */
+ virAsprintf(&nicname, "%.*s%s-nic",
+ IFNAMSIZ - 8, /* space for last 3 chars + "-nic" + NULL
*/
+ brname, brname + strlen(brname) - 3);
+ } else {
+ virAsprintf(&nicname, "%s-nic", brname);
+ }
ACK.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
1:52 a.m.
New subject: [libvirt] [PATCH] network: truncate bridges' dummy tap device names to IFNAMSIZ (15) chars
On 04/13/2011 03:24 PM, Eric Blake wrote:
On 04/13/2011 11:55 AM, Laine Stump wrote:
> The solution is to shorten the part of the original name used to
> generate the tap device name. However, simply truncating it is
> insufficient, because the last few characters of an interface name are
> often a number used to indicate one of a list of several similar
> devices (for example, "verylongname123", "verylongname124", etc)
and
> simple truncation would lead to duplicate names. So instead we take
> the first 8 characters of $bridgename ("verylong" in the example), add
> on the final 3 bytes ("123"), then add "-nic" (so
"verylong123-nic").
> Not pretty, but it is much more likely to generate a unique name, and
> is reproducible (unlike, say, a random number).
Should we also minimize the truncation by adding just "-n" instead of
"-nic", so that there are fewer user strings being butchered? Or would
that cause problems for existing users that already have bridge-nic and
would now also have bridge-n?
Well, for someone who upgraded with a network that was still up, then
restarted the network without rebooting, it would leave around an extra
bridge-nic tap interface with the same MAC address as the new bridge-n
interface. But that interface would be down (which also means it
wouldn't show up in the ifconfig output unless you added "-a"), not
connected to anything, and would disappear completely the next time the
system was rebooted. (Also, I just checked to verify that an extra tap
device with matching MAC address doesn't mess with proper networking of
the guests, and it doesn't.)
So, is this minor artifact worth getting the extra two characters? I'll
let the patch sit until midday, and if someone comes out in support of
changing it, I'll reduce the suffix to "-n" and push, otherwise I'll
push as-is.
> - virAsprintf(&nicname, "%s-nic", brname);
> + if (strlen(brname)> (IFNAMSIZ - 5)) {
> + /* because the length of an ifname is limited to IFNAMSIZ-1
> + * (usually 15), and we're adding 4 more characters, we must
> + * truncate the original name to 11 to fit. In order to catch
> + * a possible numeric ending (eg virbr0, virbr1, etc), we grab
> + * the first 8 and last 3 characters of the string.
> + */
> + virAsprintf(&nicname, "%.*s%s-nic",
> + IFNAMSIZ - 8, /* space for last 3 chars + "-nic" +
NULL */
> + brname, brname + strlen(brname) - 3);
> + } else {
> + virAsprintf(&nicname, "%s-nic", brname);
> + }
ACK.
10:51 a.m.
New subject: [libvirt] [PATCH] network: truncate bridges' dummy tap device names to IFNAMSIZ (15) chars
On 04/14/2011 12:52 AM, Laine Stump wrote:
> Should we also minimize the truncation by adding just
"-n" instead of
> "-nic", so that there are fewer user strings being butchered? Or would
> that cause problems for existing users that already have bridge-nic and
> would now also have bridge-n?
So, is this minor artifact worth getting the extra two characters?
I'll
let the patch sit until midday, and if someone comes out in support of
changing it, I'll reduce the suffix to "-n" and push, otherwise I'll
push as-is.
I can live with the patch as-is, if no one else speaks for the two extra
characters.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
2:06 p.m.
New subject: [libvirt] [PATCHv2] network: truncate bridges' dummy tap device names to IFNAMSIZ (15) chars
(Change from V1 - define a static char[] containing the desired
prefix, and use its size to figure out how much to truncate, rather
than having a bunch of magic numbers. This also makes it simpler and
less error-prone to change the suffix if we decide to.)
This patch addresses:
https://bugzilla.redhat.com/show_bug.cgi?id=694382
In order to give each libvirt-created bridge a fixed MAC address,
commit 5754dbd56d4738112a86776c09e810e32f7c3224, added code to create
a dummy tap device with guaranteed lowest MAC address and attach it to
the bridge. This tap device was given the name "${bridgename}-nic".
However, an interface device name must be IFNAMSIZ (15) characters or
less, so a valid ${bridgename} such as "verylongname123" (15
characters) would lead to an invalid tap device name
("verylongname123-nic" - 19 characters), and that in turn led to a
failure to bring up the network.
The solution is to shorten the part of the original name used to
generate the tap device name. However, simply truncating it is
insufficient, because the last few characters of an interface name are
often a number used to indicate one of a list of several similar
devices (for example, "verylongname123", "verylongname124", etc) and
simple truncation would lead to duplicate names (eg "verlongnam-nic"
and "verylongnam-nic"). So instead we take the first 8 characters of
$bridgename ("verylong" in the example), add on the final 3 bytes
("123"), then add "-nic" (so "verylong123-nic"). Not
pretty, but it
is much more likely to generate a unique name, and is reproducible
(unlike, say, a random number).
---
src/network/bridge_driver.c | 16 +++++++++++++++-
1 files changed, 15 insertions(+), 1 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index ea2bfd4..b108bb9 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -141,9 +141,23 @@ networkRadvdConfigFileName(const char *netname)
static char *
networkBridgeDummyNicName(const char *brname)
{
+ static const char dummyNicSuffix[] = "-nic";
char *nicname;
- virAsprintf(&nicname, "%s-nic", brname);
+ if (strlen(brname) + sizeof(dummyNicSuffix) > IFNAMSIZ) {
+ /* because the length of an ifname is limited to IFNAMSIZ-1
+ * (usually 15), and we're adding 4 more characters, we must
+ * truncate the original name to 11 to fit. In order to catch
+ * a possible numeric ending (eg virbr0, virbr1, etc), we grab
+ * the first 8 and last 3 characters of the string.
+ */
+ virAsprintf(&nicname, "%.*s%s%s",
+ /* space for last 3 chars + "-nic" + NULL */
+ (int)(IFNAMSIZ - (3 + sizeof(dummyNicSuffix))),
+ brname, brname + strlen(brname) - 3, dummyNicSuffix);
+ } else {
+ virAsprintf(&nicname, "%s%s", brname, dummyNicSuffix);
+ }
return nicname;
}
--
1.7.3.4
2:19 p.m.
New subject: [libvirt] [PATCHv2] network: truncate bridges' dummy tap device names to IFNAMSIZ (15) chars
On 04/14/2011 01:06 PM, Laine Stump wrote:
(Change from V1 - define a static char[] containing the desired
prefix, and use its size to figure out how much to truncate, rather
than having a bunch of magic numbers. This also makes it simpler and
less error-prone to change the suffix if we decide to.)
Indeed :)
The solution is to shorten the part of the original name used to
generate the tap device name. However, simply truncating it is
insufficient, because the last few characters of an interface name are
often a number used to indicate one of a list of several similar
devices (for example, "verylongname123", "verylongname124", etc) and
simple truncation would lead to duplicate names (eg "verlongnam-nic"
and "verylongnam-nic"). So instead we take the first 8 characters of
$bridgename ("verylong" in the example), add on the final 3 bytes
("123"), then add "-nic" (so "verylong123-nic"). Not
pretty, but it
is much more likely to generate a unique name, and is reproducible
(unlike, say, a random number).
---
src/network/bridge_driver.c | 16 +++++++++++++++-
1 files changed, 15 insertions(+), 1 deletions(-)
Here's making my IRC review official:
ACK.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
3:43 p.m.
New subject: [libvirt] [PATCHv2] network: truncate bridges' dummy tap device names to IFNAMSIZ (15) chars
On 04/14/2011 03:19 PM, Eric Blake wrote:
---
> src/network/bridge_driver.c | 16 +++++++++++++++-
> 1 files changed, 15 insertions(+), 1 deletions(-)
Here's making my IRC review official:
ACK.
Pushed. Thanks!
5007
days inactive
5008
days old
6 comments
2 participants
participants (2)
-
Eric Blake -
Laine Stump