Commit report for master branch
New activity for FreeRADIUS (the high performance and highly configurable RADIUS server) ====== More doxygen warnings Arran Cudbard-Bell@2013-04-09T22:18:10Z Files modified: * src/main/xlat.c Commit diff: https://github.com/FreeRADIUS/freeradius-server/commit/48d46e233fa9dfa235221... ====== Fix doxygen documentation Arran Cudbard-Bell@2013-04-09T21:19:23Z Files modified: * src/main/xlat.c * src/modules/rlm_rest/rest.c * src/modules/rlm_yubikey/rlm_yubikey.c Commit diff: https://github.com/FreeRADIUS/freeradius-server/commit/a0015b19255051a851c0b... ====== doxygen updates Alan T. DeKok@2013-04-09T21:01:40Z Files modified: * src/main/xlat.c Commit diff: https://github.com/FreeRADIUS/freeradius-server/commit/26ec9984f97d4846715ba... ====== Fix typo Alan T. DeKok@2013-04-09T21:00:56Z Files modified: * src/main/modcall.c Commit diff: https://github.com/FreeRADIUS/freeradius-server/commit/97002a4776741ed0d4af0... ====== Brand new xlat functions. All xlat's are now run through the new routines. The old (i.e. horrific) routines are gone. The result is more functionality, simpler debugging, better errors, and ~400 lines fewer code. The downside is that (for the next day or so), calling a module xlat routine MAY mean that it gets expanded twice. i.e. the old code would do %{mod: foo} and pass "foo" to module "mod". The new code (for reasons of simplicity) xlat's "foo" before passing it to module "mod". Some modules (i.e. expr) call radius_xlat again. This is now wrong. The fix is to update the signature of the xlat functions so that "fmt" is no longer a "const char*", as it is now a talloc'd string. Once that's done, all of the modules will need a visual pass-through to remove the extra recursive calls to xlat. We've also deleted xlat registration for %{1} and %[request:..} The regex and attribute parsing is now done by the xlat core. In the future, it should be passed to a module-specific "parse" or "validate" routine. That routine would ensure that the RHS of an xlat expansion is valid Alan T. DeKok@2013-04-09T21:00:56Z Files modified: * src/main/xlat.c Commit diff: https://github.com/FreeRADIUS/freeradius-server/commit/5deec98232056e3fdff9b... ====== Fix network / host byte order issue Alan T. DeKok@2013-04-09T21:00:56Z Files modified: * src/lib/print.c Commit diff: https://github.com/FreeRADIUS/freeradius-server/commit/b2a2fb7e465f597fa3e06... ====== Remove radiusd.pid on failed kill Alan T. DeKok@2013-04-09T20:39:10Z Files modified: * src/tests/Makefile Commit diff: https://github.com/FreeRADIUS/freeradius-server/commit/8aace8a6e3f19f73e5034... ====== Clean radius.log and gdb.log only when re-starting the server Alan T. DeKok@2013-04-09T20:31:50Z Files modified: * src/tests/Makefile Commit diff: https://github.com/FreeRADIUS/freeradius-server/commit/6a14f05101eb318bc87e0... ====== Do a better job of cleanin up before running the tests Alan T. DeKok@2013-04-09T20:22:33Z Files modified: * src/tests/Makefile Commit diff: https://github.com/FreeRADIUS/freeradius-server/commit/ab919e91228c273ad188d... ====== Don't shadow globals Alan T. DeKok@2013-04-09T18:10:00Z Files modified: * src/main/radwho.c Commit diff: https://github.com/FreeRADIUS/freeradius-server/commit/d4f7aed442584fe58e829... ====== Added vp_asprintf() for VPs prints the string value of a VP to a talloc'd buffer, and returns the buffer Alan T. DeKok@2013-04-09T14:43:32Z Files modified: * src/include/libradius.h * src/lib/print.c Commit diff: https://github.com/FreeRADIUS/freeradius-server/commit/22628b7ebbbe440b1188d... ====== -- This commit summary was generated @2013-04-10T00:00:02Z by lgfeed version 0.00 (https://github.com/arr2036/lgfeed).
On Wed, Apr 10, 2013 at 12:00:02AM +0200, The git bot wrote:
Brand new xlat functions.
All xlat's are now run through the new routines. The old (i.e. horrific) routines are gone. The result is more functionality, simpler debugging, better errors, and ~400 lines fewer code.
The downside is that (for the next day or so), calling a module xlat routine MAY mean that it gets expanded twice.
i.e. the old code would do %{mod: foo} and pass "foo" to module "mod". The new code (for reasons of simplicity) xlat's "foo" before passing it to module "mod".
Is this configurable, so that this preprocessing can be disabled for a particular module? It's just that %{exec:...} and %{redis:...} expect to get a single un-xlat'd string, which they split on space, and then xlat the parts individually.
Brian Candler wrote:
Is this configurable, so that this preprocessing can be disabled for a particular module?
Not right now.
It's just that %{exec:...} and %{redis:...} expect to get a single un-xlat'd string, which they split on space, and then xlat the parts individually.
Yes. That can be fixed with simple changes. a) the module supplies an "escape" function which escapes spaces b) the "split on spaces" code converts escaped characters to the non-escaped version... but doesn't split on escaped spaces One other downside of the new code is that it does a LOT of memory allocation for each call to radius_xlat(). The old code allocated tons of memory on the stack, which was faster. The goal is to both decrease the memory allocation, and also to do less dynamic parsing of expansions. i.e. the "files" module should parse the xlat expansion once, and save the parsed result. It can then be interpreted dynamically multiple times. The configuration file parser should do that, too. The conditions should be parsed once, along with with the xlat expansions. Then, the parsed tree should be evaluated at run time. The end result will be less memory used, less CPU time used, and less code. For example, even with new support functions, the new xlat code is ~400 LoC smaller than the old one. And MUCH more understandable. The other benefit is good errors: ... (0) update reply { (0) ERROR: %{expr: %{Stripped-User-Name} %X * 12} (0) ERROR: ^ Invalid variable expansion ... Each parse error now has a unique error message. The error message is used to print the location where the error occurred. This means it's MUCH easier to understand what's gone wrong. The new errors will be used in the config files && users file. So mis-typing an xlat expansion means that the server will get a syntax error on the config files, and refuse to use the bad data. Over all, this is much better. The main cost is additional memory allocation, which can be slow. However, this is 2013. If your RADIUS server is CPU bound, you're probably doing something wrong. Alan DeKok.
... (0) update reply { (0) ERROR: %{expr: %{Stripped-User-Name} %X * 12} (0) ERROR: ^ Invalid variable expansion ...
For those not using a monospaced font the carrot would be placed at the S.
Over all, this is much better. The main cost is additional memory allocation, which can be slow.
Which is mitigated against by allocating per request memory pools. -Arran
On 10 Apr 2013, at 10:03, Arran Cudbard-Bell <a.cudbardb@freeradius.org> wrote:
... (0) update reply { (0) ERROR: %{expr: %{Stripped-User-Name} %X * 12} (0) ERROR: ^ Invalid variable expansion ...
For those not using a monospaced font the carrot would be placed at the S.
make that the %X :)
On Wed, Apr 10, 2013 at 10:00:07AM -0400, Alan DeKok wrote:
It's just that %{exec:...} and %{redis:...} expect to get a single un-xlat'd string, which they split on space, and then xlat the parts individually.
Yes. That can be fixed with simple changes.
a) the module supplies an "escape" function which escapes spaces
b) the "split on spaces" code converts escaped characters to the non-escaped version... but doesn't split on escaped spaces
Like this suggestion? :-) http://lists.freeradius.org/pipermail/freeradius-devel/2012-October/007207.h...
i.e. the "files" module should parse the xlat expansion once, and save the parsed result. It can then be interpreted dynamically multiple times.
Makes sense - like regcomp. (And of course, "files" could also precompile the RHS of regexp match operators)
The other benefit is good errors:
That's a *big* benefit. Regards, Brian.
Brian Candler wrote:=
Like this suggestion? :-) http://lists.freeradius.org/pipermail/freeradius-devel/2012-October/007207.h...
Well, yes. The issue for me is "what are special characters for redis?" That isn't well documented. If we assume spaces only, we should be OK.
i.e. the "files" module should parse the xlat expansion once, and save the parsed result. It can then be interpreted dynamically multiple times.
Makes sense - like regcomp. (And of course, "files" could also precompile the RHS of regexp match operators)
Yeah. That would help, too.
The other benefit is good errors:
That's a *big* benefit.
Yes. Even more so when they become initialization errors, and not run-time errors. Alan DeKok.
On Wed, Apr 10, 2013 at 12:22:55PM -0400, Alan DeKok wrote:
Well, yes. The issue for me is "what are special characters for redis?"
That isn't well documented. If we assume spaces only, we should be OK.
redis is 8-bit clean - there are no special characters. Cheers, Brian.
On 10 Apr 2013, at 11:43, Brian Candler <B.Candler@pobox.com> wrote:
On Wed, Apr 10, 2013 at 10:00:07AM -0400, Alan DeKok wrote:
It's just that %{exec:...} and %{redis:...} expect to get a single un-xlat'd string, which they split on space, and then xlat the parts individually.
Yes. That can be fixed with simple changes.
a) the module supplies an "escape" function which escapes spaces
b) the "split on spaces" code converts escaped characters to the non-escaped version... but doesn't split on escaped spaces
Like this suggestion? :-) http://lists.freeradius.org/pipermail/freeradius-devel/2012-October/007207.h...
i.e. the "files" module should parse the xlat expansion once, and save the parsed result. It can then be interpreted dynamically multiple times.
Makes sense - like regcomp. (And of course, "files" could also precompile the RHS of regexp match operators)
No. Not unless the regexp library supports compiled, parameterised expressions. If the files parser doesn't currently support nested expansions within the expression, it will once it calls the new tokenizer. For things like SQL however, it should be possible to precompile the statements and you may get a small performance gain doing that. This would be one advantage of having an xlat specific tokenizer callback. It also means that the code that parses attribute references can be moved out of the tokenizer, it really doesn't belong there. Conflating xlat functions and attributes makes the code harder to understand, there should be one common xlat attribute-ref function, (wrapped and registered a bunch of times for the different lists) to retrieve attribute values. Once nested VPs are done, then there should be a single xlat function to expand all attribute references. -Arran
On Wed, Apr 10, 2013 at 12:41:43PM -0400, Arran Cudbard-Bell wrote:
Makes sense - like regcomp. (And of course, "files" could also precompile the RHS of regexp match operators)
No. Not unless the regexp library supports compiled, parameterised expressions.
I was thinking of something like foo NAS-IP-Address =~ "^192\.0\.2\." It never occurred to me that the RHS of a condition like that might include xlats. Does the files module actually allow stuff like this?? e.g. foo NAS-IP-Address == "%{Client-IP-Address}" Reply-Message += "Hello %{Client-IP-Address}" Now, a quick test with a master build from a month ago suggests that Reply-Message += "Hello %{Client-IP-Address}", works (which is new to me). However steve Cleartext-Password := "testing", NAS-IP-Address =~ "%{Client-IP-Address}" fails to start with the following error: reading pairlist file /Users/brian/Build/freeradius-master/etc/raddb/users /Users/brian/Build/freeradius-master/etc/raddb/users[76]: Parse error (check) for entry steve: dict_init: /Users/brian/Build/freeradius-master/etc/raddb/dictionary[65]: Couldn't open dictionary "/Users/brian/Build/freeradius-master/etc/raddb/dictionary.local": No such file or directory Errors reading /Users/brian/Build/freeradius-master/etc/raddb/users Very strange, the section in etc/raddb/dictionary says: # Include dictionary.local, IF it exists. Otherwise, ignore it. # # This file WILL NOT EVER be created, edited, or modified # by FreeRADIUS. # $INCLUDE- dictionary.local so it shouldn't matter that the file doesn't exist. When if I comment out this $INCLUDE- line, I get "unknown error" instead: reading pairlist file /Users/brian/Build/freeradius-master/etc/raddb/users /Users/brian/Build/freeradius-master/etc/raddb/users[76]: Parse error (check) for entry steve: (unknown error) Errors reading /Users/brian/Build/freeradius-master/etc/raddb/users But it works if I change the condition to a fixed string literal: steve Cleartext-Password := "testing", NAS-IP-Address =~ "^192\." Odd. I have to say I can't really see a need for expansions in check items, and if they don't work today, maybe it's still reasonable to say that regexp values could be precompiled. Regards, Brian.
Brian Candler wrote:
I was thinking of something like
foo NAS-IP-Address =~ "^192\.0\.2\."
It never occurred to me that the RHS of a condition like that might include xlats. Does the files module actually allow stuff like this?? e.g.
Sometimes... it doesn't allow it for check items.
steve Cleartext-Password := "testing", NAS-IP-Address =~ "%{Client-IP-Address}"
fails to start with the following error:
reading pairlist file /Users/brian/Build/freeradius-master/etc/raddb/users /Users/brian/Build/freeradius-master/etc/raddb/users[76]: Parse error (check) for entry steve: dict_init: /Users/brian/Build/freeradius-master/etc/raddb/dictionary[65]: Couldn't open dictionary "/Users/brian/Build/freeradius-master/etc/raddb/dictionary.local": No such file or directory Errors reading /Users/brian/Build/freeradius-master/etc/raddb/users
Weird.
so it shouldn't matter that the file doesn't exist. When if I comment out this $INCLUDE- line, I get "unknown error" instead:
reading pairlist file /Users/brian/Build/freeradius-master/etc/raddb/users /Users/brian/Build/freeradius-master/etc/raddb/users[76]: Parse error (check) for entry steve: (unknown error) Errors reading /Users/brian/Build/freeradius-master/etc/raddb/users
It looks like something else is going wrong. Maybe a DNS issue?
But it works if I change the condition to a fixed string literal:
steve Cleartext-Password := "testing", NAS-IP-Address =~ "^192\."
Yeah... it looks like the expansion on the RHS doesn't work.
Odd. I have to say I can't really see a need for expansions in check items, and if they don't work today, maybe it's still reasonable to say that regexp values could be precompiled.
Yes. Alan DeKok.
Odd. I have to say I can't really see a need for expansions in check items, and if they don't work today, maybe it's still reasonable to say that regexp values could be precompiled.
They worked (last time I checked) fine in unlang conditions. They should work ubiquitously once everything uses the same tokenizer. We could of course conditionally compile expressions that had no expanded children, it would be fairly easy to do. -Arran
participants (4)
-
Alan DeKok -
announce＠freeradius.org -
Arran Cudbard-Bell -
Brian Candler