On Thu, Feb 14, 2013 at 08:19:29PM +0000, Kyle Mestery (kmestery) wrote: > On Feb 14, 2013, at 12:49 PM, Ben Pfaff <b...@nicira.com> wrote: > > On Thu, Feb 14, 2013 at 06:47:56PM +0000, Kyle Mestery (kmestery) wrote: > >> On Feb 14, 2013, at 12:33 PM, Ben Pfaff <b...@nicira.com> wrote: > >>> On Thu, Feb 14, 2013 at 09:37:30AM -0500, Kyle Mestery wrote: > >>>> Garbage collect tnl_backers during type_run(). Add new > >>>> tnl_backers if a VXLAN ports UDP port changes. > >>>> > >>>> Signed-off-by: Kyle Mestery <kmest...@cisco.com> > >>> > >>> The error handling here is bad. If it fails to add or remove a port, > >>> then ovs_assert() aborts the whole ovs-vswitchd process. > >>> > >> I can change the asserts into something more sane. > >> > >>> But I don't fully understand the code. It looks to me like the first > >>> time we encounter a particular dp_port_name in the inner HMAP_FOR_EACH, > >>> we transfer that dp_port_name from tmp_simap to backer->tnl_backers. > >>> Straightforward enough. However, I believe that the second time we > >>> encounter that same dp_port_name, we will not find it in tmp_simap > >>> (because we deleted it) and will therefore try to add a new port for > >>> it. I believe that will fail, because there is already a port with > >>> that dst_port, and then we'll assert-fail the process. > >>> > >>> Am I reading the code correctly? > >>> > >> Actually, the second time the same dp_port_name is encountered, > >> we first look and see if we need to reconfigure it, and then we check if > >> it's already in backer->tnl_backers, which it will be since we added it > >> there the first time we found it. I don't see it adding the port twice, > >> unless > >> I'm misreading it. > > > > Looking again, you're right, thanks. > > > >>> I've now pushed patch 1-4 to master, waiting for an ack from Jesse on > >>> #5, waiting for your reply on this one. > >>> > >> Thanks Ben! Let me know if you want me to modify the error handling > >> behavior. > > > > Please do, I'm not sure of the correct behavior. > > I sent a revised version with error handling in lieu of ovs_asserts() for > port creation and deletion. For the netdev_get_tunnel_config() assert, > Ethan indicated that should never happen, so I'd rather leave that as an > ovs_assert().
Sure, I'm happy with that. Ethan told me out-of-band he wants to look this over, so we can await his review. Thanks a lot. Ben. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
