Patch Set 1: (8 comments)
This review touches on various topics that are not strictly related to this, especially determining the cn_domain. What I'd like to improve in this patch the most: would it not be easy to factor out the common parts instead of code dup and copy FIXME code around? https://gerrit.osmocom.org/#/c/7743/1//COMMIT_MSG Commit Message: Line 18: advertise itself as an SGSN in the IPA unit name). In the osmo_gsup_message, we have a cn_domain indicator. I suppose our GSUP connection dance doesn't send this to the GSUP server, does it? If we did that, we would save the need to interpret an IPA peer name as PS or not. Also thinking, so far, a GSUP client is always either PS or not, so as soon as we receive the first lu_op request, we can remember the cn_domain globally for that peer. That would also save us from string parsing. (and fail loudly as soon as a peer changes its cn_domain; or allow a peer to collect both ps and cs flags, while practically MSC would just remain cs and SGSN would just remain ps) https://gerrit.osmocom.org/#/c/7743/1/src/hlr.c File src/hlr.c: Line 76: unit_len = osmo_gsup_conn_ccm_get(co, &unit, IPAC_IDTAG_UNITNAME); heh, I wasn't actually aware that we keep the GSUP conn peer info in a tlv ... looks like we might want to decode the key tlvs once upon connecting, instead of every time we revisit it? Line 89: "IMSI='%s': Cannot notify GSUP client, cannot get peer address " slightly confusing nomenclature ... IDTAG_SERNR is an IPA peer name, we call it addr in osmo_gsup_server_ccm_cb(), but that might not be a good choice. The log message could say IPA peer name instead. Line 97: gsup.message_type = OSMO_GSUP_MSGT_INSERT_DATA_REQUEST; (syntactically nicer IMHO, above: struct osmo_gsup_msg gsup = { .message_type = OSMO...., }; Initializes all remaining members to zero.) Line 109: /* XXX Cast to 'char *' avoids a "pointer targets differ in signedness" warning from GCC 7.2.0. */ (does this need an "XXX"? it's ok to cast like this, right?) Line 110: is_ps = (strncmp((char *)unit, "SGSN", 4) == 0); if we do this slightly hacky determination whether the peer is ps or not, I'd really want this to be in one central place upon the peer connecting, and then keep that flag in struct osmo_gsup_conn. (also possible: have one separate central function that does the determination, but again it doesn't make sense to re-determine every time we visit this peer) (hmm, osmo_gsup_conn is defined in osmo-hlr; for some time now we want the osmo_ prefix reserved for libosmocore definitions. just noting. could rename if we had the time) Line 115: instead of wildcard APN */ so this is copied from lu_op_tx_insert_subscr_data(). Now we have the same FIXME twice. I don't see what's so hard about separating common code into a separate function to serve both use cases and avoid code dup? Line 127: /* Send ISD to VLR/SGSN */ (in an aside, the MSC and SGSN are both supposed to have a VLR, this comment wants to say "to MSC/SGSN") -- To view, visit https://gerrit.osmocom.org/7743 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I06c43ece2b48dc63d599000eb6d6d51e08963067 Gerrit-PatchSet: 1 Gerrit-Project: osmo-hlr Gerrit-Branch: master Gerrit-Owner: Stefan Sperling <ssperl...@sysmocom.de> Gerrit-Reviewer: Harald Welte <lafo...@gnumonks.org> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr <nhofm...@sysmocom.de> Gerrit-Reviewer: neels <nhofm...@sysmocom.de> Gerrit-HasComments: Yes
