[libvirt] [PATCH 1/2] util: Functions to update host network device's multicast filter
Tony Krowiak
akrowiak at linux.vnet.ibm.com
Wed Oct 8 17:41:32 UTC 2014
On 10/08/2014 11:36 AM, Laine Stump wrote:
> On 10/06/2014 05:37 PM, akrowiak at linux.vnet.ibm.com wrote:
>> From: Tony Krowiak <akrowiak at linux.vnet.ibm.com>
>>
>> This patch provides the utility functions to needed to synchronize the
>> changes made to a guest domain network device's multicast filter
>> with the corresponding macvtap device's filter on the host:
>>
>> * Get/add/remove multicast MAC addresses
>> * Get the macvtap device's RX filter list
>>
>> Signed-off-by: Tony Krowiak <akrowiak at linux.vnet.ibm.com>
>> ---
>> src/libvirt_private.syms | 4 +
>> src/util/virmacaddr.c | 37 +++++
>> src/util/virmacaddr.h | 4 +
>> src/util/virnetdev.c | 360 ++++++++++++++++++++++++++++++++++++++++++++++
>> src/util/virnetdev.h | 11 ++
>> 5 files changed, 416 insertions(+), 0 deletions(-)
>>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index d6265ac..6d06a2c 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -1590,13 +1590,16 @@ virMacAddrIsBroadcastRaw;
>> virMacAddrIsMulticast;
>> virMacAddrIsUnicast;
>> virMacAddrParse;
>> +virMacAddrParseHex;
>> virMacAddrSet;
>> virMacAddrSetRaw;
>>
>>
>> # util/virnetdev.h
>> +virNetDevAddMulti;
>> virNetDevAddRoute;
>> virNetDevClearIPv4Address;
>> +virNetDevDelMulti;
>> virNetDevExists;
>> virNetDevGetIndex;
>> virNetDevGetIPv4Address;
>> @@ -1604,6 +1607,7 @@ virNetDevGetLinkInfo;
>> virNetDevGetMAC;
>> virNetDevGetMTU;
>> virNetDevGetPhysicalFunction;
>> +virNetDevGetRxFilter;
>> virNetDevGetVirtualFunctionIndex;
>> virNetDevGetVirtualFunctionInfo;
>> virNetDevGetVirtualFunctions;
>> diff --git a/src/util/virmacaddr.c b/src/util/virmacaddr.c
>> index ebd1182..ae5e5d2 100644
>> --- a/src/util/virmacaddr.c
>> +++ b/src/util/virmacaddr.c
>> @@ -198,6 +198,43 @@ virMacAddrFormat(const virMacAddr *addr,
>> return str;
>> }
>>
>> +/**
>> + * virMacAddrParseHex:
>> + * @str: string hexadecimal representation of MAC address, e.g., "F801EFCE3aCB"
>> + * @addr: 6-byte MAC address
>> + *
>> + * Parse the hexadecimal representation of a MAC address
>> + *
>> + * Return 0 upon success, or -1 in case of error.
>> + */
>> +int
>> +virMacAddrParseHex(const char* str, virMacAddrPtr addr)
>> +{
>> + if (strlen(str) != VIR_MAC_HEXLEN)
>> + return -1;
>> +
>> + size_t iaddr;
>> + size_t istr;
>> +
>> +
>> + for (istr = 0, iaddr = 0; iaddr < VIR_MAC_BUFLEN; istr += 2, iaddr++) {
>> + unsigned int hex;
>> +
>> + if (sscanf(&str[istr], "%02x", &hex) != 1)
>> + break;
> I'm not keen on sscanf(), but since we're already using it elsewhere,
> and the existing virMacAddrParse uses the normally-blacklisted strtoul()
> (and using virStrToLong_ui() on pieces of the string would require a
> terminating character after each piece), I don't have a ready answer for
> an alternative.
>
> Eric, do you have an opinion on this? Is sscanf okay when we want to parse:
>
> XXXXXXXXXXXX
>
> of hex digits into 6 binary bytes?
See my response to Eric's response.
>
>
>> +
>> + if (hex > UCHAR_MAX)
>> + break;
>> +
>> + addr->addr[iaddr] = hex;
>> + }
>> +
>> + if (istr == VIR_MAC_HEXLEN)
>> + return 0;
>> +
>> + return -1;
>> +}
>> +
>> void virMacAddrGenerate(const unsigned char prefix[VIR_MAC_PREFIX_BUFLEN],
>> virMacAddrPtr addr)
>> {
>> diff --git a/src/util/virmacaddr.h b/src/util/virmacaddr.h
>> index 49efc36..72a285a 100644
>> --- a/src/util/virmacaddr.h
>> +++ b/src/util/virmacaddr.h
>> @@ -27,6 +27,7 @@
>> # include "internal.h"
>>
>> # define VIR_MAC_BUFLEN 6
>> +#define VIR_MAC_HEXLEN (VIR_MAC_BUFLEN * 2)
>> # define VIR_MAC_PREFIX_BUFLEN 3
>> # define VIR_MAC_STRING_BUFLEN (VIR_MAC_BUFLEN * 3)
>>
>> @@ -50,6 +51,9 @@ void virMacAddrGenerate(const unsigned char prefix[VIR_MAC_PREFIX_BUFLEN],
>> virMacAddrPtr addr);
>> int virMacAddrParse(const char* str,
>> virMacAddrPtr addr) ATTRIBUTE_RETURN_CHECK;
>> +int virMacAddrParseHex(const char* str,
>> + virMacAddrPtr addr)
>> + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
>> bool virMacAddrIsUnicast(const virMacAddr *addr);
>> bool virMacAddrIsMulticast(const virMacAddr *addr);
>> bool virMacAddrIsBroadcastRaw(const unsigned char s[VIR_MAC_BUFLEN]);
>> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
>> index db5623a..5e53f5f 100644
>> --- a/src/util/virnetdev.c
>> +++ b/src/util/virnetdev.c
>> @@ -56,6 +56,33 @@
>>
>> VIR_LOG_INIT("util.netdev");
>>
>> +# define VIR_MCAST_NAME_LEN (IFNAMSIZ + 1)
>> +# define VIR_MCAST_INDEX_TOKEN_IDX 0
>> +# define VIR_MCAST_NAME_TOKEN_IDX 1
>> +# define VIR_MCAST_USERS_TOKEN_IDX 2
>> +# define VIR_MCAST_GLOBAL_TOKEN_IDX 3
>> +# define VIR_MCAST_ADDR_TOKEN_IDX 4
>> +# define VIR_MCAST_NUM_TOKENS 5
>> +# define VIR_MCAST_TOKEN_DELIMS " \n"
>> +# define VIR_MCAST_ADDR_LEN (VIR_MAC_HEXLEN + 1)
>> +
>> +typedef struct _virNetDevMcastEntry virNetDevMcastEntry;
>> +typedef virNetDevMcastEntry *virNetDevMcastEntryPtr;
>> +struct _virNetDevMcastEntry {
>> + int index;
>> + char name[VIR_MCAST_NAME_LEN];
>> + int users;
>> + bool global;
>> + virMacAddr macaddr;
>> +};
>> +
>> +typedef struct _virNetDevMcast virNetDevMcast;
>> +typedef virNetDevMcast *virNetDevMcastPtr;
>> +struct _virNetDevMcast {
>> + size_t nentries;
>> + virNetDevMcastEntryPtr *entries;
>> +};
>> +
>> #if defined(HAVE_STRUCT_IFREQ)
>> static int virNetDevSetupControlFull(const char *ifname,
>> struct ifreq *ifr,
>> @@ -1934,6 +1961,266 @@ virNetDevGetLinkInfo(const char *ifname,
>> #endif /* defined(__linux__) */
>>
>>
>> +#if defined(SIOCADDMULTI) && defined(HAVE_STRUCT_IFREQ)
>> +/**
>> + * virNetDevAddMulti:
>> + * @ifname: interface name to which to add multicast MAC address
>> + * @macaddr: MAC address
>> + *
>> + * This function adds the @macaddr to the multicast list for a given interface
>> + * @ifname.
>> + *
>> + * Returns 0 in case of success or -1 on failure
>> + */
>> +int virNetDevAddMulti(const char *ifname,
>> + virMacAddrPtr macaddr)
>> +{
>> + int fd = -1;
>> + int ret = -1;
>> + struct ifreq ifr;
>> +
>> + if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0)
>> + return -1;
>> +
>> + ifr.ifr_hwaddr.sa_family = AF_UNSPEC;
>> + virMacAddrGetRaw(macaddr, (unsigned char *)ifr.ifr_hwaddr.sa_data);
>> +
>> + if (ioctl(fd, SIOCADDMULTI, &ifr) < 0) {
>> + char macstr[VIR_MAC_STRING_BUFLEN];
>> + virReportSystemError(errno,
>> + _("Cannot add multicast MAC %s on '%s' interface"),
>> + virMacAddrFormat(macaddr, macstr), ifname);
>> + goto cleanup;
>> + }
>> +
>> + ret = 0;
>> +
>> + cleanup:
>> + VIR_FORCE_CLOSE(fd);
>> + return ret;
>> +}
>> +#else
>> +int virNetDevAddMulti(const char *ifname,
>> + virMacAddrPtr macaddr ATTRIBUTE_UNUSED)
>> +{
>> + char macstr[VIR_MAC_STRING_BUFLEN];
>> + virReportSystemError(errno,
> For all of these empty functions used on platforms that don't have the
> necessary functionality, you need to replace "errno" with "ENOSYS" -
> since we haven't tried to call anything, errno will not have been set.
Oops, cut and paste error.
>
>> + _("Cannot add multicast MAC %s on '%s' interface"),
>> + virMacAddrFormat(macaddr, macstr), ifname);
>> + return -1;
>> +}
>> +#endif
>> +
>> +#if defined(SIOCDELMULTI) && defined(HAVE_STRUCT_IFREQ)
>> +/**
>> + * virNetDevDelMulti:
>> + * @ifname: interface name from which to delete the multicast MAC address
>> + * @macaddr: MAC address
>> + *
>> + * This function deletes the @macaddr from the multicast list for a given
>> + * interface @ifname.
>> + *
>> + * Returns 0 in case of success or -1 on failure
>> + */
>> +int virNetDevDelMulti(const char *ifname,
>> + virMacAddrPtr macaddr)
>> +{
>> + int fd = -1;
>> + int ret = -1;
>> + struct ifreq ifr;
>> +
>> + if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0)
>> + return -1;
>> +
>> + ifr.ifr_hwaddr.sa_family = AF_UNSPEC;
>> + virMacAddrGetRaw(macaddr, (unsigned char *)ifr.ifr_hwaddr.sa_data);
>> +
>> + if (ioctl(fd, SIOCDELMULTI, &ifr) < 0) {
>> + char macstr[VIR_MAC_STRING_BUFLEN];
>> + virReportSystemError(errno,
>> + _("Cannot add multicast MAC %s on '%s' interface"),
>> + virMacAddrFormat(macaddr, macstr), ifname);
>> + goto cleanup;
>> + }
>> +
>> + ret = 0;
>> +
>> + cleanup:
>> + VIR_FORCE_CLOSE(fd);
>> + return ret;
>> +}
>> +#else
>> +int virNetDevDelMulti(const char *ifname,
>> + virMacAddrPtr macaddr ATTRIBUTE_UNUSED)
>> +{
>> + char macstr[VIR_MAC_STRING_BUFLEN];
>> + virReportSystemError(errno,
> ENOSYS
Ditto on the cut and paste error.
>
>> + _("Cannot delete multicast MAC %s on '%s' interface"),
>> + virMacAddrFormat(macaddr, macstr), ifname);
>> + return -1;
>> +}
>> +#endif
>> +
>> +static int virNetDevParseMcast(char *buf, virNetDevMcastEntryPtr mcast)
>> +{
>> + int ifindex;
>> + int num;
>> + char *next;
>> + char *token;
>> + char *saveptr;
>> + char *endptr;
>> +
>> + for (ifindex = 0, next = buf; ifindex < VIR_MCAST_NUM_TOKENS; ifindex++,
>> + next = NULL) {
>> + token = strtok_r(next, VIR_MCAST_TOKEN_DELIMS, &saveptr);
>> +
>> + if (token == NULL) {
>> + virReportSystemError(EINVAL,
>> + _("failed to parse multicast address from '%s'"),
>> + buf);
>> + return -1;
>> + }
>> +
>> + switch (ifindex) {
>> + case VIR_MCAST_INDEX_TOKEN_IDX:
>> + if (virStrToLong_i(token, &endptr, 10, &num) < 0) {
>> + virReportSystemError(EINVAL,
>> + _("Failed to parse index from '%s'"),
> "Failed to parse interface index from '%s'"
I will rephrase the message.
>
>> + buf);
>> + return -1;
>> +
>> + }
>> +
>> + mcast->index = num;
>> +
>> + break;
>> + case VIR_MCAST_NAME_TOKEN_IDX:
>> + if (virStrncpy(mcast->name, token, strlen(token),
>> + VIR_MCAST_NAME_LEN) == NULL) {
>> + virReportSystemError(EINVAL,
>> + _("Failed to parse NIC name from '%s'"),
> I prefer "network device" rather than "NIC".
I will rephrase the message.
>
>> + buf);
>> + return -1;
>> + }
>> +
>> + break;
>> + case VIR_MCAST_USERS_TOKEN_IDX:
>> + if (virStrToLong_i(token, &endptr, 10, &num) < 0) {
>> + virReportSystemError(EINVAL,
>> + _("Failed to parse users from '%s'"),
>> + buf);
>> + return -1;
>> +
>> + }
>> +
>> + mcast->users = num;
>> +
>> + break;
>> + case VIR_MCAST_GLOBAL_TOKEN_IDX:
>> + if (virStrToLong_i(token, &endptr, 10, &num) < 0) {
>> + virReportSystemError(EINVAL,
>> + _("Failed to parse users from '%s'"),
>> + buf);
>> + return -1;
>> +
>> + }
>> +
>> + mcast->global = num;
>> +
>> + break;
>> + case VIR_MCAST_ADDR_TOKEN_IDX:
>> + if (virMacAddrParseHex((const char*)token,
>> + &mcast->macaddr) < 0) {
>> + virReportSystemError(EINVAL,
>> + _("Failed to parse MAC address from '%s'"),
>> + buf);
>> + }
>> +
>> + break;
>> + default:
>> + break;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +
>> +static int virNetDevGetMcast(const char *ifname,
>> + virNetDevMcastPtr mcast)
>> +{
>> + FILE *file;
>> + const char *path = "/proc/net/dev_mcast";
>> + char buf[256];
>> + int ret = -1;
>> + virNetDevMcastEntry entry;
>> + virNetDevMcastEntryPtr *entries = NULL;
>> + size_t nentries = 0;
>> + size_t entries_sz = 0;
>> + size_t i;
>> + mcast->entries = NULL;
>> + mcast->nentries = 0;
>> +
>> + file = fopen(path, "r");
>> +
>> + if (!file) {
>> + virReportSystemError(errno,
>> + _("cannot open multicast address file %s"), path);
>> + return -1;
>> + }
>> +
>> + while (fgets(buf, sizeof(buf), file)) {
> We prefer to not use fgets(), but instead use virFileReadAll() to read
> the entire file into a buffer, then parse it from there. There are
> several examples of this in the code, for example
> network/bridge_driver_linux.c:networkCheckRouteCollision().\
I will take a look at that and use virFileReadAll(). For the record,
what do you have against fgets?
>
>> + if (virNetDevParseMcast(buf, &entry) < 0) {
>> + goto error;
>> + }
>> +
>> + if (entry.global && STREQ(ifname, entry.name)) {
>> + if (VIR_RESIZE_N(entries, entries_sz,
>> + nentries, 1) < 0) {
>> + virReportSystemError(ENOMEM,
>> + _("Failed to resize multicast MAC address array: "
>> + "ptr=%p, alloc=%zu, count=%zu, add=1"),
>> + entries, entries_sz, nentries);
> Any memory allocation error will already automatically get an error
> message logged, so this is redundant. Aside from that, you could instead
> use VIR_APPEND_ELEMENT() to replace the VIR_RESIZE_N(...., 1) ...
> memcpy(entries[nentries++], &entry) ... memset(&entry...) (it takes care
> of all three of those) .
Yes, I now see that after looking the VIR_RESIZE_N macro.
>
> Basically, make "entry" a virNetDevMcastEntryPtr entry = NULL;, then
> each time through the loop allocate a new table entry in "entry" and
> fill it in, then call
>
>
> if (VIR_APPEND_ELEMENT(entries, nentries, entry) < 0)
> goto cleanup;
>
> (and by the way, because normal execution also goes past the "error"
> label, it should be changed to cleanup in order to avoid misunderstandings)
I will take a look at VIR_APPEND_ELEMENT(). Sounds like it might be
cleaner and easier to understand.
>
>> + goto error;
>> + }
>> +
>> + if (VIR_ALLOC(entries[nentries]) < 0) {
>> + char addr[VIR_MAC_STRING_BUFLEN];
>> + virReportSystemError(ENOMEM,
>> + _("Failed to allocate storage for MAC address %s"),
>> + virMacAddrFormat(&mcast->entries[nentries]->macaddr,
>> + addr));
> Same here - no need to report the error, as a general error has already
> been reported, and we know that the source of the error has nothing to
> do with this specific MAC address, so no need to log an extra message.
Yep
>
>> + goto error;
>> + }
>> +
>> + memcpy(entries[nentries++], &entry,
>> + sizeof(virNetDevMcastEntry));
>> + }
>> +
>> + memset(buf, 0, sizeof(buf));
>> + memset(&entry, 0, sizeof(virNetDevMcastEntry));
>> + }
>> +
>> +
>> + mcast->nentries = nentries;
>> + mcast->entries = entries;
>> + ret = 0;
>> +
>> + error:
>> + VIR_FORCE_FCLOSE(file);
>> +
>> + if ((ret < 0) && (nentries > 0)) {
> If nentries == 0, the following code becomes a NOP anyway, so you can
> just follow the libvirt convention of unconditionally calling VIR_FREE()
> (as long as ret < 0)
Got it.
>
>> + for (i = 0; i < nentries; i++) {
>> + VIR_FREE(entries[i]);
>> + }
>> +
>> + VIR_FREE(entries);
> The above should be turned into a helper function called something like
> virNetDevMcastEntryListFree(), because you're going to need it down below...
Okay
>
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +
>> VIR_ENUM_IMPL(virNetDevRxFilterMode,
>> VIR_NETDEV_RX_FILTER_MODE_LAST,
>> "none",
>> @@ -1941,6 +2228,37 @@ VIR_ENUM_IMPL(virNetDevRxFilterMode,
>> "all");
>>
>>
>> +static int virNetDevGetMulticastTable(const char *ifname,
>> + virNetDevRxFilterPtr filter)
>> +{
>> + int i;
>> + int ret = -1;
>> + virNetDevMcast mcast;
>> + filter->multicast.nTable = 0;
>> + filter->multicast.table = NULL;
>> +
>> + if (virNetDevGetMcast(ifname, &mcast) < 0)
>> + goto error;
>> +
>> + if (mcast.nentries > 0) {
>> + if (VIR_ALLOC_N(filter->multicast.table, mcast.nentries))
>> + goto error;
>> +
>> + for (i = 0; i < mcast.nentries; i++) {
>> + virMacAddrSet(&filter->multicast.table[i],
>> + &mcast.entries[i]->macaddr);
>> + }
>> +
>> + filter->multicast.nTable = mcast.nentries;
>> + }
>> +
>> + ret = 0;
>> +
>> + error:
> Same here - also change this label to cleanup.
>
> Also, you returned without freeing the virNetDevMcastEntry list. You
> should call the new virNetDevMcastEntryListFree() here.
Okay
>
>> + return ret;
>> +}
>> +
>> +
>> virNetDevRxFilterPtr
>> virNetDevRxFilterNew(void)
>> {
>> @@ -1963,3 +2281,45 @@ virNetDevRxFilterFree(virNetDevRxFilterPtr filter)
>> VIR_FREE(filter);
>> }
>> }
>> +
>> +
>> +/**
>> + * virNetDevGetRxFilter:
>> + * This function supplies the RX filter list for a given device interface
>> + *
>> + * @ifname: Name of the interface
>> + * @filter: The RX filter list
>> + *
>> + * Returns 0 or -1 on failure.
>> + */
>> +int virNetDevGetRxFilter(const char *ifname,
>> + virNetDevRxFilterPtr *filter)
>> +{
>> + int ret = -1;
>> + virNetDevRxFilterPtr fil = virNetDevRxFilterNew();
>> +
>> + if (!fil) {
>> + virReportSystemError(ENOMEM,
>> + _("Failed to allocate filter for %s interface"),
>> + ifname);
> No need to log an extra error here - it's already done when the memory
> allocation fails.
Got it.
>
>> +
>> + }
>> +
>> + if (virNetDevGetMAC(ifname, &fil->mac))
>> + goto cleanup;
> Indentation problem ^^
Will fix.
>
>> +
>> + if (virNetDevGetMulticastTable(ifname, fil))
>> + goto cleanup;
>> +
>> + ret = 0;
>> +
>> + cleanup:
> remove the blank line between ret = 0 and cleanup: to reduce the chance
> that someone will attempt to add new code in between them.
Okay
>
>> + if (ret < 0) {
>> + virNetDevRxFilterFree(fil);
>> + fil = NULL;
>> + }
>> +
>> + *filter = fil;
>> +
>> + return ret;
>> +}
>> diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h
>> index 2a6e67d..1d274b3 100644
>> --- a/src/util/virnetdev.h
>> +++ b/src/util/virnetdev.h
>> @@ -30,6 +30,7 @@
>> # include "virmacaddr.h"
>> # include "virpci.h"
>> # include "device_conf.h"
>> +# include "virutil.h"
>>
>> # ifdef HAVE_STRUCT_IFREQ
>> typedef struct ifreq virIfreq;
>> @@ -189,5 +190,15 @@ int virNetDevGetLinkInfo(const char *ifname,
>> virNetDevRxFilterPtr virNetDevRxFilterNew(void)
>> ATTRIBUTE_RETURN_CHECK;
>> void virNetDevRxFilterFree(virNetDevRxFilterPtr filter);
>> +int virNetDevGetRxFilter(const char *ifname,
>> + virNetDevRxFilterPtr *filter)
>> + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
>> +
>> +int virNetDevAddMulti(const char *ifname,
>> + virMacAddrPtr macaddr)
>> + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
>> +int virNetDevDelMulti(const char *ifname,
>> + virMacAddrPtr macaddr)
>> + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
>>
>> #endif /* __VIR_NETDEV_H__ */
> Otherwise it looks okay. As we've discussed, it would be nice to use
> netlink instead of /proc/net and ioctl(), but that can be added later.
I agree. When I originally started working on this, that was my plan,
however; after spending days trying to understand how to use netlink to
do retrieve the multicast list, I gave up when I found that iproute2
does it by reading the file. Can you point me to some good, detailed
netlink documentation? I've googled for it, looked at code examples and
experimented by writing code snippets and have not been able to figure
out how to retrieve the multicast list using netlink.
>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
More information about the libvir-list
mailing list