Re: [PATCH] GSM SMS reception code

[Dan Williams <dcbw redhat com>]

On Tue, 2011-04-12 at 17:05 -0400, Nathan Williams wrote:
> Here's the implementation I've done of SMS reception code for GSM
> modems, exclusive of the bug fixes discussed here earlier. It
> implements the SmsReceived signal and the Get, Delete, and List
> commands; it does not group multi-part messages together into one, but
> does skip over the user-data header to avoid presenting garbage to the
> user. 
>  
> Credit to Torgny Johansson <torgny johansson ericsson com> for the
> initial basis of this code.

Looks great, pushed with a few cleanups as
487972c1ac40b057c35dac51958f04c13f6137d8.

Some suggestions for further patches:

1) give sms_parse_pdu() a GError **error argument, and set that
internally when things fail so that callers can get usable error
information instead of calling mm_err().  If you want to define new
errors, see mm-errors.c and mm-errors.h.  Those files basically map the
ETSI standard error numbers to GErrors, so we should add a bunch for the
CMS errors, and then we can also add MM-specific errors with higher
numbers.  The numbers are only used internally for mapping the returned
numberic CMS/CME to a GError, while dbus-glib translates them into the
D-Bus familiar format of "org.freedesktop.ModemManager.xxxx" instead.

2) Testcases for PDU decoding; best way to do this is to split out the
bits of the SMS stuff that doesn't depend on anything MM-related like
MMCallbackInfo and whatnot into a separate file, like mm-sms-utils.c or
something, and build that file into libmodem_helpers_la_SOURCES in
Makefile.am.  That makes it easier to link that code to the testcases
without including a bunch of random stuff that isn't related to the unit
tests.  Then we grab a bunch of real-world encoded SMS PDUs and shove
them through the decoder and make sure it works on those, then we can
start fuzzing it too and make sure it's more robust.  We'll really want
testcases as there's quite a bit of variation between devices and
frankly nobody interprets the standards the same way :(

3) Do we need to do endian conversions here for BE arches when decoding
or constructing the PDU?

4) The whole index thing, figuring out if we can use a generic index
that's MM specific and maps internally to SIM indexes, or whether we can
in fact just use the SIM index...

Keep the patches coming :)

Thanks!
Dan