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

Proposed by Christoph Viethen on 2012-01-08
Status: Needs review
Proposed branch: lp:~cviethen/hipl/pisa-pairing-tng
Merge into: lp:hipl
Diff against target: 370 lines (+216/-4)
8 files modified
hipd/hadb.c (+36/-0)
hipd/hadb.h (+5/-0)
hipd/netdev.c (+49/-2)
hipd/netdev.h (+1/-0)
hipd/user.c (+4/-0)
lib/core/builder.c (+1/-0)
lib/core/conf.c (+119/-2)
lib/core/icomm.h (+1/-0)
To merge this branch: bzr merge lp:~cviethen/hipl/pisa-pairing-tng
Reviewer Review Type Date Requested Status
Stefan Götz (community) Needs Fixing on 2012-01-10
Miika Komu Needs Fixing on 2012-01-09
Diego Biurrun 2012-01-08 Needs Fixing on 2012-01-08
Review via email: mp+87876@code.launchpad.net

Description of the change

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!

To post a comment you must log in.
Christof Mroz (christof-mroz) wrote :
Download full text (3.7 KiB)

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

Read more...

Diego Biurrun (diego-biurrun) wrote :
Download full text (4.7 KiB)

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

Read more...

review: Needs Fixing
Christoph Viethen (cviethen) wrote :
Download full text (3.5 KiB)

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

Read more...

Christoph Viethen (cviethen) wrote :
Download full text (3.8 KiB)

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

Read more...

lp:~cviethen/hipl/pisa-pairing-tng updated on 2012-01-09
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) wrote :

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) wrote :

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) wrote :
Download full text (3.6 KiB)

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

Read more...

Download full text (3.2 KiB)

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

Read more...

review: Needs Fixing
Christoph Viethen (cviethen) 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?

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) wrote :
Download full text (4.3 KiB)

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

