From: Guillaume Zajac <guillaume.zajac@linux.intel.com>
To: ofono@ofono.org
Subject: Re: [PATCH 2/4] cdma-connman: add possibility to set a username and a password
Date: Wed, 20 Jul 2011 12:10:10 +0200 [thread overview]
Message-ID: <4E26A982.4090704@linux.intel.com> (raw)
In-Reply-To: <1311155307.21109.190.camel@aeonflux>
[-- 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
next prev parent reply other threads:[~2011-07-20 10:10 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-07-20 9:26 [PATCH 0/4] CDMA-connman add username and password properties Guillaume Zajac
2011-07-20 9:26 ` [PATCH 1/4] cdma-connman: add prototype to retrieve username and password Guillaume Zajac
2011-07-20 9:26 ` [PATCH 2/4] cdma-connman: add possibility to set a username and a password Guillaume Zajac
2011-07-20 9:48 ` Marcel Holtmann
2011-07-20 10:10 ` Guillaume Zajac [this message]
2011-07-20 9:26 ` [PATCH 3/4] driver cdma-connman: add credential while setting up PPP if required Guillaume Zajac
2011-07-20 9:26 ` [PATCH 4/4] test: add script to set credentials for cdma connection Guillaume Zajac
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4E26A982.4090704@linux.intel.com \
--to=guillaume.zajac@linux.intel.com \
--cc=ofono@ofono.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox