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)
> log error, or maybe OSMO_ASSERT(sms_report)?
Is there anything the user can do after this error?

Why bother?

I mean, you only hit this is there is not memory left... So likely there is a 
leak.

OSMO_ASSERT() would stop openBSC from running, is that getting this any better? 
This sounds like calling BUG() in the kernel.

So what do we get from spamming the user with an error that *only* happens if 
we have no memory left?


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
> (hard to understand, fix typo/punctuation?)
This is a reply to a DELIVER_SM with esm_class = Delivery Receipt, we're just 
having a conversation that was initiated by the SMSC. So we don't need to send 
any RP-ACK to the mobile, as we do with DELIVER_SM for a plan SMS.


Line 659:               deliver.esm_class = 0x04;
> (would be nice to have constants instead of magic numbers ... but not relat
I agree, this applies all over the place, but that would require a bit of work.


Line 709:                      sms->msg_ref);
> user message reference was always missing? maybe the fact that it is added 
Yes, message reference was always missing because there was no status-reports 
;-).

We can just add a branch here to skip this if saving bytes on the network are 
really a concern at the cost of slowing down the CPU path.


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

Line 95:        bool                    report;
> (can be understood as "you should report" or "this is a report" ... would b
This just annotates that the SMPP command that we send represents a 
deliver-report.


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

Reply via email to