Merge lp:~ted/ubuntu-app-launch/bad-appid-recoverable-error into lp:ubuntu-app-launch/14.04

Proposed by Ted Gould on 2014-04-16
Status: Merged
Approved by: Charles Kerr on 2014-04-22
Approved revision: 151
Merged at revision: 147
Proposed branch: lp:~ted/ubuntu-app-launch/bad-appid-recoverable-error
Merge into: lp:ubuntu-app-launch/14.04
Diff against target: 286 lines (+235/-2)
5 files modified
CMakeLists.txt (+1/-1)
desktop-exec.c (+42/-1)
libupstart-app-launch/upstart-app-launch.c (+1/-0)
recoverable-problem.c (+165/-0)
recoverable-problem.h (+26/-0)
To merge this branch: bzr merge lp:~ted/ubuntu-app-launch/bad-appid-recoverable-error
Reviewer Review Type Date Requested Status
Charles Kerr (community) 2014-04-16 Approve on 2014-04-22
PS Jenkins bot (community) continuous-integration Approve on 2014-04-16
Review via email: mp+216192@code.launchpad.net

Commit message

More elegant handling of bad application id errors

Description of the change

We get various bugs on this, which is good that they're being reported, but there's a bit of noise. Trying to filter out that noise and be a bit more sophisticated on the reporting.

To post a comment you must log in.
Charles Kerr (charlesk) wrote :

* cmdline is leaked.

* Not a blocker for this merge, but as a general comment: we really need to move recoverable-problem.c into some shared location

Charles Kerr (charlesk) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'CMakeLists.txt'
2--- CMakeLists.txt 2014-03-12 00:28:05 +0000
3+++ CMakeLists.txt 2014-04-16 18:51:37 +0000
4@@ -83,7 +83,7 @@
5 ####################
6
7 add_lttng_gen_tp(NAME desktop-exec-trace)
8-add_executable(desktop-exec desktop-exec.c desktop-exec-trace.c)
9+add_executable(desktop-exec desktop-exec.c desktop-exec-trace.c recoverable-problem.c)
10 set_target_properties(desktop-exec PROPERTIES OUTPUT_NAME "desktop-exec")
11 target_link_libraries(desktop-exec helpers ${LTTNG_LIBRARIES})
12 install(TARGETS desktop-exec RUNTIME DESTINATION "${pkglibexecdir}")
13
14=== modified file 'desktop-exec.c'
15--- desktop-exec.c 2014-02-07 14:56:27 +0000
16+++ desktop-exec.c 2014-04-16 18:51:37 +0000
17@@ -25,6 +25,7 @@
18
19 #include "helpers.h"
20 #include "desktop-exec-trace.h"
21+#include "recoverable-problem.h"
22
23 int
24 main (int argc, char * argv[])
25@@ -65,7 +66,47 @@
26 GKeyFile * keyfile = keyfile_for_appid(app_id, &desktopfilename);
27
28 if (keyfile == NULL) {
29- g_error("Unable to find keyfile for application '%s'", app_id);
30+ g_warning("Unable to find keyfile for application '%s'", app_id);
31+
32+ const gchar * props[3] = {
33+ "AppId", NULL,
34+ NULL
35+ };
36+ props[1] = app_id;
37+
38+ GPid pid = 0;
39+ const gchar * launcher_pid = g_getenv("APP_LAUNCHER_PID");
40+ if (launcher_pid != NULL) {
41+ pid = atoi(launcher_pid);
42+ }
43+
44+ /* Checking to see if we're using the command line tool to create
45+ the appid. Chances are in that case it's a user error, and we
46+ don't need to automatically record it, the user mistyped. */
47+ gboolean debugtool = FALSE;
48+ if (pid != 0) {
49+ gchar * cmdpath = g_strdup_printf("/proc/%d/cmdline", pid);
50+ gchar * cmdline = NULL;
51+
52+ if (g_file_get_contents(cmdpath, &cmdline, NULL, NULL)) {
53+ if (g_strstr_len(cmdline, -1, "upstart-app-launch") != NULL) {
54+ debugtool = TRUE;
55+ }
56+
57+ g_free(cmdline);
58+ } else {
59+ /* The caller has already exited, probably a debug tool */
60+ debugtool = TRUE;
61+ }
62+
63+ g_free(cmdpath);
64+ }
65+
66+ if (!debugtool) {
67+ report_recoverable_problem("upstart-app-launch-invalid-appid", pid, TRUE, props);
68+ } else {
69+ g_debug("Suppressing appid recoverable error for debug tool");
70+ }
71 return 1;
72 }
73
74
75=== modified file 'libupstart-app-launch/upstart-app-launch.c'
76--- libupstart-app-launch/upstart-app-launch.c 2014-03-21 17:56:15 +0000
77+++ libupstart-app-launch/upstart-app-launch.c 2014-04-16 18:51:37 +0000
78@@ -228,6 +228,7 @@
79 g_variant_builder_open(&builder, G_VARIANT_TYPE_ARRAY);
80
81 g_variant_builder_add_value(&builder, g_variant_new_take_string(g_strdup_printf("APP_ID=%s", appid)));
82+ g_variant_builder_add_value(&builder, g_variant_new_take_string(g_strdup_printf("APP_LAUNCHER_PID=%d", getpid())));
83
84 if (uris != NULL) {
85 gchar * urisjoin = app_uris_string(uris);
86
87=== added file 'recoverable-problem.c'
88--- recoverable-problem.c 1970-01-01 00:00:00 +0000
89+++ recoverable-problem.c 2014-04-16 18:51:37 +0000
90@@ -0,0 +1,165 @@
91+/*
92+ * Copyright 2013 Canonical Ltd.
93+ *
94+ * This program is free software: you can redistribute it and/or modify it
95+ * under the terms of the GNU General Public License version 3, as published
96+ * by the Free Software Foundation.
97+ *
98+ * This program is distributed in the hope that it will be useful, but
99+ * WITHOUT ANY WARRANTY; without even the implied warranties of
100+ * MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
101+ * PURPOSE. See the GNU General Public License for more details.
102+ *
103+ * You should have received a copy of the GNU General Public License along
104+ * with this program. If not, see <http://www.gnu.org/licenses/>.
105+ *
106+ * Authors:
107+ * Ted Gould <ted.gould@canonical.com>
108+ */
109+
110+#include "recoverable-problem.h"
111+#include <glib/gstdio.h>
112+#include <string.h>
113+#include <errno.h>
114+
115+/* Helpers to ensure we write nicely */
116+static void
117+write_string (int fd,
118+ const gchar *string)
119+{
120+ int res;
121+ do
122+ res = write (fd, string, strlen (string));
123+ while (G_UNLIKELY (res == -1 && errno == EINTR));
124+}
125+
126+/* Make NULLs fast and fun! */
127+static void
128+write_null (int fd)
129+{
130+ int res;
131+ do
132+ res = write (fd, "", 1);
133+ while (G_UNLIKELY (res == -1 && errno == EINTR));
134+}
135+
136+/* Child watcher */
137+static gboolean
138+apport_child_watch (GPid pid, gint status, gpointer user_data)
139+{
140+ g_main_loop_quit((GMainLoop *)user_data);
141+ return FALSE;
142+}
143+
144+static gboolean
145+apport_child_timeout (gpointer user_data)
146+{
147+ g_warning("Recoverable Error Reporter Timeout");
148+ g_main_loop_quit((GMainLoop *)user_data);
149+ return FALSE;
150+}
151+
152+/* Code to report an error */
153+void
154+report_recoverable_problem (const gchar * signature, GPid report_pid, gboolean wait, const gchar * additional_properties[])
155+{
156+ GError * error = NULL;
157+ gint error_stdin = 0;
158+ GPid pid = 0;
159+ gchar * pid_str = NULL;
160+ gchar ** argv = NULL;
161+ gchar * argv_nopid[2] = {
162+ "/usr/share/apport/recoverable_problem",
163+ NULL
164+ };
165+ gchar * argv_pid[4] = {
166+ "/usr/share/apport/recoverable_problem",
167+ "-p",
168+ NULL, /* put pid_str when allocated here */
169+ NULL
170+ };
171+
172+
173+ argv = (gchar **)argv_nopid;
174+
175+ if (report_pid != 0) {
176+ pid_str = g_strdup_printf("%d", report_pid);
177+ argv_pid[2] = pid_str;
178+ argv = (gchar**)argv_pid;
179+ }
180+
181+ GSpawnFlags flags = G_SPAWN_STDOUT_TO_DEV_NULL | G_SPAWN_STDERR_TO_DEV_NULL;
182+ if (wait) {
183+ flags |= G_SPAWN_DO_NOT_REAP_CHILD;
184+ }
185+
186+ g_spawn_async_with_pipes(NULL, /* cwd */
187+ argv,
188+ NULL, /* envp */
189+ flags,
190+ NULL, NULL, /* child setup func */
191+ &pid,
192+ &error_stdin,
193+ NULL, /* stdout */
194+ NULL, /* stderr */
195+ &error);
196+
197+ if (error != NULL) {
198+ g_warning("Unable to report a recoverable error: %s", error->message);
199+ g_error_free(error);
200+ }
201+
202+ gboolean first = TRUE;
203+
204+ if (error_stdin != 0 && signature != NULL) {
205+ write_string(error_stdin, "DuplicateSignature");
206+ write_null(error_stdin);
207+ write_string(error_stdin, signature);
208+
209+ first = FALSE;
210+ }
211+
212+ if (error_stdin != 0 && additional_properties != NULL) {
213+ gint i;
214+ for (i = 0; additional_properties[i] != NULL; i++) {
215+ if (!first) {
216+ write_null(error_stdin);
217+ } else {
218+ first = FALSE;
219+ }
220+
221+ write_string(error_stdin, additional_properties[i]);
222+ }
223+ }
224+
225+ if (error_stdin != 0) {
226+ close(error_stdin);
227+ }
228+
229+ if (wait && pid != 0) {
230+ GSource * child_source, * timeout_source;
231+ GMainContext * context = g_main_context_new();
232+ GMainLoop * loop = g_main_loop_new(context, FALSE);
233+
234+ child_source = g_child_watch_source_new(pid);
235+ g_source_attach(child_source, context);
236+ g_source_set_callback(child_source, (GSourceFunc)apport_child_watch, loop, NULL);
237+
238+ timeout_source = g_timeout_source_new_seconds(5);
239+ g_source_attach(timeout_source, context);
240+ g_source_set_callback(timeout_source, apport_child_timeout, loop, NULL);
241+
242+ g_main_loop_run(loop);
243+
244+ g_source_destroy(timeout_source);
245+ g_source_destroy(child_source);
246+ g_main_loop_unref(loop);
247+ g_main_context_unref(context);
248+
249+ g_spawn_close_pid(pid);
250+ }
251+
252+ g_free(pid_str);
253+
254+ return;
255+}
256
257=== added file 'recoverable-problem.h'
258--- recoverable-problem.h 1970-01-01 00:00:00 +0000
259+++ recoverable-problem.h 2014-04-16 18:51:37 +0000
260@@ -0,0 +1,26 @@
261+/*
262+ * Copyright 2013 Canonical Ltd.
263+ *
264+ * This program is free software: you can redistribute it and/or modify it
265+ * under the terms of the GNU General Public License version 3, as published
266+ * by the Free Software Foundation.
267+ *
268+ * This program is distributed in the hope that it will be useful, but
269+ * WITHOUT ANY WARRANTY; without even the implied warranties of
270+ * MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
271+ * PURPOSE. See the GNU General Public License for more details.
272+ *
273+ * You should have received a copy of the GNU General Public License along
274+ * with this program. If not, see <http://www.gnu.org/licenses/>.
275+ *
276+ * Authors:
277+ * Ted Gould <ted.gould@canonical.com>
278+ */
279+
280+#include <gio/gio.h>
281+
282+void report_recoverable_problem (const gchar * signature,
283+ GPid report_pid,
284+ gboolean wait,
285+ const gchar * additional_properties[]);
286+

Subscribers

People subscribed via source and target branches