Merge lp:~stefan.goetz-deactivatedaccount/hipl/hidb-db into lp:hipl

[Stefan Götz]

Cleans up the API of the HIDB. It removes all direct external accesses to the HIDB and ensures that the HIDB is only accessed through accessor functions.

Reviewing the individual commits provides additional context.

Changes since the previous merge proposal: re-based on master branch, no functional changes. Please re-review for any issues that would prevent merging this branch.

[David Martin (martin-lp)]

Hi,
after having looked through this branch I have one question and only a single remark on your code:

In revision 5941:
> Remove the locking macros for the HIDB because they are not used.

What do you mean with "they are not used"? Because there are lots of calls to lock / unlock that
you remove and without looking deeper into it seems like it is used here. Or is it because all
access is done on our local hidb and we are neither multi-threaded nor does it get accessed from
beyond the file so no locking is necessary?

> -struct local_host_id *hip_get_hostid_entry_by_lhi_and_algo(HIP_HASHTABLE *db,
> - const struct in6_addr *hit,
> +struct local_host_id *hip_get_hostid_entry_by_lhi_and_algo(const struct in6_addr *hit,

"const struct in6_addr *const hit" would be more correct here?

> +/**
> + * A call-back function for finding a host association between a given peer HIT
> + * and any of the iteratively provided local HIs.
> + *
> + * @param lhi Points to a local_host_id object from which contains the
> + * local HIT of the HA to find.

"Points to a local_host_id object which contains"

> -int hip_hidb_match(const void *ptr1, const void *ptr2)
> +static int hip_hidb_match(const void *ptr1, const void *ptr2)

*const ptr1 and *const ptr2 would be more correct?

Other than that, as far as I'm able to judge it's looking good. Nice one! :)

PS: hipd crashes on shutdown after doing a base exchange. This has already been fixed in trunk revision 5948 and applying the change fixes it good.

review: Needs Information

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

Hi David!

Thanks for the review!

> In revision 5941:
>> Remove the locking macros for the HIDB because they are not used.
>
> What do you mean with "they are not used"? Because there are lots of calls to lock / unlock that
> you remove and without looking deeper into it seems like it is used here. Or is it because all
> access is done on our local hidb and we are neither multi-threaded nor does it get accessed from
> beyond the file so no locking is necessary?

I believe the locking is not used because:

- The code does not compile when enabling locking (note that locking is disabled
via a '#if 0')

- Locking was disabled by a commit in 2007 and I have no evidence that it was
ever enabled again

- The locking logic in trunk is badly broken. Exhibit 1: the core lookup
function hip_get_hostid_entry_by_lhi_and_algo() does not use locking. Exhibit 2:
hip_del_host_id() unlocks the database although the code path up to that point
never acquires a lock. Exhibit 3: Some code paths in hip_del_host_id() unlock
the database, some others do not. The list goes on...

- There is no multi-threading in HIPL (... for all I know. But given the above
reasons, I really really hope so, too)

Thus, I think it makes a lot of sense to remove the locking code because it
needs *major* re-working anyway.

>> -struct local_host_id *hip_get_hostid_entry_by_lhi_and_algo(HIP_HASHTABLE *db,
>> - const struct in6_addr *hit,
>> +struct local_host_id *hip_get_hostid_entry_by_lhi_and_algo(const struct in6_addr *hit,
>
> "const struct in6_addr *const hit" would be more correct here?

I wanted to address const correctness in one big commit, but anyway. Fixed in
rev. 5954

>> +/**
>> + * A call-back function for finding a host association between a given peer HIT
>> + * and any of the iteratively provided local HIs.
>> + *
>> + * @param lhi Points to a local_host_id object from which contains the
>> + * local HIT of the HA to find.
>
> "Points to a local_host_id object which contains"

Oops. Fixed in rev. 5953

>> -int hip_hidb_match(const void *ptr1, const void *ptr2)
>> +static int hip_hidb_match(const void *ptr1, const void *ptr2)
>
> *const ptr1 and *const ptr2 would be more correct?

Fixed in rev. 5954

> Other than that, as far as I'm able to judge it's looking good. Nice one! :)

Thanks! If this addresses all your concerns, please do not forget to change your
vote to 'accept'.

> PS: hipd crashes on shutdown after doing a base exchange. This has already been fixed in trunk revision 5948 and applying the change fixes it good.

Good to know, thanks!

Stefan

[René Hummen (rene-hummen)]

> Hi David!
>
> Thanks for the review!
>
> > In revision 5941:
> >> Remove the locking macros for the HIDB because they are not used.
> >
> > What do you mean with "they are not used"? Because there are lots of calls
> to lock / unlock that
> > you remove and without looking deeper into it seems like it is used here. Or
> is it because all
> > access is done on our local hidb and we are neither multi-threaded nor does
> it get accessed from
> > beyond the file so no locking is necessary?
>
> I believe the locking is not used because:
>
> - The code does not compile when enabling locking (note that locking is
> disabled
> via a '#if 0')
>
> - Locking was disabled by a commit in 2007 and I have no evidence that it was
> ever enabled again
>
> - The locking logic in trunk is badly broken. Exhibit 1: the core lookup
> function hip_get_hostid_entry_by_lhi_and_algo() does not use locking. Exhibit
> 2:
> hip_del_host_id() unlocks the database although the code path up to that point
> never acquires a lock. Exhibit 3: Some code paths in hip_del_host_id() unlock
> the database, some others do not. The list goes on...
>
> - There is no multi-threading in HIPL (... for all I know. But given the above
> reasons, I really really hope so, too)
>
> Thus, I think it makes a lot of sense to remove the locking code because it
> needs *major* re-working anyway.

Miika might be the person to judge this best, but to the best of my knowledge locking is not used in HIPL in the current implementation.

[René Hummen (rene-hummen)]

review needs-fixing

On 20.05.2011, at 23:26, Stefan Götz wrote:
> Stefan Götz has proposed merging lp:~stefan.goetz/hipl/hidb-db into lp:hipl.
>
> Requested reviews:
> HIPL core team (hipl-core)
>
> For more details, see:
> https://code.launchpad.net/~stefan.goetz/hipl/hidb-db/+merge/61832
>
> Cleans up the API of the HIDB. It removes all direct external accesses to the HIDB and ensures that the HIDB is only accessed through accessor functions.
>
> Reviewing the individual commits provides additional context.
[...]
> === modified file 'hipd/hadb.c'
> @@ -983,10 +1001,12 @@
> * Note, that hip_get_host_id() allocates a new buffer and this buffer
> * must be freed in out_err if an error occurs. */
>
> - if (hip_get_host_id_and_priv_key(HIP_DB_LOCAL_HID, hit_our, HIP_HI_RSA,
> + if (hip_get_host_id_and_priv_key(hit_our, HIP_HI_RSA,
> &entry->our_pub, &entry->our_priv_key)) {
> - HIP_IFEL(hip_get_host_id_and_priv_key(HIP_DB_LOCAL_HID, hit_our,
> - HIP_HI_DSA, &entry->our_pub, &entry->our_priv_key),
> + HIP_IFEL(hip_get_host_id_and_priv_key(hit_our,
> + HIP_HI_DSA,
> + &entry->our_pub,
> + &entry->our_priv_key),
> -1, "Local host identity not found\n");
> }

Would it be possible to further abstract from the HI algorithm here? Right now, we need to look for an RSA key first and then fall back to DSA. What happens when we add ECDSA? We will need to add that case to many if not all calls to hip_get_host_id_and_priv_key().

> === modified file 'hipd/hidb.c'
[...]
> - id = hip_get_hostid_entry_by_lhi_and_algo(db, &hit, HIP_ANY_ALGO, -1);
> + id = hip_get_hostid_entry_by_lhi_and_algo(&hit, HIP_ANY_ALGO, -1);

Is there any call to hip_get_hostid_entry_by_lhi_and_algo() with a parameter different from HIP_ANY_ALGO? Furthermore, is there any call with a parameter different from (anon=) -1? If not, these point to another possibility of cleaning up the API.

> === modified file 'hipd/hidb.h'
[...]
> void hip_uninit_host_id_dbs(void);

This function seems to introduce unnecessary call indirection seeing that hip_uninit_hostid_db() does exactly the same.

Otherwise, this merge proposal seems good to me. However, you made changes to core HI handling components of HIPL, so please test your changes extensively. I suggest running tests in your own VMs including BEX and UPDATE exchanges, running Valgrind and making a test deployment in our testbed before committing these changes to trunk.

Thanks for these changes to core components!

--
Dipl.-Inform. Rene Hummen, Ph.D. Student
Chair of Communication and Distributed Systems
RWTH Aachen University, Germany
tel: [phone]
web: http://www.comsys.rwth-aachen.de/team/rene-hummen/

[Miika Komu (miika-iki)]

We don't have yet threading support, hence there's no need for locking. I think (most of) the old locking code should have been removed by now.

[Miika Komu (miika-iki)]

> Is there any call to hip_get_hostid_entry_by_lhi_and_algo() with a parameter different from
> HIP_ANY_ALGO? Furthermore, is there any call with a parameter different from (anon=) -1? If not,
> these point to another possibility of cleaning up the API.

I would encourage to keep it. We need it for RFC5201-bis and crypto agility.

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

Hi René!

Thanks for reviewing!

>> -    if (hip_get_host_id_and_priv_key(HIP_DB_LOCAL_HID, hit_our, HIP_HI_RSA,
>> +    if (hip_get_host_id_and_priv_key(hit_our, HIP_HI_RSA,
>>                                      &entry->our_pub, &entry->our_priv_key)) {
>> -        HIP_IFEL(hip_get_host_id_and_priv_key(HIP_DB_LOCAL_HID, hit_our,
>> -                                              HIP_HI_DSA, &entry->our_pub, &entry->our_priv_key),
>> +        HIP_IFEL(hip_get_host_id_and_priv_key(hit_our,
>> +                                              HIP_HI_DSA,
>> +                                              &entry->our_pub,
>> +                                              &entry->our_priv_key),
>>                  -1, "Local host identity not found\n");
>>     }
>
> Would it be possible to further abstract from the HI algorithm here? Right now, we need to look for an RSA key first and then fall back to DSA. What happens when we add ECDSA? We will need to add that case to many if not all calls to hip_get_host_id_and_priv_key().

Further abstraction is certainly possible but right now not the focus
of this branch. I have a keys branch for toying around with an
algorithm-agnostic interface to crypto functionality but that is at an
infant's stage. Expect it to be ready anytime around 2020 :)

>> === modified file 'hipd/hidb.c'
> [...]
>> -    id = hip_get_hostid_entry_by_lhi_and_algo(db, &hit, HIP_ANY_ALGO, -1);
>> +    id = hip_get_hostid_entry_by_lhi_and_algo(&hit, HIP_ANY_ALGO, -1);
>
> Is there any call to hip_get_hostid_entry_by_lhi_and_algo() with a parameter different from HIP_ANY_ALGO? Furthermore, is there any call with a parameter different from (anon=) -1? If not, these point to another possibility of cleaning up the API.

We cannot get rid of this function altogether but similar cleanup will
be part of future merge proposals on the HIDB API.

>> void hip_uninit_host_id_dbs(void);
>
> This function seems to introduce unnecessary call indirection seeing that hip_uninit_hostid_db() does exactly the same.

You are correct in terms of functionality but these are two different
interfaces, one for the HIDB, one for the LSIDB, and they should not
be mixed. A user of the HIDB (who might want to uninitialize it at
some point via this function) should not be exposed to the LSIDB API.
Performance is obviously not an issue here.

> Otherwise, this merge proposal seems good to me. However, you made changes to core HI handling components of HIPL, so please test your changes extensively. I suggest running tests in your own VMs including BEX and UPDATE exchanges, running Valgrind and making a test deployment in our testbed before committing these changes to trunk.

Will do that once the regression test framework is in place and the
current bugs are fixed. That might be a while.

Stefan

[David Martin (martin-lp)]

Hi,

> Thanks for the review!

You are welcome!

> I believe the locking is not used because:
>
> - The code does not compile when enabling locking (note that locking is
> disabled
> via a '#if 0')

oh, I missed the if condition there.

> - Locking was disabled by a commit in 2007 and I have no evidence that it was
> ever enabled again
>
> - The locking logic in trunk is badly broken. Exhibit 1: the core lookup
> function hip_get_hostid_entry_by_lhi_and_algo() does not use locking. Exhibit
> 2:
> hip_del_host_id() unlocks the database although the code path up to that point
> never acquires a lock. Exhibit 3: Some code paths in hip_del_host_id() unlock
> the database, some others do not. The list goes on...
>
> - There is no multi-threading in HIPL (... for all I know. But given the above
> reasons, I really really hope so, too)
>
> Thus, I think it makes a lot of sense to remove the locking code because it
> needs *major* re-working anyway.

Ok, sounds reasonable, I'm convinced. Thanks for clarifying!

> I wanted to address const correctness in one big commit, but anyway. Fixed in
> rev. 5954

Thanks.

> >> +/**
> >> + * A call-back function for finding a host association between a given
> peer HIT
> >> + * and any of the iteratively provided local HIs.
> >> + *
> >> + * @param lhi Points to a local_host_id object from which contains the
> >> + * local HIT of the HA to find.
> >
> > "Points to a local_host_id object which contains"
>
> Oops. Fixed in rev. 5953

:)

> >> -int hip_hidb_match(const void *ptr1, const void *ptr2)
> >> +static int hip_hidb_match(const void *ptr1, const void *ptr2)
> >
> > *const ptr1 and *const ptr2 would be more correct?
>
> Fixed in rev. 5954

> Thanks! If this addresses all your concerns, please do not forget to change
> your
> vote to 'accept'.

My question has been answered and the minor issues fixed. All good by me! :)

David

review: Approve

[Diego Biurrun (diego-biurrun)]

 review needs-fixing

On Fri, May 20, 2011 at 09:26:05PM +0000, Stefan Götz wrote:
> Stefan Götz has proposed merging lp:~stefan.goetz/hipl/hidb-db into lp:hipl.
>
> --- firewall/user_ipsec_sadb.c 2011-04-05 16:44:22 +0000
> +++ firewall/user_ipsec_sadb.c 2011-05-20 21:25:59 +0000
> @@ -824,29 +823,38 @@
>
> +/**
> * flushes all entries in the sadb
> *
> - * @return -1, if error occurred, else 0
> + * @return 0
> */
> int hip_sadb_flush(void)
> {
> - int err = 0, i = 0;
> - LHASH_NODE *item = NULL, *tmp = NULL;
> - struct hip_sa_entry *entry = NULL;
> -
> - // iterating over all elements
> - list_for_each_safe(item, tmp, sadb, i)
> - {
> - HIP_IFEL(!(entry = list_entry(item)), -1,
> - "failed to get list entry\n");
> - HIP_IFEL(hip_sa_entry_delete(&entry->inner_src_addr, &entry->inner_dst_addr), -1,
> - "failed to delete sa entry\n");
> - }
> + hip_ht_doall(sadb, delete_sa_entry);
>
> HIP_DEBUG("sadb flushed\n");

This debug line is rather pointless IMO, I'd remove it.

> -out_err:
> - return err;
> + return 0;
> }

Why not make the function void instead of always returning the same value?

> --- hipd/cookie.c 2011-05-10 22:14:13 +0000
> +++ hipd/cookie.c 2011-05-20 21:25:59 +0000
> @@ -372,24 +367,10 @@
> /**
> * precreate all R1 packets
> *
> - * @return zero on success or negative on error
> + * @return 0
> */
> int hip_recreate_all_precreated_r1_packets(void)
> {
> - HIP_HASHTABLE *ht = hip_ht_init(hip_hidb_hash, hip_hidb_match);
> - LHASH_NODE *curr, *iter;
> - struct hip_host_id *tmp;
> - int c;
> -
> - hip_for_each_hi(hip_recreate_r1s_for_entry_move, ht);
> -
> - list_for_each_safe(curr, iter, ht, c)
> - {
> - tmp = list_entry(curr);
> - hip_ht_add(HIP_DB_LOCAL_HID, tmp);
> - list_del(tmp, ht);
> - }
> -
> - hip_ht_uninit(ht);
> + hip_for_each_hi(hip_recreate_r1s_for_entry_move, NULL);
> return 0;
> }

ditto

> --- hipd/init.c 2011-05-04 16:20:00 +0000
> +++ hipd/init.c 2011-05-20 21:25:59 +0000
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2010 Aalto University and RWTH Aachen University.
> + * Copyright (c) 2010-2011 Aalto University and RWTH Aachen University.
> *
> * Permission is hereby granted, free of charge, to any person
> * obtaining a copy of this software and associated documentation
> @@ -28,6 +28,7 @@
> * This file defines initialization functions for the HIP daemon.
> *
> * @note HIPU: BSD platform needs to be autodetected in hip_set_lowcapability
> + * @author Stefan G??tz <email address hidden>
> */

I'm sceptical that removing a function constitutes a real contribution.

There are more similar cases in this merge request, we need to come to
a conclusion what to do with these cases.

Diego

review: Needs Fixing

[David Martin (martin-lp)]

Hi,

On Mon, Aug 8, 2011 at 5:00 PM, Diego Biurrun <email address hidden> wrote:
> review needs-fixing
>
> On Fri, May 20, 2011 at 09:26:05PM +0000, Stefan Götz wrote:
>> Stefan Götz has proposed merging lp:~stefan.goetz/hipl/hidb-db into lp:hipl.
>>
>> HIP_DEBUG("sadb flushed\n");
>
> This debug line is rather pointless IMO, I'd remove it.
>
>> -out_err:
>> - return err;
>> + return 0;
>> }
>
> Why not make the function void instead of always returning the same value?
>
> There are more similar cases in this merge request, we need to come to
> a conclusion what to do with these cases.

well, shouldn't a merge review focus on the changes to be merged more than
on general problems with the code around it? Your points are valid but if I
see it right those parts of the code weren't touched by Stefan and in my
opinion unrelated to the goal of this branch.

There's the boyscout rule but you can only apply it so far else you never get
done with what you wanted to do in the first place or you needlessly bloat
the merge request, no?

[Diego Biurrun (diego-biurrun)]

On Wed, Aug 10, 2011 at 02:14:13PM +0000, David Martin wrote:
>
> On Mon, Aug 8, 2011 at 5:00 PM, Diego Biurrun <email address hidden> wrote:
> > review needs-fixing
> >
> > On Fri, May 20, 2011 at 09:26:05PM +0000, Stefan Götz wrote:
> >> Stefan Götz has proposed merging lp:~stefan.goetz/hipl/hidb-db into lp:hipl.
> >>
> >> HIP_DEBUG("sadb flushed\n");
> >
> > This debug line is rather pointless IMO, I'd remove it.
> >
> >> -out_err:
> >> - return err;
> >> + return 0;
> >> }
> >
> > Why not make the function void instead of always returning the same value?
> >
> > There are more similar cases in this merge request, we need to come to
> > a conclusion what to do with these cases.
>
> well, shouldn't a merge review focus on the changes to be merged more than
> on general problems with the code around it? Your points are valid but if I
> see it right those parts of the code weren't touched by Stefan and in my
> opinion unrelated to the goal of this branch.
>
> There's the boyscout rule but you can only apply it so far else you never get
> done with what you wanted to do in the first place or you needlessly bloat
> the merge request, no?

He is changing the whole function anyway, so this seemed relevant...

That said, yes, one has to focus, otherwise you never get finished.

Diego

[Diego Biurrun (diego-biurrun)]

 review approve

On Tue, Jan 17, 2012 at 09:02:28AM +0000, Stefan Götz wrote:
> Stefan Götz has proposed merging lp:~stefan.goetz/hipl/hidb-db into lp:hipl.
>
> --- hipd/init.c 2011-12-13 13:50:53 +0000
> +++ hipd/init.c 2012-01-17 09:01:25 +0000
> @@ -28,6 +28,7 @@
> * This file defines initialization functions for the HIP daemon.
> *
> * @note HIPU: BSD platform needs to be autodetected in hip_set_lowcapability
> + * @author Stefan Götz <email address hidden>
> */

We dropped all of these. Please remove before merging.

Diego

review: Approve

[lp:~stefan.goetz-deactivatedaccount/hipl/hidb-db]

6259. By Stefan Götz on 2012-01-18

Remove authorship attribution

[René Hummen (rene-hummen)]

LGTM. I like the removal of the duplicate functionality in return_first_rsa().

[René Hummen (rene-hummen)]

review: Approve

=== modified file 'hipd/cert.c'
--- hipd/cert.c	2012-01-14 15:29:47 +0000
+++ hipd/cert.c	2012-01-18 21:24:26 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010-2011 Aalto University and RWTH Aachen University.
+ * Copyright (c) 2010-2012 Aalto University and RWTH Aachen University.
  *
  * Permission is hereby granted, free of charge, to any person
  * obtaining a copy of this software and associated documentation
@@ -104,8 +104,10 @@
 
     HIP_DEBUG_HIT("Getting keys for HIT", &cert->issuer_hit);
 
-    HIP_IFEL(hip_get_host_id_and_priv_key(hip_local_hostid_db, &cert->issuer_hit,
-                                          HIP_ANY_ALGO, &host_id, (void **) &rsa),
+    HIP_IFEL(hip_get_host_id_and_priv_key(&cert->issuer_hit,
+                                          HIP_ANY_ALGO,
+                                          &host_id,
+                                          (void **) &rsa),
              -1, "Private key not found\n");
 
     algo = host_id->rdata.algorithm;
@@ -802,8 +804,10 @@
 
     HIP_DEBUG("Getting the key\n");
 
-    HIP_IFEL(hip_get_host_id_and_priv_key(hip_local_hostid_db, issuer_hit_n,
-                                          HIP_ANY_ALGO, &host_id, (void *) &rsa),
+    HIP_IFEL(hip_get_host_id_and_priv_key(issuer_hit_n,
+                                          HIP_ANY_ALGO,
+                                          &host_id,
+                                          (void *) &rsa),
              -1, "Private key not found\n");
 
     algo = host_id->rdata.algorithm;

=== modified file 'hipd/cookie.c'
--- hipd/cookie.c	2012-01-06 13:19:09 +0000
+++ hipd/cookie.c	2012-01-18 21:24:26 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010-2011 Aalto University and RWTH Aachen University.
+ * Copyright (c) 2010-2012 Aalto University and RWTH Aachen University.
  *
  * Permission is hereby granted, free of charge, to any person
  * obtaining a copy of this software and associated documentation
@@ -196,8 +196,7 @@
     int                   idx, len;
 
     /* Find the proper R1 table and copy the R1 message from the table */
-    HIP_IFEL(!(hid = hip_get_hostid_entry_by_lhi_and_algo(HIP_DB_LOCAL_HID,
-                                                          our_hit, HIP_ANY_ALGO, -1)),
+    HIP_IFEL(!(hid = hip_get_hostid_entry_by_lhi_and_algo(our_hit, HIP_ANY_ALGO, -1)),
              NULL, "Unknown HIT\n");
 
     hip_r1table = hid->r1;
@@ -280,8 +279,7 @@
     int                      err = 0;
 
     /* Find the proper R1 table */
-    HIP_IFEL(!(hid = hip_get_hostid_entry_by_lhi_and_algo(HIP_DB_LOCAL_HID,
-                                                          &hdr->hitr,
+    HIP_IFEL(!(hid = hip_get_hostid_entry_by_lhi_and_algo(&hdr->hitr,
                                                           HIP_ANY_ALGO,
                                                           -1)),
              -1, "Requested source HIT not (any more) available.\n");

=== modified file 'hipd/hadb.c'
--- hipd/hadb.c	2011-12-30 23:20:44 +0000
+++ hipd/hadb.c	2012-01-18 21:24:26 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010-2011 Aalto University and RWTH Aachen University.
+ * Copyright (c) 2010-2012 Aalto University and RWTH Aachen University.
  *
  * Permission is hereby granted, free of charge, to any person
  * obtaining a copy of this software and associated documentation
@@ -274,8 +274,8 @@
  * A call-back function for finding a host association between a given peer HIT
  * and any of the iteratively provided local HIs.
  *
- * @param lhi     Points to a local_host_id object from which contains the
- *                local HIT of the HA to find.
+ * @param lhi     Points to a local_host_id object which contains the local HIT
+ *                of the HA to find.
  * @param context Points to a ha_pattern object which contains the peer HIT of
  *                the HA to find and where the result is stored if a HA is
  *                found.
@@ -972,20 +972,17 @@
      * Note, that hip_get_host_id() allocates a new buffer and this buffer
      * must be freed in out_err if an error occurs. */
 
-    if (!hip_get_host_id_and_priv_key(HIP_DB_LOCAL_HID,
-                                      hit_our,
+    if (!hip_get_host_id_and_priv_key(hit_our,
                                       HIP_HI_RSA,
                                       &entry->our_pub,
                                       &entry->our_priv_key)) {
         HIP_DEBUG("Found RSA host identity\n");
-    } else if (!hip_get_host_id_and_priv_key(HIP_DB_LOCAL_HID,
-                                             hit_our,
+    } else if (!hip_get_host_id_and_priv_key(hit_our,
                                              HIP_HI_DSA,
                                              &entry->our_pub,
                                              &entry->our_priv_key)) {
         HIP_DEBUG("Found DSA host identity\n");
-    } else if (!hip_get_host_id_and_priv_key(HIP_DB_LOCAL_HID,
-                                             hit_our,
+    } else if (!hip_get_host_id_and_priv_key(hit_our,
                                              HIP_HI_ECDSA,
                                              &entry->our_pub,
                                              &entry->our_priv_key)) {

=== modified file 'hipd/hidb.c'
--- hipd/hidb.c	2011-12-30 23:20:44 +0000
+++ hipd/hidb.c	2012-01-18 21:24:26 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010-2011 Aalto University and RWTH Aachen University.
+ * Copyright (c) 2010-2012 Aalto University and RWTH Aachen University.
  *
  * Permission is hereby granted, free of charge, to any person
  * obtaining a copy of this software and associated documentation
@@ -46,6 +46,7 @@
 #include "lib/core/hostid.h"
 #include "lib/core/hit.h"
 #include "lib/core/ife.h"
+#include "lib/core/list.h"
 #include "lib/core/prefix.h"
 #include "lib/core/protodefs.h"
 #include "lib/core/straddr.h"
@@ -223,7 +224,7 @@
  * @param ptr a pointer to a local_host_id structure
  * @return the calculated hash value
  */
-unsigned long hip_hidb_hash(const void *ptr)
+static unsigned long hip_hidb_hash(const void *ptr)
 {
     const hip_hit_t *hit = &((const struct local_host_id *) ptr)->hit;
     union {
@@ -249,7 +250,7 @@
  * @param ptr2 a pointer to local_host_id
  * @return zero on match or non-zero on unmatch
  */
-int hip_hidb_match(const void *ptr1, const void *ptr2)
+static int hip_hidb_match(const void *const ptr1, const void *const ptr2)
 {
     const hip_hit_t *hit1 = &((const struct local_host_id *) ptr1)->hit;
     const hip_hit_t *hit2 = &((const struct local_host_id *) ptr2)->hit;
@@ -268,26 +269,19 @@
  * Deletes the given HI (network byte order) from the database. Matches HIs
  * based on the HIT.
  *
- * @param db  database from which to delete.
  * @param hit the HIT to be deleted from the database.
  * @return    zero on success, otherwise negative.
  */
-static int del_host_id(HIP_HASHTABLE *db, hip_hit_t hit)
+static int del_host_id(hip_hit_t hit)
 {
     struct local_host_id *id = NULL;
 
-    id = hip_get_hostid_entry_by_lhi_and_algo(db, &hit, HIP_ANY_ALGO, -1);
+    id = hip_get_hostid_entry_by_lhi_and_algo(&hit, HIP_ANY_ALGO, -1);
     if (id == NULL) {
         HIP_ERROR("hit not found\n");
         return -ENOENT;
     }
 
-    /* Call the handler to execute whatever required after the
-     * host id is no more in the database */
-    if (id->remove) {
-        id->remove(id, &id->arg);
-    }
-
     switch (hip_get_host_id_algo(&id->host_id)) {
     case HIP_HI_RSA:
         RSA_free(id->private_key);
@@ -304,7 +298,7 @@
         HIP_ERROR("Cannot free key because key type is unknown.\n");
     }
 
-    list_del(id, db);
+    list_del(id, hip_local_hostid_db);
     free(id);
     id = NULL;
 
@@ -315,25 +309,23 @@
  * Uninitializes local/peer Host Id table. All elements of the @c db are
  * deleted. Since local and peer host id databases include dynamically allocated
  * host_id element, it is also freed.
- *
- * @param db database structure to delete.
  */
-static void uninit_hostid_db(HIP_HASHTABLE *db)
+static void uninit_hostid_db(void)
 {
     LHASH_NODE           *curr, *iter;
     struct local_host_id *tmp;
     int                   count;
 
-    list_for_each_safe(curr, iter, db, count) {
+    list_for_each_safe(curr, iter, hip_local_hostid_db, count) {
         hip_hit_t hit;
 
         tmp = list_entry(curr);
 
         memcpy(&hit, &tmp->hit, sizeof(hit));
-        del_host_id(db, hit);
+        del_host_id(hit);
     }
 
-    hip_ht_uninit(db);
+    hip_ht_uninit(hip_local_hostid_db);
 }
 
 /**
@@ -342,22 +334,19 @@
  * If @c hit is NULL, finds the first used host id.
  * If algo is HIP_ANY_ALGO, ignore algore comparison.
  *
- * @param db   database to be searched. Usually either HIP_DB_PEER_HID or
- *             HIP_DB_LOCAL_HID
  * @param hit  the local host id to be searched
  * @param anon -1 if you don't care, 1 if anon, 0 if public
  * @param algo the algorithm
  * @return     NULL, if failed or non-NULL if succeeded.
  */
-struct local_host_id *hip_get_hostid_entry_by_lhi_and_algo(HIP_HASHTABLE *db,
-                                                           const struct in6_addr *hit,
-                                                           int algo,
-                                                           int anon)
+struct local_host_id *hip_get_hostid_entry_by_lhi_and_algo(const struct in6_addr *const hit,
+                                                           const int algo,
+                                                           const int anon)
 {
     struct local_host_id *id_entry;
     LHASH_NODE           *item;
     int                   c;
-    list_for_each(item, db, c) {
+    list_for_each(item, hip_local_hostid_db, c) {
         id_entry = list_entry(item);
 
         if ((hit == NULL || !ipv6_addr_cmp(&id_entry->hit, hit)) &&
@@ -379,8 +368,7 @@
  */
 int hip_hidb_hit_is_our(const hip_hit_t *our)
 {
-    return hip_get_hostid_entry_by_lhi_and_algo(hip_local_hostid_db, our,
-                                                HIP_ANY_ALGO, -1) != NULL;
+    return hip_get_hostid_entry_by_lhi_and_algo(our, HIP_ANY_ALGO, -1) != NULL;
 }
 
 /**
@@ -409,11 +397,10 @@
 /**
  * Assign a free LSI to a host id entry
  *
- * @param db database structure
  * @param id_entry contains an entry to the db, will contain an unsigned lsi
  * @return zero on success, or negative error value on failure.
  */
-static int hidb_add_lsi(HIP_HASHTABLE *db, struct local_host_id *id_entry)
+static int hidb_add_lsi(struct local_host_id *id_entry)
 {
     struct local_host_id *id_entry_aux;
     LHASH_NODE           *item;
@@ -425,7 +412,7 @@
         inet_aton(lsi_addresses[i], &lsi_aux);
         used_lsi = 0;
 
-        list_for_each(item, db, c) {
+        list_for_each(item, hip_local_hostid_db, c) {
             id_entry_aux = list_entry(item);
             if (hip_lsi_are_equal(&lsi_aux, &id_entry_aux->lsi)) {
                 used_lsi = 1;
@@ -463,28 +450,23 @@
  */
 void hip_uninit_host_id_dbs(void)
 {
-    uninit_hostid_db(hip_local_hostid_db);
+    uninit_hostid_db();
 }
 
 /**
  * Adds the given HI into the database. Checks for duplicates. If one is found,
  * the current HI is @b NOT stored.
  *
- * @param db      database structure.
  * @param hit     HIT
  * @param anon    whether the host ID is anonymous or not
+ * @param lsi     the LSI
  * @param host_id HI
- * @param add     the handler to call right after the host id is added
- * @param del     the handler to call right before the host id is removed
- * @param arg     argument passed for the handlers
- * @param lsi     the LSI
  * @return        0 on success, otherwise an negative error value is returned.
  */
-static int add_host_id(HIP_HASHTABLE *db, const hip_hit_t hit, bool anon,
-                       hip_lsi_t *lsi, const struct hip_host_id_priv *host_id,
-                       int (*add)(struct local_host_id *, void **arg),
-                       int (*del)(struct local_host_id *, void **arg),
-                       void *arg)
+static int add_host_id(const hip_hit_t hit,
+                       bool anon,
+                       hip_lsi_t *lsi,
+                       const struct hip_host_id_priv *host_id)
 {
     int                   err      = 0;
     struct local_host_id *id_entry = NULL;
@@ -498,8 +480,7 @@
     id_entry->anonymous = anon;
 
     /* check for duplicates */
-    old_entry = hip_get_hostid_entry_by_lhi_and_algo(db, &hit,
-                                                     HIP_ANY_ALGO, -1);
+    old_entry = hip_get_hostid_entry_by_lhi_and_algo(&hit, HIP_ANY_ALGO, -1);
     if (old_entry != NULL) {
         HIP_ERROR("Trying to add duplicate local host ID\n");
         err = -EEXIST;
@@ -507,14 +488,11 @@
     }
 
     /* assign a free lsi address */
-    HIP_IFEL(hidb_add_lsi(db, id_entry) < 0, -EEXIST, "No LSI free\n");
+    HIP_IFEL(hidb_add_lsi(id_entry) < 0, -EEXIST, "No LSI free\n");
 
     memcpy(lsi, &id_entry->lsi, sizeof(hip_lsi_t));
-    id_entry->insert = add;
-    id_entry->remove = del;
-    id_entry->arg    = arg;
 
-    list_add(id_entry, db);
+    list_add(id_entry, hip_local_hostid_db);
 
     switch (hip_get_host_id_algo((const struct hip_host_id *) host_id)) {
     case HIP_HI_RSA:
@@ -560,12 +538,6 @@
              -ENOENT,
              "Unable to precreate R1s.\n");
 
-    /* Called while the database is locked, perhaps not the best
-     * option but HIs are not added often */
-    if (add) {
-        add(id_entry, &arg);
-    }
-
 out_err:
     if (err && id_entry) {
         switch (hip_get_host_id_algo(&id_entry->host_id)) {
@@ -639,8 +611,7 @@
 
         anonymous = (eid_endpoint->endpoint.flags & HIP_ENDPOINT_FLAG_ANON);
 
-        err = add_host_id(HIP_DB_LOCAL_HID, hit, anonymous, &lsi,
-                          host_identity, NULL, NULL, NULL);
+        err = add_host_id(hit, anonymous, &lsi, host_identity);
 
         /* Currently only RSA pub is added by default (bug id 592127).
          * Ignore redundant adding in case user wants to enable
@@ -699,7 +670,7 @@
     hip_in6_ntop(hit, buf);
     HIP_INFO("del HIT: %s\n", buf);
 
-    if ((err = del_host_id(HIP_DB_LOCAL_HID, *hit))) {
+    if ((err = del_host_id(*hit))) {
         HIP_ERROR("deleting of local host identity failed\n");
         return err;
     }
@@ -723,8 +694,7 @@
 {
     struct local_host_id *entry;
 
-    entry = hip_get_hostid_entry_by_lhi_and_algo(hip_local_hostid_db,
-                                                 NULL, algo, anon);
+    entry = hip_get_hostid_entry_by_lhi_and_algo(NULL, algo, anon);
     if (!entry) {
         return -ENOENT;
     }
@@ -787,18 +757,16 @@
 /**
  * find the local host identifier corresponding to the local LSI
  *
- * @param db the local host identifier database to be searched for
  * @param lsi the local LSI to be matched
  * @return the local host identifier structure
  */
-static struct local_host_id *hidb_get_entry_by_lsi(HIP_HASHTABLE *db,
-                                                   const struct in_addr *lsi)
+static struct local_host_id *hidb_get_entry_by_lsi(const struct in_addr *lsi)
 {
     struct local_host_id *id_entry;
     LHASH_NODE           *item;
     int                   c;
 
-    list_for_each(item, db, c) {
+    list_for_each(item, hip_local_hostid_db, c) {
         id_entry = list_entry(item);
         if (!ipv4_addr_cmp(&id_entry->lsi, lsi)) {
             return id_entry;
@@ -826,14 +794,13 @@
         return -1;
     }
     if (ipv4_addr_cmp(&aux_lsi, default_lsi)) {
-        if (!(tmp1 = hip_get_hostid_entry_by_lhi_and_algo(HIP_DB_LOCAL_HID,
-                                                          default_hit,
+        if (!(tmp1 = hip_get_hostid_entry_by_lhi_and_algo(default_hit,
                                                           HIP_ANY_ALGO,
                                                           -1))) {
             HIP_ERROR("Default hit not found in hidb\n");
             return -1;
         }
-        if (!(tmp2 = hidb_get_entry_by_lsi(HIP_DB_LOCAL_HID, default_lsi))) {
+        if (!(tmp2 = hidb_get_entry_by_lsi(default_lsi))) {
             HIP_ERROR("Default lsi not found in hidb\n");
             return -1;
         }
@@ -848,20 +815,21 @@
 /**
  * find a host identifier from the database
  *
- * @param db the host identifier databased
  * @param hit the HIT to be searched for
  * @param algo the algorithm for the HI
  * @param host_id A copy of the host is stored here. Caller deallocates.
  * @param key a pointer to the private key (caller should not deallocate)
  * @return zero on success or negative on error
  */
-int hip_get_host_id_and_priv_key(HIP_HASHTABLE *db, struct in6_addr *hit,
-                                 int algo, struct hip_host_id **host_id, void **key)
+int hip_get_host_id_and_priv_key(struct in6_addr *hit,
+                                 int algo,
+                                 struct hip_host_id **host_id,
+                                 void **key)
 {
     int                   host_id_len;
     struct local_host_id *entry = NULL;
 
-    entry = hip_get_hostid_entry_by_lhi_and_algo(db, hit, algo, -1);
+    entry = hip_get_hostid_entry_by_lhi_and_algo(hit, algo, -1);
     if (!entry) {
         return -1;
     }

=== modified file 'hipd/hidb.h'
--- hipd/hidb.h	2011-11-25 17:56:24 +0000
+++ hipd/hidb.h	2012-01-18 21:24:26 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010 Aalto University and RWTH Aachen University.
+ * Copyright (c) 2010,2012 Aalto University and RWTH Aachen University.
  *
  * Permission is hereby granted, free of charge, to any person
  * obtaining a copy of this software and associated documentation
@@ -26,12 +26,15 @@
 #ifndef HIPL_HIPD_HIDB_H
 #define HIPL_HIPD_HIDB_H
 
+/**
+ * @file
+ * Public interface for the HIDB, the database of local host identities (LHIs),
+ * i.e., all the HIP identities known for the local host.
+ */
+
 #include <stdbool.h>
 #include <netinet/in.h>
-#include <openssl/lhash.h>
 
-#include "lib/core/hashtable.h"
-#include "lib/core/list.h"
 #include "lib/core/protodefs.h"
 #include "cookie.h"
 
@@ -43,24 +46,14 @@
     struct hip_host_id host_id;
     void              *private_key;       /* RSA or DSA */
     struct hip_r1entry r1[HIP_R1TABLESIZE];       /* precreated R1s */
-    /* Handler to call after insert with an argument, return 0 if OK*/
-    int (*insert)(struct local_host_id *, void **arg);
-    /* Handler to call before remove with an argument, return 0 if OK*/
-    int   (*remove)(struct local_host_id *, void **arg);
-    void *arg;
 };
 
-/* Use this to point your target while accessing a database */
-#define HIP_DB_LOCAL_HID   (hip_local_hostid_db)
-
-/* ... and not this! */
-extern HIP_HASHTABLE *hip_local_hostid_db;
-
-struct local_host_id *hip_get_hostid_entry_by_lhi_and_algo(HIP_HASHTABLE *db,
-                                                           const struct in6_addr *hit,
-                                                           int algo, int anon);
-int hip_get_host_id_and_priv_key(HIP_HASHTABLE *db, struct in6_addr *hit,
-                                 int algo, struct hip_host_id **host_id, void **key);
+struct local_host_id *hip_get_hostid_entry_by_lhi_and_algo(const struct in6_addr *const hit,
+                                                           const int algo, const int anon);
+int hip_get_host_id_and_priv_key(struct in6_addr *hit,
+                                 int algo,
+                                 struct hip_host_id **host_id,
+                                 void **key);
 
 void hip_uninit_host_id_dbs(void);
 
@@ -77,8 +70,6 @@
 /* existence */
 int hip_hidb_hit_is_our(const hip_hit_t *src);
 
-unsigned long hip_hidb_hash(const void *ptr);
-int hip_hidb_match(const void *ptr1, const void *ptr2);
 void hip_init_hostid_db(void);
 int hip_get_default_hit(struct in6_addr *hit);
 int hip_get_default_hit_msg(struct hip_common *msg);

=== modified file 'hipd/init.c'
--- hipd/init.c	2011-12-13 13:50:53 +0000
+++ hipd/init.c	2012-01-18 21:24:26 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010-2011 Aalto University and RWTH Aachen University.
+ * Copyright (c) 2010-2012 Aalto University and RWTH Aachen University.
  *
  * Permission is hereby granted, free of charge, to any person
  * obtaining a copy of this software and associated documentation
@@ -373,36 +373,6 @@
 }
 
 /**
- * find the first RSA-based host id
- *
- * @return the host id or NULL if none found
- */
-static struct local_host_id *return_first_rsa(void)
-{
-    LHASH_NODE           *curr, *iter;
-    struct local_host_id *tmp = NULL;
-    int                   c;
-    uint16_t              algo = 0;
-
-    list_for_each_safe(curr, iter, hip_local_hostid_db, c) {
-        tmp = list_entry(curr);
-        HIP_DEBUG_HIT("Found HIT", &tmp->hit);
-        algo = hip_get_host_id_algo(&tmp->host_id);
-        HIP_DEBUG("hits algo %d HIP_HI_RSA = %d\n",
-                  algo, HIP_HI_RSA);
-        if (algo == HIP_HI_RSA) {
-            goto out_err;
-        }
-    }
-
-out_err:
-    if (algo == HIP_HI_RSA) {
-        return tmp;
-    }
-    return NULL;
-}
-
-/**
  * Initialize local host IDs.
  *
  * @return zero on success or negative on failure
@@ -489,7 +459,7 @@
         HIP_DEBUG("Configuration file did NOT exist creating it and "
                   "filling it with default information\n");
         /* Fetch the first RSA HIT */
-        entry = return_first_rsa();
+        entry = hip_get_hostid_entry_by_lhi_and_algo(NULL, HIP_HI_RSA, -1);
         if (entry == NULL) {
             HIP_DEBUG("Failed to get the first RSA HI");
             goto out_err;

=== modified file 'hipd/output.c'
--- hipd/output.c	2011-12-29 10:00:51 +0000
+++ hipd/output.c	2012-01-18 21:24:26 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010-2011 Aalto University and RWTH Aachen University.
+ * Copyright (c) 2010-2012 Aalto University and RWTH Aachen University.
  *
  * Permission is hereby granted, free of charge, to any person
  * obtaining a copy of this software and associated documentation
@@ -453,8 +453,7 @@
         /* add host id in plaintext without encrypted wrapper */
         /* Parameter HOST_ID. Notice that hip_get_public_key overwrites
          * the argument pointer, so we have to allocate some extra memory */
-        HIP_IFEL(!(host_id_entry = hip_get_hostid_entry_by_lhi_and_algo(HIP_DB_LOCAL_HID,
-                                                                        &ctx->input_msg->hitr,
+        HIP_IFEL(!(host_id_entry = hip_get_hostid_entry_by_lhi_and_algo(&ctx->input_msg->hitr,
                                                                         HIP_ANY_ALGO,
                                                                         -1)),
                  -1, "Unknown HIT\n");

=== modified file 'modules/midauth/hipd/midauth.c'
--- modules/midauth/hipd/midauth.c	2011-10-17 15:22:35 +0000
+++ modules/midauth/hipd/midauth.c	2012-01-18 21:24:26 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010-2011 Aalto University and RWTH Aachen University.
+ * Copyright (c) 2010-2012 Aalto University and RWTH Aachen University.
  *
  * Permission is hereby granted, free of charge, to any person
  * obtaining a copy of this software and associated documentation
@@ -125,8 +125,7 @@
     // add HOST_ID to packets containing a CHALLENGE_RESPONSE
     if (challenge_request) {
         const struct local_host_id *const host_id_entry =
-            hip_get_hostid_entry_by_lhi_and_algo(HIP_DB_LOCAL_HID,
-                                                 &ctx->input_msg->hitr,
+            hip_get_hostid_entry_by_lhi_and_algo(&ctx->input_msg->hitr,
                                                  HIP_ANY_ALGO,
                                                  -1);
         if (!host_id_entry) {

=== modified file 'modules/update/hipd/update.c'
--- modules/update/hipd/update.c	2011-12-14 11:17:58 +0000
+++ modules/update/hipd/update.c	2012-01-18 21:24:26 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010-2011 Aalto University and RWTH Aachen University.
+ * Copyright (c) 2010-2012 Aalto University and RWTH Aachen University.
  *
  * Permission is hereby granted, free of charge, to any person
  * obtaining a copy of this software and associated documentation
@@ -52,6 +52,7 @@
 #include "lib/core/debug.h"
 #include "lib/core/hip_udp.h"
 #include "lib/core/ife.h"
+#include "lib/core/list.h"
 #include "lib/core/modularization.h"
 #include "lib/core/prefix.h"
 #include "lib/core/state.h"