[PATCH 0/3] Add CDMA features to load/save/remove credentials

[PATCH 0/3] Add CDMA features to load/save/remove credentials

[Guillaume Zajac]

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

Credentials can now be saved and loaded at starting.
Credentials can now be removed using new cdma_remove-credentials script.

Guillaume Zajac (3):
  cdma-connman: add feature to load/save credentials
  cdma-connman: Add D-Bus API to remove credentials
  test: Add cdma-remove-credentials script

 Makefile.am                  |    1 +
 include/cdma-connman.h       |    3 +
 src/cdma-connman.c           |   91 ++++++++++++++++++++++++++++++++++++++++++
 src/modem.c                  |   35 ++++++++++++++++
 test/cdma-remove-credentials |   21 ++++++++++
 5 files changed, 151 insertions(+), 0 deletions(-)
 create mode 100755 test/cdma-remove-credentials

[PATCH 1/3] cdma-connman: add feature to load/save credentials

[Guillaume Zajac]

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

---
 include/cdma-connman.h |    3 ++
 src/cdma-connman.c     |   57 ++++++++++++++++++++++++++++++++++++++++++++++++
 src/modem.c            |   35 +++++++++++++++++++++++++++++
 3 files changed, 95 insertions(+), 0 deletions(-)

diff --git a/include/cdma-connman.h b/include/cdma-connman.h
index 6a1c9ff..0f8e388 100644
--- a/include/cdma-connman.h
+++ b/include/cdma-connman.h
@@ -77,6 +77,9 @@ void ofono_cdma_connman_set_data(struct ofono_cdma_connman *cm,
 						void *data);
 void *ofono_cdma_connman_get_data(struct ofono_cdma_connman *cm);
 
+void ofono_cdma_connman_load_credentials(struct ofono_cdma_connman *cm,
+					const char *serial);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/src/cdma-connman.c b/src/cdma-connman.c
index 0c9013b..3967080 100644
--- a/src/cdma-connman.c
+++ b/src/cdma-connman.c
@@ -37,6 +37,10 @@
 
 #include "ofono.h"
 #include "common.h"
+#include "storage.h"
+
+#define CREDENTIALS_STORE "cdma"
+#define CREDENTIALS_GROUP "Credentials"
 
 static GSList *g_drivers;
 
@@ -59,6 +63,8 @@ struct ofono_cdma_connman {
 	struct ofono_atom *atom;
 	char username[OFONO_CDMA_CONNMAN_MAX_USERNAME_LENGTH + 1];
 	char password[OFONO_CDMA_CONNMAN_MAX_PASSWORD_LENGTH + 1];
+	GKeyFile *credentials;
+	char *serial;
 };
 
 static void cdma_connman_settings_free(struct cdma_connman_settings *settings)
