Re: [PATCH 3/9] libmsc: Update USSD to use new ss_request structure

On Fri, Apr 22, 2016 at 02:41:32PM +0200, Sergey Kostanbaev wrote:

---

openbsc/include/openbsc/gsm_04_80.h | 15 +++-- openbsc/src/libmsc/gsm_04_80.c | 128 ++++++++++++++++++++++++------------ openbsc/src/libmsc/ussd.c | 79 ++++++++++++++++++---- 3 files changed, 162 insertions(+), 60 deletions(-)

diff --git a/openbsc/include/openbsc/gsm_04_80.h b/openbsc/include/openbsc/gsm_04_80.h index 0a60652..371bc17 100644 --- a/openbsc/include/openbsc/gsm_04_80.h +++ b/openbsc/include/openbsc/gsm_04_80.h @@ -7,12 +7,17 @@

struct gsm_subscriber_connection;

-int gsm0480_send_ussd_response(struct gsm_subscriber_connection *conn,

• 	       const struct msgb *in_msg, const char* response_text, 
  

• 	       const struct ussd_request *req);
  

+int gsm0480_send_component(struct gsm_subscriber_connection *conn,

Why remove the '_ussd_' name component here while you keep it in the other functions?

IMHO you should also describe function renames in the log message.

• 	   struct msgb *msg,
  

• 	   struct ss_header* reqhdr);
  

int gsm0480_send_ussd_reject(struct gsm_subscriber_connection *conn,

• 	     const struct msgb *msg, 
  

• 	     const struct ussd_request *request);
  

• 	     uint8_t invoke_id,
  

• 	     uint8_t transaction_id);
  

+struct msgb *gsm0480_compose_ussd_component(struct ss_request* req);

...and additions/out-factorings as well.

^ osmo* is usually sparse on whitespace ;)

int gsm0480_send_ussdNotify(struct gsm_subscriber_connection *conn, int level, const char *text); int gsm0480_send_releaseComplete(struct gsm_subscriber_connection *conn); diff --git a/openbsc/src/libmsc/gsm_04_80.c b/openbsc/src/libmsc/gsm_04_80.c index f1d75f2..e6e4a92 100644 --- a/openbsc/src/libmsc/gsm_04_80.c +++ b/openbsc/src/libmsc/gsm_04_80.c @@ -39,6 +39,22 @@ #include <osmocom/core/msgb.h> #include <osmocom/gsm/tlv.h>

+/* This function can handle ASN1 length up to 255 which is enough for USSD */ +static inline unsigned char *msgb_wrap_with_ASN1_TL(struct msgb *msgb, uint8_t tag) +{

• uint16_t origlen = msgb->len;
• uint8_t *data = msgb_push(msgb, (origlen > 0x7f) ? 3 : 2);
• data[0] = tag;
• if (origlen > 0x7f) {

• data[1] = 0x81;
  

• data[2] = origlen;
  

• } else {

• data[1] = origlen;
  

• }
• return data;

+}

Would be good to use constants instead of the two magic values, which would also improve readability; otherwise I would like to see comments describing the semantics to the uninformed reader.

^ Again, a single line of whitespace would be usual in osmo*.

...

This patch is rather large, especially for a one-liner commit log; would it be possible to break this patch down into smaller changes that are easier to review? That would be great!

Unfortunately I have to redirect my attention to other tasks now and cannot look at the remaining changes...

Thanks! ~Neels

