[RFC PATCH v3 0/1] atmodem/lte atom

[RFC PATCH v3 0/1] atmodem/lte atom

[Giacinto Cifelli]

[-- Attachment #1: Type: text/plain, Size: 1125 bytes --]

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

[RFC PATCH v3 1/1] atmodem/lte: proto and authentication handling

[Giacinto Cifelli]

[-- Attachment #1: Type: text/plain, Size: 4308 bytes --]

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@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

Re: [RFC PATCH v3 1/1] atmodem/lte: proto and authentication handling

[Jonas Bonn]

[-- Attachment #1: Type: text/plain, Size: 4822 bytes --]

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@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);
>   }
>   

Re: [RFC PATCH v3 1/1] atmodem/lte: proto and authentication handling

[Giacinto Cifelli]

[-- Attachment #1: Type: text/plain, Size: 6085 bytes --]

Hi Jonas,

On Fri, Oct 12, 2018 at 6:14 PM Jonas Bonn <jonas@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@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);
> >   }
> >
>

Re: [RFC PATCH v3 1/1] atmodem/lte: proto and authentication handling

[Giacinto Cifelli]

[-- Attachment #1: Type: text/plain, Size: 379 bytes --]

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?

 >

Re: [RFC PATCH v3 1/1] atmodem/lte: proto and authentication handling

[Jonas Bonn]

[-- Attachment #1: Type: text/plain, Size: 650 bytes --]


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

Re: [RFC PATCH v3 1/1] atmodem/lte: proto and authentication handling

[Giacinto Cifelli]

[-- Attachment #1: Type: text/plain, Size: 1265 bytes --]

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@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
>

Re: [RFC PATCH v3 1/1] atmodem/lte: proto and authentication handling

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1036 bytes --]

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

Re: [RFC PATCH v3 1/1] atmodem/lte: proto and authentication handling

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 5573 bytes --]

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@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

Re: [RFC PATCH v3 1/1] atmodem/lte: proto and authentication handling

[Giacinto Cifelli]

[-- Attachment #1: Type: text/plain, Size: 2246 bytes --]

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@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

Re: [RFC PATCH v3 1/1] atmodem/lte: proto and authentication handling

[Giacinto Cifelli]

[-- Attachment #1: Type: text/plain, Size: 3383 bytes --]

Hi Denis,

On Fri, Oct 12, 2018 at 8:10 PM Denis Kenzior <denkenz@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

Re: [RFC PATCH v3 1/1] atmodem/lte: proto and authentication handling

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1325 bytes --]

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

Re: [RFC PATCH v3 1/1] atmodem/lte: proto and authentication handling

[Giacinto Cifelli]

[-- Attachment #1: Type: text/plain, Size: 1689 bytes --]

Hi Denis,

On Fri, Oct 12, 2018 at 8:46 PM Denis Kenzior <denkenz@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

Re: [RFC PATCH v3 1/1] atmodem/lte: proto and authentication handling

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1927 bytes --]

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

Re: [RFC PATCH v3 1/1] atmodem/lte: proto and authentication handling

[Giacinto Cifelli]

[-- Attachment #1: Type: text/plain, Size: 1031 bytes --]

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