Sounds reasonable, I'll get something together. Ethan
On Fri, Dec 21, 2012 at 3:56 PM, Jesse Gross <[email protected]> wrote: > On Fri, Dec 21, 2012 at 6:42 PM, Ethan Jackson <[email protected]> wrote: > > > >> I'm not sure that OFPP_NONE is the right value to return here, at > >> least not without additional checks. Before both > >> handle_miss_upcalls() and handle_sflow_upcalls() explicitly checked > >> for not being able to find an ofport before and would abort. However, > >> now I think they will silently continue with a different meaning. At > >> the very least, we've lost some logging of this situation in > >> handle_miss_upcalls() > > > > > > Actually, I think the existing code has the correct behavior, it's just > > written badly and therefore confusing. Both handle_miss_upcalls() and > > handle_sflow_upcalls() pass in 'ofproto' to ofproto_receive(). > Therefore, > > ofproto_receive() will return ODP_FIT_ERROR when it attempts to populate > > 'ofproto' if there is no corresponding ofport. That said, this is > terribly > > confusing, and should be refactored. > > OK, I see that now. > > > Ideally, I would just return ODP_FIT_ERROR whenever odp_port_to_ofport() > > fails. However, trace relies on being able to set no, or a non-existent > > ofport and proceeding with an in_port of OFPP_NONE. That's why I > > implemented it this way. I'm wondering if we should just remove that > > feature, and require the in_port to trace actually exist? I don't have a > > sense of how useful it is to be able to specify OFPP_NONE as an in_port > in > > this case. The unit tests utilize it fairly extensively at any rate. > > OpenFlow allows sending packets like this (and it's actually a > relatively common behavior in some applications) so it's probably > important to allow trace to handle them. > > I think the main problem is that it is just done implicitly. > handle_miss_upcall() used to log this situation, which probably isn't > a bad idea and if we enabled that to happen then it might have the > effect of making things more obvious in general. >
_______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