static inline unsigned char *msgb_wrap_with_TL(struct msgb *msgb, uint8_t tag) { uint8_t *data = msgb_push(msgb, 2); @@ -59,83 +75,111 @@ static inline unsigned char *msgb_push_TLV1(struct msgb *msgb, uint8_t tag, return data; }

+static inline unsigned char *msgb_wrap_with_L(struct msgb *msgb) +{

• uint8_t *data = msgb_push(msgb, 1);

-/* Send response to a mobile-originated ProcessUnstructuredSS-Request */ -int gsm0480_send_ussd_response(struct gsm_subscriber_connection *conn,

• 	       const struct msgb *in_msg, const char *response_text,
  

• 	       const struct ussd_request *req)
  

• data[0] = msgb->len - 1;
• return data;

+}

+/* Compose universial USSD packet invoke/return_result payload */ +struct msgb *gsm0480_compose_ussd_component(struct ss_request* req) { struct msgb *msg = gsm48_msgb_alloc_name("GSM 04.08 USSD RSP");

• struct gsm48_hdr *gh; uint8_t *ptr8;

• int response_len;

  /* First put the payload text into the message */ ptr8 = msgb_put(msg, 0);

• gsm_7bit_encode_n_ussd(ptr8, msgb_tailroom(msg), response_text, &response_len);

• msgb_put(msg, response_len);

• memcpy(ptr8, req->ussd_text, req->ussd_text_len);

• msgb_put(msg, req->ussd_text_len);

  /* Then wrap it as an Octet String */

• msgb_wrap_with_TL(msg, ASN1_OCTET_STRING_TAG);

• msgb_wrap_with_ASN1_TL(msg, ASN1_OCTET_STRING_TAG);

  /* Pre-pend the DCS octet string */

• msgb_push_TLV1(msg, ASN1_OCTET_STRING_TAG, 0x0F);

• msgb_push_TLV1(msg, ASN1_OCTET_STRING_TAG, req->ussd_text_language);

  /* Then wrap these as a Sequence */

• msgb_wrap_with_TL(msg, GSM_0480_SEQUENCE_TAG);
• /* Pre-pend the operation code */
• msgb_push_TLV1(msg, GSM0480_OPERATION_CODE,

• 	GSM0480_OP_CODE_PROCESS_USS_REQ);
  

• /* Wrap the operation code and IA5 string as a sequence */
• msgb_wrap_with_TL(msg, GSM_0480_SEQUENCE_TAG);

• msgb_wrap_with_ASN1_TL(msg, GSM_0480_SEQUENCE_TAG);
• if (req->component_type == GSM0480_CTYPE_RETURN_RESULT) {

• /* Pre-pend the operation code */
  

• msgb_push_TLV1(msg, GSM0480_OPERATION_CODE, req->opcode);
  

• /* Wrap the operation code and IA5 string as a sequence */
  

• msgb_wrap_with_ASN1_TL(msg, GSM_0480_SEQUENCE_TAG);
  

• /* Pre-pend the invoke ID */
  

• msgb_push_TLV1(msg, GSM0480_COMPIDTAG_INVOKE_ID, req->invoke_id);
  

• } else if (req->component_type == GSM0480_CTYPE_INVOKE) {

• /* Pre-pend the operation code */
  

• msgb_push_TLV1(msg, GSM0480_OPERATION_CODE, req->opcode);
  

• /* Pre-pend the invoke ID */
  

• msgb_push_TLV1(msg, GSM0480_COMPIDTAG_INVOKE_ID, req->invoke_id);
  

• } else {

• abort();
  

• }
• /* Wrap this up as an Invoke or a Return Result component */
• msgb_wrap_with_ASN1_TL(msg, req->component_type);
• return msg;

+}

• /* Pre-pend the invoke ID */
• msgb_push_TLV1(msg, GSM0480_COMPIDTAG_INVOKE_ID, req->invoke_id);

+#ifndef NO_GSM0480_SEND_FUNC

• /* Wrap this up as a Return Result component */
• msgb_wrap_with_TL(msg, GSM0480_CTYPE_RETURN_RESULT);

+int gsm0480_send_component(struct gsm_subscriber_connection *conn,

• 	   struct msgb *msg,
  

• 	   struct ss_header* reqhdr)
  

+{

• struct gsm48_hdr *gh;

• /* Wrap the component in a Facility message */
• msgb_wrap_with_TL(msg, GSM0480_IE_FACILITY);

• if (reqhdr->message_type == GSM0480_MTYPE_REGISTER ||

• reqhdr->message_type == GSM0480_MTYPE_RELEASE_COMPLETE) {
  

• /* Wrap the component in a Facility message, it's not ASN1 */
  

• msgb_wrap_with_TL(msg, GSM0480_IE_FACILITY);
  

• } else if (reqhdr->message_type == GSM0480_MTYPE_FACILITY) {

• /* For GSM0480_MTYPE_FACILITY it's LV not TLV */
  

• msgb_wrap_with_L(msg);
  

• } else {

• abort();
  

• }

  /* And finally pre-pend the L3 header */ gh = (struct gsm48_hdr *) msgb_push(msg, sizeof(*gh));

• gh->proto_discr = GSM48_PDISC_NC_SS | req->transaction_id

• gh->proto_discr = GSM48_PDISC_NC_SS | reqhdr->transaction_id | (1<<7); /* TI direction = 1 */

• gh->msg_type = GSM0480_MTYPE_RELEASE_COMPLETE;

• gh->msg_type = reqhdr->message_type;

• DEBUGP(DSS, "Sending SS to mobile: %s\n", msgb_hexdump(msg));

  return gsm0808_submit_dtap(conn, msg, 0, 0);

}

int gsm0480_send_ussd_reject(struct gsm_subscriber_connection *conn,

• 	     const struct msgb *in_msg,
  

• 	     const struct ussd_request *req)
  

• 	     uint8_t invoke_id,
  

• 	     uint8_t transaction_id)
  

