[hipl-dev] Re: [Merge] lp:~stefan.goetz/hipl/hidb-db into lp:hipl
- From: David Martin <david.martin.mailbox@xxxxxxxxxxxxxx>
- To: mp+61832@xxxxxxxxxxxxxxxxxx
- Date: Wed, 15 Jun 2011 13:48:34 -0000
Review: Approve
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
--
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:
