[PATCH] src/net_socket.c: Bind outgoing TCP sockets to `BindToAddress'.

[Florian Forster]

Hi Guus,

On Wed, May 27, 2009 at 01:18:52PM +0200, Guus Sliepen wrote:
> Please do not use asserts. I'd rather see errors logged and functions
> return safely than tincd aborting. Especially in this case, binding to
> an interface or address is not so important that tinc should fail in
> case of errors.

I disagree: Assertions are used for stuff that may never ever fail. If
any of those assertions is triggered, there is a serious bug in the
program and it's better to run into an assertion (where a SIGABORT) is
triggered than running into a segmentation fault or similar.

If you prefer the segfault, you can compile with -DNDEBUG to disable
assertions.

> > +static int bind_to_interface (int sd) /* {{{ */
> [...]
> > +} /* }}} int bind_to_interface */
> 
> Please keep the coding style consistent with what is already in tinc.

I'll try.

> Is this /* {{{ */ ... /* }}} repeated declaration */ thing something
> that is used in some IDE or is it your own style?

`{{{' and `}}}' are ``markers'' used by vim and can be used to ``fold''
functions, i. e. to make it easier to navigate within the file by
selectively only displaying those functions you're interested in. You
can enable this behavior using
  :set foldmethod=marker
See also:
  :help folding

Repeating the beginning of long blocks in a comment at the end is ``my
style'' and increases readability of large blocks (especially around the
block's end) by a lot. By the way: At the end of functions I only
document the name of the function and its return type, because that's
what is interesting when reading the end of a function.

> > +static int bind_to_address (connection_t *c) /* {{{ */
> [...]
> > +	/* We're called from `do_outgoing_connection' only. */
> 
> Hm, maybe this function could be made to bind any socket to an
> address? Maybe it can avoid some code duplication from
> setup_listen_socket() and setup_vpn_in_socket().

I haven't looked at it in so much detail, but it'd only make sense if we
pull in more code from `src/net_setup.c' and use the resolve
functionality once again. But then the loop over `ai_list' would have to
be different for listening sockets and at the end you'd have a function
with a lot of ``if(listening)'' or ``if(outgoing)'' which isn't any
easier to read..

> > +	status = getaddrinfo (node, /* service = */ NULL,
> > +			&ai_hints, &ai_list);
> 
> Why the seemingly random comment /* service = */?

Because, unless you know the `getaddrinfo' by heart you'll wonder what
that second pointer would have been. I'm therefore always naming
constant arguments where I can't use the variable name to document the
information passed to the function. Of course there are exceptions. If I
pass `0' as the second argument of `memset' for example ;)

Consider the following function:
  create_socket (sd, node, NULL, false, 0);
Can you guess what the parameters three through five do? Now consider
reading this:
  create_socket (sd, node, /* service = */ NULL,
    /* reverse lookup = */ false,
    /* flags = */ 0);
You still need to know what kind of reverse lookup the function is doing
or what flags it's accepting, but now it's *way* easier to see whether
the arguments may be related to the problem you're currently
investigating or not.

> Hm I see some functions in tinc still ints when they should return
> bools... I'll fix that :)

I have to admit I don't see the advantage of booleans over integers..
Maybe you can enlighten me?

Regards,
-octo
-- 
Florian octo Forster
Hacker in training
GnuPG: 0x91523C3D
http://verplant.org/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
Url : http://www.tinc-vpn.org/pipermail/tinc-devel/attachments/20090527/1d535a8d/attachment.pgp