* [PATCH 4/6] plugins/provisioning and mbpi: support for auth NONE @ 2018-10-04 5:05 Giacinto Cifelli 2018-10-04 5:05 ` [PATCH 6/6] drivers: " Giacinto Cifelli 0 siblings, 1 reply; 20+ messages in thread From: Giacinto Cifelli @ 2018-10-04 5:05 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 1765 bytes --] support of 'none' in file-provisioning and in mbpi. The default authentication method is set to 'none'. --- plugins/file-provision.c | 6 +++++- plugins/mbpi.c | 5 ++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/plugins/file-provision.c b/plugins/file-provision.c index d4846a65..2bd516ff 100644 --- a/plugins/file-provision.c +++ b/plugins/file-provision.c @@ -97,14 +97,18 @@ static int config_file_provision_get_settings(const char *mcc, value = g_key_file_get_string(key_file, setting_group, "internet.AuthenticationMethod", NULL); + /* select default authentication method */ + (*settings)[0].auth_method = OFONO_GPRS_AUTH_METHOD_NONE; + if (value != NULL) { + if (g_strcmp0(value, "chap") == 0) (*settings)[0].auth_method = OFONO_GPRS_AUTH_METHOD_CHAP; else if (g_strcmp0(value, "pap") == 0) (*settings)[0].auth_method = OFONO_GPRS_AUTH_METHOD_PAP; - else + else if (g_strcmp0(value, "none") != 0) DBG("Unknown auth method: %s", value); g_free(value); diff --git a/plugins/mbpi.c b/plugins/mbpi.c index ae92c762..d101085f 100644 --- a/plugins/mbpi.c +++ b/plugins/mbpi.c @@ -148,11 +148,14 @@ static void authentication_start(GMarkupParseContext *context, return; } + /* select default authentication method */ + *auth_method = OFONO_GPRS_AUTH_METHOD_NONE; + if (strcmp(text, "chap") == 0) *auth_method = OFONO_GPRS_AUTH_METHOD_CHAP; else if (strcmp(text, "pap") == 0) *auth_method = OFONO_GPRS_AUTH_METHOD_PAP; - else + else if (strcmp(text, "none") != 0) mbpi_g_set_error(context, error, G_MARKUP_ERROR, G_MARKUP_ERROR_UNKNOWN_ATTRIBUTE, "Unknown authentication method: %s", -- 2.17.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 6/6] drivers: support for auth NONE 2018-10-04 5:05 [PATCH 4/6] plugins/provisioning and mbpi: support for auth NONE Giacinto Cifelli @ 2018-10-04 5:05 ` Giacinto Cifelli 0 siblings, 0 replies; 20+ messages in thread From: Giacinto Cifelli @ 2018-10-04 5:05 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 13140 bytes --] Added the explicit support for auth NONE. It needs to be added in all drivers/*/gprs-context.c atoms. This method is already supported by all atoms that support authentication (ie, all but Sierra' swmodem driver). The behavior is left unchanged in case of inconsistent parameters: if username is empty, then fallback to auth NONE. --- drivers/atmodem/gprs-context.c | 18 ++++++++++++++---- drivers/ifxmodem/gprs-context.c | 10 ++++++++-- drivers/isimodem/gprs-context.c | 14 +++++++++----- drivers/mbimmodem/gprs-context.c | 16 +++++++++++++--- drivers/mbmmodem/gprs-context.c | 11 ++++++----- drivers/qmimodem/gprs-context.c | 13 +++++++++---- drivers/rilmodem/gprs-context.c | 13 +++++++++---- drivers/stemodem/gprs-context.c | 9 +++++---- drivers/telitmodem/gprs-context-ncm.c | 10 ++++++++-- drivers/ubloxmodem/gprs-context.c | 7 ++++--- 10 files changed, 85 insertions(+), 36 deletions(-) diff --git a/drivers/atmodem/gprs-context.c b/drivers/atmodem/gprs-context.c index 79ac4c8e..93dd3379 100644 --- a/drivers/atmodem/gprs-context.c +++ b/drivers/atmodem/gprs-context.c @@ -158,7 +158,10 @@ static gboolean setup_ppp(struct ofono_gprs_context *gc) g_at_ppp_set_debug(gcd->ppp, ppp_debug, "PPP"); g_at_ppp_set_auth_method(gcd->ppp, gcd->auth_method); - g_at_ppp_set_credentials(gcd->ppp, gcd->username, gcd->password); + + if (gcd->auth_method != G_AT_PPP_AUTH_METHOD_NONE) + g_at_ppp_set_credentials(gcd->ppp, gcd->username, + gcd->password); /* set connect and disconnect callbacks */ g_at_ppp_set_connect_function(gcd->ppp, ppp_connect, gc); @@ -247,7 +250,7 @@ static void at_gprs_activate_primary(struct ofono_gprs_context *gc, memcpy(gcd->username, ctx->username, sizeof(ctx->username)); memcpy(gcd->password, ctx->password, sizeof(ctx->password)); - /* We only support CHAP and PAP */ + /* We support CHAP, PAP and NONE */ switch (ctx->auth_method) { case OFONO_GPRS_AUTH_METHOD_CHAP: gcd->auth_method = G_AT_PPP_AUTH_METHOD_CHAP; @@ -255,8 +258,11 @@ static void at_gprs_activate_primary(struct ofono_gprs_context *gc, case OFONO_GPRS_AUTH_METHOD_PAP: gcd->auth_method = G_AT_PPP_AUTH_METHOD_PAP; break; - default: - goto error; + case OFONO_GPRS_AUTH_METHOD_NONE: + gcd->auth_method = G_AT_PPP_AUTH_METHOD_NONE; + gcd->username[0] = 0; + gcd->password[0] = 0; + break; } gcd->state = STATE_ENABLING; @@ -304,6 +310,10 @@ static void at_gprs_activate_primary(struct ofono_gprs_context *gc, snprintf(buf + len, sizeof(buf) - len - 3, ",\"PAP:%s\"", ctx->apn); break; + case OFONO_GPRS_AUTH_METHOD_NONE: + snprintf(buf + len, sizeof(buf) - len - 3, + ",\"%s\"", ctx->apn); + break; } break; default: diff --git a/drivers/ifxmodem/gprs-context.c b/drivers/ifxmodem/gprs-context.c index 885e41bb..289b4341 100644 --- a/drivers/ifxmodem/gprs-context.c +++ b/drivers/ifxmodem/gprs-context.c @@ -466,8 +466,14 @@ static void ifx_gprs_activate_primary(struct ofono_gprs_context *gc, gcd->active_context = ctx->cid; gcd->cb = cb; gcd->cb_data = data; - memcpy(gcd->username, ctx->username, sizeof(ctx->username)); - memcpy(gcd->password, ctx->password, sizeof(ctx->password)); + + if (ctx->auth_method == OFONO_GPRS_AUTH_METHOD_NONE) { + memset(gcd->username, 0, sizeof(gcd->username)); + memset(gcd->password, 0, sizeof(gcd->password)); + } else { + memcpy(gcd->username, ctx->username, sizeof(ctx->username)); + memcpy(gcd->password, ctx->password, sizeof(ctx->password)); + } gcd->state = STATE_ENABLING; gcd->proto = ctx->proto; diff --git a/drivers/isimodem/gprs-context.c b/drivers/isimodem/gprs-context.c index ce53d022..4d2e7a12 100644 --- a/drivers/isimodem/gprs-context.c +++ b/drivers/isimodem/gprs-context.c @@ -538,11 +538,15 @@ static void isi_gprs_activate_primary(struct ofono_gprs_context *gc, strncpy(cd->apn, ctx->apn, GPDS_MAX_APN_STRING_LENGTH); cd->apn[GPDS_MAX_APN_STRING_LENGTH] = '\0'; - strncpy(cd->username, ctx->username, GPDS_MAX_USERNAME_LENGTH); - cd->username[GPDS_MAX_USERNAME_LENGTH] = '\0'; - - strncpy(cd->password, ctx->password, GPDS_MAX_PASSWORD_LENGTH); - cd->username[GPDS_MAX_PASSWORD_LENGTH] = '\0'; + if (ctx->auth_method == OFONO_GPRS_AUTH_METHOD_NONE) { + memset(cd->username, 0, sizeof(cd->username)); + memset(cd->password, 0, sizeof(cd->password)); + } else { + strncpy(cd->username, ctx->username, GPDS_MAX_USERNAME_LENGTH); + cd->username[GPDS_MAX_USERNAME_LENGTH] = '\0'; + strncpy(cd->password, ctx->password, GPDS_MAX_PASSWORD_LENGTH); + cd->username[GPDS_MAX_PASSWORD_LENGTH] = '\0'; + } cd->pep = g_isi_pep_create(cd->idx, NULL, NULL); if (cd->pep == NULL) diff --git a/drivers/mbimmodem/gprs-context.c b/drivers/mbimmodem/gprs-context.c index 79793c92..be256e43 100644 --- a/drivers/mbimmodem/gprs-context.c +++ b/drivers/mbimmodem/gprs-context.c @@ -75,9 +75,11 @@ static uint32_t auth_method_to_auth_protocol(enum ofono_gprs_auth_method method) return 2; /* MBIMAuthProtocolChap */ case OFONO_GPRS_AUTH_METHOD_PAP: return 1; /* MBIMAuthProtocolPap */ + case OFONO_GPRS_AUTH_METHOD_NONE: + return 0; /* MBIMAUthProtocolNone */ } - return 0; + return 0; /* MBIMAUthProtocolNone */ } static void mbim_deactivate_cb(struct mbim_message *message, void *user) @@ -345,6 +347,8 @@ static void mbim_gprs_activate_primary(struct ofono_gprs_context *gc, { struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc); struct mbim_message *message; + const char username = NULL; + const char password = NULL; DBG("cid %u", ctx->cid); @@ -354,6 +358,12 @@ static void mbim_gprs_activate_primary(struct ofono_gprs_context *gc, gcd->active_context = ctx->cid; gcd->proto = ctx->proto; + if (ctx->auth_method != OFONO_GPRS_AUTH_METHOD_NONE && ctx->username[0]) + username = ctx->username; + + if (ctx->auth_method != OFONO_GPRS_AUTH_METHOD_NONE && ctx->password[0]) + password = ctx->password; + message = mbim_message_new(mbim_uuid_basic_connect, MBIM_CID_CONNECT, MBIM_COMMAND_TYPE_SET); @@ -361,8 +371,8 @@ static void mbim_gprs_activate_primary(struct ofono_gprs_context *gc, ctx->cid, 1, /* MBIMActivationCommandActivate */ ctx->apn, - ctx->username[0] ? ctx->username : NULL, - ctx->password[0] ? ctx->password : NULL, + username, + password, 0, /*MBIMCompressionNone */ auth_method_to_auth_protocol(ctx->auth_method), proto_to_context_ip_type(ctx->proto), diff --git a/drivers/mbmmodem/gprs-context.c b/drivers/mbmmodem/gprs-context.c index e961afa1..fa8b44b6 100644 --- a/drivers/mbmmodem/gprs-context.c +++ b/drivers/mbmmodem/gprs-context.c @@ -394,11 +394,12 @@ static void mbm_gprs_activate_primary(struct ofono_gprs_context *gc, * Set username and password, this should be done after CGDCONT * or an error can occur. We don't bother with error checking * here - * */ - snprintf(buf, sizeof(buf), "AT*EIAAUW=%d,1,\"%s\",\"%s\"", - ctx->cid, ctx->username, ctx->password); - - g_at_chat_send(gcd->chat, buf, none_prefix, NULL, NULL, NULL); + */ + if (ctx->auth_method != OFONO_GPRS_AUTH_METHOD_NONE) { + snprintf(buf, sizeof(buf), "AT*EIAAUW=%d,1,\"%s\",\"%s\"", + ctx->cid, ctx->username, ctx->password); + g_at_chat_send(gcd->chat, buf, none_prefix, NULL, NULL, NULL); + } return; diff --git a/drivers/qmimodem/gprs-context.c b/drivers/qmimodem/gprs-context.c index 9a22b89f..7f31dd0e 100644 --- a/drivers/qmimodem/gprs-context.c +++ b/drivers/qmimodem/gprs-context.c @@ -238,7 +238,12 @@ static void qmi_activate_primary(struct ofono_gprs_context *gc, struct cb_data *cbd = cb_data_new(cb, user_data); struct qmi_param *param; uint8_t ip_family; - uint8_t auth; + /* + * set default authentication to CHAP, even if unneeded, + * otherwise the compiler complains that: + * ‘auth’ may be used uninitialized in this function + */ + uint8_t auth = QMI_WDS_AUTHENTICATION_CHAP; DBG("cid %u", ctx->cid); @@ -273,7 +278,7 @@ static void qmi_activate_primary(struct ofono_gprs_context *gc, case OFONO_GPRS_AUTH_METHOD_PAP: auth = QMI_WDS_AUTHENTICATION_PAP; break; - default: + case OFONO_GPRS_AUTH_METHOD_NONE: auth = QMI_WDS_AUTHENTICATION_NONE; break; } @@ -281,11 +286,11 @@ static void qmi_activate_primary(struct ofono_gprs_context *gc, qmi_param_append_uint8(param, QMI_WDS_PARAM_AUTHENTICATION_PREFERENCE, auth); - if (ctx->username[0] != '\0') + if (auth != QMI_WDS_AUTHENTICATION_NONE && ctx->username[0] != '\0') qmi_param_append(param, QMI_WDS_PARAM_USERNAME, strlen(ctx->username), ctx->username); - if (ctx->password[0] != '\0') + if (auth != QMI_WDS_AUTHENTICATION_NONE && ctx->password[0] != '\0') qmi_param_append(param, QMI_WDS_PARAM_PASSWORD, strlen(ctx->password), ctx->password); diff --git a/drivers/rilmodem/gprs-context.c b/drivers/rilmodem/gprs-context.c index 1f476e23..ef62cba9 100644 --- a/drivers/rilmodem/gprs-context.c +++ b/drivers/rilmodem/gprs-context.c @@ -598,9 +598,12 @@ static void ril_gprs_context_activate_primary(struct ofono_gprs_context *gc, * We do the same as in $AOSP/frameworks/opt/telephony/src/java/com/ * android/internal/telephony/dataconnection/DataConnection.java, * onConnect(), and use authentication or not depending on whether - * the user field is empty or not. + * the user field is empty or not, + * on top of the verification for the authentication method. */ - if (ctx->username[0] != '\0') + + if (ctx->auth_method != OFONO_GPRS_AUTH_METHOD_NONE && + ctx->username[0] != '\0') auth_type = RIL_AUTH_BOTH; else auth_type = RIL_AUTH_NONE; @@ -615,8 +618,10 @@ static void ril_gprs_context_activate_primary(struct ofono_gprs_context *gc, parcel_w_string(&rilp, buf); g_ril_append_print_buf(gcd->ril, "(%d,%s,%s,%s,%s,%d,%s,%u)", - tech, profile, ctx->apn, ctx->username, - ctx->password, auth_type, + tech, profile, ctx->apn, + auth_type == RIL_AUTH_NONE ? "" : ctx->username, + auth_type == RIL_AUTH_NONE ? "" : ctx->password, + auth_type, ril_util_gprs_proto_to_ril_string(ctx->proto), ctx->cid); } else diff --git a/drivers/stemodem/gprs-context.c b/drivers/stemodem/gprs-context.c index 18b2bfa4..32facd8c 100644 --- a/drivers/stemodem/gprs-context.c +++ b/drivers/stemodem/gprs-context.c @@ -307,10 +307,11 @@ static void ste_gprs_activate_primary(struct ofono_gprs_context *gc, * or an error can occur. We don't bother with error checking * here */ - snprintf(buf, sizeof(buf), "AT*EIAAUW=%d,1,\"%s\",\"%s\"", - ctx->cid, ctx->username, ctx->password); - - g_at_chat_send(gcd->chat, buf, none_prefix, NULL, NULL, NULL); + if (ctx->auth_method != OFONO_GPRS_AUTH_METHOD_NONE) { + snprintf(buf, sizeof(buf), "AT*EIAAUW=%d,1,\"%s\",\"%s\"", + ctx->cid, ctx->username, ctx->password); + g_at_chat_send(gcd->chat, buf, none_prefix, NULL, NULL, NULL); + } return; diff --git a/drivers/telitmodem/gprs-context-ncm.c b/drivers/telitmodem/gprs-context-ncm.c index 7139740c..9c9b9500 100644 --- a/drivers/telitmodem/gprs-context-ncm.c +++ b/drivers/telitmodem/gprs-context-ncm.c @@ -277,7 +277,8 @@ static void setup_cb(gboolean ok, GAtResult *result, gpointer user_data) return; } - if (gcd->username[0] && gcd->password[0]) + if (gcd->auth_method != AUTH_METHOD_NONE && + gcd->username[0] && gcd->password[0]) sprintf(buf, "AT#PDPAUTH=%u,%u,\"%s\",\"%s\"", gcd->active_context, gcd->auth_method, gcd->username, gcd->password); @@ -320,7 +321,7 @@ static void telitncm_gprs_activate_primary(struct ofono_gprs_context *gc, gcd->state = STATE_ENABLING; gcd->proto = ctx->proto; - /* We only support CHAP and PAP */ + /* We support CHAP, PAP and NONE */ switch (ctx->auth_method) { case OFONO_GPRS_AUTH_METHOD_CHAP: gcd->auth_method = AUTH_METHOD_CHAP; @@ -328,6 +329,11 @@ static void telitncm_gprs_activate_primary(struct ofono_gprs_context *gc, case OFONO_GPRS_AUTH_METHOD_PAP: gcd->auth_method = AUTH_METHOD_PAP; break; + case OFONO_GPRS_AUTH_METHOD_NONE: + gcd->auth_method = AUTH_METHOD_NONE; + gcd->username[0] = 0; + gcd->password[0] = 0; + break; default: goto error; } diff --git a/drivers/ubloxmodem/gprs-context.c b/drivers/ubloxmodem/gprs-context.c index 6fe2719f..7eb18f09 100644 --- a/drivers/ubloxmodem/gprs-context.c +++ b/drivers/ubloxmodem/gprs-context.c @@ -315,9 +315,10 @@ static void ublox_send_uauthreq(struct ofono_gprs_context *gc, case OFONO_GPRS_AUTH_METHOD_CHAP: auth = 2; break; - default: - ofono_error("Unsupported auth type %u", auth_method); - return; + case OFONO_GPRS_AUTH_METHOD_NONE: + auth = 0; + username = password = ""; + break; } snprintf(buf, sizeof(buf), "AT+UAUTHREQ=%u,%u,\"%s\",\"%s\"", -- 2.17.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 1/6] connman-api: added "none" auth_method @ 2018-10-02 6:26 Giacinto Cifelli 2018-10-02 6:26 ` [PATCH 6/6] drivers: support for auth NONE Giacinto Cifelli 0 siblings, 1 reply; 20+ messages in thread From: Giacinto Cifelli @ 2018-10-02 6:26 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 566 bytes --] --- doc/connman-api.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/doc/connman-api.txt b/doc/connman-api.txt index 9220d0de..578b9755 100644 --- a/doc/connman-api.txt +++ b/doc/connman-api.txt @@ -192,7 +192,8 @@ Properties boolean Active [readwrite] string AuthenticationMethod [readwrite] Holds the PPP authentication method to use. Valid - values are "pap" and "chap". Defaults to "chap". + values are "pap", "chap" and "none". + Defaults to "chap". string Username [readwrite] -- 2.17.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 6/6] drivers: support for auth NONE 2018-10-02 6:26 [PATCH 1/6] connman-api: added "none" auth_method Giacinto Cifelli @ 2018-10-02 6:26 ` Giacinto Cifelli 2018-10-03 21:29 ` Denis Kenzior 0 siblings, 1 reply; 20+ messages in thread From: Giacinto Cifelli @ 2018-10-02 6:26 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 12607 bytes --] Added the explicit support for auth NONE. It is added in all drivers/*/gprs-context.c atoms that support authentication. The support for no-authentication case is already present in all drivers. This patch allows to use it explicitly. Note that the swmodem driver does not have any authentication concept: no changes. Additionally, the behavior is left unchanged in case of inconsistent parameters: if username is empty, then fallback to auth NONE. --- drivers/atmodem/gprs-context.c | 20 ++++++++++++++++---- drivers/ifxmodem/gprs-context.c | 6 ++++++ drivers/isimodem/gprs-context.c | 5 +++++ drivers/mbimmodem/gprs-context.c | 16 +++++++++++++--- drivers/mbmmodem/gprs-context.c | 11 ++++++----- drivers/qmimodem/gprs-context.c | 13 +++++++++---- drivers/rilmodem/gprs-context.c | 13 +++++++++---- drivers/stemodem/gprs-context.c | 9 +++++---- drivers/telitmodem/gprs-context-ncm.c | 10 ++++++++-- drivers/ubloxmodem/gprs-context.c | 7 ++++--- 10 files changed, 81 insertions(+), 29 deletions(-) diff --git a/drivers/atmodem/gprs-context.c b/drivers/atmodem/gprs-context.c index 79ac4c8e..d53fd1d8 100644 --- a/drivers/atmodem/gprs-context.c +++ b/drivers/atmodem/gprs-context.c @@ -158,7 +158,12 @@ static gboolean setup_ppp(struct ofono_gprs_context *gc) g_at_ppp_set_debug(gcd->ppp, ppp_debug, "PPP"); g_at_ppp_set_auth_method(gcd->ppp, gcd->auth_method); - g_at_ppp_set_credentials(gcd->ppp, gcd->username, gcd->password); + + if (gcd->auth_method != G_AT_PPP_AUTH_METHOD_NONE) + g_at_ppp_set_credentials(gcd->ppp, gcd->username, + gcd->password); + else + g_at_ppp_set_credentials(gcd->ppp, NULL, NULL); /* set connect and disconnect callbacks */ g_at_ppp_set_connect_function(gcd->ppp, ppp_connect, gc); @@ -247,7 +252,7 @@ static void at_gprs_activate_primary(struct ofono_gprs_context *gc, memcpy(gcd->username, ctx->username, sizeof(ctx->username)); memcpy(gcd->password, ctx->password, sizeof(ctx->password)); - /* We only support CHAP and PAP */ + /* We support CHAP, PAP and NONE */ switch (ctx->auth_method) { case OFONO_GPRS_AUTH_METHOD_CHAP: gcd->auth_method = G_AT_PPP_AUTH_METHOD_CHAP; @@ -255,8 +260,11 @@ static void at_gprs_activate_primary(struct ofono_gprs_context *gc, case OFONO_GPRS_AUTH_METHOD_PAP: gcd->auth_method = G_AT_PPP_AUTH_METHOD_PAP; break; - default: - goto error; + case OFONO_GPRS_AUTH_METHOD_NONE: + gcd->auth_method = G_AT_PPP_AUTH_METHOD_NONE; + gcd->username[0] = 0; + gcd->password[0] = 0; + break; } gcd->state = STATE_ENABLING; @@ -304,6 +312,10 @@ static void at_gprs_activate_primary(struct ofono_gprs_context *gc, snprintf(buf + len, sizeof(buf) - len - 3, ",\"PAP:%s\"", ctx->apn); break; + case OFONO_GPRS_AUTH_METHOD_NONE: + snprintf(buf + len, sizeof(buf) - len - 3, + ",\"%s\"", ctx->apn); + break; } break; default: diff --git a/drivers/ifxmodem/gprs-context.c b/drivers/ifxmodem/gprs-context.c index 885e41bb..81b056cc 100644 --- a/drivers/ifxmodem/gprs-context.c +++ b/drivers/ifxmodem/gprs-context.c @@ -466,9 +466,15 @@ static void ifx_gprs_activate_primary(struct ofono_gprs_context *gc, gcd->active_context = ctx->cid; gcd->cb = cb; gcd->cb_data = data; + memcpy(gcd->username, ctx->username, sizeof(ctx->username)); memcpy(gcd->password, ctx->password, sizeof(ctx->password)); + if (ctx->auth_method == OFONO_GPRS_AUTH_METHOD_NONE) { + gcd->username[0] = 0; + gcd->password[0] = 0; + } + gcd->state = STATE_ENABLING; gcd->proto = ctx->proto; diff --git a/drivers/isimodem/gprs-context.c b/drivers/isimodem/gprs-context.c index ce53d022..b1c819c2 100644 --- a/drivers/isimodem/gprs-context.c +++ b/drivers/isimodem/gprs-context.c @@ -544,6 +544,11 @@ static void isi_gprs_activate_primary(struct ofono_gprs_context *gc, strncpy(cd->password, ctx->password, GPDS_MAX_PASSWORD_LENGTH); cd->username[GPDS_MAX_PASSWORD_LENGTH] = '\0'; + if (ctx->auth_method == OFONO_GPRS_AUTH_METHOD_NONE) { + cd->username[0] = 0; + cd->password[0] = 0; + } + cd->pep = g_isi_pep_create(cd->idx, NULL, NULL); if (cd->pep == NULL) goto error; diff --git a/drivers/mbimmodem/gprs-context.c b/drivers/mbimmodem/gprs-context.c index 79793c92..be256e43 100644 --- a/drivers/mbimmodem/gprs-context.c +++ b/drivers/mbimmodem/gprs-context.c @@ -75,9 +75,11 @@ static uint32_t auth_method_to_auth_protocol(enum ofono_gprs_auth_method method) return 2; /* MBIMAuthProtocolChap */ case OFONO_GPRS_AUTH_METHOD_PAP: return 1; /* MBIMAuthProtocolPap */ + case OFONO_GPRS_AUTH_METHOD_NONE: + return 0; /* MBIMAUthProtocolNone */ } - return 0; + return 0; /* MBIMAUthProtocolNone */ } static void mbim_deactivate_cb(struct mbim_message *message, void *user) @@ -345,6 +347,8 @@ static void mbim_gprs_activate_primary(struct ofono_gprs_context *gc, { struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc); struct mbim_message *message; + const char username = NULL; + const char password = NULL; DBG("cid %u", ctx->cid); @@ -354,6 +358,12 @@ static void mbim_gprs_activate_primary(struct ofono_gprs_context *gc, gcd->active_context = ctx->cid; gcd->proto = ctx->proto; + if (ctx->auth_method != OFONO_GPRS_AUTH_METHOD_NONE && ctx->username[0]) + username = ctx->username; + + if (ctx->auth_method != OFONO_GPRS_AUTH_METHOD_NONE && ctx->password[0]) + password = ctx->password; + message = mbim_message_new(mbim_uuid_basic_connect, MBIM_CID_CONNECT, MBIM_COMMAND_TYPE_SET); @@ -361,8 +371,8 @@ static void mbim_gprs_activate_primary(struct ofono_gprs_context *gc, ctx->cid, 1, /* MBIMActivationCommandActivate */ ctx->apn, - ctx->username[0] ? ctx->username : NULL, - ctx->password[0] ? ctx->password : NULL, + username, + password, 0, /*MBIMCompressionNone */ auth_method_to_auth_protocol(ctx->auth_method), proto_to_context_ip_type(ctx->proto), diff --git a/drivers/mbmmodem/gprs-context.c b/drivers/mbmmodem/gprs-context.c index e961afa1..fa8b44b6 100644 --- a/drivers/mbmmodem/gprs-context.c +++ b/drivers/mbmmodem/gprs-context.c @@ -394,11 +394,12 @@ static void mbm_gprs_activate_primary(struct ofono_gprs_context *gc, * Set username and password, this should be done after CGDCONT * or an error can occur. We don't bother with error checking * here - * */ - snprintf(buf, sizeof(buf), "AT*EIAAUW=%d,1,\"%s\",\"%s\"", - ctx->cid, ctx->username, ctx->password); - - g_at_chat_send(gcd->chat, buf, none_prefix, NULL, NULL, NULL); + */ + if (ctx->auth_method != OFONO_GPRS_AUTH_METHOD_NONE) { + snprintf(buf, sizeof(buf), "AT*EIAAUW=%d,1,\"%s\",\"%s\"", + ctx->cid, ctx->username, ctx->password); + g_at_chat_send(gcd->chat, buf, none_prefix, NULL, NULL, NULL); + } return; diff --git a/drivers/qmimodem/gprs-context.c b/drivers/qmimodem/gprs-context.c index 9a22b89f..7f31dd0e 100644 --- a/drivers/qmimodem/gprs-context.c +++ b/drivers/qmimodem/gprs-context.c @@ -238,7 +238,12 @@ static void qmi_activate_primary(struct ofono_gprs_context *gc, struct cb_data *cbd = cb_data_new(cb, user_data); struct qmi_param *param; uint8_t ip_family; - uint8_t auth; + /* + * set default authentication to CHAP, even if unneeded, + * otherwise the compiler complains that: + * ‘auth’ may be used uninitialized in this function + */ + uint8_t auth = QMI_WDS_AUTHENTICATION_CHAP; DBG("cid %u", ctx->cid); @@ -273,7 +278,7 @@ static void qmi_activate_primary(struct ofono_gprs_context *gc, case OFONO_GPRS_AUTH_METHOD_PAP: auth = QMI_WDS_AUTHENTICATION_PAP; break; - default: + case OFONO_GPRS_AUTH_METHOD_NONE: auth = QMI_WDS_AUTHENTICATION_NONE; break; } @@ -281,11 +286,11 @@ static void qmi_activate_primary(struct ofono_gprs_context *gc, qmi_param_append_uint8(param, QMI_WDS_PARAM_AUTHENTICATION_PREFERENCE, auth); - if (ctx->username[0] != '\0') + if (auth != QMI_WDS_AUTHENTICATION_NONE && ctx->username[0] != '\0') qmi_param_append(param, QMI_WDS_PARAM_USERNAME, strlen(ctx->username), ctx->username); - if (ctx->password[0] != '\0') + if (auth != QMI_WDS_AUTHENTICATION_NONE && ctx->password[0] != '\0') qmi_param_append(param, QMI_WDS_PARAM_PASSWORD, strlen(ctx->password), ctx->password); diff --git a/drivers/rilmodem/gprs-context.c b/drivers/rilmodem/gprs-context.c index 1f476e23..ef62cba9 100644 --- a/drivers/rilmodem/gprs-context.c +++ b/drivers/rilmodem/gprs-context.c @@ -598,9 +598,12 @@ static void ril_gprs_context_activate_primary(struct ofono_gprs_context *gc, * We do the same as in $AOSP/frameworks/opt/telephony/src/java/com/ * android/internal/telephony/dataconnection/DataConnection.java, * onConnect(), and use authentication or not depending on whether - * the user field is empty or not. + * the user field is empty or not, + * on top of the verification for the authentication method. */ - if (ctx->username[0] != '\0') + + if (ctx->auth_method != OFONO_GPRS_AUTH_METHOD_NONE && + ctx->username[0] != '\0') auth_type = RIL_AUTH_BOTH; else auth_type = RIL_AUTH_NONE; @@ -615,8 +618,10 @@ static void ril_gprs_context_activate_primary(struct ofono_gprs_context *gc, parcel_w_string(&rilp, buf); g_ril_append_print_buf(gcd->ril, "(%d,%s,%s,%s,%s,%d,%s,%u)", - tech, profile, ctx->apn, ctx->username, - ctx->password, auth_type, + tech, profile, ctx->apn, + auth_type == RIL_AUTH_NONE ? "" : ctx->username, + auth_type == RIL_AUTH_NONE ? "" : ctx->password, + auth_type, ril_util_gprs_proto_to_ril_string(ctx->proto), ctx->cid); } else diff --git a/drivers/stemodem/gprs-context.c b/drivers/stemodem/gprs-context.c index 18b2bfa4..32facd8c 100644 --- a/drivers/stemodem/gprs-context.c +++ b/drivers/stemodem/gprs-context.c @@ -307,10 +307,11 @@ static void ste_gprs_activate_primary(struct ofono_gprs_context *gc, * or an error can occur. We don't bother with error checking * here */ - snprintf(buf, sizeof(buf), "AT*EIAAUW=%d,1,\"%s\",\"%s\"", - ctx->cid, ctx->username, ctx->password); - - g_at_chat_send(gcd->chat, buf, none_prefix, NULL, NULL, NULL); + if (ctx->auth_method != OFONO_GPRS_AUTH_METHOD_NONE) { + snprintf(buf, sizeof(buf), "AT*EIAAUW=%d,1,\"%s\",\"%s\"", + ctx->cid, ctx->username, ctx->password); + g_at_chat_send(gcd->chat, buf, none_prefix, NULL, NULL, NULL); + } return; diff --git a/drivers/telitmodem/gprs-context-ncm.c b/drivers/telitmodem/gprs-context-ncm.c index 7139740c..9c9b9500 100644 --- a/drivers/telitmodem/gprs-context-ncm.c +++ b/drivers/telitmodem/gprs-context-ncm.c @@ -277,7 +277,8 @@ static void setup_cb(gboolean ok, GAtResult *result, gpointer user_data) return; } - if (gcd->username[0] && gcd->password[0]) + if (gcd->auth_method != AUTH_METHOD_NONE && + gcd->username[0] && gcd->password[0]) sprintf(buf, "AT#PDPAUTH=%u,%u,\"%s\",\"%s\"", gcd->active_context, gcd->auth_method, gcd->username, gcd->password); @@ -320,7 +321,7 @@ static void telitncm_gprs_activate_primary(struct ofono_gprs_context *gc, gcd->state = STATE_ENABLING; gcd->proto = ctx->proto; - /* We only support CHAP and PAP */ + /* We support CHAP, PAP and NONE */ switch (ctx->auth_method) { case OFONO_GPRS_AUTH_METHOD_CHAP: gcd->auth_method = AUTH_METHOD_CHAP; @@ -328,6 +329,11 @@ static void telitncm_gprs_activate_primary(struct ofono_gprs_context *gc, case OFONO_GPRS_AUTH_METHOD_PAP: gcd->auth_method = AUTH_METHOD_PAP; break; + case OFONO_GPRS_AUTH_METHOD_NONE: + gcd->auth_method = AUTH_METHOD_NONE; + gcd->username[0] = 0; + gcd->password[0] = 0; + break; default: goto error; } diff --git a/drivers/ubloxmodem/gprs-context.c b/drivers/ubloxmodem/gprs-context.c index 6fe2719f..7eb18f09 100644 --- a/drivers/ubloxmodem/gprs-context.c +++ b/drivers/ubloxmodem/gprs-context.c @@ -315,9 +315,10 @@ static void ublox_send_uauthreq(struct ofono_gprs_context *gc, case OFONO_GPRS_AUTH_METHOD_CHAP: auth = 2; break; - default: - ofono_error("Unsupported auth type %u", auth_method); - return; + case OFONO_GPRS_AUTH_METHOD_NONE: + auth = 0; + username = password = ""; + break; } snprintf(buf, sizeof(buf), "AT+UAUTHREQ=%u,%u,\"%s\",\"%s\"", -- 2.17.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 6/6] drivers: support for auth NONE 2018-10-02 6:26 ` [PATCH 6/6] drivers: support for auth NONE Giacinto Cifelli @ 2018-10-03 21:29 ` Denis Kenzior 2018-10-04 3:44 ` Giacinto Cifelli 2018-10-04 4:43 ` Giacinto Cifelli 0 siblings, 2 replies; 20+ messages in thread From: Denis Kenzior @ 2018-10-03 21:29 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 14736 bytes --] Hi Giacinto, On 10/02/2018 01:26 AM, Giacinto Cifelli wrote: > Added the explicit support for auth NONE. > It is added in all drivers/*/gprs-context.c atoms that support > authentication. > > The support for no-authentication case is already present in all > drivers. This patch allows to use it explicitly. > Note that the swmodem driver does not have any authentication > concept: no changes. > > Additionally, the behavior is left unchanged in case of inconsistent > parameters: if username is empty, then fallback to auth NONE. > --- > drivers/atmodem/gprs-context.c | 20 ++++++++++++++++---- > drivers/ifxmodem/gprs-context.c | 6 ++++++ > drivers/isimodem/gprs-context.c | 5 +++++ > drivers/mbimmodem/gprs-context.c | 16 +++++++++++++--- > drivers/mbmmodem/gprs-context.c | 11 ++++++----- > drivers/qmimodem/gprs-context.c | 13 +++++++++---- > drivers/rilmodem/gprs-context.c | 13 +++++++++---- > drivers/stemodem/gprs-context.c | 9 +++++---- > drivers/telitmodem/gprs-context-ncm.c | 10 ++++++++-- > drivers/ubloxmodem/gprs-context.c | 7 ++++--- > 10 files changed, 81 insertions(+), 29 deletions(-) > > diff --git a/drivers/atmodem/gprs-context.c b/drivers/atmodem/gprs-context.c > index 79ac4c8e..d53fd1d8 100644 > --- a/drivers/atmodem/gprs-context.c > +++ b/drivers/atmodem/gprs-context.c > @@ -158,7 +158,12 @@ static gboolean setup_ppp(struct ofono_gprs_context *gc) > g_at_ppp_set_debug(gcd->ppp, ppp_debug, "PPP"); > > g_at_ppp_set_auth_method(gcd->ppp, gcd->auth_method); > - g_at_ppp_set_credentials(gcd->ppp, gcd->username, gcd->password); > + > + if (gcd->auth_method != G_AT_PPP_AUTH_METHOD_NONE) > + g_at_ppp_set_credentials(gcd->ppp, gcd->username, > + gcd->password); > + else > + g_at_ppp_set_credentials(gcd->ppp, NULL, NULL); This else part seems unneeded > > /* set connect and disconnect callbacks */ > g_at_ppp_set_connect_function(gcd->ppp, ppp_connect, gc); > @@ -247,7 +252,7 @@ static void at_gprs_activate_primary(struct ofono_gprs_context *gc, > memcpy(gcd->username, ctx->username, sizeof(ctx->username)); > memcpy(gcd->password, ctx->password, sizeof(ctx->password)); > > - /* We only support CHAP and PAP */ > + /* We support CHAP, PAP and NONE */ > switch (ctx->auth_method) { > case OFONO_GPRS_AUTH_METHOD_CHAP: > gcd->auth_method = G_AT_PPP_AUTH_METHOD_CHAP; > @@ -255,8 +260,11 @@ static void at_gprs_activate_primary(struct ofono_gprs_context *gc, > case OFONO_GPRS_AUTH_METHOD_PAP: > gcd->auth_method = G_AT_PPP_AUTH_METHOD_PAP; > break; > - default: > - goto error; > + case OFONO_GPRS_AUTH_METHOD_NONE: > + gcd->auth_method = G_AT_PPP_AUTH_METHOD_NONE; > + gcd->username[0] = 0; > + gcd->password[0] = 0; > + break; > } > > gcd->state = STATE_ENABLING; > @@ -304,6 +312,10 @@ static void at_gprs_activate_primary(struct ofono_gprs_context *gc, > snprintf(buf + len, sizeof(buf) - len - 3, > ",\"PAP:%s\"", ctx->apn); > break; > + case OFONO_GPRS_AUTH_METHOD_NONE: > + snprintf(buf + len, sizeof(buf) - len - 3, > + ",\"%s\"", ctx->apn); > + break; > } > break; > default: > diff --git a/drivers/ifxmodem/gprs-context.c b/drivers/ifxmodem/gprs-context.c > index 885e41bb..81b056cc 100644 > --- a/drivers/ifxmodem/gprs-context.c > +++ b/drivers/ifxmodem/gprs-context.c > @@ -466,9 +466,15 @@ static void ifx_gprs_activate_primary(struct ofono_gprs_context *gc, > gcd->active_context = ctx->cid; > gcd->cb = cb; > gcd->cb_data = data; > + > memcpy(gcd->username, ctx->username, sizeof(ctx->username)); > memcpy(gcd->password, ctx->password, sizeof(ctx->password)); > > + if (ctx->auth_method == OFONO_GPRS_AUTH_METHOD_NONE) { > + gcd->username[0] = 0; > + gcd->password[0] = 0; > + } > + Ugh, I'd really prefer something like: if (method == NONE) { memset(gcd->username, 0, sizeof(gcd->username)); ... } else { memcpy(...); } > gcd->state = STATE_ENABLING; > gcd->proto = ctx->proto; > > diff --git a/drivers/isimodem/gprs-context.c b/drivers/isimodem/gprs-context.c > index ce53d022..b1c819c2 100644 > --- a/drivers/isimodem/gprs-context.c > +++ b/drivers/isimodem/gprs-context.c > @@ -544,6 +544,11 @@ static void isi_gprs_activate_primary(struct ofono_gprs_context *gc, > strncpy(cd->password, ctx->password, GPDS_MAX_PASSWORD_LENGTH); > cd->username[GPDS_MAX_PASSWORD_LENGTH] = '\0'; > > + if (ctx->auth_method == OFONO_GPRS_AUTH_METHOD_NONE) { > + cd->username[0] = 0; > + cd->password[0] = 0; > + } > + > cd->pep = g_isi_pep_create(cd->idx, NULL, NULL); > if (cd->pep == NULL) > goto error; > diff --git a/drivers/mbimmodem/gprs-context.c b/drivers/mbimmodem/gprs-context.c > index 79793c92..be256e43 100644 > --- a/drivers/mbimmodem/gprs-context.c > +++ b/drivers/mbimmodem/gprs-context.c > @@ -75,9 +75,11 @@ static uint32_t auth_method_to_auth_protocol(enum ofono_gprs_auth_method method) > return 2; /* MBIMAuthProtocolChap */ > case OFONO_GPRS_AUTH_METHOD_PAP: > return 1; /* MBIMAuthProtocolPap */ > + case OFONO_GPRS_AUTH_METHOD_NONE: > + return 0; /* MBIMAUthProtocolNone */ > } > > - return 0; > + return 0; /* MBIMAUthProtocolNone */ > } > > static void mbim_deactivate_cb(struct mbim_message *message, void *user) > @@ -345,6 +347,8 @@ static void mbim_gprs_activate_primary(struct ofono_gprs_context *gc, > { > struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc); > struct mbim_message *message; > + const char username = NULL; > + const char password = NULL; > > DBG("cid %u", ctx->cid); > > @@ -354,6 +358,12 @@ static void mbim_gprs_activate_primary(struct ofono_gprs_context *gc, > gcd->active_context = ctx->cid; > gcd->proto = ctx->proto; > > + if (ctx->auth_method != OFONO_GPRS_AUTH_METHOD_NONE && ctx->username[0]) > + username = ctx->username; > + > + if (ctx->auth_method != OFONO_GPRS_AUTH_METHOD_NONE && ctx->password[0]) > + password = ctx->password; > + > message = mbim_message_new(mbim_uuid_basic_connect, > MBIM_CID_CONNECT, > MBIM_COMMAND_TYPE_SET); > @@ -361,8 +371,8 @@ static void mbim_gprs_activate_primary(struct ofono_gprs_context *gc, > ctx->cid, > 1, /* MBIMActivationCommandActivate */ > ctx->apn, > - ctx->username[0] ? ctx->username : NULL, > - ctx->password[0] ? ctx->password : NULL, > + username, > + password, > 0, /*MBIMCompressionNone */ > auth_method_to_auth_protocol(ctx->auth_method), > proto_to_context_ip_type(ctx->proto), > diff --git a/drivers/mbmmodem/gprs-context.c b/drivers/mbmmodem/gprs-context.c > index e961afa1..fa8b44b6 100644 > --- a/drivers/mbmmodem/gprs-context.c > +++ b/drivers/mbmmodem/gprs-context.c > @@ -394,11 +394,12 @@ static void mbm_gprs_activate_primary(struct ofono_gprs_context *gc, > * Set username and password, this should be done after CGDCONT > * or an error can occur. We don't bother with error checking > * here > - * */ > - snprintf(buf, sizeof(buf), "AT*EIAAUW=%d,1,\"%s\",\"%s\"", > - ctx->cid, ctx->username, ctx->password); > - > - g_at_chat_send(gcd->chat, buf, none_prefix, NULL, NULL, NULL); > + */ > + if (ctx->auth_method != OFONO_GPRS_AUTH_METHOD_NONE) { > + snprintf(buf, sizeof(buf), "AT*EIAAUW=%d,1,\"%s\",\"%s\"", > + ctx->cid, ctx->username, ctx->password); > + g_at_chat_send(gcd->chat, buf, none_prefix, NULL, NULL, NULL); > + } > > return; > > diff --git a/drivers/qmimodem/gprs-context.c b/drivers/qmimodem/gprs-context.c > index 9a22b89f..7f31dd0e 100644 > --- a/drivers/qmimodem/gprs-context.c > +++ b/drivers/qmimodem/gprs-context.c > @@ -238,7 +238,12 @@ static void qmi_activate_primary(struct ofono_gprs_context *gc, > struct cb_data *cbd = cb_data_new(cb, user_data); > struct qmi_param *param; > uint8_t ip_family; > - uint8_t auth; > + /* > + * set default authentication to CHAP, even if unneeded, > + * otherwise the compiler complains that: > + * ‘auth’ may be used uninitialized in this function > + */ > + uint8_t auth = QMI_WDS_AUTHENTICATION_CHAP; Ugh. I really hate GCC people sometimes. While the warning is strictly correct, it is useless for 99% of the code and forces us to jump through hoops. Anyway, I would propose we use the following pattern: int auth_method_to_qmi(enum ofono_auth_method method, uint8_t *out_auth) { switch (method) case CHAP: *out_auth = ... return 0; case: ... } return -ERANGE; } > > DBG("cid %u", ctx->cid); > > @@ -273,7 +278,7 @@ static void qmi_activate_primary(struct ofono_gprs_context *gc, > case OFONO_GPRS_AUTH_METHOD_PAP: > auth = QMI_WDS_AUTHENTICATION_PAP; > break; > - default: > + case OFONO_GPRS_AUTH_METHOD_NONE: > auth = QMI_WDS_AUTHENTICATION_NONE; > break; > } > @@ -281,11 +286,11 @@ static void qmi_activate_primary(struct ofono_gprs_context *gc, > qmi_param_append_uint8(param, QMI_WDS_PARAM_AUTHENTICATION_PREFERENCE, > auth); > > - if (ctx->username[0] != '\0') > + if (auth != QMI_WDS_AUTHENTICATION_NONE && ctx->username[0] != '\0') > qmi_param_append(param, QMI_WDS_PARAM_USERNAME, > strlen(ctx->username), ctx->username); > > - if (ctx->password[0] != '\0') > + if (auth != QMI_WDS_AUTHENTICATION_NONE && ctx->password[0] != '\0') > qmi_param_append(param, QMI_WDS_PARAM_PASSWORD, > strlen(ctx->password), ctx->password); > > diff --git a/drivers/rilmodem/gprs-context.c b/drivers/rilmodem/gprs-context.c > index 1f476e23..ef62cba9 100644 > --- a/drivers/rilmodem/gprs-context.c > +++ b/drivers/rilmodem/gprs-context.c > @@ -598,9 +598,12 @@ static void ril_gprs_context_activate_primary(struct ofono_gprs_context *gc, > * We do the same as in $AOSP/frameworks/opt/telephony/src/java/com/ > * android/internal/telephony/dataconnection/DataConnection.java, > * onConnect(), and use authentication or not depending on whether > - * the user field is empty or not. > + * the user field is empty or not, > + * on top of the verification for the authentication method. > */ > - if (ctx->username[0] != '\0') > + > + if (ctx->auth_method != OFONO_GPRS_AUTH_METHOD_NONE && > + ctx->username[0] != '\0') > auth_type = RIL_AUTH_BOTH; > else > auth_type = RIL_AUTH_NONE; > @@ -615,8 +618,10 @@ static void ril_gprs_context_activate_primary(struct ofono_gprs_context *gc, > parcel_w_string(&rilp, buf); > > g_ril_append_print_buf(gcd->ril, "(%d,%s,%s,%s,%s,%d,%s,%u)", > - tech, profile, ctx->apn, ctx->username, > - ctx->password, auth_type, > + tech, profile, ctx->apn, > + auth_type == RIL_AUTH_NONE ? "" : ctx->username, > + auth_type == RIL_AUTH_NONE ? "" : ctx->password, > + auth_type, > ril_util_gprs_proto_to_ril_string(ctx->proto), > ctx->cid); > } else > diff --git a/drivers/stemodem/gprs-context.c b/drivers/stemodem/gprs-context.c > index 18b2bfa4..32facd8c 100644 > --- a/drivers/stemodem/gprs-context.c > +++ b/drivers/stemodem/gprs-context.c > @@ -307,10 +307,11 @@ static void ste_gprs_activate_primary(struct ofono_gprs_context *gc, > * or an error can occur. We don't bother with error checking > * here > */ > - snprintf(buf, sizeof(buf), "AT*EIAAUW=%d,1,\"%s\",\"%s\"", > - ctx->cid, ctx->username, ctx->password); > - > - g_at_chat_send(gcd->chat, buf, none_prefix, NULL, NULL, NULL); > + if (ctx->auth_method != OFONO_GPRS_AUTH_METHOD_NONE) { > + snprintf(buf, sizeof(buf), "AT*EIAAUW=%d,1,\"%s\",\"%s\"", > + ctx->cid, ctx->username, ctx->password); > + g_at_chat_send(gcd->chat, buf, none_prefix, NULL, NULL, NULL); > + } > > return; > > diff --git a/drivers/telitmodem/gprs-context-ncm.c b/drivers/telitmodem/gprs-context-ncm.c > index 7139740c..9c9b9500 100644 > --- a/drivers/telitmodem/gprs-context-ncm.c > +++ b/drivers/telitmodem/gprs-context-ncm.c > @@ -277,7 +277,8 @@ static void setup_cb(gboolean ok, GAtResult *result, gpointer user_data) > return; > } > > - if (gcd->username[0] && gcd->password[0]) > + if (gcd->auth_method != AUTH_METHOD_NONE && > + gcd->username[0] && gcd->password[0]) > sprintf(buf, "AT#PDPAUTH=%u,%u,\"%s\",\"%s\"", > gcd->active_context, gcd->auth_method, > gcd->username, gcd->password); > @@ -320,7 +321,7 @@ static void telitncm_gprs_activate_primary(struct ofono_gprs_context *gc, > gcd->state = STATE_ENABLING; > gcd->proto = ctx->proto; > > - /* We only support CHAP and PAP */ > + /* We support CHAP, PAP and NONE */ > switch (ctx->auth_method) { > case OFONO_GPRS_AUTH_METHOD_CHAP: > gcd->auth_method = AUTH_METHOD_CHAP; > @@ -328,6 +329,11 @@ static void telitncm_gprs_activate_primary(struct ofono_gprs_context *gc, > case OFONO_GPRS_AUTH_METHOD_PAP: > gcd->auth_method = AUTH_METHOD_PAP; > break; > + case OFONO_GPRS_AUTH_METHOD_NONE: > + gcd->auth_method = AUTH_METHOD_NONE; > + gcd->username[0] = 0; > + gcd->password[0] = 0; > + break; > default: > goto error; > } > diff --git a/drivers/ubloxmodem/gprs-context.c b/drivers/ubloxmodem/gprs-context.c > index 6fe2719f..7eb18f09 100644 > --- a/drivers/ubloxmodem/gprs-context.c > +++ b/drivers/ubloxmodem/gprs-context.c > @@ -315,9 +315,10 @@ static void ublox_send_uauthreq(struct ofono_gprs_context *gc, > case OFONO_GPRS_AUTH_METHOD_CHAP: > auth = 2; > break; > - default: > - ofono_error("Unsupported auth type %u", auth_method); > - return; > + case OFONO_GPRS_AUTH_METHOD_NONE: > + auth = 0; > + username = password = ""; > + break; > } > > snprintf(buf, sizeof(buf), "AT+UAUTHREQ=%u,%u,\"%s\",\"%s\"", > Anyway, these look fine to me otherwise. But I still wouldn't mind some explanation of how we're addressing the questions I raised in my E-mail on Sep 27. Also, what are we doing with plugins/mbpi.c? That database only supports pap/chap but potentially with missing username/password attributes. Should we be converting these to type 'none'? Or leaving things as is and having the drivers deal with converting type chap, empty username/password to type none. I thought the whole idea of introducing type 'None' was that you hated that drivers were doing this? I believe you referred to this as a 'hack' or similar terminology? :) Regards, -Denis ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 6/6] drivers: support for auth NONE 2018-10-03 21:29 ` Denis Kenzior @ 2018-10-04 3:44 ` Giacinto Cifelli 2018-10-04 4:40 ` Denis Kenzior 2018-10-04 4:43 ` Giacinto Cifelli 1 sibling, 1 reply; 20+ messages in thread From: Giacinto Cifelli @ 2018-10-04 3:44 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 20606 bytes --] hi Denis, thank you for your comments on the code. I will change and re-submit. On Wed, Oct 3, 2018 at 11:29 PM Denis Kenzior <denkenz@gmail.com> wrote: > > Hi Giacinto, > > On 10/02/2018 01:26 AM, Giacinto Cifelli wrote: > > Added the explicit support for auth NONE. > > It is added in all drivers/*/gprs-context.c atoms that support > > authentication. > > > > The support for no-authentication case is already present in all > > drivers. This patch allows to use it explicitly. > > Note that the swmodem driver does not have any authentication > > concept: no changes. > > > > Additionally, the behavior is left unchanged in case of inconsistent > > parameters: if username is empty, then fallback to auth NONE. > > --- > > drivers/atmodem/gprs-context.c | 20 ++++++++++++++++---- > > drivers/ifxmodem/gprs-context.c | 6 ++++++ > > drivers/isimodem/gprs-context.c | 5 +++++ > > drivers/mbimmodem/gprs-context.c | 16 +++++++++++++--- > > drivers/mbmmodem/gprs-context.c | 11 ++++++----- > > drivers/qmimodem/gprs-context.c | 13 +++++++++---- > > drivers/rilmodem/gprs-context.c | 13 +++++++++---- > > drivers/stemodem/gprs-context.c | 9 +++++---- > > drivers/telitmodem/gprs-context-ncm.c | 10 ++++++++-- > > drivers/ubloxmodem/gprs-context.c | 7 ++++--- > > 10 files changed, 81 insertions(+), 29 deletions(-) > > > > diff --git a/drivers/atmodem/gprs-context.c b/drivers/atmodem/gprs-context.c > > index 79ac4c8e..d53fd1d8 100644 > > --- a/drivers/atmodem/gprs-context.c > > +++ b/drivers/atmodem/gprs-context.c > > @@ -158,7 +158,12 @@ static gboolean setup_ppp(struct ofono_gprs_context *gc) > > g_at_ppp_set_debug(gcd->ppp, ppp_debug, "PPP"); > > > > g_at_ppp_set_auth_method(gcd->ppp, gcd->auth_method); > > - g_at_ppp_set_credentials(gcd->ppp, gcd->username, gcd->password); > > + > > + if (gcd->auth_method != G_AT_PPP_AUTH_METHOD_NONE) > > + g_at_ppp_set_credentials(gcd->ppp, gcd->username, > > + gcd->password); > > + else > > + g_at_ppp_set_credentials(gcd->ppp, NULL, NULL); > > This else part seems unneeded > > > > > /* set connect and disconnect callbacks */ > > g_at_ppp_set_connect_function(gcd->ppp, ppp_connect, gc); > > @@ -247,7 +252,7 @@ static void at_gprs_activate_primary(struct ofono_gprs_context *gc, > > memcpy(gcd->username, ctx->username, sizeof(ctx->username)); > > memcpy(gcd->password, ctx->password, sizeof(ctx->password)); > > > > - /* We only support CHAP and PAP */ > > + /* We support CHAP, PAP and NONE */ > > switch (ctx->auth_method) { > > case OFONO_GPRS_AUTH_METHOD_CHAP: > > gcd->auth_method = G_AT_PPP_AUTH_METHOD_CHAP; > > @@ -255,8 +260,11 @@ static void at_gprs_activate_primary(struct ofono_gprs_context *gc, > > case OFONO_GPRS_AUTH_METHOD_PAP: > > gcd->auth_method = G_AT_PPP_AUTH_METHOD_PAP; > > break; > > - default: > > - goto error; > > + case OFONO_GPRS_AUTH_METHOD_NONE: > > + gcd->auth_method = G_AT_PPP_AUTH_METHOD_NONE; > > + gcd->username[0] = 0; > > + gcd->password[0] = 0; > > + break; > > } > > > > gcd->state = STATE_ENABLING; > > @@ -304,6 +312,10 @@ static void at_gprs_activate_primary(struct ofono_gprs_context *gc, > > snprintf(buf + len, sizeof(buf) - len - 3, > > ",\"PAP:%s\"", ctx->apn); > > break; > > + case OFONO_GPRS_AUTH_METHOD_NONE: > > + snprintf(buf + len, sizeof(buf) - len - 3, > > + ",\"%s\"", ctx->apn); > > + break; > > } > > break; > > default: > > diff --git a/drivers/ifxmodem/gprs-context.c b/drivers/ifxmodem/gprs-context.c > > index 885e41bb..81b056cc 100644 > > --- a/drivers/ifxmodem/gprs-context.c > > +++ b/drivers/ifxmodem/gprs-context.c > > @@ -466,9 +466,15 @@ static void ifx_gprs_activate_primary(struct ofono_gprs_context *gc, > > gcd->active_context = ctx->cid; > > gcd->cb = cb; > > gcd->cb_data = data; > > + > > memcpy(gcd->username, ctx->username, sizeof(ctx->username)); > > memcpy(gcd->password, ctx->password, sizeof(ctx->password)); > > > > + if (ctx->auth_method == OFONO_GPRS_AUTH_METHOD_NONE) { > > + gcd->username[0] = 0; > > + gcd->password[0] = 0; > > + } > > + > > Ugh, I'd really prefer something like: > > if (method == NONE) { > memset(gcd->username, 0, sizeof(gcd->username)); > ... > } else { > memcpy(...); > } > > > gcd->state = STATE_ENABLING; > > gcd->proto = ctx->proto; > > > > diff --git a/drivers/isimodem/gprs-context.c b/drivers/isimodem/gprs-context.c > > index ce53d022..b1c819c2 100644 > > --- a/drivers/isimodem/gprs-context.c > > +++ b/drivers/isimodem/gprs-context.c > > @@ -544,6 +544,11 @@ static void isi_gprs_activate_primary(struct ofono_gprs_context *gc, > > strncpy(cd->password, ctx->password, GPDS_MAX_PASSWORD_LENGTH); > > cd->username[GPDS_MAX_PASSWORD_LENGTH] = '\0'; > > > > + if (ctx->auth_method == OFONO_GPRS_AUTH_METHOD_NONE) { > > + cd->username[0] = 0; > > + cd->password[0] = 0; > > + } > > + > > cd->pep = g_isi_pep_create(cd->idx, NULL, NULL); > > if (cd->pep == NULL) > > goto error; > > diff --git a/drivers/mbimmodem/gprs-context.c b/drivers/mbimmodem/gprs-context.c > > index 79793c92..be256e43 100644 > > --- a/drivers/mbimmodem/gprs-context.c > > +++ b/drivers/mbimmodem/gprs-context.c > > @@ -75,9 +75,11 @@ static uint32_t auth_method_to_auth_protocol(enum ofono_gprs_auth_method method) > > return 2; /* MBIMAuthProtocolChap */ > > case OFONO_GPRS_AUTH_METHOD_PAP: > > return 1; /* MBIMAuthProtocolPap */ > > + case OFONO_GPRS_AUTH_METHOD_NONE: > > + return 0; /* MBIMAUthProtocolNone */ > > } > > > > - return 0; > > + return 0; /* MBIMAUthProtocolNone */ > > } > > > > static void mbim_deactivate_cb(struct mbim_message *message, void *user) > > @@ -345,6 +347,8 @@ static void mbim_gprs_activate_primary(struct ofono_gprs_context *gc, > > { > > struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc); > > struct mbim_message *message; > > + const char username = NULL; > > + const char password = NULL; > > > > DBG("cid %u", ctx->cid); > > > > @@ -354,6 +358,12 @@ static void mbim_gprs_activate_primary(struct ofono_gprs_context *gc, > > gcd->active_context = ctx->cid; > > gcd->proto = ctx->proto; > > > > + if (ctx->auth_method != OFONO_GPRS_AUTH_METHOD_NONE && ctx->username[0]) > > + username = ctx->username; > > + > > + if (ctx->auth_method != OFONO_GPRS_AUTH_METHOD_NONE && ctx->password[0]) > > + password = ctx->password; > > + > > message = mbim_message_new(mbim_uuid_basic_connect, > > MBIM_CID_CONNECT, > > MBIM_COMMAND_TYPE_SET); > > @@ -361,8 +371,8 @@ static void mbim_gprs_activate_primary(struct ofono_gprs_context *gc, > > ctx->cid, > > 1, /* MBIMActivationCommandActivate */ > > ctx->apn, > > - ctx->username[0] ? ctx->username : NULL, > > - ctx->password[0] ? ctx->password : NULL, > > + username, > > + password, > > 0, /*MBIMCompressionNone */ > > auth_method_to_auth_protocol(ctx->auth_method), > > proto_to_context_ip_type(ctx->proto), > > diff --git a/drivers/mbmmodem/gprs-context.c b/drivers/mbmmodem/gprs-context.c > > index e961afa1..fa8b44b6 100644 > > --- a/drivers/mbmmodem/gprs-context.c > > +++ b/drivers/mbmmodem/gprs-context.c > > @@ -394,11 +394,12 @@ static void mbm_gprs_activate_primary(struct ofono_gprs_context *gc, > > * Set username and password, this should be done after CGDCONT > > * or an error can occur. We don't bother with error checking > > * here > > - * */ > > - snprintf(buf, sizeof(buf), "AT*EIAAUW=%d,1,\"%s\",\"%s\"", > > - ctx->cid, ctx->username, ctx->password); > > - > > - g_at_chat_send(gcd->chat, buf, none_prefix, NULL, NULL, NULL); > > + */ > > + if (ctx->auth_method != OFONO_GPRS_AUTH_METHOD_NONE) { > > + snprintf(buf, sizeof(buf), "AT*EIAAUW=%d,1,\"%s\",\"%s\"", > > + ctx->cid, ctx->username, ctx->password); > > + g_at_chat_send(gcd->chat, buf, none_prefix, NULL, NULL, NULL); > > + } > > > > return; > > > > diff --git a/drivers/qmimodem/gprs-context.c b/drivers/qmimodem/gprs-context.c > > index 9a22b89f..7f31dd0e 100644 > > --- a/drivers/qmimodem/gprs-context.c > > +++ b/drivers/qmimodem/gprs-context.c > > @@ -238,7 +238,12 @@ static void qmi_activate_primary(struct ofono_gprs_context *gc, > > struct cb_data *cbd = cb_data_new(cb, user_data); > > struct qmi_param *param; > > uint8_t ip_family; > > - uint8_t auth; > > + /* > > + * set default authentication to CHAP, even if unneeded, > > + * otherwise the compiler complains that: > > + * ‘auth’ may be used uninitialized in this function > > + */ > > + uint8_t auth = QMI_WDS_AUTHENTICATION_CHAP; > > Ugh. I really hate GCC people sometimes. While the warning is strictly > correct, it is useless for 99% of the code and forces us to jump through > hoops. Anyway, I would propose we use the following pattern: > > int auth_method_to_qmi(enum ofono_auth_method method, uint8_t *out_auth) > { > switch (method) > case CHAP: > *out_auth = ... > return 0; > case: > ... > } > > return -ERANGE; > } > > > > > DBG("cid %u", ctx->cid); > > > > @@ -273,7 +278,7 @@ static void qmi_activate_primary(struct ofono_gprs_context *gc, > > case OFONO_GPRS_AUTH_METHOD_PAP: > > auth = QMI_WDS_AUTHENTICATION_PAP; > > break; > > - default: > > + case OFONO_GPRS_AUTH_METHOD_NONE: > > auth = QMI_WDS_AUTHENTICATION_NONE; > > break; > > } > > @@ -281,11 +286,11 @@ static void qmi_activate_primary(struct ofono_gprs_context *gc, > > qmi_param_append_uint8(param, QMI_WDS_PARAM_AUTHENTICATION_PREFERENCE, > > auth); > > > > - if (ctx->username[0] != '\0') > > + if (auth != QMI_WDS_AUTHENTICATION_NONE && ctx->username[0] != '\0') > > qmi_param_append(param, QMI_WDS_PARAM_USERNAME, > > strlen(ctx->username), ctx->username); > > > > - if (ctx->password[0] != '\0') > > + if (auth != QMI_WDS_AUTHENTICATION_NONE && ctx->password[0] != '\0') > > qmi_param_append(param, QMI_WDS_PARAM_PASSWORD, > > strlen(ctx->password), ctx->password); > > > > diff --git a/drivers/rilmodem/gprs-context.c b/drivers/rilmodem/gprs-context.c > > index 1f476e23..ef62cba9 100644 > > --- a/drivers/rilmodem/gprs-context.c > > +++ b/drivers/rilmodem/gprs-context.c > > @@ -598,9 +598,12 @@ static void ril_gprs_context_activate_primary(struct ofono_gprs_context *gc, > > * We do the same as in $AOSP/frameworks/opt/telephony/src/java/com/ > > * android/internal/telephony/dataconnection/DataConnection.java, > > * onConnect(), and use authentication or not depending on whether > > - * the user field is empty or not. > > + * the user field is empty or not, > > + * on top of the verification for the authentication method. > > */ > > - if (ctx->username[0] != '\0') > > + > > + if (ctx->auth_method != OFONO_GPRS_AUTH_METHOD_NONE && > > + ctx->username[0] != '\0') > > auth_type = RIL_AUTH_BOTH; > > else > > auth_type = RIL_AUTH_NONE; > > @@ -615,8 +618,10 @@ static void ril_gprs_context_activate_primary(struct ofono_gprs_context *gc, > > parcel_w_string(&rilp, buf); > > > > g_ril_append_print_buf(gcd->ril, "(%d,%s,%s,%s,%s,%d,%s,%u)", > > - tech, profile, ctx->apn, ctx->username, > > - ctx->password, auth_type, > > + tech, profile, ctx->apn, > > + auth_type == RIL_AUTH_NONE ? "" : ctx->username, > > + auth_type == RIL_AUTH_NONE ? "" : ctx->password, > > + auth_type, > > ril_util_gprs_proto_to_ril_string(ctx->proto), > > ctx->cid); > > } else > > diff --git a/drivers/stemodem/gprs-context.c b/drivers/stemodem/gprs-context.c > > index 18b2bfa4..32facd8c 100644 > > --- a/drivers/stemodem/gprs-context.c > > +++ b/drivers/stemodem/gprs-context.c > > @@ -307,10 +307,11 @@ static void ste_gprs_activate_primary(struct ofono_gprs_context *gc, > > * or an error can occur. We don't bother with error checking > > * here > > */ > > - snprintf(buf, sizeof(buf), "AT*EIAAUW=%d,1,\"%s\",\"%s\"", > > - ctx->cid, ctx->username, ctx->password); > > - > > - g_at_chat_send(gcd->chat, buf, none_prefix, NULL, NULL, NULL); > > + if (ctx->auth_method != OFONO_GPRS_AUTH_METHOD_NONE) { > > + snprintf(buf, sizeof(buf), "AT*EIAAUW=%d,1,\"%s\",\"%s\"", > > + ctx->cid, ctx->username, ctx->password); > > + g_at_chat_send(gcd->chat, buf, none_prefix, NULL, NULL, NULL); > > + } > > > > return; > > > > diff --git a/drivers/telitmodem/gprs-context-ncm.c b/drivers/telitmodem/gprs-context-ncm.c > > index 7139740c..9c9b9500 100644 > > --- a/drivers/telitmodem/gprs-context-ncm.c > > +++ b/drivers/telitmodem/gprs-context-ncm.c > > @@ -277,7 +277,8 @@ static void setup_cb(gboolean ok, GAtResult *result, gpointer user_data) > > return; > > } > > > > - if (gcd->username[0] && gcd->password[0]) > > + if (gcd->auth_method != AUTH_METHOD_NONE && > > + gcd->username[0] && gcd->password[0]) > > sprintf(buf, "AT#PDPAUTH=%u,%u,\"%s\",\"%s\"", > > gcd->active_context, gcd->auth_method, > > gcd->username, gcd->password); > > @@ -320,7 +321,7 @@ static void telitncm_gprs_activate_primary(struct ofono_gprs_context *gc, > > gcd->state = STATE_ENABLING; > > gcd->proto = ctx->proto; > > > > - /* We only support CHAP and PAP */ > > + /* We support CHAP, PAP and NONE */ > > switch (ctx->auth_method) { > > case OFONO_GPRS_AUTH_METHOD_CHAP: > > gcd->auth_method = AUTH_METHOD_CHAP; > > @@ -328,6 +329,11 @@ static void telitncm_gprs_activate_primary(struct ofono_gprs_context *gc, > > case OFONO_GPRS_AUTH_METHOD_PAP: > > gcd->auth_method = AUTH_METHOD_PAP; > > break; > > + case OFONO_GPRS_AUTH_METHOD_NONE: > > + gcd->auth_method = AUTH_METHOD_NONE; > > + gcd->username[0] = 0; > > + gcd->password[0] = 0; > > + break; > > default: > > goto error; > > } > > diff --git a/drivers/ubloxmodem/gprs-context.c b/drivers/ubloxmodem/gprs-context.c > > index 6fe2719f..7eb18f09 100644 > > --- a/drivers/ubloxmodem/gprs-context.c > > +++ b/drivers/ubloxmodem/gprs-context.c > > @@ -315,9 +315,10 @@ static void ublox_send_uauthreq(struct ofono_gprs_context *gc, > > case OFONO_GPRS_AUTH_METHOD_CHAP: > > auth = 2; > > break; > > - default: > > - ofono_error("Unsupported auth type %u", auth_method); > > - return; > > + case OFONO_GPRS_AUTH_METHOD_NONE: > > + auth = 0; > > + username = password = ""; > > + break; > > } > > > > snprintf(buf, sizeof(buf), "AT+UAUTHREQ=%u,%u,\"%s\",\"%s\"", > > > > Anyway, these look fine to me otherwise. But I still wouldn't mind some > explanation of how we're addressing the questions I raised in my E-mail > on Sep 27. > I suppose your comments on [PATCH 2/4]. I have addressed them on the [PATCH 2/6]. Basically, as you can also see from the code, the user-set value are stored, but the driver changes them internally if suited: - if no username it changes to AUTH_NONE, this is already in place for some - if AUTH_NONE, username and password are cleared internally This permissive approach allows also to deal with the next question (see below). > Also, what are we doing with plugins/mbpi.c? That database only > supports pap/chap but potentially with missing username/password > attributes. Should we be converting these to type 'none'? Or leaving > things as is and having the drivers deal with converting type chap, > empty username/password to type none. I thought the whole idea of > introducing type 'None' was that you hated that drivers were doing this? I had added the 'none' also there, then removed it. It looks to me this database is for CDMA (but I am not sure). In the database itself I have on my machine, there is a single 'pap', no 'chap', and in all other cases the fields are missing. If it is for CDMA, I am not sure how much should this database be maintained. I know that several users have a private branch of ofono for CDMA, and do not intend to change or submit, because the technology is felt EOL. Maybe this could be removed in a 2.x branch that you mentioned apropos AT+CGATT=0 when roaming. In the meanwhile, I have chosen to ignore it. It is not difficult to add it in the code if necessary, and I can also considered it separately from this series of patches. > I believe you referred to this as a 'hack' or similar terminology? :) undocumented hack, yes :) it is the undocumented part that I don't like. For this db, for example, one may assume that without user/pwd it is no-auth. Which is true for all drivers except PPP, that would default to chap and attempt to generate a password, as you described 2 days ago. My concern is for the use of the auth_method for the default LTE bearer. In that case I would prefer to have such a method clearly identified, because the combined attach could fail for various reasons, the network answer is in general inconsistent (I have counted already half a dozen failure codes when APN/auth are incorrect), and - mainly - there is no immediate feedback to the application: the registration is attempted a few times, then there is a back-off timer of 12 minutes (T3346), when the module could attach to a legacy technology. In all this, the user might just believe that it is out of LTE coverage at first, and will take a long time to associate the symptoms with the actual issue. Another issue for the default context is that the parameters are in general non-volatile, so if the SIM is changed, and with it perhaps only the APN, it would still refer to the previous auth_method and parameters until changed explicitly. And in the future, it seems the SIM will be changed quite often And it seems to be particularly important for VoLTE, because no operator supports it in roaming. The standard exists, but seems unattractive. With the sunset of 2G/3G, the only way to have voice will be using a local SIM profile. This SIM exchange is technical achieved with the eUICC. BTW, the SIM hot swap seems to be another point to improve, but for now I am not going into this. > > Regards, > -Denis Regards, Giacinto ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 6/6] drivers: support for auth NONE 2018-10-04 3:44 ` Giacinto Cifelli @ 2018-10-04 4:40 ` Denis Kenzior 2018-10-04 4:51 ` Giacinto Cifelli 0 siblings, 1 reply; 20+ messages in thread From: Denis Kenzior @ 2018-10-04 4:40 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 5186 bytes --] Hi Giacinto, > I suppose your comments on [PATCH 2/4]. I have addressed them on the > [PATCH 2/6]. > Basically, as you can also see from the code, the user-set value are stored, > but the driver changes them internally if suited: > - if no username it changes to AUTH_NONE, this is already in place for some > - if AUTH_NONE, username and password are cleared internally > This permissive approach allows also to deal with the next question (see below). All right. But do you also want to mention this behavior in the D-Bus documentation? E.g. something to the effect of if AuthenticationMethod is set to None, then username / password values are ignored? > >> Also, what are we doing with plugins/mbpi.c? That database only >> supports pap/chap but potentially with missing username/password >> attributes. Should we be converting these to type 'none'? Or leaving >> things as is and having the drivers deal with converting type chap, >> empty username/password to type none. I thought the whole idea of >> introducing type 'None' was that you hated that drivers were doing this? > > I had added the 'none' also there, then removed it. It looks to me > this database is for CDMA (but I am not sure). > In the database itself I have on my machine, there is a single 'pap', > no 'chap', and in all other cases the fields are missing. This database is for both CDMA and GSM. Essentially CHAP is assumed if PAP is not explicitly specified. > > If it is for CDMA, I am not sure how much should this database be maintained. > I know that several users have a private branch of ofono for CDMA, and > do not intend to change or submit, > because the technology is felt EOL. Mobile Broadband Provider Info is still maintained. See https://github.com/GNOME/mobile-broadband-provider-info. The database itself was never really that good, but it was at least something. I think the Ubuntu guys had a plugin that used the Android provisioning db, but it was never contributed upstream. > > Maybe this could be removed in a 2.x branch that you mentioned apropos > AT+CGATT=0 when roaming. > > In the meanwhile, I have chosen to ignore it. It is not difficult to > add it in the code if necessary, > and I can also considered it separately from this series of patches. > Given how closely related it is, it probably should be considered in the same series. This is one of those situations where we're messing with the existing API and so all changes go in or none at all... But given that the drivers are expected to handle empty username/passwords already, I'm okay leaving mbpi as is... but it might be a good idea to fix it anyway... >> I believe you referred to this as a 'hack' or similar terminology? :) > > undocumented hack, yes :) > it is the undocumented part that I don't like. For this db, for > example, one may assume that > without user/pwd it is no-auth. Which is true for all drivers except > PPP, that would > default to chap and attempt to generate a password, as you described 2 days ago. That actually depends on what the modem PPP stack does. If it doesn't request authentication, then none will be attempted. > > My concern is for the use of the auth_method for the default LTE bearer. > In that case I would prefer to have such a method clearly identified, > because the combined attach could So isn't this a good reason to make sure provisioning sets these properly? Default attach parameters are not currently provisioned, but perhaps they need to be. > fail for various reasons, the network answer is in general > inconsistent (I have counted already > half a dozen failure codes when APN/auth are incorrect), and - mainly > - there is no immediate feedback > to the application: the registration is attempted a few times, then > there is a back-off timer of 12 minutes (T3346), > when the module could attach to a legacy technology. In all this, the > user might just believe that it is out of LTE > coverage at first, and will take a long time to associate the symptoms > with the actual issue. > > Another issue for the default context is that the parameters are in > general non-volatile, so if the SIM is changed, > and with it perhaps only the APN, it would still refer to the previous > auth_method and parameters until changed explicitly. Another reason to provision the default attach parameters. If you change the SIM and no settings are read from storage (which is stored per IMSI), then oFono should attempt to provision the default attach parameters. > And in the future, it seems the SIM will be changed quite often > And it seems to be particularly important for VoLTE, because no > operator supports it in roaming. The standard exists, > but seems unattractive. > With the sunset of 2G/3G, the only way to have voice will be using a > local SIM profile. > This SIM exchange is technical achieved with the eUICC. > > BTW, the SIM hot swap seems to be another point to improve, but for > now I am not going into this. > Out of curiosity, what do you have in mind? Regards, -Denis ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 6/6] drivers: support for auth NONE 2018-10-04 4:40 ` Denis Kenzior @ 2018-10-04 4:51 ` Giacinto Cifelli 2018-10-05 2:04 ` Denis Kenzior 0 siblings, 1 reply; 20+ messages in thread From: Giacinto Cifelli @ 2018-10-04 4:51 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 6182 bytes --] Hi Denis, On Thu, Oct 4, 2018 at 6:40 AM Denis Kenzior <denkenz@gmail.com> wrote: > > Hi Giacinto, > > > I suppose your comments on [PATCH 2/4]. I have addressed them on the > > [PATCH 2/6]. > > Basically, as you can also see from the code, the user-set value are stored, > > but the driver changes them internally if suited: > > - if no username it changes to AUTH_NONE, this is already in place for some > > - if AUTH_NONE, username and password are cleared internally > > This permissive approach allows also to deal with the next question (see below). > > All right. But do you also want to mention this behavior in the D-Bus > documentation? E.g. something to the effect of if AuthenticationMethod > is set to None, then username / password values are ignored? gladly, but I didn't find a document for this. We are going to add it in the lte-api, but the only place I have found for the rest is in the connman-api. Is there some other documents? > > > > >> Also, what are we doing with plugins/mbpi.c? That database only > >> supports pap/chap but potentially with missing username/password > >> attributes. Should we be converting these to type 'none'? Or leaving > >> things as is and having the drivers deal with converting type chap, > >> empty username/password to type none. I thought the whole idea of > >> introducing type 'None' was that you hated that drivers were doing this? > > > > I had added the 'none' also there, then removed it. It looks to me > > this database is for CDMA (but I am not sure). > > In the database itself I have on my machine, there is a single 'pap', > > no 'chap', and in all other cases the fields are missing. > > This database is for both CDMA and GSM. Essentially CHAP is assumed if > PAP is not explicitly specified. ok, then it is to be kept. > > > > > If it is for CDMA, I am not sure how much should this database be maintained. > > I know that several users have a private branch of ofono for CDMA, and > > do not intend to change or submit, > > because the technology is felt EOL. > > Mobile Broadband Provider Info is still maintained. See > https://github.com/GNOME/mobile-broadband-provider-info. The database > itself was never really that good, but it was at least something. I > think the Ubuntu guys had a plugin that used the Android provisioning > db, but it was never contributed upstream. > that's a pity. > > > > Maybe this could be removed in a 2.x branch that you mentioned apropos > > AT+CGATT=0 when roaming. > > > > In the meanwhile, I have chosen to ignore it. It is not difficult to > > add it in the code if necessary, > > and I can also considered it separately from this series of patches. > > > > Given how closely related it is, it probably should be considered in the > same series. This is one of those situations where we're messing with > the existing API and so all changes go in or none at all... But given > that the drivers are expected to handle empty username/passwords > already, I'm okay leaving mbpi as is... but it might be a good idea to > fix it anyway... I can fix it, but need to decide on a default then. It looks to me that 'none' would be a better default than 'chap'. The problem is that 'chap' is assumed so far and therefore it would be regressive. > > >> I believe you referred to this as a 'hack' or similar terminology? :) > > > > undocumented hack, yes :) > > it is the undocumented part that I don't like. For this db, for > > example, one may assume that > > without user/pwd it is no-auth. Which is true for all drivers except > > PPP, that would > > default to chap and attempt to generate a password, as you described 2 days ago. > > That actually depends on what the modem PPP stack does. If it doesn't > request authentication, then none will be attempted. > > > > > My concern is for the use of the auth_method for the default LTE bearer. > > In that case I would prefer to have such a method clearly identified, > > because the combined attach could > > So isn't this a good reason to make sure provisioning sets these > properly? Default attach parameters are not currently provisioned, but > perhaps they need to be. > > > fail for various reasons, the network answer is in general > > inconsistent (I have counted already > > half a dozen failure codes when APN/auth are incorrect), and - mainly > > - there is no immediate feedback > > to the application: the registration is attempted a few times, then > > there is a back-off timer of 12 minutes (T3346), > > when the module could attach to a legacy technology. In all this, the > > user might just believe that it is out of LTE > > coverage at first, and will take a long time to associate the symptoms > > with the actual issue. > > > > Another issue for the default context is that the parameters are in > > general non-volatile, so if the SIM is changed, > > and with it perhaps only the APN, it would still refer to the previous > > auth_method and parameters until changed explicitly. > > Another reason to provision the default attach parameters. If you > change the SIM and no settings are read from storage (which is stored > per IMSI), then oFono should attempt to provision the default attach > parameters. > ok for all the points above. So I change the default for 'none', is this ok? > > And in the future, it seems the SIM will be changed quite often > > And it seems to be particularly important for VoLTE, because no > > operator supports it in roaming. The standard exists, > > but seems unattractive. > > With the sunset of 2G/3G, the only way to have voice will be using a > > local SIM profile. > > This SIM exchange is technical achieved with the eUICC. > > > > BTW, the SIM hot swap seems to be another point to improve, but for > > now I am not going into this. > > > > Out of curiosity, what do you have in mind? I haven't looked into it yet. I just have complaints that removing and re-inserting the SIM doesn't work. We will work on this in November or December. > > Regards, > -Denis Regards, Giacinto ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 6/6] drivers: support for auth NONE 2018-10-04 4:51 ` Giacinto Cifelli @ 2018-10-05 2:04 ` Denis Kenzior 2018-10-05 2:07 ` Giacinto Cifelli 0 siblings, 1 reply; 20+ messages in thread From: Denis Kenzior @ 2018-10-05 2:04 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 1187 bytes --] Hi Giacinto, >> All right. But do you also want to mention this behavior in the D-Bus >> documentation? E.g. something to the effect of if AuthenticationMethod >> is set to None, then username / password values are ignored? > > gladly, but I didn't find a document for this. > We are going to add it in the lte-api, but the only place I have found > for the rest is in the > connman-api. > Is there some other documents? > I don't believe so. >> Given how closely related it is, it probably should be considered in the >> same series. This is one of those situations where we're messing with >> the existing API and so all changes go in or none at all... But given >> that the drivers are expected to handle empty username/passwords >> already, I'm okay leaving mbpi as is... but it might be a good idea to >> fix it anyway... > > I can fix it, but need to decide on a default then. > It looks to me that 'none' would be a better default than 'chap'. > The problem is that 'chap' is assumed so far and therefore it would be > regressive. > That's what I would do. Use none if no password/username/auth method are provided. Regards, -Denis ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 6/6] drivers: support for auth NONE 2018-10-05 2:04 ` Denis Kenzior @ 2018-10-05 2:07 ` Giacinto Cifelli 0 siblings, 0 replies; 20+ messages in thread From: Giacinto Cifelli @ 2018-10-05 2:07 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 1451 bytes --] Hi Denis, On Fri, Oct 5, 2018 at 4:04 AM Denis Kenzior <denkenz@gmail.com> wrote: > > Hi Giacinto, > > >> All right. But do you also want to mention this behavior in the D-Bus > >> documentation? E.g. something to the effect of if AuthenticationMethod > >> is set to None, then username / password values are ignored? > > > > gladly, but I didn't find a document for this. > > We are going to add it in the lte-api, but the only place I have found > > for the rest is in the > > connman-api. > > Is there some other documents? > > > > I don't believe so. Shall I prepare a new document gprs-context? > > >> Given how closely related it is, it probably should be considered in the > >> same series. This is one of those situations where we're messing with > >> the existing API and so all changes go in or none at all... But given > >> that the drivers are expected to handle empty username/passwords > >> already, I'm okay leaving mbpi as is... but it might be a good idea to > >> fix it anyway... > > > > I can fix it, but need to decide on a default then. > > It looks to me that 'none' would be a better default than 'chap'. > > The problem is that 'chap' is assumed so far and therefore it would be > > regressive. > > > > That's what I would do. Use none if no password/username/auth method > are provided. OK. Please look at what I have submitted. > > Regards, > -Denis Regards, Giacinto ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 6/6] drivers: support for auth NONE 2018-10-03 21:29 ` Denis Kenzior 2018-10-04 3:44 ` Giacinto Cifelli @ 2018-10-04 4:43 ` Giacinto Cifelli 2018-10-05 2:20 ` Denis Kenzior 1 sibling, 1 reply; 20+ messages in thread From: Giacinto Cifelli @ 2018-10-04 4:43 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 19104 bytes --] Hi Denis, second answer, specifically on the code. On Wed, Oct 3, 2018 at 11:29 PM Denis Kenzior <denkenz@gmail.com> wrote: > > Hi Giacinto, > > On 10/02/2018 01:26 AM, Giacinto Cifelli wrote: > > Added the explicit support for auth NONE. > > It is added in all drivers/*/gprs-context.c atoms that support > > authentication. > > > > The support for no-authentication case is already present in all > > drivers. This patch allows to use it explicitly. > > Note that the swmodem driver does not have any authentication > > concept: no changes. > > > > Additionally, the behavior is left unchanged in case of inconsistent > > parameters: if username is empty, then fallback to auth NONE. > > --- > > drivers/atmodem/gprs-context.c | 20 ++++++++++++++++---- > > drivers/ifxmodem/gprs-context.c | 6 ++++++ > > drivers/isimodem/gprs-context.c | 5 +++++ > > drivers/mbimmodem/gprs-context.c | 16 +++++++++++++--- > > drivers/mbmmodem/gprs-context.c | 11 ++++++----- > > drivers/qmimodem/gprs-context.c | 13 +++++++++---- > > drivers/rilmodem/gprs-context.c | 13 +++++++++---- > > drivers/stemodem/gprs-context.c | 9 +++++---- > > drivers/telitmodem/gprs-context-ncm.c | 10 ++++++++-- > > drivers/ubloxmodem/gprs-context.c | 7 ++++--- > > 10 files changed, 81 insertions(+), 29 deletions(-) > > > > diff --git a/drivers/atmodem/gprs-context.c b/drivers/atmodem/gprs-context.c > > index 79ac4c8e..d53fd1d8 100644 > > --- a/drivers/atmodem/gprs-context.c > > +++ b/drivers/atmodem/gprs-context.c > > @@ -158,7 +158,12 @@ static gboolean setup_ppp(struct ofono_gprs_context *gc) > > g_at_ppp_set_debug(gcd->ppp, ppp_debug, "PPP"); > > > > g_at_ppp_set_auth_method(gcd->ppp, gcd->auth_method); > > - g_at_ppp_set_credentials(gcd->ppp, gcd->username, gcd->password); > > + > > + if (gcd->auth_method != G_AT_PPP_AUTH_METHOD_NONE) > > + g_at_ppp_set_credentials(gcd->ppp, gcd->username, > > + gcd->password); > > + else > > + g_at_ppp_set_credentials(gcd->ppp, NULL, NULL); > > This else part seems unneeded ok > > > > > /* set connect and disconnect callbacks */ > > g_at_ppp_set_connect_function(gcd->ppp, ppp_connect, gc); > > @@ -247,7 +252,7 @@ static void at_gprs_activate_primary(struct ofono_gprs_context *gc, > > memcpy(gcd->username, ctx->username, sizeof(ctx->username)); > > memcpy(gcd->password, ctx->password, sizeof(ctx->password)); > > > > - /* We only support CHAP and PAP */ > > + /* We support CHAP, PAP and NONE */ > > switch (ctx->auth_method) { > > case OFONO_GPRS_AUTH_METHOD_CHAP: > > gcd->auth_method = G_AT_PPP_AUTH_METHOD_CHAP; > > @@ -255,8 +260,11 @@ static void at_gprs_activate_primary(struct ofono_gprs_context *gc, > > case OFONO_GPRS_AUTH_METHOD_PAP: > > gcd->auth_method = G_AT_PPP_AUTH_METHOD_PAP; > > break; > > - default: > > - goto error; > > + case OFONO_GPRS_AUTH_METHOD_NONE: > > + gcd->auth_method = G_AT_PPP_AUTH_METHOD_NONE; > > + gcd->username[0] = 0; > > + gcd->password[0] = 0; > > + break; > > } > > > > gcd->state = STATE_ENABLING; > > @@ -304,6 +312,10 @@ static void at_gprs_activate_primary(struct ofono_gprs_context *gc, > > snprintf(buf + len, sizeof(buf) - len - 3, > > ",\"PAP:%s\"", ctx->apn); > > break; > > + case OFONO_GPRS_AUTH_METHOD_NONE: > > + snprintf(buf + len, sizeof(buf) - len - 3, > > + ",\"%s\"", ctx->apn); > > + break; > > } > > break; > > default: > > diff --git a/drivers/ifxmodem/gprs-context.c b/drivers/ifxmodem/gprs-context.c > > index 885e41bb..81b056cc 100644 > > --- a/drivers/ifxmodem/gprs-context.c > > +++ b/drivers/ifxmodem/gprs-context.c > > @@ -466,9 +466,15 @@ static void ifx_gprs_activate_primary(struct ofono_gprs_context *gc, > > gcd->active_context = ctx->cid; > > gcd->cb = cb; > > gcd->cb_data = data; > > + > > memcpy(gcd->username, ctx->username, sizeof(ctx->username)); > > memcpy(gcd->password, ctx->password, sizeof(ctx->password)); > > > > + if (ctx->auth_method == OFONO_GPRS_AUTH_METHOD_NONE) { > > + gcd->username[0] = 0; > > + gcd->password[0] = 0; > > + } > > + > > Ugh, I'd really prefer something like: > > if (method == NONE) { > memset(gcd->username, 0, sizeof(gcd->username)); > ... > } else { > memcpy(...); > } > ok, even if the code is comparing for username[0]. It looked sensible to change the switching variable, > > gcd->state = STATE_ENABLING; > > gcd->proto = ctx->proto; > > > > diff --git a/drivers/isimodem/gprs-context.c b/drivers/isimodem/gprs-context.c > > index ce53d022..b1c819c2 100644 > > --- a/drivers/isimodem/gprs-context.c > > +++ b/drivers/isimodem/gprs-context.c > > @@ -544,6 +544,11 @@ static void isi_gprs_activate_primary(struct ofono_gprs_context *gc, > > strncpy(cd->password, ctx->password, GPDS_MAX_PASSWORD_LENGTH); > > cd->username[GPDS_MAX_PASSWORD_LENGTH] = '\0'; > > > > + if (ctx->auth_method == OFONO_GPRS_AUTH_METHOD_NONE) { > > + cd->username[0] = 0; > > + cd->password[0] = 0; > > + } > > + > > cd->pep = g_isi_pep_create(cd->idx, NULL, NULL); > > if (cd->pep == NULL) > > goto error; > > diff --git a/drivers/mbimmodem/gprs-context.c b/drivers/mbimmodem/gprs-context.c > > index 79793c92..be256e43 100644 > > --- a/drivers/mbimmodem/gprs-context.c > > +++ b/drivers/mbimmodem/gprs-context.c > > @@ -75,9 +75,11 @@ static uint32_t auth_method_to_auth_protocol(enum ofono_gprs_auth_method method) > > return 2; /* MBIMAuthProtocolChap */ > > case OFONO_GPRS_AUTH_METHOD_PAP: > > return 1; /* MBIMAuthProtocolPap */ > > + case OFONO_GPRS_AUTH_METHOD_NONE: > > + return 0; /* MBIMAUthProtocolNone */ > > } > > > > - return 0; > > + return 0; /* MBIMAUthProtocolNone */ > > } > > > > static void mbim_deactivate_cb(struct mbim_message *message, void *user) > > @@ -345,6 +347,8 @@ static void mbim_gprs_activate_primary(struct ofono_gprs_context *gc, > > { > > struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc); > > struct mbim_message *message; > > + const char username = NULL; > > + const char password = NULL; > > > > DBG("cid %u", ctx->cid); > > > > @@ -354,6 +358,12 @@ static void mbim_gprs_activate_primary(struct ofono_gprs_context *gc, > > gcd->active_context = ctx->cid; > > gcd->proto = ctx->proto; > > > > + if (ctx->auth_method != OFONO_GPRS_AUTH_METHOD_NONE && ctx->username[0]) > > + username = ctx->username; > > + > > + if (ctx->auth_method != OFONO_GPRS_AUTH_METHOD_NONE && ctx->password[0]) > > + password = ctx->password; > > + > > message = mbim_message_new(mbim_uuid_basic_connect, > > MBIM_CID_CONNECT, > > MBIM_COMMAND_TYPE_SET); > > @@ -361,8 +371,8 @@ static void mbim_gprs_activate_primary(struct ofono_gprs_context *gc, > > ctx->cid, > > 1, /* MBIMActivationCommandActivate */ > > ctx->apn, > > - ctx->username[0] ? ctx->username : NULL, > > - ctx->password[0] ? ctx->password : NULL, > > + username, > > + password, > > 0, /*MBIMCompressionNone */ > > auth_method_to_auth_protocol(ctx->auth_method), > > proto_to_context_ip_type(ctx->proto), > > diff --git a/drivers/mbmmodem/gprs-context.c b/drivers/mbmmodem/gprs-context.c > > index e961afa1..fa8b44b6 100644 > > --- a/drivers/mbmmodem/gprs-context.c > > +++ b/drivers/mbmmodem/gprs-context.c > > @@ -394,11 +394,12 @@ static void mbm_gprs_activate_primary(struct ofono_gprs_context *gc, > > * Set username and password, this should be done after CGDCONT > > * or an error can occur. We don't bother with error checking > > * here > > - * */ > > - snprintf(buf, sizeof(buf), "AT*EIAAUW=%d,1,\"%s\",\"%s\"", > > - ctx->cid, ctx->username, ctx->password); > > - > > - g_at_chat_send(gcd->chat, buf, none_prefix, NULL, NULL, NULL); > > + */ > > + if (ctx->auth_method != OFONO_GPRS_AUTH_METHOD_NONE) { > > + snprintf(buf, sizeof(buf), "AT*EIAAUW=%d,1,\"%s\",\"%s\"", > > + ctx->cid, ctx->username, ctx->password); > > + g_at_chat_send(gcd->chat, buf, none_prefix, NULL, NULL, NULL); > > + } > > > > return; > > > > diff --git a/drivers/qmimodem/gprs-context.c b/drivers/qmimodem/gprs-context.c > > index 9a22b89f..7f31dd0e 100644 > > --- a/drivers/qmimodem/gprs-context.c > > +++ b/drivers/qmimodem/gprs-context.c > > @@ -238,7 +238,12 @@ static void qmi_activate_primary(struct ofono_gprs_context *gc, > > struct cb_data *cbd = cb_data_new(cb, user_data); > > struct qmi_param *param; > > uint8_t ip_family; > > - uint8_t auth; > > + /* > > + * set default authentication to CHAP, even if unneeded, > > + * otherwise the compiler complains that: > > + * ‘auth’ may be used uninitialized in this function > > + */ > > + uint8_t auth = QMI_WDS_AUTHENTICATION_CHAP; > > Ugh. I really hate GCC people sometimes. While the warning is strictly > correct, it is useless for 99% of the code and forces us to jump through > hoops. Anyway, I would propose we use the following pattern: > > int auth_method_to_qmi(enum ofono_auth_method method, uint8_t *out_auth) > { > switch (method) > case CHAP: > *out_auth = ... > return 0; > case: > ... > } > > return -ERANGE; > } > I tried but it is worst. Now gcc complains about potentially uninitialized variable. Here is the code static int auth_method_to_qmi(enum ofono_gprs_auth_method method, uint8_t *out_auth) { switch (method) { case OFONO_GPRS_AUTH_METHOD_CHAP: *out_auth = QMI_WDS_AUTHENTICATION_CHAP; return 0; case OFONO_GPRS_AUTH_METHOD_PAP: *out_auth = QMI_WDS_AUTHENTICATION_PAP; return 0; case OFONO_GPRS_AUTH_METHOD_NONE: *out_auth = QMI_WDS_AUTHENTICATION_NONE; return 0; } return -1; } static void qmi_activate_primary(struct ofono_gprs_context *gc, [...] uint8_t auth; [...] auth_method_to_qmi(ctx->auth_method, &auth); and here the compiler answer: drivers/qmimodem/gprs-context.c:288:2: error: ‘auth’ may be used uninitialized in this function [-Werror=maybe-uninitialized] qmi_param_append_uint8(param, QMI_WDS_PARAM_AUTHENTICATION_PREFERENCE, ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ auth); ~~~~~ It doesn't look like we have a clean way out of this. I can pre-initialize the variable, add a default to the switch, return the value for uint8 out_auth, but none of these will give you a clean solution. I prefer therefore to keep it as it is, with a comment about why it is done so. > > > > DBG("cid %u", ctx->cid); > > > > @@ -273,7 +278,7 @@ static void qmi_activate_primary(struct ofono_gprs_context *gc, > > case OFONO_GPRS_AUTH_METHOD_PAP: > > auth = QMI_WDS_AUTHENTICATION_PAP; > > break; > > - default: > > + case OFONO_GPRS_AUTH_METHOD_NONE: > > auth = QMI_WDS_AUTHENTICATION_NONE; > > break; > > } > > @@ -281,11 +286,11 @@ static void qmi_activate_primary(struct ofono_gprs_context *gc, > > qmi_param_append_uint8(param, QMI_WDS_PARAM_AUTHENTICATION_PREFERENCE, > > auth); > > > > - if (ctx->username[0] != '\0') > > + if (auth != QMI_WDS_AUTHENTICATION_NONE && ctx->username[0] != '\0') > > qmi_param_append(param, QMI_WDS_PARAM_USERNAME, > > strlen(ctx->username), ctx->username); > > > > - if (ctx->password[0] != '\0') > > + if (auth != QMI_WDS_AUTHENTICATION_NONE && ctx->password[0] != '\0') > > qmi_param_append(param, QMI_WDS_PARAM_PASSWORD, > > strlen(ctx->password), ctx->password); > > > > diff --git a/drivers/rilmodem/gprs-context.c b/drivers/rilmodem/gprs-context.c > > index 1f476e23..ef62cba9 100644 > > --- a/drivers/rilmodem/gprs-context.c > > +++ b/drivers/rilmodem/gprs-context.c > > @@ -598,9 +598,12 @@ static void ril_gprs_context_activate_primary(struct ofono_gprs_context *gc, > > * We do the same as in $AOSP/frameworks/opt/telephony/src/java/com/ > > * android/internal/telephony/dataconnection/DataConnection.java, > > * onConnect(), and use authentication or not depending on whether > > - * the user field is empty or not. > > + * the user field is empty or not, > > + * on top of the verification for the authentication method. > > */ > > - if (ctx->username[0] != '\0') > > + > > + if (ctx->auth_method != OFONO_GPRS_AUTH_METHOD_NONE && > > + ctx->username[0] != '\0') > > auth_type = RIL_AUTH_BOTH; > > else > > auth_type = RIL_AUTH_NONE; > > @@ -615,8 +618,10 @@ static void ril_gprs_context_activate_primary(struct ofono_gprs_context *gc, > > parcel_w_string(&rilp, buf); > > > > g_ril_append_print_buf(gcd->ril, "(%d,%s,%s,%s,%s,%d,%s,%u)", > > - tech, profile, ctx->apn, ctx->username, > > - ctx->password, auth_type, > > + tech, profile, ctx->apn, > > + auth_type == RIL_AUTH_NONE ? "" : ctx->username, > > + auth_type == RIL_AUTH_NONE ? "" : ctx->password, > > + auth_type, > > ril_util_gprs_proto_to_ril_string(ctx->proto), > > ctx->cid); > > } else > > diff --git a/drivers/stemodem/gprs-context.c b/drivers/stemodem/gprs-context.c > > index 18b2bfa4..32facd8c 100644 > > --- a/drivers/stemodem/gprs-context.c > > +++ b/drivers/stemodem/gprs-context.c > > @@ -307,10 +307,11 @@ static void ste_gprs_activate_primary(struct ofono_gprs_context *gc, > > * or an error can occur. We don't bother with error checking > > * here > > */ > > - snprintf(buf, sizeof(buf), "AT*EIAAUW=%d,1,\"%s\",\"%s\"", > > - ctx->cid, ctx->username, ctx->password); > > - > > - g_at_chat_send(gcd->chat, buf, none_prefix, NULL, NULL, NULL); > > + if (ctx->auth_method != OFONO_GPRS_AUTH_METHOD_NONE) { > > + snprintf(buf, sizeof(buf), "AT*EIAAUW=%d,1,\"%s\",\"%s\"", > > + ctx->cid, ctx->username, ctx->password); > > + g_at_chat_send(gcd->chat, buf, none_prefix, NULL, NULL, NULL); > > + } > > > > return; > > > > diff --git a/drivers/telitmodem/gprs-context-ncm.c b/drivers/telitmodem/gprs-context-ncm.c > > index 7139740c..9c9b9500 100644 > > --- a/drivers/telitmodem/gprs-context-ncm.c > > +++ b/drivers/telitmodem/gprs-context-ncm.c > > @@ -277,7 +277,8 @@ static void setup_cb(gboolean ok, GAtResult *result, gpointer user_data) > > return; > > } > > > > - if (gcd->username[0] && gcd->password[0]) > > + if (gcd->auth_method != AUTH_METHOD_NONE && > > + gcd->username[0] && gcd->password[0]) > > sprintf(buf, "AT#PDPAUTH=%u,%u,\"%s\",\"%s\"", > > gcd->active_context, gcd->auth_method, > > gcd->username, gcd->password); > > @@ -320,7 +321,7 @@ static void telitncm_gprs_activate_primary(struct ofono_gprs_context *gc, > > gcd->state = STATE_ENABLING; > > gcd->proto = ctx->proto; > > > > - /* We only support CHAP and PAP */ > > + /* We support CHAP, PAP and NONE */ > > switch (ctx->auth_method) { > > case OFONO_GPRS_AUTH_METHOD_CHAP: > > gcd->auth_method = AUTH_METHOD_CHAP; > > @@ -328,6 +329,11 @@ static void telitncm_gprs_activate_primary(struct ofono_gprs_context *gc, > > case OFONO_GPRS_AUTH_METHOD_PAP: > > gcd->auth_method = AUTH_METHOD_PAP; > > break; > > + case OFONO_GPRS_AUTH_METHOD_NONE: > > + gcd->auth_method = AUTH_METHOD_NONE; > > + gcd->username[0] = 0; > > + gcd->password[0] = 0; > > + break; > > default: > > goto error; > > } > > diff --git a/drivers/ubloxmodem/gprs-context.c b/drivers/ubloxmodem/gprs-context.c > > index 6fe2719f..7eb18f09 100644 > > --- a/drivers/ubloxmodem/gprs-context.c > > +++ b/drivers/ubloxmodem/gprs-context.c > > @@ -315,9 +315,10 @@ static void ublox_send_uauthreq(struct ofono_gprs_context *gc, > > case OFONO_GPRS_AUTH_METHOD_CHAP: > > auth = 2; > > break; > > - default: > > - ofono_error("Unsupported auth type %u", auth_method); > > - return; > > + case OFONO_GPRS_AUTH_METHOD_NONE: > > + auth = 0; > > + username = password = ""; > > + break; > > } > > > > snprintf(buf, sizeof(buf), "AT+UAUTHREQ=%u,%u,\"%s\",\"%s\"", > > > > Anyway, these look fine to me otherwise. But I still wouldn't mind some > explanation of how we're addressing the questions I raised in my E-mail > on Sep 27. > > Also, what are we doing with plugins/mbpi.c? That database only > supports pap/chap but potentially with missing username/password > attributes. Should we be converting these to type 'none'? Or leaving > things as is and having the drivers deal with converting type chap, > empty username/password to type none. I thought the whole idea of > introducing type 'None' was that you hated that drivers were doing this? > I believe you referred to this as a 'hack' or similar terminology? :) > > Regards, > -Denis Regards, Giacinto ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 6/6] drivers: support for auth NONE 2018-10-04 4:43 ` Giacinto Cifelli @ 2018-10-05 2:20 ` Denis Kenzior 2018-10-05 2:23 ` Giacinto Cifelli 0 siblings, 1 reply; 20+ messages in thread From: Denis Kenzior @ 2018-10-05 2:20 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 2285 bytes --] Hi Giacinto, >> Ugh. I really hate GCC people sometimes. While the warning is strictly >> correct, it is useless for 99% of the code and forces us to jump through >> hoops. Anyway, I would propose we use the following pattern: >> >> int auth_method_to_qmi(enum ofono_auth_method method, uint8_t *out_auth) >> { >> switch (method) >> case CHAP: >> *out_auth = ... >> return 0; >> case: >> ... >> } >> >> return -ERANGE; >> } >> > > I tried but it is worst. Now gcc complains about potentially > uninitialized variable. > > Here is the code > static int auth_method_to_qmi(enum ofono_gprs_auth_method method, > uint8_t *out_auth) > { > switch (method) { > case OFONO_GPRS_AUTH_METHOD_CHAP: > *out_auth = QMI_WDS_AUTHENTICATION_CHAP; > return 0; > case OFONO_GPRS_AUTH_METHOD_PAP: > *out_auth = QMI_WDS_AUTHENTICATION_PAP; > return 0; > case OFONO_GPRS_AUTH_METHOD_NONE: > *out_auth = QMI_WDS_AUTHENTICATION_NONE; > return 0; > } > return -1; > } > > static void qmi_activate_primary(struct ofono_gprs_context *gc, > [...] > uint8_t auth; > [...] > auth_method_to_qmi(ctx->auth_method, &auth); Well you might want to check for an error return here. e.g. if (auth_method_to_qmi() < 0) { CALLBACK_WITH_FAILURE(); return; } > > and here the compiler answer: > > drivers/qmimodem/gprs-context.c:288:2: error: ‘auth’ may be used > uninitialized in this function [-Werror=maybe-uninitialized] > qmi_param_append_uint8(param, QMI_WDS_PARAM_AUTHENTICATION_PREFERENCE, > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > auth); > ~~~~~ > > It doesn't look like we have a clean way out of this. I can > pre-initialize the variable, add a default to the switch, return the > value for uint8 out_auth, > but none of these will give you a clean solution. No, we should not initialize variables unnecessarily. This is even documented: doc/coding-style.txt, item M7 > > I prefer therefore to keep it as it is, with a comment about why it is done so. > I prefer we set a good example :) Regards, -Denis ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 6/6] drivers: support for auth NONE 2018-10-05 2:20 ` Denis Kenzior @ 2018-10-05 2:23 ` Giacinto Cifelli 2018-10-05 2:47 ` Denis Kenzior 0 siblings, 1 reply; 20+ messages in thread From: Giacinto Cifelli @ 2018-10-05 2:23 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 2707 bytes --] Hi Denis, On Fri, Oct 5, 2018 at 4:20 AM Denis Kenzior <denkenz@gmail.com> wrote: > > Hi Giacinto, > > >> Ugh. I really hate GCC people sometimes. While the warning is strictly > >> correct, it is useless for 99% of the code and forces us to jump through > >> hoops. Anyway, I would propose we use the following pattern: > >> > >> int auth_method_to_qmi(enum ofono_auth_method method, uint8_t *out_auth) > >> { > >> switch (method) > >> case CHAP: > >> *out_auth = ... > >> return 0; > >> case: > >> ... > >> } > >> > >> return -ERANGE; > >> } > >> > > > > I tried but it is worst. Now gcc complains about potentially > > uninitialized variable. > > > > Here is the code > > static int auth_method_to_qmi(enum ofono_gprs_auth_method method, > > uint8_t *out_auth) > > { > > switch (method) { > > case OFONO_GPRS_AUTH_METHOD_CHAP: > > *out_auth = QMI_WDS_AUTHENTICATION_CHAP; > > return 0; > > case OFONO_GPRS_AUTH_METHOD_PAP: > > *out_auth = QMI_WDS_AUTHENTICATION_PAP; > > return 0; > > case OFONO_GPRS_AUTH_METHOD_NONE: > > *out_auth = QMI_WDS_AUTHENTICATION_NONE; > > return 0; > > } > > return -1; > > } > > > > static void qmi_activate_primary(struct ofono_gprs_context *gc, > > [...] > > uint8_t auth; > > [...] > > auth_method_to_qmi(ctx->auth_method, &auth); > > Well you might want to check for an error return here. e.g. > > if (auth_method_to_qmi() < 0) { > CALLBACK_WITH_FAILURE(); > return; > } > > > > > and here the compiler answer: > > > > drivers/qmimodem/gprs-context.c:288:2: error: ‘auth’ may be used > > uninitialized in this function [-Werror=maybe-uninitialized] > > qmi_param_append_uint8(param, QMI_WDS_PARAM_AUTHENTICATION_PREFERENCE, > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > auth); > > ~~~~~ > > > > It doesn't look like we have a clean way out of this. I can > > pre-initialize the variable, add a default to the switch, return the > > value for uint8 out_auth, > > but none of these will give you a clean solution. > > No, we should not initialize variables unnecessarily. This is even > documented: doc/coding-style.txt, item M7 I know this, that's why there are 3 lines of comment on it. > > > > > I prefer therefore to keep it as it is, with a comment about why it is done so. > > > > I prefer we set a good example :) Then the cleanest way is to set a default in the switch, without any function. Ok? > > Regards, > -Denis Regards, Giacinto ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 6/6] drivers: support for auth NONE 2018-10-05 2:23 ` Giacinto Cifelli @ 2018-10-05 2:47 ` Denis Kenzior 2018-10-05 2:51 ` Giacinto Cifelli 0 siblings, 1 reply; 20+ messages in thread From: Denis Kenzior @ 2018-10-05 2:47 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 519 bytes --] Hi Giacinto, >> >> I prefer we set a good example :) > > Then the cleanest way is to set a default in the switch, without any function. > Ok? > That is not really great either. We want to make sure that the compiler warns us if we add a new enumeration and don't take care of it in the code. In fact we have a coding-style.txt entry for this as well, see item M12. If there's code that uses default labels for enums, it is likely not compliant and should probably be fixed. Regards, -Denis ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 6/6] drivers: support for auth NONE 2018-10-05 2:47 ` Denis Kenzior @ 2018-10-05 2:51 ` Giacinto Cifelli 2018-10-05 3:23 ` Denis Kenzior 0 siblings, 1 reply; 20+ messages in thread From: Giacinto Cifelli @ 2018-10-05 2:51 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 854 bytes --] Hi Denis, On Fri, Oct 5, 2018 at 4:47 AM Denis Kenzior <denkenz@gmail.com> wrote: > > Hi Giacinto, > > >> > >> I prefer we set a good example :) > > > > Then the cleanest way is to set a default in the switch, without any function. > > Ok? > > > > That is not really great either. We want to make sure that the compiler > warns us if we add a new enumeration and don't take care of it in the > code. In fact we have a coding-style.txt entry for this as well, see > item M12. If there's code that uses default labels for enums, it is > likely not compliant and should probably be fixed. > You are not giving any choice here, other than rewriting gcc. The function I have tried, and the compiler still complains. I need a way out: please decide for the lesser evil, and I'll do it. > Regards, > -Denis Regards, Giacinto ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 6/6] drivers: support for auth NONE 2018-10-05 2:51 ` Giacinto Cifelli @ 2018-10-05 3:23 ` Denis Kenzior 2018-10-05 3:30 ` Giacinto Cifelli 0 siblings, 1 reply; 20+ messages in thread From: Denis Kenzior @ 2018-10-05 3:23 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 989 bytes --] Hi Giacinto, > You are not giving any choice here, other than rewriting gcc. > The function I have tried, and the compiler still complains. > I need a way out: please decide for the lesser evil, and I'll do it. > I already gave you two way out actually. So tell me why either of the below wouldn't work: So for example, the way gprs-context.c for mbim does this is just fine. switch (method) { case CHAP: return .. case PAP: return .. } return something, anything; Since the core guarantees that method will always be a valid enumeration, the above approach is just fine and satisfies both items M7 and M12. If you want to be more paranoid, then: int auth_method_to_foo(enum ofono_gprs_auth_method method, uint32_t *out) { switch (method) { case PAP: *out = .. return 0; case CHAP: *out = .. return 0; } return -ERANGE; } uint32_t auth; if (auth_method_to_foo(method, &auth) < 0) goto error; Regards, -Denis ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 6/6] drivers: support for auth NONE 2018-10-05 3:23 ` Denis Kenzior @ 2018-10-05 3:30 ` Giacinto Cifelli 2018-10-05 3:37 ` Denis Kenzior 0 siblings, 1 reply; 20+ messages in thread From: Giacinto Cifelli @ 2018-10-05 3:30 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 2259 bytes --] Hi Denis, maybe it is only a misunderstanding, but... On Fri, Oct 5, 2018 at 5:23 AM Denis Kenzior <denkenz@gmail.com> wrote: > > Hi Giacinto, > > > You are not giving any choice here, other than rewriting gcc. > > The function I have tried, and the compiler still complains. > > I need a way out: please decide for the lesser evil, and I'll do it. > > > > I already gave you two way out actually. So tell me why either of the > below wouldn't work: > > So for example, the way gprs-context.c for mbim does this is just fine. > > switch (method) { > case CHAP: > return .. > case PAP: > return .. > } > > return something, anything; this is in a function that assigns the variable for sure: static uint32_t auth_method_to_auth_protocol(enum ofono_gprs_auth_method method) { switch (method) { case OFONO_GPRS_AUTH_METHOD_CHAP: return 2; /* MBIMAuthProtocolChap */ case OFONO_GPRS_AUTH_METHOD_PAP: return 1; /* MBIMAuthProtocolPap */ } return 0; // < see here? } that's why the compiler doesnt complain. > > Since the core guarantees that method will always be a valid > enumeration, the above approach is just fine and satisfies both items M7 > and M12. > > If you want to be more paranoid, then: > > int auth_method_to_foo(enum ofono_gprs_auth_method method, uint32_t *out) > { > switch (method) { > case PAP: > *out = .. > return 0; > case CHAP: > *out = .. > return 0; > } > > return -ERANGE; > } > > uint32_t auth; > > if (auth_method_to_foo(method, &auth) < 0) > goto error; > this doesnt work, because it could leave the variable auth unassigned, according to the compiler. Please note that the current code for qmimodem has a default already, and it is the only way the compiler doesn't complain: switch (ctx->auth_method) { case OFONO_GPRS_AUTH_METHOD_CHAP: auth = QMI_WDS_AUTHENTICATION_CHAP; break; case OFONO_GPRS_AUTH_METHOD_PAP: auth = QMI_WDS_AUTHENTICATION_PAP; break; default: // < see here? auth = QMI_WDS_AUTHENTICATION_NONE; break; } I will go for the mbim method. > Regards, > -Denis Regards, Giacinto ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 6/6] drivers: support for auth NONE 2018-10-05 3:30 ` Giacinto Cifelli @ 2018-10-05 3:37 ` Denis Kenzior 2018-10-05 3:54 ` Giacinto Cifelli 0 siblings, 1 reply; 20+ messages in thread From: Denis Kenzior @ 2018-10-05 3:37 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 1419 bytes --] Hi Giacinto, > this is in a function that assigns the variable for sure: > > static uint32_t auth_method_to_auth_protocol(enum ofono_gprs_auth_method method) > { > switch (method) { > case OFONO_GPRS_AUTH_METHOD_CHAP: > return 2; /* MBIMAuthProtocolChap */ > case OFONO_GPRS_AUTH_METHOD_PAP: > return 1; /* MBIMAuthProtocolPap */ > } > > return 0; // < see here? > } > > that's why the compiler doesnt complain. > Sure, but you also know the core will never send you a value that is not PAP/CHAP. So you know that in reality we never get to the 'return 0' part, even though the compiler might think that we could. And if we ever add a new enumeration and forget to update this code, the compiler will complain. This is what we want. If we add a default: here, compiler will not warn us, that leads to bugs. >> uint32_t auth; >> >> if (auth_method_to_foo(method, &auth) < 0) >> goto error; >> > > this doesnt work, because it could leave the variable auth unassigned, > according to the compiler. Not according to any compiler I tried. I literally tested this before sending the above email on both GCC 7.3 and GCC 8.2. So what compiler are you using? > > Please note that the current code for qmimodem has a default already, > and it is the only way the compiler doesn't complain: And that is wrong and should be fixed. Regards, -Denis ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 6/6] drivers: support for auth NONE 2018-10-05 3:37 ` Denis Kenzior @ 2018-10-05 3:54 ` Giacinto Cifelli 2018-10-05 4:09 ` Denis Kenzior 0 siblings, 1 reply; 20+ messages in thread From: Giacinto Cifelli @ 2018-10-05 3:54 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 1138 bytes --] Hi Denis, > > And if we ever add a new enumeration and forget to update this code, the > compiler will complain. This is what we want. If we add a default: > here, compiler will not warn us, that leads to bugs. in this construct yes, I perfectly agree. > > Not according to any compiler I tried. I literally tested this before > sending the above email on both GCC 7.3 and GCC 8.2. So what compiler > are you using? default ubuntu 18.04 compiler: gcc (Ubuntu 7.3.0-27ubuntu1~18.04) 7.3.0 > > > > > Please note that the current code for qmimodem has a default already, > > and it is the only way the compiler doesn't complain: > > And that is wrong and should be fixed. I will do it, in the mbim way. Even if technically this should be a different patch, in this episode I am saving the world from the implicit lack of authentication, not from the bad coding style. But the two this time go together. I will submit a new patch later today. Would you please comment on the [PATCH 4/6] too, about the plugins, so I will take care of both together? > > Regards, > -Denis Regards, Giacinto ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 6/6] drivers: support for auth NONE 2018-10-05 3:54 ` Giacinto Cifelli @ 2018-10-05 4:09 ` Denis Kenzior 2018-10-05 4:13 ` Giacinto Cifelli 0 siblings, 1 reply; 20+ messages in thread From: Denis Kenzior @ 2018-10-05 4:09 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 646 bytes --] Hi Giacinto, >> Not according to any compiler I tried. I literally tested this before >> sending the above email on both GCC 7.3 and GCC 8.2. So what compiler >> are you using? > > default ubuntu 18.04 compiler: > gcc (Ubuntu 7.3.0-27ubuntu1~18.04) 7.3.0 This was the same on my box. I bet you were just missing the if (auth_to_foo() < 0) part > I will submit a new patch later today. > Would you please comment on the [PATCH 4/6] too, about the plugins, so > I will take care of both together? > Done. And please re-submit the entire series. Otherwise I get lost what version goes with what. Regards, -Denis ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 6/6] drivers: support for auth NONE 2018-10-05 4:09 ` Denis Kenzior @ 2018-10-05 4:13 ` Giacinto Cifelli 0 siblings, 0 replies; 20+ messages in thread From: Giacinto Cifelli @ 2018-10-05 4:13 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 928 bytes --] Hi Denis, On Fri, Oct 5, 2018 at 6:09 AM Denis Kenzior <denkenz@gmail.com> wrote: > > Hi Giacinto, > > >> Not according to any compiler I tried. I literally tested this before > >> sending the above email on both GCC 7.3 and GCC 8.2. So what compiler > >> are you using? > > > > default ubuntu 18.04 compiler: > > gcc (Ubuntu 7.3.0-27ubuntu1~18.04) 7.3.0 > > This was the same on my box. I bet you were just missing the > > if (auth_to_foo() < 0) part maybe. I will go for the mbim way, in any case. > > > I will submit a new patch later today. > > Would you please comment on the [PATCH 4/6] too, about the plugins, so > > I will take care of both together? > > > > Done. And please re-submit the entire series. Otherwise I get lost > what version goes with what. Ok. The series now has 5 parts, because you have taken the 5/6 for gatchat. > > Regards, > -Denis Regards, Giacinto ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2018-10-05 4:13 UTC | newest] Thread overview: 20+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-10-04 5:05 [PATCH 4/6] plugins/provisioning and mbpi: support for auth NONE Giacinto Cifelli 2018-10-04 5:05 ` [PATCH 6/6] drivers: " Giacinto Cifelli -- strict thread matches above, loose matches on Subject: below -- 2018-10-02 6:26 [PATCH 1/6] connman-api: added "none" auth_method Giacinto Cifelli 2018-10-02 6:26 ` [PATCH 6/6] drivers: support for auth NONE Giacinto Cifelli 2018-10-03 21:29 ` Denis Kenzior 2018-10-04 3:44 ` Giacinto Cifelli 2018-10-04 4:40 ` Denis Kenzior 2018-10-04 4:51 ` Giacinto Cifelli 2018-10-05 2:04 ` Denis Kenzior 2018-10-05 2:07 ` Giacinto Cifelli 2018-10-04 4:43 ` Giacinto Cifelli 2018-10-05 2:20 ` Denis Kenzior 2018-10-05 2:23 ` Giacinto Cifelli 2018-10-05 2:47 ` Denis Kenzior 2018-10-05 2:51 ` Giacinto Cifelli 2018-10-05 3:23 ` Denis Kenzior 2018-10-05 3:30 ` Giacinto Cifelli 2018-10-05 3:37 ` Denis Kenzior 2018-10-05 3:54 ` Giacinto Cifelli 2018-10-05 4:09 ` Denis Kenzior 2018-10-05 4:13 ` Giacinto Cifelli
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox