[ovs-dev] [PATCH v2 5/5] datapath-windows: Add support for OVS_KEY_ATTR_TCP set action

[Sairam Venugopal]

I was looking to consolidate both TCP and UDP actions to minimize duplication and error. The two functions have very similar behavior except for the protocol.

Thanks,
Sairam



On 1/10/17, 1:32 AM, "Alin Serdean" <aserdean at cloudbasesolutions.com> wrote:

>Hi Sai,
>
>Thanks for the review! Comments inlined.
>
>> -----Original Message-----
>> From: Sairam Venugopal [mailto:vsairam at vmware.com]
>> Sent: Tuesday, January 10, 2017 3:40 AM
>> To: Alin Serdean <aserdean at cloudbasesolutions.com>;
>> dev at openvswitch.org
>> Subject: Re: [ovs-dev] [PATCH v2 5/5] datapath-windows: Add support for
>> OVS_KEY_ATTR_TCP set action
>> 
>> Please see my comment inline:
>> >+
>> >+    ASSERT(layers->value != 0);
>> >+
>> >+    if (!layers->isUdp) {
>> 
>> Sai: This is incorrect. Should be checking for !layers->isTcp. This will end up
>> failing for tcp set action.
>> Can you consolidate TCP/UDP actions to call into 1 function and use switch
>> instead. Most of the code has been copied over from UDP.
>[Alin Serdean] Sorry I missed that! I think it is more readable this way and can be reused under different circumstances. Do you want me to try to move more code outside of the functions?
>> 
>