Code review comment for lp://staging/~cviethen/hipl/pisa-pairing-tng

Hi Miika,

you wrote:

> While the code base your suggesting isn't huge, it duplicates the client-side registration code (hipconf add server) that can also trigger opportunistic (and normal) BEX. I would suggest to aim for code reuse especially for the hipd part to have a single point triggering base exchange from hipconf (because this eases code maintenance). More specifically:
>
> * You can have a separate hipconf type (if you will) for triggering a BEX but it should support normal BEX as well. Consider reusing the parameter parsing in hipconf to accomodate normal vs opportunistic BEX, however.
> * The hipd code in hip_handle_req_user_msg() should be shared (it's called from user.c).
> * A opp/normal BEX should be a special case of "none" registration, i.e., no service parameters are included

Do you think it might be useful to aim at unifying all "trigger a base exchange"-functionality that exists in the backend into one place? I mean, looking at hip_handle_req_user_msg() and netdev_trigger_bex() and how they both are meant to trigger base exchanges: Maybe it's useful to chop them up into somewhat smaller entities - they are quite long anyway -, and remove redundancies as far as reasonable?

It seems not so good to have two functions that can trigger base exchanges ...

Thinking about that, there are a few things that those functions do differently - without reasons obvious to me.

So please let me ask:

1) hip_handle_req_user_msg() makes the following distinction between the modes: in opp. mode, it uses hip_hadb_add_peer_info_complete() in order to add the entry to the HADB, which causes one entry for (default_hit, opp_hit) (as source and destination HIT) to be created. In non-opp. mode, it uses hip_hadb_add_peer_info(), which uses hip_for_each_hi() to add (potentially) a number of entries, one for each local HI, to the HADB. If the host has only one Host Identifier, that's no difference, obviously - in case we have several different host identifiers, though, it is.

Is this distinction intentional? If so, why? If not, which one would be "the right" way of doing it?

2) in netdev_trigger_bex(), I'm unsure about what "reuse_hadb_local_address" is supposed to achieve. My impression is (but I might be mistaken) that it will generally have been set to 1 before execution goes beyond the "send_i1" label - and the value of that variable only gets evaluated in one place, and there it will always have the value 1. I guess that variable (and any functionality attached to it) can be removed?

Those two for now - more as they crop up. :)

Thanks,

  Christoph

--
 <email address hidden>