Merge lp://staging/~cviethen/hipl/pisa-pairing-tng into lp://staging/hipl

Good work concerning functionality! Some remarks below.

On 08.01.2012 18:55, Christoph Viethen wrote:
> === modified file 'hipd/hadb.c'
> --- hipd/hadb.c 2011-12-30 23:20:44 +0000
> +++ hipd/hadb.c 2012-01-08 17:54:25 +0000
> @@ -1400,6 +1400,45 @@
> }
>
> /**
> + * This function searches for a<tt> struct hip_hadb_state</tt> entry from the
> + * @c hadb_hit by the given tuple (source hit, dest. ip, dest. port).

Use ::hip_hadb_state and ::hadb_hit rather than syntactic markup, so
that doxygen can generate cross-references in html output.

> +struct hip_hadb_state *hip_hadb_find_by_srchit_destip_port(hip_hit_t *src_hit,
> + const struct in6_addr *dst_ip,
> + in_port_t dst_port)

src_hit can be const.

> +{
> + LHASH_NODE *item, *aux;
> + struct hip_hadb_state *tmp;
> + int i;
> +
> + list_for_each_safe(item, aux, hadb_hit, i)
> + {
> + tmp = list_entry(item);
> + if (ipv6_addr_cmp(&tmp->hit_our, src_hit)) {
> + continue;
> + } else if (!ipv6_addr_cmp(&tmp->peer_addr, dst_ip)) {
> + if (tmp->peer_udp_port == dst_port) {
> + return tmp;
> + } else {
> + continue;
> + }
> + } else {
> + continue;
> + }

Would the following do the trick?

list_for_each_safe(item, aux, hadb_hit, i)
{
     tmp = list_entry(item);

     const bool ok = !ipv6_addr_cmp(&tmp->hit_our, src_hit) &&
                     !ipv6_addr_cmp(&tmp->peer_addr, dst_ip) &&
                      tmp->peer_udp_port == dst_port;
     if(ok) {
         return tmp;
     }
}

Extra bool indirection because our policy of keeping the opening brace
on the same line as the closing paren makes multi-line conditions very
hard to comprehend IMHO, but feel free to inline if you're fine with it.

> === modified file 'hipd/hadb.h'
> --- hipd/hadb.h 2011-11-25 17:56:24 +0000
> +++ hipd/hadb.h 2012-01-08 17:54:25 +0000
> @@ -1094,6 +1093,56 @@
> + /* Find out: 1. our own hit, */
> + if (hip_get_default_hit(&our_hit)< 0) {
> + return -1;
> + }

You should run uncrustify again.

> @@ -2424,6 +2433,111 @@
> }
>
> /**
> + * Handles the<tt> hipconf daemon trigger bex</tt> command.

Same as above

> + if (ai_err == 0) {
> + int done = 0;
> + const struct addrinfo *ai_res = addrinfo_result;
> +
> + while (done == 0&& ai_res != NULL) {
> + if (ai_res->ai_addr->sa_family == AF_INET) {
> + /* turn address into an IPv4-mapped IPv6 address
> + * (RFC 4291, 2.5.5.2.) */
> + IPV4_TO_IPV6_MAP(&((struct sockaddr_in *) ai_res->ai_addr)->sin_addr,&ipv6_addr);
> + done = 1;
> + }
> + if (ai_res->ai_addr->sa_family == AF_INET6) {
> + memcpy(&ipv6_addr,&((struct sockaddr_in6 *) ai_res->ai_addr)->sin6_addr, sizeof(struct in6_addr));
> + done = 1;
> + }

These lines can be shortened without much hassle. Also, are you sure
t...

 review needs-fixing

On Sun, Jan 08, 2012 at 05:55:27PM +0000, Christoph Viethen wrote:
> Christoph Viethen has proposed merging lp:~cviethen/hipl/pisa-pairing-tng into lp:hipl.

Just a couple of nits, I don't know that part of the code well.

