Patch Set 1:
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 :)
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.
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-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>