Hi Gokhan,

> I will let you know my thoughts on the patch a little later.

Generally speaking it looks very good. However, I've only had time this
evening to read the smtp code in details rather than the modifications to
the md5 code. I will hopefully take a look at that in more detail tomorrow
but I might need someone else on the list to help out here.

1) I would recommend splitting your change set up into three separate
patches. I found this a bit strange when I joined curl but it allows for
easier track-ability of changes and reverting them if we accidentally break
something. As such I would recommend the following patches:

A) Make the necessary changes to curl_md5.h and md5.c including the
introduction of the MD5_DIGEST_LEN constant. At this point we shouldn't have
broken any existing code.
B) Make the changes to smtp_state_authcram_resp() and perform any of the
items that I've listed in point 4 below.
C) A final patch introducing the new digest-md5 functionality in smtp.c.

Not only will this allow easier tracking of changes but it will also allow
someone else from this list to take a look at the md5 modifications as a
separate exercise ;-)

2) Is there any reason why AUTH_DIGEST_MD5 appears before AUTH_CRAM? The
code isn't at fault... I just wondered what the rational for this was and
what would a server prefer if it supported both? Would the server send "250
AUTH DIGEST-MD5 CRAM-MD5" or "250 AUTH CRAM-MD5 DIGEST-MD5". Obviously I'm
hoping to change this when I make the modification to curl to allow it to
honour the server's preferred order of mechanisms but in the meantime I have
tried to order the mechanisms based on priority / security level but I
honestly don't know how it compares to cram.

3) A few general style issues:

A) Could you align the parameter of smtp_digest_get_key_value() and
smtp_state_authdigest_resp() with the first parameter of those functions?
B) Could you declare pointers with the asterisk next to the variable name
please rather than for example "char * chlg64" and "unsigned char * chlg" -
My own preference is to have it next to the type (as I'm mainly a C++
programmer with the exception of curl and some C# stuff I do) but we do this
the old skool way here so "char *chlg64" for example - and yes I admit that
smtp_state_authcram_resp() is inconsistent here as well but I will tidy that
up after your work is complete ;-)
C) Also could you have one variable per line in declarations so i in
smtp_state_authdigest_resp() should be "size_t i;" on its own line rather
than coupled with len.
D) Please don't compare against NULL, TRUE or FALSE in if statements -
instead use if(variable) or if(!variable) as you have done elsewhere.
E) I would put spaces between the 2 and the asterisk and between the
asterisk and MD5_DIGEST_LEN on lines 863, 876 and 928 just to be consistent
with the rest of your changes.
F) No need for extra blank line on line 1075

4) Other changes that I think we should consider:

A) I prefer the name of your state constants that include MD5 on the end.
(For example SMTP_AUTHDIGESTMD5) I think we should change SMTP_AUTHCRAM to
SMTP_AUTHCRAMMD5.
B) It might we worth changing the state constants to be of the format
SMTP_STATE_AUTH_XXXX (For example: SMTP_STATE_AUTH_CRAM_MD5 and
SMTP_STATE_AUTH_DIGEST_MD5 as these are starting to get a little confusing
when compared to the AUTH constants).

If you fancy doing both of these I would recommend that it is done in your
second patch (as mentioned earlier) as a "smtp: Pre digest-md5 tidy up"
patch if you like ;-)

Sorry if some of that seems petty or ruthless - that's not my intention.

Kind Regards

Steve

-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette: http://curl.haxx.se/mail/etiquette.html
Received on 2012-01-10