osmo-msc[master]: libmsc/ussd: implement basic USSD session management

[Neels Hofmeyr]

Patch Set 1:

(1 comment)

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, an
(hm, seems that 'ti 08' is also in old code)


-- 
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 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-HasComments: Yes

osmo-msc[master]: libmsc/ussd: implement basic USSD session management

[Neels Hofmeyr]

Patch Set 1:

(1 comment)

https://gerrit.osmocom.org/#/c/7699/1/src/libmsc/ussd.c
File src/libmsc/ussd.c:

Line 129:   OSMO_ASSERT(session);
> if we pass a NULL in here, osmo-msc would crash on the assert, taking down 
btw, I made the same mistake recently and you hit that in OS#3125 -- remember 
the crash on OSMO_ASSERT(conn->conn_fsm) ;)


-- 
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 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-HasComments: Yes

osmo-msc[master]: libmsc/ussd: implement basic USSD session management

[Neels Hofmeyr]

Patch 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 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-HasComments: Yes

[PATCH] osmo-msc[master]: libmsc/ussd: implement basic USSD session management

[Vadim Yanitskiy]

Review at  https://gerrit.osmocom.org/7699

libmsc/ussd: implement basic USSD session management

During some long period, including the OpenBSC time, the USSD
processing code was (and still is) pretty trivial, so there
was no transaction management, nor connection reference
counting.

Meanwhile, according to GSM TS 04.10, section 2.2.6 "Multiple
supplementary service invocations", call independent SS/USSD
transactions can exist in parallel with other CM-Layer and MM
transactions. Several call independent SS/USSD transactions
can be used simultaneously.

Thus, a single USSD session is identified by:

  - MM / CM connection (pointer address),
  - Transaction ID (see TS GSM 04.08).

This change introduces a new structure, named 'ussd_session',
that corresponds to a single SS/USSD-session, and allows a
subscriber to have a few concurrent sessions in parallel.

Also, this facilitates the further implementation of network
originated SS/USSD-sessions, and the development of external
USSD interface.

Change-Id: I21c6777cb88f1f4f80f75dcd39734e952bd4e8b0
---
M include/osmocom/msc/ussd.h
M src/libmsc/transaction.c
M src/libmsc/ussd.c
M tests/msc_vlr/msc_vlr_test_authen_reuse.err
M tests/msc_vlr/msc_vlr_test_gsm_authen.err
M tests/msc_vlr/msc_vlr_test_gsm_ciph.err
M tests/msc_vlr/msc_vlr_test_no_authen.err
M tests/msc_vlr/msc_vlr_test_reject_concurrency.err
M tests/msc_vlr/msc_vlr_test_umts_authen.err
9 files changed, 486 insertions(+), 134 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-msc refs/changes/99/7699/1

diff --git a/include/osmocom/msc/ussd.h b/include/osmocom/msc/ussd.h
index 6c821b1..b4144ab 100644
--- a/include/osmocom/msc/ussd.h
+++ b/include/osmocom/msc/ussd.h
@@ -12,6 +12,40 @@
 /* Forward declarations to avoid mutual include */
 struct gsm_subscriber_connection;
 struct gsm_network;
+struct gsm_trans;
+
+struct ussd_session {
+   /**
+* GSM TS 04.10, section 2.2.6 "Multiple supplementary service
+* invocations" says: Call independent SS transactions can exist
+* in parallel with other CM-Layer and MM transactions. Several
+* call independent SS transactions can be used simultaneously.
+*
+* So, a single USSD session is identified by:
+*
+*   - MM / CM connection (pointer address),
+*   - Transaction ID (see TS GSM 04.08).
+*
+* Multiple operations may be sent on the same transaction
+* (see session), and are identified by Invoke ID.
+*/
+   struct gsm_subscriber_connection *conn;
+   struct gsm_trans *trans;
+
+   /* A link to the global sessions list */
+   struct llist_head list;
+   /* An unique session ID */
+   uint32_t sid;
+};
+
+uint32_t ussd_session_id_gen(void);
+struct ussd_session *ussd_session_alloc(struct gsm_network *net);
+void ussd_session_free(struct ussd_session *session);
+
+struct ussd_session *ussd_session_get_by_sid(uint32_t sid);
+struct ussd_session *ussd_session_get(
+   struct gsm_subscriber_connection *conn,
+   struct gsm_trans *trans);
 
 int ussd_init(struct gsm_network *net);
 void ussd_shutdown(struct gsm_network *net);
diff --git a/src/libmsc/transaction.c b/src/libmsc/transaction.c
index 5b033dc..f3da003 100644
--- a/src/libmsc/transaction.c
+++ b/src/libmsc/transaction.c
@@ -137,6 +137,9 @@
_gsm411_sms_trans_free(trans);
conn_usage_token = MSC_CONN_USE_TRANS_SMS;
break;
+   case GSM48_PDISC_NC_SS:
+   conn_usage_token = MSC_CONN_USE_TRANS_USSD;
+   break;
}
 
if (trans->paging_request) {
diff --git a/src/libmsc/ussd.c b/src/libmsc/ussd.c
index 22d1ba8..7f72a65 100644
--- a/src/libmsc/ussd.c
+++ b/src/libmsc/ussd.c
@@ -3,6 +3,7 @@
 /* (C) 2008-2009 by Harald Welte 
  * (C) 2008, 2009 by Holger Hans Peter Freyther 
  * (C) 2009 by Mike Haben 
+ * (C) 2018 by Vadim Yanitskiy 
  *
  * All Rights Reserved
  *
@@ -29,6 +30,9 @@
 #include 
 #include 
 
+#include 
+#include 
+
 #include 
 #include 
 #include 
@@ -36,9 +40,102 @@
 #include 
 #include 
 #include 
+#include 
 
-static int (*ussd_handler)(struct gsm_subscriber_connection *conn,
+static int (*ussd_handler)(struct ussd_session *session,
const struct ss_request *req) = NULL;
+
+/* Global sessions list */
+static LLIST_HEAD(ussd_active_sessions);
+
+uint32_t ussd_session_id_gen(void)
+{
+   static uint32_t sid_src = 0;
+   return sid_src++;
+}
+
+struct ussd_session *ussd_session_alloc(struct gsm_network *net)
+{
+   struct ussd_session *session;
+
+   DEBUGP(DMM, "Allocate a new USSD session\n");
+
+   /* Allocate a new session */
+   session = talloc_zero(net, struct ussd_session);
+   if (!session)
+   return NULL;
+
+   /* Generate unique session ID */
+   session->sid = ussd_session_id_gen();
+