[ovs-dev] [PATCH 06/10] ofproto-dpif: New function ofproto_receive().

[Ethan Jackson]

Sounds reasonable, I'll get something together.

Ethan

On Fri, Dec 21, 2012 at 3:56 PM, Jesse Gross <jesse at nicira.com> wrote:

> 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.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20121221/eaf9cc86/attachment-0003.html>