1.1.7 rlm_detail locking is broken

[Brian De Wolf]

Thanks for the quick response!

On 12/03/07 16:40, Nicolas Baradakis wrote:
>> Now, instead of radrelay's previous behavior, radrelay will attempt to
>> open and lock <detail>.work.  If it can, it will then read/transmit all
>> of the entries and delete <detail>.work.  If it can't lock, it will
>> simply try locking later.
> 
> The new algorith doesn't require a non-blocking lock in a loop. After the
> file is renamed rlm_detail will not try to lock it anymore.

Well, I thought the lock was not to prevent rlm_detail from writing, but to 
prevent radrelay from reading before rlm_detail was finished writing.

>> If it doesn't exist, it will check if <detail> has entries and if so
>> it will move <detail> to <detail>.work (and make a new <detail>).
> 
> rlm_detail will create the new detail file. I think the function
> detail_move() is not needed anymore. Just use a normal rename().

I left that in there because there was an exit condition if radrelay couldn't 
open either the work file or the original file.  I eventually took out that exit 
condition altogether, but didn't take out the file creation for some reason...

> This is only cosmetics.

Oops...I commented out the file creation, but as mentioned above, put it back in 
for the time being.  Out it goes again...

> Maybe re-use the comments from Alan in src/main/detail.c:

That sounds a lot better than mine, so sure!

>> +				if(!stat(r_args->detail, &st)) {
>> +					if(st.st_size > 0) {
>> +						detail_move(r_args->detail, work);
> 
> As said above, I think you could just use rename() and then you don't
> need to check st_size > 0.

I was thinking about that, but I didn't want to make a loop where <detail> kept 
appearing as an empty file and I kept moving it away.

>> +						continue;
> 
> I have a question about this "continue": please check that radrelay will
> not use 100% cpu when there is no detail file to read. (as many of the
> sleep() are gone)

I used continue so that, once <detail> was renamed to <detail>.work, the loop 
would start over and open <detail>.work.  However, this makes assumptions that 
this block of code will always be at the beginning of the loop, so I'm going to 
change 'continue' to the fopen that I was taking the long way to get to.

> Please no line of more than 80 characters in comments.

Oops, sorry about that.  My co-worker just showed me the "gq" command in vim, 
though, so it shouldn't happen any more...

>> +			 * fseek to the start of the file.
>> +			 * 
>> +			 * If we can't lock the detail file, we close it and
>> +			 * will try it again on the next iteration.  Note that
>> +			 * we will never unlock the file, on purpose, to stop
>> +			 * rlm_detail from reading it.
>> +			 */
> 
> The last part is confusing. rlm_detail must not try to acquire the lock
> anymore after the file is renamed to detail.work.

Oops, that's not confusing, that's just wrong altogether :).  I meant to say 
lock and write.  However, this seems to be well explained by the comment from 
src/main/detail.c.

> No need to lock in a loop, so you can simplify that part. The lock is
> only there to catch a corner case where the rename() happened while
> the current rlm_detail was still writing in the file. (but new rlm_detail
> execution will create and use a new file)

Good point, revised.  I thought about making this a blocking lock (rlm_detail 
should release it quickly, I would hope) but it's probably better to let 
radrelay run through the other parts of the loop while waiting.

I've attached a revised patch.  I've dropped it in, and it works as expected. 
It doesn't peg the CPU at all, I assume it spends most of its time in the 
select's waiting on network traffic.  I almost want to add some more sleeping 
though, it's hard to see the files appear and disappear, they move so fast... :) 
  I'll see how it responds during the day tomorrow, load is lighter right now 
(wireless users).
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: radrelay.diff
URL: <http://lists.freeradius.org/pipermail/freeradius-devel/attachments/20071203/46456ff1/attachment.ksh>