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

[Tony Espy]

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

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.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:52
http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-ci/69/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-saucy-armhf-ci/44
        deb: http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-saucy-armhf-ci/44/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/phablet-extras-ofono-ci/69/rebuild

review: Approve (continuous-integration)

[Ricardo Salveti (rsalveti)]

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

[lp:~awe/phablet-extras/ofono-lp1204683]

53. By Tony Espy on 2013-08-05

[provision] Address review comments.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:53
http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-ci/70/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-saucy-armhf-ci/45
        deb: http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-saucy-armhf-ci/45/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/phablet-extras-ofono-ci/70/rebuild

review: Approve (continuous-integration)

[Ricardo Salveti (rsalveti)]

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

review: Approve

=== modified file 'debian/changelog'
--- debian/changelog	2013-07-30 15:56:40 +0000
+++ debian/changelog	2013-08-05 17:26:26 +0000
@@ -1,3 +1,10 @@
+ofono (1.12phablet12) saucy; urgency=low
+
+  * plugins/provision.c: if multiple "internet" data contexts
+    are found, only provision the first (LP:1204683).
+
+ -- Tony Espy <[e-mail]>  Thu, 01 Aug 2013 12:13:37 -0400
+
 ofono (1.12phablet11) saucy; urgency=low
 
   * drivers/rilmodem/gprs.c: adding data network state tracking back, but

=== modified file 'plugins/provision.c'
--- plugins/provision.c	2013-05-24 22:11:34 +0000
+++ plugins/provision.c	2013-08-05 17:26:26 +0000
@@ -67,7 +67,18 @@
 
 	ap_count = g_slist_length(apns);
 
-	DBG("Found %d APs", ap_count);
+	ofono_info("GPRS Provisioning found %d matching APNs for "
+			"SPN: %s MCC: %d MNC: %d",
+			ap_count, spn, mcc, mnc);
+	/*
+	 * Only keep the first APN found.
+	 *
+	 * This allows auto-provisioning to work most of the time vs.
+	 * passing FALSE to mbpi_lookup_apn() which would return an
+	 * an empty list if duplicates are found.
+	 */
+	if (ap_count > 1)
+		ap_count = 1;
 
 	*settings = g_try_new0(struct ofono_gprs_provision_data, ap_count);
 	if (*settings == NULL) {
@@ -86,14 +97,20 @@
 	for (l = apns, i = 0; l; l = l->next, i++) {
 		struct ofono_gprs_provision_data *ap = l->data;
 
-		DBG("Name: '%s'", ap->name);
-		DBG("APN: '%s'", ap->apn);
-		DBG("Type: %s", mbpi_ap_type(ap->type));
-		DBG("Username: '%s'", ap->username);
-		DBG("Password: '%s'", ap->password);
+		/*
+		 * Only create a data context for the first matching APN.
+		 * See comment above that restricts restricts apn_count.
+		 */
+		if (i == 0) {
+			ofono_info("Name: '%s'", ap->name);
+			ofono_info("APN: '%s'", ap->apn);
+			ofono_info("Type: %s", mbpi_ap_type(ap->type));
+			ofono_info("Username: '%s'", ap->username);
+			ofono_info("Password: '%s'", ap->password);
 
-		memcpy(*settings + i, ap,
-			sizeof(struct ofono_gprs_provision_data));
+			memcpy(*settings + i, ap,
+				sizeof(struct ofono_gprs_provision_data));
+		}
 
 		g_free(ap);
 	}