Attention is currently required from: clufn, laforge, lynxis lazus. fixeria has posted comments on this change by clufn. ( https://gerrit.osmocom.org/c/erlang/osmo-epdg/+/40255?usp=email ) Change subject: Add required conversions for IPv6 and IPv4v6 support ...................................................................... Patch Set 8: Code-Review-1 (7 comments) File src/conv.erl: https://gerrit.osmocom.org/c/erlang/osmo-epdg/+/40255/comment/a076ddb5_85b9… : PS8, Line 66: A:8, `A` is not used, so please do `_:8`. https://gerrit.osmocom.org/c/erlang/osmo-epdg/+/40255/comment/9a3d491d_af7b… : PS8, Line 66: when is_binary(Rest) Can this condition ever be false? `Rest/binary` is a `binary` by definition... https://gerrit.osmocom.org/c/erlang/osmo-epdg/+/40255/comment/3171e95a_8788… : PS8, Line 69: get_6_from_bin This can be done in a less verbose way: ``` get_6_from_bin(<< IPv6:16/binary >>) -> list_to_tuple(binary_to_list(IPv6)). ``` https://gerrit.osmocom.org/c/erlang/osmo-epdg/+/40255/comment/f5a56b01_1c86… : PS8, Line 72: get_v4v6 ``` get_v4v6(<< IPv6:16/binary >>, << IPv4:4/binary >>) -> << 8, IPv6/binary, IPv4/binary >>. ``` https://gerrit.osmocom.org/c/erlang/osmo-epdg/+/40255/comment/cf28cc79_cdc2… : PS8, Line 75: when is_binary(Rest) Likewise here: * `A` is unused, so `_:136`. * `Rest/binary` is a binary by definition. Also, your implementation does not impose any length restrictions on `Rest`, so this function may return e.g. an empty binary. I propose the following: ``` get_4_from_v4v6(<< _:8, _IPv6:16/binary, IPv4:4/binary >>) -> IPv4. ``` https://gerrit.osmocom.org/c/erlang/osmo-epdg/+/40255/comment/d46934fb_c39c… : PS8, Line 77: get_6_from_v4v6 ``` get_6_from_v4v6(<< _:8, IPv6:16/binary, _IPv4:4/binary >>) -> IPv6. ``` https://gerrit.osmocom.org/c/erlang/osmo-epdg/+/40255/comment/634aa8ae_7cd2… : PS8, Line 147: #{pdp_type_org => 1, : pdp_type_nr => ?GTP_PDP_ADDR_TYPE_NR_IPv4v6, : Please fix formatting here. -- To view, visit https://gerrit.osmocom.org/c/erlang/osmo-epdg/+/40255?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: I2426e0dcee3ca92075b845f6b21a175a3d047c08 Gerrit-Change-Number: 40255 Gerrit-PatchSet: 8 Gerrit-Owner: clufn &lt;callumfarrbennett(a)yahoo.com&gt; Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria &lt;vyanitskiy(a)sysmocom.de&gt; Gerrit-Reviewer: lynxis lazus &lt;lynxis(a)fe80.eu&gt; Gerrit-Reviewer: pespin &lt;pespin(a)sysmocom.de&gt; Gerrit-CC: laforge &lt;laforge(a)osmocom.org&gt; Gerrit-Attention: laforge &lt;laforge(a)osmocom.org&gt; Gerrit-Attention: lynxis lazus &lt;lynxis(a)fe80.eu&gt; Gerrit-Attention: clufn &lt;callumfarrbennett(a)yahoo.com&gt; Gerrit-Comment-Date: Thu, 08 May 2025 18:28:25 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes