[libvirt] [PATCH v3 3/4] FreeBSD: implement virNetDevSetMAC() and virNetDevGetMTU().

[Eric Blake]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130204/dfcf97a6/attachment-0001.sig>