[ovs-dev] [PATCH] lib: More intuitive syntax for TCP flags matching.

[Ben Pfaff]

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 <jrajahalme at 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.

In match_format(), CONSTANT_HTONS(~0) might be written more obviously
as OVS_BE16_MAX.  Ditto in mf_from_tcp_flags_string().

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".

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, "+-");

I think I can set an invalid bit by specifying "+[Unknown]".

In mf_from_tcp_flags_string(), the 'result' and 'flags' local
variables seem to have the same purpose, as do 'resmask' and 'mask'.

The manpage might be improved by an example for the new syntax.

Thanks,

Ben.