From 5c0024cd76782f67499a09f7c524845d913408fc Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Wed, 30 Aug 2017 23:32:44 +0200 Subject: Enumerate AVPs in diameter_avp.index (again) Commit 96cd627a changed the way the index field was used, but the enumeration is used in at least one known application (as a pointer from elements of diameter_packet.errors to elements of diameter_packet.avps) and the motivation for the change is questionable: the lookup that was avoided was unnecessary given that it was already performed in incrementing a counter. Revert to enumerating as before in the non-relay case, but not in the relay case since there's no corresponding usecase. --- lib/diameter/src/base/diameter_gen.erl | 63 ++++++++++++++++++---------------- 1 file changed, 33 insertions(+), 30 deletions(-) (limited to 'lib/diameter') diff --git a/lib/diameter/src/base/diameter_gen.erl b/lib/diameter/src/base/diameter_gen.erl index 7a1a46ec52..2ef27e4b2e 100644 --- a/lib/diameter/src/base/diameter_gen.erl +++ b/lib/diameter/src/base/diameter_gen.erl @@ -238,7 +238,7 @@ enc(AvpName, Value, Opts, Mod) -> decode_avps(Name, Bin, #{module := Mod, decode_format := Fmt} = Opts) -> Strict = mget(strict_arities, Opts, decode), [AM, Avps, Failed | Rec] - = decode(Bin, Name, Mod, Fmt, Strict, Opts, #{}), + = decode(Bin, Name, Mod, Fmt, Strict, Opts, 0, #{}), %% AM counts the number of top-level AVPs, which missing/5 then %% uses when appending 5005 errors. {reformat(Name, Rec, Strict, Mod, Fmt), @@ -249,7 +249,7 @@ decode_avps(Name, Bin, #{module := Mod, decode_format := Fmt} = Opts) -> %% encountered. Failed-AVP should typically contain the first %% error encountered. -%% decode/7 +%% decode/8 decode(<>, Name, @@ -257,6 +257,7 @@ decode(<>, Fmt, Strict, Opts, + Idx, AM) -> decode(Rest, Code, @@ -270,21 +271,23 @@ decode(<>, Fmt, Strict, Opts, + Idx, AM); -decode(<<>>, Name, Mod, Fmt, Strict, _, AM) -> +decode(<<>>, Name, Mod, Fmt, Strict, _, _, AM) -> [AM, [], [] | newrec(Fmt, Mod, Name, Strict)]; -decode(Bin, Name, Mod, Fmt, Strict, _, AM) -> - Avp = #diameter_avp{data = Bin}, +decode(Bin, Name, Mod, Fmt, Strict, _, Idx, AM) -> + Avp = #diameter_avp{data = Bin, index = Idx}, [AM, [Avp], [{5014, Avp}] | newrec(Fmt, Mod, Name, Strict)]. -%% decode/13 +%% decode/14 -decode(Bin, Code, Vid, DataLen, Pad, M, P, Name, Mod, Fmt, Strict, Opts0, AM0) -> +decode(Bin, Code, Vid, DataLen, Pad, M, P, Name, Mod, Fmt, Strict, Opts0, + Idx, AM0) -> case Bin of <> -> - {NameT, AvpName, Arity, {Idx, AM}} + {NameT, AvpName, Arity, {I, AM}} = incr(Name, Code, Vid, M, Mod, Strict, Opts0, AM0), Opts = setopts(NameT, Name, M, Opts0), @@ -300,11 +303,11 @@ decode(Bin, Code, Vid, DataLen, Pad, M, P, Name, Mod, Fmt, Strict, Opts0, AM0) - type = type(NameT), index = Idx}, - Dec = decode(Data, Name, NameT, Mod, Opts, Avp), %% decode - Acc = decode(T, Name, Mod, Fmt, Strict, Opts, AM), %% recurse - acc(Acc, Dec, Name, AvpName, Arity, Strict, Mod, Opts); + Dec = decode(Data, Name, NameT, Mod, Opts, Avp), %% decode + Acc = decode(T, Name, Mod, Fmt, Strict, Opts, Idx+1, AM),%% recurse + acc(Acc, Dec, I, Name, AvpName, Arity, Strict, Mod, Opts); _ -> - {NameT, _AvpName, _Arity, {Idx, AM}} + {NameT, _AvpName, _Arity, {_, AM}} = incr(Name, Code, Vid, M, Mod, Strict, Opts0, AM0), Avp = #diameter_avp{code = Code, @@ -577,57 +580,57 @@ set_failed('Failed-AVP', #{failed_avp := false} = Opts) -> set_failed(_, Opts) -> Opts. -%% acc/8 +%% acc/9 -acc([AM | Acc], As, Name, AvpName, Arity, Strict, Mod, Opts) -> - [AM | acc1(Acc, As, Name, AvpName, Arity, Strict, Mod, Opts)]. +acc([AM | Acc], As, I, Name, AvpName, Arity, Strict, Mod, Opts) -> + [AM | acc1(Acc, As, I, Name, AvpName, Arity, Strict, Mod, Opts)]. -%% acc1/8 +%% acc1/9 %% Faulty AVP, not grouped. -acc1(Acc, {_RC, Avp} = E, _, _, _, _, _, _) -> +acc1(Acc, {_RC, Avp} = E, _, _, _, _, _, _, _) -> [Avps, Failed | Rec] = Acc, [[Avp | Avps], [E | Failed] | Rec]; %% Faulty component in grouped AVP. -acc1(Acc, {RC, As, Avp}, _, _, _, _, _, _) -> +acc1(Acc, {RC, As, Avp}, _, _, _, _, _, _, _) -> [Avps, Failed | Rec] = Acc, [[As | Avps], [{RC, Avp} | Failed] | Rec]; %% Grouped AVP ... -acc1([Avps | Acc], [Avp|_] = As, Name, AvpName, Arity, Strict, Mod, Opts) -> - [[As|Avps] | acc2(Acc, Avp, Name, AvpName, Arity, Strict, Mod, Opts)]; +acc1([Avps | Acc], [Avp|_] = As, I, Name, AvpName, Arity, Strict, Mod, Opts) -> + [[As|Avps] | acc2(Acc, Avp, I, Name, AvpName, Arity, Strict, Mod, Opts)]; %% ... or not. -acc1([Avps | Acc], Avp, Name, AvpName, Arity, Strict, Mod, Opts) -> - [[Avp|Avps] | acc2(Acc, Avp, Name, AvpName, Arity, Strict, Mod, Opts)]. +acc1([Avps | Acc], Avp, I, Name, AvpName, Arity, Strict, Mod, Opts) -> + [[Avp|Avps] | acc2(Acc, Avp, I, Name, AvpName, Arity, Strict, Mod, Opts)]. -%% acc2/8 +%% acc2/9 %% No errors, but nowhere to pack. -acc2(Acc, Avp, _, 'AVP', 0, _, _, _) -> +acc2(Acc, Avp, _, _, 'AVP', 0, _, _, _) -> [Failed | Rec] = Acc, [[{rc(Avp), Avp} | Failed] | Rec]; %% No AVP of this name: try to pack as 'AVP'. -acc2(Acc, Avp, Name, AvpName, 0, Strict, Mod, Opts) -> +acc2(Acc, Avp, I, Name, AvpName, 0, Strict, Mod, Opts) -> M = Avp#diameter_avp.is_mandatory, Arity = pack_arity(Name, AvpName, Opts, Mod, M), - acc2(Acc, Avp, Name, 'AVP', Arity, Strict, Mod, Opts); + acc2(Acc, Avp, I, Name, 'AVP', Arity, Strict, Mod, Opts); %% Relaxed arities. -acc2(Acc, Avp, _, AvpName, Arity, Strict, Mod, _) +acc2(Acc, Avp, _, _, AvpName, Arity, Strict, Mod, _) when Strict /= decode -> pack(Arity, AvpName, Avp, Mod, Acc); %% No maximum arity. -acc2(Acc, Avp, _, AvpName, {_,'*'} = Arity, _, Mod, _) -> +acc2(Acc, Avp, _, _, AvpName, {_,'*'} = Arity, _, Mod, _) -> pack(Arity, AvpName, Avp, Mod, Acc); %% Or check. -acc2(Acc, Avp, _, AvpName, Arity, _, Mod, _) -> +acc2(Acc, Avp, I, _, AvpName, Arity, _, Mod, _) -> Mx = max_arity(Arity), - if Mx =< Avp#diameter_avp.index -> + if Mx =< I -> [Failed | Rec] = Acc, [[{5009, Avp} | Failed] | Rec]; true -> -- cgit v1.2.3 From 883ad0559b1a3551688866d88fa3089728e13b0c Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Thu, 31 Aug 2017 00:28:05 +0200 Subject: Fix decode of too many generic AVPs That is, when the arity of an 'AVP' field has an upper bound. This shouldn't happen in practice, but if an AVP is known but its name not explicit in the message grammar then its count was confused with that of AVPs packed into the 'AVP' field. --- lib/diameter/src/base/diameter_gen.erl | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) (limited to 'lib/diameter') diff --git a/lib/diameter/src/base/diameter_gen.erl b/lib/diameter/src/base/diameter_gen.erl index 2ef27e4b2e..2f84b2eae6 100644 --- a/lib/diameter/src/base/diameter_gen.erl +++ b/lib/diameter/src/base/diameter_gen.erl @@ -326,9 +326,14 @@ decode(Bin, Code, Vid, DataLen, Pad, M, P, Name, Mod, Fmt, Strict, Opts0, incr(Name, Code, Vid, M, Mod, Strict, Opts, AM0) -> NameT = Mod:avp_name(Code, Vid), %% {AvpName, Type} | 'AVP' - AvpName = field(NameT), - Arity = avp_arity(Name, AvpName, Mod, Opts, M), - {NameT, AvpName, Arity, incr(AvpName, Arity, Strict, AM0)}. + Field = field(NameT), %% AvpName | 'AVP' + Arity = avp_arity(Name, Field, Mod, Opts, M), + if 0 == Arity, 'AVP' /= Field -> + A = pack_arity(Name, Field, Opts, Mod, M), + {NameT, 'AVP', A, incr('AVP', A, Strict, AM0)}; + true -> + {NameT, Field, Arity, incr(Field, Arity, Strict, AM0)} + end. %% Data is a truncated header if command_code = undefined, otherwise %% payload bytes. The former is padded to the length of a header if @@ -345,9 +350,8 @@ setopts({_, Type}, Name, M, Opts) -> %% incr/4 -incr(F, A, SA, AM) - when F == 'AVP'; - A == ?ANY; +incr(_, A, SA, AM) + when A == ?ANY; A == 0; SA /= decode -> {undefined, AM}; @@ -612,12 +616,6 @@ acc2(Acc, Avp, _, _, 'AVP', 0, _, _, _) -> [Failed | Rec] = Acc, [[{rc(Avp), Avp} | Failed] | Rec]; -%% No AVP of this name: try to pack as 'AVP'. -acc2(Acc, Avp, I, Name, AvpName, 0, Strict, Mod, Opts) -> - M = Avp#diameter_avp.is_mandatory, - Arity = pack_arity(Name, AvpName, Opts, Mod, M), - acc2(Acc, Avp, I, Name, 'AVP', Arity, Strict, Mod, Opts); - %% Relaxed arities. acc2(Acc, Avp, _, _, AvpName, Arity, Strict, Mod, _) when Strict /= decode -> -- cgit v1.2.3 From ff4a0956d132c3af002bfbe5d48f18e492d3763f Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Thu, 31 Aug 2017 00:32:53 +0200 Subject: Rename variable It's no longer the AVP name as of the parent commit, but the name of the field/member the value will be stored in. Typically the AVP name, but possibly 'AVP'. --- lib/diameter/src/base/diameter_gen.erl | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) (limited to 'lib/diameter') diff --git a/lib/diameter/src/base/diameter_gen.erl b/lib/diameter/src/base/diameter_gen.erl index 2f84b2eae6..f9172ec59d 100644 --- a/lib/diameter/src/base/diameter_gen.erl +++ b/lib/diameter/src/base/diameter_gen.erl @@ -287,7 +287,7 @@ decode(Bin, Code, Vid, DataLen, Pad, M, P, Name, Mod, Fmt, Strict, Opts0, Idx, AM0) -> case Bin of <> -> - {NameT, AvpName, Arity, {I, AM}} + {NameT, Field, Arity, {I, AM}} = incr(Name, Code, Vid, M, Mod, Strict, Opts0, AM0), Opts = setopts(NameT, Name, M, Opts0), @@ -305,9 +305,9 @@ decode(Bin, Code, Vid, DataLen, Pad, M, P, Name, Mod, Fmt, Strict, Opts0, Dec = decode(Data, Name, NameT, Mod, Opts, Avp), %% decode Acc = decode(T, Name, Mod, Fmt, Strict, Opts, Idx+1, AM),%% recurse - acc(Acc, Dec, I, Name, AvpName, Arity, Strict, Mod, Opts); + acc(Acc, Dec, I, Name, Field, Arity, Strict, Mod, Opts); _ -> - {NameT, _AvpName, _Arity, {_, AM}} + {NameT, _Field, _Arity, {_, AM}} = incr(Name, Code, Vid, M, Mod, Strict, Opts0, AM0), Avp = #diameter_avp{code = Code, @@ -586,8 +586,8 @@ set_failed(_, Opts) -> %% acc/9 -acc([AM | Acc], As, I, Name, AvpName, Arity, Strict, Mod, Opts) -> - [AM | acc1(Acc, As, I, Name, AvpName, Arity, Strict, Mod, Opts)]. +acc([AM | Acc], As, I, Name, Field, Arity, Strict, Mod, Opts) -> + [AM | acc1(Acc, As, I, Name, Field, Arity, Strict, Mod, Opts)]. %% acc1/9 @@ -602,12 +602,12 @@ acc1(Acc, {RC, As, Avp}, _, _, _, _, _, _, _) -> [[As | Avps], [{RC, Avp} | Failed] | Rec]; %% Grouped AVP ... -acc1([Avps | Acc], [Avp|_] = As, I, Name, AvpName, Arity, Strict, Mod, Opts) -> - [[As|Avps] | acc2(Acc, Avp, I, Name, AvpName, Arity, Strict, Mod, Opts)]; +acc1([Avps | Acc], [Avp|_] = As, I, Name, Field, Arity, Strict, Mod, Opts) -> + [[As|Avps] | acc2(Acc, Avp, I, Name, Field, Arity, Strict, Mod, Opts)]; %% ... or not. -acc1([Avps | Acc], Avp, I, Name, AvpName, Arity, Strict, Mod, Opts) -> - [[Avp|Avps] | acc2(Acc, Avp, I, Name, AvpName, Arity, Strict, Mod, Opts)]. +acc1([Avps | Acc], Avp, I, Name, Field, Arity, Strict, Mod, Opts) -> + [[Avp|Avps] | acc2(Acc, Avp, I, Name, Field, Arity, Strict, Mod, Opts)]. %% acc2/9 @@ -617,22 +617,22 @@ acc2(Acc, Avp, _, _, 'AVP', 0, _, _, _) -> [[{rc(Avp), Avp} | Failed] | Rec]; %% Relaxed arities. -acc2(Acc, Avp, _, _, AvpName, Arity, Strict, Mod, _) +acc2(Acc, Avp, _, _, Field, Arity, Strict, Mod, _) when Strict /= decode -> - pack(Arity, AvpName, Avp, Mod, Acc); + pack(Arity, Field, Avp, Mod, Acc); %% No maximum arity. -acc2(Acc, Avp, _, _, AvpName, {_,'*'} = Arity, _, Mod, _) -> - pack(Arity, AvpName, Avp, Mod, Acc); +acc2(Acc, Avp, _, _, Field, {_,'*'} = Arity, _, Mod, _) -> + pack(Arity, Field, Avp, Mod, Acc); %% Or check. -acc2(Acc, Avp, I, _, AvpName, Arity, _, Mod, _) -> +acc2(Acc, Avp, I, _, Field, Arity, _, Mod, _) -> Mx = max_arity(Arity), if Mx =< I -> [Failed | Rec] = Acc, [[{5009, Avp} | Failed] | Rec]; true -> - pack(Arity, AvpName, Avp, Mod, Acc) + pack(Arity, Field, Avp, Mod, Acc) end. %% 3588/6733: -- cgit v1.2.3