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

[Guus Sliepen]

On Wed, May 27, 2009 at 02:18:35PM +0200, Florian Forster 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.

I do not suggest removing the check. But assert() just calls abort() if the
check fails, so in effect the tincd process is killed anyway, just not by a
segmentation fault. What I suggest is to replace assert(foo) with if(!foo)
return false.

> > 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

Ah ok. But vim supports folding by indentation and by syntax, is there an
advantage to manually adding markers?

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

Ok.

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

Ok, that might be true.

> > 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?

It conveys the meaning much better. False = negative = failure, true = positive
= success. While for ints, the meaning of 0 is less clear. Yes, a lot of system
calls return 0 in case of success, but there are also many calls that return 0
in case of failure. Theoretically, the compiler could also be strict in what
you assign to bools and what you can do with them, which would help catch
mistakes, but unfortunately the C compiler treats them mostly like ints.

Anyway, I'm not religious about coding style, but it helps if the style is
consistent throughout the whole code base. And if we decide something about
the style needs changing, it's better to refactor all the code in one go. Your
use of asserts and markers look very out of place in the rest of the code. I
also don't mind cleaning up afterwards, but I'd rather apply patches without
changing them :)

-- 
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/5559c262/attachment.pgp