[agent] Version 4.20 introduced a change that broke trap sending from

subagents. Due to a bug in the test code, this was not discovered,
until that bug was fixed.
OTP-9745

5 files changed, 104 insertions, 143 deletions

diff --git a/lib/snmp/src/agent/snmpa_agent.erl b/lib/snmp/src/agent/snmpa_agent.erl
index 6322f0f21d..46c634969d 100644
--- a/lib/snmp/src/agent/snmpa_agent.erl
+++ b/lib/snmp/src/agent/snmpa_agent.erl
@@ -558,25 +558,6 @@ send_trap(Agent, Trap, NotifyName, CtxName, Recv, Varbinds) ->
 	       ],
     send_notification(Agent, Trap, SendOpts).
     
-%% send_trap(Agent, Trap, NotifyName, CtxName, Recv, Varbinds) ->
-%%     ?d("send_trap -> entry with"
-%%        "~n   self():        ~p"
-%%        "~n   Agent:         ~p [~p]"
-%%        "~n   Trap:          ~p"
-%%        "~n   NotifyName:    ~p"
-%%        "~n   CtxName:       ~p"
-%%        "~n   Recv:          ~p"
-%%        "~n   Varbinds:      ~p", 
-%%        [self(), Agent, wis(Agent), 
-%% 	Trap, NotifyName, CtxName, Recv, Varbinds]),
-%%     Msg = {send_trap, Trap, NotifyName, CtxName, Recv, Varbinds}, 
-%%     case (wis(Agent) =:= self()) of
-%% 	false ->
-%% 	    call(Agent, Msg);
-%% 	true ->
-%% 	    Agent ! Msg
-%%     end.
-
 send_trap(Agent, Trap, NotifyName, CtxName, Recv, Varbinds, LocalEngineID) ->
     ?d("send_trap -> entry with"
        "~n   self():        ~p"
@@ -599,27 +580,6 @@ send_trap(Agent, Trap, NotifyName, CtxName, Recv, Varbinds, LocalEngineID) ->
 	       ],
     send_notification(Agent, Trap, SendOpts).
     
-%% send_trap(Agent, Trap, NotifyName, CtxName, Recv, Varbinds, LocalEngineID) ->
-%%     ?d("send_trap -> entry with"
-%%        "~n   self():        ~p"
-%%        "~n   Agent:         ~p [~p]"
-%%        "~n   Trap:          ~p"
-%%        "~n   NotifyName:    ~p"
-%%        "~n   CtxName:       ~p"
-%%        "~n   Recv:          ~p"
-%%        "~n   Varbinds:      ~p" 
-%%        "~n   LocalEngineID: ~p", 
-%%        [self(), Agent, wis(Agent), 
-%% 	Trap, NotifyName, CtxName, Recv, Varbinds, LocalEngineID]),
-%%     Msg = 
-%% 	{send_trap, Trap, NotifyName, CtxName, Recv, Varbinds, LocalEngineID}, 
-%%     case (wis(Agent) =:= self()) of
-%% 	false ->
-%% 	    call(Agent, Msg);
-%% 	true ->
-%% 	    Agent ! Msg
-%%     end.
-
 %% </BACKWARD-COMPAT>
 
 
@@ -709,11 +669,6 @@ wis(Atom) when is_atom(Atom) ->
     whereis(Atom).
 
 
