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_85b9f... : PS8, Line 66: A:8, `A` is not used, so please do `_:8`.

https://gerrit.osmocom.org/c/erlang/osmo-epdg/+/40255/comment/9a3d491d_af7bb... : 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_8788b... : 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_1c863... : 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_cdc24... : 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_c39c4... : 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_7cd22... : PS8, Line 147: #{pdp_type_org => 1, : pdp_type_nr => ?GTP_PDP_ADDR_TYPE_NR_IPv4v6, : Please fix formatting here.