Hi Daniel,

2012/4/16 Daniel Stenberg <daniel_at_haxx.se>
>
> 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.

Sure, thanks a lot. You don't need to do the work yourself, just tell
me what needs to be fixed/changed, just like you are doing now. ;-)

>
>
> 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.

Yes, that's totally true and needs to be changed for real-world scenarios.

>
>
> 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...

Yes, this is correct. The SSPI API and especially Schannel uses the
term "credential handle" for the session context and the term
"security context" for the connection context. I can add comments in
the code to make that clear. It is required to store the "credential
handle" in order to make it able to re-use SSL/TLS sessions.
Please see this question/answer on stackoverflow for more information:
http://stackoverflow.com/questions/10084257/sspi-schannel-api-can-credential-handles-be-re-used

>
>
> 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'

Ok, will fix those.

>
>
> 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?

There is basically no reason for these specific numbers. 4096 is the
initial read buffer size and 2048 is the increase/decrease step size.
I could certainly use defines for those, but in reality it might be
even better to remove them and find a way to figure out how many data
actually needs to be stored. I am currently freeing unneeded space
after the read process finishes, but there might be a better solution.

>
> 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?

You mean building up the message instead of doing multiple failf
calls? Basically any of these flags can be unset and this means that
the required attribute was unable to be established for the
connection. Therefore it is important to output any missing flag for
debug purposes.

>
>
> 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.

I guess you are talking about curl_schannel_cred and
curl_schannel_ctxt? Sure, that can be easily changed.

>
>
> 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.

I am fine with sending you everything as a series for patches, sure.
Squashing individual commits also makes sense, but generally speaking
I must say that I am not really a fan of rewriting the history of a
published git repository. Even though it is not the main-line, but my
fork. Rewriting the history means that my fork will need to be
force-pushed (or later hard-resetted to whatever you have merged).

It might take me a while to work on all this, as I need to focus on
something else for the next 3 weeks. But after that I will look into
new functionality and the things on the TODO list as well.

Thanks for the feedback so far!

Best regards,
Marc

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