[PATCH v2 0/2] ofono: Correct conditional in 'cm_get_contexts_reply'.

[PATCH v2 0/2] ofono: Correct conditional in 'cm_get_contexts_reply'.

[Grant Erickson]

At the appropriate moment in the oFono plugin lifecycle, it gets and
evaluates contexts from a given Cellular modem. Depending on the
HNI/MCC+MNC and the network operator, there may be one or more such
contexts, each with a different type.

Within 'cm_get_contexts_reply', each received context is iterated on,
evaluating each for the type 'internet' by calling
'add_cm_context'. If the context is not of type 'internet',
'cm_get_contexts_reply' should continue to iterate until all contexts
are exhausted or a matching context is found.

The return semantics of 'add_cm_context' are '-ENOMEM' if memory could
not be allocated, '-EINVAL' if a context is *not* of the type
'internet'; otherwise zero ('0') if a context of type 'internet' was
found.

However, the conditional logic of 'cm_get_contexts_reply' prior to
this change was:

   while (dbus_message_iter_get_arg_type(&dict) == DBUS_TYPE_STRUCT) {
       const char *context_path;

       dbus_message_iter_recurse(&dict, &entry);
       dbus_message_iter_get_basic(&entry, &context_path);

       dbus_message_iter_next(&entry);
       dbus_message_iter_recurse(&entry, &value);

       if (add_cm_context(modem, context_path, &value))
           break;

       dbus_message_iter_next(&dict);
   }

So, assuming a set of contexts from, for example Verizon, such as '[ {
vzwims, ims }, { vzwinternet, internet }, { vzwapp, wap } ]', then the
above logic will encounter '{ vzwims, ims }' on the first iteration,
evaluate that it is not of type 'internet', return '-EINVAL' from
'add_cm_context', that will satisfy the conditional and trigger the
'break' from the 'while' loop and context iteration will
terminate. This then misses the second, desired '{ vzwinternet,
internet }' context and the Cellular interface will never come up.

By changing the conditional logic to match the return semantics of
'add_cm_context' the code now works correctly, regardless of the number
of contexts iterated on or their order.

Fixes: 4b238d8590942 ("ofono: Implementation of simultaneous APNS")

Grant Erickson (2):
  ofono: Correct conditional in 'cm_get_contexts_reply'.
  ofono: Document 'add_cm_context'.

 plugins/ofono.c | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

-- 
2.45.0

[PATCH v2 1/2] ofono: Correct conditional in 'cm_get_contexts_reply'.

[Grant Erickson]

At the appropriate moment in the oFono plugin lifecycle, it gets and
evaluates contexts from a given Cellular modem. Depending on the
HNI/MCC+MNC and the network operator, there may be one or more such
contexts, each with a different type.

Within 'cm_get_contexts_reply', each received context is iterated on,
evaluating each for the type 'internet' by calling
'add_cm_context'. If the context is not of type 'internet',
'cm_get_contexts_reply' should continue to iterate until all contexts
are exhausted or a matching context is found.

The return semantics of 'add_cm_context' are '-ENOMEM' if memory could
not be allocated, '-EINVAL' if a context is *not* of the type
'internet'; otherwise zero ('0') if a context of type 'internet' was
found.

However, the conditional logic of 'cm_get_contexts_reply' prior to
this change was:

   while (dbus_message_iter_get_arg_type(&dict) == DBUS_TYPE_STRUCT) {
       const char *context_path;

       dbus_message_iter_recurse(&dict, &entry);
       dbus_message_iter_get_basic(&entry, &context_path);

       dbus_message_iter_next(&entry);
       dbus_message_iter_recurse(&entry, &value);

       if (add_cm_context(modem, context_path, &value))
           break;

       dbus_message_iter_next(&dict);
   }

So, assuming a set of contexts from, for example Verizon, such as '[ {
vzwims, ims }, { vzwinternet, internet }, { vzwapp, wap } ]', then the
above logic will encounter '{ vzwims, ims }' on the first iteration,
evaluate that it is not of type 'internet', return '-EINVAL' from
'add_cm_context', that will satisfy the conditional and trigger the
'break' from the 'while' loop and context iteration will
terminate. This then misses the second, desired '{ vzwinternet,
internet }' context and the Cellular interface will never come up.

