[ovs-dev] [PATCH 06/10] ofproto-dpif: New function ofproto_receive().
Jesse Gross
jesse at nicira.com
Fri Dec 21 23:56:20 UTC 2012
On Fri, Dec 21, 2012 at 6:42 PM, Ethan Jackson <ethan at nicira.com> 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.
More information about the dev
mailing list