Re: [PATCH 1/3] gprs: Add DBus method to reset contexts

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Alfonso,

On 05/14/2015 11:39 AM, Alfonso Sanchez-Beato wrote:
> Add DBus method that removes the current contexts and re-provisions
> using the APN database.
> ---
>   src/gprs.c | 144 ++++++++++++++++++++++++++++++++++++++++++++++++++-----------
>   1 file changed, 120 insertions(+), 24 deletions(-)
>

Looks okay to me, just a couple of small nitpicks:

> diff --git a/src/gprs.c b/src/gprs.c
> index 05ab499..de57b14 100644
> --- a/src/gprs.c
> +++ b/src/gprs.c
> @@ -149,6 +149,8 @@ struct pri_context {
>
>   static void gprs_netreg_update(struct ofono_gprs *gprs);
>   static void gprs_deactivate_next(struct ofono_gprs *gprs);
> +static void provision_contexts(struct ofono_gprs *gprs, const char *mcc,
> +				const char *mnc, const char *spn);

Our preference is not to have forward declarations of static functions 
unless absolutely necessary (e.g. circular dependencies).  Can we move 
provision_contexts up to avoid this?  Feel free to do this in a separate 
commit if this makes things clearer.

>
>   static GSList *g_drivers = NULL;
>   static GSList *g_context_drivers = NULL;
> @@ -1885,6 +1887,38 @@ static struct pri_context *add_context(struct ofono_gprs *gprs,
>   	return context;
>   }
>
> +static void send_context_added_signal(struct ofono_gprs *gprs,
> +					struct pri_context *context,
> +					DBusConnection *conn)
> +{
> +	const char *path;
> +	DBusMessage *signal;
> +
> +	path = __ofono_atom_get_path(gprs->atom);
> +	signal = dbus_message_new_signal(path,
> +					OFONO_CONNECTION_MANAGER_INTERFACE,
> +					"ContextAdded");
> +
> +	if (signal) {

Please avoid unnecessary nesting.  It is much easier to read this code 
if it was changed to:

if (!signal)
	return;

> +		DBusMessageIter iter;
> +		DBusMessageIter dict;
> +
> +		dbus_message_iter_init_append(signal, &iter);
> +
> +		path = context->path;
> +		dbus_message_iter_append_basic(&iter, DBUS_TYPE_OBJECT_PATH,
> +						&path);
> +
> +		dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
> +					OFONO_PROPERTIES_ARRAY_SIGNATURE,
> +					&dict);
> +		append_context_properties(context, &dict);
> +		dbus_message_iter_close_container(&iter, &dict);
> +
> +		g_dbus_send_message(conn, signal);
> +	}
> +}
> +
>   static DBusMessage *gprs_add_context(DBusConnection *conn,
>   					DBusMessage *msg, void *data)
>   {
> @@ -1894,7 +1928,6 @@ static DBusMessage *gprs_add_context(DBusConnection *conn,
>   	const char *name;
>   	const char *path;
>   	enum ofono_gprs_context_type type;
> -	DBusMessage *signal;
>
>   	if (!dbus_message_get_args(msg, NULL, DBUS_TYPE_STRING, &typestr,
>   					DBUS_TYPE_INVALID))
> @@ -1916,29 +1949,7 @@ static DBusMessage *gprs_add_context(DBusConnection *conn,
>   	g_dbus_send_reply(conn, msg, DBUS_TYPE_OBJECT_PATH, &path,
>   					DBUS_TYPE_INVALID);
>
> -	path = __ofono_atom_get_path(gprs->atom);
> -	signal = dbus_message_new_signal(path,
> -					OFONO_CONNECTION_MANAGER_INTERFACE,
> -					"ContextAdded");
> -
> -	if (signal) {
> -		DBusMessageIter iter;
> -		DBusMessageIter dict;
> -
> -		dbus_message_iter_init_append(signal, &iter);
> -
> -		path = context->path;
> -		dbus_message_iter_append_basic(&iter, DBUS_TYPE_OBJECT_PATH,
> -						&path);
> -
> -		dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
> -					OFONO_PROPERTIES_ARRAY_SIGNATURE,
> -					&dict);
> -		append_context_properties(context, &dict);
> -		dbus_message_iter_close_container(&iter, &dict);
> -
> -		g_dbus_send_message(conn, signal);
> -	}
> +	send_context_added_signal(gprs, context, conn);
>
>   	return NULL;
>   }
> @@ -2173,6 +2184,89 @@ static DBusMessage *gprs_get_contexts(DBusConnection *conn,
>   	return reply;
>   }
>
> +static void remove_non_active_context(struct ofono_gprs *gprs,
> +				struct pri_context *ctx, DBusConnection *conn)
> +{
> +	char *path;
> +	const char *atompath;
> +
> +	if (gprs->settings) {
> +		g_key_file_remove_group(gprs->settings, ctx->key, NULL);
> +		storage_sync(gprs->imsi, SETTINGS_STORE, gprs->settings);
> +	}
> +
> +	/* Make a backup copy of path for signal emission below */
> +	path = g_strdup(ctx->path);
> +
> +	context_dbus_unregister(ctx);
> +	gprs->contexts = g_slist_remove(gprs->contexts, ctx);
> +
> +	atompath = __ofono_atom_get_path(gprs->atom);
> +	g_dbus_emit_signal(conn, atompath, OFONO_CONNECTION_MANAGER_INTERFACE,
> +				"ContextRemoved", DBUS_TYPE_OBJECT_PATH, &path,
> +				DBUS_TYPE_INVALID);
> +	g_free(path);
> +}
> +
> +static DBusMessage *gprs_reset_contexts(DBusConnection *conn,
> +					DBusMessage *msg, void *data)
> +{
> +	struct ofono_gprs *gprs = data;
> +	struct ofono_modem *modem = __ofono_atom_get_modem(gprs->atom);
> +	struct ofono_sim *sim = __ofono_atom_find(OFONO_ATOM_TYPE_SIM, modem);
> +	DBusMessage *reply;
> +	GSList *l;
> +
> +	if (gprs->pending)
> +		return __ofono_error_busy(msg);
> +
> +	for (l = gprs->contexts; l; l = l->next) {
> +		struct pri_context *ctx = l->data;
> +
> +		if (ctx->pending)
> +			return __ofono_error_busy(msg);
> +	}
> +
> +	if (!dbus_message_get_args(msg, NULL, DBUS_TYPE_INVALID))
> +		return __ofono_error_invalid_args(msg);
> +
> +	if (gprs->powered)
> +		return __ofono_error_not_allowed(msg);
> +
> +	for (l = gprs->contexts; l; l = l->next) {
> +		struct pri_context *ctx = l->data;
> +
> +		if (ctx->active)
> +			return __ofono_error_not_allowed(msg);

Is there a particular reason you're running through the list of contexts 
again here?  Why not check for ctx->active at the same time as ctx->pending?

> +	}
> +
> +	reply = dbus_message_new_method_return(msg);
> +	if (reply == NULL)
> +		return NULL;
> +
> +	/* Remove first the current contexts, re-provision after */
> +
> +	while (gprs->contexts != NULL) {
> +		struct pri_context *ctx = gprs->contexts->data;
> +		remove_non_active_context(gprs, ctx, conn);
> +	}
> +
> +	gprs->last_context_id = 0;
> +
> +	provision_contexts(gprs, ofono_sim_get_mcc(sim),
> +				ofono_sim_get_mnc(sim), ofono_sim_get_spn(sim));
> +
> +	if (gprs->contexts == NULL) /* Automatic provisioning failed */
> +		add_context(gprs, NULL, OFONO_GPRS_CONTEXT_TYPE_INTERNET);
> +
> +	for (l = gprs->contexts; l; l = l->next) {
> +		struct pri_context *ctx = l->data;
> +		send_context_added_signal(gprs, ctx, conn);
> +	}
> +
> +	return reply;
> +}
> +
>   static const GDBusMethodTable manager_methods[] = {
>   	{ GDBUS_METHOD("GetProperties",
>   			NULL, GDBUS_ARGS({ "properties", "a{sv}" }),
> @@ -2192,6 +2286,8 @@ static const GDBusMethodTable manager_methods[] = {
>   	{ GDBUS_METHOD("GetContexts", NULL,
>   			GDBUS_ARGS({ "contexts_with_properties", "a(oa{sv})" }),
>   			gprs_get_contexts) },
> +	{ GDBUS_ASYNC_METHOD("ResetContexts", NULL, NULL,
> +			gprs_reset_contexts) },
>   	{ }
>   };
>
>

Regards,
-Denis