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

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/