> --- hipd/hadb.c 2011-12-30 23:20:44 +0000
> +++ hipd/hadb.c 2012-01-08 17:54:25 +0000
> @@ -1400,6 +1400,45 @@
> }
>
> /**
> + * This function searches for a <tt> struct hip_hadb_state </tt> entry from the
> + * @c hadb_hit by the given tuple (source hit, dest. ip, dest. port).

This sentence explains why starting a sentence with "this sentence" is
silly; same for "this function" in Doxygen function documentation.

> --- hipd/hadb.h 2011-11-25 17:56:24 +0000
> +++ hipd/hadb.h 2012-01-08 17:54:25 +0000
> @@ -115,6 +115,8 @@
>
> +struct hip_hadb_state *hip_hadb_find_by_srchit_destip_port(hip_hit_t *src_hit, const struct in6_addr *dst_ip, in_port_t dst_port);
> +

Break this long line.

> --- hipd/netdev.c 2011-12-30 23:20:44 +0000
> +++ hipd/netdev.c 2012-01-08 17:54:25 +0000
> @@ -1094,6 +1093,56 @@
>
> /**
> + * Gets called in case of an intended opportunistic base exchange - checks
> + * first whether a host association is already established, and won't perform
> + * the base exchange if so.
> + *
> + * If no host association exists, proceeds to pass everything through to
> + * hip_netdev_trigger_bex_msg().

Please start by explaining what the function does, not why and avoid the
third person singular.

> + * @param msg the message from hipconf
> + * @return zero on success and non-zero on error

It returns -1.

> +int hip_netdev_trigger_opp_bex_msg(const struct hip_common *const msg)
> +{
> + hip_hit_t our_hit;
> +
> + /* Find out: 1. our own hit, */
> + if (hip_get_default_hit(&our_hit) < 0) {
> + return -1;
> + }
> +
> + /* 2. the IP address the base exchange should be sent to, */
> + const struct hip_tlv_common *const param = hip_get_param(msg, HIP_PARAM_IPV6_ADDR);
> +
> + if (!param) {
> + return -1;
> + }

These two conditions could be merged.

> + /* and 3. the UDP port number base exchanges will be sent to. */
> + const in_port_t ha_peer_port = (hip_nat_status ? hip_get_peer_nat_udp_port() : 0);

pointless parentheses

