fixeria has submitted this change. ( 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(-) Approvals: fixeria: Looks good to me, approved Jenkins Builder: Verified pespin: Looks good to me, but someone else must approve osmith: Looks good to me, but someone else must approve 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: -- To view, visit https://gerrit.osmocom.org/c/erlang/osmo-s1gw/+/41615?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email Gerrit-MessageType: merged Gerrit-Project: erlang/osmo-s1gw Gerrit-Branch: master Gerrit-Change-Id: Ice9c255c0cf14db0a216bb078198b9b9c76d22a7 Gerrit-Change-Number: 41615 Gerrit-PatchSet: 2 Gerrit-Owner: fixeria <vyanitskiy@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy@sysmocom.de> Gerrit-Reviewer: osmith <osmith@sysmocom.de> Gerrit-Reviewer: pespin <pespin@sysmocom.de>