openbsc[master]: libmsc: add support for SMPP delivery receipts

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

Neels Hofmeyr gerrit-no-reply at lists.osmocom.org
Tue Aug 8 12:50:26 UTC 2017


Patch Set 1:

(5 comments)

https://gerrit.osmocom.org/#/c/3434/1/openbsc/src/libmsc/gsm_04_11.c
File openbsc/src/libmsc/gsm_04_11.c:

Line 637: 	if (!sms_report)
> Is there anything the user can do after this error?
if mem is up, the program will likely not do *anything* useful anymore. Instead of going on to try, probably producing lots of weird output after this, we might as well say what's up and bail. An OSMO_ASSERT() would sufficiently convey such meaning (and avoids possible mem problems for log composition). The current patch says nothing, drops the status report on the floor and goes on silently with other things, I believe that's not a good paradigm.

If you're bothering to check the pointer, then let's bother properly :)


https://gerrit.osmocom.org/#/c/3434/1/openbsc/src/libmsc/smpp_openbsc.c
File openbsc/src/libmsc/smpp_openbsc.c:

Line 521: 	/* We got a DELIVER_SM response for this is status report, this was
> This is a reply to a DELIVER_SM with esm_class = Delivery Receipt, we're ju
I was referring to

  "we got a X response for this is status report, this"

which is gramatically wrong, thus I find it hard to figure out... If this needs more comment, I'd appreciate it as in-code comment instead of gerrit review comment, for the benefit of future code readers.


Line 659: 		deliver.esm_class = 0x04;
> I agree, this applies all over the place, but that would require a bit of w
agreed


Line 709: 		       sms->msg_ref);
> Yes, message reference was always missing because there was no status-repor
cpu cycles or bytes aren't really a concern, only semantics. We're adding a TLV but not mentioning it, that's all.


https://gerrit.osmocom.org/#/c/3434/1/openbsc/src/libmsc/smpp_smsc.h
File openbsc/src/libmsc/smpp_smsc.h:

Line 95: 	bool			report;
> This just annotates that the SMPP command that we send represents a deliver
maybe 'bool is_report;' then? (nitpick)


-- 
To view, visit https://gerrit.osmocom.org/3434
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic1a9023074bfa938099377980b6aff9b262fab2a
Gerrit-PatchSet: 1
Gerrit-Project: openbsc
Gerrit-Branch: master
Gerrit-Owner: Pablo Neira Ayuso <pablo at gnumonks.org>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Holger Freyther <holger at freyther.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Keith Whyte <keith at rhizomatica.org>
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Pablo Neira Ayuso <pablo at gnumonks.org>
Gerrit-HasComments: Yes



More information about the gerrit-log mailing list