11:20 p.m.
---
include/gprs-context.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/gprs-context.h b/include/gprs-context.h
index 20ca9efc..8869c12e 100644
--- a/include/gprs-context.h
+++ b/include/gprs-context.h
@@ -57,6 +57,7 @@ enum ofono_gprs_context_type {
enum ofono_gprs_auth_method {
OFONO_GPRS_AUTH_METHOD_CHAP = 0,
OFONO_GPRS_AUTH_METHOD_PAP,
+ OFONO_GPRS_AUTH_METHOD_NONE,
};
struct ofono_gprs_primary_context {
--
2.17.1
11:20 p.m.
New subject: [PATCH 2/4] src/gprs: support for NONE auth
---
src/gprs.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/src/gprs.c b/src/gprs.c
index 79fafdbc..235c8884 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -261,6 +261,10 @@ static const char *gprs_auth_method_to_string(enum
ofono_gprs_auth_method auth)
return "chap";
case OFONO_GPRS_AUTH_METHOD_PAP:
return "pap";
+ case OFONO_GPRS_AUTH_METHOD_NONE:
+ return "none";
+ default:
+ return NULL;
};
return NULL;
@@ -275,6 +279,9 @@ static gboolean gprs_auth_method_from_string(const char *str,
} else if (g_str_equal(str, "pap")) {
*auth = OFONO_GPRS_AUTH_METHOD_PAP;
return TRUE;
+ } else if (g_str_equal(str, "none")) {
+ *auth = OFONO_GPRS_AUTH_METHOD_NONE;
+ return TRUE;
}
return FALSE;
--
2.17.1
2:28 a.m.
New subject: [PATCH 2/4] src/gprs: support for NONE auth
Hi Giacinto,
On 09/25/2018 08:20 AM, Giacinto Cifelli wrote:
---
src/gprs.c | 7 +++++++
1 file changed, 7 insertions(+)
I don't believe this patch is complete. It doesn't address certain
issues that arise from the introduction of this auth method. For
example, what happens if a context with:
AuthenticationMethod=Chap
Username=foo
Password=bar
gets AuthenticationMethod property set to 'None'? Do Username &
Password remain untouched? Are they changed to empty automatically?
What happens now to the drivers if such a context is activated? Some of
them can handle this because they only look at the presence of
Username/Password. But others will try to set authentication method to
None yet still append username / password...
What happens if
AuthenticationMethod=None and the client tries to set
Username to 'foo'? Is that an error? Success?
All of these issues need to be explained in your patch description...
Regards,
-Denis
11:20 p.m.
New subject: [PATCH 3/4] plugins/file-provisioning.c: support for NONE auth
---
plugins/file-provision.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/plugins/file-provision.c b/plugins/file-provision.c
index d4846a65..9c4a02d1 100644
--- a/plugins/file-provision.c
+++ b/plugins/file-provision.c
@@ -104,6 +104,9 @@ static int config_file_provision_get_settings(const char *mcc,
else if (g_strcmp0(value, "pap") == 0)
(*settings)[0].auth_method =
OFONO_GPRS_AUTH_METHOD_PAP;
+ else if (g_strcmp0(value, "none") == 0)
+ (*settings)[0].auth_method =
+ OFONO_GPRS_AUTH_METHOD_NONE;
else
DBG("Unknown auth method: %s", value);
--
2.17.1
11:20 p.m.
New subject: [PATCH 4/4] drivers: default gprs authentication
Default value for gprs authentication method:
for most of the atoms, there is already a default, that is either
no authentication or error.
In case of error (only PAP and CHAP supported), this is unchanged, like
for atmodem PPP. This legacy code will run as-is today, defaulting later
to no authentication if username or password are empty.
In case of default to no authentication, the new value
OFONO_GPRS_AUTH_METHOD_NONE is added to make this explicit.
It turns out that only the atmodem (PPP) and the mbimmodem need to have
default value in the switch/case for selecting the method.
For the atmodem, it is in the ublox vendor logic, and the fix removes
the method prefix in the APN name. According to ublox specification,
it is the right way to do it.
For the mbim modem, NONE is supported, it just needs to be added to the
switch/case.
There are 3 more drivers containing a gprs-context atom, which have a
default behavior as follow:
error for: telitmodem, ubloxmodem,
no authentication for: qmimodem
---
drivers/atmodem/gprs-context.c | 4 ++++
drivers/mbimmodem/gprs-context.c | 2 ++
2 files changed, 6 insertions(+)
diff --git a/drivers/atmodem/gprs-context.c b/drivers/atmodem/gprs-context.c
index 79ac4c8e..b10777a5 100644
--- a/drivers/atmodem/gprs-context.c
+++ b/drivers/atmodem/gprs-context.c
@@ -304,6 +304,10 @@ static void at_gprs_activate_primary(struct ofono_gprs_context *gc,
snprintf(buf + len, sizeof(buf) - len - 3,
",\"PAP:%s\"", ctx->apn);
break;
+ default:
+ snprintf(buf + len, sizeof(buf) - len - 3,
+ ",\"%s\"", ctx->apn);
+ break;
}
break;
default:
diff --git a/drivers/mbimmodem/gprs-context.c b/drivers/mbimmodem/gprs-context.c
index 79793c92..ce33b927 100644
--- a/drivers/mbimmodem/gprs-context.c
+++ b/drivers/mbimmodem/gprs-context.c
@@ -75,6 +75,8 @@ 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 */
+ default:
+ return 0;
}
return 0;
--
2.17.1
2:32 a.m.
New subject: [PATCH 4/4] drivers: default gprs authentication
Hi Giacinto,
On 09/25/2018 08:20 AM, Giacinto Cifelli wrote:
Default value for gprs authentication method:
for most of the atoms, there is already a default, that is either
no authentication or error.
In case of error (only PAP and CHAP supported), this is unchanged, like
for atmodem PPP. This legacy code will run as-is today, defaulting later
to no authentication if username or password are empty.
In case of default to no authentication, the new value
OFONO_GPRS_AUTH_METHOD_NONE is added to make this explicit.
I'm lost now. Your patch results in atmodem driver failing the context
activation if auth_method=None is used. How is this good?
It turns out that only the atmodem (PPP) and the mbimmodem need to have
default value in the switch/case for selecting the method.
For the atmodem, it is in the ublox vendor logic, and the fix removes
the method prefix in the APN name. According to ublox specification,
it is the right way to do it.
For the mbim modem, NONE is supported, it just needs to be added to the
switch/case.
There are 3 more drivers containing a gprs-context atom, which have a
default behavior as follow:
error for: telitmodem, ubloxmodem,
no authentication for: qmimodem
---
drivers/atmodem/gprs-context.c | 4 ++++
drivers/mbimmodem/gprs-context.c | 2 ++
2 files changed, 6 insertions(+)
diff --git a/drivers/atmodem/gprs-context.c b/drivers/atmodem/gprs-context.c
index 79ac4c8e..b10777a5 100644
--- a/drivers/atmodem/gprs-context.c
+++ b/drivers/atmodem/gprs-context.c
@@ -304,6 +304,10 @@ static void at_gprs_activate_primary(struct ofono_gprs_context *gc,
snprintf(buf + len, sizeof(buf) - len - 3,
",\"PAP:%s\"", ctx->apn);
break;
+ default:
+ snprintf(buf + len, sizeof(buf) - len - 3,
+ ",\"%s\"", ctx->apn);
+ break;
Actually please avoid 'default' statements for well defined
enumerations. The compiler will warn us if a given enumeration is not
handled.
}
break;
default:
diff --git a/drivers/mbimmodem/gprs-context.c b/drivers/mbimmodem/gprs-context.c
index 79793c92..ce33b927 100644
--- a/drivers/mbimmodem/gprs-context.c
+++ b/drivers/mbimmodem/gprs-context.c
@@ -75,6 +75,8 @@ 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 */
+ default:
+ return 0;
As above.
}
return 0;
Regards,
-Denis
1452
days inactive
1454
days old
5 comments
2 participants
participants (2)
-
Denis Kenzior -
Giacinto Cifelli