On Tue, Mar 14, 2017 at 01:09:20PM -0700, Joe Stringer wrote:
> On 16 February 2017 at 14:25, 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.c | 58
> > ++++++++++++++++++++++++++++++++++++++++++++------
> > lib/dpif-netlink.h | 2 ++
> > lib/dpif-rtnetlink.c | 60
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > lib/dpif-rtnetlink.h | 46 ++++++++++++++++++++++++++++++++++++++++
> > 5 files changed, 162 insertions(+), 7 deletions(-)
> > create mode 100644 lib/dpif-rtnetlink.c
> > create mode 100644 lib/dpif-rtnetlink.h
> >
> > diff --git a/lib/automake.mk b/lib/automake.mk
> > index abc9d0d5cc4e..288a828d9007 100644
> > --- a/lib/automake.mk
> > +++ b/lib/automake.mk
> > @@ -351,6 +351,8 @@ if LINUX
> > lib_libopenvswitch_la_SOURCES += \
> > lib/dpif-netlink.c \
> > lib/dpif-netlink.h \
> > + lib/dpif-rtnetlink.c \
> > + lib/dpif-rtnetlink.h \
> > lib/if-notifier.c \
> > lib/if-notifier.h \
> > lib/netdev-linux.c \
> > @@ -381,6 +383,7 @@ if WIN32
> > lib_libopenvswitch_la_SOURCES += \
> > lib/dpif-netlink.c \
> > lib/dpif-netlink.h \
> > + lib/dpif-rtnetlink.h \
> > lib/netdev-windows.c \
> > lib/netlink-conntrack.c \
> > lib/netlink-conntrack.h \
> > diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> > index a882ea683114..ae8204ffe6e1 100644
> > --- a/lib/dpif-netlink.c
> > +++ b/lib/dpif-netlink.c
> > @@ -34,6 +34,7 @@
> >
> > #include "bitmap.h"
> > #include "dpif-provider.h"
> > +#include "dpif-rtnetlink.h"
> > #include "openvswitch/dynamic-string.h"
> > #include "flow.h"
> > #include "fat-rwlock.h"
> > @@ -221,6 +222,9 @@ static void dpif_netlink_vport_to_ofpbuf(const struct
> > dpif_netlink_vport *,
> > struct ofpbuf *);
> > static int dpif_netlink_vport_from_ofpbuf(struct dpif_netlink_vport *,
> > const struct ofpbuf *);
> > +static int
> > +dpif_netlink_port_query__(const struct dpif_netlink *dpif, odp_port_t
> > port_no,
> > + const char *port_name, struct dpif_port
> > *dpif_port);
> >
> > static struct dpif_netlink *
> > dpif_netlink_cast(const struct dpif *dpif)
> > @@ -780,7 +784,7 @@ get_vport_type(const struct dpif_netlink_vport *vport)
> > return "unknown";
> > }
> >
> > -static enum ovs_vport_type
> > +enum ovs_vport_type
> > netdev_to_ovs_vport_type(const char *type)
> > {
> > if (!strcmp(type, "tap") || !strcmp(type, "system")) {
> > @@ -942,8 +946,33 @@ dpif_netlink_port_add_compat(struct dpif_netlink
> > *dpif, struct netdev *netdev,
> >
> > }
> >
> > +static int
> > +dpif_rtnetlink_port_create_and_add(struct dpif_netlink *dpif,
> > + struct netdev *netdev, odp_port_t
> > *port_nop)
> > + OVS_REQ_WRLOCK(dpif->upcall_lock)
> > +{
> > + int error;
> > + char namebuf[NETDEV_VPORT_NAME_BUFSIZE];
> > + const char *name = netdev_vport_get_dpif_port(netdev,
> > + namebuf, sizeof namebuf);
> >
> > + error = dpif_rtnetlink_port_create(netdev);
> > + if (error) {
> > + if (error != EOPNOTSUPP) {
> > + VLOG_DBG("Failed to create %s with rtnetlink. error = %d",
> > + netdev_get_name(netdev), error);
> > + }
> > + return error;
> > + }
> >
> > + error = dpif_netlink_port_add__(dpif, name, OVS_VPORT_TYPE_NETDEV,
> > NULL,
> > + port_nop);
> > + if (error) {
> > + VLOG_DBG("failed to add port, destroying: %d", error);
> > + dpif_rtnetlink_port_destroy(name, netdev_get_type(netdev));
> > + }
> > + return error;
> > +}
>
> If these non-EOPNOTSUPP errors could show up in real environments, it
> might be nice to have them at a higher level than debug to highlight
> that something went wrong. If so, they should also be ratelimited by
> using VLOG_FOO_RL(&rl, ...)
True. An example would be a tunnel parameter that an older kernel
doesn't support, e.g. VXLAN GBP. I'll change them to VLOG_INFO_RL().
> >
> > static int
> > dpif_netlink_port_add(struct dpif *dpif_, struct netdev *netdev,
> > @@ -953,7 +982,10 @@ dpif_netlink_port_add(struct dpif *dpif_, struct
> > netdev *netdev,
> > int error;
> >
> > fat_rwlock_wrlock(&dpif->upcall_lock);
> > - error = dpif_netlink_port_add_compat(dpif, netdev, port_nop);
> > + error = dpif_rtnetlink_port_create_and_add(dpif, netdev, port_nop);
> > + if (error == EOPNOTSUPP) {
> > + error = dpif_netlink_port_add_compat(dpif, netdev, port_nop);
> > + }
> > fat_rwlock_unlock(&dpif->upcall_lock);
> >
> > return error;
> > @@ -964,19 +996,23 @@ dpif_netlink_port_del__(struct dpif_netlink *dpif,
> > odp_port_t port_no)
> > OVS_REQ_WRLOCK(dpif->upcall_lock)
> > {
> > struct dpif_netlink_vport vport;
> > + struct dpif_port dpif_port;
> > int error;
> >
> > + error = dpif_netlink_port_query__(dpif, port_no, NULL, &dpif_port);
> > + if (error) {
> > + return error;
> > + }
> > +
> > dpif_netlink_vport_init(&vport);
> > vport.cmd = OVS_VPORT_CMD_DEL;
> > vport.dp_ifindex = dpif->dp_ifindex;
> > vport.port_no = port_no;
> > #ifdef _WIN32
> > - struct dpif_port temp_dpif_port;
> > - dpif_netlink_port_query__(dpif, port_no, NULL, &temp_dpif_port);
> > - if (!strcmp(temp_dpif_port.type, "internal")) {
> > - if (!delete_wmi_port(temp_dpif_port.name)){
> > + if (!strcmp(dpif_port.type, "internal")) {
> > + if (!delete_wmi_port(dpif_port.name)) {
> > VLOG_ERR("Could not delete wmi port with name: %s",
> > - temp_dpif_port.name);
> > + dpif_port.name);
> > };
> > }
> > #endif
> > @@ -984,6 +1020,14 @@ dpif_netlink_port_del__(struct dpif_netlink *dpif,
> > odp_port_t port_no)
> >
> > vport_del_channels(dpif, port_no);
> >
> > + if (!error) {
> > + error = dpif_rtnetlink_port_destroy(dpif_port.name,
> > dpif_port.type);
> > + if (error == EOPNOTSUPP) {
> > + error = 0;
> > + }
> > + }
> > + dpif_port_destroy(&dpif_port);
> > +
> > return error;
> > }
> >
> > diff --git a/lib/dpif-netlink.h b/lib/dpif-netlink.h
> > index 6d1505713a72..568b81441ddc 100644
> > --- a/lib/dpif-netlink.h
> > +++ b/lib/dpif-netlink.h
> > @@ -58,4 +58,6 @@ int dpif_netlink_vport_get(const char *name, struct
> > dpif_netlink_vport *reply,
> >
> > bool dpif_netlink_is_internal_device(const char *name);
> >
> > +enum ovs_vport_type netdev_to_ovs_vport_type(const char *type);
> > +
> > #endif /* dpif-netlink.h */
> > diff --git a/lib/dpif-rtnetlink.c b/lib/dpif-rtnetlink.c
> > new file mode 100644
> > index 000000000000..b309c88d187a
> > --- /dev/null
> > +++ b/lib/dpif-rtnetlink.c
> > @@ -0,0 +1,60 @@
> > +/*
> > + * 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-rtnetlink.h"
> > +
> > +#include "dpif-netlink.h"
>
> Usually, #includes within the lib/ directory will be consecutive
> without whitespace separation.
Many files separate the header file of the same name.
e.g.
#include <config.h>
#include <thisfilesheader.h>
#include ... OS HEADERS ...
#include ... OVS HEADERS ...
>
> > +
> > +
> > +int
> > +dpif_rtnetlink_port_create(struct netdev *netdev)
> > +{
> > + switch (netdev_to_ovs_vport_type(netdev_get_type(netdev))) {
> > + 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;
> > +}
> > +
> > +int
> > +dpif_rtnetlink_port_destroy(const char *name OVS_UNUSED, const char *type)
> > +{
> > + switch (netdev_to_ovs_vport_type(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;
> > +}
> > diff --git a/lib/dpif-rtnetlink.h b/lib/dpif-rtnetlink.h
> > new file mode 100644
> > index 000000000000..56ab63532829
> > --- /dev/null
> > +++ b/lib/dpif-rtnetlink.h
> > @@ -0,0 +1,46 @@
> > +/*
> > + * 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.
> > + */
> > +
> > +#ifndef DPIF_RTNETLINK_H
> > +#define DPIF_RTNETLINK_H 1
> > +
> > +#include <errno.h>
> > +
> > +#include "netdev.h"
> > +
>
> Perhaps here, you could put the comment from below, eg
>
> /* Declare these functions to keep sparse happy. */
> > +int dpif_rtnetlink_port_create(struct netdev *netdev);
> > +int dpif_rtnetlink_port_destroy(const char *name OVS_UNUSED, const char
> > *type);
> > +
> > +#ifndef __linux__
> > +/* Dummy implementations for non Linux builds.
> > + *
> > + * NOTE: declaration above are for all platforms to keep sparse happy.
>
> Then, this can reduce to a regular one-line comment (and it's clearer
> what the comments refer to).
Will do.
> > + */
> > +
> > +static inline int dpif_rtnetlink_port_create(struct netdev *netdev
> > OVS_UNUSED)
> > +{
> > + return EOPNOTSUPP;
> > +}
> > +
> > +static inline int dpif_rtnetlink_port_destroy(const char *name OVS_UNUSED,
> > + const char *type OVS_UNUSED)
> > +{
> > + return EOPNOTSUPP;
> > +}
> > +
> > +#endif
> > +
> > +#endif /* DPIF_RTNETLINK_H */
> > --
> > 2.10.0
> >
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
