Attention is currently required from: laforge, pespin.
6 comments:
Patchset:
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:
Patch Set #1, 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.
Patch Set #1, 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.
Patch Set #1, 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:
Patch Set #1, 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:
Patch Set #1, 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 change 35796. To unsubscribe, or for help writing mail filters, visit settings.