@@ -387,6 +393,12 @@ static DBusMessage *cdma_connman_set_username(struct ofono_cdma_connman *cm,
 
 	strcpy(cm->username, username);
 
+	if (cm->credentials) {
+		g_key_file_set_string(cm->credentials, CREDENTIALS_GROUP,
+					"Username", username);
+		storage_sync(cm->serial, CREDENTIALS_STORE, cm->credentials);
+	}
+
 	g_dbus_send_reply(conn, msg, DBUS_TYPE_INVALID);
 
 	path = __ofono_atom_get_path(cm->atom);
@@ -411,6 +423,12 @@ static DBusMessage *cdma_connman_set_password(struct ofono_cdma_connman *cm,
 
 	strcpy(cm->password, password);
 
+	if (cm->credentials) {
+		g_key_file_set_string(cm->credentials, CREDENTIALS_GROUP,
+					"Password", password);
+		storage_sync(cm->serial, CREDENTIALS_STORE, cm->credentials);
+	}
+
 	g_dbus_send_reply(conn, msg, DBUS_TYPE_INVALID);
 
 	path = __ofono_atom_get_path(cm->atom);
@@ -552,6 +570,8 @@ static void cdma_connman_remove(struct ofono_atom *atom)
 	if (cm->driver && cm->driver->remove)
 		cm->driver->remove(cm);
 
+	g_free(cm->serial);
+
 	g_free(cm);
 }
 
@@ -632,3 +652,40 @@ void *ofono_cdma_connman_get_data(struct ofono_cdma_connman *cm)
 {
 	return cm->driver_data;
 }
+
+void ofono_cdma_connman_load_credentials(struct ofono_cdma_connman *cm,
+					const char *serial)
+{
+	char *username = NULL;
+	char *password = NULL;
+
+	DBG("Serial %s", serial);
+
+	cm->credentials = storage_open(serial, CREDENTIALS_STORE);
+	if (cm->credentials == NULL)
+		return;
+
+	cm->serial = g_strdup(serial);
+
+	username = g_key_file_get_string(cm->credentials,
+						CREDENTIALS_GROUP,
+						"Username", NULL);
+	if (username == NULL)
+		goto error;
+
+	password = g_key_file_get_string(cm->credentials,
+						CREDENTIALS_GROUP,
+						"Password", NULL);
+	if (password == NULL)
+		goto error;
+
+	DBG("Usr %s Pwd %s", username, password);
+	strcpy(cm->username, username);
+	strcpy(cm->password, password);
+
+	return;
+
+error:
+	g_free(username);
+	g_free(password);
+}
diff --git a/src/modem.c b/src/modem.c
index 14c7a20..80a08c6 100644
--- a/src/modem.c
+++ b/src/modem.c
@@ -95,6 +95,7 @@ struct ofono_devinfo {
 	char *revision;
 	char *serial;
 	unsigned int dun_watch;
+	unsigned int cdma_watch;
 	const struct ofono_devinfo_driver *driver;
 	void *driver_data;
 	struct ofono_atom *atom;
@@ -1315,16 +1316,44 @@ void ofono_modem_remove_interface(struct ofono_modem *modem,
 	modem->interface_update = g_idle_add(trigger_interface_update, modem);
 }
 
+static void cdma_watch(struct ofono_atom *atom,
+			enum ofono_atom_watch_condition cond, void *data)
+{
+	struct ofono_devinfo *info = data;
+	struct ofono_cdma_connman *cm = __ofono_atom_get_data(atom);
+
+	if (cond == OFONO_ATOM_WATCH_CONDITION_UNREGISTERED)
+		return;
+
+	ofono_cdma_connman_load_credentials(cm, info->serial);
+}
+
 static void query_serial_cb(const struct ofono_error *error,
 				const char *serial, void *user)
 {
 	struct ofono_devinfo *info = user;
+	struct ofono_atom *cdma_atom;
+	struct ofono_cdma_connman *cm;
+	struct ofono_modem *modem;
 	DBusConnection *conn = ofono_dbus_get_connection();
 	const char *path = __ofono_atom_get_path(info->atom);
 
 	if (error->type != OFONO_ERROR_TYPE_NO_ERROR)
 		return;
 
+	cdma_atom = __ofono_modem_find_atom(__ofono_atom_get_modem(info->atom),
+						OFONO_ATOM_TYPE_CDMA_CONNMAN);
+
+	if (cdma_atom != NULL) {
+		cm = __ofono_atom_get_data(cdma_atom);
+		ofono_cdma_connman_load_credentials(cm, serial);
+	} else {
+		modem = __ofono_atom_get_modem(info->atom);
+		info->cdma_watch = __ofono_modem_add_atom_watch(modem,
+						OFONO_ATOM_TYPE_CDMA_CONNMAN,
+						cdma_watch, info, NULL);
+	}
+
 	info->serial = g_strdup(serial);
 
 	ofono_dbus_signal_property_changed(conn, path,
@@ -1578,6 +1607,7 @@ struct ofono_devinfo *ofono_devinfo_create(struct ofono_modem *modem,
 static void devinfo_unregister(struct ofono_atom *atom)
 {
 	struct ofono_devinfo *info = __ofono_atom_get_data(atom);
+	struct ofono_modem *modem = __ofono_atom_get_modem(atom);
 
 	g_free(info->manufacturer);
 	info->manufacturer = NULL;
@@ -1590,6 +1620,11 @@ static void devinfo_unregister(struct ofono_atom *atom)
 
 	g_free(info->serial);
 	info->serial = NULL;
+
+	if (info->cdma_watch) {
+		__ofono_modem_remove_atom_watch(modem, info->cdma_watch);
+		info->cdma_watch = 0;
+	}
 }
 
 void ofono_devinfo_register(struct ofono_devinfo *info)
-- 
1.7.1

Re: [PATCH 1/3] cdma-connman: add feature to load/save credentials

[Denis Kenzior]

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

Hi Guillaume,

On 08/09/2011 09:48 AM, Guillaume Zajac wrote:
> ---
>  include/cdma-connman.h |    3 ++
>  src/cdma-connman.c     |   57 ++++++++++++++++++++++++++++++++++++++++++++++++
>  src/modem.c            |   35 +++++++++++++++++++++++++++++
>  3 files changed, 95 insertions(+), 0 deletions(-)
> 
> diff --git a/include/cdma-connman.h b/include/cdma-connman.h
> index 6a1c9ff..0f8e388 100644
> --- a/include/cdma-connman.h
> +++ b/include/cdma-connman.h
> @@ -77,6 +77,9 @@ void ofono_cdma_connman_set_data(struct ofono_cdma_connman *cm,
>  						void *data);
>  void *ofono_cdma_connman_get_data(struct ofono_cdma_connman *cm);
>  
> +void ofono_cdma_connman_load_credentials(struct ofono_cdma_connman *cm,
> +					const char *serial);
> +

Stop right there, this is pretty much the wrong design.  In fact it is
completely opposite of what you want to have from an encapsulation / API
point of view.

You need to ensure that the imsi/meid are known prior to cdma-connman
going live.  That way the cdma-connman atom can lookup the relevant
information and load its settings as needed.

If that means that the modem needs to query the relevant information
prior to calling post_sim/post_online, then that is what you have to do.

>  #ifdef __cplusplus
>  }
>  #endif

<snip>

Regards,
-Denis

Re: [PATCH 1/3] cdma-connman: add feature to load/save credentials

[Guillaume Zajac]

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

Hi Denis,

On 09/08/2011 20:52, Denis Kenzior wrote:
> Hi Guillaume,
>
> On 08/09/2011 09:48 AM, Guillaume Zajac wrote:
>> ---
>>   include/cdma-connman.h |    3 ++
>>   src/cdma-connman.c     |   57 ++++++++++++++++++++++++++++++++++++++++++++++++
>>   src/modem.c            |   35 +++++++++++++++++++++++++++++
>>   3 files changed, 95 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/cdma-connman.h b/include/cdma-connman.h
>> index 6a1c9ff..0f8e388 100644
>> --- a/include/cdma-connman.h
>> +++ b/include/cdma-connman.h
>> @@ -77,6 +77,9 @@ void ofono_cdma_connman_set_data(struct ofono_cdma_connman *cm,
>>   						void *data);
>>   void *ofono_cdma_connman_get_data(struct ofono_cdma_connman *cm);
>>
>> +void ofono_cdma_connman_load_credentials(struct ofono_cdma_connman *cm,
>> +					const char *serial);
>> +
> Stop right there, this is pretty much the wrong design.  In fact it is
> completely opposite of what you want to have from an encapsulation / API
> point of view.
>
> You need to ensure that the imsi/meid are known prior to cdma-connman
> going live.  That way the cdma-connman atom can lookup the relevant
> information and load its settings as needed.
>
> If that means that the modem needs to query the relevant information
> prior to calling post_sim/post_online, then that is what you have to do.
>

What about using a __ofono_devinfo_add_serial_watch() similar to 
__ofono_netreg_add_status_watch()?
cdma-connman atom would be notified when credentials can be loaded.

Kind regards,
Guillaume

Re: [PATCH 1/3] cdma-connman: add feature to load/save credentials

[Denis Kenzior]

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

Hi Guillaume,

On 08/11/2011 08:15 AM, Guillaume Zajac wrote:
> Hi Denis,
> 
> On 09/08/2011 20:52, Denis Kenzior wrote:
>> Hi Guillaume,
>>
>> On 08/09/2011 09:48 AM, Guillaume Zajac wrote:
>>> ---
>>>   include/cdma-connman.h |    3 ++
>>>   src/cdma-connman.c     |   57
>>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>>   src/modem.c            |   35 +++++++++++++++++++++++++++++
>>>   3 files changed, 95 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/include/cdma-connman.h b/include/cdma-connman.h
>>> index 6a1c9ff..0f8e388 100644
>>> --- a/include/cdma-connman.h
>>> +++ b/include/cdma-connman.h
>>> @@ -77,6 +77,9 @@ void ofono_cdma_connman_set_data(struct
>>> ofono_cdma_connman *cm,
>>>                           void *data);
>>>   void *ofono_cdma_connman_get_data(struct ofono_cdma_connman *cm);
>>>
>>> +void ofono_cdma_connman_load_credentials(struct ofono_cdma_connman *cm,
>>> +                    const char *serial);
>>> +
>> Stop right there, this is pretty much the wrong design.  In fact it is
>> completely opposite of what you want to have from an encapsulation / API
>> point of view.
>>
>> You need to ensure that the imsi/meid are known prior to cdma-connman
>> going live.  That way the cdma-connman atom can lookup the relevant
>> information and load its settings as needed.
>>
>> If that means that the modem needs to query the relevant information
>> prior to calling post_sim/post_online, then that is what you have to do.
>>
> 
> What about using a __ofono_devinfo_add_serial_watch() similar to
> __ofono_netreg_add_status_watch()?
> cdma-connman atom would be notified when credentials can be loaded.
> 

You could, but that is really complicated.  I'd just not proceed to
post_sim until the serial number was retrieved.

Regards,
-Denis

[PATCH 2/3] cdma-connman: Add D-Bus API to remove credentials

[Guillaume Zajac]

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

---
 src/cdma-connman.c |   34 ++++++++++++++++++++++++++++++++++
 1 files changed, 34 insertions(+), 0 deletions(-)

diff --git a/src/cdma-connman.c b/src/cdma-connman.c
index 3967080..9f40104 100644
--- a/src/cdma-connman.c
+++ b/src/cdma-connman.c
@@ -510,16 +510,50 @@ static DBusMessage *cdma_connman_set_property(DBusConnection *conn,
 	return __ofono_error_invalid_args(msg);
 }
 
+static DBusMessage *cdma_connman_remove_credentials(DBusConnection *conn,
+							DBusMessage *msg,
+							void *data)
+{
+	struct ofono_cdma_connman *cm = data;
+	const char *path;
+
+	DBG("");
+
+	strcpy(cm->username, "");
+	strcpy(cm->password, "");
+
+	if (cm->powered == TRUE)
+		cm->driver->deactivate(cm, deactivate_callback, cm);
+
+	if (cm->credentials) {
+		g_key_file_remove_group(cm->credentials, CREDENTIALS_GROUP,
+					NULL);
+		storage_sync(cm->serial, CREDENTIALS_STORE, cm->credentials);
+	}
+
+	g_dbus_send_reply(conn, msg, DBUS_TYPE_INVALID);
+
+	path = __ofono_atom_get_path(cm->atom);
+	g_dbus_emit_signal(conn, path, OFONO_CONNECTION_MANAGER_INTERFACE,
+				"CredentialsRemoved", DBUS_TYPE_INVALID);
+
+	return NULL;
+}
+
 static GDBusMethodTable cdma_connman_methods[] = {
 	{ "GetProperties",	"",	"a{sv}",
 						cdma_connman_get_properties },
 	{ "SetProperty",	"sv",	"",	cdma_connman_set_property,
 						G_DBUS_METHOD_FLAG_ASYNC },
+	{ "RemoveCredentials",	"",	"",
+					cdma_connman_remove_credentials,
+						G_DBUS_METHOD_FLAG_ASYNC },
 	{ }
 };
 
 static GDBusSignalTable cdma_connman_signals[] = {
 	{ "PropertyChanged",	"sv" },
+	{ "CredentialsRemoved",	"" },
 	{ }
 };
 
-- 
1.7.1

Re: [PATCH 2/3] cdma-connman: Add D-Bus API to remove credentials

[Denis Kenzior]

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

Hi Guillaume,

On 08/09/2011 09:48 AM, Guillaume Zajac wrote:
> ---
>  src/cdma-connman.c |   34 ++++++++++++++++++++++++++++++++++
>  1 files changed, 34 insertions(+), 0 deletions(-)
> 
> diff --git a/src/cdma-connman.c b/src/cdma-connman.c
> index 3967080..9f40104 100644
> --- a/src/cdma-connman.c
> +++ b/src/cdma-connman.c
> @@ -510,16 +510,50 @@ static DBusMessage *cdma_connman_set_property(DBusConnection *conn,
>  	return __ofono_error_invalid_args(msg);
>  }
>  
> +static DBusMessage *cdma_connman_remove_credentials(DBusConnection *conn,
> +							DBusMessage *msg,
> +							void *data)
> +{
> +	struct ofono_cdma_connman *cm = data;
> +	const char *path;
> +
> +	DBG("");
> +
> +	strcpy(cm->username, "");
> +	strcpy(cm->password, "");
> +
> +	if (cm->powered == TRUE)
> +		cm->driver->deactivate(cm, deactivate_callback, cm);

You can't do it this way, you need to ensure no other operations are in
progress...

> +
> +	if (cm->credentials) {
> +		g_key_file_remove_group(cm->credentials, CREDENTIALS_GROUP,
> +					NULL);
> +		storage_sync(cm->serial, CREDENTIALS_STORE, cm->credentials);
> +	}
> +
> +	g_dbus_send_reply(conn, msg, DBUS_TYPE_INVALID);
> +
> +	path = __ofono_atom_get_path(cm->atom);
> +	g_dbus_emit_signal(conn, path, OFONO_CONNECTION_MANAGER_INTERFACE,
> +				"CredentialsRemoved", DBUS_TYPE_INVALID);
> +
> +	return NULL;
> +}
> +
>  static GDBusMethodTable cdma_connman_methods[] = {
>  	{ "GetProperties",	"",	"a{sv}",
>  						cdma_connman_get_properties },
>  	{ "SetProperty",	"sv",	"",	cdma_connman_set_property,
>  						G_DBUS_METHOD_FLAG_ASYNC },
> +	{ "RemoveCredentials",	"",	"",
> +					cdma_connman_remove_credentials,
> +						G_DBUS_METHOD_FLAG_ASYNC },
>  	{ }
>  };
>  
>  static GDBusSignalTable cdma_connman_signals[] = {
>  	{ "PropertyChanged",	"sv" },
> +	{ "CredentialsRemoved",	"" },
>  	{ }
>  };
>  

However, I'm pretty much against these changes right now.  What exactly
is your usecase here?

Regards,
-Denis

Re: [PATCH 2/3] cdma-connman: Add D-Bus API to remove credentials

[Guillaume Zajac]

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

Hi Denis,

On 09/08/2011 20:37, Denis Kenzior wrote:
> Hi Guillaume,
>
> On 08/09/2011 09:48 AM, Guillaume Zajac wrote:
>> ---
>>   src/cdma-connman.c |   34 ++++++++++++++++++++++++++++++++++
>>   1 files changed, 34 insertions(+), 0 deletions(-)
>>
>> diff --git a/src/cdma-connman.c b/src/cdma-connman.c
>> index 3967080..9f40104 100644
>> --- a/src/cdma-connman.c
>> +++ b/src/cdma-connman.c
>> @@ -510,16 +510,50 @@ static DBusMessage *cdma_connman_set_property(DBusConnection *conn,
>>   	return __ofono_error_invalid_args(msg);
>>   }
>>
>> +static DBusMessage *cdma_connman_remove_credentials(DBusConnection *conn,
>> +							DBusMessage *msg,
>> +							void *data)
>> +{
>> +	struct ofono_cdma_connman *cm = data;
>> +	const char *path;
>> +
>> +	DBG("");
>> +
>> +	strcpy(cm->username, "");
>> +	strcpy(cm->password, "");
>> +
>> +	if (cm->powered == TRUE)
>> +		cm->driver->deactivate(cm, deactivate_callback, cm);
> You can't do it this way, you need to ensure no other operations are in
> progress...
>
>> +
>> +	if (cm->credentials) {
>> +		g_key_file_remove_group(cm->credentials, CREDENTIALS_GROUP,
>> +					NULL);
>> +		storage_sync(cm->serial, CREDENTIALS_STORE, cm->credentials);
>> +	}
>> +
>> +	g_dbus_send_reply(conn, msg, DBUS_TYPE_INVALID);
>> +
>> +	path = __ofono_atom_get_path(cm->atom);
>> +	g_dbus_emit_signal(conn, path, OFONO_CONNECTION_MANAGER_INTERFACE,
>> +				"CredentialsRemoved", DBUS_TYPE_INVALID);
>> +
>> +	return NULL;
>> +}
>> +
>>   static GDBusMethodTable cdma_connman_methods[] = {
>>   	{ "GetProperties",	"",	"a{sv}",
>>   						cdma_connman_get_properties },
>>   	{ "SetProperty",	"sv",	"",	cdma_connman_set_property,
>>   						G_DBUS_METHOD_FLAG_ASYNC },
>> +	{ "RemoveCredentials",	"",	"",
>> +					cdma_connman_remove_credentials,
>> +						G_DBUS_METHOD_FLAG_ASYNC },
>>   	{ }
>>   };
>>
>>   static GDBusSignalTable cdma_connman_signals[] = {
>>   	{ "PropertyChanged",	"sv" },
>> +	{ "CredentialsRemoved",	"" },
>>   	{ }
>>   };
>>
> However, I'm pretty much against these changes right now.  What exactly
> is your usecase here?
>
>


At the beginning, I would have liked to be able to emit 
"CredentialsAdded" "CredentialsRemoved", to notify ConnMan
that the Cellular service can be published or removed like we do it for 
GSM Contexts.

Now, using "Status" == "registered" property from 
cdma.NetworkRegistration to publish the service into ConnMan might be 
cleaner.

With both solutions, we should handle into oFono the ppp_disconnect 
reason G_AT_PPP_REASON_AUTH_FAIL to try again with other credentials.

Kind regards,
Guillaume

[PATCH 3/3] test: Add cdma-remove-credentials script

[Guillaume Zajac]

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

---
 Makefile.am                  |    1 +
 test/cdma-remove-credentials |   21 +++++++++++++++++++++
 2 files changed, 22 insertions(+), 0 deletions(-)
 create mode 100755 test/cdma-remove-credentials

diff --git a/Makefile.am b/Makefile.am
index 62174db..eec08d8 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -544,6 +544,7 @@ test_scripts = test/backtrace \
 		test/cdma-dial-number \
 		test/cdma-hangup \
 		test/cdma-set-credentials \
+		test/cdma-remove-credentials \
 		test/disable-call-forwarding \
 		test/list-messages \
 		test/test-sms \
diff --git a/test/cdma-remove-credentials b/test/cdma-remove-credentials
new file mode 100755
index 0000000..28bc128
--- /dev/null
+++ b/test/cdma-remove-credentials
@@ -0,0 +1,21 @@
+#!/usr/bin/python
+
+import dbus
+import sys
+
+bus = dbus.SystemBus()
+
+manager = dbus.Interface(bus.get_object('org.ofono', '/'),
+						'org.ofono.Manager')
+
+modems = manager.GetModems()
+
+for path, properties in modems:
+	if "org.ofono.cdma.ConnectionManager" not in properties["Interfaces"]:
+		continue
+
+	cm = dbus.Interface(bus.get_object('org.ofono', path),
+					'org.ofono.cdma.ConnectionManager')
+
+	cm.RemoveCredentials()
+	print "Removing all credentials"
-- 
1.7.1