[ovs-dev] [PATCH v2] dpif-netlink: convert packet_type netlink attribute
Eric Garver
e at erig.me
Sat Jul 8 18:18:16 UTC 2017
On Fri, Jul 07, 2017 at 02:48:20PM +0000, Zoltán Balogh wrote:
> Hi Eric,
Hi. Please see the bottom.
>
> I agree, my last patch did not help at all. We have reworked it with Jan, removed the unneeded code parts, it's simpler now. This way, removal of the PACKET_TYPE netlink attribute and adding of ETHER_TYPE netlink attribute happens at the same place.
> Could you have a look at it and test it, please?
>
> --->8---
>
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index 562f6134c..4ef8d9965 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -3434,31 +3434,35 @@ dpif_netlink_flow_from_ofpbuf(struct dpif_netlink_flow *flow,
>
>
> /*
> - * If PACKET_TYPE attribute is present in 'data', it filters PACKET_TYPE out,
> - * then puts 'data' to 'buf'.
> + * If PACKET_TYPE attribute is present in 'data', converts it to ETHERNET and
> + * ETHERTYPE attributes, then puts 'data' to 'buf'.
> */
> static void
> put_exclude_packet_type(struct ofpbuf *buf, uint16_t type,
> const struct nlattr *data, uint16_t data_len)
> {
> - const struct nlattr *packet_type;
> -
> - packet_type = nl_attr_find__(data, data_len, OVS_KEY_ATTR_PACKET_TYPE);
> -
> + 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) == NL_A_U32_SIZE);
> size_t packet_type_len = NL_A_U32_SIZE;
> size_t first_chunk_size = (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);
> + struct nlattr *second_chunk = nl_attr_next(packet_type);
> + const struct nlattr *ethernet = nl_attr_find__(data, data_len,
> + OVS_KEY_ATTR_ETHERNET);
> + size_t offset = buf->size;
> +
> + nl_msg_put_unspec(buf, type, data, first_chunk_size);
>
> - 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);
> + if (!ethernet) {
> + ovs_be32 value = nl_attr_get_be32(packet_type);
> + nl_msg_put_be16(buf, OVS_KEY_ATTR_ETHERTYPE, pt_ns_type_be(value));
> + }
> +
> + nl_msg_put(buf, second_chunk, second_chunk_size);
> + nl_msg_end_nested(buf, offset);
> } else {
> nl_msg_put_unspec(buf, type, data, data_len);
> }
> @@ -3489,12 +3493,13 @@ dpif_netlink_flow_to_ofpbuf(const struct dpif_netlink_flow *flow,
> }
> if (!flow->ufid_terse || !flow->ufid_present) {
> if (flow->key_len) {
> - put_exclude_packet_type(buf, OVS_FLOW_ATTR_KEY, flow->key,
> - flow->key_len);
> + put_exclude_packet_type(buf, OVS_FLOW_ATTR_KEY,
> + flow->key, flow->key_len);
> }
> +
> if (flow->mask_len) {
> - put_exclude_packet_type(buf, OVS_FLOW_ATTR_MASK, flow->mask,
> - flow->mask_len);
> + 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,
>
This still did not do the trick. The ETHERTYPE is placed at the same
level of the message as OVS_FLOW_ATTR_MASK, rather than being place
_inside_ OVS_FLOW_ATTR_MASK.
Below is what I came up with. All test cases pass unchanged. It also
works for my series to add kernel L3 VXLAN-GPE/GRE.
https://github.com/erig0/ovs/commits/46aafc3ad12e30772f9d8561e6750e6c6334077c
Let me know how it works for you.
Eric.
--->8---
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index 562f6134c3a5..0654ba1b2131 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -3434,31 +3434,47 @@ dpif_netlink_flow_from_ofpbuf(struct dpif_netlink_flow *flow,
/*
- * If PACKET_TYPE attribute is present in 'data', it filters PACKET_TYPE out,
- * then puts 'data' to 'buf'.
+ * If PACKET_TYPE attribute is present in 'data', it filters PACKET_TYPE out.
+ * If the flow is not Ethernet, packet_type is converted to ETHERTYPE.
+ * Puts 'data' to 'buf'.
*/
static void
put_exclude_packet_type(struct ofpbuf *buf, uint16_t type,
- const struct nlattr *data, uint16_t data_len)
+ const struct nlattr *data, size_t data_len)
{
const struct nlattr *packet_type;
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) == NL_A_U32_SIZE);
- size_t packet_type_len = NL_A_U32_SIZE;
- size_t first_chunk_size = (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);
+ struct odputil_keybuf new_attrs_buf;
+ struct nlattr *new_attrs;
+ const struct nlattr *ethernet;
+ size_t new_attrs_len;
+ ovs_be32 packet_type_value = nl_attr_get_be32(packet_type);
+
+ new_attrs = (struct nlattr *) &new_attrs_buf;
+ memcpy(new_attrs, data, data_len);
+ new_attrs_len = data_len;
+
+ nl_attrs_filter(new_attrs, &new_attrs_len, OVS_KEY_ATTR_PACKET_TYPE);
+
+ /* If it's not Ethernet, then we need to override or add the Ethertype
+ * with the one from the packet_type. */
+ ethernet = nl_attr_find__(data, data_len, OVS_KEY_ATTR_ETHERNET);
+ if (!ethernet) {
+ struct ofpbuf new_buf;
+
+ nl_attrs_filter(new_attrs, &new_attrs_len, OVS_KEY_ATTR_ETHERTYPE);
+
+ ofpbuf_use_stack(&new_buf, new_attrs, sizeof new_attrs_buf);
+ nl_msg_put_uninit(&new_buf, new_attrs_len);
+ nl_msg_put_be16(&new_buf, OVS_KEY_ATTR_ETHERTYPE,
+ pt_ns_type_be(packet_type_value));
+ new_attrs_len = new_buf.size;
+ }
+
+ nl_msg_put_unspec(buf, type, new_attrs, new_attrs_len);
} else {
nl_msg_put_unspec(buf, type, data, data_len);
}
diff --git a/lib/netlink.c b/lib/netlink.c
index 4cf1aaca621c..c9b83fc681c4 100644
--- a/lib/netlink.c
+++ b/lib/netlink.c
@@ -927,3 +927,23 @@ nl_attr_find_nested(const struct nlattr *nla, uint16_t type)
{
return nl_attr_find__(nl_attr_get(nla), nl_attr_get_size(nla), type);
}
+
+/*
+ * Filter nlattr type from set of nlattrs.
+ * This changes the data in place. So caller should make a copy if required.
+ */
+void
+nl_attrs_filter(struct nlattr *attrs, size_t *attrs_len, uint16_t type)
+{
+ size_t size = *attrs_len;
+ struct nlattr *nla;
+ size_t left;
+
+ NL_ATTR_FOR_EACH (nla, left, attrs, size) {
+ if (nl_attr_type(nla) == type) {
+ *attrs_len -= nl_attr_len_pad(nla, left);
+ memmove(nla, nl_attr_next(nla), left - nl_attr_len_pad(nla, left));
+ return;
+ }
+ }
+}
diff --git a/lib/netlink.h b/lib/netlink.h
index 6dfac27c9d4b..744c9520966f 100644
--- a/lib/netlink.h
+++ b/lib/netlink.h
@@ -240,5 +240,6 @@ const struct nlattr *nl_attr_find(const struct ofpbuf *, size_t hdr_len,
const struct nlattr *nl_attr_find_nested(const struct nlattr *, uint16_t type);
const struct nlattr *nl_attr_find__(const struct nlattr *attrs, size_t size,
uint16_t type);
+void nl_attrs_filter(struct nlattr *attrs, size_t *attrs_len, uint16_t type);
#endif /* netlink.h */
More information about the dev
mailing list