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? -- To view, visit https://gerrit.osmocom.org/c/libosmo-sccp/+/35796?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmo-sccp Gerrit-Branch: master Gerrit-Change-Id: I8c76d271472befacbeb998a93bbdc9e8660d9b5d Gerrit-Change-Number: 35796 Gerrit-PatchSet: 1 Gerrit-Owner: fixeria <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <[email protected]> Gerrit-Reviewer: pespin <[email protected]> Gerrit-Attention: laforge <[email protected]> Gerrit-Attention: pespin <[email protected]> Gerrit-Comment-Date: Sat, 03 Feb 2024 10:28:44 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin <[email protected]> Comment-In-Reply-To: fixeria <[email protected]> Gerrit-MessageType: comment
