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!
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
