Yes. Exactly. Neil
------ Neil McKee InMon Corp. http://www.inmon.com On Wed, Sep 19, 2018 at 12:10 PM, Justin Pettit <[email protected]> wrote: > Thanks for the report. The existing code seems to use "ip" in a way that's > inconsistent, which I think makes it confusing. I think the approach is > good, but I wonder about calling that variable "target", since don't we want > the source address, not the target address? > > How about the diff at the bottom of this message? > > --Justin > > > -=-=-=-=-=-=-=-=-=- > > diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c > index d17d7a8be83c..62a09b5d1b1e 100644 > --- a/ofproto/ofproto-dpif-sflow.c > +++ b/ofproto/ofproto-dpif-sflow.c > @@ -472,11 +472,12 @@ sflow_choose_agent_address(const char *agent_device, > /* sFlow only supports target in default routing table with > * packet mark zero. > */ > - ip = ss_get_address(&ss); > + struct in6_addr target_ip = ss_get_address(&ss); > > struct in6_addr gw, src = in6addr_any; > char name[IFNAMSIZ]; > - if (ovs_router_lookup(0, &ip, name, &src, &gw)) { > + if (ovs_router_lookup(0, &target_ip, name, &src, &gw)) { > + ip = src; > goto success; > } > } > > >> On Sep 19, 2018, at 10:16 AM, Neil McKee <[email protected]> wrote: >> >> It looks like the intent here was to get the local source-address >> associated with the route to the sFlow target-address: >> >> https://github.com/openvswitch/ovs/blob/master/ofproto/ofproto-dpif-sflow.c#L475-L481 >> >> That's a good idea, but the answer supplied by ovs_router_lookup() via >> &src is ignored, so this code just sets the sFlow agent-address to >> the sFlow target-address... with a high potential for turmoil :) >> >> The suggested fix would be to introduce a new local var "struct >> in6_addr target", offer &target to ovs_router_lookup(), and if the >> lookup works then set ip = target before goto success. That way you >> are never setting "ip" to a toxic value. >> >> Sorry didn't notice this at the time. >> >> The workaround is just to make sure that the agent-address is set in >> the config and not left to automatic selection, which is probably >> what most deployments do anyway. (e.g. running hsflowd with the ovs{} >> module enabled will take care of it). >> >> Neil >> >> ------ >> Neil McKee >> InMon Corp. >> http://www.inmon.com >> _______________________________________________ >> dev mailing list >> [email protected] >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
