12:55 p.m.
From: Tony Krowiak <akrowiak(a)linux.vnet.ibm.com>
This patch set extends the handler for qemu's NIC_RX_FILTER_CHANGED event. It adds
code to compare the old and new multicast MAC address lists and programs the
macvtap filters to match the guest.
Changes from V1:
Responded to review comments from Laine Stump and Eric Blake
Tony Krowiak (2):
util: Functions to update host network device's multicast filter
qemu: change macvtap multicast list in response to
NIC_RX_FILTER_CHANGED
12:55 p.m.
New subject: [libvirt] [PATCHv2 1/2] util: Functions to update host network device's multicast filter
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(-)
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
+# 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;
+
+ 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 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;
+ default:
+ 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;
+ mcast->nentries = 0;
+
+ /* Read entire multicast table into memory */
+ if ((len = virFileReadAll(PROC_NET_DEV_MCAST, MAX_MCAST_SIZE, &buf)) <= 0)
+ 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;
+ cleanup:
+ if (ret < 0)
+ virNetDevMcastEntryListFree(nentries, entries);
+
+ VIR_FREE(entry);
+
+ 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__ */
--
1.7.1
1:17 p.m.
New subject: [libvirt] [PATCHv2 1/2] util: Functions to update host network device's multicast filter
[meta-comment]
On 10/10/2014 11:55 AM, 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
The next paragraph:
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>
---
belongs better here, after the ---. Sending patch revision history is
GOOD - it tells reviewers what to look for. But it is a comment to the
email, and not something intended for inclusion in the actual
libvirt.git (a year from now, we won't care that it was version 2 or
even version 10 committed, or what earlier versions looked like; we only
care about the commit that was actually accepted).
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(-)
I'll let Laine do the actual code review...
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
11:12 a.m.
New subject: [libvirt] [PATCHv2 1/2] util: Functions to update host network device's multicast filter
Note that I've attached a patch that addresses all the issues I raise in
this review. Please try it out; if you approve, I will squash it in and
push the result (with my name added as Signed-Off-By:, so that you don't
have to take all the blame if I screwed something up :-))
Oh, and so that it is officially said: ACK, with my small patch squashed in.
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(-)
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)
make syntax-check complains about the missing space between # and define
(we mandate proper "nesting" of the spaces after preprocessor #
characters - 1 per level; this helps to remind examiners of the code
just how deeply any piece of code is #ifdefed).
(For future reference - you should always run make syntax-check on each
patch of a series and fix the failures before posting.)
# 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
+# define VIR_MCAST_TOKEN_DELIMS " \n"
+# define VIR_MCAST_ADDR_LEN (VIR_MAC_HEXLEN + 1)
Similar problem - since there is no enclosing #ifdef, these #defines
shouldn't have a space after the #.
+
+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;
+};
I think this would be better named virNetDevMcastList.
+
#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);
To be more useful, this message should instead state
"Unable to add entries to an interface multicast list on this platform"
(which matches the wording of similar ENOSYS error messages)
+ 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);
Same comment as above.
+ 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 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;
+ default:
+ break;
+ }
+ }
+
+ return 0;
+}
+
+
+static void virNetDevMcastEntryListFree(size_t nentries,
+ virNetDevMcastEntryPtr *entries)
Since you already have virNetDevMcast (or, following my suggestion
"virNetDevMcastList") defined to be a struct that has an entries and an
nentries, why not send a pointer to one of those rather than the
separate args? If you did this, the name of the function should be
changed to virNetDevMcastListClear(), since it's not freeing the object
itself, but clearing out its contents.
+{
+ size_t i;
+
+ if (entries) {
The "if (entries)" is superfluous, since you wouldn't get into the loop
if nentries == 0, and VIR_FREE() is safe to call with a NULL arg.
+ 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;
The code would be simpler if it just populated mcast directly, rather
than putting the list into a temporary location first. This would also
allow changing virNetDevMcastListClear
+ mcast->entries = NULL;
+ mcast->nentries = 0;
+
+ /* Read entire multicast table into memory */
+ if ((len = virFileReadAll(PROC_NET_DEV_MCAST, MAX_MCAST_SIZE, &buf)) <= 0)
+ 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;
+ cleanup:
+ if (ret < 0)
+ virNetDevMcastEntryListFree(nentries, entries);
+
+ VIR_FREE(entry);
+
+ 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__ */
5:54 p.m.
New subject: [libvirt] [PATCHv2 1/2] util: Functions to update host network device's multicast filter
On 10/26/2014 12:12 PM, Laine Stump wrote:
Note that I've attached a patch that addresses all the issues I
raise in
this review. Please try it out; if you approve, I will squash it in and
push the result (with my name added as Signed-Off-By:, so that you don't
have to take all the blame if I screwed something up :-))
Oh, and so that it is officially said: ACK, with my small patch squashed in.
I
applied your patches and tested them. All is working as before
applying it.
ACK this patch to my patch.
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(-)
>
> 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)
make syntax-check complains about the missing space between # and define
(we mandate proper "nesting" of the spaces after preprocessor #
characters - 1 per level; this helps to remind examiners of the code
just how deeply any piece of code is #ifdefed).
(For future reference - you should always run make syntax-check on each
patch of a series and fix the failures before posting.)
> # 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
> +# define VIR_MCAST_TOKEN_DELIMS " \n"
> +# define VIR_MCAST_ADDR_LEN (VIR_MAC_HEXLEN + 1)
Similar problem - since there is no enclosing #ifdef, these #defines
shouldn't have a space after the #.
> +
> +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;
> +};
I think this would be better named virNetDevMcastList.
> +
> #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);
To be more useful, this message should instead state
"Unable to add entries to an interface multicast list on this platform"
(which matches the wording of similar ENOSYS error messages)
> + 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);
Same comment as above.
> + 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 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;
> + default:
> + break;
> + }
> + }
> +
> + return 0;
> +}
> +
> +
> +static void virNetDevMcastEntryListFree(size_t nentries,
> + virNetDevMcastEntryPtr *entries)
Since you already have virNetDevMcast (or, following my suggestion
"virNetDevMcastList") defined to be a struct that has an entries and an
nentries, why not send a pointer to one of those rather than the
separate args? If you did this, the name of the function should be
changed to virNetDevMcastListClear(), since it's not freeing the object
itself, but clearing out its contents.
> +{
> + size_t i;
> +
> + if (entries) {
The "if (entries)" is superfluous, since you wouldn't get into the loop
if nentries == 0, and VIR_FREE() is safe to call with a NULL arg.
> + 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;
The code would be simpler if it just populated mcast directly, rather
than putting the list into a temporary location first. This would also
allow changing virNetDevMcastListClear
> + mcast->entries = NULL;
> + mcast->nentries = 0;
> +
> + /* Read entire multicast table into memory */
> + if ((len = virFileReadAll(PROC_NET_DEV_MCAST, MAX_MCAST_SIZE, &buf)) <=
0)
> + 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;
> + cleanup:
> + if (ret < 0)
> + virNetDevMcastEntryListFree(nentries, entries);
> +
> + VIR_FREE(entry);
> +
> + 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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
2:13 p.m.
New subject: [libvirt] [PATCHv2 1/2] util: Functions to update host network device's multicast filter
On 10/27/2014 06:54 PM, Tony Krowiak wrote:
On 10/26/2014 12:12 PM, Laine Stump wrote:
> Note that I've attached a patch that addresses all the issues I raise in
> this review. Please try it out; if you approve, I will squash it in and
> push the result (with my name added as Signed-Off-By:, so that you don't
> have to take all the blame if I screwed something up :-))
>
> Oh, and so that it is officially said: ACK, with my small patch squashed in.
I applied your patches and tested them. All is working as before
applying it.
ACK this patch to my patch.
Thanks for the contribution!
I made a couple other minor changes to spacing, braces, and checking for
return value < 0 (rather than non-0), and pushed the result.
4:24 p.m.
New subject: [libvirt] [PATCHv2 1/2] util: Functions to update host network device's multicast filter
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__ */
>
12:55 p.m.
New subject: [libvirt] [PATCHv2 2/2] qemu: change macvtap multicast list in response to NIC_RX_FILTER_CHANGED
From: Tony Krowiak <akrowiak(a)linux.vnet.ibm.com>
This patch adds functionality to processNicRxFilterChangedEvent().
The old and new multicast lists are compared and the filters in
the macvtap are programmed to match the guest's filters.
Signed-off-by: Tony Krowiak <akrowiak(a)linux.vnet.ibm.com>
---
src/qemu/qemu_driver.c | 138 +++++++++++++++++++++++++++++++++++++++--------
1 files changed, 114 insertions(+), 24 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 4e2b356..7ff9c38 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4147,6 +4147,106 @@ processDeviceDeletedEvent(virQEMUDriverPtr driver,
static void
+syncNicRxFilterMacAddr(char *ifname, virNetDevRxFilterPtr guestFilter,
+ virNetDevRxFilterPtr hostFilter)
+{
+ char newMacStr[VIR_MAC_STRING_BUFLEN];
+
+ if (virMacAddrCmp(&hostFilter->mac, &guestFilter->mac)) {
+ virMacAddrFormat(&guestFilter->mac, newMacStr);
+
+ /* set new MAC address from guest to associated macvtap device */
+ if (virNetDevSetMAC(ifname, &guestFilter->mac)) {
+ VIR_WARN("Couldn't set new MAC address %s to device %s "
+ "while responding to NIC_RX_FILTER_CHANGED",
+ newMacStr, ifname);
+ } else {
+ VIR_DEBUG("device %s MAC address set to %s", ifname, newMacStr);
+ }
+ }
+}
+
+
+static void
+syncNicRxFilterGuestMulticast(char *ifname, virNetDevRxFilterPtr guestFilter,
+ virNetDevRxFilterPtr hostFilter)
+{
+ size_t i, j;
+ bool found;
+ char macstr[VIR_MAC_STRING_BUFLEN];
+
+ for (i = 0; i < guestFilter->multicast.nTable; i++) {
+ found = false;
+
+ for (j = 0; j < hostFilter->multicast.nTable; j++) {
+ if (virMacAddrCmp(&guestFilter->multicast.table[i],
+ &hostFilter->multicast.table[j]) == 0) {
+ found = true;
+ break;
+ }
+ }
+
+ if (!found) {
+ virMacAddrFormat(&guestFilter->multicast.table[i], macstr);
+
+ if (virNetDevAddMulti(ifname, &guestFilter->multicast.table[i])) {
+ VIR_WARN("Couldn't add new multicast MAC address %s to "
+ "device %s while responding to
NIC_RX_FILTER_CHANGED",
+ macstr, ifname);
+ } else {
+ VIR_DEBUG("Added multicast MAC %s to %s interface",
+ macstr, ifname);
+ }
+ }
+ }
+}
+
+
+static void
+syncNicRxFilterHostMulticast(char *ifname, virNetDevRxFilterPtr guestFilter,
+ virNetDevRxFilterPtr hostFilter)
+{
+ size_t i, j;
+ bool found;
+ char macstr[VIR_MAC_STRING_BUFLEN];
+
+ for (i = 0; i < hostFilter->multicast.nTable; i++) {
+ found = false;
+
+ for (j = 0; j < guestFilter->multicast.nTable; j++) {
+ if (virMacAddrCmp(&hostFilter->multicast.table[i],
+ &guestFilter->multicast.table[j]) == 0) {
+ found = true;
+ break;
+ }
+ }
+
+ if (!found) {
+ virMacAddrFormat(&hostFilter->multicast.table[i], macstr);
+
+ if (virNetDevDelMulti(ifname, &hostFilter->multicast.table[i])) {
+ VIR_WARN("Couldn't delete multicast MAC address %s from "
+ "device %s while responding to
NIC_RX_FILTER_CHANGED",
+ macstr, ifname);
+ } else {
+ VIR_DEBUG("Deleted multicast MAC %s from %s interface",
+ macstr, ifname);
+ }
+ }
+ }
+}
+
+
+static void
+syncNicRxFilterMulticast(char *ifname,
+ virNetDevRxFilterPtr guestFilter,
+ virNetDevRxFilterPtr hostFilter)
+{
+ syncNicRxFilterGuestMulticast(ifname, guestFilter, hostFilter);
+ syncNicRxFilterHostMulticast(ifname, guestFilter, hostFilter);
+}
+
+static void
processNicRxFilterChangedEvent(virQEMUDriverPtr driver,
virDomainObjPtr vm,
char *devAlias)
@@ -4155,9 +4255,8 @@ processNicRxFilterChangedEvent(virQEMUDriverPtr driver,
qemuDomainObjPrivatePtr priv = vm->privateData;
virDomainDeviceDef dev;
virDomainNetDefPtr def;
- virNetDevRxFilterPtr filter = NULL;
- virMacAddr oldMAC;
- char newMacStr[VIR_MAC_STRING_BUFLEN];
+ virNetDevRxFilterPtr guestFilter = NULL;
+ virNetDevRxFilterPtr hostFilter = NULL;
int ret;
VIR_DEBUG("Received NIC_RX_FILTER_CHANGED event for device %s "
@@ -4202,37 +4301,27 @@ processNicRxFilterChangedEvent(virQEMUDriverPtr driver,
"device %s in domain %s", def->info.alias,
vm->def->name);
qemuDomainObjEnterMonitor(driver, vm);
- ret = qemuMonitorQueryRxFilter(priv->mon, devAlias, &filter);
+ ret = qemuMonitorQueryRxFilter(priv->mon, devAlias, &guestFilter);
qemuDomainObjExitMonitor(driver, vm);
if (ret < 0)
goto endjob;
- virMacAddrFormat(&filter->mac, newMacStr);
-
if (virDomainNetGetActualType(def) == VIR_DOMAIN_NET_TYPE_DIRECT) {
- /* For macvtap connections, set the macvtap device's MAC
- * address to match that of the guest device.
- */
-
- if (virNetDevGetMAC(def->ifname, &oldMAC) < 0) {
- VIR_WARN("Couldn't get current MAC address of device %s "
+ if (virNetDevGetRxFilter(def->ifname, &hostFilter)) {
+ VIR_WARN("Couldn't get current RX filter for device %s "
"while responding to NIC_RX_FILTER_CHANGED",
def->ifname);
goto endjob;
}
- if (virMacAddrCmp(&oldMAC, &filter->mac)) {
- /* set new MAC address from guest to associated macvtap device */
- if (virNetDevSetMAC(def->ifname, &filter->mac) < 0) {
- VIR_WARN("Couldn't set new MAC address %s to device %s "
- "while responding to NIC_RX_FILTER_CHANGED",
- newMacStr, def->ifname);
- } else {
- VIR_DEBUG("device %s MAC address set to %s",
- def->ifname, newMacStr);
- }
- }
+ /* For macvtap connections, set the following macvtap network device
+ * attributes to match those of the guest network device:
+ * - MAC address
+ * - Multicast MAC address table
+ */
+ syncNicRxFilterMacAddr(def->ifname, guestFilter, hostFilter);
+ syncNicRxFilterMulticast(def->ifname, guestFilter, hostFilter);
}
endjob:
@@ -4242,7 +4331,8 @@ processNicRxFilterChangedEvent(virQEMUDriverPtr driver,
ignore_value(qemuDomainObjEndJob(driver, vm));
cleanup:
- virNetDevRxFilterFree(filter);
+ virNetDevRxFilterFree(hostFilter);
+ virNetDevRxFilterFree(guestFilter);
VIR_FREE(devAlias);
virObjectUnref(cfg);
}
--
1.7.1
11:21 a.m.
New subject: [libvirt] [PATCHv2 2/2] qemu: change macvtap multicast list in response to NIC_RX_FILTER_CHANGED
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 adds functionality to processNicRxFilterChangedEvent().
The old and new multicast lists are compared and the filters in
the macvtap are programmed to match the guest's filters.
Signed-off-by: Tony Krowiak <akrowiak(a)linux.vnet.ibm.com>
---
src/qemu/qemu_driver.c | 138 +++++++++++++++++++++++++++++++++++++++--------
1 files changed, 114 insertions(+), 24 deletions(-)
The ACK stands on this patch. I'll push it when I get confirmation from
you that the changes I proposed to you patch 1/2 are okay.
3676
days inactive
3695
days old
8 comments
5 participants
participants (5)
-
akrowiak＠linux.vnet.ibm.com -
Eric Blake -
John Ferlan -
Laine Stump -
Tony Krowiak