This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
Harald Welte gerrit-no-reply at lists.osmocom.orgHarald 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 <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: Thu, 13 Dec 2018 10:21:03 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20181213/8ba781b3/attachment.htm>