Hi Alin,
Thanks for the review. I¹ve taken care of the extra whitespace and also
the dead (placeholder) code.
>> --- a/datapath-windows/ovsext/Vport.c
>
>> +++ b/datapath-windows/ovsext/Vport.c
>
>> POVS_VPORT_ENTRY
>
>> +OvsFindTunnelVportByDstPortAndNWProto(POVS_SWITCH_CONTEXT
>
>> switchContext,
>
>> + UINT16 dstPort,
>
>> + UINT8 nwProto) {
>
>> + POVS_VPORT_ENTRY vport;
>
>> + PLIST_ENTRY head, link;
>
>> + UINT32 hash = OvsJhashBytes((const VOID *)&dstPort,
>>sizeof(dstPort),
>
>> + OVS_HASH_BASIS);
>
>> + head = &(switchContext->tunnelVportsArray[hash &
>
>> OVS_VPORT_MASK]);
>
>> + LIST_FORALL(head, link) {
>
>> + vport = CONTAINING_RECORD(link, OVS_VPORT_ENTRY,
>
>> tunnelVportLink);
>
>> + if (GetPortFromPriv(vport) == dstPort) {
>
>> + switch (nwProto) {
>
>[Alin Gabriel Serdean: ] This will break in case we have a GRE tunnel set
>up. They rely only on the IP protocol; their destination port will be
>always set to zero. We can have packets which have l4 port zero and a gre
>tunnel which will result in a misinterpreted patcket. Please leave the
>function OvsFindTunnelVportByPortType the way it was and create a new one.
[Nithin]: There¹s no notion of a L4 port in a GRE packet, hence we use Œ0¹
as a convenient placeholder L4 port. What is the concern here? I don¹t
mind leaving the function OvsFindTunnelVportByPortType() alone, but I am
not sure how the patch is wrong. I¹ll add OvsFindTunnelVportByPortType()
back.
As such, I don¹t see any functionality breakage here.
>>
>
>> + /*
>
>> + * We don't allow two tunnel ports on identical N/W
>>protocol and
>
>> + * L4 port number. This is applicable even if the two
>>ports are of
>
>> + * different tunneling types.
>
>> + */
>
>> + dupVport =
>
>> +
>>OvsFindTunnelVportByDstPortAndNWProto(gOvsSwitchContext,
>
>> +
>>transportPortDest,
>
>> + nwProto);
>
>[Alin Gabriel Serdean: ] Please skip this check in the case of GRE( it
>relies only on the IP protocol).
[Nithin]: The code is simpler this way. No? I can add a comment to clarify
and an ASSERT as well. BTW, we already make assumptions in the code w.r.t
the L4 port number for GRE ports. Pls. have a look at the following code:
NDIS_STATUS
InitOvsVportCommon(POVS_SWITCH_CONTEXT switchContext,
POVS_VPORT_ENTRY vport)
{
UINT32 hash;
switch(vport->ovsType) {
case OVS_VPORT_TYPE_GRE:
case OVS_VPORT_TYPE_VXLAN:
case OVS_VPORT_TYPE_STT:
{
UINT16 dstPort = GetPortFromPriv(vport); <<<<< L4 port # is 0 for GRE
port.
hash = OvsJhashBytes(&dstPort,
sizeof(dstPort),
OVS_HASH_BASIS);
InsertHeadList(
&gOvsSwitchContext->tunnelVportsArray[hash & OVS_VPORT_MASK],
&vport->tunnelVportLink);
switchContext->numNonHvVports++;
break;
}
I¹ll send out a v3. Pls. have a look.
Thanks,
-- Nithin
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
