Change in libosmocore[master]: GSUP/SMS: introduce MO-/MT-FORWARD-SM messages
Harald Welte has submitted this change and it was merged. (
https://gerrit.osmocom.org/11069 )
Change subject: GSUP/SMS: introduce MO-/MT-FORWARD-SM messages
..
GSUP/SMS: introduce MO-/MT-FORWARD-SM messages
According to 3GPP TS 29.002, there are two services:
- MAP-MO-FORWARD-SHORT-MESSAGE (see 12.2),
- MAP-MT-FORWARD-SHORT-MESSAGE (see 12.9),
which are used to forward MO/MT short messages.
This change replicates both services as GSUP messages:
- OSMO_GSUP_MSGT_MO_FORWARD_SM_*,
- OSMO_GSUP_MSGT_MT_FORWARD_SM_*.
Please note, that only the 'must-have' IEs are introduced
by this change, in particular the following:
- OSMO_GSUP_SM_RP_MR_IE (see note below),
- OSMO_GSUP_SM_RP_DA_IE (see 7.6.8.1),
- OSMO_GSUP_SM_RP_OA_IE (see 7.6.8.2),
- OSMO_GSUP_SM_RP_UI_IE (see 7.6.8.4),
- OSMO_GSUP_SM_RP_MMS_IE (see 7.6.8.7),
- OSMO_GSUP_SM_RP_CAUSE_IE (see GSM TS 04.11, 8.2.5.4),
where both SM_RP_DA and SM_RP_OA IEs basically contain
a single nested TV of the following format:
- T: identity type (see 'osmo_gsup_sms_sm_rp_oda_t'),
- V: encoded identity itself (optional).
According to GSM TS 04.11, every single message on the SM-RL has
an unique message reference (see 8.2.3), that is used to link
an RP-ACK or RP-ERROR message to the associated (preceding)
RP-DATA or RP-SMMA message transfer attempt.
In case of TCAP/MAP, this message reference is being mapped to the
Invoke ID. But since GSUP has no 'Invoke ID' IE, and it is not
required for other applications (other than SMS), this change
introduces a special 'SM_RP_MR' IE that doesn't exist in MAP.
Change-Id: Ibe325c64ae2d6c626b232533bb4cbc65fc2b5d71
Related Change-Id: (docs) Ie0150756c33c1352bc4eb49421824542c711175c
Related Change-Id: (TTCN) Ibf49474a81235096c032ea21f217170f523bd94e
Related: OS#3587
---
M TODO-RELEASE
M include/Makefile.am
M include/osmocom/gsm/gsup.h
A include/osmocom/gsm/gsup_sms.h
M src/gsm/Makefile.am
M src/gsm/gsup.c
A src/gsm/gsup_sms.c
M src/gsm/libosmogsm.map
M tests/gsup/gsup_test.c
M tests/gsup/gsup_test.err
M tests/gsup/gsup_test.ok
11 files changed, 527 insertions(+), 15 deletions(-)
Approvals:
Jenkins Builder: Verified
Stefan Sperling: Looks good to me, but someone else must approve
Harald Welte: Looks good to me, approved
diff --git a/TODO-RELEASE b/TODO-RELEASE
index 00720f6..16d96ec 100644
--- a/TODO-RELEASE
+++ b/TODO-RELEASE
@@ -16,3 +16,6 @@
libosmogsm gsm0808_cause_ext() check for cause extended bit
libosmogsm gsm0808_cause_name()use enum as parameter
libosmogsm gsm0808_create_cipher_reject() use enum as parameter
+libosmogsm osmo_gsup_message extended with SMS
related fields
+libosmogsm osmo_gsup_sms_{en|de}code_sm_rp_da GSUP SM-RP-DA coding
helpers
+libosmogsm osmo_gsup_sms_{en|de}code_sm_rp_oa GSUP SM-RP-OA coding
helpers
diff --git a/include/Makefile.am b/include/Makefile.am
index ccf9e10..86d8d15 100644
--- a/include/Makefile.am
+++ b/include/Makefile.am
@@ -94,6 +94,7 @@
osmocom/gsm/gsm48_ie.h \
osmocom/gsm/gsm_utils.h \
osmocom/gsm/gsup.h \
+ osmocom/gsm/gsup_sms.h \
osmocom/gsm/ipa.h \
osmocom/gsm/lapd_core.h \
osmocom/gsm/lapdm.h \
diff --git a/include/osmocom/gsm/gsup.h b/include/osmocom/gsm/gsup.h
index cd6fd31..192b877 100644
--- a/include/osmocom/gsm/gsup.h
+++ b/include/osmocom/gsm/gsup.h
@@ -40,6 +40,7 @@
#include
#include
+#include
#include
#include
#include
@@ -88,6 +89,14 @@
/*! Supplementary Services payload */
OSMO_GSUP_SS_INFO_IE= 0x35,
+
+ /* SM related IEs (see 3GPP TS 29.002, section 7.6.8) */
+ OSMO_GSUP_SM_RP_MR_IE = 0x40,
+ OSMO_GSUP_SM_RP_DA_IE = 0x41,
+ OSMO_GSUP_SM_RP_OA_IE = 0x42,
+ OSMO_GSUP_SM_RP_UI_IE = 0x43,
+ OSMO_GSUP_SM_RP_CAUSE_IE= 0x44,
+ OSMO_GSUP_SM_RP_MMS_IE = 0x45,
};
/*! GSUP message type */
@@ -121,6 +130,14 @@
OSMO_GSUP_MSGT_PROC_SS_REQUEST = 0b0010,
OSMO_GSUP_MSGT_PROC_SS_ERROR= 0b0011,
OSMO_GSUP_MSGT_PROC_SS_RESULT = 0b00100010,
+
+ OSMO_GSUP_MSGT_MO_FORWARD_SM_REQUEST= 0b00100100,
+ OSMO_GSUP_MSGT_MO_FORWARD_SM_ERROR = 0b00100101,
+ OSMO_GSUP_MSGT_MO_FORWARD_SM_RESULT = 0b00100110,
+
+ OSMO_GSUP_MSGT_MT_FORWARD_SM_REQUEST= 0b00101000,
+ OSMO_GSUP_MSGT_MT_FORWARD_SM_ERROR = 0b00101001,
+ OSMO_GSUP_MSGT_MT_FORWARD_SM_RESULT = 0b00101010,
};
#define OSMO_GSUP_IS_MSGT_REQUEST(msgt) (((msgt) & 0b0011) == 0b00)
@@ -213,6 +230,26 @@
/*! ASN.1 encoded MAP payload for Supplementary Services */
uint8_t
Change in libosmocore[master]: GSUP/SMS: introduce MO-/MT-FORWARD-SM messages
Harald Welte has posted comments on this change. ( https://gerrit.osmocom.org/11069 ) Change subject: GSUP/SMS: introduce MO-/MT-FORWARD-SM messages .. Patch Set 12: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/11069 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibe325c64ae2d6c626b232533bb4cbc65fc2b5d71 Gerrit-Change-Number: 11069 Gerrit-PatchSet: 12 Gerrit-Owner: Vadim Yanitskiy Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: Stefan Sperling Gerrit-Reviewer: Vadim Yanitskiy Gerrit-Comment-Date: Tue, 18 Dec 2018 16:42:50 + Gerrit-HasComments: No Gerrit-HasLabels: Yes
Change in libosmocore[master]: GSUP/SMS: introduce MO-/MT-FORWARD-SM messages
Stefan Sperling has posted comments on this change. ( https://gerrit.osmocom.org/11069 ) Change subject: GSUP/SMS: introduce MO-/MT-FORWARD-SM messages .. Patch Set 12: Code-Review+1 -- To view, visit https://gerrit.osmocom.org/11069 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibe325c64ae2d6c626b232533bb4cbc65fc2b5d71 Gerrit-Change-Number: 11069 Gerrit-PatchSet: 12 Gerrit-Owner: Vadim Yanitskiy Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: Stefan Sperling Gerrit-Reviewer: Vadim Yanitskiy Gerrit-Comment-Date: Tue, 18 Dec 2018 15:17:37 + Gerrit-HasComments: No Gerrit-HasLabels: Yes
Change in libosmocore[master]: GSUP/SMS: introduce MO-/MT-FORWARD-SM messages
Vadim Yanitskiy has posted comments on this change. ( https://gerrit.osmocom.org/11069 ) Change subject: GSUP/SMS: introduce MO-/MT-FORWARD-SM messages .. Patch Set 11: (3 comments) https://gerrit.osmocom.org/#/c/11069/11/src/gsm/gsup.c File src/gsm/gsup.c: https://gerrit.osmocom.org/#/c/11069/11/src/gsm/gsup.c@668 PS11, Line 668: sizeof(u8), gsup_msg->sm_rp_mr); > Use sizeof(*gsup_msg->sm_rp_mr) here? Done https://gerrit.osmocom.org/#/c/11069/11/src/gsm/gsup.c@694 PS11, Line 694: sizeof(u8), gsup_msg->sm_rp_mms); > Use sizeof(*gsup_msg->sm_rp_mms)? Done https://gerrit.osmocom.org/#/c/11069/11/src/gsm/gsup.c@699 PS11, Line 699: sizeof(u8), gsup_msg->sm_rp_cause); > Use sizeof(*gsup_msg->sm_rp_cause)? Done -- To view, visit https://gerrit.osmocom.org/11069 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibe325c64ae2d6c626b232533bb4cbc65fc2b5d71 Gerrit-Change-Number: 11069 Gerrit-PatchSet: 11 Gerrit-Owner: Vadim Yanitskiy Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: Stefan Sperling Gerrit-Reviewer: Vadim Yanitskiy Gerrit-Comment-Date: Tue, 18 Dec 2018 14:53:40 + Gerrit-HasComments: Yes Gerrit-HasLabels: No
Change in libosmocore[master]: GSUP/SMS: introduce MO-/MT-FORWARD-SM messages
Hello Stefan Sperling, Neels Hofmeyr, Harald Welte, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/11069 to look at the new patch set (#12). Change subject: GSUP/SMS: introduce MO-/MT-FORWARD-SM messages .. GSUP/SMS: introduce MO-/MT-FORWARD-SM messages According to 3GPP TS 29.002, there are two services: - MAP-MO-FORWARD-SHORT-MESSAGE (see 12.2), - MAP-MT-FORWARD-SHORT-MESSAGE (see 12.9), which are used to forward MO/MT short messages. This change replicates both services as GSUP messages: - OSMO_GSUP_MSGT_MO_FORWARD_SM_*, - OSMO_GSUP_MSGT_MT_FORWARD_SM_*. Please note, that only the 'must-have' IEs are introduced by this change, in particular the following: - OSMO_GSUP_SM_RP_MR_IE (see note below), - OSMO_GSUP_SM_RP_DA_IE (see 7.6.8.1), - OSMO_GSUP_SM_RP_OA_IE (see 7.6.8.2), - OSMO_GSUP_SM_RP_UI_IE (see 7.6.8.4), - OSMO_GSUP_SM_RP_MMS_IE (see 7.6.8.7), - OSMO_GSUP_SM_RP_CAUSE_IE (see GSM TS 04.11, 8.2.5.4), where both SM_RP_DA and SM_RP_OA IEs basically contain a single nested TV of the following format: - T: identity type (see 'osmo_gsup_sms_sm_rp_oda_t'), - V: encoded identity itself (optional). According to GSM TS 04.11, every single message on the SM-RL has an unique message reference (see 8.2.3), that is used to link an RP-ACK or RP-ERROR message to the associated (preceding) RP-DATA or RP-SMMA message transfer attempt. In case of TCAP/MAP, this message reference is being mapped to the Invoke ID. But since GSUP has no 'Invoke ID' IE, and it is not required for other applications (other than SMS), this change introduces a special 'SM_RP_MR' IE that doesn't exist in MAP. Change-Id: Ibe325c64ae2d6c626b232533bb4cbc65fc2b5d71 Related Change-Id: (docs) Ie0150756c33c1352bc4eb49421824542c711175c Related Change-Id: (TTCN) Ibf49474a81235096c032ea21f217170f523bd94e Related: OS#3587 --- M TODO-RELEASE M include/Makefile.am M include/osmocom/gsm/gsup.h A include/osmocom/gsm/gsup_sms.h M src/gsm/Makefile.am M src/gsm/gsup.c A src/gsm/gsup_sms.c M src/gsm/libosmogsm.map M tests/gsup/gsup_test.c M tests/gsup/gsup_test.err M tests/gsup/gsup_test.ok 11 files changed, 527 insertions(+), 15 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/69/11069/12 -- To view, visit https://gerrit.osmocom.org/11069 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ibe325c64ae2d6c626b232533bb4cbc65fc2b5d71 Gerrit-Change-Number: 11069 Gerrit-PatchSet: 12 Gerrit-Owner: Vadim Yanitskiy Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: Stefan Sperling Gerrit-Reviewer: Vadim Yanitskiy
Change in libosmocore[master]: GSUP/SMS: introduce MO-/MT-FORWARD-SM messages
Stefan Sperling has posted comments on this change. ( https://gerrit.osmocom.org/11069 ) Change subject: GSUP/SMS: introduce MO-/MT-FORWARD-SM messages .. Patch Set 11: Code-Review+1 (3 comments) https://gerrit.osmocom.org/#/c/11069/11/src/gsm/gsup.c File src/gsm/gsup.c: https://gerrit.osmocom.org/#/c/11069/11/src/gsm/gsup.c@668 PS11, Line 668: sizeof(u8), gsup_msg->sm_rp_mr); Use sizeof(*gsup_msg->sm_rp_mr) here? https://gerrit.osmocom.org/#/c/11069/11/src/gsm/gsup.c@694 PS11, Line 694: sizeof(u8), gsup_msg->sm_rp_mms); Use sizeof(*gsup_msg->sm_rp_mms)? https://gerrit.osmocom.org/#/c/11069/11/src/gsm/gsup.c@699 PS11, Line 699: sizeof(u8), gsup_msg->sm_rp_cause); Use sizeof(*gsup_msg->sm_rp_cause)? -- To view, visit https://gerrit.osmocom.org/11069 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibe325c64ae2d6c626b232533bb4cbc65fc2b5d71 Gerrit-Change-Number: 11069 Gerrit-PatchSet: 11 Gerrit-Owner: Vadim Yanitskiy Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: Stefan Sperling Gerrit-Reviewer: Vadim Yanitskiy Gerrit-Comment-Date: Fri, 14 Dec 2018 12:54:57 + Gerrit-HasComments: Yes Gerrit-HasLabels: Yes
Change in libosmocore[master]: GSUP/SMS: introduce MO-/MT-FORWARD-SM messages
Hello Neels Hofmeyr, Harald Welte, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/11069 to look at the new patch set (#11). Change subject: GSUP/SMS: introduce MO-/MT-FORWARD-SM messages .. GSUP/SMS: introduce MO-/MT-FORWARD-SM messages According to 3GPP TS 29.002, there are two services: - MAP-MO-FORWARD-SHORT-MESSAGE (see 12.2), - MAP-MT-FORWARD-SHORT-MESSAGE (see 12.9), which are used to forward MO/MT short messages. This change replicates both services as GSUP messages: - OSMO_GSUP_MSGT_MO_FORWARD_SM_*, - OSMO_GSUP_MSGT_MT_FORWARD_SM_*. Please note, that only the 'must-have' IEs are introduced by this change, in particular the following: - OSMO_GSUP_SM_RP_MR_IE (see note below), - OSMO_GSUP_SM_RP_DA_IE (see 7.6.8.1), - OSMO_GSUP_SM_RP_OA_IE (see 7.6.8.2), - OSMO_GSUP_SM_RP_UI_IE (see 7.6.8.4), - OSMO_GSUP_SM_RP_MMS_IE (see 7.6.8.7), - OSMO_GSUP_SM_RP_CAUSE_IE (see GSM TS 04.11, 8.2.5.4), where both SM_RP_DA and SM_RP_OA IEs basically contain a single nested TV of the following format: - T: identity type (see 'osmo_gsup_sms_sm_rp_oda_t'), - V: encoded identity itself (optional). According to GSM TS 04.11, every single message on the SM-RL has an unique message reference (see 8.2.3), that is used to link an RP-ACK or RP-ERROR message to the associated (preceding) RP-DATA or RP-SMMA message transfer attempt. In case of TCAP/MAP, this message reference is being mapped to the Invoke ID. But since GSUP has no 'Invoke ID' IE, and it is not required for other applications (other than SMS), this change introduces a special 'SM_RP_MR' IE that doesn't exist in MAP. Change-Id: Ibe325c64ae2d6c626b232533bb4cbc65fc2b5d71 Related Change-Id: (docs) Ie0150756c33c1352bc4eb49421824542c711175c Related Change-Id: (TTCN) Ibf49474a81235096c032ea21f217170f523bd94e Related: OS#3587 --- M TODO-RELEASE M include/Makefile.am M include/osmocom/gsm/gsup.h A include/osmocom/gsm/gsup_sms.h M src/gsm/Makefile.am M src/gsm/gsup.c A src/gsm/gsup_sms.c M src/gsm/libosmogsm.map M tests/gsup/gsup_test.c M tests/gsup/gsup_test.err M tests/gsup/gsup_test.ok 11 files changed, 527 insertions(+), 15 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/69/11069/11 -- To view, visit https://gerrit.osmocom.org/11069 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ibe325c64ae2d6c626b232533bb4cbc65fc2b5d71 Gerrit-Change-Number: 11069 Gerrit-PatchSet: 11 Gerrit-Owner: Vadim Yanitskiy Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: Vadim Yanitskiy
Change in libosmocore[master]: GSUP/SMS: introduce MO-/MT-FORWARD-SM messages
Vadim Yanitskiy has posted comments on this change. (
https://gerrit.osmocom.org/11069 )
Change subject: GSUP/SMS: introduce MO-/MT-FORWARD-SM messages
..
Patch Set 10:
(23 comments)
https://gerrit.osmocom.org/#/c/11069/10/include/osmocom/gsm/gsup.h
File include/osmocom/gsm/gsup.h:
https://gerrit.osmocom.org/#/c/11069/10/include/osmocom/gsm/gsup.h@235
PS10, Line 235: * Please note that there is no SM-RP-MR in TCAP/MAP!
SM-RP-MR
> in doxygen this becomes […]
Done
https://gerrit.osmocom.org/#/c/11069/10/include/osmocom/gsm/gsup.h@237
PS10, Line 237: const uint8_t *sm_rp_mr;
> oh wow, so we don't inline uint8_t[] then? hmm. ok. ok then... […]
Sorry, what is this comment / question about? Alignment?
How is this comment related to this change?
https://gerrit.osmocom.org/#/c/11069/10/include/osmocom/gsm/gsup_sms.h
File include/osmocom/gsm/gsup_sms.h:
https://gerrit.osmocom.org/#/c/11069/10/include/osmocom/gsm/gsup_sms.h@17
PS10, Line 17: OSMO_GSUP_SMS_SM_RP_ODA_NONE= 0x00,
> OSMO_GSUP_SMS is the gsup-sub-type SMS […]
Harald is correct here, so ignored.
https://gerrit.osmocom.org/#/c/11069/10/include/osmocom/gsm/gsup_sms.h@25
PS10, Line 25: /* Forward declarations (to avoid mutual include) */
> yes, that's what they are, and I think all C programmers should be aware of
> that concept?
Done
https://gerrit.osmocom.org/#/c/11069/10/include/osmocom/gsm/gsup_sms.h@29
PS10, Line 29: /* SM-RP-DA IE coding functions */
> a) I see that in the function name; unless you also explain "SM", "RP" or
> "DA", you might as well dr […]
Removed
https://gerrit.osmocom.org/#/c/11069/10/src/gsm/gsup.c
File src/gsm/gsup.c:
https://gerrit.osmocom.org/#/c/11069/10/src/gsm/gsup.c@569
PS10, Line 569: int idx, rc;
> (rather place each var on its own line)
Ignored. Why not?
https://gerrit.osmocom.org/#/c/11069/10/src/gsm/gsup.c@668
PS10, Line 668: sizeof(u8), gsup_msg->sm_rp_mr);
> (either line up with '(' or use a single tab as indent)
I actually followed the style of msgb_tlv_put() statements
above. We can unify the alignment in a separate path.
https://gerrit.osmocom.org/#/c/11069/10/src/gsm/gsup_sms.c
File src/gsm/gsup_sms.c:
https://gerrit.osmocom.org/#/c/11069/10/src/gsm/gsup_sms.c@33
PS10, Line 33: * SMS (Short Message Service) extensions for Osmocom GSUP
> please please use punctuation to end the summary line.
Done
https://gerrit.osmocom.org/#/c/11069/10/src/gsm/gsup_sms.c@35
PS10, Line 35:
> (I think we usually write in one line […]
Agree, fixed.
https://gerrit.osmocom.org/#/c/11069/10/src/gsm/gsup_sms.c@37
PS10, Line 37: * Encode SM-RP-DA IE (see 7.6.8.1), Destination Address
> please please use punctuation to end the summary line.
Done
https://gerrit.osmocom.org/#/c/11069/10/src/gsm/gsup_sms.c@43
PS10, Line 43: const struct osmo_gsup_message *gsup_msg)
> (I guess this fits on a 120 wide line?)
Not critical, ignored.
https://gerrit.osmocom.org/#/c/11069/10/src/gsm/gsup_sms.c@53
PS10, Line 53: /*! Special case for noSM-RP-DA and noSM-RP-OA */
> i doubt code inline doxygen makes sense. […]
Copy-pasted comment. Fixed, thanks!
https://gerrit.osmocom.org/#/c/11069/10/src/gsm/gsup_sms.c@55
PS10, Line 55: msgb_tv_put
> I don't think we should introduce anything but TLV in GSUP. […]
I was actually abusing this function in order to put TL of the final TLTLV.
Anyway, this doesn't make sense anymore, please see the new patch.
https://gerrit.osmocom.org/#/c/11069/10/src/gsm/gsup_sms.c@62
PS10, Line 62: "(type=0x%02x)!\n", gsup_msg->sm_rp_da_type);
> (also looks like comfortable fit for 120 width, at least for the char
> constant)
Not critical, ignored.
https://gerrit.osmocom.org/#/c/11069/10/src/gsm/gsup_sms.c@74
PS10, Line 74: ie_len = msg->tail + 1; /* To be calculated later */
> max recently added API for this, see msgb_tl_put()
Good to know, thanks!
https://gerrit.osmocom.org/#/c/11069/10/src/gsm/gsup_sms.c@82
PS10, Line 82: *ie_len = msg->tail - (ie_len + 1);
> I'm sure we have examples of nested IEs in other cases / protocol layers
> (like TS 12. […]
Done
https://gerrit.osmocom.org/#/c/11069/10/src/gsm/gsup_sms.c@88
PS10, Line 88: * Decode SM-RP-DA IE (see 7.6.8.1), Destination Address
> .
Done
https://gerrit.osmocom.org/#/c/11069/10/src/gsm/gsup_sms.c@125
PS10, Line 125: /*! Special case for noSM-RP-DA and noSM-RP-OA */
> .
Copy pasted comment. Thanks, fixed!
https://gerrit.osmocom.org/#/c/11069/10/src/gsm/gsup_sms.c@142
PS10, Line 142: gsup_msg->sm_rp_da_len = id_len;
> you are failing to check that the inner len does not surpass the outer TLV.
> […]
Done
https://gerrit.osmocom.org/#/c/11069/10/src/gsm/gsup_sms.c@147
PS10, Line 147: /*!
: * Encode SM-RP-OA IE (see 7.6.8.2), Originating Address
Fixed too.
https://gerrit.osmocom.org/#/c/11069/10/src/gsm/gsup_sms.c@163
PS10,
Change in libosmocore[master]: GSUP/SMS: introduce MO-/MT-FORWARD-SM messages
Harald Welte has posted comments on this change. (
https://gerrit.osmocom.org/11069 )
Change subject: GSUP/SMS: introduce MO-/MT-FORWARD-SM messages
..
Patch Set 10: -Code-Review
(6 comments)
https://gerrit.osmocom.org/#/c/11069/10/include/osmocom/gsm/gsup_sms.h
File include/osmocom/gsm/gsup_sms.h:
https://gerrit.osmocom.org/#/c/11069/10/include/osmocom/gsm/gsup_sms.h@17
PS10, Line 17: OSMO_GSUP_SMS_SM_RP_ODA_NONE= 0x00,
> isn't "SMS_SM" redundant? could it be just OSMO_GSUP_SMS_ or just
> OSMO_GSUP_SM_?
OSMO_GSUP_SMS is the gsup-sub-type SMS
SM_RP_ODA is the 3GPP part for the SM- sub-layer. Everything fine here.
https://gerrit.osmocom.org/#/c/11069/10/include/osmocom/gsm/gsup_sms.h@29
PS10, Line 29: /* SM-RP-DA IE coding functions */
> a) I see that in the function name; unless you also explain "SM", "RP" or
> "DA", you might as well dr […]
The assumption here is probably that the potential reader is familiar with the
SMS system as specified by 3GPP (and hence knows SM-RP-DA), but he may not be
familiar with the osmocom specific GSUP protocol.
https://gerrit.osmocom.org/#/c/11069/10/src/gsm/gsup.c
File src/gsm/gsup.c:
https://gerrit.osmocom.org/#/c/11069/10/src/gsm/gsup.c@569
PS10, Line 569: int idx, rc;
> (rather place each var on its own line)
we never had any such rule
https://gerrit.osmocom.org/#/c/11069/10/src/gsm/gsup.c@668
PS10, Line 668: sizeof(u8), gsup_msg->sm_rp_mr);
> (either line up with '(' or use a single tab as indent)
we also never enforced a rule like this. I'm sure there are plenty of examples
that look like this.
https://gerrit.osmocom.org/#/c/11069/10/src/gsm/gsup_sms.c
File src/gsm/gsup_sms.c:
https://gerrit.osmocom.org/#/c/11069/10/src/gsm/gsup_sms.c@55
PS10, Line 55: msgb_tv_put
I don't think we should introduce anything but TLV in GSUP. Having the IEI
determine if it's a TV/TLV/TL16V etc. is quite ugly and we shouldn't replicate
this in our own protocols. Let's have each IE be a TLV.
https://gerrit.osmocom.org/#/c/11069/10/src/gsm/gsup_sms.c@82
PS10, Line 82: *ie_len = msg->tail - (ie_len + 1);
> so ... this looks like this now? […]
I'm sure we have examples of nested IEs in other cases / protocol layers (like
TS 12.21 Abis OML), in the xUA routing key management of osmo-stp. Not in GSUP
so far, but I don't understand your strong opposition to it.
What I do understand is that the current proposed code by vadim isn't nice.
Whatever the solutoin (nested or not), there must be proper helper functions
that avoid manual pointer calculations like the lines above.
--
To view, visit https://gerrit.osmocom.org/11069
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe325c64ae2d6c626b232533bb4cbc65fc2b5d71
Gerrit-Change-Number: 11069
Gerrit-PatchSet: 10
Gerrit-Owner: Vadim Yanitskiy
Gerrit-Reviewer: Harald Welte
Gerrit-Reviewer: Jenkins Builder (102)
Gerrit-Reviewer: Neels Hofmeyr
Gerrit-Reviewer: Vadim Yanitskiy
Gerrit-Comment-Date: Thu, 13 Dec 2018 10:21:03 +
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
Change in libosmocore[master]: GSUP/SMS: introduce MO-/MT-FORWARD-SM messages
Neels Hofmeyr has posted comments on this change. (
https://gerrit.osmocom.org/11069 )
Change subject: GSUP/SMS: introduce MO-/MT-FORWARD-SM messages
..
Patch Set 10: Code-Review-1
(25 comments)
many comments aren't critical, except:
- I think the OA/DA IE structure needs to be flattened.
- Use punctuation and other doxygen misc.
couldn't resist to remark on some peculiarities that you are just taking over
from other code around there, please ignore those...
https://gerrit.osmocom.org/#/c/11069/10/include/osmocom/gsm/gsup.h
File include/osmocom/gsm/gsup.h:
https://gerrit.osmocom.org/#/c/11069/10/include/osmocom/gsm/gsup.h@94
PS10, Line 94: OSMO_GSUP_SM_RP_MR_IE = 0x40,
oh ok, we really have "_IE" in the end then? well... ok then.
https://gerrit.osmocom.org/#/c/11069/10/include/osmocom/gsm/gsup.h@134
PS10, Line 134: OSMO_GSUP_MSGT_MO_FORWARD_SM_REQUEST= 0b00100100,
also interesting that we define the message type in binary... ok then
https://gerrit.osmocom.org/#/c/11069/10/include/osmocom/gsm/gsup.h@235
PS10, Line 235: * Please note that there is no SM-RP-MR in TCAP/MAP!
SM-RP-MR
in doxygen this becomes
SM-RP-MR (see 3GPP TS 29.002, 7.6.1.1), Message Reference Please note that...
Please use punctuation!!
https://gerrit.osmocom.org/#/c/11069/10/include/osmocom/gsm/gsup.h@237
PS10, Line 237: const uint8_t *sm_rp_mr;
oh wow, so we don't inline uint8_t[] then? hmm. ok. ok then...
and we use tabs... well then
https://gerrit.osmocom.org/#/c/11069/10/include/osmocom/gsm/gsup_sms.h
File include/osmocom/gsm/gsup_sms.h:
https://gerrit.osmocom.org/#/c/11069/10/include/osmocom/gsm/gsup_sms.h@17
PS10, Line 17: OSMO_GSUP_SMS_SM_RP_ODA_NONE= 0x00,
isn't "SMS_SM" redundant? could it be just OSMO_GSUP_SMS_ or just OSMO_GSUP_SM_?
https://gerrit.osmocom.org/#/c/11069/10/include/osmocom/gsm/gsup_sms.h@25
PS10, Line 25: /* Forward declarations (to avoid mutual include) */
yes, that's what they are, and I think all C programmers should be aware of
that concept?
https://gerrit.osmocom.org/#/c/11069/10/include/osmocom/gsm/gsup_sms.h@29
PS10, Line 29: /* SM-RP-DA IE coding functions */
a) I see that in the function name; unless you also explain "SM", "RP" or "DA",
you might as well drop the entire comment. (I would opt for dropping cruft)
b) place function comments in the .c file plz
https://gerrit.osmocom.org/#/c/11069/10/src/gsm/gsup.c
File src/gsm/gsup.c:
https://gerrit.osmocom.org/#/c/11069/10/src/gsm/gsup.c@569
PS10, Line 569: int idx, rc;
(rather place each var on its own line)
https://gerrit.osmocom.org/#/c/11069/10/src/gsm/gsup.c@668
PS10, Line 668: sizeof(u8), gsup_msg->sm_rp_mr);
(either line up with '(' or use a single tab as indent)
https://gerrit.osmocom.org/#/c/11069/10/src/gsm/gsup_sms.c
File src/gsm/gsup_sms.c:
https://gerrit.osmocom.org/#/c/11069/10/src/gsm/gsup_sms.c@33
PS10, Line 33: * SMS (Short Message Service) extensions for Osmocom GSUP
please please use punctuation to end the summary line.
https://gerrit.osmocom.org/#/c/11069/10/src/gsm/gsup_sms.c@35
PS10, Line 35:
(I think we usually write in one line
/*! Foo yada
but then again I think Linus Torvalds referred to that as brain damaged once,
so whatever.)
https://gerrit.osmocom.org/#/c/11069/10/src/gsm/gsup_sms.c@37
PS10, Line 37: * Encode SM-RP-DA IE (see 7.6.8.1), Destination Address
please please use punctuation to end the summary line.
https://gerrit.osmocom.org/#/c/11069/10/src/gsm/gsup_sms.c@43
PS10, Line 43: const struct osmo_gsup_message *gsup_msg)
(I guess this fits on a 120 wide line?)
https://gerrit.osmocom.org/#/c/11069/10/src/gsm/gsup_sms.c@53
PS10, Line 53: /*! Special case for noSM-RP-DA and noSM-RP-OA */
i doubt code inline doxygen makes sense. it might even end up in above function
doc block?
https://gerrit.osmocom.org/#/c/11069/10/src/gsm/gsup_sms.c@62
PS10, Line 62: "(type=0x%02x)!\n", gsup_msg->sm_rp_da_type);
(also looks like comfortable fit for 120 width, at least for the char constant)
https://gerrit.osmocom.org/#/c/11069/10/src/gsm/gsup_sms.c@74
PS10, Line 74: ie_len = msg->tail + 1; /* To be calculated later */
max recently added API for this, see msgb_tl_put()
https://gerrit.osmocom.org/#/c/11069/10/src/gsm/gsup_sms.c@82
PS10, Line 82: *ie_len = msg->tail - (ie_len + 1);
so ... this looks like this now?
[DA_IE, len, [DA_TYPE, len, DA data...] ]
Can this instead remain a flat layer of TLV structures? There are two nested
layers, I think we should avoid that. Otherwise we are opening up corner cases,
and both len fields need to be checked: the first one implicitly by our TLV
decoder (nice), and the second one manually after having retrieved the first V
(sigh)
Define the IE as
[ DA_IE, len, TYPE, data ]
and you can derive data_len = overall_len -
Change in libosmocore[master]: GSUP/SMS: introduce MO-/MT-FORWARD-SM messages
Vadim Yanitskiy has posted comments on this change. ( https://gerrit.osmocom.org/11069 ) Change subject: GSUP/SMS: introduce MO-/MT-FORWARD-SM messages .. Set Ready For Review -- To view, visit https://gerrit.osmocom.org/11069 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibe325c64ae2d6c626b232533bb4cbc65fc2b5d71 Gerrit-Change-Number: 11069 Gerrit-PatchSet: 10 Gerrit-Owner: Vadim Yanitskiy Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Vadim Yanitskiy Gerrit-Comment-Date: Sun, 02 Dec 2018 22:21:25 + Gerrit-HasComments: No Gerrit-HasLabels: No
Change in libosmocore[master]: GSUP/SMS: introduce MO-/MT-FORWARD-SM messages
Vadim Yanitskiy has posted comments on this change. ( https://gerrit.osmocom.org/11069 ) Change subject: GSUP/SMS: introduce MO-/MT-FORWARD-SM messages .. Set Ready For Review -- To view, visit https://gerrit.osmocom.org/11069 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibe325c64ae2d6c626b232533bb4cbc65fc2b5d71 Gerrit-Change-Number: 11069 Gerrit-PatchSet: 9 Gerrit-Owner: Vadim Yanitskiy Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Vadim Yanitskiy Gerrit-Comment-Date: Tue, 27 Nov 2018 09:25:05 + Gerrit-HasComments: No Gerrit-HasLabels: No
