This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
Neels Hofmeyr gerrit-no-reply at lists.osmocom.orgPatch Set 1: Code-Review-1 (15 comments) https://gerrit.osmocom.org/#/c/7699/1/include/osmocom/msc/ussd.h File include/osmocom/msc/ussd.h: Line 33: struct gsm_trans *trans; a gsm_trans always belongs to a conn. Do you really need a separate conn pointer? Can there be a valid ussd_session without a trans? Line 35: /* A link to the global sessions list */ more like an entry within the global sessions list Line 41: uint32_t ussd_session_id_gen(void); (this could remain static in ussd.c) Line 48: struct gsm_trans *trans); again, you shouldn't need both conn and trans https://gerrit.osmocom.org/#/c/7699/1/src/libmsc/ussd.c File src/libmsc/ussd.c: Line 61: DEBUGP(DMM, "Allocate a new USSD session\n"); When reading this in the log, I'd immediately want to know: for which subscriber?? Even besides mere logging, it makes sense to always firmly associate a ussd session with a given subscriber, or even with a conn. You will always have at least a vlr_subscr first and then allocate a USSD session for that subscr, right? Even better would be to even have a gsm_subscriber_connection before allocating ussd. Then talloc that ussd session as child of that conn, not child of the network. How are the semantics of USSD sessions? Line 88: DEBUGP(DMM, "Found a USSD session (sid=%u)\n", session->sid); log context (vlr_subscr_name()) Line 103: OSMO_ASSERT(trans); I think you could just rely on the calling code to not pass NULL. Line 117: session->trans->transaction_id, session->sid); we commonly log the context first, then the message; and when logging hex, prepend a 0x to make it clear, like: (subscr %s, trans_id=0x%x, sid=%u) Found USSD session Line 126: DEBUGP(DMM, "Deallocate an USSD session\n"); log context Line 129: OSMO_ASSERT(session); if we pass a NULL in here, osmo-msc would crash on the assert, taking down all active connections. Decide: * it is valid to pass NULL into ussd_session_free(). Then if (!session) return; i.e. don't crash, just ignore. OR * it is invalid to pass NULL in here. Then simply never do that and also don't assert nor check it here. Line 183: ussd_session_free(session); wait, a ussd_session is discarded basically right upon receiving the USSD request, in all cases? Does it never live past the first rx of SS Request? If we, for example, were to pass the request on to the HLR, we'd have to asynchronously wait for the HLR's response to reply, and only then discard. I'm getting the impression ussd_session should not be a separate struct, but rather be a third part of the cc,sms union in struct gsm_trans. Then make use of the common transaction alloc and teardown API. For example, if the BSC decides to suddenly send a BSSMAP Clear Request, the subscr_conn.c wants to free all transactions. Will it know that a ussd_session is associated with a trans? If the ussd session *is* a gsm_trans, then it certainly will. Also, IMHO, if a session was allocated outside of the handler, the deallocation should also happen outside. Line 218: DEBUGP(DMM, "Received SS/USSD data (trans_id=%x)\n", tid); log context, and the same multiple times below Line 221: OSMO_ASSERT(conn->vsub); you can assume that conn->vsub is always populated when you're as far as handling DTAP https://gerrit.osmocom.org/#/c/7699/1/tests/msc_vlr/msc_vlr_test_authen_reuse.err File tests/msc_vlr/msc_vlr_test_authen_reuse.err: Line 263: DCC (ti 08 sub MSISDN:42342 callref 0) New transaction read above three log lines. the first two have incomplete or no context, and are basically obsolete once the third is logged. One time you log 'trans_id=8', then you log 'ti 08'. What is 08, octal representation? Line 268: DMM USSD: Own number requested This above (probably very old) log line might have gotten you the idea that we don't need context ;) -- To view, visit https://gerrit.osmocom.org/7699 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I21c6777cb88f1f4f80f75dcd39734e952bd4e8b0 Gerrit-PatchSet: 1 Gerrit-Project: osmo-msc Gerrit-Branch: master Gerrit-Owner: Vadim Yanitskiy <axilirator at gmail.com> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de> Gerrit-HasComments: Yes