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

[Christoph Viethen]

Here's a new incarnation of my patch to enable triggering an opportunistic base exchange from hipconf. I made it a new branch to have it correspond closely to the current state of HIPL.

This functionality is necessary for performing the pairing procedure in PiSA, but since it may be
quite useful for other purposes as well, no explicit reference to PiSA is made within the code.

I tried to take into account feedback I got from my previous version a couple months ago, focusing especially on minimizing code duplication and, accordingly, on re-using as many bits and pieces as already available.

It - almost - perfectly worked: I had to introduce a new message type that hipconf will send to hipd since the available message type (HIP_MSG_TRIGGER_BEX) only got used by hipfw so far, and accordingly hipd makes special assumptions about the status of the firewall when this type of message arrives. It doesn't seem to be possible to trivially determine who sent the message (hipconf or hipfw) at that point, so I opted for creating a new type (HIP_MSG_TRIGGER_OPP_BEX) that will only be sent from hipconf.

Additional advantage is that, when receiving such a message, the code can be sure an opportunistic base exchange was intended, so I could add an additional check for an already-existing host association with the desired target:

In case that association exists in ESTABLISHED state, it's not a good idea to try and send another I1 message to the opposing host - it will respond with an R1, containing full source and destination HITs, but that one will be rejected by the local host since there's already an established host association. As a consequence, the newly created HA would be stuck in I1_SENT state. To prevent this, opportunistic base exchanges are not sent to hosts with which we already have an established association.

Thank you!

[Christof Mroz (christof-mroz)]

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

[Diego Biurrun (diego-biurrun)]

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

review: Needs Fixing

[Christoph Viethen (cviethen)]

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

[Christoph Viethen (cviethen)]

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

[lp:~cviethen/hipl/pisa-pairing-tng]

6235. By Christoph Viethen on 2012-01-09

Misc. code cleanup and cosmetics - no change in functionality. Many thanks
to Christof and Diego for their input.

[Diego Biurrun (diego-biurrun)]

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

[Miika Komu (miika-iki)]

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().

review: Needs Fixing

[Christof Mroz (christof-mroz)]

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

[Stefan Götz (stefan.goetz-deactivatedaccount)]

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

review: Needs Fixing

[Christoph Viethen (cviethen)]

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>

[Miika Komu (miika-iki)]

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

Unmerged revisions

6235. By Christoph Viethen on 2012-01-09

Misc. code cleanup and cosmetics - no change in functionality. Many thanks
to Christof and Diego for their input.

6234. By Christoph Viethen on 2012-01-08

Fix a little bug concerning Doxygen.

6233. By Christoph Viethen on 2012-01-08

Submit an updated version of patch, based on current HIPL. This one will enable
using the command "hipconf daemon trigger bex <ip-addr>" to trigger an
opportunistic base exchange with the given host.

To achieve this, a few places in HIPL are touched:

1) Add functionality to hipconf, using a new message type
 (HIP_MSG_TRIGGER_OPP_BEX) and an appropriate handling function
 (conf_handle_trigger_bex()) (lib/core/builder.c, lib/core/conf.c,
 lib/core/icomm.h).

2) Enable hipd to handle such a message (hipd/user.c), adding a new function
 called hip_netdev_trigger_opp_bex_msg() (hipd/netdev.c).

3) That new function which will check for an already-existing host
 association before calling a function that will perform the actual
 opportunistic base exchange; check will prevent creating associations that
 will get stuck in I1_SENT state. (Add helper function to hipd/hadb.c.)

4) Patch netdev_trigger_bex() slightly to not error out if encountering an
 pseudo HIT as a target, and use this function for the actual base exchange
 (hipd/netdev.c).

