Re: [PATCH 1/2] Fix: Username and Password must be set after context is created.

[Marcel Holtmann <marcel@holtmann.org>]

[-- Attachment #1: Type: text/plain, Size: 1878 bytes --]

Hi Sjur,

> diff --git a/drivers/stemodem/gprs-context.c b/drivers/stemodem/gprs-context.c
> index 6743aba..c178fb6 100644
> --- a/drivers/stemodem/gprs-context.c
> +++ b/drivers/stemodem/gprs-context.c
> @@ -416,7 +416,7 @@ static void ste_gprs_activate_primary(struct ofono_gprs_context *gc,
>  {
>  	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
>  	struct cb_data *cbd = cb_data_new(cb, data);
> -	char buf[AUTH_BUF_LENGTH];
> +	char buf[2*AUTH_BUF_LENGTH];
>  	int len;
>  
>  	if (!cbd)
> @@ -425,19 +425,17 @@ static void ste_gprs_activate_primary(struct ofono_gprs_context *gc,
>  	gcd->active_context = ctx->cid;
>  	cbd->user = gc;
>  
> -	/* Set username and password */
> -	sprintf(buf, "AT*EIAAUW=%d,1,\"%s\",\"%s\"", ctx->cid,
> -		ctx->username, ctx->password);
> -
> -	if (g_at_chat_send(gcd->chat, buf, none_prefix, NULL, NULL, NULL) == 0)
> -		goto error;
> -
>  	len = sprintf(buf, "AT+CGDCONT=%u,\"IP\"", ctx->cid);
>  
>  	if (ctx->apn)
>  		snprintf(buf + len, sizeof(buf) - len - 3, ",\"%s\"",
>  				ctx->apn);
>  
> +	/* Set username and password. Must be done after context creation */
> +	len = strlen(buf);
> +	sprintf(buf+len, ";*EIAAUW=%d,1,\"%s\",\"%s\"", ctx->cid,
> +		ctx->username, ctx->password);
> +
>  	if (g_at_chat_send(gcd->chat, buf, none_prefix,
>  				ste_cgdcont_cb, cbd, g_free) > 0)
>  		return;

this looks pretty much complicated and I prefer we don't use this crazy
concat of AT commands. Also it violates the coding style.

There is no problem to just use g_at_chat_send twice since it will queue
the commands for you properly. However if this really depends on CGDCONT
succeeding, then we better do it in the callback.

And we might wanna check if MBM cards behave similar and ensure that STE
and MBM cards use a similar code flow.

Regards

Marcel