4:24 p.m.
On 10/10/2014 01:55 PM, akrowiak(a)linux.vnet.ibm.com wrote:
From: Tony Krowiak <akrowiak(a)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
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(a)linux.vnet.ibm.com>
---
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__ */
>