[hipl-dev] Re: [Merge] lp:~stefan.goetz/hipl/hidb-db into lp:hipl
- From: Stefan Götz <stefan.goetz@xxxxxxxxxxxxxxxxx>
- To: mp+61832@xxxxxxxxxxxxxxxxxx
- Date: Fri, 03 Jun 2011 21:18:53 -0000
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
--
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:
