Code review comment for lp:~stefan.goetz-deactivatedaccount/hipl/hidb-db

Revision history for this message
René Hummen (rene-hummen) wrote :

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

« Back to merge proposal