=== modified file 'hipd/hadb.c'
--- hipd/hadb.c	2011-12-30 23:20:44 +0000
+++ hipd/hadb.c	2012-01-09 03:18:25 +0000
@@ -1400,6 +1400,42 @@
 }
 
 /**
+ * Search 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.
+ */
+const struct hip_hadb_state *hip_hadb_find_by_srchit_destip_port(const hip_hit_t *const src_hit,
+                                                                 const struct in6_addr *const dst_ip,
+                                                                 const in_port_t dst_port)
+{
+    const LHASH_NODE *item, *aux;
+    int               i;
+
+    list_for_each_safe(item, aux, hadb_hit, i)
+    {
+        const struct hip_hadb_state *const 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;
+        }
+    }
+
+    return NULL;
+}
+
+/**
  * This function simply goes through all HADB to find an entry that
  * matches the given lsi pair. First matching HADB entry is then returned.
  *
 
=== modified file 'hipd/hadb.h'
--- hipd/hadb.h	2011-11-25 17:56:24 +0000
+++ hipd/hadb.h	2012-01-09 03:18:25 +0000
@@ -115,6 +115,11 @@
 
 int hip_handle_get_ha_info(struct hip_hadb_state *entry, void *);
 
+const struct hip_hadb_state *
+       hip_hadb_find_by_srchit_destip_port(const hip_hit_t *const src_hit,
+                                           const struct in6_addr *const dst_ip,
+                                           const in_port_t dst_port);
+
 /*lsi support functions*/
 int hip_generate_peer_lsi(hip_lsi_t *lsi);
 struct hip_hadb_state *hip_hadb_try_to_find_by_peer_lsi(const hip_lsi_t *lsi);
 
=== modified file 'hipd/netdev.c'
--- hipd/netdev.c	2011-12-30 23:20:44 +0000
+++ hipd/netdev.c	2012-01-09 03:18:25 +0000
@@ -832,8 +832,7 @@
     /* Now we should have at least source HIT and destination HIT.
      * Sometimes we get deformed HITs from kernel, skip them */
     HIP_IFEL(!(ipv6_addr_is_hit(src_hit) && ipv6_addr_is_hit(dst_hit) &&
-               hip_hidb_hit_is_our(src_hit) &&
-               hit_is_real_hit(dst_hit)), -1,
+               hip_hidb_hit_is_our(src_hit)), -1,
              "Received rubbish from netlink, skip\n");
 
     /* Existing entry found. No need for peer IP checks */
@@ -1094,6 +1093,54 @@
 }
 
 /**
+ * Check first whether a host association is already established, and don't
+ * perform a base exchange if so.
+ *
+ * If no host association exists, proceeds to pass everything through to
+ * hip_netdev_trigger_bex_msg().
+ *
+ * Gets called in case of an intended opportunistic base exchange.
+ *
+ * @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)
+{
+    hip_hit_t our_hit;
+
+    /* Find out: 1. our own hit,
+     *           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 (hip_get_default_hit(&our_hit) < 0 || !param) {
+        return -1;
+    }
+
+    const struct in6_addr *const dst_addr = hip_get_param_contents_direct(param);
+
+    /* 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;
+
+    /* use these three parameters to find a matching entry in the HADB */
+    const struct hip_hadb_state *const entry =
+        hip_hadb_find_by_srchit_destip_port(&our_hit, dst_addr, ha_peer_port);
+
+    if (entry) {
+        /* if we already have an association in HIP_STATE_ESTABLISHED with
+         *  that peer, don't create another HADB entry and don't send an
+         *  opportunistic base exchange: the resulting R1 response coming
+         *  in from the other side would be discarded by our input.c:packet_to_drop(),
+         *  causing the new HADB entry to be kept hanging in HIP_STATE_I1_SENT
+         */
+        if (entry->state == HIP_STATE_ESTABLISHED) {
+            return -1;
+        }
+    }
+
+    return hip_netdev_trigger_bex_msg(msg);
+}
+
+/**
  * Add or delete an address to the cache of localhost addresses. This
  * function also checks if the address is already on the list when adding
  * or absent from the list when deleting.
 
=== modified file 'hipd/netdev.h'
--- hipd/netdev.h	2011-11-25 17:56:24 +0000
+++ hipd/netdev.h	2012-01-09 03:18:25 +0000
@@ -48,6 +48,7 @@
 int hip_add_iface_local_route(const hip_hit_t *local_hit);
 int hip_select_source_address(struct in6_addr *src, const struct in6_addr *dst);
 int hip_netdev_trigger_bex_msg(const struct hip_common *msg);
+int hip_netdev_trigger_opp_bex_msg(const struct hip_common *msg);
 void hip_add_address_to_list(struct sockaddr *addr, int ifindex, int flags);
 
 int hip_netdev_white_list_add(const char *const device_name);
 
=== modified file 'hipd/user.c'
--- hipd/user.c	2011-11-25 16:40:40 +0000
+++ hipd/user.c	2012-01-09 03:18:25 +0000
@@ -709,6 +709,10 @@
         HIP_DEBUG("hip_broadcast_status =  %d (should be %d)\n",
                   hip_broadcast_status, HIP_MSG_BROADCAST_OFF);
         break;
+    case HIP_MSG_TRIGGER_OPP_BEX:
+        HIP_DEBUG("HIP_MSG_TRIGGER_OPP_BEX\n");
+        err = hip_netdev_trigger_opp_bex_msg(msg);
+        break;
     default:
         HIP_ERROR("Unknown socket option (%d)\n", msg_type);
         err = -ESOCKTNOSUPPORT;
 
=== modified file 'lib/core/builder.c'
--- lib/core/builder.c	2012-01-04 18:44:42 +0000
+++ lib/core/builder.c	2012-01-09 03:18:25 +0000
@@ -1131,6 +1131,7 @@
     case HIP_MSG_REINIT_FULLRELAY:   return "HIP_MSG_REINIT_FULLRELAY";
     case HIP_MSG_FIREWALL_START:     return "HIP_MSG_FIREWALL_START";
     case HIP_MSG_MANUAL_UPDATE_PACKET: return "HIP_MSG_MANUAL_UPDATE_PACKET";
+    case HIP_MSG_TRIGGER_OPP_BEX:    return "HIP_MSG_TRIGGER_OPP_BEX";
     default:
         return lmod_get_packet_identifier(msg_type);
     }
 
=== modified file 'lib/core/conf.c'
--- lib/core/conf.c	2012-01-05 15:56:43 +0000
+++ lib/core/conf.c	2012-01-09 03:18:25 +0000
@@ -126,7 +126,8 @@
 /* unused, was ACTION_HANDOVER 39 */
 #define ACTION_MANUAL_UPDATE 40
 #define ACTION_BROADCAST 41
