Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/13479 )
Change subject: fix USSD routing to multiple MSC ...................................................................... Patch Set 5: Code-Review-1 (6 comments) current patch set has more holes than a swiss cheese, but I have a better idea... https://gerrit.osmocom.org/#/c/13479/5/src/hlr_ussd.c File src/hlr_ussd.c: https://gerrit.osmocom.org/#/c/13479/5/src/hlr_ussd.c@175 PS5, Line 175: struct osmo_timer_list subscr_timeout; let's take this ss->subscr from the top. I had a very simple idea in mind, but it wasn't thought through properly, confused you guys and has now mutated to a timeouted cache of stale data... Let's drop this again. Here is a probably much better approach: - store only the MSC name in ss->. - For all MO USSD sessions, store the originating MSC's vlr_name in ss-> on the first message coming in from the MSC/VLR. Whatever happens, always route back to that MSC name: all errors and replies. This should then prevent all additional database lookups. - For MT USSD sessions, there will be no MSC's vlr_name stored in ss-> at first. Do a db lookup for routing to the right vlr_name, store it in ss->vlr_name. Hence we will hit the db for routing at most once per ss_session, will not keep any other stale subscr state, don't need timers. https://gerrit.osmocom.org/#/c/13479/5/src/hlr_ussd.c@251 PS5, Line 251: return -EINVAL; > memleak: msg was allocated dynamically, so here you need to free it. thx https://gerrit.osmocom.org/#/c/13479/5/src/hlr_ussd.c@254 PS5, Line 254: osmo_timer_schedule(&ss->subscr_timeout, OSMO_SESSION_SUBSCRIBER_CACHE_TIMEOUT, 0); (this was not even assigning anything to ss->subscr) https://gerrit.osmocom.org/#/c/13479/5/src/hlr_ussd.c@263 PS5, Line 263: USSD > 'SS/USSD' would be more correct, as we would also deal with "structured" > Supplementary Services some […] (ack, I have no good idea about naming here) https://gerrit.osmocom.org/#/c/13479/5/src/hlr_ussd.c@286 PS5, Line 286: resp_msg = gsm0480_msgb_alloc_name(__func__); > So here we allocate a message buffer on heap... […] agree, but separate patch. https://gerrit.osmocom.org/#/c/13479/5/src/hlr_ussd.c@343 PS5, Line 343: osmo_timer_schedule(&ss->subscr_timeout, OSMO_SESSION_SUBSCRIBER_CACHE_TIMEOUT, 0); (setting ss->subscr to point at local struct subscr becomes invalid memory as soon as the function exits) -- To view, visit https://gerrit.osmocom.org/13479 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-hlr Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I18067bfadd33a6bc59a9ee336b6937313826fce3 Gerrit-Change-Number: 13479 Gerrit-PatchSet: 5 Gerrit-Owner: Neels Hofmeyr <nhofm...@sysmocom.de> Gerrit-Reviewer: Jenkins Builder (1000002) Gerrit-Reviewer: Neels Hofmeyr <nhofm...@sysmocom.de> Gerrit-Reviewer: Pau Espin Pedrol <pes...@sysmocom.de> Gerrit-Reviewer: Vadim Yanitskiy <axilira...@gmail.com> Gerrit-Reviewer: osmith <osm...@sysmocom.de> Gerrit-Comment-Date: Sat, 06 Apr 2019 15:14:56 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: Yes
