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

On 27.10.2011, at 00:53, Diego Biurrun wrote:
> On Wed, Oct 26, 2011 at 11:16:18PM +0200, Christof Mroz wrote:
>> On 26.10.2011 22:57, Diego Biurrun wrote:
>>> On Tue, Oct 25, 2011 at 11:43:43PM +0200, Christof Mroz wrote:
>>>> On 25.10.2011 23:02, Diego Biurrun wrote:
>>>>> 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.
>>>>
>>>> Including the .c file allows you to access static identifiers in
>>>> unit tests, but the libipq problem (see referenced thread) is a
>>>> different one.
>>>> Apparently, Hendrik already tried to mimic the other (working) tests
>>>> already without success.
>>>
>>> I'd like to know what is failing exactly and how before making
>>> a judgement here.
>>
>> See the referenced thread for details, but essentially we need to
>> mock ipq_get_packet() which is usually provided by linking -lipq.
>
> What I don't get is the reason why we need to mock that function
> in the first place. Probably I am missing something totally
> obvious.

We are calling fw_init_context() to derive a firewall context structure of an inbound HIP packet for unit-tests of midauth and rewrite functionality. fw_init_context() in turn calls ipq_get_packet(). The linker will complain about multiple definitions of ipq_get_packet() when compiling the unit tests with mocking of ipq_get_packet(). While this has not been a problem for other library functions so far, libipq behaves differently for some reason.

Anyway, I tried splitting up fw_init_context() into one function that calls ipq_get_packet() and another function that implements the setup of the firewall context struct - fw_setup_context(). This fixes the need for mocking ipq_get_packet(), so there should be no need for -z muldefs in the Makefile any more. However, now I am running in a new error:
/test/firewall/midauth.c and /test/firewall/rewrite.c both include firewall/firewall.c in order to use the new static function fw_setup_context() that sets up the firewall context struct. This makes the linker complain about multiple definitions again.

Diego, any suggestions? Otherwise, I suggest to add -z muldefs to the Makefile for the unit tests. Note that this step will not affect the compilation of the daemons, so I don't consider this a major problem.

Ciao,
René

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