[ovs-dev] [PATCH 2/2] vswitchd: Log all tunnel parameters of given flow.

[Ben Pfaff]

On Tue, Nov 20, 2012 at 10:15:52AM -0800, Pravin B Shelar wrote:
> Signed-off-by: Pravin B Shelar <pshelar at nicira.com>

I don't see a good reason to make flow_tun_flag_to_string() a "static
inline" function in a header file.  It's not performance-critical, I
hope.

In flow_tunnel_format, passing '.' to format_flags() as a delimiter
character seems odd to me.  How about '|'?  Anyone familiar with C or
Python or Java (or ...) would immediately understand.

For "tun_flags", I would use MF_FIELD_SIZES(u16) instead of
MF_FIELD_SIZES(be16).  Both have the same effect, but tun_flags is in
host byte order so I think that its implication is more correct.

For "tun_tos" and "tun_ttl", I would use MF_FIELD_SIZES(u8) instead of
literal 1, 8, just for consistency with other entries.

tun_tos specifies MFS_HEXADECIMAL but nw_tos specifies MFS_DECIMAL.
It's probably better to use MFS_DECIMAL in tun_tos for compatibility.

I think that tun_ttl should use MFS_DECIMAL also, because I don't
usually see TTLs written in hex.

I think that CASE_MFF_TUN_ID is a poor name, because it covers more
than just the tun_id.  I would either use CASE_MFF_TUN_FIELDS (or
another name with broader meaning) or write out the individual cases.
I lean toward the latter: although CASE_MFF_REGS is a precedent for
macros of this form, there's the additional reason in that case that
CASE_MFF_REGS expands differently based on #defines.

I don't see any users of CASE_MFF_TUN_ID outside meta-flow.c, so if
you keep it I would also consider moving its definition there.

The MFF_TUN_TTL and MFF_TUN_TOS cases in mf_set_flow_value() are
backwards.  Also in mf_set_wild().

mf_set_wild() needs to set not just the flow values to 0 but also the
wc values, as the other cases do in that function.

mf_set() also needs to set the wc values.

In the comment on MFS_TNL_FLAGS in enum mf_string, I think that saying
"FLOW_TNL_F_*" might be easier to understand than the current form.

Thanks,

Ben.