[PATCH 0/4] CDMA-connman add username and password properties

[PATCH 0/4] CDMA-connman add username and password properties

[Guillaume Zajac]

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

Hi,

These patches will be used to set PPP credentials when CDMA network will need them.
It contains a test script based on create-internet-context script.

Guillaume Zajac (4):
  cdma-connman: add prototype to retrieve username and password
  cdma-connman: add possibility to set a username and a password
  driver cdma-connman: add credential while setting up PPP if required
  test: add script to set credentials for cdma connection

 drivers/cdmamodem/connman.c  |    8 +++++++
 include/cdma-connman.h       |    4 +++
 src/cdma-connman.c           |   44 +++++++++++++++++++++++++++++++++++++++++-
 test/create-cdma-credentials |   29 +++++++++++++++++++++++++++
 4 files changed, 84 insertions(+), 1 deletions(-)
 create mode 100755 test/create-cdma-credentials

[PATCH 1/4] cdma-connman: add prototype to retrieve username and password

[Guillaume Zajac]

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

---
 include/cdma-connman.h |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/include/cdma-connman.h b/include/cdma-connman.h
index 22252e0..1439ea9 100644
--- a/include/cdma-connman.h
+++ b/include/cdma-connman.h
@@ -72,6 +72,10 @@ void ofono_cdma_connman_set_data(struct ofono_cdma_connman *cm,
 						void *data);
 void *ofono_cdma_connman_get_data(struct ofono_cdma_connman *cm);
 
+const char *ofono_cdma_connman_get_username(struct ofono_cdma_connman *cm);
+
+const char *ofono_cdma_connman_get_password(struct ofono_cdma_connman *cm);
+
 #ifdef __cplusplus
 }
 #endif
-- 
1.7.1

[PATCH 2/4] cdma-connman: add possibility to set a username and a password

[Guillaume Zajac]

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

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

diff --git a/src/cdma-connman.c b/src/cdma-connman.c
index 3321b87..7ca5a0a 100644
--- a/src/cdma-connman.c
+++ b/src/cdma-connman.c
@@ -57,6 +57,8 @@ struct ofono_cdma_connman {
 	const struct ofono_cdma_connman_driver *driver;
 	void *driver_data;
 	struct ofono_atom *atom;
+	char *username;
+	char *password;
 };
 
 static void cdma_connman_settings_free(struct cdma_connman_settings *settings)
@@ -379,6 +381,8 @@ static DBusMessage *cdma_connman_set_property(DBusConnection *conn,
 	DBusMessageIter var;
 	const char *property;
 	dbus_bool_t value;
+	const char *username;
+	const char *password;
 
 	DBG("");
 
@@ -399,7 +403,7 @@ static DBusMessage *cdma_connman_set_property(DBusConnection *conn,
 
 	dbus_message_iter_recurse(&iter, &var);
 
-	if (!strcmp(property, "Powered")) {
+	if (!g_strcmp0(property, "Powered")) {
 		if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_BOOLEAN)
 			return __ofono_error_invalid_args(msg);
 
@@ -421,6 +425,22 @@ static DBusMessage *cdma_connman_set_property(DBusConnection *conn,
 			cm->driver->deactivate(cm, deactivate_callback, cm);
 
 		return dbus_message_new_method_return(msg);
+	} else if (!g_strcmp0(property, "Username")) {
+		if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_STRING)
+			return __ofono_error_invalid_args(msg);
+
+		dbus_message_iter_get_basic(&var, &username);
+		cm->username = g_strdup(username);
+		DBG("username: %s", username);
+		return dbus_message_new_method_return(msg);
+	} else if (!g_strcmp0(property, "Password")) {
+		if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_STRING)
+			return __ofono_error_invalid_args(msg);
+
+		dbus_message_iter_get_basic(&var, &password);
+		cm->password = g_strdup(password);
+		DBG("password: %s", password);
+		return dbus_message_new_method_return(msg);
 	}
 
 	/* TODO: Dormant property. Not yet supported. */
@@ -488,6 +508,12 @@ static void cdma_connman_remove(struct ofono_atom *atom)
 	if (cm->driver && cm->driver->remove)
 		cm->driver->remove(cm);
 
+	if (cm->username)
+		g_free(cm->username);
+
+	if (cm->password)
+		g_free(cm->password);
+
 	g_free(cm);
 }
 
