[PATCH 0/2] Add ResetProperties method to ConnectionContext interface

[PATCH 0/2] Add ResetProperties method to ConnectionContext interface

[Tommi Kenakkala]

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

In some cases it is useful to reset connection context settings back to ones
ofono provisioning resolved initially.
What's your opinion on the API design, implementation and use-case described
next?

Deactivation of the context is obviously mandatory. The reasoning for placing
this into ofono is handling client requests consistently and avoiding race
conditions. Using this approach ofono does not re-activate the context, that's 
done by ConnMan or whatever connection manger the system uses.

Ps. Patches on behalf of Slava whose emails didn't get through to the mailing
list.

Slava Monich (2):
  gprs: Add ResetProperties method to ConnectionContext interface
  doc: Document ResetProperties method

 doc/connman-api.txt |    9 ++
 src/gprs.c          |  292 +++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 295 insertions(+), 6 deletions(-)

-- 
1.7.9.5

[PATCH 1/2] gprs: Add ResetProperties method to ConnectionContext interface

[Tommi Kenakkala]

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

From: Slava Monich <slava.monich@jolla.com>

Allows to reset connection context properties back to default.
---
 src/gprs.c |  292 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 286 insertions(+), 6 deletions(-)

diff --git a/src/gprs.c b/src/gprs.c
index 05ab499..cd3a567 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -130,6 +130,13 @@ struct ofono_gprs_context {
 	struct ofono_atom *atom;
 };
 
+struct context_reset {
+	GSList *msgs;
+	const struct ofono_gprs_provision_data *ap;
+	struct ofono_gprs_provision_data *settings;
+	int count;
+};
+
 struct pri_context {
 	ofono_bool_t active;
 	enum ofono_gprs_context_type type;
@@ -142,6 +149,7 @@ struct pri_context {
 	char *proxy_host;
 	uint16_t proxy_port;
 	DBusMessage *pending;
+	struct context_reset *pending_reset;
 	struct ofono_gprs_primary_context context;
 	struct ofono_gprs_context *context_driver;
 	struct ofono_gprs *gprs;
@@ -149,6 +157,10 @@ struct pri_context {
 
 static void gprs_netreg_update(struct ofono_gprs *gprs);
 static void gprs_deactivate_next(struct ofono_gprs *gprs);
+static void write_context_settings(struct ofono_gprs *gprs,
+						struct pri_context *context);
+static void pri_deactivate_callback(const struct ofono_error *error,
+								void *data);
 
 static GSList *g_drivers = NULL;
 static GSList *g_context_drivers = NULL;
@@ -802,6 +814,256 @@ static void pri_update_mms_context_settings(struct pri_context *ctx)
 		pri_setproxy(settings->interface, ctx->proxy_host);
 }
 
+static gboolean pri_str_changed(const char *val, const char *newval)
+{
+	return newval ? (strcmp(val, newval) != 0) : !val[0];
+}
+
+static gboolean pri_str_update(char *val, const char *newval)
+{
+	if (newval) {
+		if (strcmp(val, newval)) {
+			strcpy(val, newval);
+			return TRUE;
+		}
+	} else {
+		if (val[0]) {
+			val[0] = 0;
+			return TRUE;
+		}
+	}
+	return FALSE;
+}
+
+static void pri_str_signal_change(struct pri_context *ctx,
+					const char *name, const char *value)
+{
+	ofono_dbus_signal_property_changed(ofono_dbus_get_connection(),
+			ctx->path, OFONO_CONNECTION_CONTEXT_INTERFACE,
+					name, DBUS_TYPE_STRING, &value);
+}
+
+static void pri_reset_context_properties(struct pri_context *ctx,
+				const struct ofono_gprs_provision_data *ap)
+{
+	struct ofono_gprs *gprs = ctx->gprs;
+	gboolean changed = FALSE;
+
+	DBG("%s", ctx->path);
+
+	if (strcmp(ctx->context.apn, ap->apn)) {
+		changed = TRUE;
+		strcpy(ctx->context.apn, ap->apn);
+		pri_str_signal_change(ctx, "AccessPointName", ap->apn);
+	}
+
+	if (ap->name && strncmp(ctx->name, ap->name, MAX_CONTEXT_NAME_LENGTH)) {
+		changed = TRUE;
+		strncpy(ctx->name, ap->name, MAX_CONTEXT_NAME_LENGTH);
+		pri_str_signal_change(ctx, "Name", ap->name);
+	}
+
+	if (pri_str_update(ctx->context.username, ap->username)) {
+		changed = TRUE;
+		pri_str_signal_change(ctx, "Username", ap->username);
+	}
+
+	if (pri_str_update(ctx->context.password, ap->password)) {
+		changed = TRUE;
+		pri_str_signal_change(ctx, "Password", ap->password);
+	}
+
+	if (ctx->context.proto != ap->proto) {
+		ctx->context.proto = ap->proto;
+		changed = TRUE;
+		pri_str_signal_change(ctx, "Protocol",
+					gprs_proto_to_string(ap->proto));
+	}
+
+	if (ap->type == OFONO_GPRS_CONTEXT_TYPE_MMS) {
+		if (pri_str_update(ctx->message_proxy, ap->message_proxy)) {
+			changed = TRUE;
+			pri_str_signal_change(ctx, "MessageProxy",
+							ap->message_proxy);
+		}
+
+		if (pri_str_update(ctx->message_center, ap->message_center)) {
+			changed = TRUE;
+			pri_str_signal_change(ctx, "MessageCenter",
+							ap->message_center);
+		}
+	}
+
+	if (gprs->settings && changed) {
+		write_context_settings(gprs, ctx);
+		storage_sync(gprs->imsi, SETTINGS_STORE, gprs->settings);
+	}
+}
+
+static gboolean ap_valid(const struct ofono_gprs_provision_data *ap)
+{
+	if (!ap->apn || strlen(ap->apn) > OFONO_GPRS_MAX_APN_LENGTH ||
+							!is_valid_apn(ap->apn))
+		return FALSE;
+
+	if (ap->username &&
+			strlen(ap->username) > OFONO_GPRS_MAX_USERNAME_LENGTH)
+		return FALSE;
+
+	if (ap->password &&
+			strlen(ap->password) > OFONO_GPRS_MAX_PASSWORD_LENGTH)
+		return FALSE;
+
+	if (ap->message_proxy &&
+			strlen(ap->message_proxy) > MAX_MESSAGE_PROXY_LENGTH)
+		return FALSE;
+
+	if (ap->message_center &&
+			strlen(ap->message_center) > MAX_MESSAGE_CENTER_LENGTH)
+		return FALSE;
+
+	return TRUE;
+}
+
+static gboolean pri_deactivation_required(struct pri_context *ctx,
+				const struct ofono_gprs_provision_data *ap)
+{
+	if (ctx->context.proto != ap->proto)
+		return TRUE;
+
+	if (strcmp(ctx->context.apn, ap->apn))
+		return TRUE;
+
+	if (pri_str_changed(ctx->context.username, ap->username))
+		return TRUE;
+
+	if (pri_str_changed(ctx->context.password, ap->password))
+		return TRUE;
+
+	if (ap->type == OFONO_GPRS_CONTEXT_TYPE_MMS) {
+		if (pri_str_changed(ctx->message_proxy, ap->message_proxy))
+			return TRUE;
+
+		if (pri_str_changed(ctx->message_center, ap->message_center))
+			return TRUE;
+	}
+
+	return FALSE;
+}
+
+static void pri_pending_reply_ok(gpointer data)
+{
+	DBusMessage *msg = data;
+	__ofono_dbus_pending_reply(&msg, dbus_message_new_method_return(msg));
+}
+
+static void pri_pending_reply_error(gpointer data)
+{
+	DBusMessage *msg = data;
+	__ofono_dbus_pending_reply(&msg, __ofono_error_failed(msg));
+}
+
+static void pri_reset_finish(struct pri_context *ctx, gboolean ok)
+{
+	struct context_reset *pr;
+
+	if (!ctx->pending_reset)
+		return;
+
+	pr = ctx->pending_reset;
+	ctx->pending_reset = NULL;
+
+	if (ok) {
+		pri_reset_context_properties(ctx, pr->ap);
+		g_slist_free_full(pr->msgs, pri_pending_reply_ok);
+	} else {
+		g_slist_free_full(pr->msgs, pri_pending_reply_error);
+	}
+
+	__ofono_gprs_provision_free_settings(pr->settings, pr->count);
+	g_free(pr);
+}
+
+static DBusMessage *pri_reset_async(struct pri_context *ctx, DBusMessage *msg,
+			struct ofono_gprs_provision_data *settings, int count,
+				const struct ofono_gprs_provision_data *ap)
+{
+	struct context_reset *pr;
+	struct ofono_gprs_context *gc = ctx->context_driver;
+
+	if (gc == NULL)
+		return __ofono_error_failed(msg);
+
+	pr = g_new0(struct context_reset, 1);
+	pr->msgs = g_slist_append(NULL, dbus_message_ref(msg));
+	pr->settings = settings;
+	pr->count = count;
+	pr->ap = ap;
+
+	ctx->pending_reset = pr;
+
+	/*
+	 * Context activation/deactivation or DeactivateAll may already
+	 * be in progress. In that case reset will complete once those
+	 * are done.
+	 */
+	if (!ctx->pending && !ctx->gprs->pending)
+		gc->driver->deactivate_primary(gc, ctx->context.cid,
+					pri_deactivate_callback, ctx);
+
+	return NULL;
+}
+
+static DBusMessage *pri_reset_properties(DBusConnection *conn,
+					DBusMessage *msg, void *data)
+{
+	struct pri_context *ctx = data;
+	struct ofono_gprs *gprs = ctx->gprs;
+	struct ofono_modem *modem = __ofono_atom_get_modem(gprs->atom);
+	struct ofono_sim *sim = __ofono_atom_find(OFONO_ATOM_TYPE_SIM, modem);
+	struct ofono_gprs_provision_data *settings;
+	DBusMessage *reply = NULL;
+	int i, count = 0;
+
+	if (sim == NULL)
+		return __ofono_error_failed(msg);
+
+	if (ctx->pending_reset) {
+		ctx->pending_reset->msgs = g_slist_append(
+			ctx->pending_reset->msgs, dbus_message_ref(msg));
+		return NULL;
+	}
+
+	if (__ofono_gprs_provision_get_settings(ofono_sim_get_mcc(sim),
+				ofono_sim_get_mnc(sim), ofono_sim_get_spn(sim),
+						 &settings, &count) == FALSE)
+		return __ofono_error_failed(msg);
+
+	for (i = 0; i < count; i++) {
+		const struct ofono_gprs_provision_data *ap = settings + i;
+		if (ap->type == ctx->type && ap_valid(ap)) {
+			if (!(ctx->active || ctx->pending) ||
+					!pri_deactivation_required(ctx, ap)) {
+				/* Can do it right away */
+				pri_reset_context_properties(ctx, ap);
+				reply =  dbus_message_new_method_return(msg);
+				break;
+			}
+
+			/* Otherwise have to wait for context to deactivate */
+			reply = pri_reset_async(ctx, msg, settings, count, ap);
+			if (!reply)
+				return NULL;
+
+			break;
+		}
+	}
+
+	__ofono_gprs_provision_free_settings(settings, count);
+
+	return reply ? reply : __ofono_error_not_available(msg);
+}
+
 static void append_context_properties(struct pri_context *ctx,
 					DBusMessageIter *dict)
 {
@@ -895,6 +1157,7 @@ static void pri_activate_callback(const struct ofono_error *error, void *data)
 					__ofono_error_failed(ctx->pending));
 		context_settings_free(ctx->context_driver->settings);
 		release_context(ctx);
+		pri_reset_finish(ctx, TRUE);
 		return;
 	}
 
@@ -917,6 +1180,11 @@ static void pri_activate_callback(const struct ofono_error *error, void *data)
 	ofono_dbus_signal_property_changed(conn, ctx->path,
 					OFONO_CONNECTION_CONTEXT_INTERFACE,
 					"Active", DBUS_TYPE_BOOLEAN, &value);
+
+	/* Immediately deactivate if reset was requested */
+	if (ctx->pending_reset)
+		gc->driver->deactivate_primary(gc, ctx->context.cid,
+					pri_deactivate_callback, ctx);
 }
 
 static void pri_deactivate_callback(const struct ofono_error *error, void *data)
@@ -928,16 +1196,20 @@ static void pri_deactivate_callback(const struct ofono_error *error, void *data)
 	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
 		DBG("Deactivating context failed with error: %s",
 				telephony_error_to_str(error));
-		__ofono_dbus_pending_reply(&ctx->pending,
+		pri_reset_finish(ctx, FALSE);
+		if (ctx->pending)
+			__ofono_dbus_pending_reply(&ctx->pending,
 					__ofono_error_failed(ctx->pending));
 		return;
 	}
 
-	__ofono_dbus_pending_reply(&ctx->pending,
+	if (ctx->pending)
+		__ofono_dbus_pending_reply(&ctx->pending,
 				dbus_message_new_method_return(ctx->pending));
 
 	pri_reset_context_settings(ctx);
 	release_context(ctx);
+	pri_reset_finish(ctx, TRUE);
 
 	value = ctx->active;
 	ofono_dbus_signal_property_changed(conn, ctx->path,
@@ -1240,7 +1512,7 @@ static DBusMessage *pri_set_property(DBusConnection *conn,
 		if (ctx->gprs->pending)
 			return __ofono_error_busy(msg);
 
-		if (ctx->pending)
+		if (ctx->pending || ctx->pending_reset)
 			return __ofono_error_busy(msg);
 
 		if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_BOOLEAN)
@@ -1358,6 +1630,8 @@ static const GDBusMethodTable context_methods[] = {
 	{ GDBUS_ASYNC_METHOD("SetProperty",
 			GDBUS_ARGS({ "property", "s" }, { "value", "v" }),
 			NULL, pri_set_property) },
+	{ GDBUS_ASYNC_METHOD("ResetProperties", NULL, NULL,
+			pri_reset_properties) },
 	{ }
 };
 
@@ -1395,6 +1669,7 @@ static void pri_context_destroy(gpointer userdata)
 {
 	struct pri_context *ctx = userdata;
 
+	pri_reset_finish(ctx, FALSE);
 	g_free(ctx->proxy_host);
 	g_free(ctx->path);
 	g_free(ctx);
@@ -1538,7 +1813,7 @@ static void release_active_contexts(struct ofono_gprs *gprs)
 			continue;
 
 		/* This context is already being messed with */
-		if (ctx->pending)
+		if (ctx->pending || ctx->pending_reset)
 			continue;
 
 		gc = ctx->context_driver;
@@ -1964,6 +2239,7 @@ static void gprs_deactivate_for_remove(const struct ofono_error *error,
 
 	pri_reset_context_settings(ctx);
 	release_context(ctx);
+	pri_reset_finish(ctx, FALSE);
 
 	value = FALSE;
 	ofono_dbus_signal_property_changed(conn, ctx->path,
@@ -2017,7 +2293,7 @@ static DBusMessage *gprs_remove_context(DBusConnection *conn,
 		struct ofono_gprs_context *gc = ctx->context_driver;
 
 		/* This context is already being messed with */
-		if (ctx->pending)
+		if (ctx->pending || ctx->pending_reset)
 			return __ofono_error_busy(msg);
 
 		gprs->pending = dbus_message_ref(msg);
@@ -2054,6 +2330,7 @@ static void gprs_deactivate_for_all(const struct ofono_error *error,
 	dbus_bool_t value;
 
 	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
+		pri_reset_finish(ctx, FALSE);
 		__ofono_dbus_pending_reply(&gprs->pending,
 					__ofono_error_failed(gprs->pending));
 		return;
@@ -2061,6 +2338,7 @@ static void gprs_deactivate_for_all(const struct ofono_error *error,
 
 	pri_reset_context_settings(ctx);
 	release_context(ctx);
+	pri_reset_finish(ctx, TRUE);
 
 	value = ctx->active;
 	conn = ofono_dbus_get_connection();
@@ -2110,7 +2388,7 @@ static DBusMessage *gprs_deactivate_all(DBusConnection *conn,
 	for (l = gprs->contexts; l; l = l->next) {
 		ctx = l->data;
 
-		if (ctx->pending)
+		if (ctx->pending || ctx->pending_reset)
 			return __ofono_error_busy(msg);
 	}
 
@@ -2296,6 +2574,7 @@ static void gprs_context_unregister(struct ofono_atom *atom)
 
 		pri_reset_context_settings(ctx);
 		release_context(ctx);
+		pri_reset_finish(ctx, FALSE);
 
 		value = FALSE;
 		ofono_dbus_signal_property_changed(conn, ctx->path,
@@ -2367,6 +2646,7 @@ void ofono_gprs_context_deactivated(struct ofono_gprs_context *gc,
 
 		pri_reset_context_settings(ctx);
 		release_context(ctx);
+		pri_reset_finish(ctx, TRUE);
 
 		value = FALSE;
 		ofono_dbus_signal_property_changed(conn, ctx->path,
-- 
1.7.9.5

[PATCH 2/2] doc: Document ResetProperties method

[Tommi Kenakkala]

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

From: Slava Monich <slava.monich@jolla.com>

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

diff --git a/doc/connman-api.txt b/doc/connman-api.txt
index 8f72087..3861c9a 100644
--- a/doc/connman-api.txt
+++ b/doc/connman-api.txt
@@ -155,6 +155,15 @@ Methods		dict GetProperties()
 					 [service].Error.AttachInProgress
 					 [service].Error.NotImplemented
 
+		void ResetProperties()
+			Resets all properties back to default.
+			Fails if there are no provisioning settings.
+			The context will be deactivated before any significant
+			property is updated.
+
+			Possible Errors: [service].Error.Failed
+					 [service].Error.NotAvailable
+
 Signals		PropertyChanged(string property, variant value)
 
 			This signal indicates a changed value of the given
-- 
1.7.9.5

Re: [PATCH 0/2] Add ResetProperties method to ConnectionContext interface

[Denis Kenzior]

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

Hi Tommi,

On 02/19/2015 05:59 AM, Tommi Kenakkala wrote:
> In some cases it is useful to reset connection context settings back to ones
> ofono provisioning resolved initially.
> What's your opinion on the API design, implementation and use-case described
> next?
>

Why don't you just remove the settings file and restart oFono?

> Deactivation of the context is obviously mandatory. The reasoning for placing
> this into ofono is handling client requests consistently and avoiding race
> conditions. Using this approach ofono does not re-activate the context, that's
> done by ConnMan or whatever connection manger the system uses.
>

I'm not so sure that this is a good idea.  A 'live' reset of the 
settings just seems wrong to me.  Why would you ever want to do that 
anyway?  If something is working, why bother resetting the settings?

oFono supports arbitrary number of contexts, so your UI can always add 
more.  So if you need to re-provision, let your UI handle this.  The UI 
should be capable of reading the provisioning database anyway in order 
to handle duplicates, etc.

Regards,
-Denis

Re: [PATCH 0/2] Add ResetProperties method to ConnectionContext interface

[Denis Kenzior]

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

Hi Tommi,

 >> Why don't you just remove the settings file and restart oFono?
> oFono handles many other services so restarting would have undesirable effects.
> For a moment clients would think there is no telephony at all: no SIM,
> no cellular signal strength, no voice/emergency calls, no SIM ATK etc.
>

Which is probably OK given that you're resetting cellular settings. 
Unless you expect the user to know / have control over very fine-grained 
details.

>>> Deactivation of the context is obviously mandatory. The reasoning for placing
>>> this into ofono is handling client requests consistently and avoiding race
>>> conditions. Using this approach ofono does not re-activate the context, that's
>>> done by ConnMan or whatever connection manger the system uses.
>>>
>> I'm not so sure that this is a good idea.  A 'live' reset of the
>> settings just seems wrong to me.  Why would you ever want to do that
>> anyway?  If something is working, why bother resetting the settings?
> Generally speaking I agree with the point of view. The main point was
> not the 'live' part, but to allow running provisioning again, e.g. if
> user has set wrong ones.

Then this has to be done with Powered = False as a prerequisite.  And 
you need some way to tell ConnMan what is happening and why.  Since 
someone still has to go in and activate the new contexts afterwards.

> FYI a MMS context may be active using a correct APN but messages don't
> flow due to a wrong proxy. In such a case context remains active for
> some time.

I don't get it, if the proxy is wrong, it must have been provisioned 
that way.  How is re-provisioning going to help?  Either way, it sounds 
like you want the UI to reset a particular context's settings, not 
everything.

>
>> oFono supports arbitrary number of contexts, so your UI can always add
>> more.  So if you need to re-provision, let your UI handle this.  The UI
>> should be capable of reading the provisioning database anyway in order
>> to handle duplicates, etc.
> When ofono already implements the initial provisioning logic it is not
> tempting to duplicate it but instead trigger it again.

Not tempting, no.  Provisioning is there to handle simple cases.  E.g. 
user receives new device, turns it on and gets connected.  If you want 
the user to mess with the settings, or if your database might contain 
ambiguous information, then you need to write a proper UI.  One that can 
read the provisioning database as well and present choices to the user.

> (A more suitable name for the new method could be  "ProvisiongContext"
> or similar.)
> If reverting provisioned settings is done by creating a new empty
> context, one would still want to run ofono provisioning for that
> (ofono restart not an option).

The problem is there's really no mapping between what the provisioning 
database returns and what the settings are right now.  You can sort of 
kludge around by comparing the context type, but you might be 
over-writing the wrong context.

The better way is to wipe all existing settings and re-populate.  But 
that would require quite a bit of ContextRemoved, ContextAdded signals 
to keep the clients consistent.

Might be easier to just re-boot the gprs atom.

Regards,
-Denis