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

Hi,

On 12/11/11 00:09, Miika Komu wrote:
> Hi,
>
> On 08/11/11 20:49, René Hummen wrote:
>> 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.
>
> it appears that this -z muldefs option has broken the ARM cross
> compilation in revision 6122. Help?

let's continue the discussion on a separate bug report:

https://bugs.launchpad.net/hipl/+bug/889495