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

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

[sjur.brandeland]

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

From: Sjur Brændeland <sjur.brandeland@stericsson.com>

---
 drivers/stemodem/gprs-context.c |   14 ++++++--------
 1 files changed, 6 insertions(+), 8 deletions(-)

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;
-- 
1.6.3.3

[PATCH 2/2] Fix: Check if interface exists before creating it.

[sjur.brandeland]

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

From: Sjur Brændeland <sjur.brandeland@stericsson.com>

---
 drivers/stemodem/gprs-context.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/stemodem/gprs-context.c b/drivers/stemodem/gprs-context.c
index c178fb6..79e697b 100644
--- a/drivers/stemodem/gprs-context.c
+++ b/drivers/stemodem/gprs-context.c
@@ -187,9 +187,11 @@ static gboolean caif_if_create(const char *interface, unsigned int connid)
 		return FALSE;
 	}
 
-	if (ioctl(s, SIOCCAIFNETNEW, &ifr) < 0) {
-		ofono_debug("Failed to create IP interface for CAIF");
-		return FALSE;
+	if (ioctl(s, SIOCGIFINDEX, &ifr) != 0) {
+		if (ioctl(s, SIOCCAIFNETNEW, &ifr) < 0) {
+			ofono_debug("Failed to create IP interface for CAIF");
+			return FALSE;
+		}
 	}
 
 	return TRUE;
-- 
1.6.3.3

Re: [PATCH 2/2] Fix: Check if interface exists before creating it.

[Marcel Holtmann]

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

Hi Sjur,

> diff --git a/drivers/stemodem/gprs-context.c b/drivers/stemodem/gprs-context.c
> index c178fb6..79e697b 100644
> --- a/drivers/stemodem/gprs-context.c
> +++ b/drivers/stemodem/gprs-context.c
> @@ -187,9 +187,11 @@ static gboolean caif_if_create(const char *interface, unsigned int connid)
>  		return FALSE;
>  	}
>  
> -	if (ioctl(s, SIOCCAIFNETNEW, &ifr) < 0) {
> -		ofono_debug("Failed to create IP interface for CAIF");
> -		return FALSE;
> +	if (ioctl(s, SIOCGIFINDEX, &ifr) != 0) {
> +		if (ioctl(s, SIOCCAIFNETNEW, &ifr) < 0) {
> +			ofono_debug("Failed to create IP interface for CAIF");
> +			return FALSE;
> +		}
>  	}
>  
>  	return TRUE;

just doing it like this would be better:

	/* Check if interface exists */
	if (ioctl(s, SIOCGIFINDEX, &ifr) == 0)
		return TRUE;

	if (ioctl(s, SIOCCAIFNETNEW, &ifr) < 0) {
		...
		return FALSE;
	}

	return TRUE;

We are not a big fan complicated if clauses if they can be avoid with a
simple goto or just a return.

Also I think we need to check errno value here. Since potentially the
ioctl can fail for other reasons. And maybe extending CAIF with a proper
way of checking for an existing interface might be better.

Regards

Marcel

RE: [PATCH 2/2] Fix: Check if interface exists before creating it.

[Sjur =?unknown-8bit?q?Br=C3=A6ndeland?=]

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

Hi Marcel.

Marcel Holtmann wrote:
> Hi Sjur,
> 
>> diff --git a/drivers/stemodem/gprs-context.c
>> b/drivers/stemodem/gprs-context.c index c178fb6..79e697b 100644
>> --- a/drivers/stemodem/gprs-context.c
>> +++ b/drivers/stemodem/gprs-context.c
>> @@ -187,9 +187,11 @@ static gboolean caif_if_create(const char
>>  	*interface, unsigned int connid)  		return FALSE; }
>> 
>> -	if (ioctl(s, SIOCCAIFNETNEW, &ifr) < 0) {
>> -		ofono_debug("Failed to create IP interface for CAIF");
>> -		return FALSE;
>> +	if (ioctl(s, SIOCGIFINDEX, &ifr) != 0) {
>> +		if (ioctl(s, SIOCCAIFNETNEW, &ifr) < 0) {
>> +			ofono_debug("Failed to create IP interface for CAIF");
>> +			return FALSE; +		}
>>  	}
>> 
>>  	return TRUE;
> 
> just doing it like this would be better:
> 
> 	/* Check if interface exists */
> 	if (ioctl(s, SIOCGIFINDEX, &ifr) == 0)
> 		return TRUE;
> 
> 	if (ioctl(s, SIOCCAIFNETNEW, &ifr) < 0) {
> 		...
> 		return FALSE;
> 	}
> 
> 	return TRUE;
> 
> We are not a big fan complicated if clauses if they can be avoid with
> a simple goto or just a return. 
> 
> Also I think we need to check errno value here. Since potentially the
> ioctl can fail for other reasons. And maybe extending CAIF with a
> proper way of checking for an existing interface might be better.  

Do you have anything particular in mind here? 
Maybe CAIF could simply return EEXIST from SIOCCAIFNETNEW if the interface
already exists? In this case we could return TRUE anyway.

BR/Sjur

RE: [PATCH 2/2] Fix: Check if interface exists before creating it.

[Marcel Holtmann]

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

Hi Sjur,

> >> diff --git a/drivers/stemodem/gprs-context.c
> >> b/drivers/stemodem/gprs-context.c index c178fb6..79e697b 100644
> >> --- a/drivers/stemodem/gprs-context.c
> >> +++ b/drivers/stemodem/gprs-context.c
> >> @@ -187,9 +187,11 @@ static gboolean caif_if_create(const char
> >>  	*interface, unsigned int connid)  		return FALSE; }
> >> 
> >> -	if (ioctl(s, SIOCCAIFNETNEW, &ifr) < 0) {
> >> -		ofono_debug("Failed to create IP interface for CAIF");
> >> -		return FALSE;
> >> +	if (ioctl(s, SIOCGIFINDEX, &ifr) != 0) {
> >> +		if (ioctl(s, SIOCCAIFNETNEW, &ifr) < 0) {
> >> +			ofono_debug("Failed to create IP interface for CAIF");
> >> +			return FALSE; +		}
> >>  	}
> >> 
> >>  	return TRUE;
> > 
> > just doing it like this would be better:
> > 
> > 	/* Check if interface exists */
> > 	if (ioctl(s, SIOCGIFINDEX, &ifr) == 0)
> > 		return TRUE;
> > 
> > 	if (ioctl(s, SIOCCAIFNETNEW, &ifr) < 0) {
> > 		...
> > 		return FALSE;
> > 	}
> > 
> > 	return TRUE;
> > 
> > We are not a big fan complicated if clauses if they can be avoid with
> > a simple goto or just a return. 
> > 
> > Also I think we need to check errno value here. Since potentially the
> > ioctl can fail for other reasons. And maybe extending CAIF with a
> > proper way of checking for an existing interface might be better.  
> 
> Do you have anything particular in mind here? 
> Maybe CAIF could simply return EEXIST from SIOCCAIFNETNEW if the interface
> already exists? In this case we could return TRUE anyway.

if it isn't that doing already, then it should be doing it. And yes,
just returning TRUE in that case seems fine to me.

Regards

Marcel

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

[Marcel Holtmann]

[-- 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

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

[Sjur =?unknown-8bit?q?Br=C3=A6ndeland?=]

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

Hi Marcel.

Marcel Holtmann wrote:
>>  	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.  

We originally did send EIAAUW in the callback, but Denis changed this 
because MBM did it EIAAUW before CGDCONT.

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

When testing this EIAAUW fails if there are no prior PDP context on the modem.
Most likely this is the case with the MBM module as well, but I have not tested this.

BR/Sjur

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

[Marcel Holtmann]

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

Hi Sjur,

> >>  	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.  
> 
> We originally did send EIAAUW in the callback, but Denis changed this 
> because MBM did it EIAAUW before CGDCONT.

I know that Denis changed it and it makes sense to keep this in sync. We
just don't know about these details. And we have to put comments in the
source code to remind us.

And to be honest, I am not sure if anybody really tested it. I only know
of one public network that requires username/password for their APN. I
need to get a SIM card from them once I am back in Germany.

> > And we might wanna check if MBM cards behave similar and ensure that
> > STE and MBM cards use a similar code flow. 
> 
> When testing this EIAAUW fails if there are no prior PDP context on the modem.
> Most likely this is the case with the MBM module as well, but I have not tested this.

Then we have to do that in the callback in an extra step. I don't really
like it, but seems the way to go. Please send a patch that also changes
this for MBM devices. We really wanna keep these in sync.

Regards

Marcel

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

[Denis Kenzior]

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

Hi Marcel,

> > > And we might wanna check if MBM cards behave similar and ensure that
> > > STE and MBM cards use a similar code flow.
> >
> > When testing this EIAAUW fails if there are no prior PDP context on the
> > modem. Most likely this is the case with the MBM module as well, but I
> > have not tested this.
> 
> Then we have to do that in the callback in an extra step. I don't really
> like it, but seems the way to go. Please send a patch that also changes
> this for MBM devices. We really wanna keep these in sync.

Actually the easiest way to do this is simply queue the EIAAUW after the 
CGDCONT in activate_context.  If CGDCONT fails then EIAAUW won't have any 
effect anyway (but will get executed nevertheless).  This is better than 
worrying about strduping username/password and freeing it in all possible code 
paths.

Regards,
-Denis

> 
> Regards
> 
> Marcel
> 

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

[Marcel Holtmann]

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

Hi Denis,

> > > > And we might wanna check if MBM cards behave similar and ensure that
> > > > STE and MBM cards use a similar code flow.
> > >
> > > When testing this EIAAUW fails if there are no prior PDP context on the
> > > modem. Most likely this is the case with the MBM module as well, but I
> > > have not tested this.
> > 
> > Then we have to do that in the callback in an extra step. I don't really
> > like it, but seems the way to go. Please send a patch that also changes
> > this for MBM devices. We really wanna keep these in sync.
> 
> Actually the easiest way to do this is simply queue the EIAAUW after the 
> CGDCONT in activate_context.  If CGDCONT fails then EIAAUW won't have any 
> effect anyway (but will get executed nevertheless).  This is better than 
> worrying about strduping username/password and freeing it in all possible code 
> paths.

sounds good enough to me. Just wanna have STE and MBM in sync here. And
of course an extra comment that explain why we do it.

Sjur, care to send a patch?

Regards

Marcel