[PATCH 4/5] atmodem/lte: proto and authentication handling

[Jonas Bonn]

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.

Make sure you've tested your patches before submission:  compiles
without warnings, runs, works as expected, no valgrind errors, etc.

The ofono_lte_default_attach_info now handles also the protocol and the
authentication method, username and password.
---
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.

+{
+ 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

+ 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) {
+ snprintf(buf + len, buflen, "1,\"%s\",\"%s\"",
+ cbd->info->username, cbd->info->password);
+ break;
+ snprintf(buf + len, buflen, "2,\"%s\",\"%s\"",
+ cbd->info->username, cbd->info->password);
+ break;
+ 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.

+
+ switch (info->proto) {
+ proto = "IPV6";
+ break;
+ proto = "IPV4V6";
+ break;
+ 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);

- /* 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.

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.

/Jonas

return FALSE;
}