8:10 p.m.
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