By changing the conditional logic to match the return semantics of
'add_cm_context' the code now works correctly, regardless of the number
of contexts iterated on or their order.

Fixes: 4b238d8590942 ("ofono: Implementation of simultaneous APNS")
---
 plugins/ofono.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/plugins/ofono.c b/plugins/ofono.c
index 062905c8a3a6..6b89df3af0ae 100644
--- a/plugins/ofono.c
+++ b/plugins/ofono.c
@@ -1415,7 +1415,11 @@ static void cm_get_contexts_reply(DBusPendingCall *call, void *user_data)
 		dbus_message_iter_next(&entry);
 		dbus_message_iter_recurse(&entry, &value);
 
-		if (add_cm_context(modem, context_path, &value))
+		/*
+		 * If a context of type 'internet' is found, stop iterating;
+		 * we have the desired context.
+		 */
+		if (add_cm_context(modem, context_path, &value) == 0)
 			break;
 
 		dbus_message_iter_next(&dict);
-- 
2.45.0

[PATCH v2 2/2] ofono: Document 'add_cm_context'.

[Grant Erickson]

Add documentation to the 'add_cm_context' function.
---
 plugins/ofono.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/plugins/ofono.c b/plugins/ofono.c
index 6b89df3af0ae..17df01ff013e 100644
--- a/plugins/ofono.c
+++ b/plugins/ofono.c
@@ -1147,6 +1147,35 @@ static int set_context_ipconfig(struct network_context *context,
 	return 0;
 }
 
+/**
+ *  @brief
+ *    Attempt to add an oFono context.
+ *
+ *  This evaluates the oFono context with the specified path and
+ *  dictionary. If it finds one of type 'internet', it adds the
+ *  context to the modem context list; adds the modem to the context
+ *  hash, keyed off the context path; and, if the context has a valid
+ *  APN, the modem is attached, and has a network registration
+ *  interface, a connman network object is created for the context.
+ *
+ *  @param[in,out]  modem         A pointer to the mutable modem data
+ *                                instance associated with @a
+ *                                context_path.
+ *  @param[in]      context_path  A pointer to an immutable, null-
+ *                                terminated C string containing the
+ *                                path of the context to evaluate.
+ *  @param[in]      dict          A pointer to a D-Bus message iterator
+ *                                for the dictionary associated with
+ *                                the context to evaluate.
+ *
+ *  @retval  0        If successful.
+ *  @retval  -ENOMEM  If memory could not be allocated for a context
+ *                    instance.
+ *  @retval  -EINVAL  If the context was not of type 'internet'.
+ *
+ *  @private
+ *
+ */
 static int add_cm_context(struct modem_data *modem, const char *context_path,
 				DBusMessageIter *dict)
 {
-- 
2.45.0

Re: [PATCH v2 0/2] ofono: Correct conditional in 'cm_get_contexts_reply'.

[patchwork-bot+connman]

Hello:

This series was applied to connman.git (master)
by Denis Kenzior <denkenz@gmail.com>:

On Mon, 10 Feb 2025 07:59:47 -0800 you wrote:
> At the appropriate moment in the oFono plugin lifecycle, it gets and
> evaluates contexts from a given Cellular modem. Depending on the
> HNI/MCC+MNC and the network operator, there may be one or more such
> contexts, each with a different type.
> 
> Within 'cm_get_contexts_reply', each received context is iterated on,
> evaluating each for the type 'internet' by calling
> 'add_cm_context'. If the context is not of type 'internet',
> 'cm_get_contexts_reply' should continue to iterate until all contexts
> are exhausted or a matching context is found.
> 
> [...]

Here is the summary with links:
  - [v2,1/2] ofono: Correct conditional in 'cm_get_contexts_reply'.
    https://git.kernel.org/pub/scm/network/connman/connman.git/?id=28a7ce1595a1
  - [v2,2/2] ofono: Document 'add_cm_context'.
    https://git.kernel.org/pub/scm/network/connman/connman.git/?id=d543e126896f

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html