Merge lp:~awe/phablet-extras/ofono-lp1204683 into lp:phablet-extras/ofono

Proposed by Tony Espy on 2013-08-01
Status: Merged
Approved by: Ricardo Salveti on 2013-08-05
Approved revision: 53
Merged at revision: 51
Proposed branch: lp:~awe/phablet-extras/ofono-lp1204683
Merge into: lp:phablet-extras/ofono
Diff against target: 66 lines (+32/-8)
2 files modified
debian/changelog (+7/-0)
plugins/provision.c (+25/-8)
To merge this branch: bzr merge lp:~awe/phablet-extras/ofono-lp1204683
Reviewer Review Type Date Requested Status
Ricardo Salveti 2013-08-01 Approve on 2013-08-05
PS Jenkins bot continuous-integration Approve on 2013-08-05
Review via email: mp+178137@code.launchpad.net

Commit message

[provision] If multiple APNs are found for a SPN/mmc/mnc search, only provision the first.

Description of the change

This fix causes the provisioning plugin to only create a data context for the first APN found in mbpi.

This is a temporary solution to prevent network-manager from creating and trying multiple contexts when only one of them is needed.

Note, this might break auto-provisioning for some people if the first APN found is not the correct APN, however the true fix is to switch to a better source of APNs.

To test, you need first need to ensure that your operator has more than one APN defined in /usr/share/mobile-broadband-provider-info/serviceproviders.xml. If not, then edit the file manually and add one or more APNs for your operator. Ensure that these data contexts have been created using the ofono script 'list-contexts'.

Now, stop ofono and NM, edit the gprs settings file ( /var/lib/ofono/<IMSI>/gprs ) and remove all the contexts. Install the ofono produced from this merge, and reboot. Verify that only one APN/context has been created.

To post a comment you must log in.
Ricardo Salveti (rsalveti) wrote :

30 + if ( ap_count > 1)

Mind removing extra space between '(' and 'ap_count'?

Also, would be nice to change the DBG message "Found %d APs" to also say we're just getting the first AP.

42 + if (i == 0)
43 + memcpy(*settings + i, ap,
44 + sizeof(struct ofono_gprs_provision_data));

Mind adding a comment in there before 'i == 0' explaining why we're only copying the first provisioning data? (small one, as you explained on a previous code block).

Would also be nice to remove the extra DGB for the other APNs, as it would confuse folks trying to debug a possible APN issue (you could remove the DBG lines and only get that for the one you copied over to 'settings').

review: Needs Fixing
53. By Tony Espy on 2013-08-05

[provision] Address review comments.

Ricardo Salveti (rsalveti) wrote :

Looks good, and tested with maguro, worked as expected.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'debian/changelog'
2--- debian/changelog 2013-07-30 15:56:40 +0000
3+++ debian/changelog 2013-08-05 17:26:26 +0000
4@@ -1,3 +1,10 @@
5+ofono (1.12phablet12) saucy; urgency=low
6+
7+ * plugins/provision.c: if multiple "internet" data contexts
8+ are found, only provision the first (LP:1204683).
9+
10+ -- Tony Espy <espy@canonical.com> Thu, 01 Aug 2013 12:13:37 -0400
11+
12 ofono (1.12phablet11) saucy; urgency=low
13
14 * drivers/rilmodem/gprs.c: adding data network state tracking back, but
15
16=== modified file 'plugins/provision.c'
17--- plugins/provision.c 2013-05-24 22:11:34 +0000
18+++ plugins/provision.c 2013-08-05 17:26:26 +0000
19@@ -67,7 +67,18 @@
20
21 ap_count = g_slist_length(apns);
22
23- DBG("Found %d APs", ap_count);
24+ ofono_info("GPRS Provisioning found %d matching APNs for "
25+ "SPN: %s MCC: %d MNC: %d",
26+ ap_count, spn, mcc, mnc);
27+ /*
28+ * Only keep the first APN found.
29+ *
30+ * This allows auto-provisioning to work most of the time vs.
31+ * passing FALSE to mbpi_lookup_apn() which would return an
32+ * an empty list if duplicates are found.
33+ */
34+ if (ap_count > 1)
35+ ap_count = 1;
36
37 *settings = g_try_new0(struct ofono_gprs_provision_data, ap_count);
38 if (*settings == NULL) {
39@@ -86,14 +97,20 @@
40 for (l = apns, i = 0; l; l = l->next, i++) {
41 struct ofono_gprs_provision_data *ap = l->data;
42
43- DBG("Name: '%s'", ap->name);
44- DBG("APN: '%s'", ap->apn);
45- DBG("Type: %s", mbpi_ap_type(ap->type));
46- DBG("Username: '%s'", ap->username);
47- DBG("Password: '%s'", ap->password);
48+ /*
49+ * Only create a data context for the first matching APN.
50+ * See comment above that restricts restricts apn_count.
51+ */
52+ if (i == 0) {
53+ ofono_info("Name: '%s'", ap->name);
54+ ofono_info("APN: '%s'", ap->apn);
55+ ofono_info("Type: %s", mbpi_ap_type(ap->type));
56+ ofono_info("Username: '%s'", ap->username);
57+ ofono_info("Password: '%s'", ap->password);
58
59- memcpy(*settings + i, ap,
60- sizeof(struct ofono_gprs_provision_data));
61+ memcpy(*settings + i, ap,
62+ sizeof(struct ofono_gprs_provision_data));
63+ }
64
65 g_free(ap);
66 }

Subscribers

People subscribed via source and target branches