Hi,
I think this patch deserves a couple of lines of justification in the
patch description. From the subject, it isn't obvious what this is
about, and a quick glance at the diff only raises further questions.
Try to explain in the patch description what this is about, what problem
your modification is solving.
On 10/10/18 08:54, Giacinto Cifelli wrote:
---
drivers/atmodem/lte.c | 79 +++++++++++++++++++++++++++----------------
1 file changed, 50 insertions(+), 29 deletions(-)
diff --git a/drivers/atmodem/lte.c b/drivers/atmodem/lte.c
index efa4e5fe..1d097c68 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
Why is copyright attributed to Gemalto when you and the other purported
authors (referencing your AUTHORS patch... see below) all use gmail
addresses?
On 10/10/18 08:54, Giacinto Cifelli wrote:
---
AUTHORS | 3 +++
1 file changed, 3 insertions(+)
diff --git a/AUTHORS b/AUTHORS
index 2d360e6e..8f0f5893 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -138,3 +138,6 @@ Florent Beillonnet<[email protected]>
Martin Hundebøll<[email protected]>
Julien Tournier<[email protected]>
Nandini Rebello<[email protected]>
+Giacinto Cifelli<[email protected]>
+Martin Baschin<[email protected]>
+Sebastian Arnd<[email protected]>
Three authors but only Giacinto's name appears in the patches?
*
* 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,48 +46,67 @@ struct lte_driver_data {
GAtChat *chat;
};
-static void at_lte_set_default_attach_info_cb(gboolean ok, GAtResult *result,
- gpointer user_data)
+static void at_set_reg_info(const struct ofono_lte *lte,
+ const struct ofono_lte_default_attach_info *info)
{
- struct cb_data *cbd = user_data;
- ofono_lte_cb_t cb = cbd->cb;
- struct ofono_error error;
+ struct lte_driver_data *ldd = ofono_lte_get_data(lte);
+ char buf_cgdcont[32 + OFONO_GPRS_MAX_APN_LENGTH +1];
+ char buf_cgauth[32 + OFONO_GPRS_MAX_USERNAME_LENGTH +
+ OFONO_GPRS_MAX_PASSWORD_LENGTH +1];
+ guint buflen = sizeof(buf_cgauth);
+ enum ofono_gprs_auth_method auth_method;
- DBG("ok %d", ok);
+ if (strlen(info->apn) > 0)
+ snprintf(buf_cgdcont, sizeof(buf_cgdcont),
+ "AT+CGDCONT=0,\"IP\",\"%s\"", info->apn);
+ else
+ snprintf(buf_cgdcont, sizeof(buf_cgdcont),
+ "AT+CGDCONT=0,\"IP\"");
- decode_at_error(&error, g_at_result_final_response(result));
- cb(&error, cbd->data);
+ if (g_at_chat_send(ldd->chat, buf_cgdcont, NULL, NULL, NULL, NULL) == 0)
+ return;
+
+ snprintf(buf_cgauth, buflen, "AT+CGAUTH=0,");
+ buflen -= strlen(buf_cgauth);
Using snprintf() without checking the return value is mostly
pointless... if you know with certainty that your buffer will hold your
string just use sprintf. The above could be better written as:
n = sprintf(...);
buflen -= n;
+
+ auth_method = info->auth_method;
+
+ /*
+ * change the authentication method if the parameters are invalid
+ * for behavior compatibility
+ */
+ if(!*info->username || !*info->password)
+ auth_method = OFONO_GPRS_AUTH_METHOD_NONE;
+
+ switch(auth_method) {
+ case OFONO_GPRS_AUTH_METHOD_PAP:
+ snprintf(buf_cgauth+strlen(buf_cgauth),
You know strlen(buf_cgauth) already... it's 'n' above, the return value
from snprintf. No need to traverse the string again to figure this out.
+ buflen, "1,\"%s\",\"%s\"",
+ info->username, info->password);
+ break;
+ case OFONO_GPRS_AUTH_METHOD_CHAP:
+ snprintf(buf_cgauth+strlen(buf_cgauth),
+ buflen, "2,\"%s\",\"%s\"",
+ info->username, info->password);
+ break;
+ case OFONO_GPRS_AUTH_METHOD_NONE:
+ snprintf(buf_cgauth+strlen(buf_cgauth), buflen, "0");
+ break;
+ }
+
+ g_at_chat_send(ldd->chat, buf_cgauth, NULL, NULL, NULL, NULL);
Can't fail?
}
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);
-
- 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;
-
- CALLBACK_WITH_FAILURE(cb, data);
+ CALLBACK_WITH_SUCCESS(cb, data);
}
Aside from abandoning all error handling, what does set_reg_info provide
that could not have been handled by set_default_attach_info?
static gboolean lte_delayed_register(gpointer user_data)
{
- struct ofono_lte *lte = user_data;
-
- ofono_lte_register(lte);
+ ofono_lte_register(user_data);
I prefer the original version... nonetheless, this change doesn't belong
in this patch.
return FALSE;
}
@@ -129,6 +149,7 @@ static struct ofono_lte_driver driver = {
.probe = at_lte_probe,
.remove = at_lte_remove,
.set_default_attach_info = at_lte_set_default_attach_info,
+ .set_reg_info = at_set_reg_info,
};
void at_lte_init(void)
/Jonas
_______________________________________________
ofono mailing list
[email protected]
https://lists.ofono.org/mailman/listinfo/ofono
