[PATCH 0/3] Add method to force re-provisioning of contexts

[PATCH 0/3] Add method to force re-provisioning of contexts

[Alfonso Sanchez-Beato]

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

This series of patches implement a way to force re-provisioning of the
ConnectionManager contexts. Although the idea is similar to a previous 
approach which was posted to the list, it is quite different as it
wipes all existing contexts and then runs the provisioning code. The
implemented method checks different conditions to make the reset safe
for clients.

Alfonso Sanchez-Beato (3):
  gprs: Add DBus method to reset contexts
  doc: Add description for ResetContexts method
  test: Add script for resetting contexts

 doc/connman-api.txt |  10 ++++
 src/gprs.c          | 144 +++++++++++++++++++++++++++++++++++++++++++---------
 test/reset-contexts |  20 ++++++++
 3 files changed, 150 insertions(+), 24 deletions(-)
 create mode 100644 test/reset-contexts

-- 
2.1.4

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

[Alfonso Sanchez-Beato]

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

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(-)

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);
 
 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) {
+		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);
+	}
+
+	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) },
 	{ }
 };
 
-- 
2.1.4

[PATCH 2/3] doc: Add description for ResetContexts method

[Alfonso Sanchez-Beato]

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

---
 doc/connman-api.txt | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/doc/connman-api.txt b/doc/connman-api.txt
index 8f72087..1ce1d61 100644
--- a/doc/connman-api.txt
+++ b/doc/connman-api.txt
@@ -60,6 +60,16 @@ Methods		dict GetProperties()
 					 [service].Error.NotFound
 					 [service].Error.Failed
 
+		void ResetContexts()
+
+			Removes all contexts and re-provisions from the APN
+			database. Contexts must all be deactivated for this
+			method to work, and the atom must not be powered.
+
+			Possible Errors: [service].Error.InProgress
+					 [service].Error.InvalidArguments
+					 [service].Error.NotAllowed
+
 Signals		PropertyChanged(string property, variant value)
 
 			This signal indicates a changed value of the given
-- 
2.1.4

[PATCH 3/3] test: Add script for resetting contexts

[Alfonso Sanchez-Beato]

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

---
 test/reset-contexts | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)
 create mode 100644 test/reset-contexts

diff --git a/test/reset-contexts b/test/reset-contexts
new file mode 100644
index 0000000..1676636
--- /dev/null
+++ b/test/reset-contexts
@@ -0,0 +1,20 @@
+#!/usr/bin/python3
+
+import dbus
+import sys
+
+bus = dbus.SystemBus()
+
+if len(sys.argv) == 2:
+	path = sys.argv[1]
+else:
+	manager = dbus.Interface(bus.get_object('org.ofono', '/'),
+			'org.ofono.Manager')
+	modems = manager.GetModems()
+	path = modems[0][0]
+
+print("Resetting contexts for SIM on modem %s..." % path)
+cm = dbus.Interface(bus.get_object('org.ofono', path),
+		'org.ofono.ConnectionManager')
+
+cm.ResetContexts()
-- 
2.1.4

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

[Denis Kenzior]

[-- 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

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

[Alfonso Sanchez-Beato]

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

Hi Denis,

On Fri, May 15, 2015 at 4:28 AM, Denis Kenzior <denkenz@gmail.com> wrote:

> 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?


I wanted to make sure that __ofono_error_busy was returned before
__ofono_error_not_allowed if one context was busy and another one was
active, for instance. Not that I feel is really important and I can change
this if the other way is preferred.


>
>
>  +       }
>> +
>> +       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
>

Thanks for the comments, I have re-submitted the patches addressing the
issues pointed out above.

Br,
Alfonso


> _______________________________________________
> ofono mailing list
> ofono(a)ofono.org
> https://lists.ofono.org/mailman/listinfo/ofono
>

[-- Attachment #2: attachment.html --]
[-- Type: text/html, Size: 12631 bytes --]

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

[Denis Kenzior]

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

On 05/18/2015 01:53 AM, Alfonso Sanchez-Beato wrote:
> Hi Denis,
>

<snip>

>         +
>         +       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?
>
>
> I wanted to make sure that __ofono_error_busy was returned before
> __ofono_error_not_allowed if one context was busy and another one was
> active, for instance. Not that I feel is really important and I can
> change this if the other way is preferred.
>

Makes sense.  However, since this is not obvious, we probably should add 
a comment as to why this is being done.  Want to submit a patch or do 
you want me to do this?

Regards,
-Denis

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

[Alfonso Sanchez-Beato]

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

On Mon, May 18, 2015 at 4:09 PM, Denis Kenzior <denkenz@gmail.com> wrote:

> On 05/18/2015 01:53 AM, Alfonso Sanchez-Beato wrote:
>
>> Hi Denis,
>>
>>
> <snip>
>
>
>          +
>>         +       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?
>>
>>
>> I wanted to make sure that __ofono_error_busy was returned before
>> __ofono_error_not_allowed if one context was busy and another one was
>> active, for instance. Not that I feel is really important and I can
>> change this if the other way is preferred.
>>
>>
> Makes sense.  However, since this is not obvious, we probably should add a
> comment as to why this is being done.  Want to submit a patch or do you
> want me to do this?
>

Submitted.

Br,
Alfonso


>
> Regards,
> -Denis
>
> _______________________________________________
> ofono mailing list
> ofono(a)ofono.org
> https://lists.ofono.org/mailman/listinfo/ofono
>

[-- Attachment #2: attachment.html --]
[-- Type: text/html, Size: 3002 bytes --]