> 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. >
Eh, not that extensively, only 4 tests. I'll just change trace unless there are any objections. Ethan > 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