[M] Change in ...osmo-epdg[master]: gsup_server: Look up epdg_ue_fsm process each time it needs to be acc...

2024-03-08 Thread pespin 
pespin has submitted this change. ( 
https://gerrit.osmocom.org/c/erlang/osmo-epdg/+/36198?usp=email )

Change subject: gsup_server: Look up epdg_ue_fsm process each time it needs to 
be accessed
..

gsup_server: Look up epdg_ue_fsm process each time it needs to be accessed

This allows simplifying a lot gsup_server state, make it far less prone
to bugs due to state ending up in an unconsistent state.
Nowadays the state is held in the epdg_ue_fsm.
It also allows easily spawning a process per rx msg, since no
start_monitor() is required (monitor would need to be passed to parent
gen_server process then from the per message spawned process).

Change-Id: I80203a7cf0efe82eec3773ee773d25310c07a2c3
---
M src/epdg_ue_fsm.erl
M src/gsup_server.erl
2 files changed, 51 insertions(+), 105 deletions(-)

Approvals:
  laforge: Looks good to me, but someone else must approve
  pespin: Looks good to me, approved
  Jenkins Builder: Verified




diff --git a/src/epdg_ue_fsm.erl b/src/epdg_ue_fsm.erl
index fd0d7f9..fc3a7e9 100644
--- a/src/epdg_ue_fsm.erl
+++ b/src/epdg_ue_fsm.erl
@@ -39,7 +39,7 @@
 -include_lib("gtp_utils.hrl").
 -include("conv.hrl").

--export([start_monitor/1, stop/1]).
+-export([start/1, stop/1]).
 -export([init/1,callback_mode/0,terminate/3]).
 -export([get_server_name_by_imsi/1, get_pid_by_imsi/1]).
 -export([auth_request/2, lu_request/1, tunnel_request/2, purge_ms_request/1]).
@@ -68,10 +68,10 @@
 ServerName = get_server_name_by_imsi(Imsi),
 whereis(ServerName).

-start_monitor(Imsi) ->
+start(Imsi) ->
 ServerName = get_server_name_by_imsi(Imsi),
-lager:info("ue_fsm start_monitor(~p)~n", [ServerName]),
-gen_statem:start_monitor({local, ServerName}, ?MODULE, Imsi, [{debug, 
[trace]}]).
+lager:info("ue_fsm start(~p)~n", [ServerName]),
+gen_statem:start({local, ServerName}, ?MODULE, Imsi, [{debug, 
[trace]}]).

 stop(SrvRef) ->
 try
diff --git a/src/gsup_server.erl b/src/gsup_server.erl
index 4299e95..ef87b67 100644
--- a/src/gsup_server.erl
+++ b/src/gsup_server.erl
@@ -49,14 +49,7 @@
lsocket, % listening socket
lport, % local port. only interesting if we bind with port 0
socket, % current active socket. we only support a single tcp connection
-   ccm_options, % ipa ccm options
-   ues = sets:new()
-   }).
-
--record(gsups_ue, {
-   imsi   :: binary(),
-   pid:: pid(),
-   mref   :: reference()
+   ccm_options % ipa ccm options
}).

 -export([start_link/3]).
@@ -180,9 +173,9 @@
tx_gsup(Socket, Resp),
{noreply, State};

-handle_cast({purge_ms_response, {Imsi, Result}}, State0) ->
+handle_cast({purge_ms_response, {Imsi, Result}}, State) ->
lager:info("purge_ms_response for ~p: ~p~n", [Imsi, Result]),
-   Socket = State0#gsups_state.socket,
+   Socket = State#gsups_state.socket,
case Result of
ok ->
Resp = #{message_type => purge_ms_res,
@@ -196,8 +189,11 @@
}
end,
tx_gsup(Socket, Resp),
-   State1 = delete_gsups_ue_by_imsi(Imsi, State0),
-   {noreply, State1};
+   case epdg_ue_fsm:get_pid_by_imsi(Imsi) of
+   Pid when is_pid(Pid) -> epdg_ue_fsm:stop(Pid);
+   undefined -> ok
+   end,
+   {noreply, State};

 % Our GSUP CEAI implementation for "IKEv2 Information Delete Request"
 handle_cast({cancel_location_request, Imsi}, State) ->
@@ -233,11 +229,6 @@
lager:info("GSUP: Rx ~p~n", [GsupMsgRx]),
rx_gsup(Socket, GsupMsgRx, State);

-handle_info({'DOWN', MRef, process, Pid, Reason}, State0) ->
-   lager:notice("GSUP: epdg_ue_fsm ~p exited, reason: ~p~n", [Pid, 
Reason]),
-   State1 = delete_gsups_ue_by_mref(MRef, State0),
-   {noreply, State1};
-
 handle_info(Info, S) ->
