Hi Jonas,

On Fri, Oct 12, 2018 at 6:14 PM Jonas Bonn <[email protected]> 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 <[email protected]>
> > ---
> >   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);
> >   }
> >
>
_______________________________________________
ofono mailing list
[email protected]
https://lists.ofono.org/mailman/listinfo/ofono

Reply via email to