12:16 a.m.
I would like to have this new version of the lte atom patch reviewed.
I have integrated all Jonas and Denis comments, except Jonas request
to change the cbd in the fuction at_lte_set_default_attach_info, which
I don't know how to do neatly in a driver. In all example I see the
allocation is done in the core, so that there is no problem for the
cleanup. If you could point out an example of structure passed as data
in the drivers, I would use the same.
This is only a partial submit, if it is ok I will send the series.
The differences in this version related to the v2 are:
- use of size_t for lengths
- naming convention for callback function
- restructuring of the cgdcont string according to Jonas wishes
- revovered memory leak in case of failure of g_at_chat_send in
the function at_lte_set_default_attach_info
- removed useless and out-of-patch change in lte_delayed_register
Giacinto Cifelli (1):
atmodem/lte: proto and authentication handling
drivers/atmodem/lte.c | 94 ++++++++++++++++++++++++++++++++++++++-----
1 file changed, 84 insertions(+), 10 deletions(-)
--
2.17.1
12:16 a.m.
New subject: [RFC PATCH v3 1/1] atmodem/lte: proto and authentication handling
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 | 94 ++++++++++++++++++++++++++++++++++++++-----
1 file changed, 84 insertions(+), 10 deletions(-)
diff --git a/drivers/atmodem/lte.c b/drivers/atmodem/lte.c
index efa4e5fe..e5af6cd2 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,27 +66,93 @@ static void at_lte_set_default_attach_info_cb(gboolean ok, GAtResult
*result,
cb(&error, cbd->data);
}
+static void at_lte_set_default_attach_info_cb(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];
+ size_t buflen = sizeof(buf);
+ size_t 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;
+ size_t len;
DBG("LTE config with APN: %s", info->apn);
- if (strlen(info->apn) > 0)
- snprintf(buf, sizeof(buf), "AT+CGDCONT=0,\"IP\",\"%s\"",
- info->apn);
- else
- snprintf(buf, sizeof(buf), "AT+CGDCONT=0,\"IP\"");
+ 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;
+ }
+
+ len = snprintf(buf, sizeof(buf), "AT+CGDCONT=0,\"%s\"", proto);
+
+ if (*info->apn)
+ snprintf(buf+len, sizeof(buf)-len, ",\"%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_default_attach_info_cb, cbd, NULL) > 0)
return;
+ g_free(cbd);
CALLBACK_WITH_FAILURE(cb, data);
}
--
2.17.1
3:14 a.m.
New subject: [RFC PATCH v3 1/1] atmodem/lte: proto and authentication handling
Hi,
This looks reasonable from my point of view. Two little comments below...
/Jonas
On 12/10/18 15:16, 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 | 94 ++++++++++++++++++++++++++++++++++++++-----
1 file changed, 84 insertions(+), 10 deletions(-)
diff --git a/drivers/atmodem/lte.c b/drivers/atmodem/lte.c
index efa4e5fe..e5af6cd2 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,27 +66,93 @@ static void at_lte_set_default_attach_info_cb(gboolean ok, GAtResult
*result,
cb(&error, cbd->data);
}
+static void at_lte_set_default_attach_info_cb(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];
+ size_t buflen = sizeof(buf);
+ size_t 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);
You'll want a return here, too.
+ }
+
+ 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;
+
Here you'll leak cbd again.
+ 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;
+ size_t len;
DBG("LTE config with APN: %s", info->apn);
- if (strlen(info->apn) > 0)
- snprintf(buf, sizeof(buf),
"AT+CGDCONT=0,\"IP\",\"%s\"",
- info->apn);
- else
- snprintf(buf, sizeof(buf), "AT+CGDCONT=0,\"IP\"");
+ 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;
+ }
+
+ len = snprintf(buf, sizeof(buf), "AT+CGDCONT=0,\"%s\"", proto);
+
+ if (*info->apn)
+ snprintf(buf+len, sizeof(buf)-len, ",\"%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_default_attach_info_cb, cbd, NULL) > 0)
return;
+ g_free(cbd);
CALLBACK_WITH_FAILURE(cb, data);
}
3:20 a.m.
New subject: [RFC PATCH v3 1/1] atmodem/lte: proto and authentication handling
Hi Jonas,
On Fri, Oct 12, 2018 at 6:14 PM Jonas Bonn <jonas(a)southpole.se> wrote:
Hi,
This looks reasonable from my point of view. Two little comments below...
excellent points. I fix them and re-submit...
I know I am hyperactive, so I will wait a little while this time
before resubmitting :)
/Jonas
Regards,
Giacinto
On 12/10/18 15:16, 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 | 94 ++++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 84 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/atmodem/lte.c b/drivers/atmodem/lte.c
> index efa4e5fe..e5af6cd2 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,27 +66,93 @@ static void at_lte_set_default_attach_info_cb(gboolean ok,
GAtResult *result,
> cb(&error, cbd->data);
> }
>
> +static void at_lte_set_default_attach_info_cb(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];
> + size_t buflen = sizeof(buf);
> + size_t 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);
You'll want a return here, too.
> + }
> +
> + 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;
> +
Here you'll leak cbd again.
> + 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;
> + size_t len;
>
> DBG("LTE config with APN: %s", info->apn);
>
> - if (strlen(info->apn) > 0)
> - snprintf(buf, sizeof(buf),
"AT+CGDCONT=0,\"IP\",\"%s\"",
> - info->apn);
> - else
> - snprintf(buf, sizeof(buf),
"AT+CGDCONT=0,\"IP\"");
> + 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;
> + }
> +
> + len = snprintf(buf, sizeof(buf), "AT+CGDCONT=0,\"%s\"",
proto);
> +
> + if (*info->apn)
> + snprintf(buf+len, sizeof(buf)-len, ",\"%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_default_attach_info_cb, cbd, NULL) > 0)
> return;
>
> + g_free(cbd);
> CALLBACK_WITH_FAILURE(cb, data);
> }
>
3:23 a.m.
New subject: [RFC PATCH v3 1/1] atmodem/lte: proto and authentication handling
actually:
> +
> + if (g_at_chat_send(cbd->ldd->chat, buf, NULL,
> + at_lte_set_auth_cb, cbd, g_free) > 0)
> + return;
> +
Here you'll leak cbd again.
here there is a g_free in the call. Also in case of failure the g_free
is executed, I think (also because it is the same construct as
everywhere else).
Or not?
3:31 a.m.
New subject: [RFC PATCH v3 1/1] atmodem/lte: proto and authentication handling
On 12/10/18 18:23, Giacinto Cifelli wrote:
actually:
>> +
>> + if (g_at_chat_send(cbd->ldd->chat, buf, NULL,
>> + at_lte_set_auth_cb, cbd, g_free) > 0)
>> + return;
>> +
> Here you'll leak cbd again.
here there is a g_free in the call. Also in case of failure the g_free
is executed, I think (also because it is the same construct as
everywhere else).
Or not?
If g_at_chat_send returns 0, g_free() won't be called so you need to
free the data yourself. That should be the construct used elsewhere so
if you see it done differently your seeing bugs (probably).
/Jonas
3:54 a.m.
New subject: [RFC PATCH v3 1/1] atmodem/lte: proto and authentication handling
Hi,
I trust you, but I have checked the code :)
you are right, the at_command_create may fail (on allocation error
from g_try_new0) and in this case it doesn't call the g_free.
However I have to say that the code leaks alot: overall the construct
is used 237 times, and only 98 times the g_free is called.
That said, according to Denis nowadays we don't use g_try_new0 because
the allocation won't fail, so we are unlikely to see it in action.
I add the g_free :D
Regards,
Giacinto
On Fri, Oct 12, 2018 at 6:31 PM Jonas Bonn <jonas(a)southpole.se> wrote:
On 12/10/18 18:23, Giacinto Cifelli wrote:
> actually:
>
>>> +
>>> + if (g_at_chat_send(cbd->ldd->chat, buf, NULL,
>>> + at_lte_set_auth_cb, cbd, g_free) > 0)
>>> + return;
>>> +
>> Here you'll leak cbd again.
> here there is a g_free in the call. Also in case of failure the g_free
> is executed, I think (also because it is the same construct as
> everywhere else).
> Or not?
If g_at_chat_send returns 0, g_free() won't be called so you need to
free the data yourself. That should be the construct used elsewhere so
if you see it done differently your seeing bugs (probably).
/Jonas
4:59 a.m.
New subject: [RFC PATCH v3 1/1] atmodem/lte: proto and authentication handling
Hi Giacinto,
On 10/12/2018 11:54 AM, Giacinto Cifelli wrote:
Hi,
I trust you, but I have checked the code :)
you are right, the at_command_create may fail (on allocation error
from g_try_new0) and in this case it doesn't call the g_free.
However I have to say that the code leaks alot: overall the construct
is used 237 times, and only 98 times the g_free is called.
Umm, can you give me some examples? Do note that we have 2 patterns
that we use:
Pattern 1 is the most common:
if (g_at_chat_send(...) > 0)
return;
g_free/destroy(...);
CALLBACK_WITH_FAILURE();
Pattern 2 is what you want:
if (g_at_chat_send(...) == 0) {
g_free/destroy(...);
CALLBACK_WITH_FAILURE();
return;
}
That said, according to Denis nowadays we don't use g_try_new0
because
the allocation won't fail, so we are unlikely to see it in action.
That is not
really an excuse, and we can change the implementation of
send() at any time. So you still need to be paranoid.
Regards,
-Denis
5:15 a.m.
New subject: [RFC PATCH v3 1/1] atmodem/lte: proto and authentication handling
I have already added it.
I have just greped through the code, here are some examples, from the
end of the output:
plugins/phonesim.c:18187: if (g_at_chat_send(data->chat, buf, none_prefix,
plugins/phonesim.c-18237- set_online_cb, cbd, g_free) > 0)
plugins/phonesim.c-18274- return;
plugins/phonesim.c-18284-
plugins/phonesim.c-18285- CALLBACK_WITH_FAILURE(cb, user_data);
gatchat/gatmux.c:19544: if (g_at_chat_send(chat, "AT+CMUX=?", cmux_prefix,
gatchat/gatmux.c-19596- mux_query_cb, msd, msd_free) > 0)
gatchat/gatmux.c-19634- return TRUE;
gatchat/gatmux.c-19649-
gatchat/gatmux.c-19650- if (msd)
gatchat/gatmux.c-19660- msd_free(msd);
// this is actually a mistake because the msd pointer won't change, freed or not
drivers/xmm7modem/netmon.c:5135: if (g_at_chat_send(nmd->chat,
"AT+XMCI=1", xmci_prefix,
drivers/xmm7modem/netmon.c-5192- xmci_cb, cbd, g_free) > 0)
drivers/xmm7modem/netmon.c-5223- return;
drivers/xmm7modem/netmon.c-5233-
drivers/xmm7modem/netmon.c-5234- CALLBACK_WITH_FAILURE(cb, data);
On Fri, Oct 12, 2018 at 7:59 PM Denis Kenzior <denkenz(a)gmail.com> wrote:
Hi Giacinto,
On 10/12/2018 11:54 AM, Giacinto Cifelli wrote:
> Hi,
>
> I trust you, but I have checked the code :)
>
> you are right, the at_command_create may fail (on allocation error
> from g_try_new0) and in this case it doesn't call the g_free.
>
> However I have to say that the code leaks alot: overall the construct
> is used 237 times, and only 98 times the g_free is called.
Umm, can you give me some examples? Do note that we have 2 patterns
that we use:
Pattern 1 is the most common:
if (g_at_chat_send(...) > 0)
return;
g_free/destroy(...);
CALLBACK_WITH_FAILURE();
Pattern 2 is what you want:
if (g_at_chat_send(...) == 0) {
g_free/destroy(...);
CALLBACK_WITH_FAILURE();
return;
}
> That said, according to Denis nowadays we don't use g_try_new0 because
> the allocation won't fail, so we are unlikely to see it in action.
That is not really an excuse, and we can change the implementation of
send() at any time. So you still need to be paranoid.
Regards,
-Denis
5:46 a.m.
New subject: [RFC PATCH v3 1/1] atmodem/lte: proto and authentication handling
Hi Giacinto,
On 10/12/2018 01:15 PM, Giacinto Cifelli wrote:
I have already added it.
I have just greped through the code, here are some examples, from the
end of the output:
plugins/phonesim.c:18187: if (g_at_chat_send(data->chat, buf, none_prefix,
plugins/phonesim.c-18237- set_online_cb, cbd, g_free) > 0)
plugins/phonesim.c-18274- return;
plugins/phonesim.c-18284-
plugins/phonesim.c-18285- CALLBACK_WITH_FAILURE(cb, user_data);
Okay, that's a bug. Fixed.
gatchat/gatmux.c:19544: if (g_at_chat_send(chat,
"AT+CMUX=?", cmux_prefix,
gatchat/gatmux.c-19596- mux_query_cb, msd, msd_free) > 0)
gatchat/gatmux.c-19634- return TRUE;
gatchat/gatmux.c-19649-
gatchat/gatmux.c-19650- if (msd)
gatchat/gatmux.c-19660- msd_free(msd);
// this is actually a mistake because the msd pointer won't change, freed or not
I don't see anything wrong here? Besides the redundant if which will
always be true.
drivers/xmm7modem/netmon.c:5135: if (g_at_chat_send(nmd->chat,
"AT+XMCI=1", xmci_prefix,
drivers/xmm7modem/netmon.c-5192- xmci_cb, cbd, g_free) > 0)
drivers/xmm7modem/netmon.c-5223- return;
drivers/xmm7modem/netmon.c-5233-
drivers/xmm7modem/netmon.c-5234- CALLBACK_WITH_FAILURE(cb, data);
And fixed this as well.
Regards,
-Denis
5:51 a.m.
New subject: [RFC PATCH v3 1/1] atmodem/lte: proto and authentication handling
Hi Denis,
On Fri, Oct 12, 2018 at 8:46 PM Denis Kenzior <denkenz(a)gmail.com> wrote:
Hi Giacinto,
On 10/12/2018 01:15 PM, Giacinto Cifelli wrote:
> I have already added it.
>
> I have just greped through the code, here are some examples, from the
> end of the output:
>
> plugins/phonesim.c:18187: if (g_at_chat_send(data->chat, buf, none_prefix,
> plugins/phonesim.c-18237- set_online_cb, cbd, g_free) > 0)
> plugins/phonesim.c-18274- return;
> plugins/phonesim.c-18284-
> plugins/phonesim.c-18285- CALLBACK_WITH_FAILURE(cb, user_data);
>
Okay, that's a bug. Fixed.
> gatchat/gatmux.c:19544: if (g_at_chat_send(chat, "AT+CMUX=?",
cmux_prefix,
> gatchat/gatmux.c-19596- mux_query_cb, msd, msd_free) > 0)
> gatchat/gatmux.c-19634- return TRUE;
> gatchat/gatmux.c-19649-
> gatchat/gatmux.c-19650- if (msd)
> gatchat/gatmux.c-19660- msd_free(msd);
> // this is actually a mistake because the msd pointer won't change, freed or
not
>
I don't see anything wrong here? Besides the redundant if which will
always be true.
right. my mistake. Here there is no risk to call msd_free twice.
> drivers/xmm7modem/netmon.c:5135: if (g_at_chat_send(nmd->chat,
> "AT+XMCI=1", xmci_prefix,
> drivers/xmm7modem/netmon.c-5192- xmci_cb, cbd, g_free) > 0)
> drivers/xmm7modem/netmon.c-5223- return;
> drivers/xmm7modem/netmon.c-5233-
> drivers/xmm7modem/netmon.c-5234- CALLBACK_WITH_FAILURE(cb, data);
And fixed this as well.
thanks.
Please note that I didn't go through the entire list of results of
grep, you asked for "examples".
Regards,
-Denis
Regards,
Giacinto
5:10 a.m.
New subject: [RFC PATCH v3 1/1] atmodem/lte: proto and authentication handling
Hi Giacinto,
On 10/12/2018 08:16 AM, 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 | 94 ++++++++++++++++++++++++++++++++++++++-----
1 file changed, 84 insertions(+), 10 deletions(-)
diff --git a/drivers/atmodem/lte.c b/drivers/atmodem/lte.c
index efa4e5fe..e5af6cd2 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,27 +66,93 @@ static void at_lte_set_default_attach_info_cb(gboolean ok, GAtResult
*result,
cb(&error, cbd->data);
}
+static void at_lte_set_default_attach_info_cb(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];
+ size_t buflen = sizeof(buf);
+ size_t 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;
+ size_t len;
DBG("LTE config with APN: %s", info->apn);
- if (strlen(info->apn) > 0)
- snprintf(buf, sizeof(buf),
"AT+CGDCONT=0,\"IP\",\"%s\"",
- info->apn);
- else
- snprintf(buf, sizeof(buf), "AT+CGDCONT=0,\"IP\"");
+ cbd->cb = cb;
+ cbd->data = data;
+ cbd->ldd = ldd;
You can't really do that. There's no guarantee that the core atom will
keep this object around for the lifetime of the driver transaction. Why
do you need this anyway? Is it just to build the +CGAUTH part? Why
don't you l_strdup_printf the +CGAUTH commmand or something instead and
invoke it in the callback?
E.g. cbd = cb_data_new();
char *cgauth_cmd = g_strdup_printf("CGAUTH=0,1,"%s","%s",
username,
password);
cbd->user = cgauth_cmd;
Alternatively you can queue both commands and save the +CGAUTH command
id inside ldd. Then if +CGDCONT fails, you can g_at_chat_cancel the
+CGAUTH.
+ 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;
+ }
+
+ len = snprintf(buf, sizeof(buf), "AT+CGDCONT=0,\"%s\"", proto);
+
+ if (*info->apn)
+ snprintf(buf+len, sizeof(buf)-len, ",\"%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_default_attach_info_cb, cbd, NULL) > 0)
Why are you removing the destructor. This will cause leaks whenever a
hot-unplug event happens and this command is queued...
If you want to use cbd across multiple commands, the proper way to do
that is with a reference counted structure.
return;
+ g_free(cbd);
CALLBACK_WITH_FAILURE(cb, data);
}
Regards,
-Denis
5:30 a.m.
New subject: [RFC PATCH v3 1/1] atmodem/lte: proto and authentication handling
Hi Denis,
On Fri, Oct 12, 2018 at 8:10 PM Denis Kenzior <denkenz(a)gmail.com> wrote:
Hi Giacinto,
> 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;
> + size_t len;
>
> + cbd->cb = cb;
> + cbd->data = data;
> + cbd->ldd = ldd;
You can't really do that. There's no guarantee that the core atom will
keep this object around for the lifetime of the driver transaction.
That's interesting. Can I pass const struct ofono_lte *lte, or the
same constraints apply?
Or some other object that I can rely upon?
Also, the core atom shouldn't be called until the callback is triggered.
Why do you need this anyway? Is it just to build the +CGAUTH part?
Why
don't you l_strdup_printf the +CGAUTH commmand or something instead and
invoke it in the callback?
E.g. cbd = cb_data_new();
char *cgauth_cmd = g_strdup_printf("CGAUTH=0,1,"%s","%s",
username,
password);
I will check this, also for compatibility with the switch(vendor) to
come, but I have to confess
I don't really like building the command somewhere and using it later.
It is unexpected for someone reading the code.
What if I have to chain more commands?
cbd->user = cgauth_cmd;
Alternatively you can queue both commands and save the +CGAUTH command
id inside ldd. Then if +CGDCONT fails, you can g_at_chat_cancel the
+CGAUTH.
this possibility is also interesting, but equally confusing: I pipe
two commands, and later remove one,
if someone follows the code up to this function, the removal might go unnoticed.
> + 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;
> + }
> +
> + len = snprintf(buf, sizeof(buf), "AT+CGDCONT=0,\"%s\"",
proto);
> +
> + if (*info->apn)
> + snprintf(buf+len, sizeof(buf)-len, ",\"%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_default_attach_info_cb, cbd, NULL) > 0)
Why are you removing the destructor. This will cause leaks whenever a
hot-unplug event happens and this command is queued...
If you want to use cbd across multiple commands, the proper way to do
that is with a reference counted structure.
is there an example elsewhere in the code?
> return;
>
> + g_free(cbd);
> CALLBACK_WITH_FAILURE(cb, data);
> }
>
>
Regards,
-Denis
Regards,
Giacinto
5:53 a.m.
New subject: [RFC PATCH v3 1/1] atmodem/lte: proto and authentication handling
Hi Giacinto,
>> + cbd->cb = cb;
>> + cbd->data = data;
>> + cbd->ldd = ldd;
>
> You can't really do that. There's no guarantee that the core atom will
> keep this object around for the lifetime of the driver transaction.
That's interesting. Can I pass const struct ofono_lte *lte, or the
same constraints apply?
Or some other object that I can rely upon?
Also, the core atom shouldn't be called until the callback is triggered.
Why would you need to? In general one can assume that the atom is
around until the .remove method is called. But you cannot assume that
anything passed into the driver will live past the point that the driver
function returns.
> Why do you need this anyway? Is it just to build the +CGAUTH part? Why
> don't you l_strdup_printf the +CGAUTH commmand or something instead and
> invoke it in the callback?
>
> E.g. cbd = cb_data_new();
> char *cgauth_cmd = g_strdup_printf("CGAUTH=0,1,"%s","%s",
username,
> password);
I will check this, also for compatibility with the switch(vendor) to
come, but I have to confess
I don't really like building the command somewhere and using it later.
It is unexpected for someone reading the code.
What if I have to chain more commands?
Then memcpy the pending structure (or the needed members) into an area
with lifetime you control.
>
> Why are you removing the destructor. This will cause leaks whenever a
> hot-unplug event happens and this command is queued...
>
> If you want to use cbd across multiple commands, the proper way to do
> that is with a reference counted structure.
is there an example elsewhere in the code?
drivers/ubloxmodem/netmon.c
drivers/hfpmodem/slc.c
There may be a few other places that get this wrong and should be fixed.
Feel free to point them out when/if you see these.
Regards,
-Denis
6:05 a.m.
New subject: [RFC PATCH v3 1/1] atmodem/lte: proto and authentication handling
Hi Denis,
Then memcpy the pending structure (or the needed members) into an area
with lifetime you control.
that's better, excellent even: I only need chat as a matter of fact.
>>
>> Why are you removing the destructor. This will cause leaks whenever a
>> hot-unplug event happens and this command is queued...
>>
>> If you want to use cbd across multiple commands, the proper way to do
>> that is with a reference counted structure.
>
> is there an example elsewhere in the code?
>
drivers/ubloxmodem/netmon.c
drivers/hfpmodem/slc.c
oh, it's done by hand. Like this I knew how to do it :)
Nevertheless, good example. I will do the same.
There may be a few other places that get this wrong and should be fixed.
Feel free to point them out when/if you see these.
ok. It is difficult to grep because the lines are broken (and
therefore cannot grep for it), so it is just in case I step on them by
chance.
Regards,
-Denis
Regards,
Giacinto
1437
days inactive
1437
days old
14 comments
3 participants
participants (3)
-
Denis Kenzior -
Giacinto Cifelli -
Jonas Bonn