Alin,
Thanks for the review. I have sent a v6. I realized that the 2nd comment
was previously addressed in v2, but not reflected in the v5 of the patch.
Thanks,
Sairam
On 6/13/16, 10:31 PM, "Alin Serdean" <aserd...@cloudbasesolutions.com>
wrote:
>Hi Sai,
>
>Thanks for incorporating the comments so far. It looks good but we need
>to treat corner cases like no valid resource allocations and NULL checks
>for parameter.
>
>
>
>A few small nits inlined.
>
>
>
>Thanks,
>
>Alin.
>
><-----------cut-------->
>
>> ctx->key.src.port = flowKey->ipKey.l4.tpSrc;
>
>> ctx->key.dst.port = flowKey->ipKey.l4.tpDst;
>
>> + if (flowKey->ipKey.nwProto == IPPROTO_ICMP) {
>
>> + ICMPHdr icmpStorage;
>
>> + const ICMPHdr *icmp;
>
>> + icmp = OvsGetIcmp(curNbl, l4Offset, &icmpStorage);
>
>> + ASSERT(icmp);
>
>> + ctx->key.src.port = ctx->key.dst.port =
>
>> + icmp->fields.echo.id;
>
>> +
>
>> + /* Related bit is set when ICMP has an error */
>
>> + /* XXX parse out the appropriate src and dst from inner
>>pkt */
>
>> + switch (icmp->type) {
>
>> + case ICMP4_DEST_UNREACH:
>
>> + case ICMP4_TIME_EXCEEDED:
>
>> + case ICMP4_PARAM_PROB:
>
>> + case ICMP4_SOURCE_QUENCH:
>
>> + case ICMP4_REDIRECT: {
>
>> + ctx->related = TRUE;
>
>> + }
>
>[Alin Gabriel Serdean: ] break;
>
>> + default:
>
>> + ctx->related = FALSE;
>
>[Alin Gabriel Serdean: ] break; Without breaks ctx->related will always
>be false;
>
>> + }
>
>> + }
>
>
>
><-----------cut-------->
>
>> //Delete and update the Conntrack
>
>> OvsCtEntryDelete(ctx->entry);
>
>> ctx->entry = NULL;
>
>> - entry = OvsCtEntryCreate(tcp, curNbl, ctx, key,
>
>> - commit, currentTime);
>
>> + entry = OvsCtEntryCreate(curNbl, key->ipKey.nwProto,
>>l4Offset,
>
>> + ctx, key, commit, currentTime);
>
>[Alin Gabriel Serdean: ] entry can be null, if for example the tcp packet
>was invalid. We need to treat that case because the following code will
>run after.
>
> /* Copy mark and label from entry into flowKey. If actions specify
>
> different mark and label, update the flowKey. */
>
> OvsCtUpdateFlowKey(key, state, zone, entry->mark, &entry->labels);
>
>
>
>> break;
>
>> }
>
>> }
>
>> @@ -401,15 +493,12 @@ OvsCtExecute_(PNET_BUFFER_LIST curNbl,
>
>> NDIS_STATUS status = NDIS_STATUS_SUCCESS;
>
>> POVS_CT_ENTRY entry = NULL;
>
>> OvsConntrackKeyLookupCtx ctx = { 0 };
>
>> - TCPHdr tcpStorage;
>
>> - UINT64 currentTime;
>
>> LOCK_STATE_EX lockState;
>
>> - const TCPHdr *tcp;
>
>> - tcp = OvsGetTcp(curNbl, layers->l4Offset, &tcpStorage);
>
>> + UINT64 currentTime;
>
>> NdisGetCurrentSystemTime((LARGE_INTEGER *) ¤tTime);
>
>>
>
>> /* Retrieve the Conntrack Key related fields from packet */
>
>> - OvsCtSetupLookupCtx(key, zone, &ctx);
>
>> + OvsCtSetupLookupCtx(key, zone, &ctx, curNbl, layers->l4Offset);
>
>>
>
>> NdisAcquireRWLockWrite(ovsConntrackLockObj, &lockState, 0);
>
>>
>
>> @@ -418,11 +507,12 @@ OvsCtExecute_(PNET_BUFFER_LIST curNbl,
>
>>
>
>> if (!entry) {
>
>> /* If no matching entry was found, create one and add New
>>state */
>
>> - entry = OvsCtEntryCreate(tcp, curNbl, &ctx,
>
>> + entry = OvsCtEntryCreate(curNbl, key->ipKey.nwProto,
>
>> + layers->l4Offset, &ctx,
>
>> key, commit, currentTime);
>
>> } else {
>
>> /* Process the entry and update CT flags */
>
>> - entry = OvsProcessConntrackEntry(curNbl, tcp, &ctx, key,
>
>> + entry = OvsProcessConntrackEntry(curNbl, layers->l4Offset,
>
>> + &ctx, key,
>
>> zone, commit, currentTime);
>
>> }
>
>>
>
>> diff --git a/datapath-windows/ovsext/Conntrack.h b/datapath-
>
>> windows/ovsext/Conntrack.h
>
>> index a754544..883ac57 100644
>
>> --- a/datapath-windows/ovsext/Conntrack.h
>
>> +++ b/datapath-windows/ovsext/Conntrack.h
>
>> @@ -80,8 +80,11 @@ typedef struct OvsConntrackKeyLookupCtx {
>
>>
>
>> #define CT_HASH_TABLE_SIZE ((UINT32)1 << 10) #define
>
>> CT_HASH_TABLE_MASK (CT_HASH_TABLE_SIZE - 1)
>
>> -#define CT_ENTRY_TIMEOUT (2 * 600000000) // 2m
>
>> -#define CT_CLEANUP_INTERVAL (2 * 600000000) // 2m
>
>> +#define CT_INTERVAL_SEC 10000000LL //1s
>
>> +#define CT_ENTRY_TIMEOUT (2 * 60 * CT_INTERVAL_SEC) // 2m
>
>> +#define CT_CLEANUP_INTERVAL (2 * 60 * CT_INTERVAL_SEC) // 2m
>
>> +
>
>> +
>
>> /* Given POINTER, the address of the given MEMBER in a STRUCT object,
>
>> returns
>
>> the STRUCT object. */
>
>> #define CONTAINER_OF(POINTER, STRUCT, MEMBER)
>> \
>
>> @@ -99,9 +102,13 @@ BOOLEAN OvsConntrackValidateTcpPacket(const
>
>> TCPHdr *tcp); OVS_CT_ENTRY * OvsConntrackCreateTcpEntry(const TCPHdr
>
>> *tcp,
>
>> PNET_BUFFER_LIST nbl,
>
>> UINT64 now);
>
>> +OVS_CT_ENTRY * OvsConntrackCreateOtherEntry(UINT64 now);
>
>> enum CT_UPDATE_RES OvsConntrackUpdateTcpEntry(OVS_CT_ENTRY*
>
>> conn_,
>
>> const TCPHdr *tcp,
>
>> PNET_BUFFER_LIST nbl,
>
>> BOOLEAN reply,
>
>> UINT64 now); -#endif /*
>>__OVS_CONNTRACK_H_ */
>
>> \ No newline at end of file
>
>> +enum ct_update_res OvsConntrackUpdateOtherEntry(OVS_CT_ENTRY
>
>> *conn_,
>
>> + BOOLEAN reply,
>
>> + UINT64 now); #endif /*
>
>> +__OVS_CONNTRACK_H_ */
>
>> diff --git a/datapath-windows/ovsext/ovsext.vcxproj b/datapath-
>
>> windows/ovsext/ovsext.vcxproj
>
>> index 0356ddf..0ad4c58 100644
>
>> --- a/datapath-windows/ovsext/ovsext.vcxproj
>
>> +++ b/datapath-windows/ovsext/ovsext.vcxproj
>
>> @@ -176,6 +176,7 @@
>
>> <ItemGroup>
>
>> <ClCompile Include="Actions.c" />
>
>> <ClCompile Include="BufferMgmt.c" />
>
>> + <ClCompile Include="Conntrack-other.c" />
>
>> <ClCompile Include="Conntrack-tcp.c" />
>
>> <ClCompile Include="Conntrack.c" />
>
>> <ClCompile Include="Debug.c" />
>
>> --
>
>> 2.5.0.windows.1
>
>>
>
>> _______________________________________________
>
>> dev mailing list
>
>> dev@openvswitch.org
>
>>
>>https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_mailm
>>an_listinfo_dev&d=CwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=
>>Dcruz40PROJ40ROzSpxyQSLw6fcrOWpJgEcEmNR3JEQ&m=xvCJOl4b1LctBJTX-A6F3pVVs73
>>xHZzLjSIA3s8XeD0&s=5QcN4e3x0ubrsCKIoGSFRIvJ6rV26f5Ope3ID635gDg&e=
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
