From patchwork Wed May 17 01:16:03 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Joe Stringer X-Patchwork-Id: 763274 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from mail.linuxfoundation.org (mail.linuxfoundation.org [140.211.169.12]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3wSGcv1BpQz9s84 for ; Wed, 17 May 2017 11:17:03 +1000 (AEST) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id D0912B62; Wed, 17 May 2017 01:16:31 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@mail.linuxfoundation.org Received: from smtp1.linuxfoundation.org (smtp1.linux-foundation.org [172.17.192.35]) by mail.linuxfoundation.org (Postfix) with ESMTPS id 6CF7DB5F for ; Wed, 17 May 2017 01:16:30 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net [217.70.183.194]) by smtp1.linuxfoundation.org (Postfix) with ESMTPS id 34C0B1B3 for ; Wed, 17 May 2017 01:16:29 +0000 (UTC) Received: from mfilter11-d.gandi.net (mfilter11-d.gandi.net [217.70.178.131]) by relay2-d.mail.gandi.net (Postfix) with ESMTP id A36F0C5A5F for ; Wed, 17 May 2017 03:16:27 +0200 (CEST) X-Virus-Scanned: Debian amavisd-new at mfilter11-d.gandi.net Received: from relay2-d.mail.gandi.net ([IPv6:::ffff:217.70.183.194]) by mfilter11-d.gandi.net (mfilter11-d.gandi.net [::ffff:10.0.15.180]) (amavisd-new, port 10024) with ESMTP id BA6Em8tfq2Xe for ; Wed, 17 May 2017 03:16:25 +0200 (CEST) X-Originating-IP: 209.85.220.176 Received: from mail-qk0-f176.google.com (mail-qk0-f176.google.com [209.85.220.176]) (Authenticated sender: joe@ovn.org) by relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 889EAC5A51 for ; Wed, 17 May 2017 03:16:25 +0200 (CEST) Received: by mail-qk0-f176.google.com with SMTP id k74so145169907qke.1 for ; Tue, 16 May 2017 18:16:25 -0700 (PDT) X-Gm-Message-State: AODbwcBHNlFc+xWjbO2I5vxIQNZXe0d+2Z1AOVgDvZAjKnQuQJ2LCiYe 4DxKsZNwcCJMHs1uFd/s4h6LwqI3dg== X-Received: by 10.55.101.20 with SMTP id z20mr679362qkb.148.1494983784175; Tue, 16 May 2017 18:16:24 -0700 (PDT) MIME-Version: 1.0 Received: by 10.12.174.123 with HTTP; Tue, 16 May 2017 18:16:03 -0700 (PDT) In-Reply-To: <20170507134343.2709-4-e@erig.me> References: <20170507134343.2709-1-e@erig.me> <20170507134343.2709-4-e@erig.me> From: Joe Stringer Date: Tue, 16 May 2017 18:16:03 -0700 X-Gmail-Original-Message-ID: Message-ID: To: Eric Garver X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_LOW, RCVD_IN_SORBS_SPAM autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on smtp1.linux-foundation.org Cc: ovs dev Subject: Re: [ovs-dev] [PATCH v4 3/7] dpif-netlink: Support rtnetlink port creation. X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.12 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: ovs-dev-bounces@openvswitch.org Errors-To: ovs-dev-bounces@openvswitch.org On 7 May 2017 at 06:43, Eric Garver 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 > Signed-off-by: Thadeu Lima de Souza Cascardo > Signed-off-by: Eric Garver > --- > 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 > + > +#include "dpif-netlink-rtnl.h" > + > +#include > +#include > +#include > + > +#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? --- lib/dpif-netlink-rtnl.c | 91 +++++++++++++++++++++++++++++++++---------------- 1 file changed, 62 insertions(+), 29 deletions(-) diff --git a/lib/dpif-netlink-rtnl.c b/lib/dpif-netlink-rtnl.c index 2669750a65bf..a9d5178cd575 100644 --- a/lib/dpif-netlink-rtnl.c +++ b/lib/dpif-netlink-rtnl.c @@ -25,6 +25,9 @@ #include "dpif-netlink.h" #include "netdev-vport.h" #include "netlink-socket.h" +#include "openvswitch/vlog.h" + +VLOG_DEFINE_THIS_MODULE(dpif_netlink_rtnl); /* On some older systems, these enums are not defined. */ #ifndef IFLA_VXLAN_MAX @@ -304,13 +307,36 @@ exit: return err; } +static const char * +vport_type_to_kind(enum ovs_vport_type type) +{ + switch (type) { + case OVS_VPORT_TYPE_VXLAN: + return "vxlan"; + case OVS_VPORT_TYPE_GRE: + return "gretap"; + case OVS_VPORT_TYPE_GENEVE: + return "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: + break; + } + + return NULL; +} + 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 *kind; const char *name; uint32_t flags; int err; @@ -321,40 +347,47 @@ dpif_netlink_rtnl_port_create(struct netdev *netdev) } type = netdev_to_ovs_vport_type(netdev_get_type(netdev)); + kind = vport_type_to_kind(type); + if (!kind) { + return EOPNOTSUPP; + } + 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: - err = dpif_netlink_rtnl_create(tnl_cfg, name, type, "vxlan", flags); - break; - case OVS_VPORT_TYPE_GRE: - err = dpif_netlink_rtnl_create(tnl_cfg, name, type, "gretap", flags); - break; - case OVS_VPORT_TYPE_GENEVE: - err = dpif_netlink_rtnl_create(tnl_cfg, name, type, "geneve", flags); - break; - 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; - } + err = dpif_netlink_rtnl_create(tnl_cfg, name, type, kind, flags); - 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; + /* If the device exists, validate and/or attempt to recreate it. */ + if (err == EEXIST) { + err = dpif_netlink_rtnl_verify(tnl_cfg, type, name); + if (!err) { + return 0; + } else { + err = dpif_netlink_rtnl_destroy(name); + if (err) { + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); + + VLOG_WARN_RL(&rl, "RTNL device %s exists and cannot be " + "deleted: %s", name, ovs_strerror(err)); + return err; } + err = dpif_netlink_rtnl_create(tnl_cfg, name, type, kind, flags); + } + } + if (err) { + return err; + } + + err = dpif_netlink_rtnl_verify(tnl_cfg, type, name); + if (err) { + int err2 = dpif_netlink_rtnl_destroy(name); + + if (err2) { + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); + + VLOG_WARN_RL(&rl, "Failed to delete device %s during rtnl port " + "creation: %s", name, ovs_strerror(err2)); } - err = err2; } return err;