{ struct msgb *msg = gsm48_msgb_alloc_name("GSM 04.08 USSD REJ");

• struct gsm48_hdr *gh;

• struct ss_header ssh;

  /* First insert the problem code */ msgb_push_TLV1(msg, GSM_0480_PROBLEM_CODE_TAG_GENERAL, GSM_0480_GEN_PROB_CODE_UNRECOGNISED);

  /* Before it insert the invoke ID */

• msgb_push_TLV1(msg, GSM0480_COMPIDTAG_INVOKE_ID, req->invoke_id);

• msgb_push_TLV1(msg, GSM0480_COMPIDTAG_INVOKE_ID, invoke_id);

  /* Wrap this up as a Reject component */

• msgb_wrap_with_TL(msg, GSM0480_CTYPE_REJECT);
• /* Wrap the component in a Facility message */
• msgb_wrap_with_TL(msg, GSM0480_IE_FACILITY);

• msgb_wrap_with_ASN1_TL(msg, GSM0480_CTYPE_REJECT);

• /* And finally pre-pend the L3 header */
• gh = (struct gsm48_hdr *) msgb_push(msg, sizeof(*gh));
• gh->proto_discr = GSM48_PDISC_NC_SS;
• gh->proto_discr |= req->transaction_id | (1<<7); /* TI direction = 1 */
• gh->msg_type = GSM0480_MTYPE_RELEASE_COMPLETE;
• return gsm0808_submit_dtap(conn, msg, 0, 0);

• /* Prepare data for L3 header */
• ssh.transaction_id = transaction_id;
• ssh.message_type = GSM0480_MTYPE_RELEASE_COMPLETE;
• return gsm0480_send_component(conn, msg, &ssh);

}

int gsm0480_send_ussdNotify(struct gsm_subscriber_connection *conn, int level, const char *text) @@ -173,3 +217,5 @@ int gsm0480_send_releaseComplete(struct gsm_subscriber_connection *conn)

return gsm0808_submit_dtap(conn, msg, 0, 0); }

+#endif diff --git a/openbsc/src/libmsc/ussd.c b/openbsc/src/libmsc/ussd.c index 7f01eae..3cafe02 100644 --- a/openbsc/src/libmsc/ussd.c +++ b/openbsc/src/libmsc/ussd.c @@ -33,41 +33,71 @@ #include <openbsc/gsm_subscriber.h> #include <openbsc/debug.h> #include <openbsc/osmo_msc.h> +#include <openbsc/ussd.h> +#include <osmocom/gsm/gsm_utils.h> +#include <osmocom/gsm/gsm0480.h>

/* Declarations of USSD strings to be recognised */ const char USSD_TEXT_OWN_NUMBER[] = "*#100#";

/* Forward declarations of network-specific handler functions */ -static int send_own_number(struct gsm_subscriber_connection *conn, const struct msgb *msg, const struct ussd_request *req); +static int send_own_number(struct gsm_subscriber_connection *conn,

• 	   const struct ss_header *reqhdr,
  

• 	   const struct ss_request *req);
  
  
  

