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

[Ethan Jackson]

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