On Wed, Jul 15, 2015 at 5:41 PM, 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.
Actually, I realized that with the current version of the patch these flags are still accepted and it's not until the next patch that we clamp down on non-public values. Therefore, I restored the test to its original form to make this a pure refactoring patch. The net result is still the same after the next patch though. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
