2011/8/26 Steve Holme wrote: > 0001-Added-support-for-NTLM-authentication-to-SMTP.patch > > I have enclosed a patch adding NTLM support to SMTP. > > Please note that halfway through this patch I realised that > Curl_ntlm_create_type1_message() and Curl_ntlm_create_type3_message didn't > support the outlen / size parameter (as I had written a couple of weeks > ago). As such, this patch includes the addition of this parameter to these > functions, as well as the actual addition of NTLM to SMTP. I hope this is > acceptable, rather than splitting the work into two patches, so for speed > and as it's almost midnight and I need to get some sleep I have attached one > patch ;-)
If case the length of the string returned by Curl_ntlm_create_type1_message() and Curl_ntlm_create_type3_message() at least was used somewhere I could find a reason to let these functions return its value using outlen. Given that it is not used I find no need right now to change the interface of these two functions. Steve, please, look at your patch the outlen is given no real use. Actually smtp_auth_ntlm() in this patch should not have the outlen parameter. In smpt.h you introduce new SMTP_AUTHNTLM and SMTP_AUTHNTLMTYPE2 in smtpstate enum. And in smtp.c you introduce new functions smtp_state_auth_ntlm_resp() and smtp_state_auth_ntlm_type2_resp(). When you are creating them it is esay to remember what they do and what the states mean. But if you read the code that uses those functions two or three months later, or anyone else right now, you could start wondering with the smtpstate enum if both NTLM and NTLM 2 are used and something similar happens with those function names. It would be more clear if 'msg-typeX' or something were used in those names. Aha!, but what actually happens is that you are introducing these to somehow allow disabling the 'initial-response' sending in the AUTH command for the NTLM authentication. In any case bad choice of names. You also place NTLM authentication as the preferred method above any aother one. I wonder if this should be the preferred method and if STARTTLS influence should be considered in this placement. For all of the above I have to reject this patch. In case mentioned problems above didn't exist, given that we are in feature freeze period and that it introduces functional changes we neither can accept it. Changes which represented no functional change, or that represented true fixes even for marginal cases, have been done for nearly a month now in preparation for the new functionalities. So lets do things right when these can finally be introduced. Is there something that I might have missed that still needs 'fixing' or preparation but that truly introduces no functional change, and that we could evaluate if it could be accepted in feature freeze period? -- -=[Yang]=- ------------------------------------------------------------------- List admin: http://cool.haxx.se/list/listinfo/curl-library Etiquette: http://curl.haxx.se/mail/etiquette.html
