Re: [PATCH 1/4] radio settings: allow for more than one property

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Mika,

On 10/07/2010 08:37 AM, Mika Liljeberg wrote:
> Restructure code to allow other properties besides
> TechnologyPreference to be returned.
> ---
>  src/radio-settings.c |   59 +++++++++++++++++++++++++------------------------
>  1 files changed, 30 insertions(+), 29 deletions(-)
> 
> diff --git a/src/radio-settings.c b/src/radio-settings.c
> index f70d870..5b6ef9e 100644
> --- a/src/radio-settings.c
> +++ b/src/radio-settings.c
> @@ -77,15 +77,17 @@ static int string_to_radio_access_mode(const char *mode)
>  	return -1;
>  }
>  
> +static void radio_rat_mode_query_callback(const struct ofono_error *error,
> +					enum ofono_radio_access_mode mode,
> +					void *data);
> +
>  static DBusMessage *radio_get_properties_reply(DBusMessage *msg,
> -						struct ofono_radio_settings *rs)
> +					       struct ofono_radio_settings *rs)

Please don't mix and match tabs and spaces for indentation

>  {
>  	DBusMessage *reply;
>  	DBusMessageIter iter;
>  	DBusMessageIter dict;
>  
> -	const char *mode = radio_access_mode_to_string(rs->mode);
> -
>  	reply = dbus_message_new_method_return(msg);
>  	if (!reply)
>  		return NULL;
> @@ -96,8 +98,17 @@ static DBusMessage *radio_get_properties_reply(DBusMessage *msg,
>  					OFONO_PROPERTIES_ARRAY_SIGNATURE,
>  					&dict);
>  
> -	ofono_dbus_dict_append(&dict, "TechnologyPreference", DBUS_TYPE_STRING,
> -				&mode);
> +	if (rs->flags & RADIO_SETTINGS_MODE_CACHED) {
> +		const char *mode = radio_access_mode_to_string(rs->mode);
> +		ofono_dbus_dict_append(&dict, "TechnologyPreference",
> +				DBUS_TYPE_STRING, &mode);
> +	} else if (rs->driver->query_rat_mode) {
> +		rs->pending = dbus_message_ref(msg);
> +		rs->driver->query_rat_mode(rs,
> +					radio_rat_mode_query_callback, rs);
> +		dbus_message_unref(reply);
> +		return NULL;
> +	}
>  
>  	dbus_message_iter_close_container(&iter, &dict);
>  
> @@ -107,23 +118,21 @@ static DBusMessage *radio_get_properties_reply(DBusMessage *msg,
>  static void radio_set_rat_mode(struct ofono_radio_settings *rs,
>  				enum ofono_radio_access_mode mode)
>  {
> -	DBusConnection *conn = ofono_dbus_get_connection();
> -	const char *path;
> -	const char *str_mode;
> +	if ((rs->flags & RADIO_SETTINGS_MODE_CACHED) &&
> +		rs->mode != (int)mode) {
>  
> -	if (rs->mode == (int)mode)
> -		return;
> +		DBusConnection *conn = ofono_dbus_get_connection();
> +		const char *path = __ofono_atom_get_path(rs->atom);
> +		const char *str_mode = radio_access_mode_to_string(rs->mode);
> +
> +		ofono_dbus_signal_property_changed(conn, path,
> +						OFONO_RADIO_SETTINGS_INTERFACE,
> +						"TechnologyPreference",
> +						DBUS_TYPE_STRING, &str_mode);
> +	}
>  
>  	rs->mode = mode;
>  	rs->flags |= RADIO_SETTINGS_MODE_CACHED;
> -
> -	path = __ofono_atom_get_path(rs->atom);
> -	str_mode = radio_access_mode_to_string(rs->mode);
> -
> -	ofono_dbus_signal_property_changed(conn, path,
> -					OFONO_RADIO_SETTINGS_INTERFACE,
> -					"TechnologyPreference", DBUS_TYPE_STRING,
> -					&str_mode);
>  }
>  
>  static void radio_mode_set_callback(const struct ofono_error *error, void *data)
> @@ -162,7 +171,8 @@ static void radio_rat_mode_query_callback(const struct ofono_error *error,
>  	radio_set_rat_mode(rs, mode);
>  
>  	reply = radio_get_properties_reply(rs->pending, rs);
> -	__ofono_dbus_pending_reply(&rs->pending, reply);
> +	if (reply)
> +		__ofono_dbus_pending_reply(&rs->pending, reply);

Actually I would like to avoid this.  If two applications send
GetProperties() at the same time and the properties have not been cached
yet, we should tell the second one to bugger off.  It will receive the
properties as signals in a little while anyway.

Your proposal would generate queries for each client, which can
potentially spam the modem with unneeded commands.

>  }
>  
>  static DBusMessage *radio_get_properties(DBusConnection *conn, DBusMessage *msg,
> @@ -170,19 +180,10 @@ static DBusMessage *radio_get_properties(DBusConnection *conn, DBusMessage *msg,
>  {
>  	struct ofono_radio_settings *rs = data;
>  
> -	if (rs->flags & RADIO_SETTINGS_MODE_CACHED)
> -		return radio_get_properties_reply(msg, rs);
> -
> -	if (!rs->driver->query_rat_mode)
> -		return __ofono_error_not_implemented(msg);
> -
>  	if (rs->pending)
>  		return __ofono_error_busy(msg);
>  
> -	rs->pending = dbus_message_ref(msg);
> -	rs->driver->query_rat_mode(rs, radio_rat_mode_query_callback, rs);
> -
> -	return NULL;
> +	return radio_get_properties_reply(msg, rs);

I prefer you keep the current implementation as it is consistent with
the rest of ofono core.  The way the rest of the core works is that you
query each in sequence, then set the the CACHED flag (only one is
actually required).

We use a single cached flag to keep the logic simpler and optimize for
the general case (e.g. UI will query the properties first before
allowing the user to set them)  While this does lead to us querying some
attributes unnecessarily in certain extremely unlikely corner cases, I
believe it is an acceptable compromise.

>  }
>  
>  static DBusMessage *radio_set_property(DBusConnection *conn, DBusMessage *msg,

Regards,
-Denis