On Wed, Jul 15, 2015 at 10:52 PM, Joe Stringer <joestrin...@nicira.com> wrote: > On 15 July 2015 at 17:41, Jesse Gross <je...@nicira.com> wrote: >> On Wed, Jul 15, 2015 at 5:31 PM, Joe Stringer <joestrin...@nicira.com> wrote: >>> On 15 July 2015 at 15:53, Jesse Gross <je...@nicira.com> wrote: >>>> On Wed, Jul 15, 2015 at 11:35 AM, Ben Pfaff <b...@nicira.com> wrote: >>>>> On Mon, Jul 13, 2015 at 02:53:56PM -0700, Jesse Gross wrote: >>>>>> There are several implementations of functions that parse/format >>>>>> flags and their binary representation. This factors them out into >>>>>> common routines. In addition to reducing code, it also makes things >>>>>> more consistent across different parts of OVS. >>>>>> >>>>>> Signed-off-by: Jesse Gross <je...@nicira.com> >>>>> >>>>> Thanks, I like reducing code size! >>>>> >>>>> This patch deletes a test and part of a test in ovs-ofctl.at. Why? >>>> >>>> I'm not sure that these tests really make sense and arguably the >>>> behavior there were checking wasn't even right in the first place. In >>>> theory they are verifying that the correct OpenFlow version is >>>> selected based on the fields provided (in this case the correct >>>> version is "none") but it is being done using names that weren't >>>> supposed to be public, though the old parsing code allowed them to >>>> leak through. The new parsing code is enforcing the same invariants as >>>> before but more carefully and now rejects these commands as a parse >>>> error before it even gets to the OpenFlow layer that is supposed to be >>>> exercised. The next patch makes this field valid anyways and verifies >>>> the correct OpenFlow behavior, so it didn't seem like it made sense to >>>> keep the test around. >>> >>> Hmm, interesting. One of the bugs found by STACK recently also changes >>> the output of this test -- that bug was long-lived, in >>> parse_ofp_str__(). I plan to send the patches soonish. As I >>> understand, these tests are also meant to pick up on fields that >>> aren't supposed to be exposed, and make sure that ovs-ofctl doesn't >>> try to pass them on. That said, if the field will be allowed after the >>> next patch then I don't object. >> >> What was the actual bug that was discovered? >> >> The problem here wasn't so much really that field should haven't been >> exposed - that part was working correctly. The issue was that the >> actual values should have been restricted by the parser. I can change >> it so that the test remains in this patch but only tests tun_flags(0) >> instead of key|csum. The next patch would then delete the test, since >> at that point the field is allowed. > > I CC'd you on the patch: > http://openvswitch.org/pipermail/dev/2015-July/057560.html > > As far as I understand, it's an invalid match which the parser should > have picked up on and returned OFPERR_OFPBAC_MATCH_INCONSISTENT. > Instead it parsed them and just said that no protocol can represent > it.
I actually think the net result of ovs-ofctl is actually more correct now than after your patch (even though the code is clearly wrong). The problem that we're testing for in the flow being added here is that the match can't be represented by any OpenFlow version, which is what we get. The action is actually fine - it's just drop. However, the check for actions comes before the check for matches and once it sees that there are no protocols are available, it concludes there is a problem with the actions. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
