On 17 May 2017 at 09:55, Eric Garver <e...@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...@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 <casca...@redhat.com> >> > Signed-off-by: Thadeu Lima de Souza Cascardo <casca...@redhat.com> >> > Signed-off-by: Eric Garver <e...@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. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev