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