/* Entrypoint - handler function common to all mobile-originated USSDs */ int handle_rcv_ussd(struct gsm_subscriber_connection *conn, struct msgb *msg) { int rc;

• struct ussd_request req;

• struct ss_header reqhdr;

• struct ss_request req;

• char request_string[MAX_LEN_USSD_STRING + 1]; struct gsm48_hdr *gh;

  memset(&req, 0, sizeof(req));

• memset(&reqhdr, 0, sizeof(reqhdr)); gh = msgb_l3(msg);

• rc = gsm0480_decode_ussd_request(gh, msgb_l3len(msg), &req);

• rc = gsm0480_decode_ss_request(gh, msgb_l3len(msg), &reqhdr); if (!rc) {

• DEBUGP(DMM, "Unhandled SS\n");
  

• rc = gsm0480_send_ussd_reject(conn, msg, &req);
  

• DEBUGP(DSS, "Incorrect SS header\n");
  

  msc_release_connection(conn); return rc; }

• /* Release-Complete */
• if (req.text[0] == '\0')

• rc = gsm0480_parse_ss_facility(gh->data + reqhdr.component_offset,

• 		       reqhdr.component_length,
  

• 		       &req);
  

• if (!rc) {

• DEBUGP(DSS, "Unhandled SS\n");
  

• /* TODO req.invoke_id may not be set!!! */
  

• rc = gsm0480_send_ussd_reject(conn, req.invoke_id, reqhdr.transaction_id);
  

• msc_release_connection(conn);
  

• return rc;
  

• }
• if (reqhdr.message_type == GSM0480_MTYPE_RELEASE_COMPLETE) return 0;

• if (!strcmp(USSD_TEXT_OWN_NUMBER, (const char *)req.text)) {

• DEBUGP(DMM, "USSD: Own number requested\n");
  

• rc = send_own_number(conn, msg, &req);
  

• if (reqhdr.message_type != GSM0480_MTYPE_REGISTER ||

• 	req.component_type != GSM0480_CTYPE_INVOKE ||
  

• 	req.opcode != GSM0480_OP_CODE_PROCESS_USS_REQ ||
  

• 	req.ussd_text_language != 0x0f)
  

• {

• DEBUGP(DSS, "Unexpected SS\n");
  

• rc = gsm0480_send_ussd_reject(conn, req.invoke_id, reqhdr.transaction_id);
  

• msc_release_connection(conn);
  

• return rc;
  

• }
• gsm_7bit_decode_n_ussd(request_string, MAX_LEN_USSD_STRING, req.ussd_text, req.ussd_text_len);
• if (!strcmp(USSD_TEXT_OWN_NUMBER, (const char *)request_string)) {

• DEBUGP(DSS, "USSD: Own number requested\n");
  

• rc = send_own_number(conn, &reqhdr, &req);
  

  } else {

• DEBUGP(DMM, "Unhandled USSD %s\n", req.text);
  

• rc = gsm0480_send_ussd_reject(conn, msg, &req);
  

• DEBUGP(DSS, "Unhandled USSD %s\n", request_string);
  

• rc = gsm0480_send_ussd_reject(conn, req.invoke_id, reqhdr.transaction_id);
  

  }

  /* check if we can release it */

@@ -76,12 +106,33 @@ int handle_rcv_ussd(struct gsm_subscriber_connection *conn, struct msgb *msg) }

/* A network-specific handler function */ -static int send_own_number(struct gsm_subscriber_connection *conn, const struct msgb *msg, const struct ussd_request *req) +static int send_own_number(struct gsm_subscriber_connection *conn,

• 	   const struct ss_header *reqhdr,
  

• 	   const struct ss_request *req)
  

{

• struct ss_request rss;

• struct ss_header rssh;

• char *own_number = conn->subscr->extension; char response_string[GSM_EXTENSION_LENGTH + 20];

• int response_len;

  /* Need trailing CR as EOT character */ snprintf(response_string, sizeof(response_string), "Your extension is %s\r", own_number);

• return gsm0480_send_ussd_response(conn, msg, response_string, req);

• memset(&rss, 0, sizeof(rss));
• gsm_7bit_encode_n_ussd(rss.ussd_text, MAX_LEN_USSD_STRING, response_string, &response_len);
• rss.ussd_text_len = response_len;
• rss.ussd_text_language = 0x0f;
• rss.component_type = GSM0480_CTYPE_RETURN_RESULT;
• rss.invoke_id = req->invoke_id;
• rss.opcode = GSM0480_OP_CODE_PROCESS_USS_REQ;
• rssh.message_type = GSM0480_MTYPE_RELEASE_COMPLETE;
• rssh.transaction_id = reqhdr->transaction_id;
• return gsm0480_send_component(conn,

• 		      gsm0480_compose_ussd_component(&rss),
  

• 		      &rssh);
  

}

1.9.1