[hipl-dev] Re: [Merge] lp:~stefan.goetz/hipl/hidb-db into lp:hipl
- From: René Hummen <rene.hummen@xxxxxxxxxxxxxxxxx>
- To: mp+61832@xxxxxxxxxxxxxxxxxx
- Date: Sun, 05 Jun 2011 11:46:24 -0000
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: +49 241 80 20772
web: http://www.comsys.rwth-aachen.de/team/rene-hummen/
--
https://code.launchpad.net/~stefan.goetz/hipl/hidb-db/+merge/61832
Your team HIPL core team is subscribed to branch lp:hipl.
Other related posts:
