--- 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

--- 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

John Ferlan wrote: Please see a commit above. 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. 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. Roman Bogorodskiy

--- 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

--- 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/20/2013 11:22 AM, Roman Bogorodskiy wrote: ACK

On 01/20/2013 11:22 AM, Roman Bogorodskiy wrote: ACK (although I believe this is all that has to change, hopefully someone with some more time/experience here will look too).

--- 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

--- 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

--- 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: 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)?

On 01/26/2013 08:13 AM, Roman Bogorodskiy wrote: 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. 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. 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); } This #if would use the same feature check as earlier code, rather than hard-coding to FreeBSD. 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) Another place where we need a feature test, rather than FreeBSD. 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

On 01/26/2013 08:13 AM, Roman Bogorodskiy wrote: Indentation is off on the second line, now (the first s of 'struct' should be directly below the c of 'const'). 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. <net/if.h> is a standard header (and therefore guaranteed by gnulib), but... 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. It is probably also worth using ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2). -- 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: 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

On 01/26/2013 08:13 AM, Roman Bogorodskiy wrote: Again, I don't like hard-coding the choice based on OS, if we can instead base it on features. 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 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). Why do you stack-allocate mac, and then malloc macstr, when both have the same size? It seems like you could just as easily stack-allocate both. 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. 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. Again, no magic numbers. Indentation is off. The 'i' should line up under '_'. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 01/26/2013 08:13 AM, Roman Bogorodskiy wrote: Pointless churn. Again, what is the REAL feature that we are probing here, instead of hard-coding things to an OS probe? Is the existence of SIOCIFCREATE2 the right feature to be probing for? 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; } at which point, you don't have to strdup(); newname is already the right allocated string. int virNetDevTapDelete(const char *ifname) 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