On 10/10/2014 01:55 PM, akrowiak@xxxxxxxxxxxxxxxxxx wrote: > From: Tony Krowiak <akrowiak@xxxxxxxxxxxxxxxxxx> > > 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 > > changes from v1: > * Using virHexToBin function to parse HEX MAC address instead of > sscanf in virMacAddrParseHex function > * Using ENOSYS in error messages for empty functions > * Reading entire file with virFileReadAll function when > parsing /proc/net/dev_mcast file > * Using VIR_APPEND_ELEMENT macro when appending array of > /proc/net/dev_mcast file objects > * Misc. formatting changes > > Signed-off-by: Tony Krowiak <akrowiak@xxxxxxxxxxxxxxxxxx> > --- > src/libvirt_private.syms | 4 + > src/util/virmacaddr.c | 25 ++++ > src/util/virmacaddr.h | 4 + > src/util/virnetdev.c | 358 ++++++++++++++++++++++++++++++++++++++++++++++ > src/util/virnetdev.h | 10 ++ > 5 files changed, 401 insertions(+), 0 deletions(-) > Beyond the recent FreeBSD build issues: http://www.redhat.com/archives/libvir-list/2014-October/msg01005.html This set of changes also had a number of Coverity issues pop up... > 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..612a409 100644 > --- a/src/util/virmacaddr.c > +++ b/src/util/virmacaddr.c > @@ -29,6 +29,7 @@ > #include "c-ctype.h" > #include "virmacaddr.h" > #include "virrandom.h" > +#include "virutil.h" > > static const unsigned char virMacAddrBroadcastAddrRaw[VIR_MAC_BUFLEN] = > { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff }; > @@ -198,6 +199,30 @@ 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) > +{ > + size_t i; > + > + if (strspn(str, "0123456789abcdefABCDEF") != VIR_MAC_HEXLEN || > + str[VIR_MAC_HEXLEN]) > + return -1; > + > + for (i = 0; i < VIR_MAC_BUFLEN; i++) > + addr->addr[i] = (virHexToBin(str[2 * i]) << 4 | > + virHexToBin(str[2 * i + 1])); > + return 0; > +} > + > 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..1410cfe 100644 > --- a/src/util/virnetdev.c > +++ b/src/util/virnetdev.c > @@ -56,6 +56,35 @@ > > VIR_LOG_INIT("util.netdev"); > > +# define PROC_NET_DEV_MCAST "/proc/net/dev_mcast" > +# define MAX_MCAST_SIZE 50*14336 > +# 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 ^^^^^^^^^^^^^^ I think these should have been enum's, especially when it comes to switch/case, e.g. typedef enum { VIR_MCAST_TYPE_INDEX_TOKEN, VIR_MCAST_TYPE_NAME_TOKEN, VIR_MCAST_TYPE_USERS_TOKEN, VIR_MCAST_TYPE_GLOBAL_TOKEN, VIR_MCAST_TYPE_ADDR_TOKEN, VIR_MCAST_TYPE_LAST } virMCastType; > +# 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 +1963,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(ENOSYS, > + _("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(ENOSYS, > + _("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; > + Coverity gets upset with this since this can be 0 < 5 and there 6 switch options (including default which cannot be reached). > + 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) { This will change to switch ((virMCastType)ifindex) { > + case VIR_MCAST_INDEX_TOKEN_IDX: > + if (virStrToLong_i(token, &endptr, 10, &num) < 0) { > + virReportSystemError(EINVAL, > + _("Failed to parse interface index from '%s'"), > + 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 network device name from '%s'"), > + 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; each of these cases, makes the following "DEADCODE" according to Coverity, but changing to enum requires each enum to be listed... > + default: This would change to /* coverity[dead_error_begin] */ case VIR_MCAST_TYPE_LAST: break; Then if anyone added something to the list that wasn't included in the switch, the compiler would balk. > + break; > + } > + } > + > + return 0; > +} > + > + > +static void virNetDevMcastEntryListFree(size_t nentries, > + virNetDevMcastEntryPtr *entries) > +{ > + size_t i; > + > + if (entries) { > + for (i = 0; i < nentries; i++) > + VIR_FREE(entries[i]); > + > + VIR_FREE(entries); > + } > +} > + > + > +static int virNetDevGetMcast(const char *ifname, > + virNetDevMcastPtr mcast) > +{ > + char *cur = NULL; > + char *buf = NULL; > + char *next = NULL; > + int ret = -1, len; > + virNetDevMcastEntryPtr entry = NULL; > + virNetDevMcastEntryPtr *entries = NULL; > + size_t nentries = 0; > + mcast->entries = NULL; (1) Event assign_zero: Assigning: "mcast->entries" = "NULL". > + mcast->nentries = 0; > + > + /* Read entire multicast table into memory */ (2) Event cond_true: Condition "(len = virFileReadAll("/proc/net/dev_mcast", 716800 /* 50 * 14336 */, &buf)) <= 0", taking true branch > + if ((len = virFileReadAll(PROC_NET_DEV_MCAST, MAX_MCAST_SIZE, &buf)) <= 0) (3) Event goto: Jumping to label "cleanup" Also note here "buf" is allocated and is never free'd causing a MEMLEAK... > + goto cleanup; > + > + cur = buf; > + > + while (cur) { > + if (!entry) { > + if (VIR_ALLOC(entry)) > + goto cleanup; > + } > + > + next = strchr(cur, '\n'); > + > + if (next) { > + next++; > + } > + > + if (virNetDevParseMcast(cur, entry)) > + goto cleanup; > + > + /* Only return global multicast MAC addresses for > + * specified interface */ > + if (entry->global && STREQ(ifname, entry->name)) { > + if (VIR_APPEND_ELEMENT(entries, nentries, entry)) > + goto cleanup; > + > + entry = NULL; > + } else { > + memset(entry, 0, sizeof(virNetDevMcastEntry)); > + } > + > + cur = next && ((next - buf) < len) ? next : NULL; > + } > + > + mcast->nentries = nentries; > + mcast->entries = entries; > + ret = 0; (4) Event label: Reached label "cleanup" > + cleanup: (5) Event cond_true: Condition "ret < 0", taking true branch > + if (ret < 0) (6) Event var_deref_model: Passing "mcast" to "virNetDevMcastListClear", which dereferences null "mcast->entries". [details] > + virNetDevMcastEntryListFree(nentries, entries); In reality the only caller of this virNetDevGetMulticastTable() will also call virNetDevMcastListClear if this function returns -1, so this call isn't necessary. > + > + VIR_FREE(entry); VIR_FREE(buf); I'll post a patch with these changes shortly. John > + > + return ret; > +} > + > + > VIR_ENUM_IMPL(virNetDevRxFilterMode, > VIR_NETDEV_RX_FILTER_MODE_LAST, > "none", > @@ -1941,6 +2230,38 @@ VIR_ENUM_IMPL(virNetDevRxFilterMode, > "all"); > > > +static int virNetDevGetMulticastTable(const char *ifname, > + virNetDevRxFilterPtr filter) > +{ > + size_t i; > + int ret = -1; > + virNetDevMcast mcast; > + filter->multicast.nTable = 0; > + filter->multicast.table = NULL; > + > + if (virNetDevGetMcast(ifname, &mcast) < 0) > + goto cleanup; > + > + if (mcast.nentries > 0) { > + if (VIR_ALLOC_N(filter->multicast.table, mcast.nentries)) > + goto cleanup; > + > + for (i = 0; i < mcast.nentries; i++) { > + virMacAddrSet(&filter->multicast.table[i], > + &mcast.entries[i]->macaddr); > + } > + > + filter->multicast.nTable = mcast.nentries; > + } > + > + ret = 0; > + cleanup: > + virNetDevMcastEntryListFree(mcast.nentries, mcast.entries); > + > + return ret; > +} > + > + > virNetDevRxFilterPtr > virNetDevRxFilterNew(void) > { > @@ -1963,3 +2284,40 @@ 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) > + goto cleanup; > + > + if (virNetDevGetMAC(ifname, &fil->mac)) > + goto cleanup; > + > + if (virNetDevGetMulticastTable(ifname, fil)) > + goto cleanup; > + > + ret = 0; > + cleanup: > + 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..ac4beff 100644 > --- a/src/util/virnetdev.h > +++ b/src/util/virnetdev.h > @@ -189,5 +189,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__ */ > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list