6:46 p.m.
The ofono_lte_default_attach_info now handles also the protocol and the
authentication method, username and password.
Co-authored-by: Martin Baschin <martin.baschin(a)googlemail.com>
---
drivers/atmodem/lte.c | 92 +++++++++++++++++++++++++++++++++++++------
1 file changed, 81 insertions(+), 11 deletions(-)
diff --git a/drivers/atmodem/lte.c b/drivers/atmodem/lte.c
index efa4e5fe..e02864ea 100644
--- a/drivers/atmodem/lte.c
+++ b/drivers/atmodem/lte.c
@@ -3,6 +3,7 @@
* oFono - Open Source Telephony
*
* Copyright (C) 2017 Intel Corporation. All rights reserved.
+ * Copyright (C) 2018 Gemalto M2M
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 2 as
@@ -45,10 +46,17 @@ struct lte_driver_data {
GAtChat *chat;
};
-static void at_lte_set_default_attach_info_cb(gboolean ok, GAtResult *result,
+struct lte_callbackdata {
+ ofono_lte_cb_t cb;
+ void *data;
+ struct lte_driver_data *ldd;
+ const struct ofono_lte_default_attach_info *info;
+};
+
+static void at_lte_set_auth_cb(gboolean ok, GAtResult *result,
gpointer user_data)
{
- struct cb_data *cbd = user_data;
+ struct lte_callbackdata *cbd = user_data;
ofono_lte_cb_t cb = cbd->cb;
struct ofono_error error;
@@ -58,25 +66,89 @@ static void at_lte_set_default_attach_info_cb(gboolean ok, GAtResult
*result,
cb(&error, cbd->data);
}
+static void at_lte_set_auth(gboolean ok, GAtResult *result, gpointer user_data)
+{
+ struct lte_callbackdata *cbd = user_data;
+ ofono_lte_cb_t cb = cbd->cb;
+ void *data = cbd->data;
+ struct ofono_error error;
+ char buf[32 + OFONO_GPRS_MAX_USERNAME_LENGTH +
+ OFONO_GPRS_MAX_PASSWORD_LENGTH + 1];
+ guint buflen = sizeof(buf);
+ guint len;
+ enum ofono_gprs_auth_method auth_method;
+
+ if (!ok) {
+ g_free(cbd);
+ decode_at_error(&error, g_at_result_final_response(result));
+ cb(&error, data);
+ }
+
+ len = snprintf(buf, buflen, "AT+CGAUTH=0,");
+ buflen -= len;
+ auth_method = cbd->info->auth_method;
+
+ /* change the authentication method if the parameters are invalid */
+ if (!*cbd->info->username || !*cbd->info->password)
+ auth_method = OFONO_GPRS_AUTH_METHOD_NONE;
+
+ switch (auth_method) {
+ case OFONO_GPRS_AUTH_METHOD_PAP:
+ snprintf(buf + len, buflen, "1,\"%s\",\"%s\"",
+ cbd->info->username, cbd->info->password);
+ break;
+ case OFONO_GPRS_AUTH_METHOD_CHAP:
+ snprintf(buf + len, buflen, "2,\"%s\",\"%s\"",
+ cbd->info->username, cbd->info->password);
+ break;
+ case OFONO_GPRS_AUTH_METHOD_NONE:
+ snprintf(buf + len, buflen, "0");
+ break;
+ }
+
+ if (g_at_chat_send(cbd->ldd->chat, buf, NULL,
+ at_lte_set_auth_cb, cbd, g_free) > 0)
+ return;
+
+ CALLBACK_WITH_FAILURE(cb, data);
+}
+
static void at_lte_set_default_attach_info(const struct ofono_lte *lte,
const struct ofono_lte_default_attach_info *info,
ofono_lte_cb_t cb, void *data)
{
struct lte_driver_data *ldd = ofono_lte_get_data(lte);
char buf[32 + OFONO_GPRS_MAX_APN_LENGTH + 1];
- struct cb_data *cbd = cb_data_new(cb, data);
+ struct lte_callbackdata *cbd = g_new0(struct lte_callbackdata ,1);
+ const char *proto;
DBG("LTE config with APN: %s", info->apn);
+ cbd->cb = cb;
+ cbd->data = data;
+ cbd->ldd = ldd;
+ cbd->info = info;
+
+ switch (info->proto) {
+ case OFONO_GPRS_PROTO_IPV6:
+ proto = "IPV6";
+ break;
+ case OFONO_GPRS_PROTO_IPV4V6:
+ proto = "IPV4V6";
+ break;
+ case OFONO_GPRS_PROTO_IP:
+ proto = "IP";
+ break;
+ }
+
if (strlen(info->apn) > 0)
- snprintf(buf, sizeof(buf), "AT+CGDCONT=0,\"IP\",\"%s\"",
- info->apn);
+ snprintf(buf, sizeof(buf),
+ "AT+CGDCONT=0,\"%s\",\"%s\"", proto, info->apn);
else
- snprintf(buf, sizeof(buf), "AT+CGDCONT=0,\"IP\"");
+ snprintf(buf, sizeof(buf), "AT+CGDCONT=0,\"%s\"", proto);
- /* We can't do much in case of failure so don't check response. */
if (g_at_chat_send(ldd->chat, buf, NULL,
- at_lte_set_default_attach_info_cb, cbd, g_free) > 0)
+ at_lte_set_auth, cbd, NULL) > 0)
return;
CALLBACK_WITH_FAILURE(cb, data);
@@ -84,9 +156,7 @@ static void at_lte_set_default_attach_info(const struct ofono_lte
*lte,
static gboolean lte_delayed_register(gpointer user_data)
{
- struct ofono_lte *lte = user_data;
-
- ofono_lte_register(lte);
+ ofono_lte_register(user_data);
return FALSE;
}
--
2.17.1
7:12 p.m.
Whoa... slow down a bit!
Firstly, your patches are coming so fast and furious that I don't even
have time to review them before there's a new one... this is a new
version, right? You didn't include a version in the subject so it's not
clear whether you've just accidentally resent this.
Secondly, you lose credibility when you send patches that aren't
properly tested. If you just want feedback on poorly tested stuff, pass
--rfc to git-format-patch so it's clear that the patch is for review and
not merging.
Make sure you've tested your patches before submission: compiles
without warnings, runs, works as expected, no valgrind errors, etc.
On 12/10/18 09:46, Giacinto Cifelli wrote:
The ofono_lte_default_attach_info now handles also the protocol and
the
authentication method, username and password.
Co-authored-by: Martin Baschin <martin.baschin(a)googlemail.com>
---
drivers/atmodem/lte.c | 92 +++++++++++++++++++++++++++++++++++++------
1 file changed, 81 insertions(+), 11 deletions(-)
diff --git a/drivers/atmodem/lte.c b/drivers/atmodem/lte.c
index efa4e5fe..e02864ea 100644
--- a/drivers/atmodem/lte.c
+++ b/drivers/atmodem/lte.c
@@ -3,6 +3,7 @@
* oFono - Open Source Telephony
*
* Copyright (C) 2017 Intel Corporation. All rights reserved.
+ * Copyright (C) 2018 Gemalto M2M
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 2 as
@@ -45,10 +46,17 @@ struct lte_driver_data {
GAtChat *chat;
};
-static void at_lte_set_default_attach_info_cb(gboolean ok, GAtResult *result,
+struct lte_callbackdata {
+ ofono_lte_cb_t cb;
+ void *data;
+ struct lte_driver_data *ldd;
+ const struct ofono_lte_default_attach_info *info;
+};
+
+static void at_lte_set_auth_cb(gboolean ok, GAtResult *result,
gpointer user_data)
{
- struct cb_data *cbd = user_data;
+ struct lte_callbackdata *cbd = user_data;
The usual pattern in ofono would be to make struct lte_callbackdata the
.data member in struct cb_data.
ofono_lte_cb_t cb = cbd->cb;
struct ofono_error error;
@@ -58,25 +66,89 @@ static void at_lte_set_default_attach_info_cb(gboolean ok, GAtResult
*result,
cb(&error, cbd->data);
}
+static void at_lte_set_auth(gboolean ok, GAtResult *result, gpointer user_data)
I don't care much for the naming here... this is the callback for
set_default_attach_info so the original name was better. It's not a
problem that this callback goes on to set up the authorization info.
+{
+ struct lte_callbackdata *cbd = user_data;
+ ofono_lte_cb_t cb = cbd->cb;
+ void *data = cbd->data;
+ struct ofono_error error;
+ char buf[32 + OFONO_GPRS_MAX_USERNAME_LENGTH +
+ OFONO_GPRS_MAX_PASSWORD_LENGTH + 1];
+ guint buflen = sizeof(buf);
+ guint len;
size_t
+ enum ofono_gprs_auth_method auth_method;
+
+ if (!ok) {
+ g_free(cbd);
+ decode_at_error(&error, g_at_result_final_response(result));
+ cb(&error, data);
+ }
+
+ len = snprintf(buf, buflen, "AT+CGAUTH=0,");
+ buflen -= len;
+ auth_method = cbd->info->auth_method;
+
+ /* change the authentication method if the parameters are invalid */
+ if (!*cbd->info->username || !*cbd->info->password)
+ auth_method = OFONO_GPRS_AUTH_METHOD_NONE;
+
+ switch (auth_method) {
+ case OFONO_GPRS_AUTH_METHOD_PAP:
+ snprintf(buf + len, buflen, "1,\"%s\",\"%s\"",
+ cbd->info->username, cbd->info->password);
+ break;
+ case OFONO_GPRS_AUTH_METHOD_CHAP:
+ snprintf(buf + len, buflen, "2,\"%s\",\"%s\"",
+ cbd->info->username, cbd->info->password);
+ break;
+ case OFONO_GPRS_AUTH_METHOD_NONE:
+ snprintf(buf + len, buflen, "0");
+ break;
+ }
+
+ if (g_at_chat_send(cbd->ldd->chat, buf, NULL,
+ at_lte_set_auth_cb, cbd, g_free) > 0)
+ return;
+
+ CALLBACK_WITH_FAILURE(cb, data);
+}
+
static void at_lte_set_default_attach_info(const struct ofono_lte *lte,
const struct ofono_lte_default_attach_info *info,
ofono_lte_cb_t cb, void *data)
{
struct lte_driver_data *ldd = ofono_lte_get_data(lte);
char buf[32 + OFONO_GPRS_MAX_APN_LENGTH + 1];
- struct cb_data *cbd = cb_data_new(cb, data);
+ struct lte_callbackdata *cbd = g_new0(struct lte_callbackdata ,1);
+ const char *proto;
DBG("LTE config with APN: %s", info->apn);
+ cbd->cb = cb;
+ cbd->data = data;
+ cbd->ldd = ldd;
+ cbd->info = info;
How about:
cbd->data = lte_cbd;
where lte_cbd contains your extra parameters.
+
+ switch (info->proto) {
+ case OFONO_GPRS_PROTO_IPV6:
+ proto = "IPV6";
+ break;
+ case OFONO_GPRS_PROTO_IPV4V6:
+ proto = "IPV4V6";
+ break;
+ case OFONO_GPRS_PROTO_IP:
+ proto = "IP";
+ break;
+ }
+
if (strlen(info->apn) > 0)
- snprintf(buf, sizeof(buf),
"AT+CGDCONT=0,\"IP\",\"%s\"",
- info->apn);
+ snprintf(buf, sizeof(buf),
+ "AT+CGDCONT=0,\"%s\",\"%s\"", proto, info->apn);
else
- snprintf(buf, sizeof(buf), "AT+CGDCONT=0,\"IP\"");
+ snprintf(buf, sizeof(buf), "AT+CGDCONT=0,\"%s\"", proto);
How about this?
n = snprintf(buf, sizeof(buf), "AT+CGDCONT=0,\"%s\"", proto);
if (info->apn[0])
n += snprintf(buf+n, sizeof(buf)-n, ",\"%s\"", info->apn);
- /* We can't do much in case of failure so don't check response. */
if (g_at_chat_send(ldd->chat, buf, NULL,
- at_lte_set_default_attach_info_cb, cbd, g_free) > 0)
+ at_lte_set_auth, cbd, NULL) > 0)
If this fails, you leak cbd.
return;
CALLBACK_WITH_FAILURE(cb, data);
@@ -84,9 +156,7 @@ static void at_lte_set_default_attach_info(const struct ofono_lte
*lte,
static gboolean lte_delayed_register(gpointer user_data)
{
- struct ofono_lte *lte = user_data;
-
- ofono_lte_register(lte);
+ ofono_lte_register(user_data);
Again, NAK to this bit (I already reviewed this). This is irrelevant to
the rest of the patch.
/Jonas
return FALSE;
}
7:27 p.m.
Hi,
On Fri, Oct 12, 2018 at 10:12 AM Jonas Bonn <jonas(a)southpole.se> wrote:
Whoa... slow down a bit!
Firstly, your patches are coming so fast and furious that I don't even
have time to review them before there's a new one... this is a new
version, right? You didn't include a version in the subject so it's not
clear whether you've just accidentally resent this.
Secondly, you lose credibility when you send patches that aren't
properly tested. If you just want feedback on poorly tested stuff, pass
--rfc to git-format-patch so it's clear that the patch is for review and
not merging.
ok, i didn't know about this. I will add it on the first submission.
Make sure you've tested your patches before submission: compiles
without warnings, runs, works as expected, no valgrind errors, etc.
ok.
On 12/10/18 09:46, Giacinto Cifelli wrote:
> The ofono_lte_default_attach_info now handles also the protocol and the
> authentication method, username and password.
>
> Co-authored-by: Martin Baschin <martin.baschin(a)googlemail.com>
> ---
> drivers/atmodem/lte.c | 92 +++++++++++++++++++++++++++++++++++++------
> 1 file changed, 81 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/atmodem/lte.c b/drivers/atmodem/lte.c
> index efa4e5fe..e02864ea 100644
> --- a/drivers/atmodem/lte.c
> +++ b/drivers/atmodem/lte.c
> @@ -3,6 +3,7 @@
> * oFono - Open Source Telephony
> *
> * Copyright (C) 2017 Intel Corporation. All rights reserved.
> + * Copyright (C) 2018 Gemalto M2M
> *
> * This program is free software; you can redistribute it and/or modify
> * it under the terms of the GNU General Public License version 2 as
> @@ -45,10 +46,17 @@ struct lte_driver_data {
> GAtChat *chat;
> };
>
> -static void at_lte_set_default_attach_info_cb(gboolean ok, GAtResult *result,
> +struct lte_callbackdata {
> + ofono_lte_cb_t cb;
> + void *data;
> + struct lte_driver_data *ldd;
> + const struct ofono_lte_default_attach_info *info;
> +};
> +
> +static void at_lte_set_auth_cb(gboolean ok, GAtResult *result,
> gpointer user_data)
> {
> - struct cb_data *cbd = user_data;
> + struct lte_callbackdata *cbd = user_data;
The usual pattern in ofono would be to make struct lte_callbackdata the
.data member in struct cb_data.
> ofono_lte_cb_t cb = cbd->cb;
> struct ofono_error error;
>
> @@ -58,25 +66,89 @@ static void at_lte_set_default_attach_info_cb(gboolean ok,
GAtResult *result,
> cb(&error, cbd->data);
> }
>
> +static void at_lte_set_auth(gboolean ok, GAtResult *result, gpointer user_data)
I don't care much for the naming here... this is the callback for
set_default_attach_info so the original name was better. It's not a
problem that this callback goes on to set up the authorization info.
ok
> +{
> + struct lte_callbackdata *cbd = user_data;
> + ofono_lte_cb_t cb = cbd->cb;
> + void *data = cbd->data;
> + struct ofono_error error;
> + char buf[32 + OFONO_GPRS_MAX_USERNAME_LENGTH +
> + OFONO_GPRS_MAX_PASSWORD_LENGTH + 1];
> + guint buflen = sizeof(buf);
> + guint len;
size_t
ok, i prefer it. I user guint because it was used everywhere.
> + enum ofono_gprs_auth_method auth_method;
> +
> + if (!ok) {
> + g_free(cbd);
> + decode_at_error(&error, g_at_result_final_response(result));
> + cb(&error, data);
> + }
> +
> + len = snprintf(buf, buflen, "AT+CGAUTH=0,");
> + buflen -= len;
> + auth_method = cbd->info->auth_method;
> +
> + /* change the authentication method if the parameters are invalid */
> + if (!*cbd->info->username || !*cbd->info->password)
> + auth_method = OFONO_GPRS_AUTH_METHOD_NONE;
> +
> + switch (auth_method) {
> + case OFONO_GPRS_AUTH_METHOD_PAP:
> + snprintf(buf + len, buflen,
"1,\"%s\",\"%s\"",
> + cbd->info->username,
cbd->info->password);
> + break;
> + case OFONO_GPRS_AUTH_METHOD_CHAP:
> + snprintf(buf + len, buflen,
"2,\"%s\",\"%s\"",
> + cbd->info->username,
cbd->info->password);
> + break;
> + case OFONO_GPRS_AUTH_METHOD_NONE:
> + snprintf(buf + len, buflen, "0");
> + break;
> + }
> +
> + if (g_at_chat_send(cbd->ldd->chat, buf, NULL,
> + at_lte_set_auth_cb, cbd, g_free) > 0)
> + return;
> +
> + CALLBACK_WITH_FAILURE(cb, data);
> +}
> +
> static void at_lte_set_default_attach_info(const struct ofono_lte *lte,
> const struct ofono_lte_default_attach_info *info,
> ofono_lte_cb_t cb, void *data)
> {
> struct lte_driver_data *ldd = ofono_lte_get_data(lte);
> char buf[32 + OFONO_GPRS_MAX_APN_LENGTH + 1];
> - struct cb_data *cbd = cb_data_new(cb, data);
> + struct lte_callbackdata *cbd = g_new0(struct lte_callbackdata ,1);
> + const char *proto;
>
> DBG("LTE config with APN: %s", info->apn);
>
> + cbd->cb = cb;
> + cbd->data = data;
> + cbd->ldd = ldd;
> + cbd->info = info;
How about:
cbd->data = lte_cbd;
where lte_cbd contains your extra parameters.
ok, I will do it this way. Do you have an example from somewhere else
in the code?
I am not sure how to allocate the structure. maybe in the
lte_driver_data, so that it doesn't require free?
Or dynamically and provide also a free method?
> +
> + switch (info->proto) {
> + case OFONO_GPRS_PROTO_IPV6:
> + proto = "IPV6";
> + break;
> + case OFONO_GPRS_PROTO_IPV4V6:
> + proto = "IPV4V6";
> + break;
> + case OFONO_GPRS_PROTO_IP:
> + proto = "IP";
> + break;
> + }
> +
> if (strlen(info->apn) > 0)
> - snprintf(buf, sizeof(buf),
"AT+CGDCONT=0,\"IP\",\"%s\"",
> - info->apn);
> + snprintf(buf, sizeof(buf),
> +
"AT+CGDCONT=0,\"%s\",\"%s\"", proto, info->apn);
> else
> - snprintf(buf, sizeof(buf),
"AT+CGDCONT=0,\"IP\"");
> + snprintf(buf, sizeof(buf), "AT+CGDCONT=0,\"%s\"",
proto);
How about this?
n = snprintf(buf, sizeof(buf), "AT+CGDCONT=0,\"%s\"", proto);
if (info->apn[0])
n += snprintf(buf+n, sizeof(buf)-n, ",\"%s\"", info->apn);
ok for me. That code was already there (apart from the static
protocol), but I can change it.
>
> - /* We can't do much in case of failure so don't check response. */
> if (g_at_chat_send(ldd->chat, buf, NULL,
> - at_lte_set_default_attach_info_cb, cbd, g_free) > 0)
> + at_lte_set_auth, cbd, NULL) > 0)
If this fails, you leak cbd.
oh right, because I can't set the g_free there. I will clean it before
the return.
> return;
>
> CALLBACK_WITH_FAILURE(cb, data);
> @@ -84,9 +156,7 @@ static void at_lte_set_default_attach_info(const struct ofono_lte
*lte,
>
> static gboolean lte_delayed_register(gpointer user_data)
> {
> - struct ofono_lte *lte = user_data;
> -
> - ofono_lte_register(lte);
> + ofono_lte_register(user_data);
Again, NAK to this bit (I already reviewed this). This is irrelevant to
the rest of the patch.
apologies, I missed this. It passed all tests...
/Jonas
Giacinto
1437
days inactive
1437
days old
2 comments
2 participants
participants (2)
-
Giacinto Cifelli -
Jonas Bonn