[ovs-dev] [PATCH v4 3/7] dpif-netlink: Support rtnetlink port creation.

Joe Stringer joe at ovn.org
Wed May 17 17:47:41 UTC 2017


On 17 May 2017 at 09:55, Eric Garver <e at erig.me> wrote:
> On Tue, May 16, 2017 at 06:16:03PM -0700, Joe Stringer wrote:
>> On 7 May 2017 at 06:43, Eric Garver <e at erig.me> wrote:
>> > In order to be able to add those tunnels, we need to add code to create
>> > the tunnels and add them as NETDEV vports. And when there is no support
>> > to create them, we need to fallback to compatibility code and add them
>> > as tunnel vports.
>> >
>> > When removing those tunnels, we need to remove the interfaces as well,
>> > and detecting the right type might be important, at least to distinguish
>> > the tunnel vports that we should remove and the interfaces that we
>> > shouldn't.
>> >
>> > Co-authored-by: Thadeu Lima de Souza Cascardo <cascardo at redhat.com>
>> > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo at redhat.com>
>> > Signed-off-by: Eric Garver <e at erig.me>
>> > ---
>> >  lib/automake.mk         |   3 +
>> >  lib/dpif-netlink-rtnl.c | 227 ++++++++++++++++++++++++++++++++++++++++++++++++
>> >  lib/dpif-netlink-rtnl.h |  47 ++++++++++
>> >  lib/dpif-netlink.c      |  29 ++++++-
>> >  lib/dpif-netlink.h      |   2 +
>> >  5 files changed, 307 insertions(+), 1 deletion(-)
>> >  create mode 100644 lib/dpif-netlink-rtnl.c
>> >  create mode 100644 lib/dpif-netlink-rtnl.h
>> >
>> > diff --git a/lib/automake.mk b/lib/automake.mk
>> > index faace7993e79..f5baba27179f 100644
>> > --- a/lib/automake.mk
>> > +++ b/lib/automake.mk
>> > @@ -352,6 +352,8 @@ if LINUX
>> >  lib_libopenvswitch_la_SOURCES += \
>> >         lib/dpif-netlink.c \
>> >         lib/dpif-netlink.h \
>> > +       lib/dpif-netlink-rtnl.c \
>> > +       lib/dpif-netlink-rtnl.h \
>> >         lib/if-notifier.c \
>> >         lib/if-notifier.h \
>> >         lib/netdev-linux.c \
>> > @@ -382,6 +384,7 @@ if WIN32
>> >  lib_libopenvswitch_la_SOURCES += \
>> >         lib/dpif-netlink.c \
>> >         lib/dpif-netlink.h \
>> > +       lib/dpif-netlink-rtnl.h \
>> >         lib/netdev-windows.c \
>> >         lib/netlink-conntrack.c \
>> >         lib/netlink-conntrack.h \
>> > diff --git a/lib/dpif-netlink-rtnl.c b/lib/dpif-netlink-rtnl.c
>> > new file mode 100644
>> > index 000000000000..906e05145190
>> > --- /dev/null
>> > +++ b/lib/dpif-netlink-rtnl.c
>> > @@ -0,0 +1,227 @@
>> > +/*
>> > + * Copyright (c) 2017 Red Hat, Inc.
>> > + *
>> > + * Licensed under the Apache License, Version 2.0 (the "License");
>> > + * you may not use this file except in compliance with the License.
>> > + * You may obtain a copy of the License at:
>> > + *
>> > + *     http://www.apache.org/licenses/LICENSE-2.0
>> > + *
>> > + * Unless required by applicable law or agreed to in writing, software
>> > + * distributed under the License is distributed on an "AS IS" BASIS,
>> > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
>> > + * See the License for the specific language governing permissions and
>> > + * limitations under the License.
>> > + */
>> > +
>> > +#include <config.h>
>> > +
>> > +#include "dpif-netlink-rtnl.h"
>> > +
>> > +#include <net/if.h>
>> > +#include <linux/ip.h>
>> > +#include <linux/rtnetlink.h>
>> > +
>> > +#include "dpif-netlink.h"
>> > +#include "netdev-vport.h"
>> > +#include "netlink-socket.h"
>> > +
>> > +static const struct nl_policy rtlink_policy[] = {
>> > +    [IFLA_LINKINFO] = { .type = NL_A_NESTED },
>> > +};
>> > +static const struct nl_policy linkinfo_policy[] = {
>> > +    [IFLA_INFO_KIND] = { .type = NL_A_STRING },
>> > +    [IFLA_INFO_DATA] = { .type = NL_A_NESTED },
>> > +};
>> > +
>> > +static int
>> > +rtnl_transact(uint32_t type, uint32_t flags, const char *name,
>> > +              struct ofpbuf **reply)
>> > +{
>> > +    struct ofpbuf request;
>> > +    int err;
>> > +
>> > +    ofpbuf_init(&request, 0);
>> > +    nl_msg_put_nlmsghdr(&request, 0, type, flags);
>> > +    ofpbuf_put_zeros(&request, sizeof(struct ifinfomsg));
>> > +    nl_msg_put_string(&request, IFLA_IFNAME, name);
>> > +
>> > +    err = nl_transact(NETLINK_ROUTE, &request, reply);
>> > +    ofpbuf_uninit(&request);
>> > +
>> > +    return err;
>> > +}
>> > +
>> > +static int
>> > +dpif_netlink_rtnl_destroy(const char *name)
>> > +{
>> > +    return rtnl_transact(RTM_DELLINK, NLM_F_REQUEST | NLM_F_ACK, name, NULL);
>> > +}
>> > +
>> > +static int OVS_UNUSED
>> > +dpif_netlink_rtnl_getlink(const char *name, struct ofpbuf **reply)
>> > +{
>> > +    return rtnl_transact(RTM_GETLINK, NLM_F_REQUEST, name, reply);
>> > +}
>> > +
>> > +static int OVS_UNUSED
>> > +rtnl_policy_parse(const char *kind, struct ofpbuf *reply,
>> > +                  const struct nl_policy *policy,
>> > +                  struct nlattr *tnl_info[],
>> > +                  size_t policy_size)
>> > +{
>> > +    struct nlattr *linkinfo[ARRAY_SIZE(linkinfo_policy)];
>> > +    struct nlattr *rtlink[ARRAY_SIZE(rtlink_policy)];
>> > +    struct ifinfomsg *ifmsg;
>> > +    int error = 0;
>> > +
>> > +    ifmsg = ofpbuf_at(reply, NLMSG_HDRLEN, sizeof *ifmsg);
>> > +    if (!nl_policy_parse(reply, NLMSG_HDRLEN + sizeof *ifmsg,
>> > +                         rtlink_policy, rtlink, ARRAY_SIZE(rtlink_policy))
>> > +        || !nl_parse_nested(rtlink[IFLA_LINKINFO], linkinfo_policy,
>> > +                            linkinfo, ARRAY_SIZE(linkinfo_policy))
>> > +        || strcmp(nl_attr_get_string(linkinfo[IFLA_INFO_KIND]), kind)
>> > +        || !nl_parse_nested(linkinfo[IFLA_INFO_DATA], policy,
>> > +                            tnl_info, policy_size)) {
>> > +        error = EINVAL;
>> > +    }
>> > +
>> > +    return error;
>> > +}
>> > +
>> > +static int
>> > +dpif_netlink_rtnl_verify(const struct netdev_tunnel_config OVS_UNUSED *tnl_cfg,
>> > +                         enum ovs_vport_type type, const char OVS_UNUSED *name)
>> > +{
>> > +    switch (type) {
>> > +    case OVS_VPORT_TYPE_VXLAN:
>> > +    case OVS_VPORT_TYPE_GRE:
>> > +    case OVS_VPORT_TYPE_GENEVE:
>> > +    case OVS_VPORT_TYPE_NETDEV:
>> > +    case OVS_VPORT_TYPE_INTERNAL:
>> > +    case OVS_VPORT_TYPE_LISP:
>> > +    case OVS_VPORT_TYPE_STT:
>> > +    case OVS_VPORT_TYPE_UNSPEC:
>> > +    case __OVS_VPORT_TYPE_MAX:
>> > +    default:
>> > +        return EOPNOTSUPP;
>> > +    }
>> > +
>> > +    return 0;
>> > +}
>> > +
>> > +static int OVS_UNUSED
>> > +dpif_netlink_rtnl_create(const struct netdev_tunnel_config OVS_UNUSED *tnl_cfg,
>> > +                         const char *name, enum ovs_vport_type type,
>> > +                         const char *kind, uint32_t flags)
>> > +{
>> > +    size_t linkinfo_off, infodata_off;
>> > +    struct ifinfomsg *ifinfo;
>> > +    struct ofpbuf request;
>> > +    int err;
>> > +
>> > +    ofpbuf_init(&request, 0);
>> > +    nl_msg_put_nlmsghdr(&request, 0, RTM_NEWLINK, flags);
>> > +    ifinfo = ofpbuf_put_zeros(&request, sizeof(struct ifinfomsg));
>> > +    ifinfo->ifi_change = ifinfo->ifi_flags = IFF_UP;
>> > +    nl_msg_put_string(&request, IFLA_IFNAME, name);
>> > +    nl_msg_put_u32(&request, IFLA_MTU, UINT16_MAX);
>> > +    linkinfo_off = nl_msg_start_nested(&request, IFLA_LINKINFO);
>> > +    nl_msg_put_string(&request, IFLA_INFO_KIND, kind);
>> > +    infodata_off = nl_msg_start_nested(&request, IFLA_INFO_DATA);
>> > +
>> > +    /* tunnel unique info */
>> > +    switch (type) {
>> > +    case OVS_VPORT_TYPE_VXLAN:
>> > +    case OVS_VPORT_TYPE_GRE:
>> > +    case OVS_VPORT_TYPE_GENEVE:
>> > +    case OVS_VPORT_TYPE_NETDEV:
>> > +    case OVS_VPORT_TYPE_INTERNAL:
>> > +    case OVS_VPORT_TYPE_LISP:
>> > +    case OVS_VPORT_TYPE_STT:
>> > +    case OVS_VPORT_TYPE_UNSPEC:
>> > +    case __OVS_VPORT_TYPE_MAX:
>> > +    default:
>> > +        err = EOPNOTSUPP;
>> > +        goto exit;
>> > +    }
>> > +
>> > +    nl_msg_end_nested(&request, infodata_off);
>> > +    nl_msg_end_nested(&request, linkinfo_off);
>> > +
>> > +    err = nl_transact(NETLINK_ROUTE, &request, NULL);
>> > +
>> > +exit:
>> > +    ofpbuf_uninit(&request);
>> > +
>> > +    return err;
>> > +}
>> > +
>> > +int
>> > +dpif_netlink_rtnl_port_create(struct netdev *netdev)
>> > +{
>> > +    const struct netdev_tunnel_config *tnl_cfg;
>> > +    char namebuf[NETDEV_VPORT_NAME_BUFSIZE];
>> > +    enum ovs_vport_type type;
>> > +    bool retried = false;
>> > +    const char *name;
>> > +    uint32_t flags;
>> > +    int err;
>> > +
>> > +    tnl_cfg = netdev_get_tunnel_config(netdev);
>> > +    if (!tnl_cfg) {
>> > +        return EINVAL;
>> > +    }
>> > +
>> > +    type = netdev_to_ovs_vport_type(netdev_get_type(netdev));
>> > +    name = netdev_vport_get_dpif_port(netdev, namebuf, sizeof namebuf);
>> > +    flags = NLM_F_REQUEST | NLM_F_ACK | NLM_F_CREATE | NLM_F_EXCL;
>> > +
>> > +try_again:
>> > +    switch (type) {
>> > +    case OVS_VPORT_TYPE_VXLAN:
>> > +    case OVS_VPORT_TYPE_GRE:
>> > +    case OVS_VPORT_TYPE_GENEVE:
>> > +    case OVS_VPORT_TYPE_NETDEV:
>> > +    case OVS_VPORT_TYPE_INTERNAL:
>> > +    case OVS_VPORT_TYPE_LISP:
>> > +    case OVS_VPORT_TYPE_STT:
>> > +    case OVS_VPORT_TYPE_UNSPEC:
>> > +    case __OVS_VPORT_TYPE_MAX:
>> > +    default:
>> > +        err = EOPNOTSUPP;
>> > +    }
>> > +
>> > +    if (!err || (err == EEXIST && !retried)) {
>> > +        int err2 = dpif_netlink_rtnl_verify(tnl_cfg, type, name);
>> > +        if (err2 && err == EEXIST) {
>> > +            err2 = dpif_netlink_rtnl_destroy(name);
>> > +            if (!err2) {
>> > +                retried = true;
>> > +                goto try_again;
>> > +            }
>> > +        }
>> > +        err = err2;
>> > +    }
>>
>> I found the above a bit tricky to follow: combines error and success
>> cases, backwards loops with an extra bool, etc. I wonder if it's
>> actually easier to grok if it's just written out longhand. Here's a
>> suggested diff on top of the series that might achieve this (with a
>> couple of extra bits of logging), do you think it improves the patch?
>
> I initially wrote it long hand style, but thought it a bit repetitive.

If it's repetitive but more readable, I think that's a better tradeoff
than terse and harder to read.

> I'll send another revision after making this changes and looking into
> the GENEVE issue.

Thanks! This version is definitely looking tidier than the previous so
I hope we can apply the series next round.


More information about the dev mailing list