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

On 25.10.2011, at 23:04, 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:
>
>>>>>> - 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.
>
> Nobody claims it has to be done as part of this merge request; it's
> easy enough to fix separately. If you are worried about the size of
> this merge request, that's all the more reason not to make unrelated
> changes to conntrack.c.

I still changed the return values of hip_fw_verify_and_store_host_id() because some calling functions actually expect non-zero values in case of an error. So I either need to change the caller or the callee. I'll directly apply this change to trunk though.

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