@@ -568,3 +594,19 @@ void *ofono_cdma_connman_get_data(struct ofono_cdma_connman *cm)
 {
 	return cm->driver_data;
 }
+
+const char *ofono_cdma_connman_get_username(struct ofono_cdma_connman *cm)
+{
+	if (cm->username)
+		return cm->username;
+
+	return NULL;
+}
+
+const char *ofono_cdma_connman_get_password(struct ofono_cdma_connman *cm)
+{
+	if (cm->password)
+		return cm->password;
+
+	return NULL;
+}
-- 
1.7.1

[PATCH 3/4] driver cdma-connman: add credential while setting up PPP if required

[Guillaume Zajac]

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

---
 drivers/cdmamodem/connman.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/cdmamodem/connman.c b/drivers/cdmamodem/connman.c
index 14c78b1..7f6bf5a 100644
--- a/drivers/cdmamodem/connman.c
+++ b/drivers/cdmamodem/connman.c
@@ -123,9 +123,14 @@ static gboolean setup_ppp(struct ofono_cdma_connman *cm)
 {
 	struct connman_data *cd = ofono_cdma_connman_get_data(cm);
 	GAtIO *io;
+	const char *username;
+	const char *password;
 
 	DBG("");
 
+	username = ofono_cdma_connman_get_username(cm);
+	password = ofono_cdma_connman_get_password(cm);
+
 	io = g_at_chat_get_io(cd->chat);
 
 	g_at_chat_suspend(cd->chat);
@@ -145,6 +150,9 @@ static gboolean setup_ppp(struct ofono_cdma_connman *cm)
 	g_at_ppp_set_connect_function(cd->ppp, ppp_connect, cm);
 	g_at_ppp_set_disconnect_function(cd->ppp, ppp_disconnect, cm);
 
+	if (username && password)
+		g_at_ppp_set_credentials(cd->ppp, username, password);
+
 	/* open the ppp connection */
 	g_at_ppp_open(cd->ppp, io);
 
-- 
1.7.1

[PATCH 4/4] test: add script to set credentials for cdma connection

[Guillaume Zajac]

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

---
 test/create-cdma-credentials |   29 +++++++++++++++++++++++++++++
 1 files changed, 29 insertions(+), 0 deletions(-)
 create mode 100755 test/create-cdma-credentials

diff --git a/test/create-cdma-credentials b/test/create-cdma-credentials
new file mode 100755
index 0000000..0f52a98
--- /dev/null
+++ b/test/create-cdma-credentials
@@ -0,0 +1,29 @@
+#!/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')
+
+	print "Connecting CDMA Packet Data Service on modem %s..." % path
+
+	if len(sys.argv) > 1:
+		cm.SetProperty("Username", (sys.argv[1]))
+		print "Setting Username to %s" % (sys.argv[1])
+
+	if len(sys.argv) > 2:
+		cm.SetProperty("Password", (sys.argv[2]))
+		print "Setting Password to %s" % (sys.argv[2])
+
-- 
1.7.1

Re: [PATCH 2/4] cdma-connman: add possibility to set a username and a password

[Marcel Holtmann]

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

Hi Guillaume,

>  src/cdma-connman.c |   44 +++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 43 insertions(+), 1 deletions(-)
> 
> diff --git a/src/cdma-connman.c b/src/cdma-connman.c
> index 3321b87..7ca5a0a 100644
> --- a/src/cdma-connman.c
> +++ b/src/cdma-connman.c
> @@ -57,6 +57,8 @@ struct ofono_cdma_connman {
>  	const struct ofono_cdma_connman_driver *driver;
>  	void *driver_data;
>  	struct ofono_atom *atom;
> +	char *username;
> +	char *password;
>  };

I am still not convinced that this is needed. Please provide logs where
it shows that it fails otherwise.
 
>  static void cdma_connman_settings_free(struct cdma_connman_settings *settings)
> @@ -379,6 +381,8 @@ static DBusMessage *cdma_connman_set_property(DBusConnection *conn,
>  	DBusMessageIter var;
>  	const char *property;
>  	dbus_bool_t value;
> +	const char *username;
> +	const char *password;
>  
>  	DBG("");
>  
> @@ -399,7 +403,7 @@ static DBusMessage *cdma_connman_set_property(DBusConnection *conn,
>  
>  	dbus_message_iter_recurse(&iter, &var);
>  
> -	if (!strcmp(property, "Powered")) {
> +	if (!g_strcmp0(property, "Powered")) {

Don't bother here. strcmp is just fine since we know both arguments are
not NULL.

Also if you wanna change this, then please have a separate patch for it.
Intermixing of code with style changes is not a good idea. And we have
always fixed style issues separately.

>  		if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_BOOLEAN)
>  			return __ofono_error_invalid_args(msg);
>  
> @@ -421,6 +425,22 @@ static DBusMessage *cdma_connman_set_property(DBusConnection *conn,
>  			cm->driver->deactivate(cm, deactivate_callback, cm);
>  
>  		return dbus_message_new_method_return(msg);
> +	} else if (!g_strcmp0(property, "Username")) {
> +		if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_STRING)
> +			return __ofono_error_invalid_args(msg);
> +
> +		dbus_message_iter_get_basic(&var, &username);
> +		cm->username = g_strdup(username);
> +		DBG("username: %s", username);
> +		return dbus_message_new_method_return(msg);
> +	} else if (!g_strcmp0(property, "Password")) {
> +		if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_STRING)
> +			return __ofono_error_invalid_args(msg);
> +
> +		dbus_message_iter_get_basic(&var, &password);
> +		cm->password = g_strdup(password);
> +		DBG("password: %s", password);
> +		return dbus_message_new_method_return(msg);
>  	}
>  
>  	/* TODO: Dormant property. Not yet supported. */
> @@ -488,6 +508,12 @@ static void cdma_connman_remove(struct ofono_atom *atom)
>  	if (cm->driver && cm->driver->remove)
>  		cm->driver->remove(cm);
>  
> +	if (cm->username)
> +		g_free(cm->username);
> +
> +	if (cm->password)
> +		g_free(cm->password);
> +

The if check are pointless. g_free (and also free for that matter) does
a NULL check on its parameter.

> @@ -568,3 +594,19 @@ void *ofono_cdma_connman_get_data(struct ofono_cdma_connman *cm)
>  {
>  	return cm->driver_data;
>  }
> +
> +const char *ofono_cdma_connman_get_username(struct ofono_cdma_connman *cm)
> +{
> +	if (cm->username)
> +		return cm->username;
> +
> +	return NULL;
> +}
> +
> +const char *ofono_cdma_connman_get_password(struct ofono_cdma_connman *cm)
> +{
> +	if (cm->password)
> +		return cm->password;
> +
> +	return NULL;
> +}

What is wrong with just "return cm->password;". This example is a bit
convoluted right now ;)

The next question is where you want the NULL handling. Or do you want it
all. Please think about that a little bit and maybe have a look at what
GPRS is doing.

Regards

Marcel

Re: [PATCH 2/4] cdma-connman: add possibility to set a username and a password

[Guillaume Zajac]

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

Hi Marcel,

On 20/07/2011 11:48, Marcel Holtmann wrote:
> Hi Guillaume,
>
>>   src/cdma-connman.c |   44 +++++++++++++++++++++++++++++++++++++++++++-
>>   1 files changed, 43 insertions(+), 1 deletions(-)
>>
>> diff --git a/src/cdma-connman.c b/src/cdma-connman.c
>> index 3321b87..7ca5a0a 100644
>> --- a/src/cdma-connman.c
>> +++ b/src/cdma-connman.c
>> @@ -57,6 +57,8 @@ struct ofono_cdma_connman {
>>   	const struct ofono_cdma_connman_driver *driver;
>>   	void *driver_data;
>>   	struct ofono_atom *atom;
>> +	char *username;
>> +	char *password;
>>   };
> I am still not convinced that this is needed. Please provide logs where
> it shows that it fails otherwise.

You will find attached an ofono log giving the result in harcoding,
g_at_ppp_set_credentials(cd->ppp, "", ""); before launching PPP session.


>
>>   static void cdma_connman_settings_free(struct cdma_connman_settings *settings)
>> @@ -379,6 +381,8 @@ static DBusMessage *cdma_connman_set_property(DBusConnection *conn,
>>   	DBusMessageIter var;
>>   	const char *property;
>>   	dbus_bool_t value;
>> +	const char *username;
>> +	const char *password;
>>
>>   	DBG("");
>>
>> @@ -399,7 +403,7 @@ static DBusMessage *cdma_connman_set_property(DBusConnection *conn,
>>
>>   	dbus_message_iter_recurse(&iter,&var);
>>
>> -	if (!strcmp(property, "Powered")) {
>> +	if (!g_strcmp0(property, "Powered")) {
> Don't bother here. strcmp is just fine since we know both arguments are
> not NULL.
>
> Also if you wanna change this, then please have a separate patch for it.
> Intermixing of code with style changes is not a good idea. And we have
> always fixed style issues separately.

Ok, I will keep strcmp() fro username and password also, and I will 
commit g_strcmp0() later if needed.

>>   		if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_BOOLEAN)
>>   			return __ofono_error_invalid_args(msg);
>>
>> @@ -421,6 +425,22 @@ static DBusMessage *cdma_connman_set_property(DBusConnection *conn,
>>   			cm->driver->deactivate(cm, deactivate_callback, cm);
>>
>>   		return dbus_message_new_method_return(msg);
>> +	} else if (!g_strcmp0(property, "Username")) {
>> +		if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_STRING)
>> +			return __ofono_error_invalid_args(msg);
>> +
>> +		dbus_message_iter_get_basic(&var,&username);
>> +		cm->username = g_strdup(username);
>> +		DBG("username: %s", username);
>> +		return dbus_message_new_method_return(msg);
>> +	} else if (!g_strcmp0(property, "Password")) {
>> +		if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_STRING)
>> +			return __ofono_error_invalid_args(msg);
>> +
>> +		dbus_message_iter_get_basic(&var,&password);
>> +		cm->password = g_strdup(password);
>> +		DBG("password: %s", password);
>> +		return dbus_message_new_method_return(msg);
>>   	}
>>
>>   	/* TODO: Dormant property. Not yet supported. */
>> @@ -488,6 +508,12 @@ static void cdma_connman_remove(struct ofono_atom *atom)
>>   	if (cm->driver&&  cm->driver->remove)
>>   		cm->driver->remove(cm);
>>
>> +	if (cm->username)
>> +		g_free(cm->username);
>> +
>> +	if (cm->password)
>> +		g_free(cm->password);
>> +
> The if check are pointless. g_free (and also free for that matter) does
> a NULL check on its parameter.
>

Right, I forgot about this null check...

>> @@ -568,3 +594,19 @@ void *ofono_cdma_connman_get_data(struct ofono_cdma_connman *cm)
>>   {
>>   	return cm->driver_data;
>>   }
>> +
>> +const char *ofono_cdma_connman_get_username(struct ofono_cdma_connman *cm)
>> +{
>> +	if (cm->username)
>> +		return cm->username;
>> +
>> +	return NULL;
>> +}
>> +
>> +const char *ofono_cdma_connman_get_password(struct ofono_cdma_connman *cm)
>> +{
>> +	if (cm->password)
>> +		return cm->password;
>> +
>> +	return NULL;
>> +}
> What is wrong with just "return cm->password;". This example is a bit
> convoluted right now ;)
>
> The next question is where you want the NULL handling. Or do you want it
> all. Please think about that a little bit and maybe have a look at what
> GPRS is doing.
>

Ok

Kind regards,
Guillaume

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: ofono-cdma.log --]
[-- Type: text/x-log, Size: 2375 bytes --]

