> On Aug 29, 2016, at 2:07 PM, Ben Pfaff <b...@ovn.org> wrote:
> 
> On Mon, Aug 22, 2016 at 04:31:28PM -0700, Jarno Rajahalme wrote:
>> OVS implementation of buffering packets that are sent to the
>> controller is not compliant with the OpenFlow specifications after
>> OpenFlow 1.0, which is possibly true since OpenFlow 1.0 is not really
>> specifying the packet buffering behavior.
>> 
>> OVS implementation executes the buffered packet against the actions of
>> the modified or added rule, whereas OpenFlow (since 1.1) specifies
>> that the packet should be matched against the flow table 0 and
>> processed accordingly.
>> 
>> Rather than fix this behavior, and potentially break OVS users, we
>> propose to remove the feature altogether. After all, such packet
>> buffering is an optional OpenFlow feature, and as such any possible
>> users should continue to work without this feature.
>> 
>> This patch also makes OVS check the received 'buffer_id' values more
>> rigorously, and fixes some internal users accordingly.
>> 
>> Signed-off-by: Jarno Rajahalme <ja...@ovn.org>
> 
> I believe that this came out of a bug report.  Will you credit the bug
> reporter?
> 

I noticed this when checking back to OpenFlow specs when developing the split 
late/execute for buffered packets and packet outs, so there is no bug report. 
I'll state “Found by inspection” in the commit message.

> I think that we will probably start getting a question about "what
> happened to the buffers?" so it might be worthwhile to add a question
> and answer to the FAQ about that.
> 

Done.

> The change to lib/learn.c seems unnecessary.
> 

Right, it was a combination of two changes made at different times cancelling 
out each other.

> I'd be inclined to also remove the buffer_id parameters from the
> functions that ofputil_encode_packet_in_private() calls.  They're always
> going to be UINT32_MAX now.
> 

I should leave the NXPINT_BUFFER_ID definition be even though we now never 
encode it, however?

> In ofputil_encode_packet_in_private(), here:
>    if (check_buffer_id && fm->buffer_id != UINT32_MAX) {
>        error = OFPERR_OFPBRC_BUFFER_UNKNOWN;
>    }
> I would add "!error &&" to the condition, because I think that this
> error is less important than the others.
> 

This was in ofproto.c:ofproto_flow_mod_init(), but I changed this like you 
suggested.

> The changes to ofctrl.c should not be necessary, because the
> encode_flow_mod() function that all of the cases use already sets
> buffer_id to UINT32_MAX.
> 

Right, reverted.

> Acked-by: Ben Pfaff <b...@ovn.org <mailto:b...@ovn.org>>

Thanks for the thorough review! Pushed to master with these revisions.

  Jarno

_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Reply via email to