-#define ACTION_MAX 42 /* exclusive */
+#define ACTION_TRIGGER 42
+#define ACTION_MAX 43 /* exclusive */
 
 /**
  * TYPE_ constant list, as an index for each action_handler function.
@@ -176,7 +177,8 @@
 /* unused, was TYPE_HANDOVER 42 */
 #define TYPE_MANUAL_UPDATE 43
 #define TYPE_BROADCAST     44
-#define TYPE_MAX           45 /* exclusive */
+#define TYPE_BEX           45
+#define TYPE_MAX           46 /* exclusive */
 
 /* #define TYPE_RELAY         22 */
 
@@ -230,6 +232,7 @@
     "shotgun on|off\n"
     "id-to-addr hit|lsi\n"
     "broadcast on|off\n"
+    "trigger bex <ip|ip6|hostname>             (trigger opp. base exchange)\n"
 ;
 
 /**
@@ -648,6 +651,8 @@
         }
     } else if (!strcmp("broadcast", argv[2])) {
         ret = ACTION_BROADCAST;
+    } else if (!strcmp("trigger", argv[2])) {
+        ret = ACTION_TRIGGER;
     }
 
     return ret;
@@ -698,6 +703,7 @@
     case ACTION_TRANSORDER:
     case ACTION_NAT_LOCAL_PORT:
     case ACTION_NAT_PEER_PORT:
+    case ACTION_TRIGGER:
         count = 2;
         break;
     default:
@@ -776,6 +782,8 @@
         ret = TYPE_LSI_TO_HIT;
     } else if (strcmp("broadcast", argv[2]) == 0) {
         ret = TYPE_BROADCAST;
+    } else if (!strcmp("bex", text)) {
+        ret = TYPE_BEX;
     } else {
         HIP_DEBUG("ERROR: NO MATCHES FOUND \n");
     }
@@ -819,6 +827,7 @@
     case ACTION_HIT_TO_IP:
     case ACTION_HIT_TO_IP_SET:
     case ACTION_BROADCAST:
+    case ACTION_TRIGGER:
         type_arg = 3;
         break;
     case ACTION_MANUAL_UPDATE:
@@ -2424,6 +2433,113 @@
 }
 
 /**
+ * Handle the <tt> hipconf daemon trigger bex </tt> command.
+ *
+ * @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.
+ * @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 action,
+                                   const char *opt[],
+                                   const int optc,
+                                   UNUSED int send_only)
+{
+    struct in6_addr       ipv6_addr;
+    hip_hit_t             pseudo_hit;
+    struct addrinfo      *addrinfo_result = NULL;
+    const struct addrinfo addrinfo_hints  = { .ai_family   = AF_UNSPEC,
+                                              .ai_protocol = IPPROTO_UDP };
+
+    if (action != ACTION_TRIGGER) {
+        return -1;
+    }
+
+    if (optc > 1) {
+        HIP_ERROR("Too many arguments for trigger bex command.\n");
+        return -1;
+    }
+
+    /* use getaddrinfo() to find an address for the given hostname;
+     *  it won't do any lookups in case the user just provides an
+     *  IP address */
+
+    const int ai_err = getaddrinfo(opt[0], NULL, &addrinfo_hints, &addrinfo_result);
+
+    /* We don't specify what address family we want, and we may get a
+     *  linked list of multiple results: IPv4, IPv6 or anything else
+     *  the specific implementation likes to come up with. The "best"
+     *  results are supposed to come first (cf. RFC 3484), with the
+     *  admin having some influence by tweaking /etc/gai.conf.
+     *
+     * Accordingly, we stop walking the linked list as soon as we find
+     *  an entry that points to an actual IP address, hoping that
+     *  everything was set up wisely. */
+
+    if (!ai_err) {
+        int                    done   = 0;
+        const struct addrinfo *ai_res = addrinfo_result;
+
+        while (!done && ai_res) {
+            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) {
+                ipv6_addr_copy(&ipv6_addr, &((struct sockaddr_in6 *)
+                                             ai_res->ai_addr)->sin6_addr);
+                done = 1;
+            }
+            ai_res = ai_res->ai_next;
+        }
+
+        if (addrinfo_result) {
+            freeaddrinfo(addrinfo_result);
+        }
+
+        if (!done) {
+            HIP_ERROR("Failed to find any IP address for hostname.");
+            return -1;
+        }
+    } else {
+        HIP_ERROR("Failed to resolve hostname: %s\n", gai_strerror(ai_err));
+        return -1;
+    }
+
+    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;
+    }
+
+    if (hip_opportunistic_ipv6_to_hit(&ipv6_addr, &pseudo_hit, HIP_HIT_TYPE_HASH100)) {
+        HIP_ERROR("hip_opportunistic_ipv6_to_hit() failed.\n");
+        return -1;
+    }
+
+    if (hip_build_param_contents(msg, &pseudo_hit, HIP_PARAM_HIT, sizeof(hip_hit_t))) {
+        HIP_ERROR("Failed to build HIT parameter to hipconf message.\n");
+        return -1;
+    }
+
+    if (hip_build_param_contents(msg, &ipv6_addr, HIP_PARAM_IPV6_ADDR, sizeof(struct in6_addr))) {
+        HIP_ERROR("Failed to build IP address parameter to hipconf message.\n");
+        return -1;
+    }
+
+    return 0;
+}
+
+/**
  * Function pointer array containing pointers to handler functions.
  * Add a handler function for your new action in the action_handler[] array.
  * If you added a handler function here, do not forget to define that function
@@ -2492,6 +2608,7 @@
     NULL,                               /* 42: unused, was TYPE_HANDOVER */
     conf_handle_manual_update,          /* 43: TYPE_MANUAL_UPDATE */
     conf_handle_broadcast,              /* 44: TYPE_BROADCAST */
+    conf_handle_trigger_bex,            /* 45: TYPE_BEX */
     NULL     /* TYPE_MAX, the end. */
 };
 
 
=== modified file 'lib/core/icomm.h'
--- lib/core/icomm.h	2011-11-25 17:56:24 +0000
+++ lib/core/icomm.h	2012-01-09 03:18:25 +0000
@@ -159,6 +159,7 @@
 #define HIP_MSG_FIREWALL_STATUS                  201
 #define HIP_MSG_BROADCAST_OFF                    202
 #define HIP_MSG_BROADCAST_ON                     203
+#define HIP_MSG_TRIGGER_OPP_BEX                  204
 /* @} */
 
 /* inclusive */