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" <aserd...@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_g372c49Nqc1vEKqHKNBkR5Q5Z7uo&m=2sPvKu >qF3qQqckq-dZ1aozPYKHpCXTBU8HLAK9IdNME&s=yJ2hG5syDtE0Rjr1S7wMgduJky837aXbsT >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_g372c49Nqc1vEKqHKNBkR5Q5Z7uo&m=2sPvKu >qF3qQqckq-dZ1aozPYKHpCXTBU8HLAK9IdNME&s=2IAdLCggWOSSUAVbyLMnclw5A_lYGVRdwY >9wTadrzrs&e= > >Some white space issues: >../ovs-dev-3-4-datapath-windows-Conntrack---Introduce-support-for-tracking >-related-connections.patch:591: new blank line at EOF. >+ >warning: 1 line adds whitespace errors. > >Other small comments inlined. > >> -----Original Message----- >> From: ovs-dev-boun...@openvswitch.org [mailto:ovs-dev- >> boun...@openvswitch.org] On Behalf Of Sairam Venugopal >> Sent: Thursday, December 1, 2016 11:19 PM >> To: d...@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 <vsai...@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. > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
