l j Ljubljana Ji Mantel moon On May 12, 2013, at 11:33 AM, freeradius-devel-request@lists.freeradius.org wrote:
Send Freeradius-Devel mailing list submissions to freeradiudevel@lists.freeradius.org I To subscribe or unsubscribe via the World Wide Web, visit http://lists.freeradius.org/mailman/listinfo/freeradius-develn or, via email, send a message with subject or body 'help' to freeradius-devel-request@lists.freeradius.org
You can reach the person managing the list at freeradius-devel-owner@lists.freeradius.org
When replying, please edit your Subject line so it is more specific than "Re: Contents of Freeradius-Devel digest..."
Today's Topics:
1. Re: New feature in v3: cast! (Brian Candler) 2. Re: New feature in v3: cast! (Brian Candler) 3. Re: New feature in v3: cast! (Alan DeKok) 4. Re: New feature in v3: cast! (Alan DeKok) 5. rad_unlockfd (Brian Candler) 6. Re: New feature in v3: cast! (Arran Cudbard-Bell)
----------------------------------------------------------------------
Message: 1 Date: Sun, 12 May 2013 11:39:30 +0100 From: Brian Candler <B.Candler@pobox.com> To: FreeRadius developers mailing list <freeradius-devel@lists.freeradius.org> Subject: Re: New feature in v3: cast! Message-ID: <20130512103930.GA27456@nsrc.org> Content-Type: text/plain; charset=us-ascii
On Sat, May 11, 2013 at 10:19:02AM -0400, Alan DeKok wrote:
Not fishing casts, but data type casts. :) See "man unlang" for details.
if (<ipaddr>127.0.0.1 < &Framed-IP-Address) { ... }
1. Why is <ipaddr> necessary before the literal? Surely an unquoted 127.0.0.1 can't be parsed as anything else.
2. What does the & in front of Framed-IP-Address do?
------------------------------
Message: 2 Date: Sun, 12 May 2013 12:18:21 +0100 From: Brian Candler <B.Candler@pobox.com> To: FreeRadius developers mailing list <freeradius-devel@lists.freeradius.org> Subject: Re: New feature in v3: cast! Message-ID: <20130512111821.GB27456@nsrc.org> Content-Type: text/plain; charset=us-ascii
On Sat, May 11, 2013 at 06:48:18PM -0400, Arran Cudbard-Bell wrote:
One useful thing which i'm not sure Alan has implemented yet is:
if (<cidr>&Framed-IP-Address == 192.168.0.0/24) {
} ... With the cast, Framed-IP-Address gets converted to a /32, and the first 24 bits from the values are compared.
Hmm, I think this syntax is obscure. Here the == operator is being changed to mean "is this IP address contained within that prefix"? They're not equal to each other. Perhaps something like =~ would be clearer?
I'm also not sure that casting the LHS to <cidr> is a clear way of achieving this. The LHS is, and remains, a single IP address. The RHS is a CIDR prefix.
So I'd rather see:
if (Framed-IP-Address =~ <cidr>192.168.0.0/24) { ... }
or:
if (Framed-IP-Address =~ 192.168.0.0/24) { ... }
since the unquoted RHS literal can be unambiguously parsed as a CIDR prefix anyway, and a comparison of IP value with CIDR value have the obvious semantics.
Maybe the reason for casting the LHS to <cidr> so that you can go a general comparison of (CIDR) to (CIDR) rather than (IP) within (CIDR). In that case, the rules for comparing two (CIDR) values need to be clear. The two prefix lengths may be different. Are you just comparing the shorter of the two prefixes (so that the == operator is commutative)? Or does the == operator return true only if the LHS CIDR range is completely contained within the RHS CIDR range?
Example:
192.168.0.0/24 == 192.168.0.0/23 # true or false? 192.168.1.0/24 == 192.168.0.0/23 # true or false? 192.168.0.0/23 == 192.168.1.0/24 # true or false?
And what about single IP addresses in CIDR notation, where the host part is non-zero?
192.168.0.1/24 == 192.168.0.1/24 # true or false? 192.168.0.1/24 == 192.168.0.2/24 # true or false? 192.168.0.1/24 == 192.168.0.1/23 # true or false?
Also, is there planned to be an ordering for CIDR values?
192.168.0.0/24 < 192.168.0.0/23 # true or false?
Finally, there is a problem of what to do with modules like rlm_sql and rlm_files. These are already restricted to returning LHS=attribute name, RHS=string literal. Unfortunately we can't use the =~ operator here because people may already be doing things like
("Framed-IP-Address", "=~", "^192\.168\.")
So maybe it makes sense for these to return
("<cidr>Framed-IP-Address", "=~", "192.168.0.0/24")
but this still doesn't feel quite right to me.
Aside: if this is going to be done, I have a much more pressing need for being able to qualify the attribute with the list name:
("reply:Framed-IP-Address", "=*", "present")
At the moment I have to copy a bunch of attributes from the reply list to the request list, just so that rlm_sql can match them.
Regards,
Brian.
------------------------------
Message: 3 Date: Sun, 12 May 2013 08:40:14 -0400 From: Alan DeKok <aland@deployingradius.com> To: FreeRadius developers mailing list <freeradius-devel@lists.freeradius.org> Subject: Re: New feature in v3: cast! Message-ID: <518F8DAE.40800@deployingradius.com> Content-Type: text/plain; charset=ISO-8859-1
Brian Candler wrote:
1. Why is <ipaddr> necessary before the literal? Surely an unquoted 127.0.0.1 can't be parsed as anything else.
See my other example for why it's necessary.
In *some* cases, you know the data type of an expression. In those cases, you can easily do type-specific comparisons. That's what unlang does today:
if (Framed-IP-Address == 127.0.0.1) {
The type is "ipaddr", because of Framed-IP-Address. The RHS is in fact parsed into a second Framed-IP-Address attribute, and the two are compared.
However... you can't currently do a type-safe comparison like:
if (127.0.0.1 < 127.0.0.2) {
The interpretor does *string* comparisons. Which is wrong for IP addresses. Adding a cast allows you to do:
if (<ipaddr>127.0.0.1 < 127.0.0.2) {
Which does type-safe checks as with Framed-IP-Address, above.
2. What does the & in front of Framed-IP-Address do?
$ man unlang :)
It's a reference.
if (&User-Name == &Filter-Id) {
Does type-safe comparisons on the *values* of the two attributes. The v2 unlang code would require you to do:
if (User-Name == "%{Filter-Id}") {
Which converts Filter-Id to a string, parses the string into a temporary User-Name attribute, and then compares the two User-Name attributes.
Using a reference means you can skip 2 out of 3 of those steps.
Alan DeKok.
------------------------------
Message: 4 Date: Sun, 12 May 2013 08:49:07 -0400 From: Alan DeKok <aland@deployingradius.com> To: FreeRadius developers mailing list <freeradius-devel@lists.freeradius.org> Subject: Re: New feature in v3: cast! Message-ID: <518F8FC3.3020502@deployingradius.com> Content-Type: text/plain; charset=ISO-8859-1
Brian Candler wrote:
Hmm, I think this syntax is obscure. Here the == operator is being changed to mean "is this IP address contained within that prefix"? They're not equal to each other. Perhaps something like =~ would be clearer?
=~ already has a meaning as a regex operator. So I'd like to avoid using that.
I'm also not sure that casting the LHS to <cidr> is a clear way of achieving this. The LHS is, and remains, a single IP address. The RHS is a CIDR prefix.
The issue is that the code does type-specific comparisons. It's hard to know what's right for inter-type comparisons.
if (127.0.0.1 < 2)
???
So I'd rather see:
if (Framed-IP-Address =~ <cidr>192.168.0.0/24) { ... }
That could work (maybe) with some changes.
since the unquoted RHS literal can be unambiguously parsed as a CIDR prefix anyway,
Well... maybe. What about:
if (Framed-IP-Address =~ "%{sql: ...") {
Is the RHS a regex? A CIDR comparison? An IP address comparison?
I'm trying to say that the code needs *unambiguous* rules. Ad-hoc rules work in some cases, and don't work in others. So they can't be used.
The casting, however, is always unambiguous. Cast both sides to a type, and do type-specific comparions.
and a comparison of IP value with CIDR value have the obvious semantics.
"obvious"? What happens when an IP address is smaller than the CIDR range? Or greater than it?
For CIDRs, we need at least a "contains" operator. As in "does CIDR 1 contain CIDR 2".
Maybe the reason for casting the LHS to <cidr> so that you can go a general comparison of (CIDR) to (CIDR) rather than (IP) within (CIDR). In that case, the rules for comparing two (CIDR) values need to be clear.
See Wikipedia for "range comparison" (IIRC). There are 20+ types of possible relationships for two ranges. In contrast, integers just have 3 (and their negations). ==, <, > (and !=, >=, <=)
It's much easier to just say define a "contains" operator for CIDRs. That way you can't even *express* the other types of relationships. Because they're not needed 99.999% of the time. And if they are needed, use Perl.
Finally, there is a problem of what to do with modules like rlm_sql and rlm_files. These are already restricted to returning LHS=attribute name, RHS=string literal. Unfortunately we can't use the =~ operator here because people may already be doing things like
("Framed-IP-Address", "=~", "^192\.168\.")
So maybe it makes sense for these to return
("<cidr>Framed-IP-Address", "=~", "192.168.0.0/24")
but this still doesn't feel quite right to me.
That will probably *not* happen.
Aside: if this is going to be done, I have a much more pressing need for being able to qualify the attribute with the list name:
("reply:Framed-IP-Address", "=*", "present")
At the moment I have to copy a bunch of attributes from the reply list to the request list, just so that rlm_sql can match them.
Adding a list qualifier is probably easy. It's on the "to do" items for v3.
Alan DeKok.
------------------------------
Message: 5 Date: Sun, 12 May 2013 15:16:42 +0100 From: Brian Candler <B.Candler@pobox.com> To: freeradius-devel@lists.freeradius.org Subject: rad_unlockfd Message-ID: <20130512141642.GA28314@nsrc.org> Content-Type: text/plain; charset=us-ascii
I noticed something odd in rad_unlockfd - both 2.x.x and master.
int rad_unlockfd(int fd, int lock_len) { ... fl.l_type = F_WRLCK; fl.l_whence = SEEK_CUR;
return fcntl(fd, F_UNLCK, (void *)&fl);
However, according to the Linux manpage for fcntl:
F_SETLK (struct flock *) Acquire a lock (when l_type is F_RDLCK or F_WRLCK) or release a lock (when l_type is F_UNLCK) on the bytes specified by the l_whence, l_start, and l_len fields of lock. If a conflicting lock is held by another process, this call returns -1 and sets errno to EACCES or EAGAIN.
That is: to unlock, the command should be F_SETLK(W) and the l_type should be F_UNLCK. The OSX (BSD) manpage agrees.
On Linux, the constant F_UNLCK is 2, so calling fcntl(fd, F_UNLCK...) is the same as calling fcntl(fd, F_SETFD...)
The other oddity is using SEEK_CUR for both lock and unlock. If you have written to the file in the mean time, then the current file offset will have changed, so you may end up unlocking a different byte range to the one you locked. I'd say SEEK_SET with offset 0 and length 0 (which locks or unlocks the entire file, including past its end) is safest - at least when unlocking.
As far as I can see, only rad_utmp calls this function, so the impact is not huge.
Regards,
Brian.
------------------------------
Message: 6 Date: Sun, 12 May 2013 11:31:16 -0400 From: Arran Cudbard-Bell <a.cudbardb@freeradius.org> To: FreeRadius developers mailing list <freeradius-devel@lists.freeradius.org> Subject: Re: New feature in v3: cast! Message-ID: <C3736289-E09A-43F4-9A58-656B35B8A2B1@freeradius.org> Content-Type: text/plain; charset=utf-8
On 12 May 2013, at 08:49, Alan DeKok <aland@deployingradius.com> wrote:
Brian Candler wrote:
Hmm, I think this syntax is obscure. Here the == operator is being changed to mean "is this IP address contained within that prefix"? They're not equal to each other. Perhaps something like =~ would be clearer?
=~ already has a meaning as a regex operator. So I'd like to avoid using that.
Yes that's extremely confusing.
I'm also not sure that casting the LHS to <cidr> is a clear way of achieving this. The LHS is, and remains, a single IP address. The RHS is a CIDR prefix.
The issue is that the code does type-specific comparisons. It's hard to know what's right for inter-type comparisons.
if (127.0.0.1 < 2)
???
So I'd rather see:
if (Framed-IP-Address =~ <cidr>192.168.0.0/24) { ... }
That could work (maybe) with some changes.
since the unquoted RHS literal can be unambiguously parsed as a CIDR prefix anyway,
Well... maybe. What about:
if (Framed-IP-Address =~ "%{sql: ...") {
Yeah the key here is the type of comparison is done at parse time, not runtime there'd need to be an exception for IP addresses such that the format of the expansion was done at runtime. It's more code and more effort.
Is the RHS a regex? A CIDR comparison? An IP address comparison?
Using the =~ operator is a bad idea.
The most logical and consistent way to do this IMO is to treat the ranges as sets of IP addresses.
== is L = R
is L ? R < is L ? R = is L ? R <= is L ? R
It's a bit weird if you don't get what's going on, but arguably more powerful if you do.
if (<cidr> Framed-IP-Address < "192.168.0.0/24") {
}
if (<cidr> "192.168.0.0/24" < "192.168.0.0/16") {
}
Where the ranges do not overlap if condition evaluates to false.
The other option:
== is L ? R
is integer comparison of masked off bits in L and R < is integer comparison of masked off bits in L and R
But that's nasty...
I'm trying to say that the code needs *unambiguous* rules. Ad-hoc rules work in some cases, and don't work in others. So they can't be used.
The casting, however, is always unambiguous. Cast both sides to a type, and do type-specific comparions.
and a comparison of IP value with CIDR value have the obvious semantics.
"obvious"? What happens when an IP address is smaller than the CIDR range? Or greater than it?
For CIDRs, we need at least a "contains" operator. As in "does CIDR 1 contain CIDR 2".
Yeah that's why the above works well. The symbols also look pretty similar :)
Maybe the reason for casting the LHS to <cidr> so that you can go a general comparison of (CIDR) to (CIDR) rather than (IP) within (CIDR). In that case, the rules for comparing two (CIDR) values need to be clear.
See Wikipedia for "range comparison" (IIRC). There are 20+ types of possible relationships for two ranges. In contrast, integers just have 3 (and their negations). ==, <, > (and !=, >=, <=)
It's much easier to just say define a "contains" operator for CIDRs. That way you can't even *express* the other types of relationships. Because they're not needed 99.999% of the time. And if they are needed, use Perl.
*Python
Finally, there is a problem of what to do with modules like rlm_sql and rlm_files. These are already restricted to returning LHS=attribute name, RHS=string literal. Unfortunately we can't use the =~ operator here because people may already be doing things like
("Framed-IP-Address", "=~", "^192\.168\.")
So maybe it makes sense for these to return
("<cidr>Framed-IP-Address", "=~", "192.168.0.0/24")
but this still doesn't feel quite right to me.
That will probably *not* happen.
Aside: if this is going to be done, I have a much more pressing need for being able to qualify the attribute with the list name:
("reply:Framed-IP-Address", "=*", "present")
At the moment I have to copy a bunch of attributes from the reply list to the request list, just so that rlm_sql can match them.
Adding a list qualifier is probably easy. It's on the "to do" items for v3.
It's not *that* easy. IIRC the problem is that the SQL code creates intermediary linked lists of VALUE_PAIRs and as VALUE_PAIRs cannot contain destination lists they also get inserted into the same request list.
It becomes easier with nested attributes as you can root the VALUE_PAIRs in the correct top level nodes, and then merge the trees.
You retain the atomic insert whilst being able to specify where the VALUE_PAIRs should go.
Arran Cudbard-Bell <a.cudbardb@freeradius.org> FreeRADIUS Development Team
------------------------------
- List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html
End of Freeradius-Devel Digest, Vol 97, Issue 15 ************************************************