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

Hi Christoph,

sorry for the late response.

On 01/16/2012 09:28 AM, Christoph Viethen wrote:
> 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?

Sure, the more unified and modularized, the better. Obviously, this
depends on how much time you have. I was suggesting to develop this
"half way" - if you want to be more ambiguous, perhaps consider
developing some of the stuff on another branch,

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

 From the view point of a purely control plane registration, only one
source HIT is needed. We could add all and they'd be expired at some
point, but it seems unnecessary. I think the opportunistic mode
registration should allow specifying of the source HIT optionally, though.

For data plane base exchange, the whole story starts with the DNS proxy
that receives a request from the application to map a host name into an
address. the DNS proxy does the look up, communicates the mapping from
HIT to the IP address to the HIP daemon and returns the HIT to the
application. HIP daemon creates four host associations it doesn't know
what ends up being the source HIT for the application.

So, from the view point of unification, the same function,
hip_hadb_add_peer_info(), could be used. However, I think this could be
determined automatically by the absence (NULL) of source HIT argument to
the unified function.

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

I believe it was an optimization. It is possible that hipd is triggered
simultaneously by two applications and we can reuse the HADB state. If a
valid HADB entry was found, the first goto that checks the value skips
the determination of the remote address. It is not always the case that
you have DNS proxy running (it's optional!) and we may have toe resort
to hosts files, previous HADB entries or even the hit-to-ip extension
(that can map flat HITs into IP addresses from your "home" domain).