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

[Dan Williams <dcbw redhat com>]

On Fri, 2011-04-08 at 18:09 -0400, Nathan Williams wrote:
> OK, the spew-control property is easy enough; see attached. However,
> being new to this codebase, I'm not sure where probing is happening
> and where I should be toggling this property on.

1c3101fbaf74cbb9d8d0eb52ad6f84dc01544cf0

The place you want is in
src/mm-plugin-base.c::mm_plugin_base_probe_port() where the probing
serial port gets opened.  Since you defaulted spew control to FALSE, we
want to turn it on in that function.  The probe ports will be closed
after probing is complete, and after that ports opened shouldn't have
spew control set at all due to the default.

Dan

> 
>     - Nathan
> 
> On Wed, Apr 6, 2011 at 7:21 PM, Dan Williams <dcbw redhat com> wrote:
>         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
>         
>         
> 
>