Not fishing casts, but data type casts. :) See "man unlang" for details. if (<ipaddr>127.0.0.1 < &Framed-IP-Address) { ... } It allows you to do type-specific comparisons. v2 usually did string comparisons for conditions. To get type-specific comparisons, you sometimes needed to put the data into an intermediate variable. That was ugly. The casting is nicer. That being said, the above example *did* work in v2, with some changes: if (Framed-IP-Address > 127.0.0.1) { ... } That worked by parsing the RHS into a temporary variable, and then doing type-specific comparisons. But you couldn't do: if (<date>"January 1 2014" < "%{sql: SELECT ...}") { ... } You had to put one or the other of the strings into an intermediate variable. This is another minor feature which just makes things nicer. Alan DeKok.
On 11 May 2013, at 10:19, Alan DeKok <aland@DEPLOYINGRADIUS.COM> wrote:
Not fishing casts, but data type casts. :) See "man unlang" for details.
if (<ipaddr>127.0.0.1 < &Framed-IP-Address) { ... }
It allows you to do type-specific comparisons. v2 usually did string comparisons for conditions. To get type-specific comparisons, you sometimes needed to put the data into an intermediate variable.
That was ugly. The casting is nicer.
That being said, the above example *did* work in v2, with some changes:
if (Framed-IP-Address > 127.0.0.1) { ... }
That worked by parsing the RHS into a temporary variable, and then doing type-specific comparisons. But you couldn't do:
if (<date>"January 1 2014" < "%{sql: SELECT ...}") { ... }
FreeRADIUS will attempt to infer right operand type from the left operand. This worked fine where the left operand was an attribute reference, but not if it was just a string. Without the cast if ("January 1 2014" < "%{sql: SELECT ...}") { ... } would be compared lexicographically which may have given the incorrect result. Where either operand is an attribute reference, if it's not already of the correct type it will be printed to a string and then re-parsed as if a new valuepair of the cast type was being created. For the above example the 2.1.x equivalent would have been: update request { Tmp-Date-0 := "January 1 2014" } if (Tmp-Date-0 < "%{sql: SELECT ...}") { ... } One useful thing which i'm not sure Alan has implemented yet is: if (<cidr>&Framed-IP-Address == 192.168.0.0/24) { } Without the cast "192.168.0.0/24" would either fail to parse or parse as "192.168.0.0". With the cast, Framed-IP-Address gets converted to a /32, and the first 24 bits from the values are compared. Arran Cudbard-Bell <a.cudbardb@freeradius.org> FreeRADIUS Development Team
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.
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.
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
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
*determined
at runtime. It's more code and more effort.
== is L = R
is L ⊃ R
< is L ⊂ R
= is L ⊇ R <= is L ⊆ R
and Alan has now implemented that, happy days! Still not sure I see the point of having different internal representations of IP addresses and netmasks, at least for IPv4, but oh well. -Arran Arran Cudbard-Bell <a.cudbardb@freeradius.org> FreeRADIUS Development Team
On Sun, May 12, 2013 at 08:49:07AM -0400, Alan DeKok wrote:
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)
???
Certainly. I'd hope this would raise a type-mismatch error, rather than a Perl-style "guess how to convert the value". Of course, Perl has different operators for different types (== for numeric comparison, eq for string comparison) so it has a clear idea how to coerce the arguments.
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?
As I see it, the problem is that the value is a string, but you want to treat it as something else. So I would be inclined to make the value conversion explicit: if (Framed-IP-Address =~ <cidr>"%{sql: ...") { or more traditionally, if (Framed-IP-Address =~ cidr("%{sql: ...")) {
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.
I wholeheartedly agree.
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.
I'm happy with the semantics of "contains". I would be surprised if the "==" operator were used to express it though. If =~ can't be used then I don't really mind if a new operator has to be introduced. Typed values are pretty easy to handle. The thing which makes RADIUS special is the need to handle cases where the RHS is an enumerated value dependent on the dictionary type of the LHS: Framed-Protocol == PPP So things like PPP == Framed-Protocol or Tmp-Integer-0 == PPP are unlikely to work. (And hence the == operator is non-commutative anyway). Regards, Brian.
Brian Candler wrote:
Certainly. I'd hope this would raise a type-mismatch error, rather than a Perl-style "guess how to convert the value".
It's more possible now that the conditions are parsed at startup.
As I see it, the problem is that the value is a string, but you want to treat it as something else. So I would be inclined to make the value conversion explicit:
Well, that's what it does now. if (<cdir>&Framed-IP-Address < "%{sql: ...}") { Casts the LHS to a CIDR. It has to be the LHS, for reasons you outline below.
Typed values are pretty easy to handle. The thing which makes RADIUS special is the need to handle cases where the RHS is an enumerated value dependent on the dictionary type of the LHS:
Framed-Protocol == PPP
So things like
PPP == Framed-Protocol
or
Tmp-Integer-0 == PPP
are unlikely to work. (And hence the == operator is non-commutative anyway).
Exactly. It could be made to work, but it would involve lots of butchering of the server internals. I've already done that for the new condition && xlat code. There's less code, with more comments, that is clearer, with more test cases, and more functionality. I'd like to get it running for a while before changing ti yet again. Alan DeKok.
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?
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.
On Sun, May 12, 2013 at 08:40:14AM -0400, Alan DeKok 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.
So if I understand you rightly, you're saying that the unlang expressions Framed-IP-Address == 127.0.0.1 and Framed-IP-Address == "127.0.0.1" are treated identically - it's the type of the LHS which makes a difference, and no distinction is made between an IP literal and a string literal. Is that correct? Is there a fundamental reason why literal values can't have types? The canonical example would be the difference between 1 and "1". Say: if (Acct-Session-Time < 100) But presumably if (Acct-Session-Time < "100") is handled the same today (i.e. the "100" is converted to an integer because of the LHS type).
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.
... which again implies this is parsed exactly the same as if ("127.0.0.1" < "127.0.0.2") But there must be something else going on, because on the LHS at least, Framed-IP-Address is not parsed the same as "Framed-IP-Address"
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.
That's pretty different to most languages though, were A == B compares the values of A and B, and & gives you a reference to the variable; you want to compare the values, not the references. Having said that, I seem to remember that C++ does magic dereferencing, converting references to values where required. But the main purpose of taking a reference is to allow a variable to be assigned to at a distance.
The v2 unlang code would require you to do:
if (User-Name == "%{Filter-Id}") {
Ah yes, because of things like Acct-Status-Type == Start, where the RHS has to be considered as an enumerated value rather than another attribute. Regards, Brian.
On 12 May 2013, at 14:02, Brian Candler <B.Candler@pobox.com> wrote:
On Sun, May 12, 2013 at 08:40:14AM -0400, Alan DeKok 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.
So if I understand you rightly, you're saying that the unlang expressions
Framed-IP-Address == 127.0.0.1
and
Framed-IP-Address == "127.0.0.1"
are treated identically - it's the type of the LHS which makes a difference, and no distinction is made between an IP literal and a string literal. Is that correct?
Is there a fundamental reason why literal values can't have types?
The canonical example would be the difference between 1 and "1". Say:
if (Acct-Session-Time < 100)
But presumably
if (Acct-Session-Time < "100")
is handled the same today (i.e. the "100" is converted to an integer because of the LHS type).
Right.
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.
... which again implies this is parsed exactly the same as
if ("127.0.0.1" < "127.0.0.2")
Yes.
But there must be something else going on, because on the LHS at least, Framed-IP-Address is not parsed the same as "Framed-IP-Address"
It's a hangover for backwards compatibility and arguably wrong. I strongly suggested that we disallow it for the sake of clarity but Alan was worried it'd break existing implementations. The proper syntax for such a comparison is: if (&Framed-IP-Address > 192.168.0.0/24) { } I originally argued that string literals must all be wrapped in single quotes, but this would have meant boolean values and enumerated values would also have been required to be wrapped. i.e. update request { Service-Type := 'Framed-User' } update control { Fall-Through := 'no' }
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.
That's pretty different to most languages though, were A == B compares the values of A and B, and & gives you a reference to the variable; you want to compare the values, not the references.
Not so much. I believe === in most languages would get you a strict comparison between references. That could be implemented here very easily but I can't see many places where it'd be useful. Think of &<attribute> being more like $<variable> in PHP.
Having said that, I seem to remember that C++ does magic dereferencing, converting references to values where required. But the main purpose of taking a reference is to allow a variable to be assigned to at a distance.
The main point here is to clearly mark attributes as distinct from literals. I believe it's also supported in unlang now. update reply { Reply-Message := "User-Name was &User-Name, Framed-IP-Address was &Framed-IP-Address" } And %{<attribute>} is deprecated.
The v2 unlang code would require you to do:
if (User-Name == "%{Filter-Id}") {
Ah yes, because of things like Acct-Status-Type == Start, where the RHS has to be considered as an enumerated value rather than another attribute.
Exactly. Though i'm guessing Acct-Status-Type == 'Start' or Acct-Status-Type == "Start" would probably work just as well. I think it's sort of dumb to treat bare words essentially the same as literals. I'd have rather used them as attribute references everywhere. But apparently not requiring enumerated values, integer values or IP addresses be wrapped in quotes trumps the convenience of barewords refer to attributes. At least attribute references are consistent now, whether it's in conditional statements, xlat strings, update blocks, switch statements (I hope), you can always reference attributes in the same way. Any added consistency is a good thing :) Arran Cudbard-Bell <a.cudbardb@freeradius.org> FreeRADIUS Development Team
Arran Cudbard-Bell wrote:
I believe it's also supported in unlang now.
update reply { Reply-Message := "User-Name was &User-Name, Framed-IP-Address was &Framed-IP-Address" }
That isn't supported yet.
I think it's sort of dumb to treat bare words essentially the same as literals. I'd have rather used them as attribute references everywhere.
Well... the parser was written many years ago.
But apparently not requiring enumerated values, integer values or IP addresses be wrapped in quotes trumps the convenience of barewords refer to attributes.
Yes.
At least attribute references are consistent now, whether it's in conditional statements, xlat strings, update blocks, switch statements (I hope), you can always reference attributes in the same way. Any added consistency is a good thing :)
Almost... the xlat strings need to be fixed. Alan DeKok.
On Sun, May 12, 2013 at 02:40:53PM -0400, Arran Cudbard-Bell wrote:
The proper syntax for such a comparison is:
if (&Framed-IP-Address > 192.168.0.0/24) {
}
Ah, suddenly I see where we're going. If it had used a more Perl-PHP-like $ I think I would have gotten it straight away. if ($Framed-IP-Address > 192.168.0.0/24) { .. } Aside: with explicit variable reference like this, I think the update {...} syntax might become redundant. e.g. you could just have $Tmp-IP-0 := $Framed-IP-Address An explicit tag could also allow the expression language to expand to cover eval, although minuses would have to be space-delimited: if ($Event-Timestamp - $Acct-Session-Time < $Tmp-Integer-0) { ... } or: if (${Event-Timestamp} - ${Acct-Session-Time} < ${Tmp-Integer-0}) { ... }
I originally argued that string literals must all be wrapped in single quotes, but this would have meant boolean values and enumerated values would also have been required to be wrapped.
i.e.
update request { Service-Type := 'Framed-User' }
update control { Fall-Through := 'no' }
Yes I see, that's another way it could disambiguate attribute names from literals. It's a bit odd, if something which you know to be an enumeration constant representing an integer, has to be quoted as a string. Having said that, things like Service-Type := "%{sql...}" are presumably still going to be allowed, so string->constant lookups can take place dynamically. So thinking aloud, the rules might be: - a quoted string is a string - an unquoted bareword which parses as an integer or an IP address has that type - otherwise, a bareword LHS is parsed as an attribute name reference, and a bareword RHS is parsed as an enumeration for to the dictionary type of the LHS - assignment or comparison where the RHS is a string but the LHS is an attribute with a non-string value causes the RHS to be converted to the type of the LHS (at parse-time if both LHS and RHS are constants, otherwise can be deferred to run-time) And if the LHS is a string expansion, that has to be deferred too. Not that I expect anyone ever to write: update control { "%{Tmp-String-0}" := "%{Tmp-String-1}" } or: $$Tmp-String-0 := $Tmp-String-1 Ugh :) Regards, Brian.
On Mon, May 13, 2013 at 08:49:44AM +0100, Brian Candler wrote:
- assignment or comparison where the RHS is a string but the LHS is an attribute with a non-string value causes the RHS to be converted to the type of the LHS (at parse-time if both LHS and RHS are constants, otherwise can be deferred to run-time)
This is also a reason to implement list qualifiers: if (reply:Session-Time < 100) allows us to know the type of the LHS, whereas if ("%{reply:Session-Time}" < 100) is just a string of characters.
On 13/05/13 11:11, Brian Candler wrote:
On Mon, May 13, 2013 at 08:49:44AM +0100, Brian Candler wrote:
- assignment or comparison where the RHS is a string but the LHS is an attribute with a non-string value causes the RHS to be converted to the type of the LHS (at parse-time if both LHS and RHS are constants, otherwise can be deferred to run-time)
This is also a reason to implement list qualifiers:
if (reply:Session-Time < 100)
I don't understand; you can do this now. Doesn't it already work?
Phil Mayers wrote:
On 13/05/13 11:11, Brian Candler wrote:
if (reply:Session-Time < 100)
I don't understand; you can do this now. Doesn't it already work?
Yes. But it doesn't do type-checking when the config files load. If you do: if ("%{reply:Session-Time}" < 100) It's harder to do tpye checking, because the LHS is a string. Alan DeKok.
On Mon, May 13, 2013 at 12:30:19PM +0100, Phil Mayers wrote:
This is also a reason to implement list qualifiers:
if (reply:Session-Time < 100)
I don't understand; you can do this now. Doesn't it already work?
As a 2.2.x user, I'd have to write if ("%{reply:Session-Time}" < 100) but I understand this would do a string comparison, so "2" > "100" If you mean list qualifiers have already been implemented in 3.x, then I apologise for the noise.
Brian Candler wrote:
On Mon, May 13, 2013 at 12:30:19PM +0100, Phil Mayers wrote:
This is also a reason to implement list qualifiers:
if (reply:Session-Time < 100) I don't understand; you can do this now. Doesn't it already work?
As a 2.2.x user, I'd have to write
if ("%{reply:Session-Time}" < 100)
No... this should work: if (reply:Session-Timeout < 100)
but I understand this would do a string comparison, so "2" > "100"
If you mean list qualifiers have already been implemented in 3.x, then I apologise for the noise.
List qualifiers work in 2.x and in 3.x. They're identical in that way. Alan DeKok.
On Mon, May 13, 2013 at 11:45:52AM -0400, Alan DeKok wrote:
No... this should work:
if (reply:Session-Timeout < 100)
Great, thank you - I have some configs I can simplify now. (But I still don't think I can use list qualifiers in attributes returned by rlm_sql or rlm_files, correct?) Incidentally, I tested with if (reply:Framed-IP-Address =~ /./) { update reply { Reply-Message += "Congratulations on your static IP" } } and this works. But I get a syntax error if I change the first line to if (reply:Framed-IP-Address =* 0.0.0.0) { Expected comparison at: =* /Users/brian/Build/freeradius/etc/raddb/sites-enabled/default[69]: Errors parsing authorize section. Looking at the 2.2.x source code, the =* operator (T_OP_CMP_TRUE) is explicitly forbidden here: token = gettoken(&p, comp, sizeof(comp)); if ((token < T_OP_NE) || (token > T_OP_CMP_EQ) || (token == T_OP_CMP_TRUE)) { radlog(L_ERR, "Expected comparison at: %s", comp); return FALSE; } After a bit of thought, I found I could rewrite it more simply as if (reply:Framed-IP-Address) { update reply { Reply-Message += "Congratulations on your static IP" } } Still, this appears to be a minor inconsistency between unlang and checkitem operators. Regards, Brian.
Brian Candler wrote:
and this works. But I get a syntax error if I change the first line to
if (reply:Framed-IP-Address =* 0.0.0.0) {
Expected comparison at: =* /Users/brian/Build/freeradius/etc/raddb/sites-enabled/default[69]: Errors parsing authorize section.
Yeah. That's forbidden in 2.x.
After a bit of thought, I found I could rewrite it more simply as
if (reply:Framed-IP-Address) {
Which is also documented. See "man unlang".
Still, this appears to be a minor inconsistency between unlang and checkitem operators.
Yes. The checkitem stuff *requires* all 3 fields. So simple checks for existence aren't allowed. In v3, the =* operator is allowed. It is re-written internally to be just an existence check. There are a number of other "peep-hole" optimizations done by the new code. See src/tests/condition.txt. e.g. (((((foo))))) --> foo !(!foo) --> foo etc. That allows the run-time interpretor to do less work. It also ensures that the run-time interpretor has *one* version of "unlang" it interprets. And not many equal (but different) variations. Alan DeKok.
Brian Candler wrote:
Ah, suddenly I see where we're going. If it had used a more Perl-PHP-like $ I think I would have gotten it straight away.
if ($Framed-IP-Address > 192.168.0.0/24) { .. }
Yeah. Except $ is already used for config-file expansions.
Aside: with explicit variable reference like this, I think the update {...} syntax might become redundant. e.g. you could just have
$Tmp-IP-0 := $Framed-IP-Address
Hmm... that's a good point. I'll look into that.
An explicit tag could also allow the expression language to expand to cover eval, although minuses would have to be space-delimited:
if ($Event-Timestamp - $Acct-Session-Time < $Tmp-Integer-0) { ... }
Yes, probably. But that's lower on the priority list.
Yes I see, that's another way it could disambiguate attribute names from literals. It's a bit odd, if something which you know to be an enumeration constant representing an integer, has to be quoted as a string.
Not really. PPP isn't a valid attribute name. So it must be an enumerated value.
Having said that, things like
Service-Type := "%{sql...}"
are presumably still going to be allowed, so string->constant lookups can take place dynamically.
Yes.
So thinking aloud, the rules might be:
- a quoted string is a string
- an unquoted bareword which parses as an integer or an IP address has that type
That's hard. There can be overlaps between the syntax of the various types. It's easier to just demand that the types be explicit. I think the "getting rid of update sections" above is a good reason to keep using "&" for attribute references. In that case, the rules are pretty simple. - & is an attribute reference (except it can be omitted on the LHS for backwards compatibility) - everything else is type-specific - strings MUST be quoted - other types MUST NOT use single quotes - other types MAY use double quotes / back-ticks for later expansion - bare words for other types MUST be parsable as the type e.g. in v2, "Framed-IP-Address < 127.0.0.1" was stored as a string, and parsed at run-time. Right now, it's split into a LHS (attribute reference), operator, and RHS (string). It *should* parse && save the RHS, so that it doesn't have to re-parse it at run-time. That's relatively easy to do in the new framework. Catching errors on startup is preferable to catching them at run-time. Alan DeKok.
On Mon, May 13, 2013 at 09:16:31AM -0400, Alan DeKok wrote:
I think the "getting rid of update sections" above is a good reason to keep using "&" for attribute references. In that case, the rules are pretty simple.
'update' sections have been a pain in the past, but with the new ability to update multiple things in one section, it's much better. It does separate 'unlang' from most languages, and makes it look more like a configuration than a program. As much as removing/depracating it seems good (and I agree it is more usual for programmers), I'm not sure it's the best thing for a configuration language.
- & is an attribute reference (except it can be omitted on the LHS for backwards compatibility)
- everything else is type-specific - strings MUST be quoted
Coming in on this party a bit late - I'm slightly struggling to see why the new '&' is needed. I'm probably being a bit slow! Obviously "%{Example-Attribute}" is a string, but why not: %{Example-Attribute} rather than &Example-Attribute ? This at least keeps things the same as before. For example, in perl: $a = "hello"; ${a} = "hello"; are the same (and, indeed, the latter is required to disambiguate when putting in a string, such as "foo${a}bar"). So what is the difference between &User-Name := "prefix%{User-Name}" and %{User-Name} := "prefix%{User-Name}" apart from the latter is slightly longer to type, of course! Obviously "%{User-Name}" := "prefix%{User-Name}" is wrong, of course.
Catching errors on startup is preferable to catching them at run-time.
Definitely. Cheers, Matthew -- Matthew Newton, Ph.D. <mcn4@le.ac.uk> Systems Specialist, Infrastructure Services, I.T. Services, University of Leicester, Leicester LE1 7RH, United Kingdom For IT help contact helpdesk extn. 2253, <ithelp@le.ac.uk>
Matthew Newton wrote:
'update' sections have been a pain in the past, but with the new ability to update multiple things in one section, it's much better. It does separate 'unlang' from most languages, and makes it look more like a configuration than a program. As much as removing/depracating it seems good (and I agree it is more usual for programmers), I'm not sure it's the best thing for a configuration language.
I agree. It may be useful, or it may be too confusing.
I'm slightly struggling to see why the new '&' is needed. I'm probably being a bit slow!
Nope.
Obviously
"%{Example-Attribute}"
is a string, but why not:
%{Example-Attribute}
rather than
&Example-Attribute
The %{...} is verbose (IMHO). Plus, the xlat API is *completely* separated from the conditional parser. So exposing the internals of xlat to unlang requires some careful butchering of the code. Adding '&' was simpler. And is prettier.
So what is the difference between
&User-Name := "prefix%{User-Name}"
and
%{User-Name} := "prefix%{User-Name}"
apart from the latter is slightly longer to type, of course!
The first one requires no knowledge of the xlat internals. Alan DeKok.
On Mon, May 13, 2013 at 12:36:48PM -0400, Alan DeKok wrote:
The %{...} is verbose (IMHO). Plus, the xlat API is *completely* separated from the conditional parser. So exposing the internals of xlat to unlang requires some careful butchering of the code.
Adding '&' was simpler. And is prettier.
So what is the difference between
&User-Name := "prefix%{User-Name}"
and
%{User-Name} := "prefix%{User-Name}"
apart from the latter is slightly longer to type, of course!
The first one requires no knowledge of the xlat internals.
Yeah, OK - agreed. It makes sense now. &User-Name for unlang attributes ("variables"). %{...} for xlat expansions (which might contain attributes). This is a good distinction. Cheers, Matthew -- Matthew Newton, Ph.D. <mcn4@le.ac.uk> Systems Specialist, Infrastructure Services, I.T. Services, University of Leicester, Leicester LE1 7RH, United Kingdom For IT help contact helpdesk extn. 2253, <ithelp@le.ac.uk>
On Mon, May 13, 2013 at 09:16:31AM -0400, Alan DeKok wrote:
Yes I see, that's another way it could disambiguate attribute names from literals. It's a bit odd, if something which you know to be an enumeration constant representing an integer, has to be quoted as a string.
Not really. PPP isn't a valid attribute name. So it must be an enumerated value.
I don't think we can guarantee non-overlap between attribute names and enumerated values though. Example: $ grep '^VALUE.*Session-Timeout' dictionary.* dictionary.ascend:VALUE Ascend-Disconnect-Cause Session-Timeout 100 dictionary.ascend.illegal:VALUE X-Ascend-Disconnect-Cause Session-Timeout 100 dictionary.cisco:VALUE Cisco-Disconnect-Cause Session-Timeout 100 dictionary.rfc2866:VALUE Acct-Terminate-Cause Session-Timeout 5 dictionary.shiva:VALUE Shiva-Disconnect-Reason Session-Timeout 4 dictionary.starent:VALUE SN-Disconnect-Reason Session-Timeout 25 dictionary.starent.vsa1:VALUE SN1-Disconnect-Reason Session-Timeout 25 dictionary.usr:VALUE USR-HARC-Disconnect-Code Session-Timeout 7 Others are Idle-Timeout, NAS-Filter-Rule, Service-Selection (*). So an example would be: if (Ascend-Disconnect-Cause == Session-Timeout) In practice it would make little sense to compare an attribute which has a defined enumerated values against a different attribute. So here we must mean the enumerated constant 'Session-Timeout'. Of course, the dictionary value of this constant is different depending on which LHS attribute we are comparing it to! In fact, I think it would be sensible to *require* unquoted literals on the RHS to be enumerated values. Otherwise, imagine what happens if someone writes this: if (Ascend-Disconnect-Cause == Idle-Timeout) I would rather this errored than silently ended up comparing two different attributes - which is almost certainly not what is intended. Regards, Brian. (*) There are also a ton of DHCP attributes where there is an attribute and a value with the same name, although I'm not sure how these attributes are supposed to be used anyway. e.g. ATTRIBUTE DHCP-ARP-Cache-Timeout 35 integer VALUE DHCP-Parameter-Request-List DHCP-ARP-Cache-Timeout 35
Brian Candler wrote:
On Mon, May 13, 2013 at 09:16:31AM -0400, Alan DeKok wrote: I don't think we can guarantee non-overlap between attribute names and enumerated values though. Example:
Hmm.. I didn't see that.
In fact, I think it would be sensible to *require* unquoted literals on the RHS to be enumerated values.
That's pretty much what happens now. The LHS is required to be an attribute reference (mostly). The RHS is then interpreted in the context of that attribute.
(*) There are also a ton of DHCP attributes where there is an attribute and a value with the same name, although I'm not sure how these attributes are supposed to be used anyway. e.g.
ATTRIBUTE DHCP-ARP-Cache-Timeout 35 integer VALUE DHCP-Parameter-Request-List DHCP-ARP-Cache-Timeout 35
The parameter request list is a way of saying "Please send me attribute X". So the attribute names and enumerated values *are* identical. As are the values. Alan DeKok.
Brian Candler wrote:
So if I understand you rightly, you're saying that the unlang expressions
Framed-IP-Address == 127.0.0.1
and
Framed-IP-Address == "127.0.0.1"
are treated identically - it's the type of the LHS which makes a difference, and no distinction is made between an IP literal and a string literal. Is that correct?
Yes.
Is there a fundamental reason why literal values can't have types?
Until a month ago, the conditions were parsed *every time* they were evaluated. Now, they're parsed once, and a data structure is stored. So... it becomes possible to do type-specific checks. i.e. Framed-IP-Address == "127.0.0.1" is wrong. Unless the right-side is something like "%{sql: ...}"
The canonical example would be the difference between 1 and "1". Say:
if (Acct-Session-Time < 100)
But presumably
if (Acct-Session-Time < "100")
is handled the same today (i.e. the "100" is converted to an integer because of the LHS type).
Yes.
... which again implies this is parsed exactly the same as
if ("127.0.0.1" < "127.0.0.2")
Right now, yes.
But there must be something else going on, because on the LHS at least, Framed-IP-Address is not parsed the same as "Framed-IP-Address"
Well, the parser isn't *completely* idiotic. Just *mostly* idiotic.
That's pretty different to most languages though, were A == B compares the values of A and B, and & gives you a reference to the variable; you want to compare the values, not the references.
I'm open to a better alternative... It's hard to come up with *new* syntax, because I'm wary of breaking existing systems. i.e. Framed-Pool == bar is allowed today. But "bar" isn't the correct name for an attribute. How do we disambiguate the two possibilities?
Having said that, I seem to remember that C++ does magic dereferencing, converting references to values where required. But the main purpose of taking a reference is to allow a variable to be assigned to at a distance.
Which isn't possible in unlang. So.. the "&" is syntactic sugar for "the contents of the attribute", and not "a reference to an attribute"
The v2 unlang code would require you to do:
if (User-Name == "%{Filter-Id}") {
Ah yes, because of things like Acct-Status-Type == Start, where the RHS has to be considered as an enumerated value rather than another attribute.
Yes. Alan DeKok.
participants (5)
-
Alan DeKok -
Arran Cudbard-Bell -
Brian Candler -
Matthew Newton -
Phil Mayers