On Mon, 6 Oct 2014, Steve Holme wrote:

> Thank you for splitting the patch up.

My fullest of apologies for not getting back to you sooner - as you're probably seen it has been a hectic few weeks here getting the 7.39.0 release out and starting to push my next batch of changes / additions.

> > Documentation, followed by tests, will be the next set of patches.
>
> Great - we look forward to receiving them.

Is there any chance you could get documentation patches to us sooner rather than later as this will help me (and possibly others) understand your implementation a little more and test it without any test cases?

> I'll take a look at the patches in more detail tomorrow as I'm pretty
> shattered now - and hopefully in the meantime others will take a look
> too ;-)

I have now created a branch and taken a proper look at your patches before I continue with my next chunk of curl changes.

First of all the quality of your patches are very high and in keeping with the curl coding standards.

1) Unfortunately I do have some nits:

* There are still a couple of instances of "if(value == 0)" - we tend to prefer "if(!value)" in this instance - smb.c Line 221, 477 (I can fix these up however please see my other comments)
* There are still some hard coded values, which don't make reading the code easy. For example:

0xe - Why 14?
If(smbc->got < 4) - What significance is 4 - is this a minimum length of something?
nbt_size = ntohs(*(uint16_t*)(buf + 2)) + 4 - Why +2 and +4?
ssize_t byte_count = strlen(smbc->user) + strlen(smbc->domain) + 2
setup.capabilities = 0x18 - This should be a constant

and others.

If these are sizes of variables then please use sizeof(). If they are constants that mean something can we please use #defines. If a + 2 exists because of the domain separator and a null terminator then please comment it as such ;-)

2) More importantly:

* Do we need to support / and \ as separators for the domain in the username? I appreciate we currently do this elsewhere in curl but my understanding is that the correct format of down-level usernames is DOMAIN\user where DOMAIN can be either the flat or full NetBIOS domain name. I also believe we opened discussion earlier this year about removing the / support as I believe this is a curlism and not industry standard - however - if you know of a reason to support both please free to correct me ;-)
* The SMB code isn't conditionally compiled out with the appropriate CURL_DISABLE_SMB protection except in url.c and version.c - the source code needs it as well just like http.[c|h], smtp.[c|h], pop3.[c|h], etc... do
* Do we need to make the SMB code dependent on USE_NTLM as well? - so when performing #ifndef CURL_DISABLE_SMB use #if defined(USE_NTLM) && !defined(CURL_DISABLE_SMB) instead - see point 5 below.
* I don’t think we should be typedefing stdint types in smb_data.h depending on whether we HAVE_STDINT_H and HAVE_LONGLONG. If we are going to take this approach then they should be defined properly in either curl_setup.h or the config header files, however, I'd rather see us use the more common non stdint compliant types used in the SMB code as we are supposed to be C89 compliant. However, we may want to open up discussion a little more rather than just input from myself.

3) When I came to build (Windows x64 with OpenSSL) I received the following builds warnings:

smb.c(229): warning C4018: '<' : signed/unsigned mismatch
smb.c(261): warning C4244: 'function' : conversion from 'unsigned __int64' to 'u_short', possible loss of data
smb.c(268): warning C4013: 'getpid' undefined; assuming extern returning int
smb.c(370): warning C4244: '=' : conversion from '__int64' to 'uint16_t', possible loss of data
smb.c(396): warning C4244: '=' : conversion from '__int64' to 'uint16_t', possible loss of data
smb.c(412): warning C4267: '=' : conversion from 'size_t' to 'uint16_t', possible loss of data
smb.c(457): warning C4244: '=' : conversion from 'curl_off_t' to 'uint32_t', possible loss of data
smb.c(484): warning C4244: '=' : conversion from 'curl_off_t' to 'uint32_t', possible loss of data

If the conversions between different sized integers is intentional then please uses the functions in warnless.h to explicitly convert them and trap a large value.

4) If I build for Windows SSPI then I also get the following undefined functions:

smb.c(343): warning C4013: 'Curl_ntlm_core_mk_lm_hash' undefined; assuming extern returning int
smb.c(344): warning C4013: 'Curl_ntlm_core_lm_resp' undefined; assuming extern returning int
smb.c(335): warning C4101: 'nt_hash' : unreferenced local variable

As such, we will need to either:

* Alter curl_ntlm_core.[c|h] so the appropriate functions are exposed for SMB
* Disable SMB for Windows SSPI
* Disable SMB for Windows.
* Use the appropriate Windows SSPI based functions - if there are some

I would be interested to know your thoughts here and what direction you are heading on Windows?

5) If I build without NTLM support (either a) without an SSL/crypto library, b) without Windows SSPI or c) I define CURL_DISABLE_CRYPTO_AUTH) then I also received the same unresolved external function warnings.

Kind Regards

Steve

-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette: http://curl.haxx.se/mail/etiquette.html
Received on 2014-11-08