[hipl-dev] Re: [Merge] lp:~diego-biurrun/hipl/hipfw-performance into lp:hipl
- From: Stefan Götz <stefan.goetz@xxxxxxxxxxxxxxxxx>
- To: mp+43568@xxxxxxxxxxxxxxxxxx
- Date: Tue, 11 Jan 2011 08:52:57 -0000
>>> @@ -116,7 +149,7 @@
>>> *
>>> * @param hiptuple HIP tuple
>>> */
>>> -static void print_tuple(const struct hip_tuple *hiptuple)
>>> +static void print_tuple(DBG const struct hip_tuple *hiptuple)
>>
>> [L] style: const correctness for pointer arguments. May apply to other
>> functions. Please check.
>
> Is this a policy already?
According to docs/HACKING, it is. So far, I've been giving const correctness
with pointers only an [L] because I thought it wasn't covered by the policy,
but after reading it again, I think it is, so I'll upgrade this to an [M] :)
> And if so, does it apply to pointers only or
> everything?
To everything, according to docs/HACKING
> I mean, the difference between
>
> const blah *ptr versus const blah *const ptr
>
> is the same as in
>
> int x versus const int x,
>
> right?
Yes, absolutely.
>>> + * @todo Test rules using userspace_ipsec, Relay, LSI, sys-opp, midauth,
>>> + * light-update and esp_prot configurations.
>>> + * @todo Test with different byte ordering.
>>
>> [L] I don't like TODOs in trunk. Especially if they effectively say that the
>> code has poor quality due to lazyness ;-)
>
> Don't worry, I explicitly stated not to merge unless these features are
> confirmed to survive the changes (or not) in the original proposal, but my
> HIPL
> knowledge is probably not good enough to thoroughly test these features myself
> yet. René expected no complications, but that doesn't warrant removing this
> TODO
> yet I think. Anyway, have to merge with trunk first...
Hm, but doesn't that mean that this branch is not ready for a merge proposal,
anyway?
> Also, explicitly stating things that might get broke could be interpreted as
> careful foresight and investigation, rather than lazyness ^^
Yupp, I agree, I'm all for TODOs in source code. But a merge proposal into
trunk effectively says 'This code is of the highest quality and ready for prime
time' - and this specific TODO contradicts that notion (because it's not a
'nice feature that could be added' TODO but a 'this stuff is not properly
tested' TODO).
>>> + if (esp_tuple->esp_prot_tfm > ESP_PROT_TFM_UNUSED) {
>>> + HIP_DEBUG("ESP Transforms requested; not handled via iptables "
>>> + "since we need to inspect packets\n");
>>> + return 0;
>>> + }
>>> +
>>> + if (esp_tuple->tuple->esp_relay) {
>>> + HIP_DEBUG("ESP Relay requested; not handled via iptables "
>>> + "since we need packet rewriting\n");
>>> + return 0;
>>> + }
>>
>> [L] design/style: It seems strange to me that 'success' is returned when the
>> function didn't actually complete successfully. It might make sense to have a
>> return value that says 'you called this function under conditions with which
>> a
>> call does not make sense'.
>
> It's expected behaviour, and because the return value is not checked anyway it
> probably doesn't warrant multiple error levels.
What?!? the return value is not even checked? OMG. You're so not getting an
approve on this :-)
[M]: correctness/style: check return value of hip_fw_manage_esp_rule()
>>> + iter_conn = conn_list;
>>> + while (iter_conn) {
>>> + struct connection *conn = (struct connection*) iter_conn->data;
>>
>> [M] style: could the cast be avoided?
>> [L] style: remove cast if possible
>
> Currently no, because containers are of the home-brew void* based kind right
> now.
Exactly. Precisely *because* the 'data' member is of type void*, you can assign
its value to the variable 'conn' without a cast.
[M] style: remove pointless cast
>>> === modified file 'hipd/hiprelay.c'
>>> --- hipd/hiprelay.c 2010-11-30 14:50:30 +0000
>>> +++ hipd/hiprelay.c 2010-12-13 21:28:35 +0000
>>> @@ -1015,16 +1015,16 @@
>>> *
>>> * @param r the HIP control message to be relayed
>>> * @param type_hdr message type
>>> - * @param r_saddr the original source address
>>> - * @param r_daddr the original destination address
>>> + * @param r_saddr (unused) the original source address
>>> + * @param r_daddr (unused) the original destination address
>>
>> [M] what is the point of adding unused function arguments to an already
>> crowded signature?
>
> For debug purposes. Touching the function body would be out of the scope of
> this
> branch (even this doc change was, sorry).
I'm not quite following. So if they are necessary for debug purposes, why are
they unused? Aren't they 'used for debugging'?
> You forgot one [H]: missing unit tests... was looking forward to do that too.
You're right, I'm getting sloppy :)
--
https://code.launchpad.net/~diego-biurrun/hipl/hipfw-performance/+merge/43568
Your team HIPL core team is requested to review the proposed merge of
lp:~diego-biurrun/hipl/hipfw-performance into lp:hipl.
Other related posts:
