From 7d38b593e8b1624e1ce2cda8fa57a42cdf2d3068 Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Fri, 23 Sep 2011 19:55:01 +0200 Subject: Fix and clarify semantics of peer filters An eval filter returning a non-true value caused the call process to fail and the doc was vague on how an exception was treated. Clarify that the non-tuple host/realm filters assume messages of a certain form. Various minor corrections to align code and doc. --- lib/diameter/doc/src/diameter.xml | 37 ++++++++++++---- lib/diameter/doc/src/diameter_app.xml | 7 ++- lib/diameter/src/app/diameter_service.erl | 71 ++++++++++++++++--------------- 3 files changed, 72 insertions(+), 43 deletions(-) diff --git a/lib/diameter/doc/src/diameter.xml b/lib/diameter/doc/src/diameter.xml index 36b6cbf0cf..2cad70e3bc 100644 --- a/lib/diameter/doc/src/diameter.xml +++ b/lib/diameter/doc/src/diameter.xml @@ -277,6 +277,10 @@ callback.

+

+An invalid option will cause call/4 +to fail.

+ @@ -405,6 +409,8 @@ sense.

eval([{M,F,A} | T]) -> apply(M, F, T ++ A); +eval([[F|A] | T]) -> + eval([F | T ++ A]); eval([F|A]) -> apply(F, A); eval(F) -> @@ -461,14 +467,14 @@ or any peer if the request does not contain a Destination-Realm AVP.

-{host, any|UTF8String()} +{host, any|DiameterIdentity()}

