> 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.
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.
Ethan
> The behavior in ofproto_unixctl_trace() is the same as before but I'm
> not sure that was correct.
>
> > @@ -7193,13 +7186,10 @@ ofproto_unixctl_trace(struct unixctl_conn *conn,
> int argc, const char *argv[],
> > goto exit;
> > }
> >
> > - fitness = odp_flow_key_to_flow(odp_key.data, odp_key.size,
> &flow);
> > - flow.in_port = odp_port_to_ofp_port(ofproto, flow.in_port);
> > -
> > - /* Convert odp_key to flow. */
> > - error = ofproto_dpif_vsp_adjust(ofproto, fitness, &flow,
> > - &initial_tci, NULL);
> > - if (error == ODP_FIT_ERROR) {
> > + fitness = ofproto_receive(ofproto->backer, NULL,
> odp_key.data,
> > + odp_key.size, &flow, NULL, NULL,
> > + &initial_tci);
> > + if (fitness == ODP_FIT_ERROR) {
> > unixctl_command_reply_error(conn, "Invalid flow");
> > goto exit;
> > }
>
> The one remaining direct user of vsp_adjust_flow() is now in the half
> of ofproto trace that handles OpenFlow style flows. This seems stuck
> between two possible interpretations of the input port:
> * If it's an odp port then it could require vlan splinters
> translation but it would also require single datapath port lookup.
> * If it's an ofp port then it shouldn't require any translation.
>
> I think #2 is the correct interpretation (especially since that's what
> you'll get out of our logs). There shouldn't be any middle ground now
> that we're doing all translation in one place, which is good.
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
