| Message ID | AM2PR07MB1042308185EDDA5E7049BA018AE20@AM2PR07MB1042.eurprd07.prod.outlook.com |
|---|---|
| State | Changes Requested |
| Headers | show |
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 <zoltan.balogh@ericsson.com> 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.
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c index 5c3cebd..573e4a9 100644 --- a/lib/dpif-netlink.c +++ b/lib/dpif-netlink.c @@ -2917,6 +2917,34 @@ dpif_netlink_flow_from_ofpbuf(struct dpif_netlink_flow *flow, return 0; } +static inline void +nl_msg_put_exclude_packet_type(struct ofpbuf* buf, uint16_t type, + const struct nlattr *data, + uint16_t data_len) +{ + const struct nlattr *packet_type = nl_attr_find__(data, data_len, + OVS_KEY_ATTR_PACKET_TYPE); + if (packet_type) { + /* exclude PACKET_TYPE Netlink attribute. */ + ovs_assert(NLA_ALIGN(packet_type->nla_len) == NLA_HDRLEN + sizeof(uint32_t)); + + size_t packet_type_len = NLA_HDRLEN + sizeof(uint32_t); + size_t first_chunk_size = + (size_t)((uint8_t *)packet_type - (uint8_t *)data); + size_t second_chunk_size = data_len - first_chunk_size + - packet_type_len; + uint8_t *first_attr = NULL; + struct nlattr *next_attr = nl_attr_next(packet_type); + + first_attr = nl_msg_put_unspec_uninit(buf, type, + data_len - packet_type_len); + memcpy(first_attr, data, first_chunk_size); + memcpy(first_attr + first_chunk_size, next_attr, second_chunk_size); + } else { + nl_msg_put_unspec(buf, type, data, data_len); + } +} + /* Appends to 'buf' (which must initially be empty) a "struct ovs_header" * followed by Netlink attributes corresponding to 'flow'. */ static void @@ -2943,13 +2971,12 @@ dpif_netlink_flow_to_ofpbuf(const struct dpif_netlink_flow *flow, } if (!flow->ufid_terse || !flow->ufid_present) { if (flow->key_len) { - nl_msg_put_unspec(buf, OVS_FLOW_ATTR_KEY, - flow->key, flow->key_len); + nl_msg_put_exclude_packet_type(buf, OVS_FLOW_ATTR_KEY, flow->key, + flow->key_len); } - if (flow->mask_len) { - nl_msg_put_unspec(buf, OVS_FLOW_ATTR_MASK, - flow->mask, flow->mask_len); + nl_msg_put_exclude_packet_type(buf, OVS_FLOW_ATTR_MASK, flow->mask, + flow->mask_len); } if (flow->actions || flow->actions_len) { nl_msg_put_unspec(buf, OVS_FLOW_ATTR_ACTIONS,
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 <zoltan.balogh@ericsson.com> --- lib/dpif-netlink.c | 37 ++++++++++++++++++++++++++++++++----- 1 file changed, 32 insertions(+), 5 deletions(-)