On 11/24/2014 12:48 PM, Laine Stump wrote:
> These two functions use netlink RTM_NEWNEIGH and RTM_DELNEIGH messages
> to add and delete entries from a bridge's fdb. The bridge itself is
> not referenced in the arguments to the functions, only the name of the
> device that is attached to the bridge (since a device can only be
> attached to one bridge at a time, and must be attached for this
> function to make sense, the kernel easily infers which bridge's fdb is
> being modified by looking at the device name/index).
Perhaps this hunk of comment can be copied into the static function as
well...
> ---
> src/libvirt_private.syms | 2 +
> src/util/virnetdevbridge.c | 128 +++++++++++++++++++++++++++++++++++++++++++++
> src/util/virnetdevbridge.h | 16 ++++++
> 3 files changed, 146 insertions(+)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 6453826..6b6c51b 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1660,6 +1660,8 @@ virNetDevBandwidthUpdateRate;
> virNetDevBridgeAddPort;
> virNetDevBridgeCreate;
> virNetDevBridgeDelete;
> +virNetDevBridgeFDBAdd;
> +virNetDevBridgeFDBDel;
> virNetDevBridgeGetSTP;
> virNetDevBridgeGetSTPDelay;
> virNetDevBridgeGetVlanFiltering;
> diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c
> index 9be835c..a38323b 100644
> --- a/src/util/virnetdevbridge.c
> +++ b/src/util/virnetdevbridge.c
> @@ -37,6 +37,9 @@
> #include <netinet/in.h>
>
> #ifdef __linux__
> +# if defined(HAVE_LIBNL)
> +# include "virnetlink.h"
> +# endif
> # include <linux/sockios.h>
> # include <linux/param.h> /* HZ */
> # if NETINET_LINUX_WORKAROUND
> @@ -915,4 +918,129 @@ virNetDevBridgeSetVlanFiltering(const char *brname ATTRIBUTE_UNUSED,
> _("Unable to set bridge vlan_filtering on this platform"));
> return -1;
> }
> +
Spurrious?
> #endif
> +
> +
> +
Three lines... Just two usually...
> +#if defined(__linux__) && defined(HAVE_LIBNL)
> +static int
> +virNetDevBridgeFDBAddDel(const virMacAddr *mac, const char *ifname,
> + unsigned int flags, bool isAdd)
> +{
> + int ret = -1;
> + struct nlmsghdr *resp = NULL;
> + struct nlmsgerr *err;
> + unsigned int recvbuflen;
> + struct nl_msg *nl_msg;
> + struct ndmsg ndm = { .ndm_family = PF_BRIDGE, .ndm_state = NUD_NOARP };
> +
> + if (virNetDevGetIndex(ifname, &ndm.ndm_ifindex) < 0)
> + return -1;
> +
> + if (flags & VIR_NETDEVBRIDGE_FDB_FLAG_ROUTER)
> + ndm.ndm_flags |= NTF_ROUTER;
> + if (flags & VIR_NETDEVBRIDGE_FDB_FLAG_SELF)
> + ndm.ndm_flags |= NTF_SELF;
> + if (flags & VIR_NETDEVBRIDGE_FDB_FLAG_MASTER)
> + ndm.ndm_flags |= NTF_MASTER;
> + /* default self (same as iproute2's bridge command */
> + if (!(ndm.ndm_flags & (NTF_MASTER | NTF_SELF)))
> + ndm.ndm_flags |= NTF_SELF;
> +
> + if (flags & VIR_NETDEVBRIDGE_FDB_FLAG_PERMANENT)
> + ndm.ndm_state |= NUD_PERMANENT;
> + if (flags & VIR_NETDEVBRIDGE_FDB_FLAG_TEMP)
> + ndm.ndm_state |= NUD_REACHABLE;
> + /* default permanent, same as iproute2's bridge command */
> + if (!(ndm.ndm_state & (NUD_PERMANENT | NUD_REACHABLE)))
> + ndm.ndm_state |= NUD_PERMANENT;
> +
> + nl_msg = nlmsg_alloc_simple(isAdd ? RTM_NEWNEIGH : RTM_DELNEIGH,
> + NLM_F_REQUEST |
> + (isAdd ? (NLM_F_CREATE | NLM_F_EXCL) : 0));
> + if (!nl_msg) {
> + virReportOOMError();
> + return -1;
> + }
> +
> + if (nlmsg_append(nl_msg, &ndm, sizeof(ndm), NLMSG_ALIGNTO) < 0)
> + goto buffer_too_small;
> + if (nla_put(nl_msg, NDA_LLADDR, VIR_MAC_BUFLEN, mac) < 0)
> + goto buffer_too_small;
So if someone adds the same thing twice, what happens? Does it matter?
IOW: Is there any need for us to check a return status here that
indicates "duplicate entry" and ignore? Or is there any need for a
*FDBGet() function to determine whether what we're about to add already
exists?
Similar argument of course for delete, but removing something that
doesn't exist - what happens?
Then course there's the "FDBModify()" type function. We have something,
but want to change a setting.
[yes, I know nothing about fdb's, but when I see database mentioned all
sorts of mgmt functions come to mind]
> +
> + /* NB: this message can also accept a Destination IP, a port, a
> + * vlan tag, and a via (see iproute2/bridge/fdb.c:fdb_modify()),
> + * but those aren't required for our application
> + */
> +
> + if (virNetlinkCommand(nl_msg, &resp, &recvbuflen, 0, 0,
> + NETLINK_ROUTE, 0) < 0) {
> + goto cleanup;
> + }
> + if (recvbuflen < NLMSG_LENGTH(0) || resp == NULL)
> + goto malformed_resp;
> +
> + switch (resp->nlmsg_type) {
> + case NLMSG_ERROR:
> + err = (struct nlmsgerr *)NLMSG_DATA(resp);
> + if (resp->nlmsg_len < NLMSG_LENGTH(sizeof(*err)))
> + goto malformed_resp;
> + if (err->error) {
> + virReportSystemError(-err->error,
> + _("error adding fdb entry for %s"), ifname);
> + goto cleanup;
> + }
> + break;
> + case NLMSG_DONE:
> + break;
> +
> + default:
> + goto malformed_resp;
> + }
> +
> + ret = 0;
> + cleanup:
> + nlmsg_free(nl_msg);
> + VIR_FREE(resp);
> + return ret;
> +
> + malformed_resp:
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("malformed netlink response message"));
> + goto cleanup;
> +
> + buffer_too_small:
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("allocated netlink buffer is too small"));
> + goto cleanup;
> +}
> +
> +
> +#else
> +static int
> +virNetDevBridgeFDBAddDel(const virMacAddr *mac, const char *ifname,
> + unsigned int flags, bool isAdd)
interesting - is because it's static the compiler didn't complain about
ATTRIBUTE_UNUSED (1-4)?
> +{
> + virReportSystemError(ENOSYS, "%s",
> + _("Unable to add/delete fdb entries on this platform"));
You could have gone with "Unable to %s fdb entries on this platform",
isAdd ? "add" : "delete")
to really hide that you didn't write separate "Add" and "Del" functions.
> + return -1;
> +}
> +
> +
> +#endif
> +
> +int
> +virNetDevBridgeFDBAdd(const virMacAddr *mac, const char *ifname,
> + unsigned int flags)
> +{
> + return virNetDevBridgeFDBAddDel(mac, ifname, flags, true);
> +}
> +
> +
Thinking out loud for the future patches. The 'flags' here - must they
match how they were added? Or do they really matter on the delete
function? I see no callers to this in any other future patch nor is
there much usage of flags for the Add function (only MASTER | TEMP).
For what's here - seems fine... ACK with nits for the extraneous lines.
Although I still am curious about where things go from here...
John
> +int
> +virNetDevBridgeFDBDel(const virMacAddr *mac, const char *ifname,
> + unsigned int flags)
> +{
> + return virNetDevBridgeFDBAddDel(mac, ifname, flags, false);
> +}
> diff --git a/src/util/virnetdevbridge.h b/src/util/virnetdevbridge.h
> index 8188a2f..da116b9 100644
> --- a/src/util/virnetdevbridge.h
> +++ b/src/util/virnetdevbridge.h
> @@ -24,6 +24,7 @@
> # define __VIR_NETDEV_BRIDGE_H__
>
> # include "internal.h"
> +# include "virmacaddr.h"
>
> int virNetDevBridgeCreate(const char *brname)
> ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
> @@ -77,4 +78,19 @@ int virNetDevBridgePortSetUnicastFlood(const char *brname,
> bool enable)
> ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
>
> +enum {
> + VIR_NETDEVBRIDGE_FDB_FLAG_ROUTER = (1 << 0),
> + VIR_NETDEVBRIDGE_FDB_FLAG_SELF = (1 << 1),
> + VIR_NETDEVBRIDGE_FDB_FLAG_MASTER = (1 << 2),
> +
> + VIR_NETDEVBRIDGE_FDB_FLAG_PERMANENT = (1 << 3),
> + VIR_NETDEVBRIDGE_FDB_FLAG_TEMP = (1 << 4),
> +};
> +
> +int virNetDevBridgeFDBAdd(const virMacAddr *mac, const char *ifname,
> + unsigned int flags)
> + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
> +int virNetDevBridgeFDBDel(const virMacAddr *mac, const char *ifname,
> + unsigned int flags)
> + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
> #endif /* __VIR_NETDEV_BRIDGE_H__ */
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list