On Thu, Feb 02, 2017 at 02:39:30PM -0800, Joe Stringer wrote: > 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.
Understood. I'll have a go at spitting this patch. > 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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
