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