> 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