Hi Dumitru, Thanks for the review and sorry for the late response.
> Nit: The "Signed-off-by" is normally the last tag of the commit message. Fixed. I also dropped the Fixes: tag > We're not correctly handling the "union" of the multiple sets of > advertised routes here, or am I reading this wrong? > [...] > Do we need to "continue" here instead of "return"? You're right, good catch, re_nl_add_route() uses NLM_F_EXCL, so this would have hit EEXIST. "continue" only covers half of it though. It helps when the route is already in the VRF, but if it isn't, handle_route_msg() never runs for that prefix and we still call re_nl_add_route() twice from the add loop. So v2 deduplicates while building the union instead: only the occurrence from the first table that has a given route ends up in routes_to_advertise. route.c already does the same within a single datapath. That also keeps the "return" valid - handle_route_msg() walks the same vector in the same order, so its first match is always the entry that survived deduplication. > route_table_watches can now contain duplicates. Is that an issue? I don't think so. advertised_routes holds one entry per table_id and HMAP_FOR_EACH_POP visits each once, so there's exactly one push per table. That part didn't change, the patch only removed the can_sync guard, which could previously skip a push. Let me know if I'm missing sth. > Maybe we should enhance the test here [...] > Should we also check what happens when we remove one router? Done in v2, both routers advertise 203.0.113.0/24 and the test checks it's only installed once, then lr2 is removed (lr1's routes including the shared prefix stay), then lr1, waiting for the VRF device to go away. v2 sent separately. Thanks, Jakob _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
