Thanks for your inputs. I added some comments inline, may you help take a look?
On 6/24/16, 2:18 AM, "Ben Pfaff" <[email protected]> wrote: >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. Wenyu: I think that it seems no need to emit an empty string when the ovirtual_obs_id is not set. Do you think that emitting an empty string is a better solution? You are right. The document may mislead users. I may change it. > >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". Wenyu: I want to introduce an entity to export the description of observation info in virtual network. Do you have any suggestion about the entity name ? > >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. Wenyu: Thanks for the catch. I will fix it. > >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; Wenyu: Due to the entity is variable-length, the length of it is necessary. I may put the byte into msg directly, instead of putting zeros and overwrite it. > >Thanks, > >Ben. _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
