fixeria has uploaded this change for review. ( https://gerrit.osmocom.org/c/erlang/osmo-s1gw/+/41615?usp=email )
Change subject: sctp_proxy: use a record for storing state data ......................................................................
sctp_proxy: use a record for storing state data
Maps are convenient, but they are also more permissive: field names are unchecked at compile time, making it easier for typos or unexpected fields to slip through. Records, on the other hand, are stricter and more declarative. Their structure is explicitly defined, the set of fields is fixed, and the compiler can validate field names.
Change-Id: Ice9c255c0cf14db0a216bb078198b9b9c76d22a7 --- M src/sctp_proxy.erl 1 file changed, 61 insertions(+), 45 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/erlang/osmo-s1gw refs/changes/15/41615/1
diff --git a/src/sctp_proxy.erl b/src/sctp_proxy.erl index 458c53a..5e0d26e 100644 --- a/src/sctp_proxy.erl +++ b/src/sctp_proxy.erl @@ -62,6 +62,18 @@ mme_aid => gen_sctp:assoc_id() }.
+-record(state, {enb_aid :: gen_sctp:assoc_id(), + mme_aid :: undefined | gen_sctp:assoc_id(), + enb_conn_info :: sctp_server:conn_info(), + mme_conn_cfg :: sctp_client:cfg(), + tx_queue :: [binary()], + sock :: undefined | gen_sctp:sctp_socket(), + enb_handle :: enb_registry:enb_handle(), + handler :: pid() %% s1ap_proxy process' pid + }). + +-type state() :: #state{}. + -export_type([conn_info/0]).
@@ -100,12 +112,12 @@ {ok, EnbHandle} = enb_registry:enb_register(), {ok, Pid} = s1ap_proxy:start_link(EnbHandle, EnbConnInfo), {ok, connecting, - #{enb_aid => maps:get(aid, EnbConnInfo), - enb_conn_info => EnbConnInfo, - mme_conn_cfg => MmeConnCfg, - tx_queue => [], - enb_handle => EnbHandle, - handler => Pid}}. + #state{enb_aid = maps:get(aid, EnbConnInfo), + enb_conn_info = EnbConnInfo, + mme_conn_cfg = MmeConnCfg, + tx_queue = [], + enb_handle = EnbHandle, + handler = Pid}}.
callback_mode() -> @@ -114,14 +126,14 @@
%% CONNECTING state connecting(enter, OldState, - #{enb_conn_info := EnbConnInfo, - mme_conn_cfg := MmeConnCfg, - enb_handle := Handle} = S) -> + #state{enb_conn_info = EnbConnInfo, + mme_conn_cfg = MmeConnCfg, + enb_handle = Handle} = S) -> ?LOG_INFO("State change: ~p -> ~p", [OldState, ?FUNCTION_NAME]), ok = enb_registry:enb_event(Handle, {?FUNCTION_NAME, EnbConnInfo}), %% Initiate connection establishment with the MME {ok, Sock} = sctp_client:connect(MmeConnCfg), - {next_state, connecting, S#{sock => Sock}, + {next_state, connecting, S#state{sock = Sock}, [{state_timeout, 2_000, conn_est_timeout}]};
%% Handle connection establishment timeout @@ -130,10 +142,10 @@
%% Handle an eNB -> MME data forwarding request (queue) connecting(cast, {send_data, Data}, - #{tx_queue := Pending} = S) -> + #state{tx_queue = Pending} = S) -> s1gw_metrics:ctr_inc(?S1GW_CTR_S1AP_PROXY_UPLINK_PACKETS_QUEUED), s1gw_metrics:gauge_inc(?S1GW_GAUGE_S1AP_PROXY_UPLINK_PACKETS_QUEUED), - {keep_state, S#{tx_queue => [Data | Pending]}}; + {keep_state, S#state{tx_queue = [Data | Pending]}};
%% Handle an #sctp_assoc_change event (connection state) connecting(info, {sctp, _Socket, MmeAddr, MmePort, @@ -143,7 +155,7 @@ comm_up -> ?LOG_NOTICE("MME connection (id=~p, ~p:~p) established", [Aid, MmeAddr, MmePort]), - {next_state, connected, S#{mme_aid => Aid}}; + {next_state, connected, S#state{mme_aid = Aid}}; _ -> ?LOG_NOTICE("MME connection establishment failed: ~p", [ConnState]), {stop, {shutdown, conn_est_fail}} @@ -155,7 +167,7 @@
%% CONNECTED state connected(enter, OldState, - #{enb_handle := Handle} = S0) -> + #state{enb_handle = Handle} = S0) -> ?LOG_INFO("State change: ~p -> ~p", [OldState, ?FUNCTION_NAME]), MmeConnInfo = conn_info(?FUNCTION_NAME, S0), ok = enb_registry:enb_event(Handle, {?FUNCTION_NAME, MmeConnInfo}), @@ -188,7 +200,7 @@ stream = SID, ssn = SSN, tsn = TSN}], Data}}, - #{mme_aid := MmeAid} = S) -> + #state{mme_aid = MmeAid} = S) -> ?LOG_DEBUG("MME connection (id=~p, ~p:~p) -> eNB: ~p", [MmeAid, MmeAddr, MmePort, #{tsn => TSN, sid => SID, ssn => SSN, @@ -231,19 +243,22 @@
terminate(Reason, State, - #{handler := Pid, - enb_handle := Handle} = S) -> + #state{handler = Pid, + enb_handle = Handle, + sock = Sock, + mme_aid = MmeAid}) -> ?LOG_NOTICE("Terminating in state ~p, reason ~p", [State, Reason]), enb_registry:enb_unregister(Handle), s1ap_proxy:shutdown(Pid), - case S of - #{sock := Sock, - mme_aid := Aid} -> - sctp_common:shutdown({Sock, Aid}), - gen_sctp:close(Sock); - #{sock := Sock} -> - gen_sctp:close(Sock); - _ -> ok + case Sock of + undefined -> ok; + _ -> + case MmeAid of + undefined -> ok; + _ -> + sctp_common:shutdown({Sock, MmeAid}) + end, + gen_sctp:close(Sock) end.
@@ -251,17 +266,13 @@ %% private API %% ------------------------------------------------------------------
-%% XXX: proper type --type state() :: map(). - - %% Send a single message: eNB -> MME -spec sctp_send_from_enb(binary(), state()) -> ok | {error, term()}. sctp_send_from_enb(Data, - #{sock := Sock, - enb_aid := EnbAid, - mme_aid := MmeAid, - handler := Pid}) -> + #state{sock = Sock, + enb_aid = EnbAid, + mme_aid = MmeAid, + handler = Pid}) -> case s1ap_proxy:process_pdu(Pid, Data) of {forward, FwdData} -> sctp_common:send_data({Sock, MmeAid}, FwdData); @@ -275,10 +286,10 @@ %% Send a single message: eNB <- MME -spec sctp_send_from_mme(binary(), state()) -> ok | {error, term()}. sctp_send_from_mme(Data, - #{sock := Sock, - enb_aid := EnbAid, - mme_aid := MmeAid, - handler := Pid}) -> + #state{sock = Sock, + enb_aid = EnbAid, + mme_aid = MmeAid, + handler = Pid}) -> case s1ap_proxy:process_pdu(Pid, Data) of {forward, FwdData} -> sctp_server:send_data(EnbAid, FwdData); @@ -290,23 +301,28 @@
%% Send pending messages to the MME -sctp_send_pending(#{tx_queue := Pending} = S) -> +-spec sctp_send_pending(state()) -> state(). +sctp_send_pending(#state{tx_queue = Pending} = S) -> sctp_send_pending(lists:reverse(Pending), S).
+-spec sctp_send_pending([binary()], state()) -> state(). sctp_send_pending([Data | Pending], S) -> sctp_send_from_enb(Data, S), s1gw_metrics:gauge_dec(?S1GW_GAUGE_S1AP_PROXY_UPLINK_PACKETS_QUEUED), sctp_send_pending(Pending, S);
sctp_send_pending([], S) -> - S#{tx_queue := []}. + S#state{tx_queue = []}.
-conn_info(State, #{mme_conn_cfg := MmeConnCfg} = S0) -> - S1 = maps:with([enb_aid, mme_aid, handler], S0), - S1#{state => State, - mme_addr => maps:get(raddr, MmeConnCfg), - mme_port => maps:get(rport, MmeConnCfg)}. - +-spec conn_info(atom(), state()) -> conn_info(). +conn_info(State, #state{mme_conn_cfg = MmeConnCfg} = S) -> + Info = #{state => State, + handler => S#state.handler, + mme_addr => maps:get(raddr, MmeConnCfg), + mme_port => maps:get(rport, MmeConnCfg), + enb_aid => S#state.enb_aid, + mme_aid => S#state.mme_aid}, + maps:filter(fun(_K, V) -> V =/= undefined end, Info).
%% vim:set ts=4 sw=4 et: