On Jan 9, 2013, at 4:00 PM, Ben Pfaff <b...@nicira.com> wrote:
> 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.)
I agree. I've moved it into ofproto_libofproto_a_SOURCES and added a comment,
and removed it from AC_CONFIG_FILES.
[...]
>> + * 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.
You're right. It was trivial, so I did that, and updated the comment.
>> +/* 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.
As you commented in your other review, IPFIX allows for some entities to be
unaligned, of variable length or 1-byte long, and makes padding optional, so
I'd feel safer to keep the packed attribute even if it may not be strictly
necessary right now.
[...]
> 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.
Done.
> 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.
How would that work?
Nothing uses the SIZE argument of the IPFIX_ENTITY in your code above, and
you're using the enum value in place of the size, which seems incorrect.
>
>> + /* 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.)
Right. I added code to calculate those based on the l2, l3, l4 fields of ofpbuf.
[...]
> I have to read the rest of the code, but I thought I'd send this out
> for an initial review
Thanks for your comments!
I addressed all, and will send updated patches after reviewing your last email.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
