On Tue, Nov 20, 2012 at 10:15:52AM -0800, Pravin B Shelar wrote: > Signed-off-by: Pravin B Shelar <[email protected]>
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. _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
