Attention is currently required from: laforge, pespin.

fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-sccp/+/35796?usp=email )

Change subject: Implement M3UA-over-TCP (in addition to SCTP) ......................................................................

Patch Set 1:

(6 comments)

Patchset:

PS1:

I wonder whether we really want to add/maintain a "OSMO_SS7_ASP_PROT_M3UA_TCP" type ...

I am open for suggestions. I see nothing wrong with adding a new `OSMO_SS7_ASP_PROT_*` entry, because it's more linear and comfortable to use than additionally having to check for some boolean flag across the structure(s). This also let's the VTY users to select M3UA-over-TCP using the existing VTY command(s). Adding a boolean flag means adding more VTY commands (not just one) for selecting the transport, which would only apply to `OSMO_SS7_ASP_PROT_M3UA`.

File src/osmo_ss7_vty.c:

https://gerrit.osmocom.org/c/libosmo-sccp/+/35796/comment/5a4be849_33f8b54e PS1, Line 62: "MTP3 User Adaptation (SCTP)\n" \

As mentioned in earlier patches, you can remove the "SCTP" stuff. […]

IMO, it does not hurt to clarify in command documentation which option of the two with similar names is using which transport.

https://gerrit.osmocom.org/c/libosmo-sccp/+/35796/comment/abfcda5a_2803213a PS1, Line 1994: "Display all M3UA (SCTP and TCP) ASs\n"

drop "SCTP and TCP".

Likewise here, I wanted to make it clear that all M3UA entries are shown, not only those using SCTP as the transport. Because in other commands `m3ua` means M3UA-over-SCTP, and it may be confusing to those using SCTP-over-TCP.

https://gerrit.osmocom.org/c/libosmo-sccp/+/35796/comment/c8a1948c_499a1666 PS1, Line 2013: if (filter && !strcmp(filter, "m3ua")) {

sounds like this can be moved into a "bool only_m3ua" out of the loop. […]

Sounds like it, but should I really spend more time optimizing this code path?

File src/osmo_ss7_xua_srv.c:

https://gerrit.osmocom.org/c/libosmo-sccp/+/35796/comment/c9a5ee4c_9ed5b640 PS1, Line 168: if (ss7_asp_proto_to_ip_proto(asp->cfg.proto) == IPPROTO_SCTP) {

you wanna name this probably "ss7_asp_proto_to_ipproto" (see "ipproto" instead of "ip_proto")

Why would I name something that already exists? So you suggest to rename existing API? Sounds like a purely cosmetic change to me, which is not directly related to this patch.

File src/xua_rkm.c:

https://gerrit.osmocom.org/c/libosmo-sccp/+/35796/comment/cb772cc9_058a008f PS1, Line 240: M3UA-over-SCTP or M3UA-over-TCP? Can we use asp->cfg.proto maybe?

I'd keep the usual one to mean sctp, and probably add something else for tcp.

The question here is not whether we leave `OSMO_SS7_ASP_PROT_*` unchanged and add a boolean flag. It's a more fundamental question why are we hard-coding M3UA here, while there can also be XUA and IPA. I am not familiar with the code base, so I am asking for clarification. I can only guess that dynamic RKM allow is only allowed for M3UA?