On Mon 29 Sep 2014, Nagel, Bill wrote:

> This patch implements the CIFS dialect of SMB.

Whilst I've taken a look at your SMTP patch I'm probably not going to be
able to merge and test this one myself as I'm a Windows engineer, however, I
do have some initial feedback:

* I would recommend you split the patch up into manageable stages - such as
the modifications to configure.ac, the introduction of the SMB defines, the
main SMB files, then integration into the rest of curl/libcurl.
* Use git patches rather than diff's as it will make merging a lot earier
for us - meaning less time to get your work into the repository ;-)
* Have you prepared any tests that can be added to our test suite?
* Have you prepared any documentation for SMB? For example in the curl man
page, the URL section of the libcurl documentation, any curl_easy_setopt()
that are affected by the new protocol, etc...
* There are a number of instances that I saw from scanning the code that
don't adhere to our coding standard - 1) braces around single line if
statements are not needed 2) Action parts of a single line if statement are
not on a separate line and currently on the same line as the condition
* I would personally prefer that you don't use assignment operators in if
statements
* How does this addition affect build platforms that don't use autoconf -
for example Windows? Is CURL_DISABLE_SMB defined for such platforms / does
it need to be?

Kind Regards

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