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
