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