Hi Pekka,
On Fri, Aug 13, 2010 at 09:14:57PM +0300, [email protected] wrote:
> From: Pekka Pessi <[email protected]>
>
> Create modem only once GetProperties succeed, thus avoiding races with
> PropertyChanged.
So you mean races between the ofono_connect() and the manager_watch() code
paths ? Could you give more details ?
> Do not call GetProperties on existing modem.
Aren't we then losing the point of watching the manager interface, except for
toggling the modem's available flag ?
Some additional comments/questions on your code:
> ---
> plugins/ofono.c | 49 +++++++++++++++++++++++++++----------------------
> 1 files changed, 27 insertions(+), 22 deletions(-)
>
> diff --git a/plugins/ofono.c b/plugins/ofono.c
> index 1a2a798..22682fb 100644
> --- a/plugins/ofono.c
> +++ b/plugins/ofono.c
> @@ -1029,14 +1029,11 @@ static struct modem_data *add_modem(const char *path)
> if (path == NULL)
> return NULL;
>
> - DBG("");
> + DBG("path %s", path);
>
> modem = g_hash_table_lookup(modem_hash, path);
> - if (modem != NULL) {
> - modem->available = TRUE;
> -
> - return modem;
> - }
> + if (modem != NULL)
> + return NULL;
>
> modem = g_try_new0(struct modem_data, 1);
> if (modem == NULL)
> @@ -1044,7 +1041,6 @@ static struct modem_data *add_modem(const char *path)
>
> modem->path = g_strdup(path);
> modem->device = NULL;
> - modem->available = TRUE;
So you're creating the modem at ofono_connect() time, and then whenever we get
a modem changed signal, we just use it to toggle the available flag ?
> g_hash_table_insert(modem_hash, g_strdup(path), modem);
>
> @@ -1077,6 +1073,9 @@ static void modem_properties_reply(DBusPendingCall
> *call, void *user_data)
> DBusError error;
> DBusMessageIter array, dict;
> const char *path = user_data;
> + dbus_bool_t powered = FALSE;
> + dbus_bool_t has_gprs = FALSE;
> + struct modem_data *new_modem;
>
> DBG("path %s", path);
>
> @@ -1103,7 +1102,6 @@ static void modem_properties_reply(DBusPendingCall
> *call, void *user_data)
> while (dbus_message_iter_get_arg_type(&dict) == DBUS_TYPE_DICT_ENTRY) {
> DBusMessageIter entry, value;
> const char *key;
> - dbus_bool_t powered;
>
> dbus_message_iter_recurse(&dict, &entry);
> dbus_message_iter_get_basic(&entry, &key);
> @@ -1113,35 +1111,39 @@ static void modem_properties_reply(DBusPendingCall
> *call, void *user_data)
>
> if (g_str_equal(key, "Powered") == TRUE) {
> dbus_message_iter_get_basic(&value, &powered);
> -
> - if (powered == FALSE) {
> - modem_change_powered(path, TRUE);
> - break;
> - }
> } else if (g_str_equal(key, "Interfaces") == TRUE) {
> if (modem_has_gprs(&value) == TRUE)
> - get_imsi(path);
> + has_gprs = TRUE;
> }
> +
> dbus_message_iter_next(&dict);
> }
>
> + new_modem = add_modem(path);
> + if (new_modem) {
We usually prefer this way:
if (new_modem == NULL)
goto done;
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
_______________________________________________
connman mailing list
[email protected]
http://lists.connman.net/listinfo/connman
