Hi Ben,
I add new comments inline . May you take a look?
On 6/24/16, 10:54 AM, "Wenyu Zhang" <wen...@vmware.com> wrote:
>Thanks for your inputs.
>
>I added some comments inline, may you help take a look?
>
>On 6/24/16, 2:18 AM, "Ben Pfaff" <b...@ovn.org> 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 <wen...@vmware.com>
>>
>>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.
Wenyu: The existing behaviour in the patch is not to emit the empty string
when
The virtual_obs_id is not set.
I will modify the document to make it clear.
>>
>>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 ?
Wenyu: I use "virutal_obs_id" in ³other_config", due to the configured
value
Will be emitted with this enterprise entity. We need a key to
set the string, not must be ³virutal_obs_id² but I didn¹t find a
proper one.
Do you mean that we should use a more general key, such as
³extra-string²?
>>
>>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 data is
>necessary.
> I may put the byte into msg directly, instead of putting zeros and
>overwrite it.
>>
>>Thanks,
>>
>>Ben.
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
