On Tue, Jan 09, 2018 at 08:11:15AM +0800, Ben Pfaff wrote: > 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.
I want to keep include/openvswitch/nsh.h consistent as possible as nsh.h in kernel, it is not good to depend on lib/unaligned.h because it is only used in lib/*, I have fixed sparse warnings. > > 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. Your concerns make sense, I use assignments for them. v8 has been posted, please review new ones. Thanks a lot. > > Thanks, > > Ben. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
