[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 2/9] util: functions to manage bridge fdb (forwarding database)




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__ */
> 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]