[libvirt] [PATCH 0/4] FreeBSD: netdev support
Add support for some essential netdev functions. Roman Bogorodskiy (4): FreeBSD: implement virNetDevExists() and virNetDevSetName() FreeBSD: implement virNetDevTapCreate() and virNetDevTapDelete(). FreeBSD: implement virNetDevSetMAC(). FreeBSD: implement virNetDevGetMTU(). src/util/virnetdev.c | 113 ++++++++++++++++++++++++++++++++++++-- src/util/virnetdevtap.c | 140 ++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 247 insertions(+), 6 deletions(-) -- 1.8.0
--- src/util/virnetdev.c | 37 ++++++++++++++++++++++++++++++++++--- 1 file changed, 34 insertions(+), 3 deletions(-) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 7ffd3c2..c7eeb50 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -83,10 +83,32 @@ static int virNetDevSetupControl(const char *ifname, { return virNetDevSetupControlFull(ifname, ifr, AF_PACKET, SOCK_DGRAM); } -#endif +#elif defined(__FreeBSD__) +static int virNetDevSetupControl(const char *ifname, + struct ifreq *ifr) +{ + int s; + memset(ifr, 0, sizeof(*ifr)); -#if defined(SIOCGIFFLAGS) && defined(HAVE_STRUCT_IFREQ) + if (virStrcpyStatic(ifr->ifr_name, ifname) == NULL) { + virReportSystemError(ERANGE, + _("Network interface name '%s' is too long"), + ifname); + return -1; + } + + if ((s = socket(AF_LOCAL, SOCK_DGRAM, 0)) < 0) { + virReportSystemError(errno, "%s", + _("Cannot open network interface control socket")); + return -1; + } + + return s; +} +#endif + +#if defined(SIOCGIFFLAGS) && (defined(HAVE_STRUCT_IFREQ) || defined(__FreeBSD__)) /** * virNetDevExists: * @ifname @@ -105,7 +127,12 @@ int virNetDevExists(const char *ifname) return -1; if (ioctl(fd, SIOCGIFFLAGS, &ifr)) { + /* FreeBSD throws ENXIO when interface doesn't exist */ +#if defined(__FreeBSD__) + if (errno == ENXIO) +#else if (errno == ENODEV) +#endif ret = 0; else virReportSystemError(errno, @@ -459,7 +486,7 @@ int virNetDevSetNamespace(const char *ifname, pid_t pidInNs) return rc; } -#if defined(SIOCSIFNAME) && defined(HAVE_STRUCT_IFREQ) +#if defined(SIOCSIFNAME) && (defined(HAVE_STRUCT_IFREQ) || defined(__FreeBSD__)) /** * virNetDevSetName: * @ifname: name of device @@ -478,12 +505,16 @@ int virNetDevSetName(const char* ifname, const char *newifname) if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0) return -1; +#if defined(__FreeBSD__) + ifr.ifr_data = newifname; +#else if (virStrcpyStatic(ifr.ifr_newname, newifname) == NULL) { virReportSystemError(ERANGE, _("Network interface name '%s' is too long"), newifname); goto cleanup; } +#endif if (ioctl(fd, SIOCSIFNAME, &ifr)) { virReportSystemError(errno, -- 1.8.0
Not a "regular reviewer" (yet), but I figured I'd give this series a look and provide some feedback... On 01/04/2013 10:00 AM, Roman Bogorodskiy wrote:
--- src/util/virnetdev.c | 37 ++++++++++++++++++++++++++++++++++--- 1 file changed, 34 insertions(+), 3 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 7ffd3c2..c7eeb50 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -83,10 +83,32 @@ static int virNetDevSetupControl(const char *ifname, { return virNetDevSetupControlFull(ifname, ifr, AF_PACKET, SOCK_DGRAM); } -#endif +#elif defined(__FreeBSD__) +static int virNetDevSetupControl(const char *ifname, + struct ifreq *ifr) +{
Why not use virNetDevSetupControlFull() passing AF_LOCAL, SOCK_DGRAM? Does the virSetInherit() not apply for FreeBSD?
+ int s;
+ memset(ifr, 0, sizeof(*ifr));
-#if defined(SIOCGIFFLAGS) && defined(HAVE_STRUCT_IFREQ) + if (virStrcpyStatic(ifr->ifr_name, ifname) == NULL) { + virReportSystemError(ERANGE, + _("Network interface name '%s' is too long"), + ifname); + return -1; + } + + if ((s = socket(AF_LOCAL, SOCK_DGRAM, 0)) < 0) { + virReportSystemError(errno, "%s", + _("Cannot open network interface control socket")); + return -1; + } + + return s; +} +#endif + +#if defined(SIOCGIFFLAGS) && (defined(HAVE_STRUCT_IFREQ) || defined(__FreeBSD__)) /** * virNetDevExists: * @ifname @@ -105,7 +127,12 @@ int virNetDevExists(const char *ifname) return -1;
if (ioctl(fd, SIOCGIFFLAGS, &ifr)) { + /* FreeBSD throws ENXIO when interface doesn't exist */ +#if defined(__FreeBSD__) + if (errno == ENXIO) +#else if (errno == ENODEV) +#endif ret = 0; else virReportSystemError(errno, @@ -459,7 +486,7 @@ int virNetDevSetNamespace(const char *ifname, pid_t pidInNs) return rc; }
-#if defined(SIOCSIFNAME) && defined(HAVE_STRUCT_IFREQ) +#if defined(SIOCSIFNAME) && (defined(HAVE_STRUCT_IFREQ) || defined(__FreeBSD__)) /** * virNetDevSetName: * @ifname: name of device @@ -478,12 +505,16 @@ int virNetDevSetName(const char* ifname, const char *newifname) if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0) return -1;
+#if defined(__FreeBSD__) + ifr.ifr_data = newifname; +#else if (virStrcpyStatic(ifr.ifr_newname, newifname) == NULL) { virReportSystemError(ERANGE, _("Network interface name '%s' is too long"), newifname); goto cleanup; } +#endif
if (ioctl(fd, SIOCSIFNAME, &ifr)) { virReportSystemError(errno,
--- src/util/virnetdevtap.c | 140 ++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 137 insertions(+), 3 deletions(-) diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c index a884de1..426d629 100644 --- a/src/util/virnetdevtap.c +++ b/src/util/virnetdevtap.c @@ -103,7 +103,7 @@ virNetDevProbeVnetHdr(int tapfd) #endif -#ifdef TUNSETIFF +#if defined(TUNSETIFF) /** * virNetDevTapCreate: * @ifname: the interface name @@ -230,7 +230,141 @@ cleanup: VIR_FORCE_CLOSE(fd); return ret; } -#else /* ! TUNSETIFF */ +#elif defined(__FreeBSD__) +int virNetDevTapCreate(char **ifname, + int *tapfd, + unsigned int flags ATTRIBUTE_UNUSED) +{ + int s; + struct ifreq ifr; + int ret = -1; + + s = socket(AF_LOCAL, SOCK_DGRAM, 0); + if (s < 0) { + virReportSystemError(errno, "%s", + _("Unable to create socket")); + return -1; + } + + memset(&ifr, 0, sizeof(ifr)); + + if (virStrcpyStatic(ifr.ifr_name, "tap") == NULL) { + virReportSystemError(ERANGE, + _("Network interface name '%s' is too long"), + *ifname); + goto cleanup; + + } + + if (ioctl(s, SIOCIFCREATE2, &ifr) < 0) { + virReportSystemError(errno, + _("Unable to create tap device %s"), + NULLSTR(*ifname)); + goto cleanup; + } + + char *newifname = NULL; + if (strstr(*ifname, "%d") == NULL) { + newifname = *ifname; + } else { + int i; + for (i = 0; i <= 255; i++) { + virBuffer newname = VIR_BUFFER_INITIALIZER; + virBufferAsprintf(&newname, *ifname, i); + + if (virBufferError(&newname)) { + virBufferFreeAndReset(&newname); + virReportOOMError(); + ret = - 1; + goto cleanup; + } + + if (virNetDevExists(virBufferCurrentContent(&newname)) == 0) { + newifname = strdup(virBufferContentAndReset(&newname)); + break; + } + } + } + + if (newifname == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to generate new name for interface %s"), + ifr.ifr_name); + ret = -1; + goto cleanup; + } + + if (virNetDevSetName(ifr.ifr_name, newifname) == -1) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to rename interface %s to %s"), + ifr.ifr_name, newifname); + ret = -1; + goto cleanup; + } + + *ifname = newifname; + + if (tapfd) { + char *dev_prefix = "/dev/"; + char *dev_path; + int dev_path_len = strlen(dev_prefix) + strlen(*ifname) + 1; + + if (VIR_ALLOC_N(dev_path, dev_path_len) < 0) { + virReportOOMError(); + ret = -1; + goto cleanup; + } + snprintf(dev_path, dev_path_len, "%s%s", dev_prefix, *ifname); + + *tapfd = open(dev_path, O_RDWR); + + VIR_FREE(dev_path); + } + + ret = 0; +cleanup: + if (ret < 0) + VIR_FORCE_CLOSE(s); + + return ret; +} + +int virNetDevTapDelete(const char *ifname) +{ + int s; + struct ifreq ifr; + int ret = -1; + + s = socket(AF_LOCAL, SOCK_DGRAM, 0); + if (s < 0) { + virReportSystemError(errno, "%s", + _("Unable to create socket")); + return -1; + } + + memset(&ifr, 0, sizeof(ifr)); + + if (virStrcpyStatic(ifr.ifr_name, ifname) == NULL) { + virReportSystemError(ERANGE, + _("Network interface name '%s' is too long"), + ifname); + goto cleanup; + } + + if (ioctl(s, SIOCIFDESTROY, &ifr) < 0) { + virReportSystemError(errno, + _("Unable to remove tap device %s"), + ifname); + goto cleanup; + } + + ret = 0; +cleanup: + VIR_FORCE_CLOSE(s); + return ret; +} + +#else int virNetDevTapCreate(char **ifname ATTRIBUTE_UNUSED, int *tapfd ATTRIBUTE_UNUSED, unsigned int flags ATTRIBUTE_UNUSED) @@ -245,7 +379,7 @@ int virNetDevTapDelete(const char *ifname ATTRIBUTE_UNUSED) _("Unable to delete TAP devices on this platform")); return -1; } -#endif /* ! TUNSETIFF */ +#endif /** -- 1.8.0
On 01/04/2013 10:00 AM, Roman Bogorodskiy wrote:
--- src/util/virnetdevtap.c | 140 ++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 137 insertions(+), 3 deletions(-)
diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c index a884de1..426d629 100644 --- a/src/util/virnetdevtap.c +++ b/src/util/virnetdevtap.c @@ -103,7 +103,7 @@ virNetDevProbeVnetHdr(int tapfd) #endif
-#ifdef TUNSETIFF +#if defined(TUNSETIFF) /** * virNetDevTapCreate: * @ifname: the interface name @@ -230,7 +230,141 @@ cleanup: VIR_FORCE_CLOSE(fd); return ret; } -#else /* ! TUNSETIFF */
Change the comment to add the !defined(TUNSETIFF)
+#elif defined(__FreeBSD__) +int virNetDevTapCreate(char **ifname, + int *tapfd, + unsigned int flags ATTRIBUTE_UNUSED) +{ + int s; + struct ifreq ifr; + int ret = -1; + + s = socket(AF_LOCAL, SOCK_DGRAM, 0); + if (s < 0) { + virReportSystemError(errno, "%s", + _("Unable to create socket")); + return -1; + } + + memset(&ifr, 0, sizeof(ifr)); + + if (virStrcpyStatic(ifr.ifr_name, "tap") == NULL) { + virReportSystemError(ERANGE, + _("Network interface name '%s' is too long"), + *ifname); + goto cleanup; + + }
Confused here - you are copying "tap", but using "*ifname" in the error message. Why not use the virNetDevSetupControl() if in fact you are trying to use "*ifname"?
+ + if (ioctl(s, SIOCIFCREATE2, &ifr) < 0) { + virReportSystemError(errno, + _("Unable to create tap device %s"), + NULLSTR(*ifname)); + goto cleanup; + } + + char *newifname = NULL;
Put this with the other declarations.
+ if (strstr(*ifname, "%d") == NULL) { + newifname = *ifname; + } else { + int i; + for (i = 0; i <= 255; i++) { + virBuffer newname = VIR_BUFFER_INITIALIZER; + virBufferAsprintf(&newname, *ifname, i); + + if (virBufferError(&newname)) { + virBufferFreeAndReset(&newname); + virReportOOMError(); + ret = - 1;
Redundant with initialization
+ goto cleanup; + } + + if (virNetDevExists(virBufferCurrentContent(&newname)) == 0) { + newifname = strdup(virBufferContentAndReset(&newname)); + break; + }
In this instance memory is allocated for newifname, when is it free()'d? Secondarily if not allocated, then the general message below may mask the reason. That is although you failed to generate a new name, it was because you were out of memory.
+ } + } + + if (newifname == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to generate new name for interface %s"), + ifr.ifr_name); + ret = -1;
Redundant with initialization
+ goto cleanup; + } + + if (virNetDevSetName(ifr.ifr_name, newifname) == -1) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to rename interface %s to %s"), + ifr.ifr_name, newifname);
This message is redundant with the virNetDevSetName() function.
+ ret = -1;
Redundant with initialization
+ goto cleanup; + } + + *ifname = newifname;
This is where a possible memory leak occurs if *ifname had been alloc'd before we'd be now losing it. I see the other virNetDevTapCreate() will VIR_FREE(*ifname) prior to strdup() attempt. Secondarily, it seems to me that if we go through the "if strstr() == NULL" path, then we're not really changing the name, correct? If that's the case, then the only time you need to change the name is in the else condition. I think the answer probably goes back to the virStrcpyStatic using "tap" above for what the expectations are here.
+ + if (tapfd) { + char *dev_prefix = "/dev/"; + char *dev_path; + int dev_path_len = strlen(dev_prefix) + strlen(*ifname) + 1; + + if (VIR_ALLOC_N(dev_path, dev_path_len) < 0) { + virReportOOMError(); + ret = -1;
Redundant with initialization
+ goto cleanup; + } + snprintf(dev_path, dev_path_len, "%s%s", dev_prefix, *ifname); + + *tapfd = open(dev_path, O_RDWR); + + VIR_FREE(dev_path); + } + + ret = 0; +cleanup: + if (ret < 0) + VIR_FORCE_CLOSE(s); + + return ret; +} + +int virNetDevTapDelete(const char *ifname) +{ + int s; + struct ifreq ifr; + int ret = -1; + + s = socket(AF_LOCAL, SOCK_DGRAM, 0); + if (s < 0) { + virReportSystemError(errno, "%s", + _("Unable to create socket")); + return -1; + } + + memset(&ifr, 0, sizeof(ifr)); + + if (virStrcpyStatic(ifr.ifr_name, ifname) == NULL) { + virReportSystemError(ERANGE, + _("Network interface name '%s' is too long"), + ifname); + goto cleanup; + }
Why not use virNetDevSetupControl()?
+ + if (ioctl(s, SIOCIFDESTROY, &ifr) < 0) { + virReportSystemError(errno, + _("Unable to remove tap device %s"), + ifname); + goto cleanup; + } + + ret = 0; +cleanup: + VIR_FORCE_CLOSE(s); + return ret; +} + +#else int virNetDevTapCreate(char **ifname ATTRIBUTE_UNUSED, int *tapfd ATTRIBUTE_UNUSED, unsigned int flags ATTRIBUTE_UNUSED) @@ -245,7 +379,7 @@ int virNetDevTapDelete(const char *ifname ATTRIBUTE_UNUSED) _("Unable to delete TAP devices on this platform")); return -1; } -#endif /* ! TUNSETIFF */ +#endif
Add a /* defined(__FreeBSD__) */ to the #endif for clarity
/**
John Ferlan wrote:
On 01/04/2013 10:00 AM, Roman Bogorodskiy wrote:
--- src/util/virnetdevtap.c | 140 ++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 137 insertions(+), 3 deletions(-)
diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c index a884de1..426d629 100644 --- a/src/util/virnetdevtap.c +++ b/src/util/virnetdevtap.c @@ -103,7 +103,7 @@ virNetDevProbeVnetHdr(int tapfd) #endif
-#ifdef TUNSETIFF +#if defined(TUNSETIFF) /** * virNetDevTapCreate: * @ifname: the interface name @@ -230,7 +230,141 @@ cleanup: VIR_FORCE_CLOSE(fd); return ret; } -#else /* ! TUNSETIFF */
Change the comment to add the !defined(TUNSETIFF)
+#elif defined(__FreeBSD__) +int virNetDevTapCreate(char **ifname, + int *tapfd, + unsigned int flags ATTRIBUTE_UNUSED) +{ + int s; + struct ifreq ifr; + int ret = -1; + + s = socket(AF_LOCAL, SOCK_DGRAM, 0); + if (s < 0) { + virReportSystemError(errno, "%s", + _("Unable to create socket")); + return -1; + } + + memset(&ifr, 0, sizeof(ifr)); + + if (virStrcpyStatic(ifr.ifr_name, "tap") == NULL) { + virReportSystemError(ERANGE, + _("Network interface name '%s' is too long"), + *ifname); + goto cleanup; + + }
Confused here - you are copying "tap", but using "*ifname" in the error message. Why not use the virNetDevSetupControl() if in fact you are trying to use "*ifname"?
Please see a commit above.
+ + if (ioctl(s, SIOCIFCREATE2, &ifr) < 0) { + virReportSystemError(errno, + _("Unable to create tap device %s"), + NULLSTR(*ifname)); + goto cleanup; + } + + char *newifname = NULL;
Put this with the other declarations.
+ if (strstr(*ifname, "%d") == NULL) { + newifname = *ifname; + } else { + int i; + for (i = 0; i <= 255; i++) { + virBuffer newname = VIR_BUFFER_INITIALIZER; + virBufferAsprintf(&newname, *ifname, i); + + if (virBufferError(&newname)) { + virBufferFreeAndReset(&newname); + virReportOOMError(); + ret = - 1;
Redundant with initialization
+ goto cleanup; + } + + if (virNetDevExists(virBufferCurrentContent(&newname)) == 0) { + newifname = strdup(virBufferContentAndReset(&newname)); + break; + }
In this instance memory is allocated for newifname, when is it free()'d? Secondarily if not allocated, then the general message below may mask the reason. That is although you failed to generate a new name, it was because you were out of memory.
+ } + } + + if (newifname == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to generate new name for interface %s"), + ifr.ifr_name); + ret = -1;
Redundant with initialization
+ goto cleanup; + } + + if (virNetDevSetName(ifr.ifr_name, newifname) == -1) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to rename interface %s to %s"), + ifr.ifr_name, newifname);
This message is redundant with the virNetDevSetName() function.
+ ret = -1;
Redundant with initialization
+ goto cleanup; + } + + *ifname = newifname;
This is where a possible memory leak occurs if *ifname had been alloc'd before we'd be now losing it. I see the other virNetDevTapCreate() will VIR_FREE(*ifname) prior to strdup() attempt.
Secondarily, it seems to me that if we go through the "if strstr() == NULL" path, then we're not really changing the name, correct? If that's the case, then the only time you need to change the name is in the else condition.
I think the answer probably goes back to the virStrcpyStatic using "tap" above for what the expectations are here.
Hi, Sorry for the late reply, it's been a busy week. The idea behind this code is the following: FreeBSD determines a type of the interface we're creating by its name, so it's not possible to create a TAP device with name "vnet" right away. Here, if we were requested to create an interface 'vnet%d', we pass "tap" interface name to create a TAP interface with the first available name (e.g. if we already have tap0 and tap1, the new one will be tap2). Then we need to find a "vnet%d" name we can use for an interface. I wasn't able to find a better way but going from vnet0, then vnet1 etc. The final step would be to rename the TAP interface we just created to the new 'vnet' name. In case fif we were passed a full interface name (e.g. 'vnet10') instead if a pattern (e.g. 'vnet%d'), we don't need to find a proper name and can just rename our tap interface right away. I will need to add a comment for that piece in the code and state that in commit message as well. And you're correct regarding the leak, I'll take that into account.
+ + if (tapfd) { + char *dev_prefix = "/dev/"; + char *dev_path; + int dev_path_len = strlen(dev_prefix) + strlen(*ifname) + 1; + + if (VIR_ALLOC_N(dev_path, dev_path_len) < 0) { + virReportOOMError(); + ret = -1;
Redundant with initialization
+ goto cleanup; + } + snprintf(dev_path, dev_path_len, "%s%s", dev_prefix, *ifname); + + *tapfd = open(dev_path, O_RDWR); + + VIR_FREE(dev_path); + } + + ret = 0; +cleanup: + if (ret < 0) + VIR_FORCE_CLOSE(s); + + return ret; +} + +int virNetDevTapDelete(const char *ifname) +{ + int s; + struct ifreq ifr; + int ret = -1; + + s = socket(AF_LOCAL, SOCK_DGRAM, 0); + if (s < 0) { + virReportSystemError(errno, "%s", + _("Unable to create socket")); + return -1; + } + + memset(&ifr, 0, sizeof(ifr)); + + if (virStrcpyStatic(ifr.ifr_name, ifname) == NULL) { + virReportSystemError(ERANGE, + _("Network interface name '%s' is too long"), + ifname); + goto cleanup; + }
Why not use virNetDevSetupControl()?
Unfortunately, it's declared static, so we cannot re-use it here. Thanks for the review, I'm working on fixing issues you pointed out.
+ + if (ioctl(s, SIOCIFDESTROY, &ifr) < 0) { + virReportSystemError(errno, + _("Unable to remove tap device %s"), + ifname); + goto cleanup; + } + + ret = 0; +cleanup: + VIR_FORCE_CLOSE(s); + return ret; +} + +#else int virNetDevTapCreate(char **ifname ATTRIBUTE_UNUSED, int *tapfd ATTRIBUTE_UNUSED, unsigned int flags ATTRIBUTE_UNUSED) @@ -245,7 +379,7 @@ int virNetDevTapDelete(const char *ifname ATTRIBUTE_UNUSED) _("Unable to delete TAP devices on this platform")); return -1; } -#endif /* ! TUNSETIFF */ +#endif
Add a /* defined(__FreeBSD__) */ to the #endif for clarity
/**
Roman Bogorodskiy
On 01/13/2013 05:51 AM, Roman Bogorodskiy wrote:
Why not use virNetDevSetupControl()?
Unfortunately, it's declared static, so we cannot re-use it here.
But there's nothing wrong with changing it to not be static, adding it to src/libvirt_private.syms, and using it, instead of copying its body. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
--- src/util/virnetdev.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index c7eeb50..6e4f7ad 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -42,6 +42,11 @@ # undef HAVE_STRUCT_IFREQ #endif +#if defined(__FreeBSD__) +# include <sys/sockio.h> +# include <net/if_dl.h> +#endif + #define VIR_FROM_THIS VIR_FROM_NONE #if defined(HAVE_STRUCT_IFREQ) @@ -200,6 +205,51 @@ cleanup: VIR_FORCE_CLOSE(fd); return ret; } +#elif defined(__FreeBSD__) +int virNetDevSetMAC(const char *ifname, + const virMacAddrPtr macaddr) +{ + struct ifreq ifr; + struct sockaddr_dl sdl; + uint8_t mac[19]; + char *macstr; + int s; + int ret = -1; + + if ((s = virNetDevSetupControl(ifname, &ifr)) < 0) + return -1; + + if (VIR_ALLOC_N(macstr, VIR_MAC_STRING_BUFLEN) < 0) { + virReportOOMError(); + ret = - 1; + goto cleanup; + } + virMacAddrFormat(&macaddr, macstr); + + virStrncpy(ifr.ifr_name, ifname, IFNAMSIZ, sizeof(ifr.ifr_name)); + memset(mac, 0, sizeof(mac)); + mac[0] = ':'; + virStrncpy(mac + 1, macstr, strlen(macstr), sizeof(mac)); + sdl.sdl_len = sizeof(sdl); + link_addr(mac, &sdl); + + bcopy(sdl.sdl_data, ifr.ifr_addr.sa_data, 6); + ifr.ifr_addr.sa_len = 6; + + if (ioctl(s, SIOCSIFLLADDR, &ifr) < 0) { + virReportSystemError(errno, + _("Cannot set interface MAC on '%s'"), + ifname); + goto cleanup; + } + + ret = 0; +cleanup: + VIR_FREE(macstr); + VIR_FORCE_CLOSE(s); + + return ret; +} #else int virNetDevSetMAC(const char *ifname, const virMacAddrPtr macaddr ATTRIBUTE_UNUSED) -- 1.8.0
On 01/04/2013 10:00 AM, Roman Bogorodskiy wrote:
--- src/util/virnetdev.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index c7eeb50..6e4f7ad 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -42,6 +42,11 @@ # undef HAVE_STRUCT_IFREQ #endif
+#if defined(__FreeBSD__) +# include <sys/sockio.h> +# include <net/if_dl.h> +#endif + #define VIR_FROM_THIS VIR_FROM_NONE
#if defined(HAVE_STRUCT_IFREQ) @@ -200,6 +205,51 @@ cleanup: VIR_FORCE_CLOSE(fd); return ret; } +#elif defined(__FreeBSD__) +int virNetDevSetMAC(const char *ifname, + const virMacAddrPtr macaddr) +{ + struct ifreq ifr; + struct sockaddr_dl sdl; + uint8_t mac[19];
19 was odd to me... I found VIR_MAC_STRING_BUFLEN (which is essentially 18) using cscope. Figured it would be better to go that route - it does make it easier to find code later on that's adjust the MAC
+ char *macstr; + int s; + int ret = -1; + + if ((s = virNetDevSetupControl(ifname, &ifr)) < 0) + return -1; + + if (VIR_ALLOC_N(macstr, VIR_MAC_STRING_BUFLEN) < 0) { + virReportOOMError(); + ret = - 1;
Redundant with the initialization
+ goto cleanup; + } + virMacAddrFormat(&macaddr, macstr); + + virStrncpy(ifr.ifr_name, ifname, IFNAMSIZ, sizeof(ifr.ifr_name));
Ignoring the status of the virStrncpy()
+ memset(mac, 0, sizeof(mac)); + mac[0] = ':'; + virStrncpy(mac + 1, macstr, strlen(macstr), sizeof(mac));
Ignoring the status of the virStrncpy()
+ sdl.sdl_len = sizeof(sdl); + link_addr(mac, &sdl); + + bcopy(sdl.sdl_data, ifr.ifr_addr.sa_data, 6); + ifr.ifr_addr.sa_len = 6; + + if (ioctl(s, SIOCSIFLLADDR, &ifr) < 0) { + virReportSystemError(errno, + _("Cannot set interface MAC on '%s'"), + ifname); + goto cleanup; + } + + ret = 0; +cleanup: + VIR_FREE(macstr); + VIR_FORCE_CLOSE(s); + + return ret; +} #else int virNetDevSetMAC(const char *ifname, const virMacAddrPtr macaddr ATTRIBUTE_UNUSED)
--- src/util/virnetdev.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 6e4f7ad..809c0f7 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -427,6 +427,32 @@ cleanup: VIR_FORCE_CLOSE(fd); return ret; } +#elif defined(__FreeBSD__) +int virNetDevGetMTU(const char *ifname) +{ + int s; + int ret; + struct ifreq ifr; + + if ((s = virNetDevSetupControl(ifname, &ifr)) < 0) + return -1; + + ifr.ifr_addr.sa_family = AF_INET; + virStrcpyStatic(ifr.ifr_name, ifname); + if (ioctl(s, SIOCGIFMTU, (caddr_t)&ifr) < 0) { + virReportSystemError(errno, + _("Cannot get interface MTU on '%s'"), + ifname); + ret = -1; + goto cleanup; + } + + ret = ifr.ifr_mtu; + +cleanup: + VIR_FORCE_CLOSE(s); + return ret; +} #else int virNetDevGetMTU(const char *ifname) { -- 1.8.0
On 01/04/2013 10:00 AM, Roman Bogorodskiy wrote:
--- src/util/virnetdev.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 6e4f7ad..809c0f7 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -427,6 +427,32 @@ cleanup: VIR_FORCE_CLOSE(fd); return ret; } +#elif defined(__FreeBSD__) +int virNetDevGetMTU(const char *ifname) +{ + int s; + int ret; + struct ifreq ifr; + + if ((s = virNetDevSetupControl(ifname, &ifr)) < 0) + return -1; + + ifr.ifr_addr.sa_family = AF_INET; + virStrcpyStatic(ifr.ifr_name, ifname);
Ignoring the result of the virStrcpyStatic() call.
+ if (ioctl(s, SIOCGIFMTU, (caddr_t)&ifr) < 0) { + virReportSystemError(errno, + _("Cannot get interface MTU on '%s'"), + ifname); + ret = -1; + goto cleanup; + } + + ret = ifr.ifr_mtu; + +cleanup: + VIR_FORCE_CLOSE(s); + return ret; +} #else int virNetDevGetMTU(const char *ifname) {
Updated patches that (hopefully) address issues noted by John Ferlan. This includes one additional patch to make virNetDevSetupControl() a public function. Roman Bogorodskiy (5): FreeBSD: implement virNetDevExists() and virNetDevSetName(). Make virNetDevSetupControl() public. FreeBSD: implement virNetDevTapCreate() and virNetDevTapDelete(). FreeBSD: implement virNetDevSetMAC(). FreeBSD: implement virNetDevGetMTU(). src/libvirt_private.syms | 1 + src/util/virnetdev.c | 103 ++++++++++++++++++++++++++++++++++++++++--- src/util/virnetdev.h | 6 +++ src/util/virnetdevtap.c | 112 +++++++++++++++++++++++++++++++++++++++++++++-- 4 files changed, 213 insertions(+), 9 deletions(-) -- 1.8.0
--- src/util/virnetdev.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 7ffd3c2..dae267b 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -44,7 +44,7 @@ #define VIR_FROM_THIS VIR_FROM_NONE -#if defined(HAVE_STRUCT_IFREQ) +#if defined(HAVE_STRUCT_IFREQ) || defined(__FreeBSD__) static int virNetDevSetupControlFull(const char *ifname, struct ifreq *ifr, int domain, @@ -81,12 +81,15 @@ static int virNetDevSetupControlFull(const char *ifname, static int virNetDevSetupControl(const char *ifname, struct ifreq *ifr) { +#if defined(__FreeBSD__) + return virNetDevSetupControlFull(ifname, ifr, AF_LOCAL, SOCK_DGRAM); +#else return virNetDevSetupControlFull(ifname, ifr, AF_PACKET, SOCK_DGRAM); +#endif } #endif - -#if defined(SIOCGIFFLAGS) && defined(HAVE_STRUCT_IFREQ) +#if defined(SIOCGIFFLAGS) && (defined(HAVE_STRUCT_IFREQ) || defined(__FreeBSD__)) /** * virNetDevExists: * @ifname @@ -105,7 +108,12 @@ int virNetDevExists(const char *ifname) return -1; if (ioctl(fd, SIOCGIFFLAGS, &ifr)) { + /* FreeBSD throws ENXIO when interface doesn't exist */ +#if defined(__FreeBSD__) + if (errno == ENXIO) +#else if (errno == ENODEV) +#endif ret = 0; else virReportSystemError(errno, @@ -459,7 +467,7 @@ int virNetDevSetNamespace(const char *ifname, pid_t pidInNs) return rc; } -#if defined(SIOCSIFNAME) && defined(HAVE_STRUCT_IFREQ) +#if defined(SIOCSIFNAME) && (defined(HAVE_STRUCT_IFREQ) || defined(__FreeBSD__)) /** * virNetDevSetName: * @ifname: name of device @@ -478,12 +486,16 @@ int virNetDevSetName(const char* ifname, const char *newifname) if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0) return -1; +#if defined(__FreeBSD__) + ifr.ifr_data = newifname; +#else if (virStrcpyStatic(ifr.ifr_newname, newifname) == NULL) { virReportSystemError(ERANGE, _("Network interface name '%s' is too long"), newifname); goto cleanup; } +#endif if (ioctl(fd, SIOCSIFNAME, &ifr)) { virReportSystemError(errno, -- 1.8.0
On 01/20/2013 11:22 AM, Roman Bogorodskiy wrote:
--- src/util/virnetdev.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-)
ACK
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 7ffd3c2..dae267b 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -44,7 +44,7 @@
#define VIR_FROM_THIS VIR_FROM_NONE
-#if defined(HAVE_STRUCT_IFREQ) +#if defined(HAVE_STRUCT_IFREQ) || defined(__FreeBSD__) static int virNetDevSetupControlFull(const char *ifname, struct ifreq *ifr, int domain, @@ -81,12 +81,15 @@ static int virNetDevSetupControlFull(const char *ifname, static int virNetDevSetupControl(const char *ifname, struct ifreq *ifr) { +#if defined(__FreeBSD__) + return virNetDevSetupControlFull(ifname, ifr, AF_LOCAL, SOCK_DGRAM); +#else return virNetDevSetupControlFull(ifname, ifr, AF_PACKET, SOCK_DGRAM); +#endif } #endif
- -#if defined(SIOCGIFFLAGS) && defined(HAVE_STRUCT_IFREQ) +#if defined(SIOCGIFFLAGS) && (defined(HAVE_STRUCT_IFREQ) || defined(__FreeBSD__)) /** * virNetDevExists: * @ifname @@ -105,7 +108,12 @@ int virNetDevExists(const char *ifname) return -1;
if (ioctl(fd, SIOCGIFFLAGS, &ifr)) { + /* FreeBSD throws ENXIO when interface doesn't exist */ +#if defined(__FreeBSD__) + if (errno == ENXIO) +#else if (errno == ENODEV) +#endif ret = 0; else virReportSystemError(errno, @@ -459,7 +467,7 @@ int virNetDevSetNamespace(const char *ifname, pid_t pidInNs) return rc; }
-#if defined(SIOCSIFNAME) && defined(HAVE_STRUCT_IFREQ) +#if defined(SIOCSIFNAME) && (defined(HAVE_STRUCT_IFREQ) || defined(__FreeBSD__)) /** * virNetDevSetName: * @ifname: name of device @@ -478,12 +486,16 @@ int virNetDevSetName(const char* ifname, const char *newifname) if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0) return -1;
+#if defined(__FreeBSD__) + ifr.ifr_data = newifname; +#else if (virStrcpyStatic(ifr.ifr_newname, newifname) == NULL) { virReportSystemError(ERANGE, _("Network interface name '%s' is too long"), newifname); goto cleanup; } +#endif
if (ioctl(fd, SIOCSIFNAME, &ifr)) { virReportSystemError(errno,
It's useful not only inside virnetdev. --- src/libvirt_private.syms | 1 + src/util/virnetdev.c | 7 ++++--- src/util/virnetdev.h | 6 ++++++ 3 files changed, 11 insertions(+), 3 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fc23adc..84eeebc 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1515,6 +1515,7 @@ virNetDevSetMTUFromDevice; virNetDevSetName; virNetDevSetNamespace; virNetDevSetOnline; +virNetDevSetupControl; virNetDevValidateConfig; diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index dae267b..5100467 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -44,7 +44,7 @@ #define VIR_FROM_THIS VIR_FROM_NONE -#if defined(HAVE_STRUCT_IFREQ) || defined(__FreeBSD__) +#if (defined(HAVE_STRUCT_IFREQ) || defined(__FreeBSD__)) static int virNetDevSetupControlFull(const char *ifname, struct ifreq *ifr, int domain, @@ -78,8 +78,8 @@ static int virNetDevSetupControlFull(const char *ifname, } -static int virNetDevSetupControl(const char *ifname, - struct ifreq *ifr) +int virNetDevSetupControl(const char *ifname, + struct ifreq *ifr) { #if defined(__FreeBSD__) return virNetDevSetupControlFull(ifname, ifr, AF_LOCAL, SOCK_DGRAM); @@ -87,6 +87,7 @@ static int virNetDevSetupControl(const char *ifname, return virNetDevSetupControlFull(ifname, ifr, AF_PACKET, SOCK_DGRAM); #endif } +#else /* ! (defined(HAVE_STRUCT_IFREQ) || defined(__FreeBSD__)) */ #endif #if defined(SIOCGIFFLAGS) && (defined(HAVE_STRUCT_IFREQ) || defined(__FreeBSD__)) diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h index d588e89..63871cc 100644 --- a/src/util/virnetdev.h +++ b/src/util/virnetdev.h @@ -28,6 +28,12 @@ # include "virmacaddr.h" # include "virpci.h" +# include <net/if.h> + +int virNetDevSetupControl(const char *ifname, + struct ifreq *ifr) + ATTRIBUTE_RETURN_CHECK; + int virNetDevExists(const char *brname) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; -- 1.8.0
On 01/20/2013 11:22 AM, Roman Bogorodskiy wrote:
It's useful not only inside virnetdev. --- src/libvirt_private.syms | 1 + src/util/virnetdev.c | 7 ++++--- src/util/virnetdev.h | 6 ++++++ 3 files changed, 11 insertions(+), 3 deletions(-)
ACK (although I believe this is all that has to change, hopefully someone with some more time/experience here will look too).
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fc23adc..84eeebc 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1515,6 +1515,7 @@ virNetDevSetMTUFromDevice; virNetDevSetName; virNetDevSetNamespace; virNetDevSetOnline; +virNetDevSetupControl; virNetDevValidateConfig;
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index dae267b..5100467 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -44,7 +44,7 @@
#define VIR_FROM_THIS VIR_FROM_NONE
-#if defined(HAVE_STRUCT_IFREQ) || defined(__FreeBSD__) +#if (defined(HAVE_STRUCT_IFREQ) || defined(__FreeBSD__)) static int virNetDevSetupControlFull(const char *ifname, struct ifreq *ifr, int domain, @@ -78,8 +78,8 @@ static int virNetDevSetupControlFull(const char *ifname, }
-static int virNetDevSetupControl(const char *ifname, - struct ifreq *ifr) +int virNetDevSetupControl(const char *ifname, + struct ifreq *ifr) { #if defined(__FreeBSD__) return virNetDevSetupControlFull(ifname, ifr, AF_LOCAL, SOCK_DGRAM); @@ -87,6 +87,7 @@ static int virNetDevSetupControl(const char *ifname, return virNetDevSetupControlFull(ifname, ifr, AF_PACKET, SOCK_DGRAM); #endif } +#else /* ! (defined(HAVE_STRUCT_IFREQ) || defined(__FreeBSD__)) */ #endif
#if defined(SIOCGIFFLAGS) && (defined(HAVE_STRUCT_IFREQ) || defined(__FreeBSD__)) diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h index d588e89..63871cc 100644 --- a/src/util/virnetdev.h +++ b/src/util/virnetdev.h @@ -28,6 +28,12 @@ # include "virmacaddr.h" # include "virpci.h"
+# include <net/if.h> + +int virNetDevSetupControl(const char *ifname, + struct ifreq *ifr) + ATTRIBUTE_RETURN_CHECK; + int virNetDevExists(const char *brname) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
On 01/20/2013 11:22 AM, Roman Bogorodskiy wrote:
It's useful not only inside virnetdev. --- src/libvirt_private.syms | 1 + src/util/virnetdev.c | 7 ++++--- src/util/virnetdev.h | 6 ++++++ 3 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fc23adc..84eeebc 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1515,6 +1515,7 @@ virNetDevSetMTUFromDevice; virNetDevSetName; virNetDevSetNamespace; virNetDevSetOnline; +virNetDevSetupControl; virNetDevValidateConfig;
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index dae267b..5100467 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -44,7 +44,7 @@
#define VIR_FROM_THIS VIR_FROM_NONE
-#if defined(HAVE_STRUCT_IFREQ) || defined(__FreeBSD__) +#if (defined(HAVE_STRUCT_IFREQ) || defined(__FreeBSD__))
We actually prefer to not have extraneous parentheses.
static int virNetDevSetupControlFull(const char *ifname, struct ifreq *ifr, int domain, @@ -78,8 +78,8 @@ static int virNetDevSetupControlFull(const char *ifname, }
-static int virNetDevSetupControl(const char *ifname, - struct ifreq *ifr) +int virNetDevSetupControl(const char *ifname, + struct ifreq *ifr)
As long as you're modifying this, you should put the return type on a separate line, which is the preferred style: int virNetDevSetupControl(const char *ifname, struct ifreq *ifr)
{ #if defined(__FreeBSD__) return virNetDevSetupControlFull(ifname, ifr, AF_LOCAL, SOCK_DGRAM); @@ -87,6 +87,7 @@ static int virNetDevSetupControl(const char *ifname, return virNetDevSetupControlFull(ifname, ifr, AF_PACKET, SOCK_DGRAM); #endif } +#else /* ! (defined(HAVE_STRUCT_IFREQ) || defined(__FreeBSD__)) */ #endif
#if defined(SIOCGIFFLAGS) && (defined(HAVE_STRUCT_IFREQ) || defined(__FreeBSD__)) diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h index d588e89..63871cc 100644 --- a/src/util/virnetdev.h +++ b/src/util/virnetdev.h @@ -28,6 +28,12 @@ # include "virmacaddr.h" # include "virpci.h"
+# include <net/if.h> + +int virNetDevSetupControl(const char *ifname, + struct ifreq *ifr) + ATTRIBUTE_RETURN_CHECK; + int virNetDevExists(const char *brname) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
--- src/util/virnetdevtap.c | 112 ++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 109 insertions(+), 3 deletions(-) diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c index a884de1..1b02e1f 100644 --- a/src/util/virnetdevtap.c +++ b/src/util/virnetdevtap.c @@ -103,7 +103,7 @@ virNetDevProbeVnetHdr(int tapfd) #endif -#ifdef TUNSETIFF +#if defined(TUNSETIFF) /** * virNetDevTapCreate: * @ifname: the interface name @@ -230,7 +230,113 @@ cleanup: VIR_FORCE_CLOSE(fd); return ret; } -#else /* ! TUNSETIFF */ +#elif defined(__FreeBSD__) +int virNetDevTapCreate(char **ifname, + int *tapfd, + unsigned int flags ATTRIBUTE_UNUSED) +{ + int s; + struct ifreq ifr; + int ret = -1; + char *newifname = NULL; + + /* As FreeBSD determines interface type by name, + * we have to create 'tap' interface first and + * then rename it to 'vnet' + */ + if ((s = virNetDevSetupControl("tap", &ifr)) < 0) + return -1; + + if (ioctl(s, SIOCIFCREATE2, &ifr) < 0) { + virReportSystemError(errno, "%s", + _("Unable to create tap device")); + goto cleanup; + } + + /* In case we were given exact interface name (e.g. 'vnetN'), + * we just rename to it. If we have format string like + * 'vnet%d', we need to find the first available name that + * matches this pattern + */ + if (strstr(*ifname, "%d") == NULL) { + newifname = strdup(*ifname); + } else { + int i; + for (i = 0; i <= 255; i++) { + virBuffer newname = VIR_BUFFER_INITIALIZER; + virBufferAsprintf(&newname, *ifname, i); + + if (virBufferError(&newname)) { + virBufferFreeAndReset(&newname); + virReportOOMError(); + goto cleanup; + } + + if (virNetDevExists(virBufferCurrentContent(&newname)) == 0) { + newifname = strdup(virBufferContentAndReset(&newname)); + break; + } + } + } + + VIR_FREE(*ifname); + + if (newifname == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to generate new name for interface %s"), + ifr.ifr_name); + goto cleanup; + } + + if (virNetDevSetName(ifr.ifr_name, newifname) == -1) { + goto cleanup; + } + + *ifname = newifname; + + if (tapfd) { + char *dev_path = NULL; + if (virAsprintf(&dev_path, "/dev/%s", *ifname) < 0) { + virReportOOMError(); + goto cleanup; + } + + *tapfd = open(dev_path, O_RDWR); + + VIR_FREE(dev_path); + } + + ret = 0; +cleanup: + if (ret < 0) + VIR_FORCE_CLOSE(s); + + return ret; +} + +int virNetDevTapDelete(const char *ifname) +{ + int s; + struct ifreq ifr; + int ret = -1; + + if ((s = virNetDevSetupControl(ifname, &ifr)) < 0) + return -1; + + if (ioctl(s, SIOCIFDESTROY, &ifr) < 0) { + virReportSystemError(errno, + _("Unable to remove tap device %s"), + ifname); + goto cleanup; + } + + ret = 0; +cleanup: + VIR_FORCE_CLOSE(s); + return ret; +} + +#else /* !defined(__FreeBSD__) */ int virNetDevTapCreate(char **ifname ATTRIBUTE_UNUSED, int *tapfd ATTRIBUTE_UNUSED, unsigned int flags ATTRIBUTE_UNUSED) @@ -245,7 +351,7 @@ int virNetDevTapDelete(const char *ifname ATTRIBUTE_UNUSED) _("Unable to delete TAP devices on this platform")); return -1; } -#endif /* ! TUNSETIFF */ +#endif /** -- 1.8.0
On 01/20/2013 11:22 AM, Roman Bogorodskiy wrote:
--- src/util/virnetdevtap.c | 112 ++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 109 insertions(+), 3 deletions(-)
diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c index a884de1..1b02e1f 100644 --- a/src/util/virnetdevtap.c +++ b/src/util/virnetdevtap.c @@ -103,7 +103,7 @@ virNetDevProbeVnetHdr(int tapfd) #endif
-#ifdef TUNSETIFF +#if defined(TUNSETIFF) /** * virNetDevTapCreate: * @ifname: the interface name @@ -230,7 +230,113 @@ cleanup: VIR_FORCE_CLOSE(fd); return ret; } -#else /* ! TUNSETIFF */ +#elif defined(__FreeBSD__) +int virNetDevTapCreate(char **ifname, + int *tapfd, + unsigned int flags ATTRIBUTE_UNUSED) +{ + int s; + struct ifreq ifr; + int ret = -1; + char *newifname = NULL; + + /* As FreeBSD determines interface type by name, + * we have to create 'tap' interface first and + * then rename it to 'vnet' + */ + if ((s = virNetDevSetupControl("tap", &ifr)) < 0) + return -1; + + if (ioctl(s, SIOCIFCREATE2, &ifr) < 0) { + virReportSystemError(errno, "%s", + _("Unable to create tap device")); + goto cleanup; + }
Trying to figure the relationship between this socket and the file (eg, tapfd) created below).
+ + /* In case we were given exact interface name (e.g. 'vnetN'), + * we just rename to it. If we have format string like + * 'vnet%d', we need to find the first available name that + * matches this pattern + */ + if (strstr(*ifname, "%d") == NULL) { + newifname = strdup(*ifname); + } else { + int i; + for (i = 0; i <= 255; i++) { + virBuffer newname = VIR_BUFFER_INITIALIZER; + virBufferAsprintf(&newname, *ifname, i); + + if (virBufferError(&newname)) { + virBufferFreeAndReset(&newname); + virReportOOMError(); + goto cleanup; + } + + if (virNetDevExists(virBufferCurrentContent(&newname)) == 0) { + newifname = strdup(virBufferContentAndReset(&newname)); + break; + } + } + } + + VIR_FREE(*ifname); + + if (newifname == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to generate new name for interface %s"), + ifr.ifr_name); + goto cleanup; + }
My comments from v1 still apply above. Since the "else" part of the above check is just replacing *ifname, then that's the only time we need to mess with *ifname. That is use "!=" and just replace *ifname at the end of the for loop as long as we generated one. The error message here would be wrong if you went through the existing "if" condition - it failed to strdup(). Thus you have: if (strstr(*ifname, "%d") != NULL) { int i; for (i = 0; i <= 255; i++) { virBuffer newname = VIR_BUFFER_INITIALIZER; virBufferAsprintf(&newname, *ifname, i); if (virBufferError(&newname)) { virBufferFreeAndReset(&newname); virReportOOMError(); goto cleanup; } if (virNetDevExists(virBufferCurrentContent(&newname)) == 0) { newifname = strdup(virBufferContentAndReset(&newname)); break; } } if (newifname) { VIR_FREE(*ifname); *ifname = newifname; } else { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to generate new name for interface %s"), ifr.ifr_name); goto cleanup; } } Also, I missed this the first time around, is there an existing constant for 255? Or is it/will it be dynamic? Beyond that I have some "thoughts" around the use of 255. First, it could be a time consuming loop - there's a lot of open, ioctl/check, close going on (and that doesn't include all the printf & Buffer mgmt code). Second, what are the chances some day someone wants 1024 tap devices. This loop is then outdated and even worse performance wise. It's too bad there isn't a way to just request the next available device and let the driver handle things rather than a linear algorithm which will cause performance problems some day. Yes, I've been down this road before.
+ + if (virNetDevSetName(ifr.ifr_name, newifname) == -1) {
Assuming the above change, then 's/newifname/*ifname/'
+ goto cleanup; + } + + *ifname = newifname;
Removing the above too. The way the code is written now, if you goto cleanup from the SetName, then newifname is leaked.
+ + if (tapfd) { + char *dev_path = NULL; + if (virAsprintf(&dev_path, "/dev/%s", *ifname) < 0) { + virReportOOMError(); + goto cleanup; + } + + *tapfd = open(dev_path, O_RDWR); + + VIR_FREE(dev_path); + } + + ret = 0; +cleanup: + if (ret < 0) + VIR_FORCE_CLOSE(s); + + return ret; +}
On success, we leave here with 's' and 'tapfd' being opened, right? Since the TapDelete will open another 's', should the "if (ret < 0)" above be removed and we always close 's'? If tapfd == NULL, then what happens? Shouldn't the device path still be opened & closed (like the "existing" code)? As it stands you return zero, but haven't necessarily opened the new dev_path. I'm trying to learn the relationship between the two. What happens when/if *tapfd == -1 because open() failed? and ret = 0?
+ +int virNetDevTapDelete(const char *ifname) +{ + int s; + struct ifreq ifr; + int ret = -1; + + if ((s = virNetDevSetupControl(ifname, &ifr)) < 0) + return -1; + + if (ioctl(s, SIOCIFDESTROY, &ifr) < 0) { + virReportSystemError(errno, + _("Unable to remove tap device %s"), + ifname); + goto cleanup; + } + + ret = 0; +cleanup: + VIR_FORCE_CLOSE(s); + return ret; +} + +#else /* !defined(__FreeBSD__) */ int virNetDevTapCreate(char **ifname ATTRIBUTE_UNUSED, int *tapfd ATTRIBUTE_UNUSED, unsigned int flags ATTRIBUTE_UNUSED) @@ -245,7 +351,7 @@ int virNetDevTapDelete(const char *ifname ATTRIBUTE_UNUSED) _("Unable to delete TAP devices on this platform")); return -1; } -#endif /* ! TUNSETIFF */ +#endif
/**
--- src/util/virnetdev.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 5100467..cd2b773 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -42,6 +42,11 @@ # undef HAVE_STRUCT_IFREQ #endif +#if defined(__FreeBSD__) +# include <sys/sockio.h> +# include <net/if_dl.h> +#endif + #define VIR_FROM_THIS VIR_FROM_NONE #if (defined(HAVE_STRUCT_IFREQ) || defined(__FreeBSD__)) @@ -182,6 +187,54 @@ cleanup: VIR_FORCE_CLOSE(fd); return ret; } +#elif defined(__FreeBSD__) +int virNetDevSetMAC(const char *ifname, + const virMacAddrPtr macaddr) +{ + struct ifreq ifr; + struct sockaddr_dl sdl; + uint8_t mac[VIR_MAC_STRING_BUFLEN + 1]; + char *macstr; + int s; + int ret = -1; + + if ((s = virNetDevSetupControl(ifname, &ifr)) < 0) + return -1; + + if (VIR_ALLOC_N(macstr, VIR_MAC_STRING_BUFLEN) < 0) { + virReportOOMError(); + goto cleanup; + } + virMacAddrFormat(&macaddr, macstr); + memset(mac, 0, sizeof(mac)); + mac[0] = ':'; + if (virStrncpy(mac + 1, macstr, strlen(macstr), + sizeof(mac)) == NULL) { + virReportSystemError(ERANGE, + _("invalid MAC %s"), + macstr); + goto cleanup; + } + sdl.sdl_len = sizeof(sdl); + link_addr(mac, &sdl); + + bcopy(sdl.sdl_data, ifr.ifr_addr.sa_data, 6); + ifr.ifr_addr.sa_len = 6; + + if (ioctl(s, SIOCSIFLLADDR, &ifr) < 0) { + virReportSystemError(errno, + _("Cannot set interface MAC on '%s'"), + ifname); + goto cleanup; + } + + ret = 0; +cleanup: + VIR_FREE(macstr); + VIR_FORCE_CLOSE(s); + + return ret; +} #else int virNetDevSetMAC(const char *ifname, const virMacAddrPtr macaddr ATTRIBUTE_UNUSED) -- 1.8.0
On 01/20/2013 11:22 AM, Roman Bogorodskiy wrote:
--- src/util/virnetdev.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+)
ACK
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 5100467..cd2b773 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -42,6 +42,11 @@ # undef HAVE_STRUCT_IFREQ #endif
+#if defined(__FreeBSD__) +# include <sys/sockio.h> +# include <net/if_dl.h> +#endif + #define VIR_FROM_THIS VIR_FROM_NONE
#if (defined(HAVE_STRUCT_IFREQ) || defined(__FreeBSD__)) @@ -182,6 +187,54 @@ cleanup: VIR_FORCE_CLOSE(fd); return ret; } +#elif defined(__FreeBSD__) +int virNetDevSetMAC(const char *ifname, + const virMacAddrPtr macaddr) +{ + struct ifreq ifr; + struct sockaddr_dl sdl; + uint8_t mac[VIR_MAC_STRING_BUFLEN + 1]; + char *macstr; + int s; + int ret = -1; + + if ((s = virNetDevSetupControl(ifname, &ifr)) < 0) + return -1; + + if (VIR_ALLOC_N(macstr, VIR_MAC_STRING_BUFLEN) < 0) { + virReportOOMError(); + goto cleanup; + } + virMacAddrFormat(&macaddr, macstr); + memset(mac, 0, sizeof(mac)); + mac[0] = ':'; + if (virStrncpy(mac + 1, macstr, strlen(macstr), + sizeof(mac)) == NULL) { + virReportSystemError(ERANGE, + _("invalid MAC %s"), + macstr); + goto cleanup; + } + sdl.sdl_len = sizeof(sdl); + link_addr(mac, &sdl); + + bcopy(sdl.sdl_data, ifr.ifr_addr.sa_data, 6); + ifr.ifr_addr.sa_len = 6; + + if (ioctl(s, SIOCSIFLLADDR, &ifr) < 0) { + virReportSystemError(errno, + _("Cannot set interface MAC on '%s'"), + ifname); + goto cleanup; + } + + ret = 0; +cleanup: + VIR_FREE(macstr); + VIR_FORCE_CLOSE(s); + + return ret; +} #else int virNetDevSetMAC(const char *ifname, const virMacAddrPtr macaddr ATTRIBUTE_UNUSED)
--- src/util/virnetdev.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index cd2b773..3f2b5f8 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -412,6 +412,31 @@ cleanup: VIR_FORCE_CLOSE(fd); return ret; } +#elif defined(__FreeBSD__) +int virNetDevGetMTU(const char *ifname) +{ + int s; + int ret; + struct ifreq ifr; + + if ((s = virNetDevSetupControl(ifname, &ifr)) < 0) + return -1; + + ifr.ifr_addr.sa_family = AF_INET; + if (ioctl(s, SIOCGIFMTU, (caddr_t)&ifr) < 0) { + virReportSystemError(errno, + _("Cannot get interface MTU on '%s'"), + ifname); + ret = -1; + goto cleanup; + } + + ret = ifr.ifr_mtu; + +cleanup: + VIR_FORCE_CLOSE(s); + return ret; +} #else int virNetDevGetMTU(const char *ifname) { -- 1.8.0
On 01/20/2013 11:22 AM, Roman Bogorodskiy wrote:
--- src/util/virnetdev.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+)
ACK
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index cd2b773..3f2b5f8 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -412,6 +412,31 @@ cleanup: VIR_FORCE_CLOSE(fd); return ret; } +#elif defined(__FreeBSD__) +int virNetDevGetMTU(const char *ifname) +{ + int s; + int ret; + struct ifreq ifr; + + if ((s = virNetDevSetupControl(ifname, &ifr)) < 0) + return -1; + + ifr.ifr_addr.sa_family = AF_INET; + if (ioctl(s, SIOCGIFMTU, (caddr_t)&ifr) < 0) { + virReportSystemError(errno, + _("Cannot get interface MTU on '%s'"), + ifname); + ret = -1; + goto cleanup; + } + + ret = ifr.ifr_mtu; + +cleanup: + VIR_FORCE_CLOSE(s); + return ret; +} #else int virNetDevGetMTU(const char *ifname) {
On 01/20/2013 11:22 AM, Roman Bogorodskiy wrote:
--- src/util/virnetdev.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index cd2b773..3f2b5f8 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -412,6 +412,31 @@ cleanup: VIR_FORCE_CLOSE(fd); return ret; } +#elif defined(__FreeBSD__) +int virNetDevGetMTU(const char *ifname) +{ + int s; + int ret; + struct ifreq ifr; + + if ((s = virNetDevSetupControl(ifname, &ifr)) < 0) + return -1; + + ifr.ifr_addr.sa_family = AF_INET;
The above line is the only difference between this and the Linux version, so why not just use a single function (as you did with virNetDevSetName() in 1/5)?
+ if (ioctl(s, SIOCGIFMTU, (caddr_t)&ifr) < 0) { + virReportSystemError(errno, + _("Cannot get interface MTU on '%s'"), + ifname); + ret = -1; + goto cleanup; + } + + ret = ifr.ifr_mtu; + +cleanup: + VIR_FORCE_CLOSE(s); + return ret; +} #else int virNetDevGetMTU(const char *ifname) {
On 01/20/2013 11:22 AM, Roman Bogorodskiy wrote:
Updated patches that (hopefully) address issues noted by John Ferlan. This includes one additional patch to make virNetDevSetupControl() a public function.
Roman Bogorodskiy (5): FreeBSD: implement virNetDevExists() and virNetDevSetName(). Make virNetDevSetupControl() public. FreeBSD: implement virNetDevTapCreate() and virNetDevTapDelete(). FreeBSD: implement virNetDevSetMAC(). FreeBSD: implement virNetDevGetMTU().
src/libvirt_private.syms | 1 + src/util/virnetdev.c | 103 ++++++++++++++++++++++++++++++++++++++++--- src/util/virnetdev.h | 6 +++ src/util/virnetdevtap.c | 112 +++++++++++++++++++++++++++++++++++++++++++++-- 4 files changed, 213 insertions(+), 9 deletions(-)
I've looked; hopefully someone else can also take a glance just to make sure I haven't missed something that is libvirt specific... I'm not a network/bridge/tap expert by any stretch either. The HPVM vswitch experience only helps me so much... Still have issues with the TapCreate function - everything else seems fine to my eyes. John
Third round of porting netdev stuff to FreeBSD. A few words on virNetDevTapCreate() changes since the previous review: - it seems that there's no way to avoid iteration over possible interface names, at least I wasn't able to find fucntions for that available from userland - replaced 255 with IF_MAXUNIT, which appears to be a proper constant for that - if we requested to set tapfd, open the device before rename while we still have the original interface name, as the device entry stays the same, i.e. if we rename tap0 to vnet0, it would be still accessible at /dev/tap0, not /dev/vnet0. Fail if the open() failed Roman Bogorodskiy (4): FreeBSD: implement virNetDevExists() and virNetDevSetName(). Make virNetDevSetupControl() public. FreeBSD: implement virNetDevSetMAC() and virNetDevGetMTU(). FreeBSD: implement virNetDevTapCreate() and virNetDevTapDelete(). src/libvirt_private.syms | 1 + src/util/virnetdev.c | 81 ++++++++++++++++++++++++++++++--- src/util/virnetdev.h | 6 +++ src/util/virnetdevtap.c | 114 +++++++++++++++++++++++++++++++++++++++++++++-- 4 files changed, 192 insertions(+), 10 deletions(-) -- 1.8.0
--- src/util/virnetdev.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 295884f..503db9d 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -44,7 +44,7 @@ #define VIR_FROM_THIS VIR_FROM_NONE -#if defined(HAVE_STRUCT_IFREQ) +#if defined(HAVE_STRUCT_IFREQ) || defined(__FreeBSD__) static int virNetDevSetupControlFull(const char *ifname, struct ifreq *ifr, int domain, @@ -81,12 +81,15 @@ static int virNetDevSetupControlFull(const char *ifname, static int virNetDevSetupControl(const char *ifname, struct ifreq *ifr) { +#if defined(__FreeBSD__) + return virNetDevSetupControlFull(ifname, ifr, AF_LOCAL, SOCK_DGRAM); +#else return virNetDevSetupControlFull(ifname, ifr, AF_PACKET, SOCK_DGRAM); +#endif } #endif - -#if defined(SIOCGIFFLAGS) && defined(HAVE_STRUCT_IFREQ) +#if defined(SIOCGIFFLAGS) && (defined(HAVE_STRUCT_IFREQ) || defined(__FreeBSD__)) /** * virNetDevExists: * @ifname @@ -105,7 +108,12 @@ int virNetDevExists(const char *ifname) return -1; if (ioctl(fd, SIOCGIFFLAGS, &ifr)) { + /* FreeBSD throws ENXIO when interface doesn't exist */ +#if defined(__FreeBSD__) + if (errno == ENXIO) +#else if (errno == ENODEV) +#endif ret = 0; else virReportSystemError(errno, @@ -459,7 +467,7 @@ int virNetDevSetNamespace(const char *ifname, pid_t pidInNs) return rc; } -#if defined(SIOCSIFNAME) && defined(HAVE_STRUCT_IFREQ) +#if defined(SIOCSIFNAME) && (defined(HAVE_STRUCT_IFREQ) || defined(__FreeBSD__)) /** * virNetDevSetName: * @ifname: name of device @@ -478,12 +486,16 @@ int virNetDevSetName(const char* ifname, const char *newifname) if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0) return -1; +#if defined(__FreeBSD__) + ifr.ifr_data = (caddr_t)newifname; +#else if (virStrcpyStatic(ifr.ifr_newname, newifname) == NULL) { virReportSystemError(ERANGE, _("Network interface name '%s' is too long"), newifname); goto cleanup; } +#endif if (ioctl(fd, SIOCSIFNAME, &ifr)) { virReportSystemError(errno, -- 1.8.0
On 01/26/2013 08:13 AM, Roman Bogorodskiy wrote:
--- src/util/virnetdev.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-)
Burying your v3 in reply to v1 and v2 made it hard to find this thread. When you post a v4, it might be nicer to post it as a fresh thread, and then in your cover letter, provide a URL back to the list archives of the earlier versions. Your patch failed 'make syntax-check' if you have the 'cppi' program installed: preprocessor_indentation cppi: src/util/virnetdev.c: line 84: not properly indented cppi: src/util/virnetdev.c: line 86: not properly indented cppi: src/util/virnetdev.c: line 88: not properly indented cppi: src/util/virnetdev.c: line 112: not properly indented cppi: src/util/virnetdev.c: line 114: not properly indented cppi: src/util/virnetdev.c: line 116: not properly indented cppi: src/util/virnetdev.c: line 489: not properly indented cppi: src/util/virnetdev.c: line 491: not properly indented cppi: src/util/virnetdev.c: line 498: not properly indented Basically, we mandate a coding style of: #if foo # if bar # define blah # else /* whatever */ # endif #endif where every nested preprocessor statement has one more space between # and the rest of the line to make it easier to detect the nesting.
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 295884f..503db9d 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -44,7 +44,7 @@
#define VIR_FROM_THIS VIR_FROM_NONE
-#if defined(HAVE_STRUCT_IFREQ) +#if defined(HAVE_STRUCT_IFREQ) || defined(__FreeBSD__)
This feels wrong. Rather than hard-coding a check to __FreeBSD__, we should instead be doing a feature check. The earlier branch is a feature check: HAVE_STRUCT_IFREQ is determined at configure time. What particular _feature_ of FreeBSD are we using, different than 'struct ifreq', but similar enough that it will compile with this patch? Once we know what that feature is, then we should fix configure.ac to probe for that feature, and then use HAVE_STRUCT_XXX here instead of hard-coding __FreeBSD__. That way, other BSDs that share the same struct will work out of the box, without us having to revisit this #if.
static int virNetDevSetupControlFull(const char *ifname, struct ifreq *ifr, int domain, @@ -81,12 +81,15 @@ static int virNetDevSetupControlFull(const char *ifname, static int virNetDevSetupControl(const char *ifname, struct ifreq *ifr) { +#if defined(__FreeBSD__) + return virNetDevSetupControlFull(ifname, ifr, AF_LOCAL, SOCK_DGRAM); +#else return virNetDevSetupControlFull(ifname, ifr, AF_PACKET, SOCK_DGRAM); +#endif
Is the only difference here AF_LOCAL vs. AF_PACKET? Could we hoist the #if out of the function body, based on feature checks, something like: #if HAVE_STRUCT_IFREQ /* such as Linux */ # define VIR_NETDEV_FAMILY AF_PACKET #elif HAVE_STRUCT_WHATEVER /* such as FreeBSD */ # define VIR_NETDEV_FAMILY AF_LOCAL #endif #ifdef VIR_NETDEV_FAMILY static int virNetDevSetupControl(const char *ifname, struct ifreq *ifr) { return virNetDevSetupControlFull(ifname, ifr, VIR_NETDEV_FAMILY, SOCK_DGRAM); }
} #endif
- -#if defined(SIOCGIFFLAGS) && defined(HAVE_STRUCT_IFREQ) +#if defined(SIOCGIFFLAGS) && (defined(HAVE_STRUCT_IFREQ) || defined(__FreeBSD__))
This #if would use the same feature check as earlier code, rather than hard-coding to FreeBSD.
/** * virNetDevExists: * @ifname @@ -105,7 +108,12 @@ int virNetDevExists(const char *ifname) return -1;
if (ioctl(fd, SIOCGIFFLAGS, &ifr)) { + /* FreeBSD throws ENXIO when interface doesn't exist */ +#if defined(__FreeBSD__) + if (errno == ENXIO) +#else if (errno == ENODEV) +#endif
No need for #if or even a complicated comment here. I'd just write this as: if (ioctl(fd, SIOCGIFFLAGS, &ifr)) { if (errno == ENXIO || errno == ENODEV)
ret = 0; else virReportSystemError(errno, @@ -459,7 +467,7 @@ int virNetDevSetNamespace(const char *ifname, pid_t pidInNs) return rc; }
-#if defined(SIOCSIFNAME) && defined(HAVE_STRUCT_IFREQ) +#if defined(SIOCSIFNAME) && (defined(HAVE_STRUCT_IFREQ) || defined(__FreeBSD__))
Another place where we need a feature test, rather than FreeBSD.
/** * virNetDevSetName: * @ifname: name of device @@ -478,12 +486,16 @@ int virNetDevSetName(const char* ifname, const char *newifname) if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0) return -1;
+#if defined(__FreeBSD__) + ifr.ifr_data = (caddr_t)newifname; +#else if (virStrcpyStatic(ifr.ifr_newname, newifname) == NULL) { virReportSystemError(ERANGE, _("Network interface name '%s' is too long"), newifname); goto cleanup; } +#endif
Ah, here we finally get to the meat of the change: Linux has ifr.ifr_newname, while FreeBSD has ifr.ifr_data. That's relatively easy to add a configure.ac feature probe for: AC_CHECK_MEMBERS([struct ifreq.ifr_data, struct ifreq.ifr_newname], [], [], [[#include <whatever.h>]]) with appropriate #include lines for struct ifreq, and where autoconf will then provide HAVE_STRUCT_IFREQ_IFR_DATA and HAVE_STRUCT_IFREQ_IFR_NEWNAME for use in the rest of our code, and we are no longer hard-coded to which OS provides which spelling. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
--- src/libvirt_private.syms | 1 + src/util/virnetdev.c | 6 ++++-- src/util/virnetdev.h | 6 ++++++ 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c589236..8ad4634 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1518,6 +1518,7 @@ virNetDevSetMTUFromDevice; virNetDevSetName; virNetDevSetNamespace; virNetDevSetOnline; +virNetDevSetupControl; virNetDevValidateConfig; diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 503db9d..9ee84c3 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -78,8 +78,9 @@ static int virNetDevSetupControlFull(const char *ifname, } -static int virNetDevSetupControl(const char *ifname, - struct ifreq *ifr) +int +virNetDevSetupControl(const char *ifname, + struct ifreq *ifr) { #if defined(__FreeBSD__) return virNetDevSetupControlFull(ifname, ifr, AF_LOCAL, SOCK_DGRAM); @@ -87,6 +88,7 @@ static int virNetDevSetupControl(const char *ifname, return virNetDevSetupControlFull(ifname, ifr, AF_PACKET, SOCK_DGRAM); #endif } +#else /* !(defined(HAVE_STRUCT_IFREQ) || defined(__FreeBSD__)) */ #endif #if defined(SIOCGIFFLAGS) && (defined(HAVE_STRUCT_IFREQ) || defined(__FreeBSD__)) diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h index d588e89..63871cc 100644 --- a/src/util/virnetdev.h +++ b/src/util/virnetdev.h @@ -28,6 +28,12 @@ # include "virmacaddr.h" # include "virpci.h" +# include <net/if.h> + +int virNetDevSetupControl(const char *ifname, + struct ifreq *ifr) + ATTRIBUTE_RETURN_CHECK; + int virNetDevExists(const char *brname) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; -- 1.8.0
On 01/26/2013 08:13 AM, Roman Bogorodskiy wrote:
--- src/libvirt_private.syms | 1 + src/util/virnetdev.c | 6 ++++-- src/util/virnetdev.h | 6 ++++++ 3 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c589236..8ad4634 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1518,6 +1518,7 @@ virNetDevSetMTUFromDevice; virNetDevSetName; virNetDevSetNamespace; virNetDevSetOnline; +virNetDevSetupControl; virNetDevValidateConfig;
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 503db9d..9ee84c3 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -78,8 +78,9 @@ static int virNetDevSetupControlFull(const char *ifname, }
-static int virNetDevSetupControl(const char *ifname, - struct ifreq *ifr) +int +virNetDevSetupControl(const char *ifname, + struct ifreq *ifr)
Indentation is off on the second line, now (the first s of 'struct' should be directly below the c of 'const').
{ #if defined(__FreeBSD__) return virNetDevSetupControlFull(ifname, ifr, AF_LOCAL, SOCK_DGRAM); @@ -87,6 +88,7 @@ static int virNetDevSetupControl(const char *ifname, return virNetDevSetupControlFull(ifname, ifr, AF_PACKET, SOCK_DGRAM); #endif } +#else /* !(defined(HAVE_STRUCT_IFREQ) || defined(__FreeBSD__)) */ #endif
What good is it to add an #else with no body? If you are trying to mark the end of an #if, it might be better to do: #endif /* defined(HAVE_STRUCT_IFREQ) || defined(__FreeBSD__) */ Meanwhile, this particular change fits better in patch 1/4, and needs to account for my suggestions of doing a feature probe rather than an OS probe.
+++ b/src/util/virnetdev.h @@ -28,6 +28,12 @@ # include "virmacaddr.h" # include "virpci.h"
+# include <net/if.h>
<net/if.h> is a standard header (and therefore guaranteed by gnulib), but...
+ +int virNetDevSetupControl(const char *ifname, + struct ifreq *ifr)
struct ifreq is a non-standard type, and therefore this will cause compilation errors on platforms that lack this struct (hello mingw). We have two choices: make a new typedef virIfreqPtr that maps to 'struct ifreq *' on Linux and FreeBSD, and to 'void *' on other platforms; or make this prototype be conditional on whether HAVE_STRUCT_IFREQ (or whatever the right name is) was determined at configure time.
+ ATTRIBUTE_RETURN_CHECK;
It is probably also worth using ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2).
+ int virNetDevExists(const char *brname) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 02/04/2013 03:38 PM, Eric Blake wrote:
On 01/26/2013 08:13 AM, Roman Bogorodskiy wrote:
--- src/libvirt_private.syms | 1 + src/util/virnetdev.c | 6 ++++-- src/util/virnetdev.h | 6 ++++++ 3 files changed, 11 insertions(+), 2 deletions(-)
+++ b/src/util/virnetdev.h @@ -28,6 +28,12 @@ # include "virmacaddr.h" # include "virpci.h"
+# include <net/if.h>
<net/if.h> is a standard header (and therefore guaranteed by gnulib), but...
+ +int virNetDevSetupControl(const char *ifname, + struct ifreq *ifr)
struct ifreq is a non-standard type, and therefore this will cause compilation errors on platforms that lack this struct (hello mingw).
Oddly enough, it also causes compilation errors on Linux; at least with the kernel headers present in current Fedora 18: CC libvirt_util_la-virnetdevmacvlan.lo In file included from util/virnetdev.h:31:0, from util/virnetdevmacvlan.c:64: /usr/include/net/if.h:44:5: error: expected identifier before numeric constant In file included from util/virnetdev.h:31:0, from util/virnetdevmacvlan.c:64: /usr/include/net/if.h:111:8: error: redefinition of 'struct ifmap' ... I'm not sure if this is related to the known kernel header bug (see the big thread at https://www.redhat.com/archives/libvir-list/2013-January/thread.html#00891), but it definitely must be resolved before we can hoist 'struct ifreq *' into a header that is included in multiple files. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 02/04/2013 03:46 PM, Eric Blake wrote:
<net/if.h> is a standard header (and therefore guaranteed by gnulib), but...
+ +int virNetDevSetupControl(const char *ifname, + struct ifreq *ifr)
struct ifreq is a non-standard type, and therefore this will cause compilation errors on platforms that lack this struct (hello mingw).
Oddly enough, it also causes compilation errors on Linux; at least with the kernel headers present in current Fedora 18:
CC libvirt_util_la-virnetdevmacvlan.lo In file included from util/virnetdev.h:31:0, from util/virnetdevmacvlan.c:64: /usr/include/net/if.h:44:5: error: expected identifier before numeric constant In file included from util/virnetdev.h:31:0, from util/virnetdevmacvlan.c:64: /usr/include/net/if.h:111:8: error: redefinition of 'struct ifmap' ...
I'm not sure if this is related to the known kernel header bug (see the big thread at https://www.redhat.com/archives/libvir-list/2013-January/thread.html#00891), but it definitely must be resolved before we can hoist 'struct ifreq *' into a header that is included in multiple files.
Thankfully, I was able to find a workaround for the Linux compilation (mingw compilation still needs help, though): diff --git i/src/util/virnetdevmacvlan.c w/src/util/virnetdevmacvlan.c index a74db1e..78a37ec 100644 --- i/src/util/virnetdevmacvlan.c +++ w/src/util/virnetdevmacvlan.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2010-2012 Red Hat, Inc. + * Copyright (C) 2010-2013 Red Hat, Inc. * Copyright (C) 2010-2012 IBM Corporation * * This library is free software; you can redistribute it and/or @@ -48,7 +48,7 @@ VIR_ENUM_IMPL(virNetDevMacVLanMode, VIR_NETDEV_MACVLAN_MODE_LAST, # include <sys/socket.h> # include <sys/ioctl.h> -# include <linux/if.h> +# include <net/if.h> # include <linux/if_tun.h> /* Older kernels lacked this enum value. */ diff --git i/src/util/virnetdevvportprofile.c w/src/util/virnetdevvportprofile.c index bb97e3a..e775ca5 100644 --- i/src/util/virnetdevvportprofile.c +++ w/src/util/virnetdevvportprofile.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2009-2012 Red Hat, Inc. + * Copyright (C) 2009-2013 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -48,7 +48,7 @@ VIR_ENUM_IMPL(virNetDevVPortProfileOp, VIR_NETDEV_VPORT_PROFILE_OP_LAST, # include <sys/socket.h> # include <sys/ioctl.h> -# include <linux/if.h> +# include <net/if.h> # include <linux/if_tun.h> # include "virnetlink.h" -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
--- src/util/virnetdev.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 54 insertions(+), 1 deletion(-) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 9ee84c3..b94e503 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -42,6 +42,11 @@ # undef HAVE_STRUCT_IFREQ #endif +#if defined(__FreeBSD__) +# include <sys/sockio.h> +# include <net/if_dl.h> +#endif + #define VIR_FROM_THIS VIR_FROM_NONE #if defined(HAVE_STRUCT_IFREQ) || defined(__FreeBSD__) @@ -183,6 +188,54 @@ cleanup: VIR_FORCE_CLOSE(fd); return ret; } +#elif defined(__FreeBSD__) +int virNetDevSetMAC(const char *ifname, + const virMacAddrPtr macaddr) +{ + struct ifreq ifr; + struct sockaddr_dl sdl; + char mac[VIR_MAC_STRING_BUFLEN + 1]; + char *macstr; + int s; + int ret = -1; + + if ((s = virNetDevSetupControl(ifname, &ifr)) < 0) + return -1; + + if (VIR_ALLOC_N(macstr, VIR_MAC_STRING_BUFLEN) < 0) { + virReportOOMError(); + goto cleanup; + } + virMacAddrFormat(macaddr, macstr); + memset(mac, 0, sizeof(mac)); + mac[0] = ':'; + if (virStrncpy(mac + 1, macstr, strlen(macstr), + sizeof(mac)) == NULL) { + virReportSystemError(ERANGE, + _("invalid MAC %s"), + macstr); + goto cleanup; + } + sdl.sdl_len = sizeof(sdl); + link_addr(mac, &sdl); + + bcopy(sdl.sdl_data, ifr.ifr_addr.sa_data, 6); + ifr.ifr_addr.sa_len = 6; + + if (ioctl(s, SIOCSIFLLADDR, &ifr) < 0) { + virReportSystemError(errno, + _("Cannot set interface MAC on '%s'"), + ifname); + goto cleanup; + } + + ret = 0; +cleanup: + VIR_FREE(macstr); + VIR_FORCE_CLOSE(s); + + return ret; +} #else int virNetDevSetMAC(const char *ifname, const virMacAddrPtr macaddr ATTRIBUTE_UNUSED) @@ -329,7 +382,7 @@ virNetDevRestoreMacAddress(const char *linkdev, } -#if defined(SIOCGIFMTU) && defined(HAVE_STRUCT_IFREQ) +#if defined(SIOCGIFMTU) && defined(HAVE_STRUCT_IFREQ) || defined(__FreeBSD__) /** * virNetDevGetMTU: * @ifname: interface name get MTU for -- 1.8.0
On 01/26/2013 08:13 AM, Roman Bogorodskiy wrote:
--- src/util/virnetdev.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 54 insertions(+), 1 deletion(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 9ee84c3..b94e503 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -42,6 +42,11 @@ # undef HAVE_STRUCT_IFREQ #endif
+#if defined(__FreeBSD__) +# include <sys/sockio.h> +# include <net/if_dl.h> +#endif
Again, I don't like hard-coding the choice based on OS, if we can instead base it on features.
+ #define VIR_FROM_THIS VIR_FROM_NONE
#if defined(HAVE_STRUCT_IFREQ) || defined(__FreeBSD__)
Hmmm. Looking just a few lines earlier, we had: #ifdef __linux__ # include <linux/sockios.h> # include <linux/if_vlan.h> #elif !defined(AF_PACKET) # undef HAVE_STRUCT_IFREQ #endif So I guess you do have _a bit_ of precedence for a hard-coded OS choice, but in the Linux case, we could still turn it into a feature test pretty easily with a configure.ac check for the <linux_sockios.h> header. Meanwhile, if I'm right, FreeBSD _does_ have struct ifreq, and the only reason you had to add '|| defined(__FreeBSD__)' was because we forcefully undefined HAVE_STRUCT_IFREQ above. Revisiting some of my comments on patch 1, maybe it would be better to do: #if HAVE_LINUX_SOCKIOS_H /* Linux */ # include <linux/sockios.h> # include <linux/if_vlan.h> # define VIR_NETDEV_FAMILY AF_PACKET #elif HAVE_NET_IF_DL_H /* FreeBSD */ # include <sys/sockio.h> # include <net/if_dl.h> # define VIR_NETDEV_FAMILY AF_LOCAL #else # undef HAVE_STRUCT_IFREQ #endif
@@ -183,6 +188,54 @@ cleanup: VIR_FORCE_CLOSE(fd); return ret; } +#elif defined(__FreeBSD__) +int virNetDevSetMAC(const char *ifname, + const virMacAddrPtr macaddr) +{
The Linux definition of virNetDevSetMAC was guarded by: #if defined(SIOCGIFHWADDR) && defined(HAVE_STRUCT_IFREQ) so that argues that the FreeBSD definition should likewise be under a feature test rather than an OS test. And yes, virnetdev.c already has far too many '#if defined(__linux__)' OS tests for my liking. That said, I can still review the function body (as it should work regardless of what #if magic we settle on for determining when to compile it).
+ struct ifreq ifr; + struct sockaddr_dl sdl; + char mac[VIR_MAC_STRING_BUFLEN + 1];
Why do you stack-allocate mac,
+ char *macstr; + int s; + int ret = -1; + + if ((s = virNetDevSetupControl(ifname, &ifr)) < 0) + return -1; + + if (VIR_ALLOC_N(macstr, VIR_MAC_STRING_BUFLEN) < 0) {
and then malloc macstr, when both have the same size? It seems like you could just as easily stack-allocate both.
+ virReportOOMError(); + goto cleanup; + } + virMacAddrFormat(macaddr, macstr); + memset(mac, 0, sizeof(mac)); + mac[0] = ':'; + if (virStrncpy(mac + 1, macstr, strlen(macstr), + sizeof(mac)) == NULL) {
Or for that matter, don't even bother with virStrncpy; just do: char mac[VIR_MAC_STRING_BUFLEN + 1] = ":"; virMacAddrFormat(macaddr, mac + 1); to get mac pointing to a colon followed by the stringized representation of macaddr.
+ virReportSystemError(ERANGE, + _("invalid MAC %s"), + macstr); + goto cleanup; + } + sdl.sdl_len = sizeof(sdl); + link_addr(mac, &sdl); + + bcopy(sdl.sdl_data, ifr.ifr_addr.sa_data, 6);
EEEWWWW. bcopy() is dead. Don't ever use it in libvirt. memcpy(), please, since the two arguments don't overlap (memmove() in cases where a bcopy() touches memory that could overlap). Also, that 6 is a magic number; VIR_MAC_BUFLEN and/or sizeof(virMacAddr) is better.
+ ifr.ifr_addr.sa_len = 6;
Again, no magic numbers.
+ + if (ioctl(s, SIOCSIFLLADDR, &ifr) < 0) { + virReportSystemError(errno, + _("Cannot set interface MAC on '%s'"), + ifname);
Indentation is off. The 'i' should line up under '_'.
+ goto cleanup; + } + + ret = 0; +cleanup: + VIR_FREE(macstr); + VIR_FORCE_CLOSE(s); + + return ret; +} #else int virNetDevSetMAC(const char *ifname, const virMacAddrPtr macaddr ATTRIBUTE_UNUSED) @@ -329,7 +382,7 @@ virNetDevRestoreMacAddress(const char *linkdev, }
-#if defined(SIOCGIFMTU) && defined(HAVE_STRUCT_IFREQ) +#if defined(SIOCGIFMTU) && defined(HAVE_STRUCT_IFREQ) || defined(__FreeBSD__) /** * virNetDevGetMTU: * @ifname: interface name get MTU for
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
--- src/util/virnetdevtap.c | 114 ++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 111 insertions(+), 3 deletions(-) diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c index a884de1..36994fc 100644 --- a/src/util/virnetdevtap.c +++ b/src/util/virnetdevtap.c @@ -103,7 +103,7 @@ virNetDevProbeVnetHdr(int tapfd) #endif -#ifdef TUNSETIFF +#if defined(TUNSETIFF) /** * virNetDevTapCreate: * @ifname: the interface name @@ -230,7 +230,115 @@ cleanup: VIR_FORCE_CLOSE(fd); return ret; } -#else /* ! TUNSETIFF */ +#elif defined(__FreeBSD__) +int virNetDevTapCreate(char **ifname, + int *tapfd, + unsigned int flags ATTRIBUTE_UNUSED) +{ + int s; + struct ifreq ifr; + int ret = -1; + char *newifname = NULL; + + /* As FreeBSD determines interface type by name, + * we have to create 'tap' interface first and + * then rename it to 'vnet' + */ + if ((s = virNetDevSetupControl("tap", &ifr)) < 0) + return -1; + + if (ioctl(s, SIOCIFCREATE2, &ifr) < 0) { + virReportSystemError(errno, "%s", + _("Unable to create tap device")); + goto cleanup; + } + + /* In case we were given exact interface name (e.g. 'vnetN'), + * we just rename to it. If we have format string like + * 'vnet%d', we need to find the first available name that + * matches this pattern + */ + if (strstr(*ifname, "%d") != NULL) { + int i; + for (i = 0; i <= IF_MAXUNIT; i++) { + virBuffer newname = VIR_BUFFER_INITIALIZER; + virBufferAsprintf(&newname, *ifname, i); + + if (virBufferError(&newname)) { + virBufferFreeAndReset(&newname); + virReportOOMError(); + goto cleanup; + } + + if (virNetDevExists(virBufferCurrentContent(&newname)) == 0) { + newifname = strdup(virBufferContentAndReset(&newname)); + break; + } + } + if (newifname) { + VIR_FREE(*ifname); + *ifname = newifname; + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to generate new name for interface %s"), + ifr.ifr_name); + goto cleanup; + } + } + + if (tapfd) { + char *dev_path = NULL; + if (virAsprintf(&dev_path, "/dev/%s", ifr.ifr_name) < 0) { + virReportOOMError(); + goto cleanup; + } + + if ((*tapfd = open(dev_path, O_RDWR)) < 0) { + virReportSystemError(errno, + _("Unable to open %s"), + dev_path); + VIR_FREE(dev_path); + goto cleanup; + } + + VIR_FREE(dev_path); + } + + if (virNetDevSetName(ifr.ifr_name, *ifname) == -1) { + goto cleanup; + } + + + ret = 0; +cleanup: + VIR_FORCE_CLOSE(s); + + return ret; +} + +int virNetDevTapDelete(const char *ifname) +{ + int s; + struct ifreq ifr; + int ret = -1; + + if ((s = virNetDevSetupControl(ifname, &ifr)) < 0) + return -1; + + if (ioctl(s, SIOCIFDESTROY, &ifr) < 0) { + virReportSystemError(errno, + _("Unable to remove tap device %s"), + ifname); + goto cleanup; + } + + ret = 0; +cleanup: + VIR_FORCE_CLOSE(s); + return ret; +} + +#else /* !defined(__FreeBSD__) */ int virNetDevTapCreate(char **ifname ATTRIBUTE_UNUSED, int *tapfd ATTRIBUTE_UNUSED, unsigned int flags ATTRIBUTE_UNUSED) @@ -245,7 +353,7 @@ int virNetDevTapDelete(const char *ifname ATTRIBUTE_UNUSED) _("Unable to delete TAP devices on this platform")); return -1; } -#endif /* ! TUNSETIFF */ +#endif /** -- 1.8.0
On 01/26/2013 08:13 AM, Roman Bogorodskiy wrote:
--- src/util/virnetdevtap.c | 114 ++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 111 insertions(+), 3 deletions(-)
diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c index a884de1..36994fc 100644 --- a/src/util/virnetdevtap.c +++ b/src/util/virnetdevtap.c @@ -103,7 +103,7 @@ virNetDevProbeVnetHdr(int tapfd) #endif
-#ifdef TUNSETIFF +#if defined(TUNSETIFF)
Pointless churn.
@@ -230,7 +230,115 @@ cleanup: VIR_FORCE_CLOSE(fd); return ret; } -#else /* ! TUNSETIFF */ +#elif defined(__FreeBSD__)
Again, what is the REAL feature that we are probing here, instead of hard-coding things to an OS probe?
+int virNetDevTapCreate(char **ifname, + int *tapfd, + unsigned int flags ATTRIBUTE_UNUSED) +{ + int s; + struct ifreq ifr; + int ret = -1; + char *newifname = NULL; + + /* As FreeBSD determines interface type by name, + * we have to create 'tap' interface first and + * then rename it to 'vnet' + */ + if ((s = virNetDevSetupControl("tap", &ifr)) < 0) + return -1; + + if (ioctl(s, SIOCIFCREATE2, &ifr) < 0) {
Is the existence of SIOCIFCREATE2 the right feature to be probing for?
+ virReportSystemError(errno, "%s", + _("Unable to create tap device")); + goto cleanup; + } + + /* In case we were given exact interface name (e.g. 'vnetN'), + * we just rename to it. If we have format string like + * 'vnet%d', we need to find the first available name that + * matches this pattern + */ + if (strstr(*ifname, "%d") != NULL) { + int i; + for (i = 0; i <= IF_MAXUNIT; i++) { + virBuffer newname = VIR_BUFFER_INITIALIZER; + virBufferAsprintf(&newname, *ifname, i); + + if (virBufferError(&newname)) { + virBufferFreeAndReset(&newname); + virReportOOMError(); + goto cleanup; + }
virBuffer is useful for concatenation. But your use case is one-shot formatting; in which case I'd use virAsprintf instead of virBufferAsprintf: char *newname; if (virAsprintf(&newname, *ifname, i) < 0) { virReportOOMError(); goto cleanup; }
+ + if (virNetDevExists(virBufferCurrentContent(&newname)) == 0) { + newifname = strdup(virBufferContentAndReset(&newname)); + break; + }
at which point, you don't have to strdup(); newname is already the right allocated string.
+ } + if (newifname) { + VIR_FREE(*ifname); + *ifname = newifname; + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to generate new name for interface %s"), + ifr.ifr_name); + goto cleanup; + } + }
+ +int virNetDevTapDelete(const char *ifname)
int virNetDevTapDelete(const char *ifname)
+ +#else /* !defined(__FreeBSD__) */
This comment isn't quite right; it is more like: #else /* !TUNSETIFF && !__FreeBSD__ */ or whatever feature check we actually use. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 01/26/2013 10:13 AM, Roman Bogorodskiy wrote:
Third round of porting netdev stuff to FreeBSD.
A few words on virNetDevTapCreate() changes since the previous review:
- it seems that there's no way to avoid iteration over possible interface names, at least I wasn't able to find fucntions for that available from userland - replaced 255 with IF_MAXUNIT, which appears to be a proper constant for that - if we requested to set tapfd, open the device before rename while we still have the original interface name, as the device entry stays the same, i.e. if we rename tap0 to vnet0, it would be still accessible at /dev/tap0, not /dev/vnet0. Fail if the open() failed
Roman Bogorodskiy (4): FreeBSD: implement virNetDevExists() and virNetDevSetName(). Make virNetDevSetupControl() public. FreeBSD: implement virNetDevSetMAC() and virNetDevGetMTU(). FreeBSD: implement virNetDevTapCreate() and virNetDevTapDelete().
src/libvirt_private.syms | 1 + src/util/virnetdev.c | 81 ++++++++++++++++++++++++++++++--- src/util/virnetdev.h | 6 +++ src/util/virnetdevtap.c | 114 +++++++++++++++++++++++++++++++++++++++++++++-- 4 files changed, 192 insertions(+), 10 deletions(-)
ACK (however I hope someone else can take a glance too for those libvirt specific things that I'm still getting used to) Just a nit in the series before this would get pushed - virnetdev.c has some extra spacing in the function header for virNetDevSetupControl() on the second line: +virNetDevSetupControl(const char *ifname, + struct ifreq *ifr) ^^^^ My Google search found IF_MAXUNIT (0x7fff) being much larger than 255! Not sure if/where this could/should be documented within the libvirt pages. That is by not requesting a specific vnet# interface name could have performance implications because of the serial nature of the algorithm to find an available name especially if there are numerous tap interfaces already defined. Although I'm not quite sure where that would occur as I'm still learning where things are myself! John
participants (4)
-
Eric Blake -
John Ferlan -
Laine Stump -
Roman Bogorodskiy