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

On Tue, Nov 08, 2011 at 06:49:23PM +0000, 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.
>
> 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.

I'm out of better ideas, so go down whatever route you prefer.

Diego