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.

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

>>>> --- 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. How should header files generally be ordered in your opinion?

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

>>>> +        if (!found) {
>>>> +            HIP_ERROR("CHALLENGE_RESPONSE found, but no matching nonce\n");
>>>> +            return ignore_missing_challenge_response ? 1 : 0;
>>>> +        }
>>>> +
>>>> +        return found ? 1 : 0;
>>> 
>>> You just checked found, no need to do it again.
>> 
>> I guess that this has been done to ensure correct type conversion from
>> bool to int.
> 
> Read the code again, it's nonsense, "found" is initialized once and
> never set again.

You're right. I removed it and changed it to

>>>>  return ignore_missing_challenge_response ? 1 : 0;




--
Dipl.-Inform. Rene Hummen, Ph.D. Student
Chair of Communication and Distributed Systems
RWTH Aachen University, Germany
tel: +49 241 80 20772
web: http://www.comsys.rwth-aachen.de/team/rene-hummen/