On Fri, 3 Oct 2014, Nagel, Bill wrote:

> The updated patch is attached.

Thank you for your updated patch and for using git format patch - it
certainly made my quick code review a lot easier ;-)

1) Unfortunately, the patch breaks both the makefile.vc based builds as well
as the Visual Studio builds with error such as:

 smb.h : error C2061: syntax error : identifier 'uint32_t'

* Can the use of uint16_t, uint32_t for example be replaced with regular
unsigned short and unsigned int as not all platforms have stdint.h?
* Should smb.h either be included when conditionally compiled using
CURL_DISABLE_SMB or are you intending to add Windows support?

2) Whilst we have defined our coding standards on the website there are a
few things that are not covered by it and I tend to be quite picky about
some of the things not covered there as well as well as coding layout ;-)

* I would recommend spacing some of the code a little more, so that logical
chunks are separate from each other by a blank line, as well as variables
being separate from code. A very simple example would be:

static CURLcode smb_send_tree_disconnect(struct connectdata *conn)
{
  struct smb_tree_disconnect tree;
  memset(&tree, 0, sizeof(tree));
  return smb_send_message(conn, SMB_COM_TREE_DISCONNECT, &tree,
sizeof(tree));
}

Could be:

static CURLcode smb_send_tree_disconnect(struct connectdata *conn)
{
  struct smb_tree_disconnect tree;

  memset(&tree, 0, sizeof(tree));

  return smb_send_message(conn, SMB_COM_TREE_DISCONNECT, &tree,
sizeof(tree));
}

This provides consistency with other areas of smb.c like
smb_send_and_recv().

* Place parts of a switch/case statement on separate lines for example in
smb.c:

  case SMB_OPEN: result = smb_send_open(conn); break;

Would be:

  case SMB_OPEN:
    result = smb_send_open(conn);
  break;

Again this would be consistent with your own code.

* I would recommend placing blank lines after if blocks as well, especially
when multiple if statements appear after each other - as they can often look
like if/else blocks at a glance. So:

  if(!smbc->send_size)
    return CURLE_OK;
  result = Curl_write(conn, FIRSTSOCKET, smbc->send_buf + smbc->sent,
                      len, &bytes_written);
  if(result)
    return result;
  if(bytes_written != len)
    smbc->sent += bytes_written;
  else
    smbc->send_size = 0;

Would become:

  if(!smbc->send_size)
    return CURLE_OK;

  result = Curl_write(conn, FIRSTSOCKET, smbc->send_buf + smbc->sent,
                      len, &bytes_written);
  if(result)
    return result;

  if(bytes_written != len)
    smbc->sent += bytes_written;
  else
    smbc->send_size = 0;

3) There are constant values in code rather than being #defined:

  setup.max_buffer_size = 4096

Rather than:

  setup.max_buffer_size = MAX_BUFFER_SIZE;

and:

  byte_count += 24 + 24

Which could be:

  byte_count += sizeof(lm) + sizeof(nt).

As well as the same for:

  setup.lengths[0] = 24;
  setup.lengths[1] = 24;
  memcpy(p, lm, 24); p += 24;

4) Whilst related please don't include multiple statement on one line.
Variables on one line, whilst I tend not to do it myself, I do allow if I
push a patch ;-)

5) As I think you may haved noticed we try and prefix the protocol name onto
static functions. So pop_message() and format_message() would become
smb_pop_message() and smb_format_message.

6) I would recommend some standard (ish) comments at the top of smb.c such
as:

/*Local API functions */

And

/*
 * SMB handler interface.
 */

7) You are missing curl memory tracking / debugging includes:

#include "curl_memory.h"
/* The last #include file should be: */
#include "memdebug.h"

8) Does smb_data.h need to be separate from smb.h? You'll note that there re
private smtp only defines and structures in smtp.h.

9) Additionally, as I mentioned in my previous email:

* Could you please split the patch up into smaller chunks - this makes
reviewing the work a lot easier and making sure we push changes
incrementally that don't necessary effect each other. For example the
configure.ac modifications could be pushed to make sure we don't break
existing builds, then the addition of the smb files, updates to version,
updates to curl command line tool, etc... rather than one great big change
that then breaks things.
* There is not documentation - are you working on that separately?

10) Finally could you please format your commit comments as per:

http://curl.haxx.se/dev/contribute.html

Many thanks

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