Merge lp:~awe/phablet-extras/ofono-socket-exit into lp:phablet-extras/ofono

Proposed by Tony Espy on 2013-06-25
Status: Merged
Approved by: Ricardo Salveti on 2013-06-26
Approved revision: 45
Merged at revision: 44
Proposed branch: lp:~awe/phablet-extras/ofono-socket-exit
Merge into: lp:phablet-extras/ofono
Diff against target: 55 lines (+10/-13)
2 files modified
debian/changelog (+6/-0)
gril/gril.c (+4/-13)
To merge this branch: bzr merge lp:~awe/phablet-extras/ofono-socket-exit
Reviewer Review Type Date Requested Status
PS Jenkins bot continuous-integration Approve on 2013-06-26
Ricardo Salveti 2013-06-25 Approve on 2013-06-26
Review via email: mp+171355@code.launchpad.net

Commit message

[gril] Exit if errors occur during RILD socket intialization.

Description of the change

This changes causes ofono to exit if errors are encountered trying to connect to the RILD socket. This should allow upstart to restart ofono, and if too many errors occur in too short a time period, upstart will mark ofono as failed and give up. This is a better approach than the current code which just ends-up in an un-usable state.

To post a comment you must log in.
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:42
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~awe/phablet-extras/ofono-socket-exit/+merge/171355/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-ci/41/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-saucy-armhf-ci/16
        deb: http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-saucy-armhf-ci/16/artifact/work/output/*zip*/output.zip

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

review: Needs Fixing (continuous-integration)
43. By Tony Espy on 2013-06-25

Re-merge from trunk.

44. By Tony Espy on 2013-06-25

Fixed changelog merge error.

Ricardo Salveti (rsalveti) wrote :

32 - /* TODO: this should have retry logic... */
33 + ril->connected = FALSE;

Please let the todo as this is not yet the final solution. This code would break the logic in case ofono is loaded but it's expected that rild is not available.

I do believe we should improve the code to give make it try to load the modem for a few times, and then just return load error instead.

review: Needs Fixing
Tony Espy (awe) wrote :

We've discussed this approach for over a week, and this is the first time I've heard your opposition to this approach.

With regards to the case where ofono is started, but it's expected that rild isn't available, I've stated in the past that the best approach to solving this is to not enable the device plugin ( plugins/ril ) in the first place. The standard technique that ofono uses is a udevng plugin, which wasn't used originally as we didn't run udev in our Ubuntyu container. The udev approach also isn't really the best approach to determining that a device runs rild.

So another approach would be to write a new plugin which understands how to query the Android properties and only enable the ril plugin based upon the values of one or more ril-specific properties.

I really would prefer not to put hard retry limits on the RILD socket in the code. Upstart has the ability to cap restarts over a given time period, which couldn't be leveraged if the retries are in the /gril layer.

45. By Tony Espy on 2013-06-26

Update debian/changelog to UNRELEASED.

Ricardo Salveti (rsalveti) wrote :

We discussed over mumble, and we decided to move forward with this change and later improve the modem detection part to only enable ril in case it's available in the system (probably via properties).

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-06-25 06:20:13 +0000
3+++ debian/changelog 2013-06-26 15:51:45 +0000
4@@ -1,3 +1,9 @@
5+ofono (1.12phablet7) UNRELEASED; urgency=low
6+
7+ * gril/gril.c: Exit on RILD socket failures.
8+
9+ -- Tony Espy <espy@canonical.com> Tue, 25 Jun 2013 15:14:13 -0400
10+
11 ofono (1.12phablet6) saucy; urgency=low
12
13 * [ril/rilmodem] Add real calls for revision and IMEI probe
14
15=== modified file 'gril/gril.c'
16--- gril/gril.c 2013-06-17 13:35:23 +0000
17+++ gril/gril.c 2013-06-26 15:51:45 +0000
18@@ -24,6 +24,7 @@
19 #include <config.h>
20 #endif
21
22+#include <stdlib.h>
23 #include <stdio.h>
24 #include <unistd.h>
25 #include <string.h>
26@@ -783,9 +784,7 @@
27 ril->debugf = NULL;
28 ril->req_bytes_written = 0;
29 ril->trace = FALSE;
30-
31-
32- /* TODO: this should have retry logic... */
33+ ril->connected = FALSE;
34
35 sk = socket(AF_UNIX, SOCK_STREAM, 0);
36 if (sk < 0) {
37@@ -838,16 +837,8 @@
38 return ril;
39
40 error:
41- g_ril_io_unref(ril->io);
42-
43- if (ril->command_queue)
44- g_queue_free(ril->command_queue);
45-
46- if (ril->notify_list)
47- g_hash_table_destroy(ril->notify_list);
48-
49- g_free(ril);
50- return NULL;
51+ ofono_error("Exiting...");
52+ exit(EXIT_FAILURE);
53 }
54
55 static struct ril_notify *ril_notify_create(struct ril_s *ril,

Subscribers

People subscribed via source and target branches