Re: ModemManager: SMS List API doesn't have an index, buffer isn't big enough

[Dan Williams <dcbw redhat com>]

On Wed, 2011-04-06 at 12:16 -0400, Nathan Williams wrote:
> Hi, all. In the process of implementing some SMS support, I've run
> into a couple of issues with the
> org.freedesktop.ModemManager.Modem.Gsm.SMS.List DBus call, one in the
> API and one in the implementation.

Excellent :)  We can change the API because for all practical purposes,
it's not being used yet.

> The API issue is that as specified, the caller gets the contents of
> all of the messages, but not their index numbers, so there's no way to
> delete the messages that have been received. Here are three ways I've
> considered fixing this:
> 
> 
>  1. Change to a List command that just gets the index numbers of the
> messages and lets the caller Get and Delete from that as needed. This
> is an OK API but doesn't really match the AT commands and would be a
> bit inefficient in that regard.
>  2. Change the return type from "aa{sv}" to something that includes
> the index distinct from the message, such as "a(ia{sv})". Reflects the
> low level well, creates a bit of extra assembly and disassembly work.
>  3. Add an "index" integer element to the dictionary of each message.
> This is both easy and still pretty closely reflects the low-level
> operation.
> 
> 
> I'm leaning towards #3.

Yeah, I agree.  #2 maps more closely to the other methods since they all
take top-level "u" arguments, but having it in the dict isn't a problem.
But make sure you make the 'index' item type uint32 ("u") to match the
rest instead of 'i'.

> 
> The implementation issue is the 2kb buffer-size limit imposed in
> mm-serial-port.c. With a bunch of test messages on my device, I'm
> already up past 3kb of data returned from a single AT+CMGL command. A
> 30-message memory could be up to 10kb with maximum-size messages, and
> I have no reason to think that there aren't devices with larger
> memories. At the very least, I'll cook up a patch to raise the buffer
> size limit, perhaps to 64k; ideally, the stack-allocated read buf size
> wouldn't have to increase as well, since most commands don't need this
> much space. Alternately, it would be nice to have some other means of
> detecting out-of-control modem spew besides an arbitrary size limit.
> Suggestions?

Just took a look at the code and that's my read of it too.  The
out-of-control thing is only required for probing, so perhaps we could
add another GObject property for "spew-control" (boolean) and when
probing we'd set that property to TRUE, but normally it would be FALSE.
THen we'd check that in data_available() like so:

    /* Make sure the response doesn't grow too long */
    if (priv->response->len > SERIAL_BUF_SIZE && priv->spew_control) {

We're certainly not going to be grabbing messages when probing the modem
so the two operations here are mutually exclusive.  Note that
priv->response will grow automatically in size so I don't think we need
to adjust the initial 500 byte size at all.

If you're not entirely familiar with GObject properties yet, here's what
you do...

1) add the property string name in mm-serial-port.h near the top; I like
to use the #defined names instead of strings to ensure that wrong
property names are compile errors instead of runtime ones

2) add the internal property number at the top of mm-serial-port.c in
the property enum

3) in mm_serial_port_class_init() register the property, you'll be using
g_param_spec_boolean, and make the default be FALSE.  You'll want
G_PARAM_CONSTRUCT_ONLY and G_PARAM_READWRITE for flags since we only
ever want to set the property when we create the port.

4) add the 'gboolean spew_control' member to the NMSerialPortPrivate
struct

5) then you add the property to get_property() and set_property() using
g_value_set_boolean() and g_value_get_boolean() respectively, and hook
up the code to set or return priv->spew_control accordingly

6) then you can use priv->spew_control to turn it off in
data_available().

If you knew all that already, my apologies... :)  Sounds good to me!

Dan