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

historical

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, Line 163: 	/*! 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@197
PS10, Line 197: /*!
              :  * Decode 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@234
PS10, Line 234: /*! 
Fixed.

-- 
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: Fri, 14 Dec 2018 00:26:39 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20181214/c304c139/attachment.htm>