Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/12402 )
Change subject: refactor log ctx for vlr_subscr and ran_conn ...................................................................... Patch Set 9: (10 comments) https://gerrit.osmocom.org/#/c/12402/9/src/libmsc/gsm_04_08.c File src/libmsc/gsm_04_08.c: https://gerrit.osmocom.org/#/c/12402/9/src/libmsc/gsm_04_08.c@304 PS9, Line 304: gsm48_mi_type_name(mi_type), mi_string > This could be also changed to osmo_mi_name(...). ok. I didn't want to refactor all the logging though :P Putting it in a separate patch. https://gerrit.osmocom.org/#/c/12402/9/src/libmsc/gsm_04_08.c@752 PS9, Line 752: req->cm_service_type > Looks like a mistake to me. The message says "mi_type ... […] that's right. I already fixed it later on my branch, didn't realize I actually introduced the mistake here. Thanks! https://gerrit.osmocom.org/#/c/12402/9/src/libmsc/ran_conn.c File src/libmsc/ran_conn.c: https://gerrit.osmocom.org/#/c/12402/9/src/libmsc/ran_conn.c@696 PS9, Line 696: _ran_conn_update_id > TBH, this looks unreadable. […] because of the string fmt args. I'd have to introduce yet another va_args thing like osmo_fsm_inst_update_id_f(). I mean, it's possible. But then I'd also need another char buffer of limited size, or another talloc string as well, and all that jazz. So I went for ## ARGS instead. Oh wait, looking at it now, I guess the FMT passed from all callers ends up just being '%s' now, so there is indeed a simpler way to implement it. (there were more complex intermediate patch states) Thanks! https://gerrit.osmocom.org/#/c/12402/9/src/libvlr/vlr.c File src/libvlr/vlr.c: https://gerrit.osmocom.org/#/c/12402/9/src/libvlr/vlr.c@82 PS9, Line 82: vlr_subscr_name > I couldn't resist and wrote an alternative version using the Pointer > arithmetic. […] I've done exactly that before, but I wanted to have simpler code this time. See used_ref_counts_str(), the APPEND_STR macro. Or see gsm0808_utils.c, the APPEND_THING, APPEND_STR and APPEND_CELL_ID_U macros. Maybe we can glorify such a macro to the OSMO_* namespace? But it's not so trivial to make it universally useful. If there is a limited fixed amount of elements, I prefer the fixed string buffers approach, because it is trivially obvious that it doesn't contain mistakes. As you can see below... oh wait... https://gerrit.osmocom.org/#/c/12402/9/src/libvlr/vlr.c@89 PS9, Line 89: present > You could just init buf[0] as '\0', and then check like this: […] no, buf only gets composed right at the end. https://gerrit.osmocom.org/#/c/12402/9/src/libvlr/vlr.c@93 PS9, Line 93: snprintf > What if this call would fail? then imsi would be empty, right? But how can it possibly fail. There is enough space. The string fmt is trivial. The input is a plain ASCII string, even only ['0'-'9'] chars. https://gerrit.osmocom.org/#/c/12402/9/src/libvlr/vlr.c@101 PS9, Line 101: tmsi, sizeof(buf) > Printing to tmsi, but sizeof(buf)? oh shit! https://gerrit.osmocom.org/#/c/12402/9/src/libvlr/vlr.c@105 PS9, Line 105: snprintf(buf, sizeof(buf), > Wouldn't this call overwrite the content of buffer? actually it writes to buf[], and later buf[] gets overwritten from scratch, so TMSInew is always lost. oh so *that's* why I'm not seeing TMSInew in any logging! :D thanks man https://gerrit.osmocom.org/#/c/12402/9/src/libvlr/vlr_core.h File src/libvlr/vlr_core.h: https://gerrit.osmocom.org/#/c/12402/9/src/libvlr/vlr_core.h@7 PS9, Line 7: const > This could be done in a separate change... someone else actually already did. https://gerrit.osmocom.org/#/c/12402/9/tests/msc_vlr/msc_vlr_test_authen_reuse.err File tests/msc_vlr/msc_vlr_test_authen_reuse.err: https://gerrit.osmocom.org/#/c/12402/9/tests/msc_vlr/msc_vlr_test_authen_reuse.err@11 PS9, Line 11: : > I think it makes sense to separate the RAT type from the subscriber name in > some different way, e.g. […] Limitation here is that the FSM instance ID must be a valid identifier as osmo_identifier_valid(). One reason is that the FSM instances are browsable from the CTRL interface. An earlier patch version had this: GERAN-A-0_IMSI-123456_LU First off, we get the RAN conn (GERAN-A-0), then obtain the mobile identity and note down the L3 complete type. My first change was to put the L3 type next to the conn. GERAN-A-0_LU_IMSI-123456 But semantically, a subscriber may contact us several times with (of course) the same IMSI. So I prefer to put the subscriber identification at the start. What connection ID is in use and its L3 complete type follows: IMSI-123456_MSISDN-1234_GERAN-A-0_LU IMSI-123456_MSISDN-1234_GERAN-A-1_CM_SERVICE_REQ IMSI-123456_MSISDN-1234_UTRAN-Iu-5_LU That seems to be more logical to me to follow subscriber context in a log. Next up, I didn't like the '-' and '_' mix, it is hard to figure out visually which of them is the separator. Neither space nor ',|/' are permitted characters for osmo_identifier_valid(). We already use ':' as separator elsewhere. A drawback of ':' is that it conflicts with stats exporting where ':' is not permitted, but IIUC we have solved this problem generically, already. That's how I reached this ordering of identifiers. I think it makes a lot of sense. If you have good reasons or strong opinions I'm fine with changing it, it's a bikeshed after all. -- To view, visit https://gerrit.osmocom.org/12402 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: I66a68ce2eb8957a35855a3743d91a86299900834 Gerrit-Change-Number: 12402 Gerrit-PatchSet: 9 Gerrit-Owner: Neels Hofmeyr <[email protected]> Gerrit-Reviewer: Harald Welte <[email protected]> Gerrit-Reviewer: Jenkins Builder (1000002) Gerrit-Reviewer: Max <[email protected]> Gerrit-Reviewer: Neels Hofmeyr <[email protected]> Gerrit-Reviewer: Vadim Yanitskiy <[email protected]> Gerrit-Reviewer: dexter <[email protected]> Gerrit-Comment-Date: Thu, 10 Jan 2019 23:32:04 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No
