On Sat, Jan 06, 2018 at 01:47:52PM +0800, Yi Yang wrote: > IETF NSH draft added a new filed ttl in NSH header, this patch > is to add new nsh key 'ttl' for it. > > Signed-off-by: Yi Yang <yi.y.y...@intel.com>
Thanks for v7! The field assignments in meta-flow.h seem wrong to me: - The TTL field is new in v2.9, so it shouldn't say v2.8. - The existing fields should not be renumbered because that breaks OpenFlow wire format compatibility with anything that already knows how to talk to OVS 2.8. Please keep the existing numbering. Why does nsh_16aligned_be32 exist? Please use get_16aligned_be32, which is identical. In meta-flow.xml, please properly document the NSH fields, following the pattern set by the other documentation in the file. I see several uses of memcpy() for copying a struct. Please use an assignment to copy structs. This statement looks suspicious, since the target and the "sizeof" are different: memset(nsh, 0, sizeof(nsh->context)); I'm concerned about how this patch introduces different structs with identical layouts and then uses memcpy() to copy between them. This is a trap for unsuspecting developers who change one structure or the other (even by reordering fields). It would probably be better to figure out a way to either use the same struct in each case, or to do member-by-member copies. Another way would be to use assertions to make sure that the structures really are identical. Thanks, Ben. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev