Re: [PATCH] ModemManager: Icera CID handling without an APN

[Dan Williams <dcbw redhat com>]

On Thu, 2011-05-12 at 19:02 -0400, Nathan Williams wrote:
> In mm-generic-gsm.c, if an APN is not provided as a property to
> simple_connect(), priv->cid is never set and remains at -1.
> The Icera modem plugin has a _get_cid() wrapper call which checks for
> this and returns 0 if the stored CID is -1. There are two problems
> with this:
>   * 0 is not a valid CID; according to 3GPP 27.007, CIDs have a
> minimum value of 1. Thus, if an APN was not provided, the plugin
> issues commands like %IPDPCFG and %IPDPACT with CID=0, which the modem
> rejects.
>   * connection_enabled() uses the priv->cid value rather than the
> _get_cid() wrapped value, and bails out if it's -1, so a connection is
> never acknowledged and the modem remains in "connecting" state.

So we should never be using an invalid CID here, it should always be 1
or higher.  I think the -1 was used as an error value that we should
never hit, and there used to be assertions for that in the code.  I
think the whole dance here was just to be paranoid since we never ever
want to send -1 to the modem, which happened before in some corner cases
which have since been fixed.

So we basically have to deal with the corner case of what happens if the
code somehow screws up and we for whatever reason can't figure out the
CID of the PDP context that we're trying to disconnect.  I think
returning "1" here should only happen for the disconnect case, since in
all other cases, there would be a logic error to have an invalid CID and
we should fail out, because that's a bug we need to fix.  So I don't
think the patch as sent is the right approach; perhaps we should
g_assert(cid > 1) in the other cases, and be more relaxed for
disconnect?  Or in disconnect if we dont' get a valid CID, we just send
"ATH" as a desperate attempt to kill everything since something went
wrong...

Dan

> 
> I've attached a patch that (I think) fixes this, by having _get_cid()
> return 1 and having connection_enabled() use it instead of
> mm_generic_gsm_get_cid(); however, I have a feeling that the way it
> works now is deliberate and I'm missing something about how the no-APN
> case is supposed to work.
> Thoughts? Or, if this analysis seems correct, the patch should be
> ready to go.
> 
> 
>     - Nathan
> 
> 
> _______________________________________________
> networkmanager-list mailing list
> networkmanager-list gnome org
> http://mail.gnome.org/mailman/listinfo/networkmanager-list