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 <[email protected]>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <[email protected]>
Gerrit-Reviewer: osmith <[email protected]>
Gerrit-Reviewer: pespin <[email protected]>
