Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/11642 )
Change subject: Add SGs Interface ...................................................................... Patch Set 30: Code-Review-1 (42 comments) Good work so far; I did still find a lot of remaining problems, but we're well on our way to get this merged. (Some of the remarks are optional to fix, skip those where you are sure and prefer to leave as is) https://gerrit.osmocom.org/#/c/11642/30/include/osmocom/msc/sgs_iface.h File include/osmocom/msc/sgs_iface.h: https://gerrit.osmocom.org/#/c/11642/30/include/osmocom/msc/sgs_iface.h@42 PS30, Line 42: /* global list of MME contexts */ this is the *entry* in that list, right? https://gerrit.osmocom.org/#/c/11642/30/include/osmocom/msc/sgs_server.h File include/osmocom/msc/sgs_server.h: https://gerrit.osmocom.org/#/c/11642/30/include/osmocom/msc/sgs_server.h@4 PS30, Line 4: #define DEFAULT_VLR_NAME "*_SGS_SERVER_*" I meant to name the variables DEFAULT_SGS_SERVER_IP and _NAME, not the enclosed URL. https://gerrit.osmocom.org/#/c/11642/30/include/osmocom/msc/sgs_server.h@18 PS30, Line 18: char *local_addr; (char array?) https://gerrit.osmocom.org/#/c/11642/30/include/osmocom/msc/sgs_server.h@21 PS30, Line 21: char *vlr_name; ... here I'd prefer char arrays. (If you have reasons against it then keep it this way though.) https://gerrit.osmocom.org/#/c/11642/30/include/osmocom/msc/vlr.h File include/osmocom/msc/vlr.h: https://gerrit.osmocom.org/#/c/11642/30/include/osmocom/msc/vlr.h@349 PS30, Line 349: const char *imsi); If an IMSI is present, it is already part of the vlr_subscr_name(). Also, since recently, vlr_susbcr_name() includes all available info on a subscriber, including IMSI, MSISDN and TMSI at the same time. (I think we shouldn't continue on the weird path I started with vlr_subscr_msisdn_or_name().) https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/gsm_04_08.c File src/libmsc/gsm_04_08.c: https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/gsm_04_08.c@453 PS29, Line 453: conn->vsub->classmark.classmark1 = lu->classmark1; > I think it makes sense to pull this apart, we already have that for BSSMAP > and for SGs as well but I […] ack https://gerrit.osmocom.org/#/c/11642/30/src/libmsc/gsm_04_08.c File src/libmsc/gsm_04_08.c: https://gerrit.osmocom.org/#/c/11642/30/src/libmsc/gsm_04_08.c@473 PS30, Line 473: * See also GSM 04.08, chapter 9.2.15a */ (I guess preferably these days include the more specific 3GPP TS 24.008 or 44.008 ... nr; maybe next time.) https://gerrit.osmocom.org/#/c/11642/8/src/libmsc/gsm_09_11.c File src/libmsc/gsm_09_11.c: https://gerrit.osmocom.org/#/c/11642/8/src/libmsc/gsm_09_11.c@347 PS8, Line 347: RV_IND_SMS); > I am not sure, I have now made it look like other function calls in the file. > […] Vadim, aligning function arguments with the opening brace is a common code formatting style, also default of vim's cindent. I've used this style ever since I started in Osmocom (and also in other code bases before that). I would gladly adopt the single-tab indent if I could only figure out cindent config to do so. Do you use vim? Do you happen to know a cindent rule set to achieve single tab? I am definitely not going to manually edit all function arguments indenting and fight against cindent, then I'd rather just keep cindent's align-with-opening-brace; I am using automatic indenting extensively, doing it manually would multiply the time i need to write code. Consistency wise, I've seen both single-tab and align-with-brace indenting in the Osmocom code base, though align-with-brace seems to be more common. (Although, for function arguments, two tabs would actually make more sense, especially for function definitions where the following body also has one tab indent.) https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/gsm_subscriber.c File src/libmsc/gsm_subscriber.c: https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/gsm_subscriber.c@119 PS29, Line 119: SCCP > This is a static function, so its not part of the API and there shouldn't be > a doc-string. just because it is a static function doesn't mean you should drop common commenting practices :P We're not as strict with static functions, but if you are documenting the function's return code, then put it in a comment above the function definition. https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/gsm_subscriber.c@130 PS29, Line 130: return sgs_iface_tx_paging(vsub, serv_ind); > When the MME performs an LU on the SGs the VLR knows that the subscriber > exists and in case of pagin […] ack https://gerrit.osmocom.org/#/c/11642/30/src/libmsc/gsm_subscriber.c File src/libmsc/gsm_subscriber.c: https://gerrit.osmocom.org/#/c/11642/30/src/libmsc/gsm_subscriber.c@120 PS30, Line 120: (unrelated ws) https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c File src/libmsc/sgs_iface.c: https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@354 PS29, Line 354: * reject, depending on the outcome of the location update. */ > Thats correct. […] then all of this is fine, including the LOGL_ERROR. (I am sometimes a bit unsure whether services being rejected should qualify for LOGL_ERROR or LOGL_NOTICE, but so far I've always decided for LOGL_ERROR in the end) https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@414 PS29, Line 414: > Yes, at least SGs calls it a "Request", see 3GPP TS 29.118 8.12. […] ack; same on the E-interface, where DTAP is forwarded both ways as "Request"s, regardless whether they are a response to earlier DTAP or not. https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@772 PS29, Line 772: /* Allocate subscriber connection */ > You mean when a subscriber has just created a conn on 2G/3G, then looses > reception and the UE does a […] for starters, this code path could check that via_ran == OSMO_RAT_EUTRAN_SGS and error out otherwise. In the long run it might make more sense to discard the other conn, to not have to wait a periodic lu expiry before SGs attach becomes possible? https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@984 PS29, Line 984: break; > The checks above abort the function early. […] ah, of course, thx https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@1212 PS29, Line 1212: * the VLR subscriber usage to be balanced. */ > This is the only FSM we need to register here. […] last time i grepped it wasn't there (?) but it's definitely there now. ack. https://gerrit.osmocom.org/#/c/11642/30/src/libmsc/sgs_iface.c File src/libmsc/sgs_iface.c: https://gerrit.osmocom.org/#/c/11642/30/src/libmsc/sgs_iface.c@54 PS30, Line 54: /* See also comment in a_iface.c */ which one, "The pointer is set once when calling a_init()"? :) https://gerrit.osmocom.org/#/c/11642/30/src/libmsc/sgs_iface.c@99 PS30, Line 99: conn->vsub = vsub; This should be vlr_subscr_get(vsub), right?? (whitespace error) https://gerrit.osmocom.org/#/c/11642/30/src/libmsc/sgs_iface.c@100 PS30, Line 100: conn->vsub->cs.attached_via_ran = conn->via_ran; here (after assigning vsub and via_ran) please call ran_conn_update_id(conn) for logging context. https://gerrit.osmocom.org/#/c/11642/30/src/libmsc/sgs_iface.c@103 PS30, Line 103: * authenticated by the MME no authentication is requred */ "required" https://gerrit.osmocom.org/#/c/11642/30/src/libmsc/sgs_iface.c@117 PS30, Line 117: vlr_subscr_msisdn_or_name(vsub)); would still prefer if the LOGxxx macros include the log ctx implicitly. Actually, I have recently added such to ran_conn, so: IMO what you should do is, in ran_conn.c, in ran_conn_alloc add case OSMO_RAT_EUTRAN_SGS: conn->log_subsys = DSGS; and then use LOG_RAN_CONN(conn, LOGL_DEBUG, "Allocated\n"); (the conn's FI should anyway already log that it is allocated, but use LOG_RAN_CONN() for all SGs logging; then you will always have the full subscriber and conn context added to the logging implicitly) For places where there is no ran_conn available I'm not sure how to best solve this, but if possible also go for logging macros that include all applicable context info implicitly. (I think I've mentioned this in an earlier review; maybe you decided it is not possible?) https://gerrit.osmocom.org/#/c/11642/30/src/libmsc/sgs_iface.c@132 PS30, Line 132: vlr_subscr_msisdn_or_name(vsub)); LOG_RAN_CONN() https://gerrit.osmocom.org/#/c/11642/30/src/libmsc/sgs_iface.c@245 PS30, Line 245: vlr_subscr_msisdn_or_name(vsub), sgsap_msg_type_name(msg_type)); (rather just vlr_subscr_name(), same below) https://gerrit.osmocom.org/#/c/11642/30/src/libmsc/sgs_server.c File src/libmsc/sgs_server.c: https://gerrit.osmocom.org/#/c/11642/30/src/libmsc/sgs_server.c@187 PS30, Line 187: talloc_free(name); suggest using osmo_sock_get_name2(), needs no talloc_free() https://gerrit.osmocom.org/#/c/11642/30/src/libmsc/sgs_vty.c File src/libmsc/sgs_vty.c: PS30: I would appreciate if you could add some .vty transcript test for the SGs VTY nodes, verifying that the new commands work as expected, show the expected docs and print back the expected 'show running-config'. https://gerrit.osmocom.org/#/c/11642/30/src/libvlr/vlr.c File src/libvlr/vlr.c: https://gerrit.osmocom.org/#/c/11642/30/src/libvlr/vlr.c@125 PS30, Line 125: const char *imsi) If you want to keep this, the name more accurately would be 'vlr_subscr_imsi_or_msisdn_or_name()' ... but again rather just use vlr_subscr_name(), as we now also do in most FSM instance IDs and log contexts https://gerrit.osmocom.org/#/c/11642/3/src/libvlr/vlr_sgs.c File src/libvlr/vlr_sgs.c: https://gerrit.osmocom.org/#/c/11642/3/src/libvlr/vlr_sgs.c@1 PS3, Line 1: /* (C) 2018-2019 by sysmocom s.f.m.c. GmbH -2019? feel free to grep the other files yourself... (BTW, I am often thinking we should automate the copyright year indications somehow, by which years contain changes to the file...) https://gerrit.osmocom.org/#/c/11642/3/src/libvlr/vlr_sgs.c@91 PS3, Line 91: vsub->sgs.paging_cb = paging_cb; > I removed this one, but there are others. I am using them as orientation > while testing things. […] confirmed: I grepped the latest patch set for '\<printf' and found no hits. https://gerrit.osmocom.org/#/c/11642/30/src/libvlr/vlr_sgs.c File src/libvlr/vlr_sgs.c: https://gerrit.osmocom.org/#/c/11642/30/src/libvlr/vlr_sgs.c@215 PS30, Line 215: vlr_subscr_put(vsub); throughout this file: do the vlr_subscr_put() *below* all uses! That's the whole idea of ref counting. https://gerrit.osmocom.org/#/c/11642/30/src/libvlr/vlr_sgs.c@223 PS30, Line 223: osmo_fsm_inst_dispatch(vsub->sgs_fsm, SGS_UE_E_RX_PAGING_FAILURE, &cause); you are still using it here. Same in other functions in this file, not marking each one now. https://gerrit.osmocom.org/#/c/11642/30/src/libvlr/vlr_sgs.c@308 PS30, Line 308: vsub->sgs.paging_serv_ind = serv_ind; I'm getting the impression that you should do another vlr_subscr_get() to increase the vlr_subscr's use count while paging, and decrease when paging is done. Have you given this consideration yet? For example, when above Ts5_timeout_cb() gets invoked, you want to be sure that the vsub has not been deallocated yet, even if the subscriber has somehow detached from the SGs (like an IMSI detach or an SGs connection reset?) https://gerrit.osmocom.org/#/c/11642/30/src/libvlr/vlr_sgs_fsm.h File src/libvlr/vlr_sgs_fsm.h: https://gerrit.osmocom.org/#/c/11642/30/src/libvlr/vlr_sgs_fsm.h@35 PS30, Line 35: SGS_UE_E_RX_SGSAP_UE_UNRECHABLE, UNREACHABLE with A https://gerrit.osmocom.org/#/c/11642/5/src/libvlr/vlr_sgs_fsm.c File src/libvlr/vlr_sgs_fsm.c: https://gerrit.osmocom.org/#/c/11642/5/src/libvlr/vlr_sgs_fsm.c@227 PS5, Line 227: (here and below would prefer N-tabs indent instead of align-with-opening-brace; curly braces are definitely no candidates for align-with-brace) https://gerrit.osmocom.org/#/c/11642/5/src/libvlr/vlr_sgs_fsm.c@231 PS5, Line 231: struct vlr_subscr *vsub = fi->priv; one more indent for multi-line member assignment https://gerrit.osmocom.org/#/c/11642/5/src/libvlr/vlr_sgs_fsm.c@238 PS5, Line 238: /* See 5.4.3 and 5.5.3 */ more indent https://gerrit.osmocom.org/#/c/11642/5/src/libvlr/vlr_sgs_fsm.c@250 PS5, Line 250: default: more indent https://gerrit.osmocom.org/#/c/11642/5/src/libvlr/vlr_sgs_fsm.c@252 PS5, Line 252: break; more indent https://gerrit.osmocom.org/#/c/11642/5/src/libvlr/vlr_sgs_fsm.c@262 PS5, Line 262: /* Failed TMSI reallocation procedure, deallocate all TMSI rather indent with two tabs (and how did you end up with the weird line breaks) https://gerrit.osmocom.org/#/c/11642/5/src/libvlr/vlr_sgs_fsm.c@293 PS5, Line 293: S(SGS_UE_E_RX_SGSAP_UE_UNRECHABLE) | S(SGS_UE_E_RX_PAGING_FAILURE) | S(SGS_UE_E_RX_ALERT_FAILURE) | drop? https://gerrit.osmocom.org/#/c/11642/30/src/libvlr/vlr_sgs_fsm.c File src/libvlr/vlr_sgs_fsm.c: https://gerrit.osmocom.org/#/c/11642/30/src/libvlr/vlr_sgs_fsm.c@61 PS30, Line 61: LOGPFSML(fi, LOGL_ERROR, "(sub %s) VLR LU tmsi allocation failed\n", vlr_subscr_msisdn_or_name(vsub)); use vlr_subscr_name(), or rather have that context in the fi->id already https://gerrit.osmocom.org/#/c/11642/30/tests/msc_vlr/stubs.h File tests/msc_vlr/stubs.h: https://gerrit.osmocom.org/#/c/11642/30/tests/msc_vlr/stubs.h@5 PS30, Line 5: * Author: Neels Hofmeyr <nhofm...@sysmocom.de> that's me, though. are you copying another stubs.h that we could just re-use instead of copying? https://gerrit.osmocom.org/#/c/11642/30/tests/sms_queue/sms_queue_test.c File tests/sms_queue/sms_queue_test.c: https://gerrit.osmocom.org/#/c/11642/30/tests/sms_queue/sms_queue_test.c@240 PS30, Line 240: are these the same as stubs.h? -- To view, visit https://gerrit.osmocom.org/11642 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-msc Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I73359925fc1ca72b33a1466e6ac41307f2f0b11d Gerrit-Change-Number: 11642 Gerrit-PatchSet: 30 Gerrit-Owner: dexter <pma...@sysmocom.de> Gerrit-Reviewer: Harald Welte <lafo...@gnumonks.org> Gerrit-Reviewer: Jenkins Builder (1000002) Gerrit-Reviewer: Neels Hofmeyr <nhofm...@sysmocom.de> Gerrit-Reviewer: dexter <pma...@sysmocom.de> Gerrit-CC: Stefan Sperling <s...@stsp.name> Gerrit-CC: Vadim Yanitskiy <axilira...@gmail.com> Gerrit-Comment-Date: Thu, 17 Jan 2019 18:27:20 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: Yes