[ovs-dev] [PATCH v7 2/4] nsh: add new flow key 'ttl'

[Ben Pfaff]

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