On Mon, Jun 20, 2016 at 08:54:12PM -0700, Wenyu Zhang wrote: > In virtual network, users want more info about the virtual point to observe > the traffic. > It should be a string to provide clear info, not a simple interger ID. > > Introduce "other-config: virtual_obs_id" in IPFIX, which is a string > configured by user. > Introduce an enterprise IPFIX entity "virtual_obs_id"(898) to export the > value. The entity is a > variable-length string. > > Signed-off-by: Wenyu Zhang <[email protected]>
Thanks for the patch. Please add an item to NEWS. Please document the enterprise entity that this adds, in the same way as the other enterprise entities are documented in vswitch.xml. The documentation should mention that this feature is new in OVS 2.6. I think that the documentation that this adds is somewhat deceptive, because it implies that omitting the virtual_obs_id produces the same results as using an empty string. I believe that this is incorrect: if virtual_obs_id is omitted, then the virtual observation ID is not emitted at all, whereas if it is an empty string then it is emitted as an empty string. The documentation doesn't say what a "virtual observation ID" is. This is not a standard term, so users can't be expected to know. Please explain. In the end, I think that this is just an extra string that accompanies the flow records, so it may not be worthwhile to invent a special name for it like "virtual observation ID". I don't see a need for struct ipfix_data_record_virtual_obs_id to be marked as OVS_PACKED. CodingStyle.md explains how to format ?: and how to break expressions across lines, but this patch does not follow this style. In ipfix_put_data_set(), it is wasteful to use dp_packet_put_zeros() then immediately copy over all of the zeros. In ipfix_put_data_set(), this cast is not needed: + data_virtual_obs_id->virtual_obs_len = (uint8_t)virtual_obs_len; Thanks, Ben. _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
