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

[Ben Pfaff]

On Fri, Jan 11, 2013 at 09:55:44AM -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 standard IPFIX entity
> definitions from the IANA specs.
> 
> Signed-off-by: Romain Lenglet <rlenglet at vmware.com>

I'm still getting "sparse" warnings on this version.  Here's an
incremental that fixes them.  Will you fold it in?

diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
index 1e68708..0985171 100644
--- a/ofproto/ofproto-dpif-ipfix.c
+++ b/ofproto/ofproto-dpif-ipfix.c
@@ -18,6 +18,7 @@
 
 #include <config.h>
 #include "ofproto-dpif-ipfix.h"
+#include "byte-order.h"
 #include "collectors.h"
 #include "flow.h"
 #include "ofpbuf.h"
@@ -52,11 +53,11 @@ struct dpif_ipfix {
 
 /* 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. */
+    ovs_be16 version;  /* IPFIX_VERSION. */
+    ovs_be16 length;  /* Length in bytes including this header. */
+    ovs_be32 export_time;  /* Seconds since the epoch. */
+    ovs_be32 seq_number;  /* Message sequence number. */
+    ovs_be32 obs_domain_id;  /* Observation Domain ID. */
 } __attribute__((packed));
 BUILD_ASSERT_DECL(sizeof(struct ipfix_header) == 16);
 
@@ -498,8 +499,8 @@ ipfix_send_data_msg(struct dpif_ipfix *di, struct ofpbuf *packet,
     /* Common Ethernet entities. */
     data_common = ofpbuf_put_zeros(msg, sizeof *data_common);
     data_common->observation_point_id = htonl(obs_point_id);
-    data_common->packet_delta_count = htobe64(packet_delta_count);
-    data_common->layer2_octet_delta_count = htobe64(layer2_octet_delta_count);
+    data_common->packet_delta_count = htonll(packet_delta_count);
+    data_common->layer2_octet_delta_count = htonll(layer2_octet_delta_count);
     memcpy(data_common->source_mac_address, flow->dl_src,
            sizeof flow->dl_src);
     memcpy(data_common->destination_mac_address, flow->dl_dst,

I'm also getting a build failure due to:
    The distribution is missing the following files:
    ofproto/ipfix-gen-entities
so I think you need to use EXTRA_DIST (or add a new
dist_noinst_SCRIPTS) instead of noinst_SCRIPTS.

This is going to require some rethinking in a bit, because we are
moving from a model where the in_port uniquely identifies the ofproto
associated with an action to one where it doesn't: because patch ports
are going to be implemented in userspace, an action might be
associated with an ofproto other than the one where the packet
arrived.  So we need to get a bit more information from the kernel to
userspace somehow.

In handle_ipfix_upcall():
+    memcpy(&cookie, &upcall->userdata, sizeof(cookie));
we generally write "sizeof" without parentheses when we can, e.g.:
+    memcpy(&cookie, &upcall->userdata, sizeof cookie);

Otherwise this seems reasonable, thank you.