-forward_trap(Agent, TrapRecord, NotifyName, CtxName, Recv, Varbinds) ->
-    ExtraInfo = ?DEFAULT_NOTIF_EXTRA_INFO, 
-    forward_trap(Agent, TrapRecord, NotifyName, CtxName, Recv, Varbinds, 
-		 ExtraInfo).
-
 forward_trap(Agent, TrapRecord, NotifyName, CtxName, Recv, Varbinds, 
 	     ExtraInfo) ->
     Agent ! {forward_trap, TrapRecord, NotifyName, CtxName, Recv, Varbinds, 
@@ -808,11 +763,11 @@ handle_info(worker_available, S) ->
     {noreply, S#state{worker_state = ready}};
 
 handle_info({send_notif, Notification, SendOpts}, S) ->
-    ?vlog("[handle_info] send trap request:"
+    ?vlog("[handle_info] send notif request:"
 	  "~n   Notification:  ~p"
 	  "~n   SendOpts:      ~p", 
 	  [Notification, SendOpts]),
-    case (catch handle_send_trap(cast, S, Notification, SendOpts)) of
+    case (catch handle_send_trap(S, Notification, SendOpts)) of
 	{ok, NewS} ->
 	    {noreply, NewS};
 	{'EXIT', R} ->
@@ -832,7 +787,7 @@ handle_info({send_trap, Trap, NotifyName, ContextName, Recv, Varbinds}, S) ->
 	  "~n   Varbinds:      ~p", 
 	  [Trap, NotifyName, ContextName, Recv, Varbinds]),
     ExtraInfo     = ?DEFAULT_NOTIF_EXTRA_INFO, 
-    LocalEngineID = ?DEFAULT_LOCAL_ENGINE_ID, 
+    LocalEngineID = local_engine_id(S),
     case (catch handle_send_trap(S, Trap, NotifyName, ContextName,
 				 Recv, Varbinds, LocalEngineID, ExtraInfo)) of
 	{ok, NewS} ->
@@ -1014,11 +969,11 @@ handle_call(restart_set_worker, _From, #state{set_worker = Pid} = S) ->
     {reply, ok, S};
 
 handle_call({send_notif, Notification, SendOpts}, _From, S) ->
-    ?vlog("[handle_info] send trap request:"
+    ?vlog("[handle_call] send notif request:"
 	  "~n   Notification:  ~p"
 	  "~n   SendOpts:      ~p", 
 	  [Notification, SendOpts]),
-    case (catch handle_send_trap(call, S, Notification, SendOpts)) of
+    case (catch handle_send_trap(S, Notification, SendOpts)) of
 	{ok, NewS} ->
 	    {reply, ok, NewS};
 	{'EXIT', Reason} ->
@@ -1039,18 +994,8 @@ handle_call({send_trap, Trap, NotifyName, ContextName, Recv, Varbinds},
 	  "~n   Recv:          ~p" 
 	  "~n   Varbinds:      ~p", 
 	  [Trap, NotifyName, ContextName, Recv, Varbinds]),
-    ExtraInfo = ?DEFAULT_NOTIF_EXTRA_INFO, 
-    LocalEngineID = 
-	case S#state.type of
-	    master_agent ->
-		?DEFAULT_LOCAL_ENGINE_ID;
-	    _ -> 
-		%% subagent - 
-		%% we don't need this now, eventually the trap send 
-		%% request will reach the master-agent and then it 
-		%% will look up the proper engine id.
-		ignore
-	end,
+    ExtraInfo     = ?DEFAULT_NOTIF_EXTRA_INFO, 
+    LocalEngineID = local_engine_id(S),
     case (catch handle_send_trap(S, Trap, NotifyName, ContextName,
 				 Recv, Varbinds, LocalEngineID, ExtraInfo)) of
 	{ok, NewS} ->
@@ -1595,7 +1540,7 @@ handle_backup_res([{Who, Crap}|Results], Acc) ->
 %% because we (for some reason) support the function
 %% snmpa:current_community().
 %%-----------------------------------------------------------------
-cheat({community, SecModel, Community, _TAddress}, Address, ContextName) ->
+cheat({community, _SecModel, Community, _TAddress}, Address, ContextName) ->
     {Community, Address, ContextName};
 cheat({community, _SecModel, Community, _TDomain, _TAddress}, 
       Address, ContextName) ->
@@ -1859,7 +1804,7 @@ handle_acm_error(Vsn, Reason, Pdu, ACMData, Address, Extra) ->
 	    ok
     end.
 
-get_opt(Key, Default, SendOpts) ->
+get_send_opt(Key, Default, SendOpts) ->
     case lists:keysearch(Key, 1, SendOpts) of
 	{value, {Key, Value}} ->
 	    Value;
@@ -1867,40 +1812,19 @@ get_opt(Key, Default, SendOpts) ->
 	    Default
     end.
 
-handle_send_trap(call, #state{type = master_agent} = S, 
-		 Notification, SendOpts) ->
-    SendOpts2 = 
-	case lists:keymember(local_engine_id, 1, SendOpts) of
-	    true ->
-		SendOpts;
-	    false ->
-		[{local_engine_id, ?DEFAULT_LOCAL_ENGINE_ID}|SendOpts]
-	end,
-    handle_send_trap(S, Notification, SendOpts2);
-handle_send_trap(call, S, Notification, SendOpts) ->
-    SendOpts2 = 
-	case lists:keymember(local_engine_id, 1, SendOpts) of
-	    true ->
-		SendOpts;
-	    false ->
-		%% subagent - 
-		%% we don't need this now, eventually the trap send 
-		%% request will reach the master-agent and then it 
-		%% will look up the proper engine id.
-		[{local_engine_id, ignore}|SendOpts]
-	end,
-    handle_send_trap(S, Notification, SendOpts2);
-handle_send_trap(_, S, Notification, SendOpts) ->
-    handle_send_trap(S, Notification, SendOpts).
-
 handle_send_trap(S, Notification, SendOpts) ->
-    NotifyName    = get_opt(name,     "",                        SendOpts),
-    ContextName   = get_opt(context,  "",                        SendOpts),
-    Recv          = get_opt(receiver, no_receiver,               SendOpts),
-    Varbinds      = get_opt(varbinds, [],                        SendOpts),
-    ExtraInfo     = get_opt(extra,    ?DEFAULT_NOTIF_EXTRA_INFO, SendOpts),
+    NotifyName    = get_send_opt(name,     "",                        SendOpts),
+    ContextName   = get_send_opt(context,  "",                        SendOpts),
+    Recv          = get_send_opt(receiver, no_receiver,               SendOpts),
+    Varbinds      = get_send_opt(varbinds, [],                        SendOpts),
+    ExtraInfo     = get_send_opt(extra,    ?DEFAULT_NOTIF_EXTRA_INFO, SendOpts),
     LocalEngineID = 
-	get_opt(local_engine_id, ?DEFAULT_LOCAL_ENGINE_ID, SendOpts),
+	case lists:keysearch(local_engine_id, 1, SendOpts) of
+	    {value, {local_engine_id, Value}} ->
+		Value;
+	    false ->
+		local_engine_id(S)
+	end,
     handle_send_trap(S, Notification, NotifyName, ContextName, Recv, Varbinds, 
 		     LocalEngineID, ExtraInfo).
 
@@ -1908,11 +1832,11 @@ handle_send_trap(#state{type = Type} = S,
 		 Notification, NotifyName, ContextName, Recv, Varbinds, 
 		 LocalEngineID, ExtraInfo) ->
     ?vtrace("handle_send_trap -> entry with"
-	"~n   Agent type:    ~p"
-	"~n   TrapName:      ~p"
-	"~n   NotifyName:    ~p"
-	"~n   ContextName:   ~p"
-	"~n   LocalEngineID: ~p", 
+	    "~n   Agent type:    ~p"
+	    "~n   TrapName:      ~p"
+	    "~n   NotifyName:    ~p"
+	    "~n   ContextName:   ~p"
+	    "~n   LocalEngineID: ~p", 
 	    [Type, Notification, NotifyName, ContextName, LocalEngineID]),
     case snmpa_trap:construct_trap(Notification, Varbinds) of
 	{ok, TrapRecord, VarList} ->
@@ -4001,6 +3925,18 @@ subagents_verbosity(_,_V) ->
 
 %% ---------------------------------------------------------------------
 
+local_engine_id(#state{type = master_agent}) ->
+    ?DEFAULT_LOCAL_ENGINE_ID;
+local_engine_id(_) ->
+    %% subagent - 
+    %% we don't need this now, eventually the trap send 
+    %% request will reach the master-agent and then it 
+    %% will look up the proper engine id.
+    ignore.
+
+
+%% ---------------------------------------------------------------------
+
 handle_get_log_type(#state{net_if_mod = Mod}) 
   when Mod =/= undefined ->
     case (catch Mod:get_log_type(get(net_if))) of
diff --git a/lib/snmp/src/agent/snmpa_internal.hrl b/lib/snmp/src/agent/snmpa_internal.hrl
index a490a78f84..20a36cc118 100644
--- a/lib/snmp/src/agent/snmpa_internal.hrl
+++ b/lib/snmp/src/agent/snmpa_internal.hrl
@@ -22,7 +22,8 @@
 
 -include_lib("snmp/src/app/snmp_internal.hrl").
 
--define(DEFAULT_LOCAL_ENGINE_ID, snmp_framework_mib:get_engine_id()).
+%% The DEFAULT_LOCAL_ENGINE_ID macro can only be used by the master_agent!!
+-define(DEFAULT_LOCAL_ENGINE_ID,  snmp_framework_mib:get_engine_id()). 
 -define(DEFAULT_NOTIF_EXTRA_INFO, {snmpa_default_notification_extra_info}).
 
 -define(snmpa_info(F, A),    ?snmp_info("agent",    F, A)).
diff --git a/lib/snmp/src/agent/snmpa_set_lib.erl b/lib/snmp/src/agent/snmpa_set_lib.erl
index 191029f6db..9f355855b4 100644
--- a/lib/snmp/src/agent/snmpa_set_lib.erl
+++ b/lib/snmp/src/agent/snmpa_set_lib.erl
@@ -143,8 +143,8 @@ consistency_check(Varbinds) ->
     consistency_check(Varbinds, []).
 consistency_check([{TableOid, TableVbs} | Varbinds], Done) ->
     ?vtrace("consistency_check -> entry with"
-	"~n   TableOid: ~p"
-	"~n   TableVbs: ~p",[TableOid,TableVbs]),
+	    "~n   TableOid: ~p"
+	    "~n   TableVbs: ~p", [TableOid, TableVbs]),
     TableOpsWithShortOids = deletePrefixes(TableOid, TableVbs),
     [#ivarbind{mibentry = MibEntry}|_] = TableVbs,
     case is_set_ok_table(MibEntry, TableOpsWithShortOids) of
@@ -158,7 +158,7 @@ consistency_check([{TableOid, TableVbs} | Varbinds], Done) ->
     end;
 consistency_check([IVarbind | Varbinds], Done) ->
     ?vtrace("consistency_check -> entry with"
-	"~n   IVarbind: ~p",[IVarbind]),
+	    "~n   IVarbind: ~p", [IVarbind]),
     #ivarbind{varbind = Varbind, mibentry = MibEntry} = IVarbind,
     #varbind{value = Value, org_index = OrgIndex} = Varbind,
     case is_set_ok_variable(MibEntry, Value) of
@@ -358,32 +358,39 @@ make_value_a_correct_value(Value, ASN1Type, Mfa) ->
 %% Runtime debug support
 %%-----------------------------------------------------------------
 
-% XXX: This function match on the exakt return codes from EXIT
-% messages. As of this writing it was not decided if this is
-% the right way so don't blindly do things this way.
-%
-% We fake a real EXIT signal as the return value because the
-% result is passed to the function snmpa_agent:validate_err()
-% that expect it.
+%% XYZ: This function match on the exakt return codes from EXIT
+%% messages. As of this writing it was not decided if this is
+%% the right way so don't blindly do things this way.
+%%
+%% We fake a real EXIT signal as the return value because the
+%% result is passed to the function snmpa_agent:validate_err()
+%% that expect it.
   
 dbg_apply(M,F,A) ->
-    Result =
-	case get(verbosity) of
-	    false ->
-		(catch apply(M,F,A));
-	    _ ->
-		?vlog("~n   apply: ~w,~w,~p~n", [M,F,A]),
-		Res = (catch apply(M,F,A)),
-		?vlog("~n   returned: ~p", [Res]),
-		Res
-	end,
-    case Result of
+    case maybe_verbose_apply(M, F, A) of
+	%% <Future proofing>
+	%% As of R15 we get extra info containing, 
+	%% among other things, line numbers.
+	{'EXIT', {undef, [{M, F, A, _} | _]}} ->
+	    {'EXIT', {hook_undef, {M, F, A}}};
+	{'EXIT', {function_clause, [{M, F, A, _} | _]}} ->
+	    {'EXIT', {hook_function_clause, {M, F, A}}};
+
+	%% This is really overkill, but just to be on the safe side...
+	{'EXIT', {undef, {M, F, A, _}}} ->
+	    {'EXIT', {hook_undef, {M, F, A}}};
+	{'EXIT', {function_clause, {M, F, A, _}}} ->
+	    {'EXIT', {hook_function_clause, {M, F, A}}};
+	%% </Future proofing>
+
+
+	%% Old format format for compatibility
 	{'EXIT', {undef, [{M, F, A} | _]}} ->
 	    {'EXIT', {hook_undef, {M, F, A}}};
 	{'EXIT', {function_clause, [{M, F, A} | _]}} ->
 	    {'EXIT', {hook_function_clause, {M, F, A}}};
 
-	% XXX: Old format for compatibility
+	% XYZ: Older format for compatibility
 	{'EXIT', {undef, {M, F, A}}} ->
 	    {'EXIT', {hook_undef, {M, F, A}}};
 	{'EXIT', {function_clause, {M, F, A}}} ->
@@ -393,3 +400,15 @@ dbg_apply(M,F,A) ->
 	    Result
     end.
 
+
+maybe_verbose_apply(M, F, A) ->
+    case get(verbosity) of
+	false ->
+	    (catch apply(M,F,A));
+	_ ->
+	    ?vlog("~n   apply: ~w,~w,~p~n", [M,F,A]),
+	    Res = (catch apply(M,F,A)),
+	    ?vlog("~n   returned: ~p", [Res]),
+	    Res
+    end.
+
diff --git a/lib/snmp/src/agent/snmpa_trap.erl b/lib/snmp/src/agent/snmpa_trap.erl
index 567de020c0..5b579efc13 100644
--- a/lib/snmp/src/agent/snmpa_trap.erl
+++ b/lib/snmp/src/agent/snmpa_trap.erl
@@ -379,8 +379,13 @@ send_discovery(TargetName, Record, ContextName, Vbs, NetIf, ExtraInfo) ->
 
 get_values(VariablesWithType) ->
     {Order, Varbinds} = extract_order(VariablesWithType, 1),
+    ?vtrace("get_values -> "
+	    "~n   Order:    ~p"
+	    "~n   Varbinds: ~p", [Order, Varbinds]),
     case snmpa_agent:do_get(snmpa_acm:get_root_mib_view(), Varbinds, true) of
 	{noError, _, NewVarbinds} ->
+	    ?vtrace("get_values -> values retrieved"
+		    "~n   NewVarbinds: ~p", [NewVarbinds]),
 	    %% NewVarbinds is the result of:
 	    %% first a reverse, then a sort on the oid field and finally 
 	    %% a reverse during the get-processing so we need to re-sort 
diff --git a/lib/snmp/test/snmp_agent_test.erl b/lib/snmp/test/snmp_agent_test.erl
index 426524be4d..c95346b5a6 100644
--- a/lib/snmp/test/snmp_agent_test.erl
+++ b/lib/snmp/test/snmp_agent_test.erl
@@ -4723,46 +4723,46 @@ snmp_view_based_acm_mib() ->
 
 do_set(Row) ->
     s(Row),
-    expect(1, Row).
+    expect(do_set_1, Row).
     
 add_row(RowStatus) ->
     s([{RowStatus, ?createAndGo}]),
-    expect(1, [{RowStatus, ?createAndGo}]).
+    expect(add_row_1, [{RowStatus, ?createAndGo}]).
 
 del_row(RowStatus) ->
     s([{RowStatus, ?destroy}]),
-    expect(1, [{RowStatus, ?destroy}]).
+    expect(del_row_1, [{RowStatus, ?destroy}]).
     
     
 
 use_no_rights() ->
     g([[xDescr,0]]),
-    ?v1_2_3(expect(11, noSuchName, 1, any),
-	    expect(12, [{[xDescr,0], noSuchObject}]),
-	    expect(13, authorizationError, 1, any)),
+    ?v1_2_3(expect(use_no_rights_11, noSuchName, 1, any),
+	    expect(use_no_rights_12, [{[xDescr,0], noSuchObject}]),
+	    expect(use_no_rights_13, authorizationError, 1, any)),
     g([[xDescr2,0]]),
-    ?v1_2_3(expect(21, noSuchName, 1, any),
-	    expect(22, [{[xDescr2,0], noSuchObject}]),
-	    expect(23, authorizationError, 1, any)),
+    ?v1_2_3(expect(use_no_rights_21, noSuchName, 1, any),
+	    expect(use_no_rights_22, [{[xDescr2,0], noSuchObject}]),
+	    expect(use_no_rights_23, authorizationError, 1, any)),
     gn([[xDescr]]),
-    ?v1_2_3(expect(31, noSuchName, 1, any),
-	    expect(32, [{[xDescr], endOfMibView}]),
-	    expect(33, authorizationError, 1, any)),
+    ?v1_2_3(expect(use_no_rights_31, noSuchName, 1, any),
+	    expect(use_no_rights_32, [{[xDescr], endOfMibView}]),
+	    expect(use_no_rights_33, authorizationError, 1, any)),
     s([{[xDescr,0], "tryit"}]),
-    ?v1_2_3(expect(41, noSuchName, 1, any),
-	    expect(42, noAccess, 1, any),
-	    expect(43, authorizationError, 1, any)).
+    ?v1_2_3(expect(use_no_rights_41, noSuchName, 1, any),
+	    expect(use_no_rights_42, noAccess, 1, any),
+	    expect(use_no_rights_43, authorizationError, 1, any)).
 
 
 use_rights() ->
     g([[xDescr,0]]),
-    expect(1, [{[xDescr,0], any}]),
+    expect(use_rights_1, [{[xDescr,0], any}]),
     g([[xDescr2,0]]),
-    expect(2, [{[xDescr2,0], any}]),
+    expect(use_rights_2, [{[xDescr2,0], any}]),
     s([{[xDescr,0], "tryit"}]),
-    expect(3, noError, 0, any),
+    expect(use_rights_3, noError, 0, any),
     g([[xDescr,0]]),
-    expect(4, [{[xDescr,0], "tryit"}]).
+    expect(use_rights_4, [{[xDescr,0], "tryit"}]).
 
 mk_ln(X) ->
     [length(X) | X].