[ovs-dev] [PATCH] datapath-windows: Update ICMP-Type and Code comparison in CT lookup

Anand Kumar kumaranand at vmware.com
Tue Aug 15 22:21:14 UTC 2017


Hi Alin,

Thanks for the review. Please find my responses inline.
Will address the review comments and send out v2 version of the patch.

Thanks,
Anand Kumar

On 8/14/17, 8:04 PM, "Alin Serdean" <aserdean at cloudbasesolutions.com> wrote:

    Hi Sai and Anand,
    
    Thanks a lot for the patch. I have a few questions regarding the approach. Please see inline.
    
    > -----Original Message-----
    > From: ovs-dev-bounces at openvswitch.org [mailto:ovs-dev-
    > bounces at openvswitch.org] On Behalf Of Anand Kumar
    > Sent: Friday, August 11, 2017 11:42 PM
    > To: dev at openvswitch.org
    > Subject: [ovs-dev] [PATCH] datapath-windows: Update ICMP-Type and Code
    > comparison in CT lookup
    > 
    >  - Update the CT comparison function to compare individual fields instead of
    > NdisEqualMemory.
    [Alin Serdean] I don't like this change, especially mixing both members of union, i.e:
    > +            (ctxKey.dst.port == entryKey.dst.port) &&
    > +            (ctxKey.dst.icmp_id == entryKey.dst.icmp_id) &&
    Why are you trying to change via member by member comparison?
   [Anand Kumar]: Done. Previously, we ran into issues while verifying NAT with ICMP, which now is fixed with my other patch.
    > - Add in some padding for the ct_endpoint's union.
    [Alin Serdean] Why is this needed? 
    [Anand Kumar] This is needed because size of the union is 32 bits. 
   Another question do we still need the 'pad' member inside ct_endpoint (https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_openvswitch_ovs_blob_master_datapath-2Dwindows_ovsext_Conntrack.h-23L51&d=DwIFAg&c=uilaK90D4TOVoH58JNXRgQ&r=Q5z9tBe-nAOpE7LIHSPV8uy5-437agMXvkeHHMkR8Us&m=1Rj8RlVaAovz2iLrWHvT6N469qFN036rzjLkBJweZDw&s=hky8AVI9m8GjZOD8r21WYvEHBPcDcVVZiwMKqk07IwU&e= ) ?
    [Anand Kumar] Yes, the padding at the end of structure is no longer needed. I will address in next version.
    > - Update the Orig Tuple to use ICMP Type and Code instead of Port for ICMP
    [Alin Serdean] Agreed
    > 
    > Co-authored-by: Sairam Venugopal <vsairam at vmware.com>
    > Signed-off-by: Anand Kumar <kumaranand at vmware.com>
    







More information about the dev mailing list