Hi Ben, I have refactored the function "dpif_port_del" and removed the new function "dpif_port_remove" added in the v1 patch. Removed the braces around the if compare.
Sending v2 patch now. Thanks for the review. -Ashish On Fri, Nov 3, 2017 at 2:26 PM, Ben Pfaff <b...@ovn.org> wrote: > On Fri, Nov 03, 2017 at 07:36:41AM -0700, Ashish Varma wrote: > > a crash is seen in "netdev_ports_remove" when an interface is deleted > and added > > back in the system and when the interface is part of a bridge > configuration. > > e.g. steps: > > create a tap0 interface using "ip tuntap add.." > > add the tap0 interface to br0 using "ovs-vsctl add-port.." > > delete the tap0 interface from system using "ip tuntap del.." > > add the tap0 interface back in system using "ip tuntap add.." > > (this changes the ifindex of the interface) > > delete tap0 from br0 using "ovs-vsctl del-port.." > > > > In the function "netdev_ports_insert", two hmap entries were created for > > mapping "portnum -> netdev" and "ifindex -> portnum". > > When the interface is deleted from the system, the "netdev_ports_remove" > > function is not getting called and the old ifindex entry is not getting > > cleaned up from the "ifindex_to_port" hmap. > > > > As part of the fix, added function "dpif_port_remove" which will call > > "netdev_ports_remove" in the path where the interface deletion from the > system > > is detected. > > Also, in "netdev_ports_remove", added the code where the > "ifindex_to_port_data" > > (ifindex -> portnum map node) is getting freed when the ifindex is not > > available any more. (as the interface is already deleted.) > > > > VMware-BZ: #1975788 > > Signed-off-by: Ashish Varma <ashishvarma....@gmail.com> > > Thanks for the patch. It's good to fix a bug and especially to fix a > crash. I'm still not entirely certain about the actual need for this > hmap, but I guess that this fixes a real problem. > > This introduces new code in netdev_ports_remove() to handle the new > case. The new case duplicates some code that I believe should be > possible to avoid duplicating. Can you try to refactor slightly to > avoid the code duplication? > > I see that this writes an expression like: (a == b) && (c == d). > Usually, in Open vSwitch, we don't add the extra parentheses unless they > are needed or clear up some genuine confusion, so that we would instead > write a == b && c == d. > > Thanks, > > Ben. > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev