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 <pa...@gnumonks.org> Gerrit-Reviewer: Harald Welte <lafo...@gnumonks.org> Gerrit-Reviewer: Holger Freyther <hol...@freyther.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Keith Whyte <ke...@rhizomatica.org> Gerrit-Reviewer: Neels Hofmeyr <nhofm...@sysmocom.de> Gerrit-Reviewer: Pablo Neira Ayuso <pa...@gnumonks.org> Gerrit-HasComments: Yes
