Attention is currently required from: laforge, lynxis lazus, pespin.
fixeria has posted comments on this change by lynxis lazus. ( https://gerrit.osmocom.org/c/erlang/osmo-epdg/+/37901?usp=email )
Change subject: s2b: when socket opening fails, stop the S2b module
......................................................................
Patch Set 3:
(1 comment)
File src/epdg_gtpc_s2b.erl:
https://gerrit.osmocom.org/c/erlang/osmo-epdg/+/37901/comment/97b39dfd_e10b… :
PS3, Line 150: io_lib:format
> Yes maybe true. I'm not sure what's the intended way of returning a reason is. […]
Well, the general approach is that formatted strings are for humans. You already let the humans know about an error via logging, so here you're letting the process' owner the failure `Reason`. The term `udp_socket_open_failed` is self-explanatory, so having a tuple `{udp_socket_open_failed, Reason}` would allow potential consumer of the `Reason` to format it any way they want (if at all).
--
To view, visit https://gerrit.osmocom.org/c/erlang/osmo-epdg/+/37901?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: erlang/osmo-epdg
Gerrit-Branch: master
Gerrit-Change-Id: I49a6a1ec0c938aa70444c19c0b7b644f2e42f1fd
Gerrit-Change-Number: 37901
Gerrit-PatchSet: 3
Gerrit-Owner: lynxis lazus <lynxis(a)fe80.eu>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: lynxis lazus <lynxis(a)fe80.eu>
Gerrit-Comment-Date: Thu, 04 Dec 2025 21:39:09 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de>
Comment-In-Reply-To: lynxis lazus <lynxis(a)fe80.eu>
Attention is currently required from: lynxis lazus.
fixeria has posted comments on this change by lynxis lazus. ( https://gerrit.osmocom.org/c/erlang/osmo-epdg/+/41588?usp=email )
Change subject: s2b: allow to set the restart counter/recovery field
......................................................................
Patch Set 2: Code-Review-1
--
To view, visit https://gerrit.osmocom.org/c/erlang/osmo-epdg/+/41588?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: erlang/osmo-epdg
Gerrit-Branch: master
Gerrit-Change-Id: I1e894a2c54dd180c0bc3a6f78d0bb7dd455cad81
Gerrit-Change-Number: 41588
Gerrit-PatchSet: 2
Gerrit-Owner: lynxis lazus <lynxis(a)fe80.eu>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Attention: lynxis lazus <lynxis(a)fe80.eu>
Gerrit-Comment-Date: Thu, 04 Dec 2025 21:33:11 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Attention is currently required from: lynxis lazus.
fixeria has posted comments on this change by lynxis lazus. ( https://gerrit.osmocom.org/c/erlang/osmo-epdg/+/41588?usp=email )
Change subject: s2b: allow to set the restart counter/recovery field
......................................................................
Patch Set 2:
(3 comments)
Commit Message:
https://gerrit.osmocom.org/c/erlang/osmo-epdg/+/41588/comment/a9841de3_fa82… :
PS2, Line 9: counter a 8 bit value
**is** a 8 bit value
File src/epdg_gtpc_s2b.erl:
https://gerrit.osmocom.org/c/erlang/osmo-epdg/+/41588/comment/a5b06913_a4e7… :
PS2, Line 112: monotonic timer could be to reproducible on systems
What do you mean here? This comment is confusing...
https://gerrit.osmocom.org/c/erlang/osmo-epdg/+/41588/comment/9570d46b_65eb… :
PS2, Line 115: UserConfig
This argument has two different names: `Options` in `start_link()` and `UserConfig` here. Let's stick to either of the two, for the sake of consistency.
Also, it's worth grouping/reworking all these list items into a map to avoid ugly constructions like this one. Might be an idea for a separate change.
--
To view, visit https://gerrit.osmocom.org/c/erlang/osmo-epdg/+/41588?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: erlang/osmo-epdg
Gerrit-Branch: master
Gerrit-Change-Id: I1e894a2c54dd180c0bc3a6f78d0bb7dd455cad81
Gerrit-Change-Number: 41588
Gerrit-PatchSet: 2
Gerrit-Owner: lynxis lazus <lynxis(a)fe80.eu>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-CC: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Attention: lynxis lazus <lynxis(a)fe80.eu>
Gerrit-Comment-Date: Thu, 04 Dec 2025 21:33:05 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Attention is currently required from: fixeria, laforge, pespin.
lynxis lazus has posted comments on this change by lynxis lazus. ( https://gerrit.osmocom.org/c/erlang/osmo-epdg/+/37901?usp=email )
Change subject: s2b: when socket opening fails, stop the S2b module
......................................................................
Patch Set 3:
(1 comment)
File src/epdg_gtpc_s2b.erl:
https://gerrit.osmocom.org/c/erlang/osmo-epdg/+/37901/comment/487936d0_16ce… :
PS3, Line 150: io_lib:format
> Not sure why would you want to include a formatted string here, given that above you're already logg […]
Yes maybe true. I'm not sure what's the intended way of returning a reason is.
Or what are good examples.
IMHO: I thought more into the direction of a python exception, which both contains a type and a human understandable reason.
--
To view, visit https://gerrit.osmocom.org/c/erlang/osmo-epdg/+/37901?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: erlang/osmo-epdg
Gerrit-Branch: master
Gerrit-Change-Id: I49a6a1ec0c938aa70444c19c0b7b644f2e42f1fd
Gerrit-Change-Number: 37901
Gerrit-PatchSet: 3
Gerrit-Owner: lynxis lazus <lynxis(a)fe80.eu>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 04 Dec 2025 21:23:45 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de>
Attention is currently required from: laforge, lynxis lazus, pespin.
fixeria has posted comments on this change by lynxis lazus. ( https://gerrit.osmocom.org/c/erlang/osmo-epdg/+/37901?usp=email )
Change subject: s2b: when socket opening fails, stop the S2b module
......................................................................
Patch Set 3: Code-Review+1
(1 comment)
File src/epdg_gtpc_s2b.erl:
https://gerrit.osmocom.org/c/erlang/osmo-epdg/+/37901/comment/85208e2d_632e… :
PS3, Line 150: io_lib:format
Not sure why would you want to include a formatted string here, given that above you're already logging an error. Returning `{udp_socket_open_failed, Reason}` looks more practical to me, but even this feels redundant... The question is, who is supposed to consume this pretty-printed string value? Not blocking, just raising a concern.
--
To view, visit https://gerrit.osmocom.org/c/erlang/osmo-epdg/+/37901?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: erlang/osmo-epdg
Gerrit-Branch: master
Gerrit-Change-Id: I49a6a1ec0c938aa70444c19c0b7b644f2e42f1fd
Gerrit-Change-Number: 37901
Gerrit-PatchSet: 3
Gerrit-Owner: lynxis lazus <lynxis(a)fe80.eu>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: lynxis lazus <lynxis(a)fe80.eu>
Gerrit-Comment-Date: Thu, 04 Dec 2025 21:20:01 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Attention is currently required from: fixeria, laforge, pespin.
Hello Jenkins Builder, fixeria, laforge, pespin,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/erlang/osmo-epdg/+/37901?usp=email
to look at the new patch set (#2).
The following approvals got outdated and were removed:
Code-Review+1 by pespin, Code-Review+2 by laforge, Code-Review-1 by fixeria, Verified+1 by Jenkins Builder
The change is no longer submittable: Code-Review and Verified are unsatisfied now.
Change subject: s2b: when socket opening fails, stop the S2b module
......................................................................
s2b: when socket opening fails, stop the S2b module
Change-Id: I49a6a1ec0c938aa70444c19c0b7b644f2e42f1fd
---
M src/epdg_gtpc_s2b.erl
1 file changed, 2 insertions(+), 1 deletion(-)
git pull ssh://gerrit.osmocom.org:29418/erlang/osmo-epdg refs/changes/01/37901/2
--
To view, visit https://gerrit.osmocom.org/c/erlang/osmo-epdg/+/37901?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: erlang/osmo-epdg
Gerrit-Branch: master
Gerrit-Change-Id: I49a6a1ec0c938aa70444c19c0b7b644f2e42f1fd
Gerrit-Change-Number: 37901
Gerrit-PatchSet: 2
Gerrit-Owner: lynxis lazus <lynxis(a)fe80.eu>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>