Re: [PATCH_v2 3/6] cdma-connman: Rely on roaming and cdma-netreg status to enable data call

[Denis Kenzior <denkenz@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5493 bytes --]

Hi Guillaume,

On 11/02/2011 05:37 AM, Guillaume Zajac wrote:
> ---
>  src/cdma-connman.c |   65 +++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 files changed, 62 insertions(+), 3 deletions(-)
> 
> diff --git a/src/cdma-connman.c b/src/cdma-connman.c
> index db8f6c5..42dcca2 100644
> --- a/src/cdma-connman.c
> +++ b/src/cdma-connman.c
> @@ -52,8 +52,11 @@ struct cdma_connman_settings {
>  struct ofono_cdma_connman {
>  	ofono_bool_t powered;
>  	ofono_bool_t dormant;
> +	ofono_bool_t roaming_allowed;
>  	struct cdma_connman_settings *settings;
>  	DBusMessage *pending;
> +	struct ofono_cdma_netreg *cdma_netreg;
> +	unsigned int cdma_netreg_watch;
>  	const struct ofono_cdma_connman_driver *driver;
>  	void *driver_data;
>  	struct ofono_atom *atom;
> @@ -338,6 +341,17 @@ static void cdma_connman_settings_append_properties(
>  	dbus_message_iter_close_container(dict, &entry);
>  }
>  
> +ofono_bool_t cdma_connman_netreg_update(struct ofono_cdma_connman *cm)
> +{

I think this function should be named a bit better.  Wouldn't
is_registered be clearer?

> +	ofono_bool_t registered;
> +	int status  = ofono_cdma_netreg_get_status(cm->cdma_netreg);
> +
> +	registered = status == NETWORK_REGISTRATION_STATUS_REGISTERED;
> +
> +	return registered || (cm->roaming_allowed &&
> +			status == NETWORK_REGISTRATION_STATUS_ROAMING);

You seem to be doing quite a bit of work to accomplish your goal.  If
you use __ofono_modem_find_atom then you can drop all the atom watch
bits and make this implementation way easier.

> +}
> +
>  static DBusMessage *cdma_connman_get_properties(DBusConnection *conn,
>  						DBusMessage *msg, void *data)
>  {
> @@ -365,6 +379,10 @@ static DBusMessage *cdma_connman_get_properties(DBusConnection *conn,
>  	value = cm->dormant;
>  	ofono_dbus_dict_append(&dict, "Dormant", DBUS_TYPE_BOOLEAN, &value);
>  
> +	value = cm->roaming_allowed;
> +	ofono_dbus_dict_append(&dict, "RoamingAllowed",
> +				DBUS_TYPE_BOOLEAN, &value);
> +
>  	if (cm->settings)
>  		cdma_connman_settings_append_properties(cm, &dict);
>  
> @@ -450,7 +468,23 @@ static DBusMessage *cdma_connman_set_property(DBusConnection *conn,
>  
>  	dbus_message_iter_recurse(&iter, &var);
>  
> -	if (!strcmp(property, "Powered")) {
> +	if (!strcmp(property, "RoamingAllowed")) {
> +		if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_BOOLEAN)
> +			return __ofono_error_invalid_args(msg);
> +
> +		dbus_message_iter_get_basic(&var, &value);
> +
> +		if (cm->roaming_allowed == (ofono_bool_t) value)
> +			return dbus_message_new_method_return(msg);
> +
> +		cm->roaming_allowed = value;
> +
> +		if (cdma_connman_netreg_update(cm) == FALSE &&
> +				cm->powered == TRUE)
> +			cm->driver->deactivate(cm, deactivate_callback, cm);
> +

Can you please put the RoamingAllowed details in a separate patch
(preferably among the last in the series), this one might need to be
thought about some more.

> +		return NULL;
> +	} if (!strcmp(property, "Powered")) {
>  		if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_BOOLEAN)
>  			return __ofono_error_invalid_args(msg);
>  
> @@ -463,9 +497,11 @@ static DBusMessage *cdma_connman_set_property(DBusConnection *conn,
>  				cm->driver->deactivate == NULL)
>  			return __ofono_error_not_implemented(msg);
>  
> +		if (cdma_connman_netreg_update(cm) == FALSE)
> +			return __ofono_error_not_registered(msg);
> +
>  		cm->pending = dbus_message_ref(msg);
>  
> -		/* TODO: add logic to support CDMA Network Registration */
>  		if (value)
>  			cm->driver->activate(cm, cm->username, cm->password,
>  						activate_callback, cm);
> @@ -529,11 +565,18 @@ void ofono_cdma_connman_driver_unregister(
>  static void cdma_connman_unregister(struct ofono_atom *atom)
>  {
>  	DBusConnection *conn = ofono_dbus_get_connection();
> +	struct ofono_cdma_connman *cm = __ofono_atom_get_data(atom);
>  	struct ofono_modem *modem = __ofono_atom_get_modem(atom);
>  	const char *path = __ofono_atom_get_path(atom);
>  
>  	DBG("");
>  
> +	if (cm->cdma_netreg_watch) {
> +		__ofono_modem_remove_atom_watch(modem, cm->cdma_netreg_watch);
> +		cm->cdma_netreg_watch = 0;
> +		cm->cdma_netreg = NULL;
> +	}
> +
>  	g_dbus_unregister_interface(conn, path,
>  				OFONO_CDMA_CONNECTION_MANAGER_INTERFACE);
>  	ofono_modem_remove_interface(modem,
> @@ -593,6 +636,20 @@ struct ofono_cdma_connman *ofono_cdma_connman_create(
>  	return cm;
>  }
>  
> +static void cdma_netreg_watch(struct ofono_atom *atom,
> +				enum ofono_atom_watch_condition cond,
> +				void *data)
> +{
> +	struct ofono_cdma_connman *cm = data;
> +
> +	if (cond == OFONO_ATOM_WATCH_CONDITION_UNREGISTERED) {
> +		cm->cdma_netreg = NULL;
> +		return;
> +	}
> +
> +	cm->cdma_netreg = __ofono_atom_get_data(atom);
> +}
> +
>  void ofono_cdma_connman_register(struct ofono_cdma_connman *cm)
>  {
>  	DBusConnection *conn = ofono_dbus_get_connection();
> @@ -613,7 +670,9 @@ void ofono_cdma_connman_register(struct ofono_cdma_connman *cm)
>  	ofono_modem_add_interface(modem,
>  				OFONO_CDMA_CONNECTION_MANAGER_INTERFACE);
>  
> -	/* TODO: add watch to support CDMA Network Registration atom */
> +	cm->cdma_netreg_watch = __ofono_modem_add_atom_watch(modem,
> +					OFONO_ATOM_TYPE_CDMA_NETREG,
> +					cdma_netreg_watch, cm, NULL);
>  
>  	__ofono_atom_register(cm->atom, cdma_connman_unregister);
>  }

Regards,
-Denis