Thanks for the review Ben!
New patch coming up, some non-essential commentary below.
Jarno
On Nov 25, 2013, at 3:24 PM, Ben Pfaff <b...@nicira.com> wrote:
> On Tue, Nov 19, 2013 at 04:43:47PM -0800, Jarno Rajahalme wrote:
>> Allow TCP flags match specification with symbolic flag names. TCP
>> flags are optionally specified as a string of flag names, each
>> preceded by '+' when the flag must be one, or '-' when the flag must
>> be zero. Any flags not explicitly included are wildcarded. The
>> existing hex syntax is still allowed, and is used in flow dumps when
>> all the flags are matched.
>>
>> Signed-off-by: Jarno Rajahalme <jrajaha...@nicira.com>
>
> Both now and previously, match_format() would omit tcp_flags if it was
> masked off (mask==0), but the new path to that is more indirect and
> less obvious. The old way might have been better.
>
Most of the existing format_*_masked() functions work in the less obvious way.
I changed the new format_flags_masked() to require the caller to filter out
empty mask to be more direct.
> In match_format(), CONSTANT_HTONS(~0) might be written more obviously
> as OVS_BE16_MAX. Ditto in mf_from_tcp_flags_string().
>
Thanks, did not remember these constants existed!
> In mf_from_tcp_flags_string(), this:
> if (ovs_scan(s, "%lli/%lli", &flags, &mask)) {
> might better check that the whole string is parsed:
> if (ovs_scan(s, "%lli/%lli%n", &flags, &mask, &n) && !s[n]) {
> and similarly for the other ovs_scan() call.
>
> Also in ovs_scan() calls, I would use "uint16_t"s and "%"SCNi16,
> instead of long long int and "%lli”.
Fixed, thanks!
>
> In mf_from_tcp_flags_string(), the following code:
> name_len = strlen(s);
> delim = strchr(s,'+');
> if (delim && delim - s < name_len) {
> name_len = delim - s;
> }
> delim = strchr(s,'-');
> if (delim && delim - s < name_len) {
> name_len = delim - s;
> }
> may be simplified to:
> name_len = strcspn(s, "+-“);
>
Changed.
> I think I can set an invalid bit by specifying "+[Unknown]".
>
I changed the packet_tcp_flag_to_string() to return NULL for unknown inputs and
format_flags_masked() to test for that. mf_from_tcp_flags_string() already
tests for NULL, so now setting invalid bits is not possible any more.
> In mf_from_tcp_flags_string(), the 'result' and 'flags' local
> variables seem to have the same purpose, as do 'resmask' and 'mask’.
Now that they are of same type, it was easy to get rid of ‘result’ and
‘resmask’.
> The manpage might be improved by an example for the new syntax.
>
Done.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
