Please disregard this review I it was intended for the V2 (https://patchwork.ozlabs.org/patch/629912/)
Thanks, Alin. > -----Mesaj original----- > De la: dev [mailto:dev-boun...@openvswitch.org] În numele Alin Serdean > Trimis: Tuesday, June 14, 2016 7:01 AM > Către: Nithin Raju <nit...@vmware.com>; dev@openvswitch.org > Subiect: Re: [ovs-dev] [PATCH] datapath-windows: use ip proto for tunnel > port lookup > > Hi Nithin, > > Thanks for the patch. Beside a few small nits regarding whitespace and extra > comments, please see considerations to GRE tunnels in inlined comments. > > Alin. > > - if (tunnelVport) { > > + break; > > + case OVS_VPORT_TYPE_VXLAN: > > ovsActionStats.rxVxlan++; > > + break; > > +#if 0 > > + case OVS_VPORT_TYPE_GENEVE: > > + ovsActionStats.rxGeneve++; > > + break; > > +#endif > [Alin Gabriel Serdean: ] I think you can fan out the ifdef and Yin can add > them > later > > + case OVS_VPORT_TYPE_GRE: > > + ovsActionStats.rxGre++; > > + break; > > } > > - break; > > } > > } > > > > diff --git a/datapath-windows/ovsext/Tunnel.c b/datapath- > > windows/ovsext/Tunnel.c index 97d2020..c5aae1a 100644 > > --- a/datapath-windows/ovsext/Tunnel.c > > +++ b/datapath-windows/ovsext/Tunnel.c > > @@ -285,9 +285,9 @@ > OvsInjectPacketThroughActions(PNET_BUFFER_LIST > > pNbl, > > > > SendFlags |= NDIS_SEND_FLAGS_DISPATCH_LEVEL; > > > > - vport = OvsFindTunnelVportByDstPort(gOvsSwitchContext, > > - htons(tunnelKey.dst_port), > > - OVS_VPORT_TYPE_VXLAN); > > + vport = OvsFindTunnelVportByDstPortAndType(gOvsSwitchContext, > > + > > + htons(tunnelKey.dst_port), > > + > > + OVS_VPORT_TYPE_VXLAN); > > > > if (vport == NULL){ > > status = STATUS_UNSUCCESSFUL; diff --git a/datapath- > > windows/ovsext/Vport.c b/datapath-windows/ovsext/Vport.c index > > 222b2c1..f5eeaa5 100644 > > --- a/datapath-windows/ovsext/Vport.c > > +++ b/datapath-windows/ovsext/Vport.c > > @@ -691,9 +691,9 @@ OvsFindVportByPortNo(POVS_SWITCH_CONTEXT > > switchContext, > > > > > > POVS_VPORT_ENTRY > > -OvsFindTunnelVportByDstPort(POVS_SWITCH_CONTEXT switchContext, > > - UINT16 dstPort, > > - OVS_VPORT_TYPE ovsPortType) > > +OvsFindTunnelVportByDstPortAndType(POVS_SWITCH_CONTEXT > > switchContext, > > + UINT16 dstPort, > > + OVS_VPORT_TYPE ovsPortType) > > { > > POVS_VPORT_ENTRY vport; > > PLIST_ENTRY head, link; > > @@ -711,6 +711,41 @@ > > OvsFindTunnelVportByDstPort(POVS_SWITCH_CONTEXT switchContext, } > > > > 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. > > + case IPPROTO_UDP: > > + if (/* nwProto != OVS_VPORT_TYPE_GENEVE || */ > [Alin Gabriel Serdean: ] I think you can fan out the comment and Yin can add > them later > > + vport->ovsType != OVS_VPORT_TYPE_VXLAN) { > > + continue; > > + } > > + break; > > + case IPPROTO_TCP: > > + if (vport->ovsType != OVS_VPORT_TYPE_STT) { > > + continue; > > + } > > + break; > > + case IPPROTO_GRE: > > + default: > > + break; > > + } > > + return vport; > > + } > > + } > > + return NULL; > > +} > > + > > +POVS_VPORT_ENTRY > > OvsFindTunnelVportByPortType(POVS_SWITCH_CONTEXT switchContext, > > OVS_VPORT_TYPE ovsPortType) { @@ > > -2222,15 +2257,20 @@ > OvsNewVportCmdHandler(POVS_USER_PARAMS_CONTEXT > > usrParamsCtx, > > > > if (OvsIsTunnelVportType(portType)) { > > UINT16 transportPortDest = 0; > > + UINT8 nwProto; > > + POVS_VPORT_ENTRY dupVport; > > > > switch (portType) { > > case OVS_VPORT_TYPE_GRE: > > + nwProto = IPPROTO_GRE; > > break; > > case OVS_VPORT_TYPE_VXLAN: > > transportPortDest = VXLAN_UDP_PORT; > > + nwProto = IPPROTO_UDP; > > break; > > case OVS_VPORT_TYPE_STT: > > transportPortDest = STT_TCP_PORT; > > + nwProto = IPPROTO_TCP; > > break; > > default: > > nlError = NL_ERROR_INVAL; @@ -2245,6 +2285,22 @@ > > OvsNewVportCmdHandler(POVS_USER_PARAMS_CONTEXT > usrParamsCtx, > > } > > } > > > > + /* > > + * 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). > > + if (dupVport) { > > + OVS_LOG_ERROR("Vport for N/W proto and port already > > exists," > [Alin Gabriel Serdean: ] ../ovs-dev-v2-datapath-windows-use-ip-proto-for- > tunnel-port-lookup.patch:162: trailing whitespace. > > + " type: %u, dst port: %u, name: %s", dupVport->ovsType, > > + transportPortDest, dupVport->ovsName); > > + goto Cleanup; > > + } > > + > > status = OvsInitTunnelVport(usrParamsCtx, > > vport, > > portType, @@ -2319,6 +2375,8 > > @@ OvsNewVportCmdHandler(POVS_USER_PARAMS_CONTEXT > usrParamsCtx, > > gOvsSwitchContext->dpNo); > > > > *replyLen = msgOut->nlMsg.nlmsgLen; > > + OVS_LOG_INFO("Created new vport, name: %s, type: %u", vport- > > >ovsName, > > + vport->ovsType); > > > > Cleanup: > > NdisReleaseRWLock(gOvsSwitchContext->dispatchLock, &lockState); > > diff - -git a/datapath-windows/ovsext/Vport.h b/datapath- > > windows/ovsext/Vport.h index 373896d..f0a9acd 100644 > > --- a/datapath-windows/ovsext/Vport.h > > +++ b/datapath-windows/ovsext/Vport.h > > @@ -145,9 +145,12 @@ POVS_VPORT_ENTRY > > OvsFindVportByHvNameA(POVS_SWITCH_CONTEXT switchContext, > > POVS_VPORT_ENTRY > OvsFindVportByPortIdAndNicIndex(POVS_SWITCH_CONTEXT > > switchContext, > > NDIS_SWITCH_PORT_ID > > portId, > > > > NDIS_SWITCH_NIC_INDEX index); - POVS_VPORT_ENTRY > > OvsFindTunnelVportByDstPort(POVS_SWITCH_CONTEXT switchContext, > > - UINT16 dstPort, > > - OVS_VPORT_TYPE ovsVportType); > > +POVS_VPORT_ENTRY > > OvsFindTunnelVportByDstPortAndType(POVS_SWITCH_CONTEXT > > switchContext, > > + UINT16 dstPort, > > + OVS_VPORT_TYPE > > +ovsPortType); POVS_VPORT_ENTRY > > OvsFindTunnelVportByDstPortAndNWProto(POVS_SWITCH_CONTEXT > > switchContext, > > + UINT16 dstPort, > > + UINT8 > > + nwProto); > > POVS_VPORT_ENTRY > > OvsFindTunnelVportByPortType(POVS_SWITCH_CONTEXT switchContext, > > OVS_VPORT_TYPE > > ovsPortType); > > > > -- > > 2.7.1.windows.1 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > http://openvswitch.org/mailman/listinfo/dev > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
