[ovs-dev] [PATCH 1/2] flow: Factor out flag parsing and formatting routines.

[Jesse Gross]

On Wed, Jul 15, 2015 at 5:31 PM, Joe Stringer <joestringer at nicira.com> wrote:
> On 15 July 2015 at 15:53, Jesse Gross <jesse at nicira.com> wrote:
>> On Wed, Jul 15, 2015 at 11:35 AM, Ben Pfaff <blp at 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 <jesse at 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.