| Message ID | 20170118194515.1307-3-e@erig.me |
|---|---|
| State | Superseded |
| Headers | show |
On 18 January 2017 at 11:45, Eric Garver <e@erig.me> wrote: > From: Thadeu Lima de Souza Cascardo <cascardo@redhat.com> > > 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 <cascardo@redhat.com> 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 <joe@ovn.org> > --- > 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?
On Thu, Feb 02, 2017 at 02:39:30PM -0800, Joe Stringer wrote: > On 18 January 2017 at 11:45, Eric Garver <e@erig.me> wrote: > > From: Thadeu Lima de Souza Cascardo <cascardo@redhat.com> > > > > 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 <cascardo@redhat.com> > > 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 <joe@ovn.org> > > > --- > > 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
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); - 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; + } 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); + dpif_port_destroy(&dpif_port); + return error; }