On Tue, Dec 18, 2012 at 11:52:12AM -0800, Romain Lenglet wrote:
> Send one IPFIX data record message for every packet sampled by a
> sample_ipfix action, and send IPFIX template set messages
> periodically. Automatically generate IPFIX entity definitions from
> the IANA specs.
>
> Signed-off-by: Romain Lenglet <rleng...@vmware.com>
I see some GCC warnings:
../ofproto/ofproto-dpif-ipfix.c: In function ‘ipfix_define_template_entity’:
../ofproto/ofproto-dpif-ipfix.c:290: error: ‘element_id’ may be used
uninitialized in this function
../ofproto/ofproto-dpif-ipfix.c:291: error: ‘field_length’ may be used
uninitialized in this function
Many of the developers build with --enable-Werror, so it is a good
idea to get rid of warnings even if they are incorrect.
The mandatory use of Python at build time is a difficulty, because the
XenServer DDK environment in which we often build OVS does not have a
Python install. We work around this in other cases in one of two
ways: by distributing the results of a Python preprocessing step
(because we build in XenServer DDKs from "make dist" output), or by
using Perl (which is always available at build time) instead of
Python.
ipfix-gen-entities is only used at build time, that is, we do not
install it, so I do not think it is worthwhile to generate it from a
.in file. (For Python programs, that is mostly to get the right
Python binary, which isn't necessary at build time since we just run
it explicitly. The @VERSION@ substitution isn't very useful at build
time either, since the build tool is never installed.)
> --- a/ofproto/automake.mk
> +++ b/ofproto/automake.mk
> @@ -33,4 +33,19 @@ ofproto_libofproto_a_SOURCES = \
> ofproto/pinsched.c \
> ofproto/pinsched.h
>
> +nodist_ofproto_libofproto_a_SOURCES = \
> + ofproto/ipfix-entities.def
> +
> +BUILT_SOURCES += \
> + ofproto/ipfix-entities.def
> +
> +CLEANFILES += \
> + ofproto/ipfix-entities.def
Each of the above += assignments would fit on a single line without
the \, so I would just write them that way.
> diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
> index 9b3bfc3..76c2de1 100644
> --- a/ofproto/ofproto-dpif-ipfix.c
> +++ b/ofproto/ofproto-dpif-ipfix.c
> @@ -17,8 +17,11 @@
> #include <config.h>
> #include "ofproto-dpif-ipfix.h"
> #include "collectors.h"
> +#include "flow.h"
> +#include "ofpbuf.h"
> #include "ofproto.h"
> #include "sset.h"
> +#include "timeval.h"
> #include "vlog.h"
>
> VLOG_DEFINE_THIS_MODULE(ipfix);
> @@ -31,13 +34,127 @@ static struct vlog_rate_limit rl =
> VLOG_RATE_LIMIT_INIT(1, 5);
> struct dpif_ipfix {
> struct collectors *collectors;
> struct ofproto_ipfix_options *options;
> + struct ofpbuf msg;
> + uint32_t seq_number;
> };
>
> +#define IPFIX_VERSION 0x000a
> +
> +/* When using UDP, IPFIX Template Records must be resent regularly.
I prefer to hyphenate that word, as "re-sent", because to "resent"
something can be impolite.
> + * By default, the interval is 10 minutes. Here we use instead an
> + * interval in terms of messages. Since we send many messages (one
> + * per sampled packet) 10000 seems reasonable and should be lower than
> + * 10 minutes. Cf. IETF RFC 5101 Section 10.3.6. */
> +#define IPFIX_TEMPLATE_INTERVAL 10000
It would be easy to keep track of the time when a template record was
most recently sent, and then send one if it was 10 minutes ago. I
don't know whether this is important.
> +/* Cf. IETF RFC 5101 Section 3.1. */
> +struct ipfix_header {
> + uint16_t version; /* IPFIX_VERSION. */
> + uint16_t length; /* Length in bytes including this header. */
> + uint32_t export_time; /* Seconds since the epoch. */
> + uint32_t seq_number; /* Message sequence number. */
> + uint32_t obs_domain_id; /* Observation Domain ID. */
> +} __attribute__((packed));
On many RISC architectures, access to "packed" data structures is more
expensive than access to unpacked data structures. So, in OVS, we try
to use "packed" only if a data structure's natural packing would
differ from the packed format, or if we expect the data structure to
be accessed when it is misaligned. The former doesn't look true here.
I didn't read the code carefully enough to figure out the latter, but
it is rarely true. So I would be inclined to omit the "packed"
attribute here.
I believe that this data structure should be in network byte order.
We use the ovs_be<N> types (e.g. ovs_be16) to indicate that. The
"sparse" tool will actually check for proper byteswapping when you do
that, if you build with "C=1" on the "make" command line. (I get
several "sparse" warnings when I fix the GCC warnings above, since it
sees, e.g., assignments of ovs_be16 results from htons() to uint16_t
values.)
The above comments apply to other structures in this file, too.
ipfix_init_header() only uses the tv_sec part of the timespec returned
by time_wall_timespec(), so it could just use time_wall() directly.
It looks like, in practice, the arguments to
ipfix_define_template_entity() are always fixed at compile time. If
I'm reading the code correctly, then this means that each call to
ipfix_define_template_entity() is going through a "switch" statement
to find out a couple of values whose values could actually be fixed at
compile time. So, how about something like this, instead:
- at top level, add:
enum ipfix_entity_size {
#define IPFIX_ENTITY(ENUM, ID, SIZE, NAME) IPFIX_ENTITY_SIZE_##ENUM,
#include "ofproto/ipfix-entities.def"
};
- Change ipfix_define_template_entity() to just:
static void
ipfix_define_template_entity(enum ipfix_entity_id id, int size,
struct ofpbuf *msg)
{
struct ipfix_template_field_specifier *field;
field = ofpbuf_put_zeros(msg, sizeof *field);
field->element_id = htons(id);
field->field_length = htons(size);
}
- Change DEF to just:
#define DEF(ID) \
{ \
ipfix_define_template_entity(IPFIX_ENTITY_ID_##ID, \
IPFIX_ENTITY_SIZE_##ID, msg); \
count++; \
}
and thereby avoid runtime work for what we know at compile time.
> + /* TODO: Extract additional headers from the packet when not in
> + * struct flow:
> + * ETHERNET_HEADER_LENGTH
> + * ETHERNET_PAYLOAD_LENGTH
> + * ETHERNET_TOTAL_LENGTH */
The above would be pretty easy to extract, if they are useful.
> + /* TODO: Extract additional headers from the packet when not
> + * in struct flow:
> + * IP_HEADER_LENGTH
> + * TOTAL_LENGTH_IPV4
> + * IP_TOTAL_LENGTH
> + * PAYLOAD_LENGTH_IPV6 */
As would these, I think, with the possible exception of IPv6 (for
which nothing is ever easy.)
> +static void
> +ipfix_send_data_msg(struct dpif_ipfix *di, const struct flow *flow,
> + uint16_t probability, uint32_t obs_point_id)
> +{
> + struct ofpbuf *msg;
> + size_t set_hdr_offset;
> + struct ipfix_set_header *set_hdr;
> + uint64_t packet_delta_count;
> + enum ipfix_proto_l2 l2;
> + enum ipfix_proto_l3 l3;
> + enum ipfix_proto_l4 l4;
> + struct ipfix_data_record_common *data_common;
> + struct ipfix_data_record_vlan *data_vlan;
> + struct ipfix_data_record_ip *data_ip;
> + struct ipfix_data_record_ipv4 *data_ipv4;
> + struct ipfix_data_record_ipv6 *data_ipv6;
> + struct ipfix_data_record_tcpudp *data_tcpudp;
> +
> + msg = &di->msg;
> +
> + ipfix_init_header(di->seq_number, di->options->obs_domain_id, msg);
> + di->seq_number++;
> + set_hdr_offset = msg->size;
> +
> + /* Choose the right template ID matching the protocols in the
> + * sampled packet. */
> + l2 = (flow->vlan_tci == 0) ? IPFIX_PROTO_L2_ETH : IPFIX_PROTO_L2_VLAN;
> +
> + switch(ntohs(flow->dl_type)) {
> + case 0x0800:
Please use ETH_TYPE_IP instead of writing 0x0800 here.
> + l3 = IPFIX_PROTO_L3_IPV4;
> + break;
> + case 0x86DD:
Please use ETH_TYPE_IPV6 instead of writing 0x86dd here.
> + l3 = IPFIX_PROTO_L3_IPV6;
> + break;
> + default:
> + l3 = IPFIX_PROTO_L3_UNKNOWN;
> + l4 = IPFIX_PROTO_L4_UNKNOWN;
> + }
> +
> + if (l3 != IPFIX_PROTO_L3_UNKNOWN) {
> + switch(flow->nw_proto) {
> + case 6: /* TCP */
> + case 17: /* UDP */
Please write IPPROTO_TCP and IPPROTO_UDP instead of 6 and 17,
respectively.
> + l4 = IPFIX_PROTO_L4_TCP_UDP;
> + break;
> + default:
> + l4 = IPFIX_PROTO_L4_UNKNOWN;
> + }
> + }
I'd feel more comfortable with a single assignment
l4 = IPFIX_PROTO_L4_UNKNOWN;
just above the "if (l3 != IPFIX_PROTO_L3_UNKNOWN)", rather than
two copies in other places.
I have to read the rest of the code, but I thought I'd send this out
for an initial review.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
