Merge lp:~ted/ubuntu-app-launch/oom-adjust-file into lp:ubuntu-app-launch/14.10

[Ted Gould]

Set OOM adjustment without using temporary files.

Fixes the OOM adjustment code which was using set_file_contents, which should work in theory, but internally it works by creating a temp file and renaming it. Which doesn't really work in /proc. So now going with the good ol' fwrite.

Also, figured out how to test this code. So now we're checking values.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:179
http://jenkins.qa.ubuntu.com/job/upstart-app-launch-ci/300/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/upstart-app-launch-utopic-amd64-ci/39
    SUCCESS: http://jenkins.qa.ubuntu.com/job/upstart-app-launch-utopic-armhf-ci/39
    SUCCESS: http://jenkins.qa.ubuntu.com/job/upstart-app-launch-utopic-i386-ci/39

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/upstart-app-launch-ci/300/rebuild

review: Approve (continuous-integration)

[Charles Kerr (charlesk)]

LGTM.

review: Approve

[lp:~ted/ubuntu-app-launch/oom-adjust-file]

180. By Ted Gould on 2014-09-29

Optimize the compiler's choice of branching

181. By Ted Gould on 2014-09-29

Return true if it does not exist

=== modified file 'libubuntu-app-launch/ubuntu-app-launch.c'
--- libubuntu-app-launch/ubuntu-app-launch.c	2014-09-15 19:40:02 +0000
+++ libubuntu-app-launch/ubuntu-app-launch.c	2014-09-29 15:38:54 +0000
@@ -23,6 +23,8 @@
 #include <upstart.h>
 #include <gio/gio.h>
 #include <string.h>
+#include <fcntl.h>
+#include <errno.h>
 #include <zeitgeist.h>
 
 #include "ubuntu-app-launch-trace.h"
@@ -414,17 +416,45 @@
 static gboolean
 set_oom_value (GPid pid, const gchar * oomscore)
 {
-	if (oomscore == NULL) {
+	static const gchar * procpath = NULL;
+	if (G_UNLIKELY(procpath == NULL)) {
+		/* Set by the test suite, probably not anyone else */
+		procpath = g_getenv("UBUNTU_APP_LAUNCH_OOM_PROC_PATH");
+		if (G_LIKELY(procpath == NULL)) {
+			procpath = "/proc";
+		}
+	}
+
+	gchar * path = g_strdup_printf("%s/%d/oom_score_adj", procpath, pid);
+	FILE * adj = fopen(path, "w");
+	int openerr = errno;
+	g_free(path);
+
+	if (adj == NULL) {
+		if (openerr != ENOENT) {
+			/* ENOENT happens a fair amount because of races, so it's not
+			   worth printing a warning about */
+			g_warning("Unable to set OOM value for '%d' to '%s': %s", pid, oomscore, strerror(openerr));
+			return FALSE;
+		} else {
+			return TRUE;
+		}
+	}
+
+	size_t writesize = fwrite(oomscore, 1, strlen(oomscore), adj);
+	int writeerr = errno;
+	fclose(adj);
+
+	if (writesize == strlen(oomscore))
 		return TRUE;
-	}
-
-	gchar * path = g_strdup_printf("/proc/%d/oom_score_adj", pid);
-
-	gboolean res = g_file_set_contents(path, oomscore, -1, NULL);
-
-	g_free(path);
-
-	return res;
+	
+	if (writeerr != 0)
+		g_warning("Unable to set OOM value for '%d' to '%s': %s", pid, oomscore, strerror(writeerr));
+	else
+		/* No error, but yet, wrong size. Not sure, what could cause this. */
+		g_debug("Unable to set OOM value for '%d' to '%s': Wrote %d bytes", pid, oomscore, (int)writesize);
+
+	return FALSE;
 }
 
 /* Gets all the pids for an appid and sends a signal to all of them. This also
@@ -436,12 +466,6 @@
 	guint hash_table_size = 0;
 	gboolean retval = TRUE;
 
-	/* In the test suite we can't set this becuase we don't have permissions,
-	   which sucks, but it's the reality of testing at package build time */
-	if (g_getenv("UBUNTU_APP_LAUNCH_NO_SET_OOM")) {
-		oomscore = NULL;
-	}
-
 	do {
 		hash_table_size = g_hash_table_size(pidssignaled);
 		GList * pidlist = pids_for_appid(appid);
@@ -467,7 +491,6 @@
 			}
 
 			if (!set_oom_value(pid, oomscore)) {
-				g_warning("Unable to set OOM score '%s' on pid %d", oomscore, pid);
 				retval = FALSE;
 			}
 
 