Matches only those peers whose Origin-Host has the specified value, or all peers if the atom any.

 -{realm, any|UTF8String() +{realm, any|DiameterIdentity()

Matches only those peers whose Origin-Realm has the @@ -478,8 +484,9 @@ value, or all peers if the atom any.

{eval, evaluable()}

-Matches only those peers for which the specified evaluable() evaluates -to true on the peer's diameter_caps record.

+Matches only those peers for which the specified evaluable() returns +true on the connection's diameter_caps record. +Any other return value or exception is equivalent to false.

 {neg, peer_filter()} @@ -503,6 +510,21 @@ specified list.

+

+Note that the host and realm filters examine the +outgoing request as passed to call/4, +assuming that this is a record- or list-valued message() as documented +in diameter_app(3), and that +the message contains at most one of each AVP. +If this is not the case then the {host|realm, DiameterIdentity()} +filters must be used to achieve the desired result. +Note also that an empty host/realm (which should not be typical) +is equivalent to an unspecified one for the purposes of filtering.

+ +

+An invalid filter is equivalent to {any, []}, a filter +that matches no peer.

+ @@ -787,7 +809,7 @@ transports.

SvcName = service_name() App = application_alias() -Request = diameter_app:message() +Request = diameter_app:message() | term() Answer = term() Options = [call_opt()] @@ -819,9 +841,8 @@ If there are no suitable peers, or if pick_peer/4 rejects them by returning 'false', then {error, no_connection} is returned. -If pick_peer/4 -selects a candidate peer then a request process is spawned for the -outgoing request, in which there is a +Otherwise pick_peer/4 +is followed by a prepare_request/3 callback, the message is encoded and sent.

diff --git a/lib/diameter/doc/src/diameter_app.xml b/lib/diameter/doc/src/diameter_app.xml index fc359b9d1d..f2bada84ae 100644 --- a/lib/diameter/doc/src/diameter_app.xml +++ b/lib/diameter/doc/src/diameter_app.xml @@ -269,7 +269,12 @@ The candidate peers list will only include those which are selected by any filter option specified in the call to diameter:call/4, and only those which have indicated support for the Diameter application in -question.

+question. +The order of the elements is unspecified except that any +peers whose Origin-Host and Origin-Realm matches that of the +outgoing request (in the sense of a {filter, {all, [host, realm]}} +option to diameter:call/4) +will be placed at the head of the list.

The return values false and {false, State} are diff --git a/lib/diameter/src/app/diameter_service.erl b/lib/diameter/src/app/diameter_service.erl index 63b0649dc4..a72453d431 100644 --- a/lib/diameter/src/app/diameter_service.erl +++ b/lib/diameter/src/app/diameter_service.erl @@ -1398,15 +1398,15 @@ recv_answer(Timeout, %% is, from the last peer to which we've transmitted. receive - {answer = A, Ref, Rq, Pkt} -> %% Answer from peer. + {answer = A, Ref, Rq, Pkt} -> %% Answer from peer {A, Rq, Pkt}; - {timeout = Reason, TRef, _} -> %% No timely reply + {timeout = Reason, TRef, _} -> %% No timely reply {error, Req, Reason}; - {failover = Reason, TRef, false} -> %% No alternative peer. + {failover = Reason, TRef, false} -> %% No alternate peer {error, Req, Reason}; - {failover, TRef, Transport} -> %% Resend to alternate peer. + {failover, TRef, Transport} -> %% Resend to alternate peer try_retransmit(Timeout, SvcName, Req, Transport); - {failover, TRef} -> %% May have missed failover notification. + {failover, TRef} -> %% May have missed failover notification Seqs = diameter_codec:sequence_numbers(RPkt), Pid = whois(SvcName), is_pid(Pid) andalso (Pid ! {failover, TRef, Seqs}), @@ -2482,6 +2482,7 @@ rpd(Pid, Alias, PDict) -> %%% %%% Output: {TransportPid, #diameter_caps{}, #diameter_app{}} %%% | false +%%% | {error, Reason} %%% --------------------------------------------------------------------------- %% Initial call, from an arbitrary process. @@ -2540,22 +2541,12 @@ get_destination(Msg, Dict) -> [str(get_avp_value(Dict, 'Destination-Realm', Msg)), str(get_avp_value(Dict, 'Destination-Host', Msg))]. -%% TODO: -%% -%% Should add some way of specifying destination directly so that the -%% only requirement is that the prepare_request callback returns -%% something specific. (eg. {host, DH}; that is, let the caller specify.) -%% -%% Also, there is no longer any need to call get_destination at all in -%% the default case. - -str(T) - when T == undefined; - T == [] -> +%% This is not entirely correct. The avp could have an arity 1, in +%% which case an empty list is a DiameterIdentity of length 0 rather +%% than the list of no values we treat it as by mapping to undefined. +%% This behaviour is documented. +str([]) -> undefined; -str([X]) - when is_list(X) -> - X; str(T) -> T. @@ -2690,7 +2681,8 @@ peers(Alias, RH, Filter, Peers) -> end. %% Place a peer whose Destination-Host/Realm matches those of the -%% request at the front of the result list. +%% request at the front of the result list. Could add some sort of +%% 'sort' option to allow more control. ps([], _, _, {Ys, Ns}) -> lists:reverse(Ys, Ns); @@ -2700,11 +2692,11 @@ ps([{_TPid, #diameter_caps{} = Caps} = TC | Rest], RH, Filter, Acc) -> TC, Acc)). -pacc(true, true, TC, {Ts, Fs}) -> - {[TC|Ts], Fs}; -pacc(true, false, TC, {Ts, Fs}) -> - {Ts, [TC|Fs]}; -pacc(false, _, _, Acc) -> +pacc(true, true, Peer, {Ts, Fs}) -> + {[Peer|Ts], Fs}; +pacc(true, false, Peer, {Ts, Fs}) -> + {Ts, [Peer|Fs]}; +pacc(_, _, _, Acc) -> Acc. %% caps_filter/3 @@ -2712,17 +2704,19 @@ pacc(false, _, _, Acc) -> caps_filter(C, RH, {neg, F}) -> not caps_filter(C, RH, F); -caps_filter(C, RH, {all, L}) -> +caps_filter(C, RH, {all, L}) + when is_list(L) -> lists:all(fun(F) -> caps_filter(C, RH, F) end, L); -caps_filter(C, RH, {any, L}) -> +caps_filter(C, RH, {any, L}) + when is_list(L) -> lists:any(fun(F) -> caps_filter(C, RH, F) end, L); -caps_filter(#diameter_caps{origin_host = {_,H}}, [_,DH], host) -> - eq(undefined, DH, H); +caps_filter(#diameter_caps{origin_host = {_,OH}}, [_,DH], host) -> + eq(undefined, DH, OH); -caps_filter(#diameter_caps{origin_realm = {_,R}}, [DR,_], realm) -> - eq(undefined, DR, R); +caps_filter(#diameter_caps{origin_realm = {_,OR}}, [DR,_], realm) -> + eq(undefined, DR, OR); caps_filter(C, _, Filter) -> caps_filter(C, Filter). @@ -2738,6 +2732,9 @@ caps_filter(#diameter_caps{origin_host = {_,OH}}, {host, H}) -> caps_filter(#diameter_caps{origin_realm = {_,OR}}, {realm, R}) -> eq(any, R, OR); +%% Anything else is expected to be an eval filter. Filter failure is +%% documented as being equivalent to a non-matching filter. + caps_filter(C, T) -> try {eval, F} = T, @@ -2746,8 +2743,14 @@ caps_filter(C, T) -> _:_ -> false end. -eq(X, A, B) -> - X == A orelse A == B. +eq(Any, Id, PeerId) -> + Any == Id orelse try + iolist_to_binary(Id) == iolist_to_binary(PeerId) + catch + _:_ -> false + end. +%% OctetString() can be specified as an iolist() so test for string +%% rather then term equality. %% transports/1 -- cgit v1.2.3