ofonod[8735]: plugins/speedupcdma.c:speedupcdma_enable() path is: /dev/ttyUSB0
ofonod[8735]: Modem: > ATE0 +CMEE=1\r
ofonod[8735]: plugins/bluetooth.c:manager_properties_cb() 
ofonod[8735]: plugins/bluetooth.c:parse_adapters() 
ofonod[8735]: plugins/bluetooth.c:parse_adapters() Calling GetProperties on /org/bluez/1461/hci0
ofonod[8735]: plugins/bluetooth.c:adapter_properties_cb() 
ofonod[8735]: plugins/bluetooth.c:parse_devices() 
ofonod[8735]: plugins/bluetooth.c:adapter_properties_cb() Adapter Address: 5C:AC:4C:FF:0C:05, Path: /org/bluez/1461/hci0
ofonod[8735]: Modem: < \r\nOK\r\n
ofonod[8735]: Modem: > AT+CFUN=1\r
ofonod[8735]: Modem: < \r\nOK\r\n
ofonod[8735]: plugins/speedupcdma.c:cfun_enable() 
ofonod[8735]: src/modem.c:modem_change_state() old state: 0, new state: 1
ofonod[8735]: plugins/speedupcdma.c:speedupcdma_pre_sim() 0x85f7170
ofonod[8735]: src/modem.c:modem_change_state() old state: 1, new state: 2
ofonod[8735]: plugins/speedupcdma.c:speedupcdma_post_sim() 0x85f7170
ofonod[8735]: src/modem.c:modem_change_state() old state: 2, new state: 3
ofonod[8735]: plugins/speedupcdma.c:speedupcdma_post_online() 0x85f7170
ofonod[8735]: src/cdma-connman.c:ofono_cdma_connman_create() 
ofonod[8735]: drivers/cdmamodem/connman.c:cdma_connman_probe() 
ofonod[8735]: Modem: > AT&C0\r
ofonod[8735]: Modem: < \r\nOK\r\n
ofonod[8735]: drivers/cdmamodem/connman.c:at_c0_cb() ok 1
ofonod[8735]: src/cdma-connman.c:ofono_cdma_connman_register() 
ofonod[8735]: Modem: > AT+GMI\r
ofonod[8735]: Modem: < \r\nManufacturer\r\n\r\nOK\r\n
ofonod[8735]: Modem: > AT+GMM\r
ofonod[8735]: Modem: < \r\nEVDO USB MODEM\r\n\r\nOK\r\n
ofonod[8735]: Modem: > AT+GMR\r
ofonod[8735]: Modem: < \r\nLCA0017.1.1_M034\r\n\r\nOK\r\n
ofonod[8735]: Modem: > AT+GSN\r
ofonod[8735]: Modem: < \r\n0x904BC325\r\n\r\nOK\r\n
ofonod[8735]: src/cdma-connman.c:cdma_connman_set_property() 
ofonod[8735]: drivers/cdmamodem/connman.c:cdma_connman_activate() 
ofonod[8735]: Modem: > ATD#777\r
ofonod[8735]: Modem: < \r\nCONNECT 3100000\r\n
ofonod[8735]: drivers/cdmamodem/connman.c:atd_cb() ok 1
ofonod[8735]: drivers/cdmamodem/connman.c:setup_ppp() 
ofonod[8735]: drivers/cdmamodem/connman.c:ppp_disconnect() 
ofonod[8735]: src/cdma-connman.c:activate_callback() 0x85f2ca8 (null)
ofonod[8735]: src/cdma-connman.c:activate_callback() Activating packet data service failed with error: Unknown error type