From 73f89318887d473536350b1531cad27f9ab94c7c Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Thu, 22 Sep 2011 19:52:47 +0200 Subject: Fix header folding bug A prepare_request callback from diameter can return a diameter_header record with in order to set values in the header of an outgoing request. The fault in diameter_lib:fold_tuple/3 caused encode of the outgoing request to fail. --- lib/diameter/src/app/diameter_lib.erl | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) (limited to 'lib/diameter/src/app') diff --git a/lib/diameter/src/app/diameter_lib.erl b/lib/diameter/src/app/diameter_lib.erl index b5c0e1bf6a..46fd87c591 100644 --- a/lib/diameter/src/app/diameter_lib.erl +++ b/lib/diameter/src/app/diameter_lib.erl @@ -255,12 +255,13 @@ w(L) -> fold_tuple(_, T, undefined) -> T; -fold_tuple(N, T0, T) -> - element(2, lists:foldl(fun(X, {M,_} = A) -> {M+1, ft(X, A)} end, - {N, T0}, - lists:nthtail(N-1, tuple_to_list(T)))). +fold_tuple(N, T0, T1) -> + {_, T} = lists:foldl(fun(V, {I,_} = IT) -> {I+1, ft(V, IT)} end, + {N, T0}, + lists:nthtail(N-1, tuple_to_list(T1))), + T. -ft(undefined, T) -> +ft(undefined, {_, T}) -> T; -ft(X, {N, T}) -> - setelement(N, T, X). +ft(Value, {Idx, T}) -> + setelement(Idx, T, Value). -- cgit v1.2.3 From 48a715dec34a61208e82f91b49a3d3670a62d7ce Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Fri, 23 Sep 2011 17:01:39 +0200 Subject: Fix improper use of error_logger:info_report/2 Function doesn't take a format string and arguments as we called it. Instead use error_logger:info_report/1 and use the same report format as used for warning and error reports. --- lib/diameter/src/app/diameter_lib.erl | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) (limited to 'lib/diameter/src/app') diff --git a/lib/diameter/src/app/diameter_lib.erl b/lib/diameter/src/app/diameter_lib.erl index b5c0e1bf6a..36cc201dde 100644 --- a/lib/diameter/src/app/diameter_lib.erl +++ b/lib/diameter/src/app/diameter_lib.erl @@ -46,14 +46,9 @@ report(Reason, MFA) -> info_report(Reason, MFA). -info_report(Reason, {M,F,A}) -> - error_logger:info_report(" Reason: ~p~n" - " Pid: ~p~n" - " Node: ~p~n" - " Module: ~p~n" - " Function: ~p~n" - "Arguments: ~p~n", - [Reason, self(), node(), M, F, A]). +info_report(Reason, MFA) -> + report(fun error_logger:info_report/1, Reason, MFA), + true. %%% --------------------------------------------------------------------------- %%% # error_report(Reason, MFA) @@ -69,7 +64,7 @@ warning_report(Reason, MFA) -> report(fun error_logger:warning_report/1, Reason, MFA). report(Fun, Reason, MFA) -> - Fun([{reason, Reason}, {who, self()}, {where, node()}, {what, MFA}]), + Fun([{why, Reason}, {who, self()}, {what, MFA}]), false. %%% --------------------------------------------------------------------------- -- cgit v1.2.3 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/src/app/diameter_service.erl | 71 ++++++++++++++++--------------- 1 file changed, 37 insertions(+), 34 deletions(-) (limited to 'lib/diameter/src/app') 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 From 322885a2c501384bb5c83d391d0c3fa91d82d5da Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Sun, 25 Sep 2011 16:49:29 +0200 Subject: Fix sending of messages of arbitrary form diameter:call/4 can be passed anything, as long as the subsequent prepare_request callback returns a term that can be encoded. --- lib/diameter/src/app/diameter_service.erl | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) (limited to 'lib/diameter/src/app') diff --git a/lib/diameter/src/app/diameter_service.erl b/lib/diameter/src/app/diameter_service.erl index 63b0649dc4..a7fe733340 100644 --- a/lib/diameter/src/app/diameter_service.erl +++ b/lib/diameter/src/app/diameter_service.erl @@ -2561,7 +2561,7 @@ str(T) -> %% get_avp_value/3 %% -%% Support outgoing messages in one of three forms: +%% Find an AVP in a message of one of three forms: %% %% - a message record (as generated from a .dia spec) or %% - a list of an atom message name followed by 2-tuple, avp name/value pairs. @@ -2593,8 +2593,9 @@ get_avp_value(_, Name, [_MsgName | Avps]) -> undefined end; -get_avp_value(Dict, Name, Rec) - when is_tuple(Rec) -> +%% Message is typically a record but not necessarily: diameter:call/4 +%% can be passed an arbitrary term. +get_avp_value(Dict, Name, Rec) -> try Dict:'#get-'(Name, Rec) catch -- cgit v1.2.3 From 2ba082f42a77d901b5306d15f05e1717ad137e00 Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Fri, 23 Sep 2011 17:25:08 +0200 Subject: Use single format for error_logger reports --- lib/diameter/src/app/diameter_config.erl | 11 +++------- lib/diameter/src/app/diameter_internal.hrl | 23 ++++---------------- lib/diameter/src/app/diameter_peer.erl | 11 +++------- lib/diameter/src/app/diameter_reg.erl | 12 ++++------ lib/diameter/src/app/diameter_service.erl | 11 ++++++---- lib/diameter/src/app/diameter_stats.erl | 11 +++------- lib/diameter/src/app/diameter_sync.erl | 35 +++++++++++++----------------- 7 files changed, 39 insertions(+), 75 deletions(-) (limited to 'lib/diameter/src/app') diff --git a/lib/diameter/src/app/diameter_config.erl b/lib/diameter/src/app/diameter_config.erl index 42c70890b3..a6b48fe65b 100644 --- a/lib/diameter/src/app/diameter_config.erl +++ b/lib/diameter/src/app/diameter_config.erl @@ -267,7 +267,7 @@ handle_call(uptime, _, #state{id = Time} = State) -> {reply, diameter_lib:now_diff(Time), State}; handle_call(Req, From, State) -> - warning_msg("received unexpected request from ~p:~n~w", [From, Req]), + ?UNEXPECTED([Req, From]), Reply = {error, {bad_request, Req}}, {reply, Reply, State}. @@ -276,7 +276,7 @@ handle_call(Req, From, State) -> %%% ---------------------------------------------------------- handle_cast(Msg, State) -> - warning_msg("received unexpected message:~n~w", [Msg]), + ?UNEXPECTED([Msg]), {noreply, State}. %%% ---------------------------------------------------------- @@ -309,7 +309,7 @@ handle_info(restart, State) -> {noreply, State}; handle_info(Info, State) -> - warning_msg("received unknown info:~n~w", [Info]), + ?UNEXPECTED([Info]), {noreply, State}. %%-------------------------------------------------------------------- @@ -674,8 +674,3 @@ cb(M,F) -> call(Request) -> gen_server:call(?SERVER, Request, infinity). - -%% warning_msg/2 - -warning_msg(F, A) -> - ?diameter_warning("~p: " ++ F, [?MODULE | A]). diff --git a/lib/diameter/src/app/diameter_internal.hrl b/lib/diameter/src/app/diameter_internal.hrl index 9de3914830..78645b1912 100644 --- a/lib/diameter/src/app/diameter_internal.hrl +++ b/lib/diameter/src/app/diameter_internal.hrl @@ -37,9 +37,10 @@ %% Failure reports always get a stack trace. -define(STACK, erlang:get_stacktrace()). -%% Info report for anything unexpected. --define(REPORT(Reason, Func, Args), - diameter_lib:report(Reason, {?MODULE, Func, Args})). +%% Warning report for unexpected messages in various processes. +-define(UNEXPECTED(F,A), + diameter_lib:warning_report(unexpected, {?MODULE, F, A})). +-define(UNEXPECTED(A), ?UNEXPECTED(?FUNC, A)). %% Something to trace on. -define(LOG(Slogan, Details), @@ -77,19 +78,3 @@ server_id, is_dynamic, expiration}). - -%%%---------------------------------------------------------------------- -%%% Error/warning/info message macro(s) -%%%---------------------------------------------------------------------- - --define(diameter_info(F, A), - (catch error_logger:info_msg("[ ~w : ~w : ~p ] ~n" ++ F ++ "~n", - [?APPLICATION, ?MODULE, self()|A]))). - --define(diameter_warning(F, A), - (catch error_logger:warning_msg("[ ~w : ~w : ~p ] ~n" ++ F ++ "~n", - [?APPLICATION, ?MODULE, self()|A]))). - --define(diameter_error(F, A), - (catch error_logger:error_msg("[ ~w : ~w : ~p ] ~n" ++ F ++ "~n", - [?APPLICATION, ?MODULE, self()|A]))). diff --git a/lib/diameter/src/app/diameter_peer.erl b/lib/diameter/src/app/diameter_peer.erl index 6b8971b8ea..3e78c4caef 100644 --- a/lib/diameter/src/app/diameter_peer.erl +++ b/lib/diameter/src/app/diameter_peer.erl @@ -148,7 +148,7 @@ handle_call(uptime, _, #state{id = Time} = State) -> {reply, diameter_lib:now_diff(Time), State}; handle_call(Req, From, State) -> - warning_msg("received unexpected request from ~p:~n~w", [From, Req]), + ?UNEXPECTED([Req, From]), {reply, nok, State}. %%% ---------------------------------------------------------- @@ -156,7 +156,7 @@ handle_call(Req, From, State) -> %%% ---------------------------------------------------------- handle_cast(Msg, State) -> - warning_msg("received unexpected message:~n~w", [Msg]), + ?UNEXPECTED([Msg]), {noreply, State}. %%% ---------------------------------------------------------- @@ -169,7 +169,7 @@ handle_info({notify, SvcName, T}, S) -> {noreply, S}; handle_info(Info, State) -> - warning_msg("received unexpected info:~n~w", [Info]), + ?UNEXPECTED([Info]), {noreply, State}. %% ---------------------------------------------------------- @@ -223,8 +223,3 @@ value([], V) -> call(Request) -> gen_server:call(?SERVER, Request, infinity). - -%% warning_msg/2 - -warning_msg(F, A) -> - ?diameter_warning("~p: " ++ F, [?MODULE | A]). diff --git a/lib/diameter/src/app/diameter_reg.erl b/lib/diameter/src/app/diameter_reg.erl index 8e5f34c2c3..882b9da238 100644 --- a/lib/diameter/src/app/diameter_reg.erl +++ b/lib/diameter/src/app/diameter_reg.erl @@ -243,7 +243,8 @@ handle_call(state, _, State) -> handle_call(uptime, _, #state{id = Time} = State) -> {reply, diameter_lib:now_diff(Time), State}; -handle_call(_Req, _From, State) -> +handle_call(Req, From, State) -> + ?UNEXPECTED([Req, From]), {reply, nok, State}. %%% ---------------------------------------------------------- @@ -251,7 +252,7 @@ handle_call(_Req, _From, State) -> %%% ---------------------------------------------------------- handle_cast(Msg, State)-> - warning_msg("received unexpected message:~n~w", [Msg]), + ?UNEXPECTED([Msg]), {noreply, State}. %%% ---------------------------------------------------------- @@ -264,7 +265,7 @@ handle_info({'DOWN', MRef, process, Pid, _}, State) -> {noreply, State}; handle_info(Info, State) -> - warning_msg("received unknown info:~n~w", [Info]), + ?UNEXPECTED([Info]), {noreply, State}. %%% ---------------------------------------------------------- @@ -324,8 +325,3 @@ repl([], _, _) -> call(Request) -> gen_server:call(?SERVER, Request, infinity). - -%% warning_msg/2 - -warning_msg(F, A) -> - ?diameter_warning("~p: " ++ F, [?MODULE | A]). diff --git a/lib/diameter/src/app/diameter_service.erl b/lib/diameter/src/app/diameter_service.erl index 63b0649dc4..45e6b90d14 100644 --- a/lib/diameter/src/app/diameter_service.erl +++ b/lib/diameter/src/app/diameter_service.erl @@ -463,7 +463,7 @@ handle_call(stop, _From, S) -> %% stating a monitor that waits for DOWN before returning. handle_call(Req, From, S) -> - ?REPORT(unknown_request, ?FUNC, [Req, From]), + unexpected(handle_call, [Req, From], S), {reply, nok, S}. %%% --------------------------------------------------------------------------- @@ -471,7 +471,7 @@ handle_call(Req, From, S) -> %%% --------------------------------------------------------------------------- handle_cast(Req, S) -> - ?REPORT(unknown_request, ?FUNC, [Req]), + unexpected(handle_cast, [Req], S), {noreply, S}. %%% --------------------------------------------------------------------------- @@ -553,8 +553,8 @@ transition({failover, TRef, Seqs}, S) -> failover(TRef, Seqs, S), ok; -transition(Req, _) -> - ?REPORT(unknown_request, ?FUNC, [Req]), +transition(Req, S) -> + unexpected(handle_info, [Req], S), ok. %%% --------------------------------------------------------------------------- @@ -591,6 +591,9 @@ code_change(FromVsn, SvcName, Extra, #diameter_app{alias = Alias} = A) -> %% =========================================================================== %% =========================================================================== +unexpected(F, A, #state{service_name = Name}) -> + ?UNEXPECTED(F, A ++ [Name]). + cb([_|_] = M, F, A) -> eval(M, F, A); cb(Rec, F, A) -> diff --git a/lib/diameter/src/app/diameter_stats.erl b/lib/diameter/src/app/diameter_stats.erl index b52d4cdcfb..71479afa95 100644 --- a/lib/diameter/src/app/diameter_stats.erl +++ b/lib/diameter/src/app/diameter_stats.erl @@ -207,7 +207,7 @@ handle_call({flush, Contrib}, _From, State) -> {reply, fetch(Contrib), State}; handle_call(Req, From, State) -> - warning_msg("received unexpected request from ~p:~n~w", [From, Req]), + ?UNEXPECTED([Req, From]), {reply, nok, State}. %% ---------------------------------------------------------- @@ -219,7 +219,7 @@ handle_cast({incr, Rec}, State) -> {noreply, State}; handle_cast(Msg, State) -> - warning_msg("received unexpected message:~n~w", [Msg]), + ?UNEXPECTED([Msg]), {noreply, State}. %% ---------------------------------------------------------- @@ -231,7 +231,7 @@ handle_info({'DOWN', _MRef, process, Pid, _}, State) -> {noreply, State}; handle_info(Info, State) -> - warning_msg("received unknown info:~n~w", [Info]), + ?UNEXPECTED([Info]), {noreply, State}. %% ---------------------------------------------------------- @@ -340,8 +340,3 @@ cast(Msg) -> call(Request) -> gen_server:call(?SERVER, Request, infinity). - -%% warning_msg/2 - -warning_msg(F, A) -> - ?diameter_warning("~p: " ++ F, [?MODULE | A]). diff --git a/lib/diameter/src/app/diameter_sync.erl b/lib/diameter/src/app/diameter_sync.erl index f7777ae809..ce2db4b3a2 100644 --- a/lib/diameter/src/app/diameter_sync.erl +++ b/lib/diameter/src/app/diameter_sync.erl @@ -204,37 +204,37 @@ handle_call(?REQUEST(Type, Name, Req, Max, Timeout), T = find(Name, QD), nq(queued(T) =< Max, T, {Type, From}, Name, Req, Timeout, State); -handle_call(Request, _From, State) -> - {reply, call(Request, State), State}. +handle_call(Request, From, State) -> + {reply, call(Request, From, State), State}. -%% call/2 +%% call/3 -call(?CARP(Name), #state{queue = QD}) -> +call(?CARP(Name), _, #state{queue = QD}) -> pcar(find(Name, QD)); -call(state, State) -> +call(state, _, State) -> State; -call(uptime, #state{time = T}) -> +call(uptime, _, #state{time = T}) -> diameter_lib:now_diff(T); -call({flush, Name}, #state{queue = QD}) -> +call({flush, Name}, _, #state{queue = QD}) -> cancel(find(Name, QD)); -call(pending, #state{pending = N}) -> +call(pending, _, #state{pending = N}) -> N; -call({pending, Name}, #state{queue = QD}) -> +call({pending, Name}, _, #state{queue = QD}) -> queued(find(Name, QD)); -call(queues, #state{queue = QD}) -> +call(queues, _, #state{queue = QD}) -> fetch_keys(QD); -call({pids, Name}, #state{queue = QD}) -> +call({pids, Name}, _, #state{queue = QD}) -> plist(find(Name, QD)); -call(Req, _State) -> %% ignore - warning_msg("received unexpected request:~n~w", [Req]), +call(Req, From, _State) -> %% ignore + ?UNEXPECTED(handle_call, [Req, From]), nok. %%% ---------------------------------------------------------- @@ -242,7 +242,7 @@ call(Req, _State) -> %% ignore %%% ---------------------------------------------------------- handle_cast(Msg, State) -> - warning_msg("received unexpected message:~n~w", [Msg]), + ?UNEXPECTED([Msg]), {noreply, State}. %%% ---------------------------------------------------------- @@ -267,7 +267,7 @@ info({'DOWN', MRef, process, Pid, Info}, queue = dq(fetch(Name, QD), Pid, Info, Name, QD)}; info(Info, State) -> - warning_msg("received unknown info:~n~w", [Info]), + ?UNEXPECTED(handle_info, [Info]), State. reply({call, From}, T) -> @@ -548,8 +548,3 @@ gen_call(Server, Req, Timeout) -> exit: _ -> timeout end. - -%% warning_msg/2 - -warning_msg(F, A) -> - ?diameter_warning("~p: " ++ F, [?MODULE | A]). -- cgit v1.2.3 From 06a771d3d8915baeaad3908d8056eba45609d32d Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Fri, 23 Sep 2011 18:52:14 +0200 Subject: Makefile dependency fix Dependency of generated dictionary modules on diameter.hrl and diameter_gen.hrl was missed. --- lib/diameter/src/app/Makefile | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'lib/diameter/src/app') diff --git a/lib/diameter/src/app/Makefile b/lib/diameter/src/app/Makefile index 6de220d282..229466e264 100644 --- a/lib/diameter/src/app/Makefile +++ b/lib/diameter/src/app/Makefile @@ -181,6 +181,10 @@ release_docs_spec: # Dependencies # ---------------------------------------------------- +$(SPEC_FILES:%.dia=$(EBIN)/%.$(EMULATOR)): \ + $(DIAMETER_TOP)/include/diameter.hrl \ + $(DIAMETER_TOP)/include/diameter_gen.hrl + depend: depend.mk # Generate dependencies makefile. It's assumed that the compile target -- cgit v1.2.3 From fbb8586af3dd74b52ecdfa74d758f14cb82683fd Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Mon, 26 Sep 2011 00:21:08 +0200 Subject: Fix and clarify relay behaviour Leave it up to a handle_request callback to decide whether or not to filter the peer from which the incoming request was sent. Reply with 3002 (DIAMETER_UNABLE_TO_DELIVER) on anything but an answer from the peer. --- lib/diameter/src/app/diameter_service.erl | 83 +++++++++++++------------------ 1 file changed, 34 insertions(+), 49 deletions(-) (limited to 'lib/diameter/src/app') diff --git a/lib/diameter/src/app/diameter_service.erl b/lib/diameter/src/app/diameter_service.erl index 63b0649dc4..b7b9ac1ef4 100644 --- a/lib/diameter/src/app/diameter_service.erl +++ b/lib/diameter/src/app/diameter_service.erl @@ -1685,9 +1685,9 @@ recv_request({Id, Alias}, T, TPid, Apps, Caps, Pkt) -> %% DIAMETER_APPLICATION_UNSUPPORTED 3007 %% A request was sent for an application that is not supported. -recv_request(false, {_, OH, OR}, TPid, _, _, Pkt) -> - ?LOG({error, application}, Pkt), - reply(answer_message({OH, OR, 3007}, collect_avps(Pkt)), ?BASE, TPid, Pkt). +recv_request(false, T, TPid, _, _, Pkt) -> + As = collect_avps(Pkt), + protocol_error(3007, T, TPid, Pkt#diameter_packet{avps = As}). collect_avps(Pkt) -> case diameter_codec:collect_avps(Pkt) of @@ -1706,13 +1706,9 @@ collect_avps(Pkt) -> %% set to an unrecognized value, or that is inconsistent with the %% AVP's definition. %% -recv_request({_, OH, OR}, {TPid, _}, _, #diameter_packet{errors = [Bs | _], - bin = Bin, - avps = Avps} - = Pkt) +recv_request(T, {TPid, _}, _, #diameter_packet{errors = [Bs | _]} = Pkt) when is_bitstring(Bs) -> - ?LOG({error, invalid_avp_bits}, Bin), - reply(answer_message({OH, OR, 3009}, Avps), ?BASE, TPid, Pkt); + protocol_error(3009, T, TPid, Pkt); %% Either we support this application but don't recognize the command %% or we're a relay and the command isn't proxiable. @@ -1722,18 +1718,15 @@ recv_request({_, OH, OR}, {TPid, _}, _, #diameter_packet{errors = [Bs | _], %% recognize or support. This MUST be used when a Diameter node %% receives an experimental command that it does not understand. %% -recv_request({_, OH, OR}, +recv_request(T, {TPid, _}, #diameter_app{id = Id}, #diameter_packet{header = #diameter_header{is_proxiable = P}, - msg = M, - avps = Avps, - bin = Bin} + msg = M} = Pkt) when ?APP_ID_RELAY /= Id, undefined == M; ?APP_ID_RELAY == Id, not P -> - ?LOG({error, command_unsupported}, Bin), - reply(answer_message({OH, OR, 3001}, Avps), ?BASE, TPid, Pkt); + protocol_error(3001, T, TPid, Pkt); %% Error bit was set on a request. %% @@ -1742,15 +1735,12 @@ recv_request({_, OH, OR}, %% either set to an invalid combination, or to a value that is %% inconsistent with the command code's definition. %% -recv_request({_, OH, OR}, +recv_request(T, {TPid, _}, _, - #diameter_packet{header = #diameter_header{is_error = true}, - avps = Avps, - bin = Bin} + #diameter_packet{header = #diameter_header{is_error = true}} = Pkt) -> - ?LOG({error, error_bit}, Bin), - reply(answer_message({OH, OR, 3008}, Avps), ?BASE, TPid, Pkt); + protocol_error(3008, T, TPid, Pkt); %% A message in a locally supported application or a proxiable message %% in the relay application. Don't distinguish between the two since @@ -1878,7 +1868,7 @@ resend(true, _, _, T, {TPid, _}, Pkt) -> %% Route-Record loop resend(false, Opts, App, - {SvcName, _, _}, + {SvcName, _, _} = T, {TPid, #diameter_caps{origin_host = {_, OH}}}, #diameter_packet{header = Hdr0, avps = Avps} @@ -1887,46 +1877,41 @@ resend(false, Seq = diameter_session:sequence(), Hdr = Hdr0#diameter_header{hop_by_hop_id = Seq}, Msg = [Hdr, Route | Avps], - %% Filter sender as ineligible receiver. - reply(call(SvcName, App, Msg, [{filter, {neg, {host, OH}}} | Opts]), - TPid, - Pkt). + resend(call(SvcName, App, Msg, Opts), T, TPid, Pkt). %% The incoming request is relayed with the addition of a -%% Route-Record. Note the requirement on the return from call/4. -%% This places a requirement on the values returned by the -%% handle_answer and handle_error callbacks of the application module -%% in question. +%% Route-Record. Note the requirement on the return from call/4 below, +%% which places a requirement on the value returned by the +%% handle_answer callback of the application module in question. +%% +%% Note that there's nothing stopping the request from being relayed +%% back to the sender. A pick_peer callback may want to avoid this but +%% a smart peer might recognize the potential loop and choose another +%% route. A less smart one will probably just relay the request back +%% again and force us to detect the loop. A pick_peer that wants to +%% avoid this can specify filter to avoid the possibility. +%% Eg. {neg, {host, OH} where #diameter_caps{origin_host = {OH, _}}. %% %% RFC 6.3 says that a relay agent does not modify Origin-Host but %% says nothing about a proxy. Assume it should behave the same way. -%% reply/3 +%% resend/4 %% %% Relay a reply to a relayed request. %% Answer from the peer: reset the hop by hop identifier and send. -reply(#diameter_packet{bin = B} - = Pkt, - TPid, - #diameter_packet{header = #diameter_header{hop_by_hop_id = Id}, - transport_data = TD}) -> +resend(#diameter_packet{bin = B} + = Pkt, + _, + TPid, + #diameter_packet{header = #diameter_header{hop_by_hop_id = Id}, + transport_data = TD}) -> send(TPid, Pkt#diameter_packet{bin = diameter_codec:hop_by_hop_id(Id, B), transport_data = TD}); %% TODO: counters -%% Not. Ignoring the error feels harsh but there is no appropriate -%% Result-Code for a protocol error (which this isn't really anyway) -%% and the RFC doesn't provide any guidance how to act. A weakness -%% here is that we don't deal well with a decode error: the request -%% will simply timeout on the peer's end. Better would be to just send -%% the answer (with modified hop by hop identifier) on regardless, at -%% least in the relay case in which there's no examination of the -%% answer. In the proxy case it's not clear that the callback won't -%% examine the answer. Just be quiet here since a decode error causes -%% the request process to crash (or not depending on the error and -%% config and/or handle_answer callback). -reply(_, _, _) -> - ok. +%% Or not: DIAMETER_UNABLE_TO_DELIVER. +resend(_, T, TPid, Pkt) -> + protocol_error(3002, T, TPid, Pkt). %% is_loop/4 %% -- cgit v1.2.3 From 4a64f3a7d64e3aae364508054c0c1a9273db2301 Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Thu, 22 Sep 2011 19:59:03 +0200 Subject: Fix bugs in sending of answer-message replies 3001 (DIAMETER_COMMAND_UNSUPPORTED) was not sent since the decode placed the AVP list in the errors field rather than the avps field of the diameter_packet, causing the subsequent encode to fail. Session-Id was also set improperly, causing encode to fail even in this case. --- lib/diameter/src/app/diameter_codec.erl | 22 +++++++--------------- lib/diameter/src/app/diameter_service.erl | 28 +++++++--------------------- 2 files changed, 14 insertions(+), 36 deletions(-) (limited to 'lib/diameter/src/app') diff --git a/lib/diameter/src/app/diameter_codec.erl b/lib/diameter/src/app/diameter_codec.erl index f6cbde5446..d88f42fb7c 100644 --- a/lib/diameter/src/app/diameter_codec.erl +++ b/lib/diameter/src/app/diameter_codec.erl @@ -140,10 +140,10 @@ make_flags(Flags0, #diameter_header{is_request = R, mf(undefined, F, _) -> F; mf(B, F, N) -> %% reset the affected bit - (F bxor (F band (1 bsl N))) bor (bit(B) bsl N). + (F bxor (F band (1 bsl N))) bor bit(B, N). -bit(true) -> 1; -bit(false) -> 0. +bit(true, N) -> 1 bsl N; +bit(false, _) -> 0. %% values/1 @@ -199,25 +199,16 @@ msg_header(Mod, MsgName, Header) -> p(Flags, #diameter_header{is_request = true, is_proxiable = P}) -> - Flags bor choose(P, 2#01000000, 0); + Flags band (2#10110000 bor choose(P, 2#01000000, 0)); p(Flags, _) -> Flags. -%% The header below is that of the incoming request being answered, -%% not of the answer (which hasn't been encoded yet). - h(Mod, 'answer-message' = MsgName, Header) -> ?BASE = Mod, - #diameter_header{is_request = true, - cmd_code = Code} - = Header, + #diameter_header{cmd_code = Code} = Header, {_, Flags, ApplId} = ?BASE:msg_header(MsgName), {Code, Flags, ApplId}; -h(Mod, MsgName, #diameter_header{is_request = true, - cmd_code = Code}) -> - {Code, _, _} = Mod:msg_header(MsgName); %% ensure Code - h(Mod, MsgName, _) -> Mod:msg_header(MsgName). @@ -290,7 +281,8 @@ decode_avps(MsgName, Mod, Pkt, {Bs, Avps}) -> %% invalid avp bits ... decode_avps('', Mod, Pkt, Avps) -> %% unknown message ... ?LOG(unknown, {Mod, Pkt#diameter_packet.header}), - Pkt#diameter_packet{errors = lists:reverse(Avps)}; + Pkt#diameter_packet{avps = lists:reverse(Avps), + errors = [3001]}; %% DIAMETER_COMMAND_UNSUPPORTED %% msg = undefined identifies this case. decode_avps(MsgName, Mod, Pkt, Avps) -> %% ... or not diff --git a/lib/diameter/src/app/diameter_service.erl b/lib/diameter/src/app/diameter_service.erl index 63b0649dc4..01a125fdc7 100644 --- a/lib/diameter/src/app/diameter_service.erl +++ b/lib/diameter/src/app/diameter_service.erl @@ -1971,24 +1971,20 @@ reply(Msg, Dict, TPid, #diameter_packet{errors = [H|_] = Es} = Pkt) -> %% make_reply_packet/2 +%% Binaries and header/avp lists are sent as-is. make_reply_packet(Bin, _) when is_binary(Bin) -> #diameter_packet{bin = Bin}; - make_reply_packet([#diameter_header{} | _] = Msg, _) -> #diameter_packet{msg = Msg}; +%% Otherwise a reply message clears the R and T flags and retains the +%% P flag. The E flag will be set at encode. make_reply_packet(Msg, #diameter_packet{header = ReqHdr}) -> - #diameter_header{end_to_end_id = EId, - hop_by_hop_id = Hid, - is_proxiable = P} - = ReqHdr, - - Hdr = #diameter_header{version = ?DIAMETER_VERSION, - end_to_end_id = EId, - hop_by_hop_id = Hid, - is_proxiable = P, - is_retransmitted = false}, + Hdr = ReqHdr#diameter_header{version = ?DIAMETER_VERSION, + is_request = false, + is_error = undefined, + is_retransmitted = false}, #diameter_packet{header = Hdr, msg = Msg}. @@ -2123,16 +2119,6 @@ answer_message({OH, OR, RC}, Avps) -> {'Result-Code', RC} | session_id(Code, Vid, Avps)]. -session_id(Code, Vid, Avps) - when is_list(Avps) -> - try - {value, #diameter_avp{} = Avp} = find_avp(Code, Vid, Avps), - Avp - catch - error: _ -> - [] - end; - session_id(Code, Vid, Avps) when is_list(Avps) -> try -- cgit v1.2.3