error_logger:error_report(["unknown handle_info", {module, ?MODULE}, 
{info, Info}, {state, S}]),
{noreply, S}.
@@ -274,7 +265,7 @@
 %% --

 % Rx send auth info / requesting authentication tuples
-rx_gsup(Socket, GsupMsgRx = #{message_type := send_auth_info_req, imsi := 
Imsi}, State0) ->
+rx_gsup(Socket, GsupMsgRx = #{message_type := send_auth_info_req, imsi := 
Imsi}, State) ->
case maps:find(pdp_info_list, GsupMsgRx) of
{ok, [PdpInfo]} ->
#{pdp_context_id := _PDPCtxId,
@@ -287,9 +278,12 @@
PdpTypeNr = ?GTP_PDP_ADDR_TYPE_NR_IPv4,
Apn = "*"
end,
-   {UE, State1} = find_or_new_gsups_ue(Imsi, State0),
-   case epdg_ue_fsm:auth_request(UE#gsups_ue.pid, {PdpTypeNr, Apn}) of
-   ok -> State2 = State1;
+   case epdg_ue_fsm:get_pid_by_imsi(Imsi) of
+   undefined -> {ok, Pid} = epdg_ue_fsm:start(Imsi);
+   Pid -> Pid
+

[M] Change in ...osmo-epdg[master]: gsup_server: Look up epdg_ue_fsm process each time it needs to be acc...

2024-03-08 Thread pespin 
pespin has posted comments on this change. ( 
https://gerrit.osmocom.org/c/erlang/osmo-epdg/+/36198?usp=email )

Change subject: gsup_server: Look up epdg_ue_fsm process each time it needs to 
be accessed
..


Patch Set 1: Code-Review+2


--
To view, visit https://gerrit.osmocom.org/c/erlang/osmo-epdg/+/36198?usp=email
To unsubscribe, or for help writing mail filters, visit 
https://gerrit.osmocom.org/settings

Gerrit-Project: erlang/osmo-epdg
Gerrit-Branch: master
Gerrit-Change-Id: I80203a7cf0efe82eec3773ee773d25310c07a2c3
Gerrit-Change-Number: 36198
Gerrit-PatchSet: 1
Gerrit-Owner: pespin 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge 
Gerrit-Reviewer: pespin 
Gerrit-Comment-Date: Fri, 08 Mar 2024 12:15:22 +
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[M] Change in ...osmo-epdg[master]: gsup_server: Look up epdg_ue_fsm process each time it needs to be acc...

2024-03-08 Thread laforge 
Attention is currently required from: pespin.

laforge has posted comments on this change. ( 
https://gerrit.osmocom.org/c/erlang/osmo-epdg/+/36198?usp=email )

Change subject: gsup_server: Look up epdg_ue_fsm process each time it needs to 
be accessed
..


Patch Set 1: Code-Review+1


--
To view, visit https://gerrit.osmocom.org/c/erlang/osmo-epdg/+/36198?usp=email
To unsubscribe, or for help writing mail filters, visit 
https://gerrit.osmocom.org/settings

Gerrit-Project: erlang/osmo-epdg
Gerrit-Branch: master
Gerrit-Change-Id: I80203a7cf0efe82eec3773ee773d25310c07a2c3
Gerrit-Change-Number: 36198
Gerrit-PatchSet: 1
Gerrit-Owner: pespin 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge 
Gerrit-Attention: pespin 
Gerrit-Comment-Date: Fri, 08 Mar 2024 08:05:29 +
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[M] Change in ...osmo-epdg[master]: gsup_server: Look up epdg_ue_fsm process each time it needs to be acc...

2024-03-07 Thread pespin 
pespin has uploaded this change for review. ( 
https://gerrit.osmocom.org/c/erlang/osmo-epdg/+/36198?usp=email )


Change subject: gsup_server: Look up epdg_ue_fsm process each time it needs to 
be accessed
..

gsup_server: Look up epdg_ue_fsm process each time it needs to be accessed

This allows simplifying a lot gsup_server state, make it far less prone
to bugs due to state ending up in an unconsistent state.
Nowadays the state is held in the epdg_ue_fsm.
It also allows easily spawning a process per rx msg, since no
start_monitor() is required (monitor would need to be passed to parent
gen_server process then from the per message spawned process).

Change-Id: I80203a7cf0efe82eec3773ee773d25310c07a2c3
---
M src/epdg_ue_fsm.erl
M src/gsup_server.erl
2 files changed, 51 insertions(+), 105 deletions(-)



  git pull ssh://gerrit.osmocom.org:29418/erlang/osmo-epdg 
refs/changes/98/36198/1

diff --git a/src/epdg_ue_fsm.erl b/src/epdg_ue_fsm.erl
index fd0d7f9..fc3a7e9 100644
--- a/src/epdg_ue_fsm.erl
+++ b/src/epdg_ue_fsm.erl
@@ -39,7 +39,7 @@
 -include_lib("gtp_utils.hrl").
 -include("conv.hrl").

--export([start_monitor/1, stop/1]).
+-export([start/1, stop/1]).
 -export([init/1,callback_mode/0,terminate/3]).
 -export([get_server_name_by_imsi/1, get_pid_by_imsi/1]).
 -export([auth_request/2, lu_request/1, tunnel_request/2, purge_ms_request/1]).
@@ -68,10 +68,10 @@
 ServerName = get_server_name_by_imsi(Imsi),
 whereis(ServerName).

-start_monitor(Imsi) ->
+start(Imsi) ->
 ServerName = get_server_name_by_imsi(Imsi),
-lager:info("ue_fsm start_monitor(~p)~n", [ServerName]),
-gen_statem:start_monitor({local, ServerName}, ?MODULE, Imsi, [{debug, 
[trace]}]).
+lager:info("ue_fsm start(~p)~n", [ServerName]),
+gen_statem:start({local, ServerName}, ?MODULE, Imsi, [{debug, 
[trace]}]).

 stop(SrvRef) ->
 try
diff --git a/src/gsup_server.erl b/src/gsup_server.erl
index 4299e95..ef87b67 100644
--- a/src/gsup_server.erl
+++ b/src/gsup_server.erl
@@ -49,14 +49,7 @@
lsocket, % listening socket
lport, % local port. only interesting if we bind with port 0
socket, % current active socket. we only support a single tcp connection
-   ccm_options, % ipa ccm options
-   ues = sets:new()
-   }).
-
--record(gsups_ue, {
-   imsi   :: binary(),
-   pid:: pid(),
-   mref   :: reference()
+   ccm_options % ipa ccm options
}).

 -export([start_link/3]).
@@ -180,9 +173,9 @@
tx_gsup(Socket, Resp),
{noreply, State};

-handle_cast({purge_ms_response, {Imsi, Result}}, State0) ->
+handle_cast({purge_ms_response, {Imsi, Result}}, State) ->
lager:info("purge_ms_response for ~p: ~p~n", [Imsi, Result]),
-   Socket = State0#gsups_state.socket,
+   Socket = State#gsups_state.socket,
case Result of
ok ->
Resp = #{message_type => purge_ms_res,
@@ -196,8 +189,11 @@
}
end,
tx_gsup(Socket, Resp),
-   State1 = delete_gsups_ue_by_imsi(Imsi, State0),
-   {noreply, State1};
+   case epdg_ue_fsm:get_pid_by_imsi(Imsi) of
+   Pid when is_pid(Pid) -> epdg_ue_fsm:stop(Pid);
+   undefined -> ok
+   end,
+   {noreply, State};

 % Our GSUP CEAI implementation for "IKEv2 Information Delete Request"
 handle_cast({cancel_location_request, Imsi}, State) ->
@@ -233,11 +229,6 @@
lager:info("GSUP: Rx ~p~n", [GsupMsgRx]),
rx_gsup(Socket, GsupMsgRx, State);

-handle_info({'DOWN', MRef, process, Pid, Reason}, State0) ->
-   lager:notice("GSUP: epdg_ue_fsm ~p exited, reason: ~p~n", [Pid, 
Reason]),
-   State1 = delete_gsups_ue_by_mref(MRef, State0),
-   {noreply, State1};
-
 handle_info(Info, S) ->
error_logger:error_report(["unknown handle_info", {module, ?MODULE}, 
{info, Info}, {state, S}]),
{noreply, S}.
@@ -274,7 +265,7 @@
 %% --

 % Rx send auth info / requesting authentication tuples
-rx_gsup(Socket, GsupMsgRx = #{message_type := send_auth_info_req, imsi := 
Imsi}, State0) ->
+rx_gsup(Socket, GsupMsgRx = #{message_type := send_auth_info_req, imsi := 
Imsi}, State) ->
case maps:find(pdp_info_list, GsupMsgRx) of
{ok, [PdpInfo]} ->
#{pdp_context_id := _PDPCtxId,
@@ -287,9 +278,12 @@
PdpTypeNr = ?GTP_PDP_ADDR_TYPE_NR_IPv4,
Apn = "*"
end,
-   {UE, State1} = find_or_new_gsups_ue(Imsi, State0),
-   case epdg_ue_fsm:auth_request(UE#gsups_ue.pid, {PdpTypeNr, Apn}) of
-   ok -> State2 = State1;
+   case epdg_ue_fsm:get_pid_by_imsi(Imsi) of
+   undefined -> {ok, Pid} = epdg_ue_fsm:start(Imsi);
+   Pid -> Pid
+   end,
+   case