Patch Set 1:

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 :)
File openbsc/src/libmsc/smpp_openbsc.c:

Line 521:       /* We got a DELIVER_SM response for this is status report, this 
> 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

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.
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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic1a9023074bfa938099377980b6aff9b262fab2a
Gerrit-PatchSet: 1
Gerrit-Project: openbsc
Gerrit-Branch: master
Gerrit-Owner: Pablo Neira Ayuso <>
Gerrit-Reviewer: Harald Welte <>
Gerrit-Reviewer: Holger Freyther <>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Keith Whyte <>
Gerrit-Reviewer: Neels Hofmeyr <>
Gerrit-Reviewer: Pablo Neira Ayuso <>
Gerrit-HasComments: Yes

Reply via email to