Code review comment for lp:~midauth-pisa-devs/hipl/midauth-firewall

On Tue, Oct 25, 2011 at 02:35:31PM +0000, René Hummen wrote:
> On 25.10.2011, at 15:24, Diego Biurrun wrote:
> > On Tue, Oct 25, 2011 at 02:28:06PM +0200, René Hummen wrote:
> >> On 20.10.2011, at 14:15, Diego Biurrun wrote:
> >>> On Wed, Oct 19, 2011 at 01:17:30PM +0000, René Hummen wrote:
> >>>
> >>>> --- Makefile.am 2011-10-17 18:14:10 +0000
> >>>> +++ Makefile.am 2011-10-19 13:13:48 +0000
> >>>> @@ -253,7 +256,7 @@
> >>>>
> >>>> -test_check_firewall_LDFLAGS = -ldl
> >>>> +test_check_firewall_LDFLAGS = -ldl -z muldefs
> >>>
> >>> We have solved this differently elsewhere by #including the .c file to
> >>> unit-test static functions.
> >>
> >> This has been discussed previously on the list:
> >> http://www.freelists.org/post/hipl-dev/Mock-functions-for-hipl-unit-tests
> >
> > But we currently don't favor that solution. If that needs changing,
> > it's a topic for a separate merge request and would need to be changed
> > in all places.
>
> The change was necessary in order to comply with our unit-test for new
> code policy as unit-tests would otherwise not link correctly. The only
> other option I see right now is to merge the code without unit-tests.

No, the change is not necessary. As I said before, we have solved this
differently elsewhere by #including the .c file to unit-test static
functions. Changing this is outside the scope of this merge request
and it is not at all clear that a GNU-specific linker flag is the
better solution.

> >>>> - if (!hip_fw_verify_packet(common, tuple)) {
> >>>> + if (!hipfw_midauth_verify_challenge(ctx, other_dir->midauth_nonce)) {
> >>>> + HIP_ERROR("failed to verify midauth challenge\n");
> >>>> + return 0;
> >>>> + }
> >>>
> >>> 0 is returned in case of error?
> >>
> >> Yes, firewall/conntrack.c does not use the same semantics for return
> >> values compared to the rest of the code.
> >
> > Then fix conntrack.c and don't make the code consistently inconsistent,
> > cf. the boyscout rule.
>
> This would further increase the (already huge) merge request. It's
> definitely out of focus of this branch.

Nobody claims it has to be done as part of this merge request; it's
easy enough to fix separately. If you are worried about the size of
this merge request, that's all the more reason not to make unrelated
changes to conntrack.c.

> >>>> --- firewall/firewall.c 2011-08-16 07:49:25 +0000
> >>>> +++ firewall/firewall.c 2011-10-19 13:13:48 +0000
> >>>> @@ -76,12 +76,13 @@
> >>>> #include "lib/core/prefix.h"
> >>>> #include "lib/core/util.h"
> >>>> #include "hipd/hipd.h"
> >>>> -#include "config.h"
> >>>> #include "cache.h"
> >>>> #include "common_types.h"
> >>>> +#include "config.h"
> >>>> #include "conntrack.h"
> >>>> #include "esp_prot_api.h"
> >>>> #include "esp_prot_conntrack.h"
> >>>> +#include "firewall.h"
> >>>> #include "firewall_control.h"
> >>>> #include "firewall_defines.h"
> >>>> #include "helpers.h"
> >>>> @@ -90,9 +91,9 @@
> >>>> #include "pisa.h"
> >>>> #include "port_bindings.h"
> >>>> #include "reinject.h"
> >>>> +#include "rewrite.h"
> >>>> #include "rule_management.h"
> >>>> #include "user_ipsec_api.h"
> >>>> -#include "firewall.h"
> >>>
> >>> The ordering is actually done on purpose. config.h is not in the same
> >>> directory, same as the other #includes with directory prefixes and
> >>> firewall.h directly "belongs" to firewall.c, so it made sense at the
> >>> time to put it last and list all dependencies before. I have this
> >>> feeling in the back of my head that the latter solved some real issue.
> >>
> >> 'make alltests' runs successfully. So, what exactly is your suggestion?
> >
> > My suggestion is not to change the #include ordering.
>
> If the order of the includes is relevant, then there probably is some
> other problem in the header files. Everything is compiling nicely now.
> So I don't know what kind of issue you are talking about.

The issue is a completely unrelated and pointless change as part of this
merge request that makes things inconsistent.

> How should header files generally be ordered in your opinion?

system headers in alphabetical order
empty line
local headers from other directories in alphabetical order
local headers from same directory in alphabetical order
header with the same basename as the .c file

> >>>> @@ -74,6 +81,8 @@
> >>>> int modified;
> >>>> };
> >>>>
> >>>> +#include "midauth.h"
> >>>
> >>> Why the peculiar placement of this #include?
> >>
> >> There is a cyclic redundancy between firewall/midauth.h (requires
> >> definition of struct hip_fw_context) and firewall/firewall_defines.h
> >> (requires definition of MIDAUTH_DEFAULT_NONCE_LENGTH). This helps
> >> mitigating the problem.
> >
> > So fix the cyclic redundancy, don't add more workarounds.
>
> I already tried moving the struct definition to firewall.h, but that
> only moved the problem somewhere else. The only solution I can see
> right now would be a file like firewall_defines2.h where I could move
> the struct. However, then I prefer the way it is implemented right
> now.

What needs to be moved is the #define. There is no inherent back and
forth dependency, so this can definitely be fixed.

Diego