obfsproxy: Please review portability fixes

As a way of getting familiar with the code, I've done a bunch of portability fixes and cleanups to obfsproxy. Please review https://github.com/zackw/obfsproxy-port-fixes/compare/master...port-fixes and pull if you like 'em.

added component::archived/obfsproxy owner::asn priority::medium resolution::fixed status::closed type::enhancement labels

Trac:
Status: new to needs_review

I'm not one to review portability fixes; my automake/portability skills are non-existent, so I just read the commits that I have some experience with.

I checked out "Avoid embedding struct sockaddr in protocol_params_t." which is a thing I wanted to do for a while and it seems okay. When my function documentation branch (#3301 (closed)) gets merged, we should mention that addr_out should be freed on resolve_address_port()'s documentation.

I checked out "Clean up the logging API." which also seems okay in my book, apart from the fact that the function documentation is now outdated, left with the "-1/1" return convention.

I also think that "Move obfs2_crypt.[ch] to top level and rename them crypt.[ch]." is a good idea (at least till a better idea comes up).

I renamed the github repository after realizing that it was silly to create N of them for every patch series I generate. The review URL is now https://github.com/zackw/obfsproxy/compare/master...port-fixes

Looking through this now.

BTW, a request for future patches: please have each commit do only one thing. In particular, it's okay to fix formatting and whitespace, but please do it as a separate commit so that it's easier to review the real changes.

Merged, with many conflicts.

Please have a close look through c37354b1e0adcb6d16929613e3bd4a509d487d2d to make sure I didn't screw anything up.

Trac:
Resolution: N/A to fixed
Status: needs_review to closed

I read carefully through both sides of the merge and didn't see any problems. Also, I've verified that the result still builds and passes unit tests on Linux and Windows.

I did notice a new portability problem introduced on the other side of the merge (specifically, with the change that introduced doubly linked lists).

{{{#define OFFSETOF(container_type, element)
(((char*)&((container_type*)0)->element) - ((char*) ((container_type*)0)))}}}

This construct provokes undefined behavior, as it dereferences the null pointer (yes, even though that dereference is inside the argument of the address-of operator). Sufficiently aggressive C optimizers can and will break it. (No version of GCC that I have convenient access to actually does break it, but I recall its fragility being discussed on the GCC mailing lists, and I wouldn't be surprised if clang had a problem with it.) It's also completely unnecessary. C89 provides offsetof, with identical effect, in <stddef.h>; the last environment I'm aware of that didn't have <stddef.h> was the K&R-only compiler bundled with some versions of Solaris -- but obfsproxy won't compile with that antique anyway.

I'll send in another patch to get rid of it.

Trac:
Component: Pluggable transport to Obfsproxy

closed