sqlcounter returning wrong value?
liran tal
liransgarage at gmail.com
Wed Oct 29 22:59:39 CET 2008
Thanks for the patch, I have not checked it yet.
What is the status on this issue though? A patch is nice but I'd like to see
support for data counters
to find it's place in the newer version as well as the old one (well, a
backport would be nice. Many of us still have
production systems running 1.1.X)
What still disturbs me is that many of the hotspot forums (coovachilli and
chillispot) has post on how to add an sqlcounter
which performs these data queries "successfully" and user's feedback is that
everything is good and working. Not that it's
something to count on but I'd guess if somsone had an issue then we'd know
about it...
Regards,
Liran.
2008/10/25 Venkatesh K <kaevee at gmail.com>
> I have had hard time using sqlcounter for data. It has (at least had)
> limitations for counting data like 2GB limit. I ended up writing my
> own code for counting data.
>
> Here is a small patch which will skip adding additional quota if
> "counter-type=time" in sqlcounter.conf file.
>
>
> ----------------------------------------------------------------------------------------------------------------
> diff -u
> ../../../freeradius-1.1.7.orig/src/modules/rlm_sqlcounter/rlm_sqlcounter.c
> rlm_sqlcounter/rlm_sqlcounter.c
> ---
> ../../../freeradius-1.1.7.orig/src/modules/rlm_sqlcounter/rlm_sqlcounter.c
> 2007-04-08
> 04:51:42.000000000 +0530
> +++ rlm_sqlcounter/rlm_sqlcounter.c 2008-10-25 22:05:58.000000000 +0530
> @@ -72,6 +72,7 @@
> char *sqlmod_inst; /* instance of SQL module to use, usually
> just 'sql' */
> char *query; /* SQL query to retrieve current session
> time */
> char *reset; /* daily, weekly, monthly, never or user
> defined */
> + char *counter_type; /* Type of counter (data / time) */
> char *allowed_chars; /* safe characters list for SQL queries */
> time_t reset_time;
> time_t last_reset;
> @@ -97,6 +98,7 @@
> { "sqlmod-inst", PW_TYPE_STRING_PTR,
> offsetof(rlm_sqlcounter_t,sqlmod_inst), NULL, NULL },
> { "query", PW_TYPE_STRING_PTR, offsetof(rlm_sqlcounter_t,query),
> NULL, NULL },
> { "reset", PW_TYPE_STRING_PTR, offsetof(rlm_sqlcounter_t,reset),
> NULL, NULL },
> + { "counter-type", PW_TYPE_STRING_PTR,
> offsetof(rlm_sqlcounter_t,counter_type), NULL, NULL },
> { "safe-characters", PW_TYPE_STRING_PTR,
> offsetof(rlm_sqlcounter_t,allowed_chars), NULL,
> "@abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789.-_:
> /"},
> { NULL, -1, 0, NULL, NULL }
> };
> @@ -675,10 +677,13 @@
> * limit, so that the user will not need to
> * login again
> */
> - if (data->reset_time && (
> - res >= (data->reset_time - request->timestamp))) {
> - res = data->reset_time - request->timestamp;
> - res += check_vp->lvalue;
> + if(strcmp(data->counter_type,"time")==0)
> + {
> + if (data->reset_time && (
> + res >= (data->reset_time -
> request->timestamp))) {
> + res = data->reset_time -
> request->timestamp;
> + res += check_vp->lvalue;
> + }
> }
>
> if ((reply_item = pairfind(request->reply->vps,
> data->reply_attr)) != NULL) {
>
> ---------------------------------------------------------------------------------------------------------------------------------------------
>
> I did compile the code successfully. I have not checked it. Let me
> know if you have
> any issues.
>
> Thanks,
>
> Venkatesh K
>
> 2008/10/25 <tnt at kalik.net>:
> > And they won't. It's nothing to do with the settings - it's this peace
> > of the code.
> >
> > Let's take your example. Limit was 26MB and about 2MB was left.
> > 2,000,000 seconds is about 23 days. So this part of the code will "kick
> > in" (there are 6 days left in this month) and returned value will be
> > 26MB + number of seconds untill 1.11. Run debug twice. You will see that
> > the returned value will be reduced by the number of seconds between two
> > requests.
> >
> > Allowance left would have to be well below 1MB for data counter to start
> > working properly for monthly reset towards the end of the month, 100KB
> > for weekly reset and 10KB for daily. Which is of no practical use.
> >
> > I don't know when was this part of the code added but data counters
> > would work fine without it. I was testing time counter and still
> > reproduced this only because limit value was so high (10 million). When
> > I reduced it to 10,000 it worked fine.
> >
> > Ivan Kalik
> > Kalik Informatika ISP
> >
> >
> > Dana 25/10/2008, "liran tal" <liransgarage at gmail.com> piše:
> >
> >>I've actually tested the sqlcounter for a data counter for both weekly
> and
> >>monthly resets and that doesn't work well either...
> >>This is odd because people have reported this working so... it is either
> >>something in the configuration that I'm missing
> >>or I really don't know what's going on but the counter seem to behave as
> >>expected.
> >>
> >>
> >>Regards,
> >>Liran.
> >>
> >>2008/10/25 <tnt at kalik.net>
> >>
> >>> OK. This where the "problem" comes from:
> >>>
> >>> /*
> >>> * If we are near a reset then add the next
> >>> * limit, so that the user will not need to
> >>> * login again
> >>> */
> >>> if (data->reset_time &&
> >>> (res >= (data->reset_time - request->timestamp))) {
> >>> res = data->reset_time - request->timestamp;
> >>> res += check_vp->vp_integer;
> >>> }
> >>>
> >>> (that's rlm_sqlcounter.c line about 710 in 2.0.5)
> >>>
> >>> Sqlcounter was designed for time counters. When your allowance (limit -
> >>> time used) is greater than the time left in the period, time left in
> the
> >>> period is added to the limit and sent as reply.
> >>>
> >>> This will create problems for data counters since limit values are much
> >>> greater than those for time counters. As a workaround I would suggest
> >>> that you comment this out if you are using data counters.
> >>>
> >>> Perhaps there is a case for adding another configuration item to
> >>> sqlcounter that would determine what is counted time or data. Then for
> >>> data counters this can be skipped or replaced with something like:
> >>>
> >>> if (data->reset_time && (trigger >= (data->reset_time -
> >>> request->timestamp))) {
> >>> res += check_vp->vp_integer;
> >>> }
> >>>
> >>> where trigger would be set to a minute for hourly counter and hour for
> >>> daily, weekly and monthly counters. I am sorry but I don't know how to
> >>> write a patch.
> >>>
> >>> Ivan Kalik
> >>> Kalik Informatika ISP
> >>>
> >>> Dana 24/10/2008, "liran tal" <liransgarage at gmail.com> piše:
> >>>
> >>> >Hey Ivan
> >>> >
> >>> >2008/10/24 <tnt at kalik.net>
> >>> >
> >>> >> It (daily sqlcounter) does the same in 2.0.5:
> >>> >>
> >>> >> rlm_sqlcounter: Authorized user jagoda, check_item=10000000,
> >>> counter=2635
> >>> >> rlm_sqlcounter: Sent Reply-Item for user jagoda,
> Type=Session-Timeout,
> >>> >> value=10027850
> >>> >>
> >>> >> Returns value that is greater than the limit. I am using noreset
> >>> >> sqlcounter and that one works fine.
> >>> >
> >>> >
> >>> >Thanks for confirming this on a more up to date version.
> >>> >Alan, this smells like a bug (unless we missed something along the
> way),
> >>> >should I open up a bug ticket?
> >>> >And what would be the chances it can be backported to 1.1.7?
> >>> >
> >>> >Thanks,
> >>> >Liran.
> >>>
> >>> -
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freeradius.org/pipermail/freeradius-users/attachments/20081029/f42c98dc/attachment.html>
More information about the Freeradius-Users
mailing list