On Mon, 29 Sep 2014, Nagel, Bill wrote:

> This patch implements the CIFS dialect of SMB.

Thanks a lot for your work on this! I'm impressed the patch isn't bigger, but
I have a feeling it'll grow...

Some additional comments on top of what Steve already mentioned. In general I
think you need to consider that you're donating this code to a project for the
long term. We, and future hackers like us, want to be able to read and
understand code we merge for a long time into the future. We need code to be
readable and understandable.

1 - your patch leaks memory in OOM situations (we want perfect cleanup in all
     error situations as well), this means that if you do 3 memory allocations
     and the 3rd call fails, the first two must free their memory when the
     failure is returned.

2 - the magic constant 0x11000 is used on several places without motivation or
     explanation. Why can't it be smaller? Why doesn't it have to be bigger?

3 - 'make checksrc' will point out some formatting nits for you

4 - In smb_send_setup

  A) what are the "Linux" and "curl_" parts used for?

  B) lots of uncommented constants in that code. 0xff and 0x73 for example.
     What about using defines with suitable names instead? Other functions
     use things like 0x71 and 0x2e. What do they mean?

  C) are those strcpy() calls there really safely bounded so there's no risk
     even if I send in a 10K long user name? Doesn't look like that! The same
     goes for the strcpy() calls in smb_send_tree_connect() and some other
     places. Every single use of strcpy() needs to be checked for length and
     boundaries and preferably a comment next to it should mention how it is
     OK.

5 - What are your thoughts around test cases for this? Without tests we know
     that code bitrots and breaks over time. How can we get a simple test
     server for this?

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