> From: Jonathan Ruano
> Sent: Tuesday, 17 June 2003 10:04 PM
> I'm debugging rlm_ippool, trying to catch the bug that causes
> ips to disappear..
(CC'd to -devel since this is leading towards a patch from me... :-)
I'm just having a look at it myself, and on first glance the
mutex locking is too fine grained, protecting the GDBM file
itself, but not the transactions being performed...
Just looking at the code, I think Multilink PPP is broken
too, since if we find an active==0 entry, we break out of
the loop, even if searching further would discover the
matching entry for Multilink PPP.
My current thought is that the module would be better served
by _one_ GDBM database, indexed by IP address. The current
system of having (nas,port) index into the IP address list
is (I think) supposed to save walking the entire database
each check, but supporting MLPPP requires almost exactly
that...
<thinks>
Maybe a DB indexed by IP address, and one indexed by CLI/NAS?
<thinks more>
Dunno, gonna need some more thought on that one, and see if
we can avoid walking the whole DB on _all_ paths:
Post-auth:
<DB Lock>
Stale NAS/Port: Lookup NAS,port; get old IP
(If there _was_ a NAS,port entry... Deallocate:)
Delete NAS,port;
Lookup IP; get oldCLI
Lookup oldCLI,NAS; decrement usage
delete if usage == 0
Lookup IP; mark inactive if deleted from (CLI,NAS)
Multilink PPP check: Lookup CLI,NAS; get current ML-PPP IP
else Find unallocated IP... <== Longest walk!!
Allocation: Lookup IP; record active, cli, NAS
Create NAS,port; record IP
Lookup CLI,NAS; increment usage or create entry
<DB unlock>
Accounting:
<DB lock>
Deallocation: Delete NAS,port;
Lookup IP; get oldCLI
Lookup oldCLI,NAS; decrement usage
delete if usage == 0
Lookup IP; mark inactive if deleted from (CLI,NAS)
<DB unlock>
DBs:
(cli,nas): ipaddr, usage
(nas,port): ipaddr
(ipaddr): cli, nas, active
Where the (cli,nas) and (nas,port) tables are only containing active
entries, and the (ipaddr) table never has entries removed.
Entries are cleaned when either a stop-record for that NAS/port or
an Auth record for that NAS/port are seen.
Each NAS,port can only have one IP address.
Each cli,NAS can have one IP address assigned to multiple ports
Each IPadress can have one or zero CLI, NAS and be assigned to multiple ports
Big locks aren't bad to my mind here, since we're not walking the
entire table anyway, which would be a step up from the current code.
In fact, only once do we need to walk rather than looking up by index...
Which worries me that I've missed something.
Hopefully this would make the next step easier (or at least possible)
of altering the tables without having to delete and recreate them. At
least _adding_ to the IP pool would be easier... Deleting has problems
when the IPs to be deleted are in use... Maybe just skip 'em until
_next_ restart.
(And yes, I _am_ volunteering for this one... So I'd appreciate anyone
banging on the ideas here and telling me in what way I've been stupid.
Patch ETA is "over the weekend")
Anyway, to reanswer the originally asked question, first glance
is that the mutexes need to be expanded to cover whole transactions
(ie subtracting one from the usage marker in the data->ip DB)
instead of the current query by query locking. It may not fix the
problem you're seeing, but it _is_ a problem waiting to happen. And
as far as I can see, that would unify the mutexes in rlm_ippool.c
into a single mutex.
--
=========================================================
Paul "TBBle" Hampson
Bubblesworth Pty Ltd (ABN: 51 095 284 361)
[EMAIL PROTECTED]
The Creation of the Universe was made
possible by a grant from Texas Instruments.
-- PBS
---------------------------------------------------------
Random signature generator 3.0 by Paul "TBBle" Hampson
=========================================================
-
List info/subscribe/unsubscribe? See http://www.freeradius.org/list/users.html