=== modified file 'tests/libual-test.cc'
--- tests/libual-test.cc	2014-09-15 19:40:02 +0000
+++ tests/libual-test.cc	2014-09-29 15:38:54 +0000
@@ -1274,7 +1274,7 @@
 
 TEST_F(LibUAL, PauseResume)
 {
-	g_setenv("UBUNTU_APP_LAUNCH_NO_SET_OOM", "TRUE", 1);
+	g_setenv("UBUNTU_APP_LAUNCH_OOM_PROC_PATH", CMAKE_BINARY_DIR "/libual-proc" , 1);
 
 	/* Setup some spew */
 	GPid spewpid = 0;
@@ -1296,6 +1296,13 @@
 	g_io_channel_set_flags(spewoutchan, G_IO_FLAG_NONBLOCK, NULL);
 	g_io_add_watch(spewoutchan, G_IO_IN, datain, &datacnt);
 
+	/* Setup our OOM adjust file */
+	gchar * procdir = g_strdup_printf(CMAKE_BINARY_DIR "/libual-proc/%d", spewpid);
+	ASSERT_EQ(0, g_mkdir_with_parents(procdir, 0700));
+	gchar * oomadjfile = g_strdup_printf("%s/oom_score_adj", procdir);
+	g_free(procdir);
+	ASSERT_TRUE(g_file_set_contents(oomadjfile, "0", -1, NULL));
+
 	/* Setup the cgroup */
 	g_setenv("UBUNTU_APP_LAUNCH_CG_MANAGER_NAME", "org.test.cgmock2", TRUE);
 	DbusTestDbusMock * cgmock2 = dbus_test_dbus_mock_new("org.test.cgmock2");
@@ -1346,6 +1353,7 @@
 
 	pause(200);
 
+	/* Check data coming out */
 	EXPECT_EQ(0, datacnt);
 
 	/* Check to make sure we sent the event to ZG */
@@ -1356,8 +1364,14 @@
 	EXPECT_EQ(1, numcalls);
 
 	dbus_test_dbus_mock_object_clear_method_calls(zgmock, zgobj, NULL);
+	
+	/* Check to ensure we set the OOM score */
+	gchar * pauseoomscore = NULL;
+	ASSERT_TRUE(g_file_get_contents(oomadjfile, &pauseoomscore, NULL, NULL));
+	EXPECT_STREQ("900", pauseoomscore);
+	g_free(pauseoomscore);
 
-	/* No Resume the App */
+	/* Now Resume the App */
 	EXPECT_TRUE(ubuntu_app_launch_resume_application("com.test.good_application_1.2.3"));
 
 	pause(200);
@@ -1371,6 +1385,12 @@
 	EXPECT_NE(nullptr, calls);
 	EXPECT_EQ(1, numcalls);
 
+	/* Check to ensure we set the OOM score */
+	gchar * resumeoomscore = NULL;
+	ASSERT_TRUE(g_file_get_contents(oomadjfile, &resumeoomscore, NULL, NULL));
+	EXPECT_STREQ("100", resumeoomscore);
+	g_free(resumeoomscore);
+
 	/* Clean up */
 	gchar * killstr = g_strdup_printf("kill -9 %d", spewpid);
 	ASSERT_TRUE(g_spawn_command_line_sync(killstr, NULL, NULL, NULL, NULL));
@@ -1378,8 +1398,12 @@
 
 	g_io_channel_unref(spewoutchan);
 
+	g_spawn_command_line_sync("rm -rf " CMAKE_BINARY_DIR "/libual-proc", NULL, NULL, NULL, NULL);
+
 	/* Kill ZG default instance :-( */
 	ZeitgeistLog * log = zeitgeist_log_get_default();
 	g_object_unref(log);
 	g_object_unref(log);
+	
+	g_free(oomadjfile);
 }
 
=== modified file 'upstart-jobs/application-click.conf.in'
--- upstart-jobs/application-click.conf.in	2014-08-22 03:28:19 +0000
+++ upstart-jobs/application-click.conf.in	2014-09-29 15:38:54 +0000
@@ -18,6 +18,9 @@
 apparmor switch ${APP_ID}
 cgroup freezer
 
+# Initial OOM Score
+oom score 100
+
 # Remember, this is confined
 exec @pkglibexecdir@/exec-line-exec
 
 
=== modified file 'upstart-jobs/application-legacy.conf.in'
--- upstart-jobs/application-legacy.conf.in	2014-08-22 03:28:19 +0000
+++ upstart-jobs/application-legacy.conf.in	2014-09-29 15:38:54 +0000
@@ -16,6 +16,9 @@
 apparmor switch $APP_EXEC_POLICY
 cgroup freezer
 
+# Initial OOM Score
+oom score 110
+
 # This could be confined
 exec @pkglibexecdir@/exec-line-exec