2:50 a.m.
From: Sjur Brændeland <sjur.brandeland(a)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
2:50 a.m.
New subject: [PATCH 2/2] Fix: Check if interface exists before creating it.
From: Sjur Brændeland <sjur.brandeland(a)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
3:03 a.m.
New subject: [PATCH 2/2] Fix: Check if interface exists before creating it.
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
7:40 a.m.
New subject: [PATCH 2/2] Fix: Check if interface exists before creating it.
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
8:40 a.m.
New subject: [PATCH 2/2] Fix: Check if interface exists before creating it.
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
3:09 a.m.
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
4:23 a.m.
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
6:10 a.m.
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
6:20 a.m.
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
8:41 a.m.
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
4612
days inactive
4612
days old
9 comments
4 participants
participants (4)
-
Denis Kenzior -
Marcel Holtmann -
Sjur Brændeland -
sjur.brandeland＠stericsson.com