On Sat, 14 Apr 2012, Marc Hoersken wrote:

> I would really like to see those changes make it into libcurl. Maybe more
> testing is required and therefore I also ask you people to test it. Once you
> also consider it stable, it can be merged into libcurl, even though there
> are some long-term TODOs open.

Thanks a lot for your great work on this Marc. I certainly intend to work on
merging this into the master branch asap, but while I'm trying to get back up
to speed from my vacation last week, I want to offer my review feedback on
your patch as posted.

1 - the biggest flaw I can see is what you seem to refer to as "write
     buffering" which isn't implemented. The code considers swrite() returns
     that are less than "full" to be errors. I suspect that will make this
     code a bit flakey when used out there in the harsh reality until fixed.

2 - The code speaks of "credential handles" when it uses the session id cache
     functions. Why? Session id is an SSL term and it is not really about
     credentials as I see it...

Then over to some pure style nits:

3 - we always put the first brace when declaring functions on column 0

4 - I spotted a case of 'char* name' while we always use 'char *name'

5 - On many places in the code you use the hardcoded numers 4096 and 2048.
     Why those numbers? And why not use defined names for them?

6 - The the function 'schannel_connect_step3' you have a series of if()s with
     a large amount of next to identical error strings. Won't it make more
     sense to just figure out the unique part and have the generic part handled
     generically?

7 - I'm not a fan of typedef'ed structs. I want code to use 'struct name' when
     structs are used instead of just 'name' to make it more obvious to
     readers.

PS, I must agree with what was said already: 'git format-patch' allows you to
get a full series of patches from a branch. And while I haven't checked your
branch in detail yet, it might make sense for you to squash that into a
smaller set of commits before we merge.

-- 
  / daniel.haxx.se
-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette:  http://curl.haxx.se/mail/etiquette.html