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.

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.</p>
 
 </taglist>
 
+<p>
+An invalid option will cause <seealso marker="#call">call/4</seealso>
+to fail.</p>
+
 <marker id="capability"/>
 </item>
 
@@ -405,6 +409,8 @@ sense.</p>
 <code>
 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 <c>Destination-Realm</c> AVP.</p>
 </item>
 
-<tag><c>{host, any|UTF8String()}</c></tag>
+<tag><c>{host, any|DiameterIdentity()}</c></tag>
 <item>
 <p>
 Matches only those peers whose <c>Origin-Host</c> has the
 specified value, or all peers if the atom <c>any</c>.</p>
 </item>
 
-<tag><c>{realm, any|UTF8String()</c></tag>
+<tag><c>{realm, any|DiameterIdentity()</c></tag>
 <item>
 <p>
 Matches only those peers whose <c>Origin-Realm</c> has the
@@ -478,8 +484,9 @@ value, or all peers if the atom <c>any</c>.</p>
 <tag><c>{eval, evaluable()}</c></tag>
 <item>
 <p>
-Matches only those peers for which the specified evaluable() evaluates
-to true on the peer's <c>diameter_caps</c> record.</p>
+Matches only those peers for which the specified evaluable() returns
+<c>true</c> on the connection's <c>diameter_caps</c> record.
+Any other return value or exception is equivalent to <c>false</c>.</p>
 </item>
 
 <tag><c>{neg, peer_filter()}</c></tag>
@@ -503,6 +510,21 @@ specified list.</p>
 
 </taglist>
 
+<p>
+Note that the <c>host</c> and <c>realm</c> filters examine the
+outgoing request as passed to <seealso marker="#call">call/4</seealso>,
+assuming that this is a record- or list-valued message() as documented
+in <seealso marker="diameter_app">diameter_app(3)</seealso>, and that
+the message contains at most one of each AVP.
+If this is not the case then the <c>{host|realm, DiameterIdentity()}</c>
+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.</p>
+
+<p>
+An invalid filter is equivalent to <c>{any, []}</c>, a filter
+that matches no peer.</p>
+
 <marker id="service_event"/>
 </item>
 
@@ -787,7 +809,7 @@ transports.</p>
 <type>
 <v>SvcName = service_name()</v>
 <v>App = application_alias()</v>
-<v>Request = diameter_app:message()</v>
+<v>Request = diameter_app:message() | term()</v>
 <v>Answer = term()</v>
 <v>Options = [call_opt()]</v>
 </type>
@@ -819,9 +841,8 @@ If there are no suitable peers, or if
 <seealso marker="diameter_app#pick_peer">pick_peer/4</seealso>
 rejects them by returning 'false', then <c>{error, no_connection}</c>
 is returned.
-If <seealso marker="diameter_app#pick_peer">pick_peer/4</seealso>
-selects a candidate peer then a request process is spawned for the
-outgoing request, in which there is a
+Otherwise <seealso marker="diameter_app#pick_peer">pick_peer/4</seealso>
+is followed by a
 <seealso
 marker="diameter_app#prepare_request">prepare_request/3</seealso>
 callback, the message is encoded and sent.</p>
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 <c>filter</c> option specified in the call to
 <seealso marker="diameter#call">diameter:call/4</seealso>, and only
 those which have indicated support for the Diameter application in
-question.</p>
+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 <c>{filter, {all, [host, realm]}}</c>
+option to <seealso marker="diameter#call">diameter:call/4</seealso>)
+will be placed at the head of the list.</p>
 
 <p>
 The return values <c>false</c> and <c>{false, State}</c> 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