> + if (entry != NULL) {

if (entry) {

> --- lib/core/conf.c 2012-01-05 15:56:43 +0000
> +++ lib/core/conf.c 2012-01-08 17:54:25 +0000
> @@ -2424,6 +2433,111 @@
>
> /**
> + * Handles the <tt> hipconf daemon trigger bex </tt> command.

Handle ..

> + * @param msg a pointer to the buffer where the message for hipd will
> + * be written.
> + * @param action numeric action identifier (ACTION_TRIGGER is expected).
> + * @param opt an array of pointers to the command line arguments after
> + * the action and type.

We don't generally use that extra space indentation.

> + * @param optc the number of elements in the opt array (@b 1).
> + * @param send_only currently unused
> + * @return zero on success, or -1 on error.
> + */
> +static int conf_handle_trigger_bex(struct hip_common *const msg,
> + const int act...

Hello,

on Jan 8, 2012, at 7:43 PM, Christof Mroz wrote:

>> + * This function searches for a<tt> struct hip_hadb_state</tt> entry from the
>> + * @c hadb_hit by the given tuple (source hit, dest. ip, dest. port).
>
> Use ::hip_hadb_state and ::hadb_hit rather than syntactic markup, so
> that doxygen can generate cross-references in html output.

Many thanks for the Doxygen advice! Not my greatest area of expertise, really.

Would it make sense to add the "::" and, additionally, keep the syntactic markup? Especially in the case of "struct hip_hadb_state" the keyword "struct" looks a little "alone" if it's kept complete unformatted.

I'll put both in right now - maybe you could take a look again whether that's okay or a bit too much?

> src_hit can be const.

And I found a few more, still. :-)

> Would the following do the trick?
>
> list_for_each_safe(item, aux, hadb_hit, i)
> {
> tmp = list_entry(item);
>
> [...]

Thanks, good idea - that happens when one just lazily copies and slightly modifies somebody else's code fragment. :)

>> @@ -1094,6 +1093,56 @@
>> + /* Find out: 1. our own hit, */
>> + if (hip_get_default_hit(&our_hit)< 0) {
>> + return -1;
>> + }
>
> You should run uncrustify again.

Did that, didn't change. :-) Though I must say that "my" code looks a bit different from the one you're quoting (e.g., there's a space between ")" and "<"). No idea what happened. E-mail software acting up?

>> + * Handles the<tt> hipconf daemon trigger bex</tt> command.
>
> Same as above

You mean the "::" part, or the "run uncrustify again" part? I'll just leave it for the moment - it's meant as a command line, so "::" at least doesn't make sense from my (limited) point of view.

>> + IPV4_TO_IPV6_MAP(&((struct sockaddr_in *) ai_res->ai_addr)->sin_addr,&ipv6_addr);
>> + done = 1;
>> + }
>> + if (ai_res->ai_addr->sa_family == AF_INET6) {
>> + memcpy(&ipv6_addr,&((struct sockaddr_in6 *) ai_res->ai_addr)->sin6_addr, sizeof(struct in6_addr));
>> + done = 1;
>> + }
>
> These lines can be shortened without much hassle. Also, are you sure
> there is no utilty function for copying IPV6 addresses yet?

Good idea - there's ipv6_addr_copy() of course. Which makes shortening the whole thing easier, as well. Please let me know whether my shortening / wrapping strategy looks good. Seems a little awkward to me, but let's see.

>> + if (addrinfo_result != NULL) {
>> + freeaddrinfo(addrinfo_result);
>> + }
>
> I'd free it even if ai_err wasn't 0, just to be sure.

I'll be naive and trust the POSIX example code and the Linux man page (which at least suggests one only needs to look after that when getaddrinfo() was successful). :-)

>> + if (done == 0) {
>> + HIP_ERROR("Failed to find any IP address for hostname.");
>> + return -1;
>> + }
>
> This while loop looks like it can be simplified:
>
> for(ai_res = addrinfo_result; ai_res != NULL; ai_res = ai_res->next) {
> if(...) { copy; break; }
> if(...) { copy; break; }
> }
>
> if(!ai_res) { error; }

Not sure - right now...

Hello,

on Jan 9, 2012, at 12:16 AM, Diego Biurrun wrote:

> Just a couple of nits,

Admit it: you love it! ;-)

> I don't know that part of the code well.

Welcome to the club. :-)

> This sentence explains why starting a sentence with "this sentence" is
> silly; same for "this function" in Doxygen function documentation.

Ack!

>> +struct hip_hadb_state *hip_hadb_find_by_srchit_destip_port(hip_hit_t *src_hit, const struct in6_addr *dst_ip, in_port_t dst_port);
>> +
>
> Break this long line.

Oh yes. It got a bit worse still in the meantime, so I went a little radical now when it came to breaking it.

Ack!

>> + * Gets called in case of an intended opportunistic base exchange - checks
>> + * first whether a host association is already established, and won't perform
>> + * the base exchange if so. [...]
>
> Please start by explaining what the function does, not why and avoid the
> third person singular.

Ack!

>> + * @param msg the message from hipconf
>> + * @return zero on success and non-zero on error
>
> It returns -1.

You may be right. But then again, I tend to trust the documentation of the functions that I'm calling. And there it says the same. Hmmm. :)

>> +int hip_netdev_trigger_opp_bex_msg(const struct hip_common *const msg)

>> + /* Find out: 1. our own hit, */

>> + /* 2. the IP address the base exchange should be sent to, */

> These two conditions could be merged.

Ah, good catch. Ack.

>> + const in_port_t ha_peer_port = (hip_nat_status ? hip_get_peer_nat_udp_port() : 0);
>
> pointless parentheses

Ack!

>> + if (entry != NULL) {
>
> if (entry) {

Ack!

>> + * Handles the <tt> hipconf daemon trigger bex </tt> command.
>
> Handle ..

Ack!

>> + * the action and type.
>
> We don't generally use that extra space indentation.

Okay, I'll try to do without it in the function headers then. Let me know please if you spot more of that kind that I may have overlooked.

>> +static int conf_handle_trigger_bex(struct hip_common *const msg,
>> + const int action,
>> + const char *opt[],
>> + const int optc,
>> + UNUSED int send_only)
>
> Ugh..

Please elaborate - are you referring more to the formatting, or to the multitude of consts, or ... ?

>> + if (action != ACTION_TRIGGER) {
>> + return -1;
>> + }
>> +
>> + if (optc > 1) {
>> + HIP_ERROR("Too many arguments for trigger bex command.\n");
>> + return -1;
>> + }
>
> Could be merged.

Umm, I'm a little slow right now, maybe. I only want that HIP_ERROR() output in one of the two cases.

>> + if (ai_err == 0) {
>
> maybe
>
> if (!ai_err) {

Looks okay to me. Ack, also to the other, similar instances. I basically like more explicit comparisons, but as far as functionality is concerned, it doesn't matter, and as long as it doesn't lead to bad obfuscation, I don't mind really.

>> + const int uh_err = hip_build_user_hdr(msg, HIP_MSG_TRIGGER_OPP_BEX, 0);
>> +
>> + if (uh_err) {
>> + HIP_ERROR("Failed to build us...

On Mon, Jan 09, 2012 at 02:55:27AM +0000, Christoph Viethen wrote:
> on Jan 9, 2012, at 12:16 AM, Diego Biurrun wrote:
> >> +static int conf_handle_trigger_bex(struct hip_common *const msg,
> >> + const int action,
> >> + const char *opt[],
> >> + const int optc,
> >> + UNUSED int send_only)
> >
> > Ugh..
>
> Please elaborate - are you referring more to the formatting, or to the
> multitude of consts, or ... ?

The formatting, which suggests your uncrustify hook is not working..

> >> + if (action != ACTION_TRIGGER) {
> >> + return -1;
> >> + }
> >> +
> >> + if (optc > 1) {
> >> + HIP_ERROR("Too many arguments for trigger bex command.\n");
> >> + return -1;
> >> + }
> >
> > Could be merged.
>
> Umm, I'm a little slow right now, maybe. I only want that HIP_ERROR()
> output in one of the two cases.

Of course, you are right ..

> >> + const int uh_err = hip_build_user_hdr(msg, HIP_MSG_TRIGGER_OPP_BEX, 0);
> >> +
> >> + if (uh_err) {
> >> + HIP_ERROR("Failed to build user message header: %s\n", strerror(uh_err));
> >> + return -1;
> >> + }
>
> > The variable indirection with uh_err looks inconsistent to me, you don't
> > do it in the other cases.
>
> I guess I was keen to keep things as const correct as possible. One
> might argue, of course, that over such a small stretch of code,
> nothing bad can happen to uh_err, since one can see with a blink of
> the eye that, past initialization, it's not written to any more.

Uh, that variable *is* used in the HIP_ERROR call - I should not
review when I'm tired...

> Many thanks for your thorough review!

It was rather shallow, thankfully Christof reviewed as well :)

Diego

At the minimum, the code should be able to trigger a normal base exchange as well with HIT as an optional argument in hipconf. I think the hipconf command was also missing from hipconf_usage and I suggest to add it to hipd/hipd.conf as a comment.

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

Indepedently of this, there is a workaround in hip_handle_req_user_msg() to call hip_hadb_find_rvs_candidate_entry(). This could be replaced with you new function hip_hadb_find_by_srchit_destip_port().

On 09.01.2012 03:43, Christoph Viethen wrote:
> Hello,
>
> on Jan 8, 2012, at 7:43 PM, Christof Mroz wrote:
>
>>> + * This function searches for a<tt> struct hip_hadb_state</tt> entry from the
>>> + * @c hadb_hit by the given tuple (source hit, dest. ip, dest. port).
>>
>> Use ::hip_hadb_state and ::hadb_hit rather than syntactic markup, so
>> that doxygen can generate cross-references in html output.
>
> Many thanks for the Doxygen advice! Not my greatest area of expertise, really.
>
> Would it make sense to add the "::" and, additionally, keep the syntactic markup? Especially in the case of "struct hip_hadb_state" the keyword "struct" looks a little "alone" if it's kept complete unformatted.

I'd just drop the "struct" altogether, because for high-level
comprehension it doesn't matter whether it's a struct or maybe a typedef
or whatever. When actually calling the function you need to look at the
prototype anyway, and will spot the "struct" qualifier.

> I'll put both in right now - maybe you could take a look again whether that's okay or a bit too much?

Sure, unless the function is now obsoleted by Miika's advice.

>>> @@ -1094,6 +1093,56 @@
>>> + /* Find out: 1. our own hit, */
>>> + if (hip_get_default_hit(&our_hit)< 0) {
>>> + return -1;
>>> + }
>>
>> You should run uncrustify again.
>
> Did that, didn't change. :-) Though I must say that "my" code looks a bit different from the one you're quoting (e.g., there's a space between ")" and "<"). No idea what happened. E-mail software acting up?

You're right there, interesting... good to know that my user-agent sucks :)

>>> + * Handles the<tt> hipconf daemon trigger bex</tt> command.
>>
>> Same as above
>
> You mean the "::" part, or the "run uncrustify again" part? I'll just leave it for the moment - it's meant as a command line, so "::" at least doesn't make sense from my (limited) point of view.

Agreed, :: doesn't make sense here.

>>> + IPV4_TO_IPV6_MAP(&((struct sockaddr_in *) ai_res->ai_addr)->sin_addr,&ipv6_addr);
>>> + done = 1;
>>> + }
>>> + if (ai_res->ai_addr->sa_family == AF_INET6) {
>>> + memcpy(&ipv6_addr,&((struct sockaddr_in6 *) ai_res->ai_addr)->sin6_addr, sizeof(struct in6_addr));
>>> + done = 1;
>>> + }
>>
>> These lines can be shortened without much hassle. Also, are you sure
>> there is no utilty function for copying IPV6 addresses yet?
>
> Good idea - there's ipv6_addr_copy() of course. Which makes shortening the whole thing easier, as well. Please let me know whether my shortening / wrapping strategy looks good. Seems a little awkward to me, but let's see.

OK, take a look at HACKING when in doubt.

>>> + if (done == 0) {
>>> + HIP_ERROR("Failed to find any IP address for hostname.");
>>> + return -1;
>>> + }
>>
>> This while loop looks like it can be simplified:
>>
>> for(ai_res = addrinfo_result; ai_res != NULL; ai_res = ai_res->next) {
>> if(...) { copy; break; }
>> if(...) { copy; break; }
>> }
>>
>> if(!ai_res) { error; }
>
> Not sure - right now I feel I should leave it the way it is. I'd like t...

> Christoph Viethen has proposed merging lp:~cviethen/hipl/pisa-pairing-tng into lp:hipl.
Useful functionality, nice const correctness & docs, btw.

What happened to the unit test requirement? Does it still exist? Just
asking because there are no unit tests for the newly introduced functions...

> /**
> + * This function searches for a <tt> struct hip_hadb_state </tt> entry from the
> + * @c hadb_hit by the given tuple (source hit, dest. ip, dest. port).
> + *
> + * This is useful to find a host association when the opposite HIT is not
> + * (yet) known, in particular in the opportunistic base exchange case.
> + *
> + * @param src_hit pointer to source HIT
> + * @param dst_ip pointer to destination IP address
> + * @param dst_port destination UDP port number (host byte order) (may be 0)
> + * @return a pointer to a matching host association, or NULL if
> + * a matching host association was not found.
> + */
> +struct hip_hadb_state *hip_hadb_find_by_srchit_destip_port(hip_hit_t *src_hit,
> + const struct in6_addr *dst_ip,
> + in_port_t dst_port)

documentation: how does the function behave when being passed NULL
pointers? Since the resulting hip_hadb_state object is returned by
reference, what about memory management? Does the caller need to
de-allocate this object or is he allowed to? Is the caller free to
modify this object?

robustness: as many objects as possible, including in6_addr and HIT
objects, should be passed by value to avoid memory management issues.

> +{
> + LHASH_NODE *item, *aux;
> + struct hip_hadb_state *tmp;
> + int i;
> +
> + list_for_each_safe(item, aux, hadb_hit, i)
> + {
> + tmp = list_entry(item);
> + if (ipv6_addr_cmp(&tmp->hit_our, src_hit)) {
> + continue;
> + } else if (!ipv6_addr_cmp(&tmp->peer_addr, dst_ip)) {
> + if (tmp->peer_udp_port == dst_port) {
> + return tmp;
> + } else {
> + continue;
> + }
> + } else {
> + continue;
> + }
> + }
> + return NULL;
> +}

style: where possible, functions should only have a single point of exit
in order to keep their control flow simple and intuitive. I suggest to
store the result of the search in a local variable which is initialized
to NULL and which is returned at the end of the function.

> /**
> + * Gets called in case of an intended opportunistic base exchange - checks
> + * first whether a host association is already established, and won't perform
> + * the base exchange if so.
> + *
> + * If no host association exists, proceeds to pass everything through to
> + * hip_netdev_trigger_bex_msg().
> + *
> + * @param msg the message from hipconf
> + * @return zero on success and non-zero on error
> + */
> +int hip_netdev_trigger_opp_bex_msg(const struct hip_common *const msg)

documentation: again, mention what happens to the msg object in terms of
memory allocation, in particular in the case of an error. Of course,
this function has the same interface as ...

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>

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 bein...