Read more...

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'hipd/hadb.c'
2--- hipd/hadb.c 2011-12-30 23:20:44 +0000
3+++ hipd/hadb.c 2012-01-09 03:18:25 +0000
4@@ -1400,6 +1400,42 @@
5 }
6
7 /**
8+ * Search for a <tt> struct ::hip_hadb_state </tt> entry from the
9+ * @c ::hadb_hit by the given tuple (source hit, dest. ip, dest. port).
10+ *
11+ * This is useful to find a host association when the opposite HIT is not
12+ * (yet) known, in particular in the opportunistic base exchange case.
13+ *
14+ * @param src_hit pointer to source HIT
15+ * @param dst_ip pointer to destination IP address
16+ * @param dst_port destination UDP port number (host byte order) (may be 0)
17+ * @return a pointer to a matching host association, or NULL if
18+ * a matching host association was not found.
19+ */
20+const struct hip_hadb_state *hip_hadb_find_by_srchit_destip_port(const hip_hit_t *const src_hit,
21+ const struct in6_addr *const dst_ip,
22+ const in_port_t dst_port)
23+{
24+ const LHASH_NODE *item, *aux;
25+ int i;
26+
27+ list_for_each_safe(item, aux, hadb_hit, i)
28+ {
29+ const struct hip_hadb_state *const tmp = list_entry(item);
30+
31+ const bool ok = !ipv6_addr_cmp(&tmp->hit_our, src_hit) &&
32+ !ipv6_addr_cmp(&tmp->peer_addr, dst_ip) &&
33+ tmp->peer_udp_port == dst_port;
34+
35+ if (ok) {
36+ return tmp;
37+ }
38+ }
39+
40+ return NULL;
41+}
42+
43+/**
44 * This function simply goes through all HADB to find an entry that
45 * matches the given lsi pair. First matching HADB entry is then returned.
46 *
47
48=== modified file 'hipd/hadb.h'
49--- hipd/hadb.h 2011-11-25 17:56:24 +0000
50+++ hipd/hadb.h 2012-01-09 03:18:25 +0000
51@@ -115,6 +115,11 @@
52
53 int hip_handle_get_ha_info(struct hip_hadb_state *entry, void *);
54
55+const struct hip_hadb_state *
56+ hip_hadb_find_by_srchit_destip_port(const hip_hit_t *const src_hit,
57+ const struct in6_addr *const dst_ip,
58+ const in_port_t dst_port);
59+
60 /*lsi support functions*/
61 int hip_generate_peer_lsi(hip_lsi_t *lsi);
62 struct hip_hadb_state *hip_hadb_try_to_find_by_peer_lsi(const hip_lsi_t *lsi);
63
64=== modified file 'hipd/netdev.c'
65--- hipd/netdev.c 2011-12-30 23:20:44 +0000
66+++ hipd/netdev.c 2012-01-09 03:18:25 +0000
67@@ -832,8 +832,7 @@
68 /* Now we should have at least source HIT and destination HIT.
69 * Sometimes we get deformed HITs from kernel, skip them */
70 HIP_IFEL(!(ipv6_addr_is_hit(src_hit) && ipv6_addr_is_hit(dst_hit) &&
71- hip_hidb_hit_is_our(src_hit) &&
72- hit_is_real_hit(dst_hit)), -1,
73+ hip_hidb_hit_is_our(src_hit)), -1,
74 "Received rubbish from netlink, skip\n");
75
76 /* Existing entry found. No need for peer IP checks */
77@@ -1094,6 +1093,54 @@
78 }
79
80 /**
81+ * Check first whether a host association is already established, and don't
82+ * perform a base exchange if so.
83+ *
84+ * If no host association exists, proceeds to pass everything through to
85+ * hip_netdev_trigger_bex_msg().
86+ *
87+ * Gets called in case of an intended opportunistic base exchange.
88+ *
89+ * @param msg the message from hipconf
90+ * @return zero on success and non-zero on error
91+ */
92+int hip_netdev_trigger_opp_bex_msg(const struct hip_common *const msg)
93+{
94+ hip_hit_t our_hit;
95+
96+ /* Find out: 1. our own hit,
97+ * 2. the IP address the base exchange should be sent to, */
98+ const struct hip_tlv_common *const param = hip_get_param(msg, HIP_PARAM_IPV6_ADDR);
99+
100+ if (hip_get_default_hit(&our_hit) < 0 || !param) {
101+ return -1;
102+ }
103+
104+ const struct in6_addr *const dst_addr = hip_get_param_contents_direct(param);
105+
106+ /* and 3. the UDP port number base exchanges will be sent to. */
107+ const in_port_t ha_peer_port = hip_nat_status ? hip_get_peer_nat_udp_port() : 0;
108+
109+ /* use these three parameters to find a matching entry in the HADB */
110+ const struct hip_hadb_state *const entry =
111+ hip_hadb_find_by_srchit_destip_port(&our_hit, dst_addr, ha_peer_port);
112+
113+ if (entry) {
114+ /* if we already have an association in HIP_STATE_ESTABLISHED with
115+ * that peer, don't create another HADB entry and don't send an
116+ * opportunistic base exchange: the resulting R1 response coming
117+ * in from the other side would be discarded by our input.c:packet_to_drop(),
118+ * causing the new HADB entry to be kept hanging in HIP_STATE_I1_SENT
119+ */
120+ if (entry->state == HIP_STATE_ESTABLISHED) {
121+ return -1;
122+ }
123+ }
124+
125+ return hip_netdev_trigger_bex_msg(msg);
126+}
127+
128+/**
129 * Add or delete an address to the cache of localhost addresses. This
130 * function also checks if the address is already on the list when adding
131 * or absent from the list when deleting.
132
133=== modified file 'hipd/netdev.h'
134--- hipd/netdev.h 2011-11-25 17:56:24 +0000
135+++ hipd/netdev.h 2012-01-09 03:18:25 +0000
136@@ -48,6 +48,7 @@
137 int hip_add_iface_local_route(const hip_hit_t *local_hit);
138 int hip_select_source_address(struct in6_addr *src, const struct in6_addr *dst);
139 int hip_netdev_trigger_bex_msg(const struct hip_common *msg);
140+int hip_netdev_trigger_opp_bex_msg(const struct hip_common *msg);
141 void hip_add_address_to_list(struct sockaddr *addr, int ifindex, int flags);
142
143 int hip_netdev_white_list_add(const char *const device_name);
144
145=== modified file 'hipd/user.c'
146--- hipd/user.c 2011-11-25 16:40:40 +0000
147+++ hipd/user.c 2012-01-09 03:18:25 +0000
148@@ -709,6 +709,10 @@
149 HIP_DEBUG("hip_broadcast_status = %d (should be %d)\n",
150 hip_broadcast_status, HIP_MSG_BROADCAST_OFF);
151 break;
152+ case HIP_MSG_TRIGGER_OPP_BEX:
153+ HIP_DEBUG("HIP_MSG_TRIGGER_OPP_BEX\n");
154+ err = hip_netdev_trigger_opp_bex_msg(msg);
155+ break;
156 default:
157 HIP_ERROR("Unknown socket option (%d)\n", msg_type);
158 err = -ESOCKTNOSUPPORT;
159
160=== modified file 'lib/core/builder.c'
161--- lib/core/builder.c 2012-01-04 18:44:42 +0000
162+++ lib/core/builder.c 2012-01-09 03:18:25 +0000
163@@ -1131,6 +1131,7 @@
164 case HIP_MSG_REINIT_FULLRELAY: return "HIP_MSG_REINIT_FULLRELAY";
165 case HIP_MSG_FIREWALL_START: return "HIP_MSG_FIREWALL_START";
166 case HIP_MSG_MANUAL_UPDATE_PACKET: return "HIP_MSG_MANUAL_UPDATE_PACKET";
167+ case HIP_MSG_TRIGGER_OPP_BEX: return "HIP_MSG_TRIGGER_OPP_BEX";
168 default:
169 return lmod_get_packet_identifier(msg_type);
170 }
171
172=== modified file 'lib/core/conf.c'
173--- lib/core/conf.c 2012-01-05 15:56:43 +0000
174+++ lib/core/conf.c 2012-01-09 03:18:25 +0000
175@@ -126,7 +126,8 @@
176 /* unused, was ACTION_HANDOVER 39 */
177 #define ACTION_MANUAL_UPDATE 40
178 #define ACTION_BROADCAST 41
179-#define ACTION_MAX 42 /* exclusive */
180+#define ACTION_TRIGGER 42
181+#define ACTION_MAX 43 /* exclusive */
182
183 /**
184 * TYPE_ constant list, as an index for each action_handler function.
185@@ -176,7 +177,8 @@
186 /* unused, was TYPE_HANDOVER 42 */
187 #define TYPE_MANUAL_UPDATE 43
188 #define TYPE_BROADCAST 44
189-#define TYPE_MAX 45 /* exclusive */
190+#define TYPE_BEX 45
191+#define TYPE_MAX 46 /* exclusive */
192
193 /* #define TYPE_RELAY 22 */
194
195@@ -230,6 +232,7 @@
196 "shotgun on|off\n"
197 "id-to-addr hit|lsi\n"
198 "broadcast on|off\n"
199+ "trigger bex <ip|ip6|hostname> (trigger opp. base exchange)\n"
200 ;
201
202 /**
203@@ -648,6 +651,8 @@
204 }
205 } else if (!strcmp("broadcast", argv[2])) {
206 ret = ACTION_BROADCAST;
207+ } else if (!strcmp("trigger", argv[2])) {
208+ ret = ACTION_TRIGGER;
209 }
210
211 return ret;
212@@ -698,6 +703,7 @@
213 case ACTION_TRANSORDER:
214 case ACTION_NAT_LOCAL_PORT:
215 case ACTION_NAT_PEER_PORT:
216+ case ACTION_TRIGGER:
217 count = 2;
218 break;
219 default:
220@@ -776,6 +782,8 @@
221 ret = TYPE_LSI_TO_HIT;
222 } else if (strcmp("broadcast", argv[2]) == 0) {
223 ret = TYPE_BROADCAST;
224+ } else if (!strcmp("bex", text)) {
225+ ret = TYPE_BEX;
226 } else {
227 HIP_DEBUG("ERROR: NO MATCHES FOUND \n");
228 }
229@@ -819,6 +827,7 @@
230 case ACTION_HIT_TO_IP:
231 case ACTION_HIT_TO_IP_SET:
232 case ACTION_BROADCAST:
233+ case ACTION_TRIGGER:
234 type_arg = 3;
235 break;
236 case ACTION_MANUAL_UPDATE:
237@@ -2424,6 +2433,113 @@
238 }
239
240 /**
241+ * Handle the <tt> hipconf daemon trigger bex </tt> command.
242+ *
243+ * @param msg a pointer to the buffer where the message for hipd will
244+ * be written.
245+ * @param action numeric action identifier (ACTION_TRIGGER is expected).
246+ * @param opt an array of pointers to the command line arguments after
247+ * the action and type.
248+ * @param optc the number of elements in the opt array (@b 1).
249+ * @param send_only currently unused
250+ * @return zero on success, or -1 on error.
251+ */
252+static int conf_handle_trigger_bex(struct hip_common *const msg,
253+ const int action,
254+ const char *opt[],
255+ const int optc,
256+ UNUSED int send_only)
257+{
258+ struct in6_addr ipv6_addr;
259+ hip_hit_t pseudo_hit;
260+ struct addrinfo *addrinfo_result = NULL;
261+ const struct addrinfo addrinfo_hints = { .ai_family = AF_UNSPEC,
262+ .ai_protocol = IPPROTO_UDP };
263+
264+ if (action != ACTION_TRIGGER) {
265+ return -1;
266+ }
267+
268+ if (optc > 1) {
269+ HIP_ERROR("Too many arguments for trigger bex command.\n");
270+ return -1;
271+ }
272+
273+ /* use getaddrinfo() to find an address for the given hostname;
274+ * it won't do any lookups in case the user just provides an
275+ * IP address */
276+
277+ const int ai_err = getaddrinfo(opt[0], NULL, &addrinfo_hints, &addrinfo_result);
278+
279+ /* We don't specify what address family we want, and we may get a
280+ * linked list of multiple results: IPv4, IPv6 or anything else
281+ * the specific implementation likes to come up with. The "best"
282+ * results are supposed to come first (cf. RFC 3484), with the
283+ * admin having some influence by tweaking /etc/gai.conf.
284+ *
285+ * Accordingly, we stop walking the linked list as soon as we find
286+ * an entry that points to an actual IP address, hoping that
287+ * everything was set up wisely. */
288+
289+ if (!ai_err) {
290+ int done = 0;
291+ const struct addrinfo *ai_res = addrinfo_result;
292+
293+ while (!done && ai_res) {
294+ if (ai_res->ai_addr->sa_family == AF_INET) {
295+ /* turn address into an IPv4-mapped IPv6 address
296+ * (RFC 4291, 2.5.5.2.) */
297+ IPV4_TO_IPV6_MAP(&((struct sockaddr_in *)
298+ ai_res->ai_addr)->sin_addr, &ipv6_addr);
299+ done = 1;
300+ }
301+ if (ai_res->ai_addr->sa_family == AF_INET6) {
302+ ipv6_addr_copy(&ipv6_addr, &((struct sockaddr_in6 *)
303+ ai_res->ai_addr)->sin6_addr);
304+ done = 1;
305+ }
306+ ai_res = ai_res->ai_next;
307+ }
308+
309+ if (addrinfo_result) {
310+ freeaddrinfo(addrinfo_result);
311+ }
312+
313+ if (!done) {
314+ HIP_ERROR("Failed to find any IP address for hostname.");
315+ return -1;
316+ }
317+ } else {
318+ HIP_ERROR("Failed to resolve hostname: %s\n", gai_strerror(ai_err));
319+ return -1;
320+ }
321+
322+ const int uh_err = hip_build_user_hdr(msg, HIP_MSG_TRIGGER_OPP_BEX, 0);
323+
324+ if (uh_err) {
325+ HIP_ERROR("Failed to build user message header: %s\n", strerror(uh_err));
326+ return -1;
327+ }
328+
329+ if (hip_opportunistic_ipv6_to_hit(&ipv6_addr, &pseudo_hit, HIP_HIT_TYPE_HASH100)) {
330+ HIP_ERROR("hip_opportunistic_ipv6_to_hit() failed.\n");
331+ return -1;
332+ }
333+
334+ if (hip_build_param_contents(msg, &pseudo_hit, HIP_PARAM_HIT, sizeof(hip_hit_t))) {
335+ HIP_ERROR("Failed to build HIT parameter to hipconf message.\n");
336+ return -1;
337+ }
338+
339+ if (hip_build_param_contents(msg, &ipv6_addr, HIP_PARAM_IPV6_ADDR, sizeof(struct in6_addr))) {
340+ HIP_ERROR("Failed to build IP address parameter to hipconf message.\n");
341+ return -1;
342+ }
343+
344+ return 0;
345+}
346+
347+/**
348 * Function pointer array containing pointers to handler functions.
349 * Add a handler function for your new action in the action_handler[] array.
350 * If you added a handler function here, do not forget to define that function
351@@ -2492,6 +2608,7 @@
352 NULL, /* 42: unused, was TYPE_HANDOVER */
353 conf_handle_manual_update, /* 43: TYPE_MANUAL_UPDATE */
354 conf_handle_broadcast, /* 44: TYPE_BROADCAST */
355+ conf_handle_trigger_bex, /* 45: TYPE_BEX */
356 NULL /* TYPE_MAX, the end. */
357 };
358
359
360=== modified file 'lib/core/icomm.h'
361--- lib/core/icomm.h 2011-11-25 17:56:24 +0000
362+++ lib/core/icomm.h 2012-01-09 03:18:25 +0000
363@@ -159,6 +159,7 @@
364 #define HIP_MSG_FIREWALL_STATUS 201
365 #define HIP_MSG_BROADCAST_OFF 202
366 #define HIP_MSG_BROADCAST_ON 203
367+#define HIP_MSG_TRIGGER_OPP_BEX 204
368 /* @} */
369
370 /* inclusive */

Subscribers

People subscribed via source and target branches