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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list