Change in libosmocore[master]: GSUP/SMS: introduce MO-/MT-FORWARD-SM messages

historical

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 - sizeof(TYPE), and you're already sure that this len doesn't break the TLV structure.

Furthermore, if this will ever be likely to need extension, there should be a reserved type value that indicates a differing structure. Like, type = 0xff indicates a format to be defined in the future. Inner TLVs? That would be quite ugly, but if we can officially reserve one type value for that now (which we can still get for free), we might be glad in the future. Then maybe NULL could be 0xfe...

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
.

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 */
.

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. Hence flatten please and this becomes unnecessary: define the IE as

   [ DA_IE, len, TYPE, data ]

and you can derive data_len = overall_len - sizeof(TYPE), and you're already sure that this len doesn't break the TLV structure.

https://gerrit.osmocom.org/#/c/11069/10/src/gsm/gsup_sms.c@163
PS10, Line 163: 	/*! Special case for noSM-RP-DA and noSM-RP-OA */
.

https://gerrit.osmocom.org/#/c/11069/10/src/gsm/gsup_sms.c@189
PS10, Line 189: 		gsup_msg->sm_rp_oa_len, gsup_msg->sm_rp_oa);
(same)

https://gerrit.osmocom.org/#/c/11069/10/src/gsm/gsup_sms.c@251
PS10, Line 251: 	gsup_msg->sm_rp_oa_len = id_len;
(same)

https://gerrit.osmocom.org/#/c/11069/10/tests/gsup/gsup_test.c
File tests/gsup/gsup_test.c:

https://gerrit.osmocom.org/#/c/11069/10/tests/gsup/gsup_test.c@231
PS10, Line 231: 			0xff, 0x00, /* Special case: noSM-RP-OA */
(again the inner len issue; would it still be valid with a nonzero len? would have to add tests...)

https://gerrit.osmocom.org/#/c/11069/10/tests/gsup/gsup_test.c@247
PS10, Line 247: 			0x03, 0x07, /* SMSC address */
(again the inner len issue; if you kept it this way you would have to add tests that assure a mismatching inner len results in parse error)

-- 
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 <axilirator at gmail.com>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder (1000002)
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Vadim Yanitskiy <axilirator at gmail.com>
Gerrit-Comment-Date: Mon, 10 Dec 2018 21:38:29 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20181210/62c9a505/attachment.htm>