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

Ben Pfaff blp at nicira.com
Thu Jan 10 19:45:06 UTC 2013


Continuing my review...

I just noticed that struct ipfix_data_record_common and other protocol
data structures lack the size assertions that we customarily include
to make sure that we haven't screwed up.  For example:
    BUILD_ASSERT_DECL(sizeof(struct ipfix_data_record_common) == 26);
(Hmm, that one really does need to be packed.  Maybe they all do,
then, if IPFIX doesn't specify any padding between structures.)

On Tue, Dec 18, 2012 at 11:52:12AM -0800, Romain Lenglet wrote:
> +    /* Common Ethernet entities. */
> +    data_common = ofpbuf_put_zeros(msg, sizeof *data_common);
> +    data_common->observation_point_id = htonl(obs_point_id);
> +        /* OBSERVATION_POINT_ID */
> +    data_common->packet_delta_count = htobe64(packet_delta_count);
> +        /* PACKET_DELTA_COUNT */
> +    memcpy(data_common->source_mac_address, flow->dl_src,
> +           sizeof flow->dl_src);  /* SOURCE_MAC_ADDRESS */
> +    memcpy(data_common->destination_mac_address, flow->dl_dst,
> +           sizeof flow->dl_dst);  /* DESTINATION_MAC_ADDRESS */
> +    data_common->ethernet_type = flow->dl_type;  /* ETHERNET_TYPE */

Above, and elsewhere in ipfix_set_data_msg(), the comments make the
code a little harder to read.  The same comments are also on the
struct members, so I wonder whether it's necessary to have them in the
code?

> +    if (l2 == IPFIX_PROTO_L2_VLAN) {
> +        uint16_t vlan_tci = ntohs(flow->vlan_tci);
> +        data_vlan = ofpbuf_put_zeros(msg, sizeof *data_vlan);
> +        data_vlan->vlan_id = htons(vlan_tci & 0x0fff);  /* VLAN_ID */
> +        data_vlan->dot1q_vlan_id = htons(vlan_tci & 0x0fff); /* DOT1Q_VLAN_ID */
> +        data_vlan->dot1q_priority = (vlan_tci & 0xe000) >> 13;
> +            /* DOT1Q_PRIORITY */
> +    }

I would tend to use the existing OVS helpers for this job, as:

        uint16_t vid = vlan_tci_to_vid(flow->vlan_tci);
        int pcp = vlan_tci_to_pcp(flow->vlan_tci);

        data_vlan = ofpbuf_put_zeros(msg, sizeof *data_vlan);
        data_vlan->vlan_id = htons(vid);
        data_vlan->dot1q_vlan_id = htons(vid);
        data_vlan->dot1q_priority = pcp;

That's the end of my comments.  Thank you!



More information about the dev mailing list