On 18 January 2017 at 11:45, Eric Garver <[email protected]> wrote: > From: Thadeu Lima de Souza Cascardo <[email protected]> > > The vport type for adding tunnels is now compatibility code and any new > features > from tunnels must configure the tunnel as an interface using the tunnel > metadata > support. > > 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 use the 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. > > Signed-off-by: Thadeu Lima de Souza Cascardo <[email protected]>
This patch involves refactoring as well as the base changes to add/remove the new tunnel types. It's always easier to review if functional changes are proposed separately from refactoring/cosmetic. (I recognize the new functions don't do anything, but they still combine with the refactoring). That said, with some minor changes this looks OK to me. Acked-by: Joe Stringer <[email protected]> > --- > lib/dpif-netlink.c | 201 > +++++++++++++++++++++++++++++++++++++++-------------- > 1 file changed, 148 insertions(+), 53 deletions(-) > > diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c > index c8b0e37f9365..e6459f358989 100644 > --- a/lib/dpif-netlink.c > +++ b/lib/dpif-netlink.c > @@ -781,10 +781,8 @@ get_vport_type(const struct dpif_netlink_vport *vport) > } > > static enum ovs_vport_type > -netdev_to_ovs_vport_type(const struct netdev *netdev) > +netdev_to_ovs_vport_type(const char *type) > { > - const char *type = netdev_get_type(netdev); > - > if (!strcmp(type, "tap") || !strcmp(type, "system")) { > return OVS_VPORT_TYPE_NETDEV; > } else if (!strcmp(type, "internal")) { > @@ -805,19 +803,14 @@ netdev_to_ovs_vport_type(const struct netdev *netdev) > } > > static int > -dpif_netlink_port_add__(struct dpif_netlink *dpif, struct netdev *netdev, > +dpif_netlink_port_add__(struct dpif_netlink *dpif, const char *name, > + enum ovs_vport_type type, > + struct ofpbuf *options, > odp_port_t *port_nop) > OVS_REQ_WRLOCK(dpif->upcall_lock) > { > - const struct netdev_tunnel_config *tnl_cfg; > - char namebuf[NETDEV_VPORT_NAME_BUFSIZE]; > - const char *name = netdev_vport_get_dpif_port(netdev, > - namebuf, sizeof namebuf); > - const char *type = netdev_get_type(netdev); > struct dpif_netlink_vport request, reply; > struct ofpbuf *buf; > - uint64_t options_stub[64 / 8]; > - struct ofpbuf options; > struct nl_sock **socksp = NULL; > uint32_t *upcall_pids; > int error = 0; > @@ -832,17 +825,80 @@ dpif_netlink_port_add__(struct dpif_netlink *dpif, > struct netdev *netdev, > dpif_netlink_vport_init(&request); > request.cmd = OVS_VPORT_CMD_NEW; > request.dp_ifindex = dpif->dp_ifindex; > - request.type = netdev_to_ovs_vport_type(netdev); > - if (request.type == OVS_VPORT_TYPE_UNSPEC) { > + request.type = type; > + request.name = name; > + > + request.port_no = *port_nop; > + upcall_pids = vport_socksp_to_pids(socksp, dpif->n_handlers); > + request.n_upcall_pids = socksp ? dpif->n_handlers : 1; > + request.upcall_pids = upcall_pids; > + > + if (options) { > + request.options = options->data; > + request.options_len = options->size; > + } > + > + error = dpif_netlink_vport_transact(&request, &reply, &buf); > + if (!error) { > + *port_nop = reply.port_no; > + } else { > + if (error == EBUSY && *port_nop != ODPP_NONE) { > + VLOG_INFO("%s: requested port %"PRIu32" is in use", > + dpif_name(&dpif->dpif), *port_nop); > + } > + > + vport_del_socksp(dpif, socksp); > + goto exit; > + } > + > + if (socksp) { > + error = vport_add_channels(dpif, *port_nop, socksp); > + if (error) { > + VLOG_INFO("%s: could not add channel for port %s", > + dpif_name(&dpif->dpif), name); > + > + /* Delete the port. */ > + dpif_netlink_vport_init(&request); > + request.cmd = OVS_VPORT_CMD_DEL; > + request.dp_ifindex = dpif->dp_ifindex; > + request.port_no = *port_nop; > + dpif_netlink_vport_transact(&request, NULL, NULL); > + vport_del_socksp(dpif, socksp); > + goto exit; > + } > + } > + free(socksp); > + > +exit: > + ofpbuf_delete(buf); > + free(upcall_pids); > + > + return error; > +} > + > +static int > +dpif_netlink_port_add_compat(struct dpif_netlink *dpif, struct netdev > *netdev, > + odp_port_t *port_nop) > + OVS_REQ_WRLOCK(dpif->upcall_lock) > +{ > + const struct netdev_tunnel_config *tnl_cfg; > + char namebuf[NETDEV_VPORT_NAME_BUFSIZE]; > + const char *name = netdev_vport_get_dpif_port(netdev, > + namebuf, sizeof namebuf); > + const char *type = netdev_get_type(netdev); > + uint64_t options_stub[64 / 8]; > + struct ofpbuf options; > + enum ovs_vport_type ovs_type; > + > + ovs_type = netdev_to_ovs_vport_type(netdev_get_type(netdev)); > + if (ovs_type == OVS_VPORT_TYPE_UNSPEC) { > VLOG_WARN_RL(&error_rl, "%s: cannot create port `%s' because it has " > "unsupported type `%s'", > dpif_name(&dpif->dpif), name, type); > - vport_del_socksp(dpif, socksp); > return EINVAL; > } > - request.name = name; > > - if (request.type == OVS_VPORT_TYPE_NETDEV) { > + if (ovs_type == OVS_VPORT_TYPE_NETDEV) { > #ifdef _WIN32 > /* XXX : Map appropiate Windows handle */ > #else > @@ -851,7 +907,7 @@ dpif_netlink_port_add__(struct dpif_netlink *dpif, struct > netdev *netdev, > } > > #ifdef _WIN32 > - if (request.type == OVS_VPORT_TYPE_INTERNAL) { > + if (ovs_type == OVS_VPORT_TYPE_INTERNAL) { > if (!create_wmi_port(name)){ > VLOG_ERR("Could not create wmi internal port with name:%s", > name); > vport_del_socksp(dpif, socksp); > @@ -879,50 +935,77 @@ dpif_netlink_port_add__(struct dpif_netlink *dpif, > struct netdev *netdev, > } > nl_msg_end_nested(&options, ext_ofs); > } > - request.options = options.data; > - request.options_len = options.size; > + return dpif_netlink_port_add__(dpif, name, ovs_type, &options, > + port_nop); > + } else { > + return dpif_netlink_port_add__(dpif, name, ovs_type, NULL, port_nop); > } > > - request.port_no = *port_nop; > - upcall_pids = vport_socksp_to_pids(socksp, dpif->n_handlers); > - request.n_upcall_pids = socksp ? dpif->n_handlers : 1; > - request.upcall_pids = upcall_pids; > +} > > - error = dpif_netlink_vport_transact(&request, &reply, &buf); > - if (!error) { > - *port_nop = reply.port_no; > - } else { > - if (error == EBUSY && *port_nop != ODPP_NONE) { > - VLOG_INFO("%s: requested port %"PRIu32" is in use", > - dpif_name(&dpif->dpif), *port_nop); > - } > +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); Could you put this declaration at the top of the file? > > - vport_del_socksp(dpif, socksp); > - goto exit; > +static int > +dpif_netlink_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; > +} > > - if (socksp) { > - error = vport_add_channels(dpif, *port_nop, socksp); > - if (error) { > - VLOG_INFO("%s: could not add channel for port %s", > - dpif_name(&dpif->dpif), name); > - > - /* Delete the port. */ > - dpif_netlink_vport_init(&request); > - request.cmd = OVS_VPORT_CMD_DEL; > - request.dp_ifindex = dpif->dp_ifindex; > - request.port_no = *port_nop; > - dpif_netlink_vport_transact(&request, NULL, NULL); > - vport_del_socksp(dpif, socksp); > - goto exit; > - } > +static int > +dpif_netlink_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; > } > - free(socksp); > + return 0; > +} > > -exit: > - ofpbuf_delete(buf); > - free(upcall_pids); > +static int > +dpif_netlink_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_netlink_port_create(netdev); > + if (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_netlink_port_destroy(name, netdev_get_type(netdev)); > + } > return error; > } > > @@ -934,7 +1017,10 @@ dpif_netlink_port_add(struct dpif *dpif_, struct netdev > *netdev, > int error; > > fat_rwlock_wrlock(&dpif->upcall_lock); > - error = dpif_netlink_port_add__(dpif, netdev, port_nop); > + error = dpif_netlink_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; > @@ -946,6 +1032,12 @@ dpif_netlink_port_del__(struct dpif_netlink *dpif, > odp_port_t port_no) > { > struct dpif_netlink_vport vport; > int error; > + struct dpif_port dpif_port; > + > + error = dpif_netlink_port_query__(dpif, port_no, NULL, &dpif_port); > + if (error) { > + return error; > + } I see that the windows code also does this, perhaps we could drop the call from the windows-only piece? > > dpif_netlink_vport_init(&vport); > vport.cmd = OVS_VPORT_CMD_DEL; > @@ -965,6 +1057,9 @@ dpif_netlink_port_del__(struct dpif_netlink *dpif, > odp_port_t port_no) > > vport_del_channels(dpif, port_no); > > + dpif_netlink_port_destroy(dpif_port.name, dpif_port.type); Should we check the error here? _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
