Re: [RFC PATCH 2/4] modem: add EmergencyMode property

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Andras,

On 11/15/2010 10:57 AM, Andras Domokos wrote:
> From: Andras Domokos <andras.domokos@nokia.com>
> 
> ---
>  src/modem.c |  134 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  src/ofono.h |    4 ++
>  2 files changed, 138 insertions(+), 0 deletions(-)
> 
> diff --git a/src/modem.c b/src/modem.c
> index f73cc1d..4307914 100644
> --- a/src/modem.c
> +++ b/src/modem.c
> @@ -61,6 +61,7 @@ enum modem_state {
>  struct ofono_modem {
>  	char			*path;
>  	enum modem_state	modem_state;
> +	enum modem_state	modem_state_pre_emergency;
>  	GSList			*atoms;
>  	struct ofono_watchlist	*atom_watches;
>  	GSList			*interface_list;
> @@ -72,6 +73,7 @@ struct ofono_modem {
>  	ofono_bool_t		powered_pending;
>  	guint			timeout;
>  	ofono_bool_t		online;
> +	unsigned int		emergency_mode;

I really prefer this to be called 'emergency'

>  	struct ofono_watchlist	*online_watches;
>  	GHashTable		*properties;
>  	struct ofono_sim	*sim;
> @@ -514,6 +516,127 @@ static void offline_cb(const struct ofono_error *error, void *data)
>  		modem_change_state(modem, MODEM_STATE_OFFLINE);
>  }
>  
> +ofono_bool_t ofono_modem_get_emergency_mode(struct ofono_modem *modem)
> +{
> +	if (modem == NULL)
> +		return FALSE;
> +
> +	return modem->emergency_mode != 0;
> +}
> +
> +static void modem_change_online(struct ofono_modem *modem,
> +				enum modem_state new_state)
> +{
> +	DBusConnection *conn = ofono_dbus_get_connection();
> +	enum modem_state old_state = modem->modem_state;
> +	ofono_bool_t new_online = new_state == MODEM_STATE_ONLINE;
> +
> +	DBG("old state: %d, new state: %d", old_state, new_state);
> +
> +	if (new_online == modem->online)
> +		return;
> +
> +	modem->modem_state = new_state;
> +	modem->online = new_online;
> +
> +	ofono_dbus_signal_property_changed(conn, modem->path,
> +					OFONO_MODEM_INTERFACE, "Online",
> +					DBUS_TYPE_BOOLEAN, &modem->online);
> +
> +	notify_online_watches(modem);
> +}
> +
> +static void emergency_online_cb(const struct ofono_error *error, void *data)
> +{
> +	struct ofono_modem *modem = data;
> +
> +	DBG("modem: %p", modem);
> +
> +	if (error->type == OFONO_ERROR_TYPE_NO_ERROR &&
> +			modem->modem_state != MODEM_STATE_ONLINE)
> +		modem_change_online(modem, MODEM_STATE_ONLINE);
> +}
> +
> +static void emergency_offline_cb(const struct ofono_error *error, void *data)
> +{
> +	struct ofono_modem *modem = data;
> +	DBusConnection *conn = ofono_dbus_get_connection();
> +	const char *path = ofono_modem_get_path(modem);
> +	gboolean state = FALSE;
> +
> +	DBG("modem: %p", modem);
> +
> +	if (error->type == OFONO_ERROR_TYPE_NO_ERROR &&
> +			modem->modem_state == MODEM_STATE_ONLINE)
> +		modem_change_online(modem, modem->modem_state_pre_emergency);
> +
> +	modem->emergency_mode--;
> +
> +	ofono_dbus_signal_property_changed(conn, path,
> +						OFONO_MODEM_INTERFACE,
> +						"EmergencyMode",

The property should really be called 'Emergency' to be in line with the
current API proposal (doc/modem-api.txt & TODO)

> +						DBUS_TYPE_BOOLEAN,
> +						&state);
> +}
> +
> +void ofono_modem_inc_emergency_mode(struct ofono_modem *modem)
> +{
> +	DBusConnection *conn = ofono_dbus_get_connection();
> +	const char *path = ofono_modem_get_path(modem);
> +	gboolean state = TRUE;
> +
> +	DBG("modem: %p", modem);
> +
> +	modem->emergency_mode++;
> +	if (modem->emergency_mode > 1)
> +		return;
> +
> +	ofono_dbus_signal_property_changed(conn, path,
> +						OFONO_MODEM_INTERFACE,
> +						"EmergencyMode",

Again, 'Emergency' here

> +						DBUS_TYPE_BOOLEAN,
> +						&state);
> +
> +	modem->modem_state_pre_emergency = modem->modem_state;
> +
> +	if (modem->modem_state == MODEM_STATE_ONLINE)
> +		return;
> +
> +	modem->driver->set_online(modem, TRUE, emergency_online_cb, modem);
> +}
> +
> +void ofono_modem_dec_emergency_mode(struct ofono_modem *modem)
> +{
> +	DBusConnection *conn = ofono_dbus_get_connection();
> +	const char *path = ofono_modem_get_path(modem);
> +	gboolean state = FALSE;
> +
> +	DBG("modem: %p", modem);
> +
> +	if (modem->emergency_mode == 0)
> +		return;

Can you be a bit more pedantic and send an ofono_error for this case?
We probably want to track whether some plugin abuses the reference counting.

> +
> +	if (modem->emergency_mode > 1) {
> +		modem->emergency_mode--;
> +		return;
> +	}
> +
> +	if (modem->modem_state == MODEM_STATE_ONLINE &&
> +		modem->modem_state_pre_emergency != MODEM_STATE_ONLINE) {

Please indent one more, item M4 in coding-style.txt

> +		modem->driver->set_online(modem, FALSE,
> +						emergency_offline_cb, modem);
> +		return;
> +	}
> +
> +	modem->emergency_mode--;
> +
> +	ofono_dbus_signal_property_changed(conn, path,
> +						OFONO_MODEM_INTERFACE,
> +						"EmergencyMode",

And again, 'Emergency' here.

> +						DBUS_TYPE_BOOLEAN,
> +						&state);
> +}
> +
>  static DBusMessage *set_property_online(struct ofono_modem *modem,
>  					DBusMessage *msg,
>  					DBusMessageIter *var)
> @@ -535,6 +658,9 @@ static DBusMessage *set_property_online(struct ofono_modem *modem,
>  	if (modem->modem_state < MODEM_STATE_OFFLINE)
>  		return __ofono_error_not_available(msg);
>  
> +	if (modem->emergency_mode)
> +		return __ofono_error_failed(msg);
> +

Is it worth to create a new dbus error for this case?  Perhaps
ofono_error_emergency, or emergency_active?

>  	if (modem->online == online)
>  		return dbus_message_new_method_return(msg);
>  
> @@ -562,6 +688,7 @@ void __ofono_modem_append_properties(struct ofono_modem *modem,
>  	int i;
>  	GSList *l;
>  	struct ofono_atom *devinfo_atom;
> +	ofono_bool_t state;

Please rename this variable to 'val' or 'value'

>  
>  	ofono_dbus_dict_append(dict, "Online", DBUS_TYPE_BOOLEAN,
>  				&modem->online);
> @@ -569,6 +696,10 @@ void __ofono_modem_append_properties(struct ofono_modem *modem,
>  	ofono_dbus_dict_append(dict, "Powered", DBUS_TYPE_BOOLEAN,
>  				&modem->powered);
>  
> +	state = ofono_modem_get_emergency_mode(modem);
> +	ofono_dbus_dict_append(dict, "EmergencyMode",

And 'Emergency' here

> +				DBUS_TYPE_BOOLEAN, &state);
> +
>  	devinfo_atom = __ofono_modem_find_atom(modem, OFONO_ATOM_TYPE_DEVINFO);
>  
>  	/* We cheat a little here and don't check the registered status */
> @@ -647,6 +778,9 @@ static int set_powered(struct ofono_modem *modem, ofono_bool_t powered)
>  	if (modem->powered_pending == powered)
>  		return -EALREADY;
>  
> +	if (modem->emergency_mode)

I really would prefer modem->emergency > 0 here

> +		return -EINVAL;
> +

If we introduce a new ofono error, then returning something like EPERM
or EACCESS for the case where emergency mode is active might be a good idea.

>  	/* Remove the atoms even if the driver is no longer available */
>  	if (powered == FALSE)
>  		modem_change_state(modem, MODEM_STATE_POWER_OFF);
> diff --git a/src/ofono.h b/src/ofono.h
> index 01cd6c0..ac56a85 100644
> --- a/src/ofono.h
> +++ b/src/ofono.h
> @@ -185,6 +185,10 @@ unsigned int __ofono_modem_add_online_watch(struct ofono_modem *modem,
>  void __ofono_modem_remove_online_watch(struct ofono_modem *modem,
>  		unsigned int id);
>  
> +ofono_bool_t ofono_modem_get_emergency_mode(struct ofono_modem *modem);
> +void ofono_modem_inc_emergency_mode(struct ofono_modem *modem);
> +void ofono_modem_dec_emergency_mode(struct ofono_modem *modem);
> +
>  #include <ofono/call-barring.h>
>  
>  gboolean __ofono_call_barring_is_busy(struct ofono_call_barring *cb);

Otherwise this one looks good to me.

Regards,
-Denis