Hi Denis, thanks for all the comments. I will fix and resubmit. Please see also below.
On Wed, Oct 24, 2018 at 9:47 PM Denis Kenzior <[email protected]> wrote: > > Hi Giacinto, > > On 10/22/2018 10:00 PM, 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 | 118 ++++++++++++++++++++++++++++++++++++------ > > 1 file changed, 101 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/atmodem/lte.c b/drivers/atmodem/lte.c > > index c4866623..ae7a815c 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 > > @@ -44,41 +45,124 @@ struct lte_driver_data { > > GAtChat *chat; > > }; > > > > -static void at_lte_set_default_attach_info_cb(gboolean ok, GAtResult > > *result, > > +struct lte_cbd { > > + gint ref_count; /* Ref count */ > > + ofono_lte_cb_t cb; > > + void *data; > > + GAtChat *chat; > > Why don't you just replace this with struct lte_driver_data * instead; > > > + const struct ofono_lte_default_attach_info *info; > > And honestly, I'd just put this into lte_driver_data as well. > > Nope you can't do this. This is the third time I nak-ed this part I can't do what? I don't see what you are referring to. > > > + struct ofono_modem *modem; > > You don't use this member, so leave it out. > > > +}; > > + > > +static struct lte_cbd *lte_cb_data_new0(void *cb, void *data, > > + GAtChat *chat, const struct ofono_lte_default_attach_info > > *info) > > +{ > > + struct lte_cbd *cbd = g_new0(struct lte_cbd, 1); > > + > > + cbd->ref_count = 1; > > + cbd->cb = cb; > > + cbd->data = data; > > + cbd->chat = chat; > > + cbd->info = info; > > + > > + return cbd; > > +} > > + > > +static inline struct lte_cbd *lte_cb_data_ref(struct lte_cbd *cbd) > > Why don't we simply add ref/unref to cb_data. e.g. > > cb_data_ref() > cb_data_unref() In atutil? that would be a good idea. > > > +{ > > + if (cbd == NULL) > > + return NULL; > > + > > + g_atomic_int_inc(&cbd->ref_count); > > + > > + return cbd; > > +} > > + > > +static void lte_cb_data_unref(gpointer user_data) > > +{ > > + gboolean is_zero; > > + struct lte_cbd *cbd = user_data; > > + > > + if (cbd == NULL) > > + return; > > + > > + is_zero = g_atomic_int_dec_and_test(&cbd->ref_count); > > + > > + if (is_zero == TRUE) > > + g_free(cbd); > > +} > > + > > +static void at_lte_set_auth_cb(gboolean ok, GAtResult *result, > > gpointer user_data) > > { > > - struct cb_data *cbd = user_data; > > + struct lte_cbd *cbd = user_data; > > ofono_lte_cb_t cb = cbd->cb; > > struct ofono_error error; > > > > - DBG("ok %d", ok); > > - > > decode_at_error(&error, g_at_result_final_response(result)); > > cb(&error, cbd->data); > > } > > > > +static void at_lte_set_default_attach_info_cb(gboolean ok, GAtResult > > *result, > > + gpointer user_data) > > +{ > > + struct lte_cbd *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) { > > + lte_cb_data_unref(cbd); > > This causes cb_data_unref to be called twice with reference count at 1. > E.g. most likely a crash right after you return from this callback. you are right. I have tested (in the gemalto version), only with good input and so never run through this line. > > > + decode_at_error(&error, g_at_result_final_response(result)); > > + cb(&error, data); > > + return; > > + } > > + > > + 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; > > + > > + len = snprintf(buf, buflen, "AT+CGAUTH=0,%d", > > + at_util_gprs_auth_method_to_auth_prot(auth_method)); > > + buflen -= len; > > + > > + if (auth_method != OFONO_GPRS_AUTH_METHOD_NONE) > > + snprintf(buf + len, buflen, ",\"%s\",\"%s\"", > > + cbd->info->username, cbd->info->password); > > + > > + cbd = lte_cb_data_ref(cbd); > > + if (g_at_chat_send(cbd->chat, buf, NULL, > > + at_lte_set_auth_cb, cbd, lte_cb_data_unref) > 0) > > + return; > > + > > + lte_cb_data_unref(cbd); > > + 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); > > - > > - DBG("LTE config with APN: %s", info->apn); > > + struct lte_cbd *cbd = lte_cb_data_new0(cb, data, ldd->chat, info); > > + char *buf = at_util_get_cgdcont_command(0, info->proto, info->apn); > > > > Honestly I'd just do it like: > cbd = cb_data_new(cb, data); > cb->user = ldd; > > memcpy(&ldd->pending_username, ...); > memcpy(&ldd->pending_password, ...); > ldd->pending_auth = ...; > > > - 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\""); > > - > > - /* 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) > > - return; > > + at_lte_set_default_attach_info_cb, > > + cbd, lte_cb_data_unref) > 0) > > cb_data_unref > > > + goto end; > > > > + lte_cb_data_unref(cbd); > > g_free/cb_data_unref. > > > CALLBACK_WITH_FAILURE(cb, data); > > +end: > > + g_free(buf); > > } > > > > static gboolean lte_delayed_register(gpointer user_data) > > > > Regards, > -Denis Regards, Giacinto _______________________________________________ ofono mailing list [email protected] https://lists.ofono.org/mailman/listinfo/ofono
