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

[Guus Sliepen]

On Wed, May 27, 2009 at 09:49:11AM +0200, Florian Forster wrote:

> This patch adds support for the `BindToInterface' and `BindToAddress' options
> to the setup of outgoing TCP connections.

Thanks! But some comments about your patch:

> +#include <assert.h>

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.

> +static int bind_to_interface (int sd) /* {{{ */
[...]
> +} /* }}} int bind_to_interface */

Please keep the coding style consistent with what is already in tinc. Is this
/* {{{ */ ... /* }}} repeated declaration */ thing something that is used in
some IDE or is it your own style? Also, braces after if, for, etc. on the same
line instead of a separate line, if (foo != 0) should be if(foo), no spaces
between function names the opening parenthesis.

> +		return (0);
> +		return -1;
> +	return (0);

If you want to have return value indicating success or failure, please use bool
as the return type, and return true in case of success and false in case of
failure.

> +static int bind_to_address (connection_t *c) /* {{{ */
[...]
> +	/* We're called from `do_outgoing_connection' only. */
> +	ai_hints.ai_socktype = SOCK_STREAM;
> +	ai_hints.ai_protocol = IPPROTO_TCP;

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().

> +	status = getaddrinfo (node, /* service = */ NULL,
> +			&ai_hints, &ai_list);

Why the seemingly random comment /* service = */?

> +		logger (LOG_DEBUG, "Successfully bound outgoing "
> +				"TCP socket to %s", node);

That should be prefixed with ifdebug(CONNECTIONS).

> -				return -1;

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

Could you address the issues mentioned above and try to follow the coding style
of the rest of tinc?  If so I can apply your patch as-is directly to the git
repository as I did with the one fixing the segfault in linux/device.c, but
otherwise I'll rewrite your patch.

-- 
Met vriendelijke groet / with kind regards,
     Guus Sliepen <guus at tinc-vpn.org>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: Digital signature
Url : http://www.tinc-vpn.org/pipermail/tinc-devel/attachments/20090527/59e50a49/attachment.pgp