| Message ID | CAPWQB7E3pM=ppixuoFa4Xha67xsT-PqbdMCLv5eVXm8qxyfNag@mail.gmail.com |
|---|---|
| State | Not Applicable |
| Headers | show |
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 <cascardo@redhat.com> > > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@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. I'll send another revision after making this changes and looking into the GENEVE issue. Thanks. Eric. > > --- > 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
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 <cascardo@redhat.com> >> > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@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.
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;