Re: [ovs-dev] [PATCHv3 1/2] netdev: Free ifidx mapping in netdev_ports_remove().

2017-08-09 Thread Joe Stringer 
On 8 August 2017 at 18:16, Andy Zhou  wrote:
> On Tue, Aug 8, 2017 at 5:10 PM, Joe Stringer  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 
>> Signed-off-by: Joe Stringer 
>
> A few minor comments inline. Otherwise,
>
> Acked-by: Andy Zhou 

Hi Andy,

Thanks for the review. Your comments are good points, and they change
the patch enough that I figured I should just send a v2. I'll follow
up shortly.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCHv3 1/2] netdev: Free ifidx mapping in netdev_ports_remove().

2017-08-08 Thread Andy Zhou 
On Tue, Aug 8, 2017 at 5:10 PM, Joe Stringer  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 
> Signed-off-by: Joe Stringer 

A few minor comments inline. Otherwise,

Acked-by: Andy Zhou 

> ---
> 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

[ovs-dev] [PATCHv3 1/2] netdev: Free ifidx mapping in netdev_ports_remove().

2017-08-08 Thread Joe Stringer 
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 
Signed-off-by: Joe Stringer 
---
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);
+struct ifindex_to_port_data *ifidx;
+
+HMAP_FOR_EACH_WITH_HASH (ifidx, node, ifindex, &ifindex_to_port) {
+if (ifidx->port == port_no) {
+break;
+}
+}
+
 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