[ovs-dev] [PATCH 4/4] ofproto: Implement IPFIX exporter

Ben Pfaff blp at nicira.com
Thu Jan 10 00:00:29 UTC 2013


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 <rlenglet at 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.



More information about the dev mailing list