Hi Denis, second answer, specifically on the code.
On Wed, Oct 3, 2018 at 11:29 PM Denis Kenzior <[email protected]> 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 _______________________________________________ ofono mailing list [email protected] https://lists.ofono.org/mailman/listinfo/ofono
