5:50 p.m.
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