On Tue, Aug 8, 2017 at 5:10 PM, Joe Stringer <j...@ovn.org> wrote:
> Previously, netdev_ports_insert() would allocate and insert an
> ifindex->odp_port mapping, but netdev_ports_remove() would never remove
> the mapping or free the mapping structure. This patch fixes these up.
>
> Fixes: 32b77c316d9982("dpif: Save added ports in a port map.")
> Reported-by: Andy Zhou <az...@ovn.org>
> Signed-off-by: Joe Stringer <j...@ovn.org>
A few minor comments inline. Otherwise,
Acked-by: Andy Zhou <az...@ovn.org>
> ---
> v3: First post.
> ---
> lib/netdev.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/lib/netdev.c b/lib/netdev.c
> index 7e9896b82928..a8f5348264c8 100644
> --- a/lib/netdev.c
> +++ b/lib/netdev.c
> @@ -2266,9 +2266,20 @@ netdev_ports_remove(odp_port_t port_no, const struct
> dpif_class *dpif_class)
> data = netdev_ports_lookup(port_no, dpif_class);
>
> if (data) {
> + int ifindex = netdev_get_ifindex(data->netdev);
I think it will be safer to check the return value of ifindex.
> + struct ifindex_to_port_data *ifidx;
> +
> + HMAP_FOR_EACH_WITH_HASH (ifidx, node, ifindex, &ifindex_to_port) {
> + if (ifidx->port == port_no) {
> + break;
> + }
> + }
Here we expect ifidx to be a valid pointer. I think the code can be
more readable and safer to
add an assertion.
> +
> dpif_port_destroy(&data->dpif_port);
> netdev_close(data->netdev); /* unref and possibly close */
> + hmap_remove(&ifindex_to_port, &ifidx->node);
> hmap_remove(&port_to_netdev, &data->node);
> + free(ifidx);
> free(data);
> ret = 0;
> }
> --
> 2.13.3
>
> _______________________________________________
> 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
