[ovs-dev] [PATCH 3/4] datapath-windows: Conntrack - Introduce support for tracking related connections

[Alin Serdean]

Sounds good :).

> -----Original Message-----
> From: Sairam Venugopal [mailto:vsairam at vmware.com]
> Sent: Tuesday, December 6, 2016 8:53 PM
> To: Alin Serdean <aserdean at cloudbasesolutions.com>;
> dev at openvswitch.org
> Subject: Re: [ovs-dev] [PATCH 3/4] datapath-windows: Conntrack - Introduce
> support for tracking related connections
> 
> Alin,
> 
> Thanks for the review.
> 
> Yes, my main intent was to introduce something simple that we can iterate
> on. I did look at the safe-string functions. Unfortunately, I couldn¹t find Rtl*
> functions that I could reuse here. That¹s the reason why I had to introduce
> the two str functions.
> 
> I will apply the other review changes.
> 
> Thanks,
> Sairam
> 
> On 12/5/16, 10:29 AM, "Alin Serdean" <aserdean at cloudbasesolutions.com>
> wrote:
> 
> >Thanks for the patch!
> >
> >It looks good as an initial point that we can add more complexity in
> >the future :).
> >
> >One personal preference would be to use the string Rtl* functions:
> >https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__msdn.microsoft.com
> >_en
> >-2Dus_library_windows_hardware_ff563642-28v-3Dvs.85-
> 29.aspx&d=DgIFAg&c=
> >uil
> >aK90D4TOVoH58JNXRgQ&r=Z6vowHUOjP5ysP_g372c49Nqc1vEKqHKNBkR5
> Q5Z7uo&m=2sP
> >vKu
> >qF3qQqckq-
> dZ1aozPYKHpCXTBU8HLAK9IdNME&s=yJ2hG5syDtE0Rjr1S7wMgduJky837aX
> >bsT
> >X8VK1LSYI&e=
> >https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__msdn.microsoft.com
> >_en
> >-2Dus_library_windows_hardware_ff563644-28v-3Dvs.85-
> 29.aspx&d=DgIFAg&c=
> >uil
> >aK90D4TOVoH58JNXRgQ&r=Z6vowHUOjP5ysP_g372c49Nqc1vEKqHKNBkR5
> Q5Z7uo&m=2sP
> >vKu
> >qF3qQqckq-
> dZ1aozPYKHpCXTBU8HLAK9IdNME&s=2IAdLCggWOSSUAVbyLMnclw5A_lYG
> VR
> >dwY
> >9wTadrzrs&e=
> >
> >Some white space issues:
> >../ovs-dev-3-4-datapath-windows-Conntrack---Introduce-support-for-track
> >ing
> >-related-connections.patch:591: new blank line at EOF.
> >+
> >warning: 1 line adds whitespace errors.
> >
> >Other small comments inlined.
> >
> >> -----Original Message-----
> >> From: ovs-dev-bounces at openvswitch.org [mailto:ovs-dev-
> >> bounces at openvswitch.org] On Behalf Of Sairam Venugopal
> >> Sent: Thursday, December 1, 2016 11:19 PM
> >> To: dev at openvswitch.org
> >> Subject: [ovs-dev] [PATCH 3/4] datapath-windows: Conntrack -
> >> Introduce support for tracking related connections
> >>
> >> Introduce a new table to track related connections. This table will
> >>be used to  track FTP data connections based on the control
> >>connection. There is a new  Conntrack-ftp.c to parse incoming FTP
> >>messages to determine the related  data ports. It creates a new entry
> >>in the related connections tracker table. If  there is a matching FTP
> >>data connection, then the state for that connection is  marked as
> >>RELATED.
> >>
> >> Signed-off-by: Sairam Venugopal <vsairam at vmware.com>
> >> ---
> >-------------------------------->cut<--------------------------
> >> +            /* End of FTP response is either ) or \r\n */
> >[Alin Serdean] Check for \n as well.
> >> +            if (*buf == ')' || *buf == '\r') {
> >-------------------------------->cut<--------------------------
> >[Alin Serdean] Maybe add define for 227 (Passive mode) and the comment
> >above in the define.
> >> +        if ((len >= 4) && (OvsStrncmp("227", ftpMsg, 3) == 0)) {
> >> +            ftpType = FTP_TYPE_PASV;
> >> +            /* There are various formats for PASV command. We try to
> >>support
> >> +             * some of them. This has been addressed by RFC 2428 -
> >>EPSV.
> >> +             * Eg:
> >> +             *    227 Entering Passive Mode (h1,h2,h3,h4,p1,p2).[Alin
> >>Serdean]
> >> +             *    227 Entering Passive Mode (h1,h2,h3,h4,p1,p2
> >> +             *    227 Entering Passive Mode. h1,h2,h3,h4,p1,p2
> >> +             *    227 =h1,h2,h3,h4,p1,p2
> >> +             */
> >-------------------------------->cut<--------------------------
> >> +VOID
> >> +OvsCleanupCtRelated (VOID)
> >[Alin Serdean] Extra " " character.
> >