Thanks for reviewing it. I am working on address the comments. And I add some
new comments inline.
Bests,
Wenyu
Important
---------
This needs a rebase because openvswitch.h has moved.
There are sparse warnings and errors. I'll append a patch to fix these.
In datapath/flow_netlink.c, ipv4_tun_from_nlattr() is missing a "break" in the
OVS_TUNNEL_KEY_ATTR_TP_DST case.
Minor
-----
The comment on 'out_tun_key' in the definition of struct dpif_upcall is wrong.
The comments in enum ovs_tunnel_key_attr say that OVS_TUNNEL_KEY_ATTR_TP_SRC
and OVS_TUNNEL_KEY_ATTR_TP_DST are u16s but they appear to be be16s instead.
I don't understand the comment for OVS_PACKET_ATTR_OUT_TUNNEL_KEY.
How is the port specified on OVS_USERSPACE_ATTR_TUNNEL_OUT_PORT used?
Wenyu: It is the odp port, which is used to look up the vport in the kernel.
I believe that OVS_USERSPACE_ATTR_TUNNEL_OUT_PORT is a kernel port number.
However, the odp_put_userspace_action() parameter for this value has 'pid' in
the name (and I see a few other related uses of "pid"). A PID is a different
concept.
Wenyu: Yes, the name of the parameter may be confused. I will change it.
NUM_DPIF_IPFIX_TUNNEL is not actually the number of tunnel types since the list
skips 0x04 and 0x06.
Wenyu: 0x04 and 0x06 are reserved for STT related tunnel types, I may set
NUM_DPIF_IPFIX_TUNNEL as 0x08
IPFIX_PROTO_TUNNEL_UNKNOWN and IPFIX_PROTO_TUNNEL_KNOWN seem like odd names
since I think they actually mean "not tunneled" and "tunneled".
Please break long lines, e.g.:
#define IPFIX_ENTERPRISE_ENTITY(ENUM, ID, SIZE, NAME, ENTERPRISE)
IPFIX_ENTITY_ID_##ENUM = IPFIX_SET_ENTERPRISE(ID),
as:
#define IPFIX_ENTERPRISE_ENTITY(ENUM, ID, SIZE, NAME, ENTERPRISE) \
IPFIX_ENTITY_ID_##ENUM = IPFIX_SET_ENTERPRISE(ID),
I don't like adding a new member to flow_tnl just to copy out a few bytes
individually in one place. The diff I appended includes a suggestion.
In dpif_ipfix_add_tunnel_port(), the check for dip == NULL should be removed.
xmalloc() never returns NULL.
Wenyu: You means that xmalloc() never fails? If it may fail, how shall I check
it?
dpif_ipfix_add_tunnel_port() consistently uses strncmp() instead of strcmp().
Why? (Please use strcmp().)
Why is Geneve restricted to a 24-bit key?
Please don't parenthesize the expression on 'return' (see
dpif_ipfix_get_tunnel_port()).
ipfix_send_template_msg() now contains a fair bit of cut-and-paste duplication.
Can you try to reduce that?
In ipfix_cache_entry_init(), please write pointer declarators with a leading
but not a trailing space, e.g.:
struct ipfix_data_record_flow_key_tunnel *data_tunnel;
Just below that, also please omit parentheses when taking the size of an
expression, e.g.:
data_tunnel = ofpbuf_put_zeros(&msg, sizeof *data_tunnel +
tunnel_port->tunnel_key_length);
Please put a space before { in cases like this:
if (dupcall->out_tun_key){
I see this in a few places.
I'm not sure why tnl_xlate_init() has this new code:
/* The tp_src and tp_dst members in flow_tnl are set to be always
* wildcarded, not to unwildcard them here. */
wc->masks.tunnel.tp_src = 0;
wc->masks.tunnel.tp_dst = 0;
Does it have any visible effect? It appears to me that these fields are
already 0, as initialized by the caller.
Wenyu: Yes, the caller has initialized the whole data strusture in the existent
codes. But we think that we should
set the two fileds as 0 obviously in this function, to avoid potential issue
if it is called by other callers in future.
It would be nice to document the VMware enterprise entities in vswitch.xml, or
at least point to documentation.
Here are changes that I suggest folding in. They include "sparse"
fixes, updates to comments and code to fit the OVS style (including a lot of
spelling errors in comments), and a couple of other minor things I noticed.
diff --git a/datapath-windows/ovsext/OvsNetProto.h
b/datapath-windows/ovsext/OvsNetProto.h
index 5e98206..a21ab5c 100644
--- a/datapath-windows/ovsext/OvsNetProto.h
+++ b/datapath-windows/ovsext/OvsNetProto.h
@@ -80,6 +80,7 @@ typedef UINT64 IP6UnitLength;
#define IPPROTO_ICMP 1
#define IPPROTO_IGMP 2
#define IPPROTO_UDP 17
+#define IPPROTO_GRE 47
#define IPPROTO_TCP 6
#define IPPROTO_RSVD 0xff
diff --git a/include/sparse/netinet/in.h b/include/sparse/netinet/in.h index
c63dca7..83ad2cc 100644
--- a/include/sparse/netinet/in.h
+++ b/include/sparse/netinet/in.h
@@ -66,6 +66,7 @@ struct sockaddr_in6 {
#define IPPROTO_UDP 17
#define IPPROTO_ROUTING 43
#define IPPROTO_FRAGMENT 44
+#define IPPROTO_GRE 47
#define IPPROTO_AH 51
#define IPPROTO_ICMPV6 58
#define IPPROTO_NONE 59
diff --git a/lib/odp-util.c b/lib/odp-util.c index 49d3b0f..c64201e 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -523,7 +523,7 @@ parse_odp_userspace_action(const char *s, struct ofpbuf
*actions)
uint32_t pid;
union user_action_cookie cookie;
struct ofpbuf buf;
- uint32_t tunnel_out_port;
+ odp_port_t tunnel_out_port;
int n = -1;
void *user_data = NULL;
size_t user_data_size = 0;
@@ -593,7 +593,7 @@ parse_odp_userspace_action(const char *s, struct ofpbuf
*actions)
&output, &n1) ) {
n += n1;
cookie.type = USER_ACTION_COOKIE_IPFIX;
- cookie.ipfix.output_odp_port = output;
+ cookie.ipfix.output_odp_port = u32_to_odp(output);
user_data = &cookie;
user_data_size = sizeof cookie.ipfix;
} else if (ovs_scan(&s[n], ",userdata(%n", @@ -3550,7 +3550,7 @@
odp_key_fitness_to_string(enum odp_key_fitness fitness) size_t
odp_put_userspace_action(uint32_t pid,
const void *userdata, size_t userdata_size,
- const uint32_t *tunnel_out_pid,
+ const odp_port_t *tunnel_out_port,
struct ofpbuf *odp_actions) {
size_t userdata_ofs;
@@ -3577,8 +3577,9 @@ odp_put_userspace_action(uint32_t pid,
} else {
userdata_ofs = 0;
}
- if (tunnel_out_pid) {
- nl_msg_put_u32(odp_actions, OVS_USERSPACE_ATTR_TUNNEL_OUT_PORT,
*tunnel_out_pid);
+ if (tunnel_out_port) {
+ nl_msg_put_odp_port(odp_actions, OVS_USERSPACE_ATTR_TUNNEL_OUT_PORT,
+ *tunnel_out_port);
}
nl_msg_end_nested(odp_actions, offset);
diff --git a/lib/odp-util.h b/lib/odp-util.h index 04f657c..f511091 100644
--- a/lib/odp-util.h
+++ b/lib/odp-util.h
@@ -230,14 +230,14 @@ union user_action_cookie {
struct {
uint16_t type; /* USER_ACTION_COOKIE_IPFIX. */
- uint32_t output_odp_port; /* The output odp port on datapath to get
tunnel info. */
+ odp_port_t output_odp_port; /* The output odp port on datapath
+ to get tunnel info. */
} ipfix;
};
BUILD_ASSERT_DECL(sizeof(union user_action_cookie) == 16);
size_t odp_put_userspace_action(uint32_t pid,
const void *userdata, size_t userdata_size,
- const uint32_t *tunnel_out_pid,
+ const odp_port_t *tunnel_out_port,
struct ofpbuf *odp_actions); void
odp_put_tunnel_action(const struct flow_tnl *tunnel,
struct ofpbuf *odp_actions); diff --git
a/lib/packets.h b/lib/packets.h index e54b287..0258745 100644
--- a/lib/packets.h
+++ b/lib/packets.h
@@ -33,10 +33,7 @@ struct ds;
/* Tunnel information used in flow key and metadata. */ struct flow_tnl {
- union {
- ovs_be64 tun_id;
- uint8_t tun_id_octarray[8];
- };
+ ovs_be64 tun_id;
ovs_be32 ip_src;
ovs_be32 ip_dst;
uint16_t flags;
@@ -68,7 +65,7 @@ struct pkt_metadata {
};
#define PKT_METADATA_INITIALIZER(PORT) \
- (struct pkt_metadata){ 0, 0, { {0}, 0, 0, 0, 0, 0, 0, 0}, 0, 0, {(PORT)} }
+ (struct pkt_metadata){ .in_port.odp_port = PORT }
bool dpid_from_string(const char *s, uint64_t *dpidp);
diff --git a/ofproto/ipfix-enterprise-entities.def
b/ofproto/ipfix-enterprise-entities.def
index 013cee2..ea94586 100644
--- a/ofproto/ipfix-enterprise-entities.def
+++ b/ofproto/ipfix-enterprise-entities.def
@@ -3,9 +3,7 @@
#define IPFIX_ENTERPRISE_ENTITY(ENUM, ID, SIZE, NAME, ENTERPRISE) #endif
-#ifndef IPFIX_ENTERPRISE_VMWARE
#define IPFIX_ENTERPRISE_VMWARE 6876
-#endif
IPFIX_ENTERPRISE_ENTITY(TUNNEL_TYPE, 891, 1, tunnelType,
IPFIX_ENTERPRISE_VMWARE) IPFIX_ENTERPRISE_ENTITY(TUNNEL_KEY, 892, 0,
tunnelKey, IPFIX_ENTERPRISE_VMWARE) @@ -15,5 +13,4 @@
IPFIX_ENTERPRISE_ENTITY(TUNNEL_PROTOCOL_IDENTIFIER, 895, 1,
tunnelProtocolIdenti IPFIX_ENTERPRISE_ENTITY(TUNNEL_SOURCE_TRANSPORT_PORT,
896, 2, tunnelSourceTransportPort, IPFIX_ENTERPRISE_VMWARE)
IPFIX_ENTERPRISE_ENTITY(TUNNEL_DESTINATION_TRANSPORT_PORT, 897, 2,
tunnelDestinationTransportPort, IPFIX_ENTERPRISE_VMWARE)
-#undef IPFIX_ENTERPRISE_VMWARE
#undef IPFIX_ENTERPRISE_ENTITY
diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c index
e742fba..6071629 100644
--- a/ofproto/ofproto-dpif-ipfix.c
+++ b/ofproto/ofproto-dpif-ipfix.c
@@ -45,12 +45,12 @@ static struct ovs_mutex mutex = OVS_MUTEX_INITIALIZER;
/* The standard layer2SegmentId (ID 351) element is included in vDS to send
* the VxLAN tunnel's VNI. It is 64-bit long, the most significant byte is
* used to indicate the type of tunnel (0x01 = VxLAN, 0x02 = GRE) and the three
- * lowest significant bytes hold the value of the layer 2 overlay network
+ * least significant bytes hold the value of the layer 2 overlay
+ network
* segment identifier: a 24-bit VxLAN tunnel's VNI or a 24-bit GRE tunnel's
- * TNI. This is not compatible with GRE-64, as implementd in OVS, as its
+ * TNI. This is not compatible with GRE-64, as implemented in OVS, as
+ its
* tunnel IDs are 64-bit.
*
- * Two new enterprise information elements are defined which are simlar to
+ * Two new enterprise information elements are defined which are
+ similar to
* laryerSegmentId but support 64-bit IDs:
* tunnelType (ID 891) and tunnelKey (ID 892).
*
@@ -374,7 +374,7 @@ BUILD_ASSERT_DECL(sizeof(struct
ipfix_data_record_aggregated_ip) == 32);
(sizeof(struct ipfix_set_header) \
+ MAX_DATA_RECORD_LEN)
-/* Max length of an IPFIX message. Arbitrarily set to accomodate low
+/* Max length of an IPFIX message. Arbitrarily set to accommodate low
* MTU. */
#define MAX_MESSAGE_LEN 1024
@@ -1027,7 +1027,7 @@ ipfix_define_template_entity(enum ipfix_entity_id id,
field->field_length = htons(size);
} else {
/* RFC 5101, Section 7. Variable-Length Information Element */
- field->field_length = 0xffff;
+ field->field_length = OVS_BE16_MAX;
}
if (enterprise) {
field->enterprise = htonl(enterprise); @@ -1099,7 +1099,7 @@
ipfix_define_template_fields(enum ipfix_proto_l2 l2, enum ipfix_proto_l3 l3,
}
}
- if (tunnel != IPFIX_PROTO_TUNNEL_UNKNOWN){
+ if (tunnel != IPFIX_PROTO_TUNNEL_UNKNOWN) {
DEF(TUNNEL_SOURCE_IPV4_ADDRESS);
DEF(TUNNEL_DESTINATION_IPV4_ADDRESS);
DEF(TUNNEL_PROTOCOL_IDENTIFIER); @@ -1165,10 +1165,10 @@
ipfix_send_template_msgs(struct dpif_ipfix_exporter *exporter,
continue;
}
for (tunnel = 0; tunnel < NUM_IPFIX_PROTO_TUNNEL; tunnel++) {
- /* When the size of the tmplate packet reaches
+ /* When the size of the template packet reaches
* MAX_MESSAGE_LEN(1024), send it out.
- * And then reinit the msg to construct a new packet for
- * the following templates.
+ * And then reinitialize the message to construct a new
+ * packet for the following templates.
*/
if (ofpbuf_size(&msg) >= MAX_MESSAGE_LEN) {
/* Send template message. */ @@ -1179,7 +1179,7 @@
ipfix_send_template_msgs(struct dpif_ipfix_exporter *exporter,
ipfix_send_msg(exporter->collectors, &msg);
ofpbuf_uninit(&msg);
- /* Reinit the msg buf. */
+ /* Reinitialize the message buffer. */
ofpbuf_use_stub(&msg, msg_stub, sizeof msg_stub);
ipfix_init_header(export_time_sec,
exporter->seq_number, @@ -1491,16 +1491,17 @@ ipfix_cache_entry_init(struct
ipfix_flow_cache_entry *entry,
if (tunnel == IPFIX_PROTO_TUNNEL_KNOWN) {
struct ipfix_data_record_flow_key_tunnel * data_tunnel;
+ const uint8_t *tun_id;
data_tunnel = ofpbuf_put_zeros(&msg, sizeof(*data_tunnel) +
tunnel_port->tunnel_key_length);
data_tunnel->tunnel_source_ipv4_address = tunnel_key->ip_src;
data_tunnel->tunnel_destination_ipv4_address = tunnel_key->ip_dst;
/* The tunnel_protocol_identifier is from tunnel_proto array, which
- * contians protocol_identifiers of each tunnel type.
+ * contains protocol_identifiers of each tunnel type.
* For the tunnel type on the top of IPSec, which uses the protocol
* identifier of the upper tunnel type is used, the tcp_src and tcp_dst
- * are decided based on the protocol identifires.
+ * are decided based on the protocol identifiers.
* E.g:
* The protocol identifier of DPIF_IPFIX_TUNNEL_IPSEC_GRE is
IPPROTO_GRE,
* and both tp_src and tp_dst are zero.
@@ -1511,9 +1512,10 @@ ipfix_cache_entry_init(struct ipfix_flow_cache_entry
*entry,
data_tunnel->tunnel_destination_transport_port = tunnel_key->tp_dst;
data_tunnel->tunnel_type = tunnel_port->tunnel_type;
data_tunnel->tunnel_key_length = tunnel_port->tunnel_key_length;
- /* tun_id is in network order, and tunnel key is in low bits*/
+ /* tun_id is in network order, and tunnel key is in low bits. */
+ tun_id = (const uint8_t *) &tunnel_key->tun_id;
memcpy(data_tunnel->tunnel_key,
- &tunnel_key->tun_id_octarray[8 -
tunnel_port->tunnel_key_length],
+ &tun_id[8 - tunnel_port->tunnel_key_length],
tunnel_port->tunnel_key_length);
}
@@ -1684,7 +1686,7 @@ dpif_ipfix_bridge_sample(struct dpif_ipfix *di, struct
ofpbuf *packet,
tunnel_port = dpif_ipfix_find_port(di, input_odp_port);
}
if (output_odp_port != ODPP_NONE && output_tunnel_key) {
- /* Output tunnel, output_tunnel_key must valid. */
+ /* Output tunnel, output_tunnel_key must be valid. */
tunnel_key = output_tunnel_key;
tunnel_port = dpif_ipfix_find_port(di, output_odp_port);
}
diff --git a/ofproto/ofproto-dpif-ipfix.h b/ofproto/ofproto-dpif-ipfix.h index
0222054..1d293a5 100644
--- a/ofproto/ofproto-dpif-ipfix.h
+++ b/ofproto/ofproto-dpif-ipfix.h
@@ -48,7 +48,7 @@ void dpif_ipfix_set_options(
void dpif_ipfix_bridge_sample(struct dpif_ipfix *, struct ofpbuf *,
const struct flow *,
- uint32_t, uint32_t, const struct flow_tnl *);
+ odp_port_t, odp_port_t, const struct
+ flow_tnl *);
void dpif_ipfix_flow_sample(struct dpif_ipfix *, struct ofpbuf *,
const struct flow *, uint32_t, uint16_t, uint32_t,
uint32_t);
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 6edda74..3ee9fe9 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -906,7 +906,7 @@ convert_upcall(struct udpif *udpif, struct upcall *upcall)
memcpy(&cookie, nl_attr_get(dupcall->userdata),
sizeof cookie.ipfix);
- if (dupcall->out_tun_key){
+ if (dupcall->out_tun_key) {
memset(&output_tunnel_key, 0, sizeof output_tunnel_key);
odp_tun_key_from_attr(dupcall->out_tun_key,
&output_tunnel_key); diff --git
a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index
ded98fe..18e3538 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2154,7 +2154,7 @@ compose_sample_action(const struct xbridge *xbridge,
const uint32_t probability,
const union user_action_cookie *cookie,
const size_t cookie_size,
- const uint32_t *tunnel_out_port)
+ const odp_port_t *tunnel_out_port)
{
size_t sample_offset, actions_offset;
odp_port_t odp_port;
@@ -2261,25 +2261,26 @@ compose_ipfix_action(const struct xbridge *xbridge, {
uint32_t probability;
union user_action_cookie cookie;
- uint32_t *tunnel_out_port = NULL;
+ odp_port_t *tunnel_out_port = NULL;
if (!xbridge->ipfix || flow->in_port.ofp_port == OFPP_NONE) {
return;
}
- /* For input case, output_odp_port is ODPP_NONE, which is an invalid port
number. */
+ /* For input case, output_odp_port is ODPP_NONE, which is an invalid port
+ * number. */
if (output_odp_port == ODPP_NONE &&
!dpif_ipfix_get_bridge_exporter_input_sampling(xbridge->ipfix)) {
return;
}
- /* For output case, output_pid is valid*/
+ /* For output case, output_odp_port is valid. */
if (output_odp_port != ODPP_NONE) {
if (!dpif_ipfix_get_bridge_exporter_output_sampling(xbridge->ipfix)) {
return;
}
- /* If enable-tunnel-sampling is enabled, need put an additional option
- * attribute: OVS_USERSPACE_ATTR_TUNNEL_OUT_PORT
+ /* If tunnel sampling is enabled, put an additional option attribute:
+ * OVS_USERSPACE_ATTR_TUNNEL_OUT_PORT.
*/
if (dpif_ipfix_get_bridge_exporter_tunnel_sampling(xbridge->ipfix) &&
dpif_ipfix_get_tunnel_port(xbridge->ipfix, output_odp_port) ) {
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
