Hi, On Fri, Oct 12, 2018 at 10:12 AM Jonas Bonn <[email protected]> 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 <[email protected]> > > --- > > 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 _______________________________________________ ofono mailing list [email protected] https://lists.ofono.org/mailman/listinfo/ofono
