On Fri, May 12, 2017 at 11:07:42AM +0000, Zoltán Balogh wrote:
> The kernel datapath does not support the packet_type match field.
> Instead it encodes the packet type implictly by the presence or absence of
> the Ethernet attribute in the flow key and mask.
>
> This patch filters the PACKET_TYPE attribute out of netlink flow key and
> mask to be sent to the kernel datapath.
>
> Signed-off-by: Zoltan Balogh <[email protected]>
Thank you for v6!
checkpatch has a few complaints:
ERROR: Inappropriate spacing in pointer declaration
#10 FILE: b/lib/dpif-netlink.c:2921:
nl_msg_put_exclude_packet_type(struct ofpbuf* buf, uint16_t type,
WARNING: Line length is >79-characters long
#15 FILE: b/lib/dpif-netlink.c:2926:
OVS_KEY_ATTR_PACKET_TYPE);
WARNING: Line length is >79-characters long
#18 FILE: b/lib/dpif-netlink.c:2929:
ovs_assert(NLA_ALIGN(packet_type->nla_len) == NLA_HDRLEN +
sizeof(uint32_t));
The commit message is clear, but it's less obvious in the code what's
going on. I suggest that some version of the commit message explanation
be put at the top of nl_msg_put_exclude_packet_type()
The name nl_msg_put_exclude_packet_type() makes it sound like the
function should be in netlink.c, not in dpif-netlink.c. I suggest
adjusting the name to avoid the nl_msg_ prefix.
nl_msg_put_exclude_packet_type() uses the expression NLA_HDRLEN +
sizeof(uint32_t) a couple of times. I suggest NL_A_U32_SIZE instead.
In:
+ size_t first_chunk_size =
+ (size_t)((uint8_t *)packet_type - (uint8_t *)data);
the cast is unneeded, because an initializer implicitly converts its
expression to the type of the initialized object.
nl_msg_put_exclude_packet_type() should not be declared "inline",
because coding-style.rst says:
Functions in ``.c`` files should not normally be marked ``inline``,
because it does not usually help code generation and it does
suppress compilers warnings about unused functions. (Functions
defined in .h usually should be marked inline.)
It looks like this patch fixes a bug that was introduced by the first
patch in the series. If that's true, then this patch should be folded
into patch 1, so that "git bisect" later does not land in a buggy commit
accidentally.
Thanks,
Ben.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
