On Wed, Jul 15, 2015 at 03:53:27PM -0700, Jesse Gross 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.
OK, thanks, I appreciate the explanation. > > format_flags_masked() will output the empty string if flags == 0 && mask > > == 0, and I think that parse_flags() will accept that too, but I don't > > know if it's the most user-friendly form. Don't know if that matters. > > I agree, it's clearer and less ambiguous to output something in all > cases. I added this: Thanks! > > format_flags_masked() will output "0" if flags == 0 && mask == max_mask, > > but parse_flags() won't accept "0". It would be nice to resolve that. > > I'm not sure if I understand correctly - parse_flags() should accept > both numbers as well as flag names either with or without masks. At > the top it scans for masked numeric values (which won't match in this > case) but in the second half of the function after it has looked for > masked flags, it tries again to look for both unmasked numbers and > names. I believe the unmasked numbers check should handle this > correctly. I misread the code, sorry. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
