Hi Pekka, On Wed, Aug 18, 2010 at 07:25:08PM +0300, Pekka Pessi wrote: > Hi Samuel, > > 2010/8/18 Samuel Ortiz <[email protected]>: > >> Create modem only after Modem.GetProperties succeeds, thus avoiding races > >> with Modem.PropertyChanged signal. > > So I've commented on this one and my concernas about the Powered property > > not > > being handled anymore, except when creating the modem. > > I must have missed that, good thing you catched it. > > The current code tries to set Modem.Powered after there is a change in > ModemManager.Modems list. The current behavior and my patch differ > only if there are multiple modems in the device. In my patch the > Modem.Powered is tried only once. If we need retries on > Modem.Powered, I think it is much better to handle explicitly than as > a side effect of plugging in another modem. No, I wasn't talking about retries, but rather about what happens when a second modem gets plugged in. But I realize that this would be handled correctly from your code. Nevermind.
> > I have another comment: > > > >> + new_modem = add_modem(path); > >> + if (new_modem == NULL) > >> + goto done; > > I don't like the fact that add_modem returns NULL when failing but also when > > the modem already exists. I would prefer to keep the semantics as they are > > right now, and handle the below code from add_modem(). So add_modem() > > prototype would change but will always return the right modem when not > > failing > > and NULL otherwise. > > Is there a need for another code path for creating modems? No, but since you're only calling add_modem() once now, you could add the following code: + if (!powered) + modem_change_powered(path, TRUE); + + if (has_gprs) + get_imsi(path); + to it, and keep the correct semantics, i.e. return NULL from add_modem() only when it failed. If the modem is already there, you just return it. Cheers, Samuel. > If so, I'm > happy to duplicate the hash table lookup in modem_properties_reply(). > > -- > Pekka.Pessi mail at nokia.com > _______________________________________________ > connman mailing list > [email protected] > http://lists.connman.net/listinfo/connman -- Intel Open Source Technology Centre http://oss.intel.com/ _______________________________________________ connman mailing list [email protected] http://lists.connman.net/listinfo/connman
