[libvirt] [PATCH] Remove MAX_TAP_ID, take 3
This is a resend of take 2 to fix formatting problems in the patch. No other changes. As far as I can tell, there's no reason to format the device string in brAddTap(). Delegate the job to TUNSETIFF, thereby removing the loop and the MAX_TAP_ID artificial limit. This patch allows me to get 421 guests running before hitting other limits. Signed-off-by: Aron Griffis <aron.griffis@hp.com> diff --git a/src/bridge.c b/src/bridge.c index 0509afd..ec37192 100644 --- a/src/bridge.c +++ b/src/bridge.c @@ -49,8 +49,6 @@ #include "util.h" #include "logging.h" -#define MAX_TAP_ID 256 - #define JIFFIES_TO_MS(j) (((j)*1000)/HZ) #define MS_TO_JIFFIES(ms) (((ms)*HZ)/1000) @@ -466,76 +464,52 @@ brAddTap(brControl *ctl, int vnet_hdr, int *tapfd) { - int id, subst, fd; + int fd, len; + struct ifreq ifr = {0}; if (!ctl || !ctl->fd || !bridge || !ifname) return EINVAL; - subst = id = 0; - - if (strstr(*ifname, "%d")) - subst = 1; - if ((fd = open("/dev/net/tun", O_RDWR)) < 0) return errno; - if (vnet_hdr) - vnet_hdr = brProbeVnetHdr(fd); + ifr.ifr_flags = IFF_TAP|IFF_NO_PI; - do { - struct ifreq try; - int len; +#ifdef IFF_VNET_HDR + if (vnet_hdr && brProbeVnetHdr(fd)) + ifr.ifr_flags |= IFF_VNET_HDR; +#endif - memset(&try, 0, sizeof(struct ifreq)); + strncpy(ifr.ifr_name, *ifname, IFNAMSIZ-1); - try.ifr_flags = IFF_TAP|IFF_NO_PI; + if (ioctl(fd, TUNSETIFF, &ifr) < 0) + goto error; -#ifdef IFF_VNET_HDR - if (vnet_hdr) - try.ifr_flags |= IFF_VNET_HDR; -#endif + len = strlen(ifr.ifr_name); + if (len >= BR_IFNAME_MAXLEN - 1) { + errno = EINVAL; + goto error; + } - if (subst) { - len = snprintf(try.ifr_name, BR_IFNAME_MAXLEN, *ifname, id); - if (len >= BR_IFNAME_MAXLEN) { - errno = EADDRINUSE; - goto error; - } - } else { - len = strlen(*ifname); - if (len >= BR_IFNAME_MAXLEN - 1) { - errno = EINVAL; - goto error; - } - - strncpy(try.ifr_name, *ifname, len); - try.ifr_name[len] = '\0'; - } - - if (ioctl(fd, TUNSETIFF, &try) == 0) { - /* We need to set the interface MTU before adding it - * to the bridge, because the bridge will have its - * MTU adjusted automatically when we add the new interface. - */ - if ((errno = brSetInterfaceMtu(ctl, bridge, try.ifr_name))) - goto error; - if ((errno = brAddInterface(ctl, bridge, try.ifr_name))) - goto error; - if ((errno = brSetInterfaceUp(ctl, try.ifr_name, 1))) - goto error; - if (!tapfd && - (errno = ioctl(fd, TUNSETPERSIST, 1))) - goto error; - VIR_FREE(*ifname); - if (!(*ifname = strdup(try.ifr_name))) - goto error; - if (tapfd) - *tapfd = fd; - return 0; - } - - id++; - } while (subst && id <= MAX_TAP_ID); + /* We need to set the interface MTU before adding it + * to the bridge, because the bridge will have its + * MTU adjusted automatically when we add the new interface. + */ + if ((errno = brSetInterfaceMtu(ctl, bridge, ifr.ifr_name))) + goto error; + if ((errno = brAddInterface(ctl, bridge, ifr.ifr_name))) + goto error; + if ((errno = brSetInterfaceUp(ctl, ifr.ifr_name, 1))) + goto error; + if (!tapfd && + (errno = ioctl(fd, TUNSETPERSIST, 1))) + goto error; + VIR_FREE(*ifname); + if (!(*ifname = strdup(ifr.ifr_name))) + goto error; + if (tapfd) + *tapfd = fd; + return 0; error: close(fd);
On Wed, Jul 29, 2009 at 12:52:30PM -0400, Aron Griffis wrote:
This is a resend of take 2 to fix formatting problems in the patch. No other changes.
As far as I can tell, there's no reason to format the device string in brAddTap(). Delegate the job to TUNSETIFF, thereby removing the loop and the MAX_TAP_ID artificial limit. This patch allows me to get 421 guests running before hitting other limits.
haha ! that one worked :-) I will review and apply, thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Wed, Jul 29, 2009 at 08:05:37PM +0200, Daniel Veillard wrote:
On Wed, Jul 29, 2009 at 12:52:30PM -0400, Aron Griffis wrote:
This is a resend of take 2 to fix formatting problems in the patch. No other changes.
As far as I can tell, there's no reason to format the device string in brAddTap(). Delegate the job to TUNSETIFF, thereby removing the loop and the MAX_TAP_ID artificial limit. This patch allows me to get 421 guests running before hitting other limits.
haha ! that one worked :-)
I will review and apply, thanks !
Actually just looking at brAddTap() after patching makes it clear, and based on Mark and Dan feedback great ! Applied and commited to git, thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
2009/7/29 Daniel Veillard <veillard@redhat.com>:
On Wed, Jul 29, 2009 at 08:05:37PM +0200, Daniel Veillard wrote:
On Wed, Jul 29, 2009 at 12:52:30PM -0400, Aron Griffis wrote:
This is a resend of take 2 to fix formatting problems in the patch. No other changes.
As far as I can tell, there's no reason to format the device string in brAddTap(). Delegate the job to TUNSETIFF, thereby removing the loop and the MAX_TAP_ID artificial limit. This patch allows me to get 421 guests running before hitting other limits.
haha ! that one worked :-)
I will review and apply, thanks !
Actually just looking at brAddTap() after patching makes it clear, and based on Mark and Dan feedback great ! Applied and commited to git, thanks !
Daniel
This patch breaks -Werror, because GCC is unhappy with the initializer for ifreq. The follow change makes GCC happy again: diff --git a/src/bridge.c b/src/bridge.c index ec37192..6480a35 100644 --- a/src/bridge.c +++ b/src/bridge.c @@ -465,7 +465,7 @@ brAddTap(brControl *ctl, int *tapfd) { int fd, len; - struct ifreq ifr = {0}; + struct ifreq ifr = {{{0}}, {{0, {0}}}}; if (!ctl || !ctl->fd || !bridge || !ifname) return EINVAL;
Matthias Bolte wrote: [Wed Jul 29 2009, 08:58:18PM EDT]
2009/7/29 Daniel Veillard <veillard@redhat.com>:
On Wed, Jul 29, 2009 at 08:05:37PM +0200, Daniel Veillard wrote:
On Wed, Jul 29, 2009 at 12:52:30PM -0400, Aron Griffis wrote:
This is a resend of take 2 to fix formatting problems in the patch. No other changes.
As far as I can tell, there's no reason to format the device string in brAddTap(). Delegate the job to TUNSETIFF, thereby removing the loop and the MAX_TAP_ID artificial limit. This patch allows me to get 421 guests running before hitting other limits.
haha ! that one worked :-)
I will review and apply, thanks !
Actually just looking at brAddTap() after patching makes it clear, and based on Mark and Dan feedback great ! Applied and commited to git, thanks !
Daniel
This patch breaks -Werror, because GCC is unhappy with the initializer for ifreq.
The follow change makes GCC happy again:
diff --git a/src/bridge.c b/src/bridge.c index ec37192..6480a35 100644 --- a/src/bridge.c +++ b/src/bridge.c @@ -465,7 +465,7 @@ brAddTap(brControl *ctl, int *tapfd) { int fd, len; - struct ifreq ifr = {0}; + struct ifreq ifr = {{{0}}, {{0, {0}}}};
if (!ctl || !ctl->fd || !bridge || !ifname) return EINVAL;
Ugh, sorry about that. And it looked so neat and clean to me. ;-) Maybe instead of the nested curlies, we should revert to memset as it was doing previously: diff --git a/src/bridge.c b/src/bridge.c index ec37192..58e480e 100644 --- a/src/bridge.c +++ b/src/bridge.c @@ -465,7 +465,7 @@ brAddTap(brControl *ctl, int *tapfd) { int fd, len; - struct ifreq ifr = {0}; + struct ifreq ifr; if (!ctl || !ctl->fd || !bridge || !ifname) return EINVAL; @@ -473,6 +473,8 @@ brAddTap(brControl *ctl, if ((fd = open("/dev/net/tun", O_RDWR)) < 0) return errno; + memset(&ifr, 0, sizeof(ifr)); + ifr.ifr_flags = IFF_TAP|IFF_NO_PI; #ifdef IFF_VNET_HDR
On Wed, Jul 29, 2009 at 10:56:48PM -0400, Aron Griffis wrote:
Matthias Bolte wrote: [Wed Jul 29 2009, 08:58:18PM EDT]
2009/7/29 Daniel Veillard <veillard@redhat.com>:
On Wed, Jul 29, 2009 at 08:05:37PM +0200, Daniel Veillard wrote:
On Wed, Jul 29, 2009 at 12:52:30PM -0400, Aron Griffis wrote:
This is a resend of take 2 to fix formatting problems in the patch. No other changes.
As far as I can tell, there's no reason to format the device string in brAddTap(). Delegate the job to TUNSETIFF, thereby removing the loop and the MAX_TAP_ID artificial limit. This patch allows me to get 421 guests running before hitting other limits.
haha ! that one worked :-)
I will review and apply, thanks !
Actually just looking at brAddTap() after patching makes it clear, and based on Mark and Dan feedback great ! Applied and commited to git, thanks !
Daniel
This patch breaks -Werror, because GCC is unhappy with the initializer for ifreq.
Sorry I should have caught that
Maybe instead of the nested curlies, we should revert to memset as it was doing previously:
diff --git a/src/bridge.c b/src/bridge.c index ec37192..58e480e 100644 --- a/src/bridge.c +++ b/src/bridge.c @@ -465,7 +465,7 @@ brAddTap(brControl *ctl, int *tapfd) { int fd, len; - struct ifreq ifr = {0}; + struct ifreq ifr;
if (!ctl || !ctl->fd || !bridge || !ifname) return EINVAL; @@ -473,6 +473,8 @@ brAddTap(brControl *ctl, if ((fd = open("/dev/net/tun", O_RDWR)) < 0) return errno;
+ memset(&ifr, 0, sizeof(ifr)); + ifr.ifr_flags = IFF_TAP|IFF_NO_PI;
yeah the memset was the status quo before, let's go back to using this. Applied and commited, thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Thu, 2009-07-30 at 02:58 +0200, Matthias Bolte wrote:
2009/7/29 Daniel Veillard <veillard@redhat.com>:
On Wed, Jul 29, 2009 at 08:05:37PM +0200, Daniel Veillard wrote:
On Wed, Jul 29, 2009 at 12:52:30PM -0400, Aron Griffis wrote:
This is a resend of take 2 to fix formatting problems in the patch. No other changes.
As far as I can tell, there's no reason to format the device string in brAddTap(). Delegate the job to TUNSETIFF, thereby removing the loop and the MAX_TAP_ID artificial limit. This patch allows me to get 421 guests running before hitting other limits.
haha ! that one worked :-)
I will review and apply, thanks !
Actually just looking at brAddTap() after patching makes it clear, and based on Mark and Dan feedback great ! Applied and commited to git, thanks !
Daniel
This patch breaks -Werror, because GCC is unhappy with the initializer for ifreq.
The follow change makes GCC happy again:
diff --git a/src/bridge.c b/src/bridge.c index ec37192..6480a35 100644 --- a/src/bridge.c +++ b/src/bridge.c @@ -465,7 +465,7 @@ brAddTap(brControl *ctl, int *tapfd) { int fd, len; - struct ifreq ifr = {0}; + struct ifreq ifr = {{{0}}, {{0, {0}}}};
AFAIR, this works? struct ifreq ifr = {0,}; Cheers, Mark.
2009/7/30 Mark McLoughlin <markmc@redhat.com>:
On Thu, 2009-07-30 at 02:58 +0200, Matthias Bolte wrote:
2009/7/29 Daniel Veillard <veillard@redhat.com>:
On Wed, Jul 29, 2009 at 08:05:37PM +0200, Daniel Veillard wrote:
On Wed, Jul 29, 2009 at 12:52:30PM -0400, Aron Griffis wrote:
This is a resend of take 2 to fix formatting problems in the patch. No other changes.
As far as I can tell, there's no reason to format the device string in brAddTap(). Delegate the job to TUNSETIFF, thereby removing the loop and the MAX_TAP_ID artificial limit. This patch allows me to get 421 guests running before hitting other limits.
haha ! that one worked :-)
I will review and apply, thanks !
Actually just looking at brAddTap() after patching makes it clear, and based on Mark and Dan feedback great ! Applied and commited to git, thanks !
Daniel
This patch breaks -Werror, because GCC is unhappy with the initializer for ifreq.
The follow change makes GCC happy again:
diff --git a/src/bridge.c b/src/bridge.c index ec37192..6480a35 100644 --- a/src/bridge.c +++ b/src/bridge.c @@ -465,7 +465,7 @@ brAddTap(brControl *ctl, int *tapfd) { int fd, len; - struct ifreq ifr = {0}; + struct ifreq ifr = {{{0}}, {{0, {0}}}};
AFAIR, this works?
struct ifreq ifr = {0,};
Cheers, Mark.
No, it doesn't, I tested it. The problem is the internal structure of ifreq. GCC complains until the initializer matches this structure. Or use memset like all other bridge functions do: struct ifreq ifr; memset(&ifr, 0, sizeof(struct ifreq)); Matthias
On Thu, Jul 30, 2009 at 12:04:40PM +0200, Matthias Bolte wrote:
2009/7/30 Mark McLoughlin <markmc@redhat.com>:
AFAIR, this works?
struct ifreq ifr = {0,};
Cheers, Mark.
No, it doesn't, I tested it. The problem is the internal structure of ifreq. GCC complains until the initializer matches this structure. Or use memset like all other bridge functions do:
struct ifreq ifr; memset(&ifr, 0, sizeof(struct ifreq));
Anyway I commited the switch back to memset(), so all set IMHO :-) Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
Matthias Bolte wrote: [Thu Jul 30 2009, 06:04:40AM EDT]
2009/7/30 Mark McLoughlin <markmc@redhat.com>:
On Thu, 2009-07-30 at 02:58 +0200, Matthias Bolte wrote:
The follow change makes GCC happy again:
- struct ifreq ifr = {0}; + struct ifreq ifr = {{{0}}, {{0, {0}}}};
AFAIR, this works?
struct ifreq ifr = {0,};
Cheers, Mark.
No, it doesn't, I tested it. The problem is the internal structure of ifreq. GCC complains until the initializer matches this structure. Or use memset like all other bridge functions do:
struct ifreq ifr; memset(&ifr, 0, sizeof(struct ifreq));
It's unfortunate, really... In general, one would prefer to ask the compiler for a zeroed structure on the stack than to call memset, which clutters the code and reduces the opportunity for the compiler to optimize. For arrays, {0} works with -Wall -Werror; the unspecified elements are zeroed. But it looks like there's no simple zero-initializer for structs. Aron
participants (4)
-
Aron Griffis -
Daniel Veillard -
Mark McLoughlin -
Matthias Bolte