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?
---
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;
--
2.11.1
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
