Merge lp:~cjwatson/ubuntu-app-launch/libclick-pkgdir into lp:ubuntu-app-launch/14.04

Proposed by Colin Watson
Status: Merged
Approved by: Charles Kerr
Approved revision: 138
Merged at revision: 140
Proposed branch: lp:~cjwatson/ubuntu-app-launch/libclick-pkgdir
Merge into: lp:ubuntu-app-launch/14.04
Diff against target: 389 lines (+91/-75)
8 files modified
CMakeLists.txt (+5/-2)
click-exec.c (+44/-33)
debian/control (+1/-0)
desktop-hook.c (+27/-34)
tests/CMakeLists.txt (+3/-0)
tests/click (+0/-3)
tests/click-db-dir/test.conf.in (+2/-0)
tests/exec-util-test.cc (+9/-3)
To merge this branch: bzr merge lp:~cjwatson/ubuntu-app-launch/libclick-pkgdir
Reviewer Review Type Date Requested Status
Charles Kerr (community) Approve
PS Jenkins bot (community) continuous-integration Approve
Review via email: mp+209909@code.launchpad.net

Commit message

Use libclick to get the package directory, saving about 0.7 seconds from Click application startup (on mako).

Description of the change

This branch converts upstart-app-launch to using libclick to get the package directory. (It still forks the click binary to get the manifest, because libclick doesn't yet have appropriate functions for that; I'm working on that and should be able to submit a follow-up branch soon.)

To make this work properly, I had to give click-exec environment variables to look at a different Click database directory, and rearrange some test files to be structured a bit more like a real Click database. Ted said on IRC that adding these environment variables should be fine from the point of view of security as all a user can do is "compromise" their own shell.

I've tested this manually on mako after building on porter-armhf.c.c, and run some timings. With upstart-app-launch 0.3+14.04.20140220-0ubuntu3, the median of nine runs of "time upstart-app-launch com.ubuntu.calculator_calculator_0.1.3.224" (followed by "upstart-app-stop com.ubuntu.calculator_calculator_0.1.3.224" to clean up) was 1.772 seconds. With this change, the same measurement returns 1.037 seconds. This is therefore a saving of a bit over 0.7 seconds here, or a little over 40% of upstart-app-launch's run-time.

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Charles Kerr (charlesk) wrote :

Colin, thanks for this, a 40% improvement is huge news! :-)

In the code itself, there are a couple of trivial issues you might want to fix:

 * in click-exec.c around line 98, need to unref the db before returning if click_user_new_for_user() fails.

 * in click-exec.c around line 103, need to unref the db and the user if click_user_get_path() fails.

138. By Colin Watson

Fix some reference leaks.

Revision history for this message
Colin Watson (cjwatson) wrote :

Ah yes, of course. Pushed a fix, thanks.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Charles Kerr (charlesk) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'CMakeLists.txt'
--- CMakeLists.txt 2014-02-13 22:43:55 +0000
+++ CMakeLists.txt 2014-03-07 14:19:16 +0000
@@ -52,6 +52,9 @@
52pkg_check_modules(ZEITGEIST REQUIRED zeitgeist-2.0)52pkg_check_modules(ZEITGEIST REQUIRED zeitgeist-2.0)
53include_directories(${ZEITGEIST_INCLUDE_DIRS})53include_directories(${ZEITGEIST_INCLUDE_DIRS})
5454
55pkg_check_modules(CLICK REQUIRED click-0.4)
56include_directories(${CLICK_INCLUDE_DIRS})
57
55pkg_check_modules(LIBUPSTART REQUIRED libupstart)58pkg_check_modules(LIBUPSTART REQUIRED libupstart)
56include_directories(${LIBUPSTART_INCLUDE_DIRS})59include_directories(${LIBUPSTART_INCLUDE_DIRS})
5760
@@ -92,7 +95,7 @@
92add_lttng_gen_tp(NAME click-exec-trace)95add_lttng_gen_tp(NAME click-exec-trace)
93add_executable(click-exec click-exec.c click-exec-trace.c)96add_executable(click-exec click-exec.c click-exec-trace.c)
94set_target_properties(click-exec PROPERTIES OUTPUT_NAME "click-exec")97set_target_properties(click-exec PROPERTIES OUTPUT_NAME "click-exec")
95target_link_libraries(click-exec helpers ${LTTNG_LIBRARIES})98target_link_libraries(click-exec helpers ${LTTNG_LIBRARIES} ${CLICK_LIBRARIES})
96install(TARGETS click-exec RUNTIME DESTINATION "${pkglibexecdir}")99install(TARGETS click-exec RUNTIME DESTINATION "${pkglibexecdir}")
97100
98####################101####################
@@ -101,7 +104,7 @@
101104
102add_executable(desktop-hook desktop-hook.c)105add_executable(desktop-hook desktop-hook.c)
103set_target_properties(desktop-hook PROPERTIES OUTPUT_NAME "desktop-hook")106set_target_properties(desktop-hook PROPERTIES OUTPUT_NAME "desktop-hook")
104target_link_libraries(desktop-hook helpers)107target_link_libraries(desktop-hook helpers ${CLICK_LIBRARIES})
105install(TARGETS desktop-hook RUNTIME DESTINATION "${pkglibexecdir}")108install(TARGETS desktop-hook RUNTIME DESTINATION "${pkglibexecdir}")
106109
107####################110####################
108111
=== modified file 'click-exec.c'
--- click-exec.c 2014-02-07 14:56:27 +0000
+++ click-exec.c 2014-03-07 14:19:16 +0000
@@ -18,6 +18,7 @@
18 */18 */
1919
20#include <gio/gio.h>20#include <gio/gio.h>
21#include <click.h>
21#include "helpers.h"22#include "helpers.h"
22#include "click-exec-trace.h"23#include "click-exec-trace.h"
2324
@@ -34,7 +35,7 @@
3435
35For information on Click packages and the manifest look at the Click package documentation:36For information on Click packages and the manifest look at the Click package documentation:
3637
37https://click-package.readthedocs.org/en/latest/38https://click.readthedocs.org/en/latest/
3839
39*/40*/
4041
@@ -80,45 +81,55 @@
80 }81 }
8182
82 /* Check click to find out where the files are */83 /* Check click to find out where the files are */
83 gchar * cmdline = g_strdup_printf("click pkgdir \"%s\"", package);84 ClickDB * db = click_db_new();
8485 /* If TEST_CLICK_DB is unset, this reads the system database. */
85 gchar * output = NULL;86 click_db_read(db, g_getenv("TEST_CLICK_DB"), &error);
86 g_spawn_command_line_sync(cmdline, &output, NULL, NULL, &error);87 if (error != NULL) {
87 g_free(cmdline);88 g_warning("Unable to read Click database: %s", error->message);
89 g_error_free(error);
90 g_free(package);
91 return 1;
92 }
93 /* If TEST_CLICK_USER is unset, this uses the current user name. */
94 ClickUser * user = click_user_new_for_user(db, g_getenv("TEST_CLICK_USER"), &error);
95 if (error != NULL) {
96 g_warning("Unable to read Click database: %s", error->message);
97 g_error_free(error);
98 g_free(package);
99 g_object_unref(db);
100 return 1;
101 }
102 gchar * pkgdir = click_user_get_path(user, package, &error);
103 if (error != NULL) {
104 g_warning("Unable to get the Click package directory for %s: %s", package, error->message);
105 g_error_free(error);
106 g_free(package);
107 g_object_unref(user);
108 g_object_unref(db);
109 return 1;
110 }
111 g_object_unref(user);
112 g_object_unref(db);
88113
89 tracepoint(upstart_app_launch, click_found_pkgdir);114 tracepoint(upstart_app_launch, click_found_pkgdir);
90115
91 /* If we have an extra newline, we can delete it. */116 if (!g_file_test(pkgdir, G_FILE_TEST_EXISTS | G_FILE_TEST_IS_DIR)) {
92 gchar * newline = g_strstr_len(output, -1, "\n");117 g_warning("Application directory '%s' doesn't exist", pkgdir);
93 if (newline != NULL) {118 g_free(pkgdir);
94 newline[0] = '\0';119 g_free(package);
95 }120 return 1;
96121 }
97 if (error != NULL) {122
98 g_warning("Unable to get the package directory from click: %s", error->message);123 g_debug("Setting 'APP_DIR' to '%s'", pkgdir);
99 g_error_free(error);124 set_upstart_variable("APP_DIR", pkgdir, FALSE);
100 g_free(output); /* Probably not set, but just in case */125
101 g_free(package);126 set_confined_envvars(package, pkgdir);
102 return 1;
103 }
104
105 if (!g_file_test(output, G_FILE_TEST_EXISTS | G_FILE_TEST_IS_DIR)) {
106 g_warning("Application directory '%s' doesn't exist", output);
107 g_free(output);
108 g_free(package);
109 return 1;
110 }
111
112 g_debug("Setting 'APP_DIR' to '%s'", output);
113 set_upstart_variable("APP_DIR", output, FALSE);
114
115 set_confined_envvars(package, output);
116127
117 tracepoint(upstart_app_launch, click_configured_env);128 tracepoint(upstart_app_launch, click_configured_env);
118129
119 gchar * desktopfile = manifest_to_desktop(output, app_id);130 gchar * desktopfile = manifest_to_desktop(pkgdir, app_id);
120131
121 g_free(output);132 g_free(pkgdir);
122 g_free(package);133 g_free(package);
123134
124 if (desktopfile == NULL) {135 if (desktopfile == NULL) {
125136
=== modified file 'debian/control'
--- debian/control 2014-02-11 03:14:30 +0000
+++ debian/control 2014-03-07 14:19:16 +0000
@@ -7,6 +7,7 @@
7 dbus-x11,7 dbus-x11,
8 dbus-test-runner,8 dbus-test-runner,
9 debhelper (>= 9),9 debhelper (>= 9),
10 libclick-0.4-dev,
10 libdbus-1-dev,11 libdbus-1-dev,
11 libdbustest1-dev (>= 14.04.0),12 libdbustest1-dev (>= 14.04.0),
12 libgirepository1.0-dev,13 libgirepository1.0-dev,
1314
=== modified file 'desktop-hook.c'
--- desktop-hook.c 2013-09-13 16:30:01 +0000
+++ desktop-hook.c 2014-03-07 14:19:16 +0000
@@ -24,7 +24,7 @@
24This is a hook for Click packages. You can find information on Click package hooks in24This is a hook for Click packages. You can find information on Click package hooks in
25the click documentation:25the click documentation:
2626
27https://click-package.readthedocs.org/en/latest/27https://click.readthedocs.org/en/latest/
2828
29Probably the biggest thing to understand for how this code works is that you need to29Probably the biggest thing to understand for how this code works is that you need to
30understand that this hook is run after one, or many packages are installed. A set of30understand that this hook is run after one, or many packages are installed. A set of
@@ -44,6 +44,7 @@
4444
45#include <gio/gio.h>45#include <gio/gio.h>
46#include <glib/gstdio.h>46#include <glib/gstdio.h>
47#include <click.h>
47#include <string.h>48#include <string.h>
48#include <errno.h>49#include <errno.h>
4950
@@ -390,40 +391,32 @@
390 }391 }
391392
392 /* Check click to find out where the files are */393 /* Check click to find out where the files are */
393 gchar * cmdline = g_strdup_printf("click pkgdir \"%s\"", package);394 ClickUser * user = click_user_new_for_user(NULL, NULL, &error);
395 if (error != NULL) {
396 g_warning("Unable to read Click database: %s", error->message);
397 g_error_free(error);
398 g_free(package);
399 return;
400 }
401 gchar * pkgdir = click_user_get_path(user, package, &error);
402 if (error != NULL) {
403 g_warning("Unable to get the Click package directory for %s: %s", package, error->message);
404 g_error_free(error);
405 g_free(package);
406 return;
407 }
408 g_object_unref(user);
394 g_free(package);409 g_free(package);
395410
396 gchar * output = NULL;411 if (!g_file_test(pkgdir, G_FILE_TEST_EXISTS | G_FILE_TEST_IS_DIR)) {
397 g_spawn_command_line_sync(cmdline, &output, NULL, NULL, &error);412 g_warning("Directory returned by click '%s' couldn't be found", pkgdir);
398 g_free(cmdline);413 g_free(pkgdir);
399414 return;
400 /* If we have an extra newline, we can hide it. */415 }
401 if (output != NULL) {416
402 gchar * newline = NULL;417 gchar * indesktop = manifest_to_desktop(pkgdir, state->app_id);
403
404 newline = g_strstr_len(output, -1, "\n");
405
406 if (newline != NULL) {
407 newline[0] = '\0';
408 }
409 }
410
411 if (error != NULL) {
412 g_warning("Unable to get the package directory from click: %s", error->message);
413 g_error_free(error);
414 g_free(output); /* Probably not set, but just in case */
415 return;
416 }
417
418 if (!g_file_test(output, G_FILE_TEST_EXISTS | G_FILE_TEST_IS_DIR)) {
419 g_warning("Directory returned by click '%s' couldn't be found", output);
420 g_free(output);
421 return;
422 }
423
424 gchar * indesktop = manifest_to_desktop(output, state->app_id);
425 if (indesktop == NULL) {418 if (indesktop == NULL) {
426 g_free(output);419 g_free(pkgdir);
427 return;420 return;
428 }421 }
429422
@@ -432,11 +425,11 @@
432 gchar * desktoppath = g_build_filename(desktopdir, desktopfile, NULL);425 gchar * desktoppath = g_build_filename(desktopdir, desktopfile, NULL);
433 g_free(desktopfile);426 g_free(desktopfile);
434427
435 copy_desktop_file(indesktop, desktoppath, output, state->app_id);428 copy_desktop_file(indesktop, desktoppath, pkgdir, state->app_id);
436429
437 g_free(desktoppath);430 g_free(desktoppath);
438 g_free(indesktop);431 g_free(indesktop);
439 g_free(output);432 g_free(pkgdir);
440433
441 return;434 return;
442}435}
443436
=== modified file 'tests/CMakeLists.txt'
--- tests/CMakeLists.txt 2014-01-22 15:47:41 +0000
+++ tests/CMakeLists.txt 2014-03-07 14:19:16 +0000
@@ -1,6 +1,9 @@
11
2set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11")2set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11")
33
4configure_file ("click-db-dir/test.conf.in" "${CMAKE_CURRENT_BINARY_DIR}/click-db-dir/test.conf" @ONLY)
5set_directory_properties (PROPERTIES ADDITIONAL_MAKE_CLEAN_FILES "${CMAKE_CURRENT_BINARY_DIR}/click-db-dir/test.conf")
6
4# Google Test7# Google Test
58
6include_directories(${GTEST_INCLUDE_DIR})9include_directories(${GTEST_INCLUDE_DIR})
710
=== modified file 'tests/click'
--- tests/click 2013-12-13 20:32:49 +0000
+++ tests/click 2014-03-07 14:19:16 +0000
@@ -2,9 +2,6 @@
22
3if [ ${1} == "info" ] ; then3if [ ${1} == "info" ] ; then
4 /bin/cat "${XDG_DATA_DIRS}/click-app-dir/.click/info/${2}.manifest"4 /bin/cat "${XDG_DATA_DIRS}/click-app-dir/.click/info/${2}.manifest"
5else if [ ${1} == "pkgdir" ] ; then
6 echo "${XDG_DATA_DIRS}/click-app-dir"
7else5else
8 exit 16 exit 1
9fi7fi
10fi
118
=== added directory 'tests/click-db-dir'
=== added file 'tests/click-db-dir/test.conf.in'
--- tests/click-db-dir/test.conf.in 1970-01-01 00:00:00 +0000
+++ tests/click-db-dir/test.conf.in 2014-03-07 14:19:16 +0000
@@ -0,0 +1,2 @@
1[Click Database]
2root = @CMAKE_CURRENT_SOURCE_DIR@/click-root-dir
03
=== added directory 'tests/click-root-dir'
=== added directory 'tests/click-root-dir/.click'
=== added directory 'tests/click-root-dir/.click/users'
=== added directory 'tests/click-root-dir/.click/users/test-user'
=== added symlink 'tests/click-root-dir/.click/users/test-user/com.test.bad-version'
=== target is u'../../../com.test.bad-version/1.2.3'
=== added symlink 'tests/click-root-dir/.click/users/test-user/com.test.good'
=== target is u'../../../com.test.good/1.2.3'
=== added symlink 'tests/click-root-dir/.click/users/test-user/com.test.multiple'
=== target is u'../../../com.test.multiple/1.2.3'
=== added symlink 'tests/click-root-dir/.click/users/test-user/com.test.no-app'
=== target is u'../../../com.test.no-app/1.2.3'
=== added symlink 'tests/click-root-dir/.click/users/test-user/com.test.no-hooks'
=== target is u'../../../com.test.no-hooks/1.2.3'
=== added symlink 'tests/click-root-dir/.click/users/test-user/com.test.no-json'
=== target is u'../../../com.test.no-json/1.2.3'
=== added symlink 'tests/click-root-dir/.click/users/test-user/com.test.no-object'
=== target is u'../../../com.test.no-object/1.2.3'
=== added symlink 'tests/click-root-dir/.click/users/test-user/com.test.no-version'
=== target is u'../../../com.test.no-version/1.2.3'
=== added directory 'tests/click-root-dir/com.test.bad-version'
=== added symlink 'tests/click-root-dir/com.test.bad-version/1.2.3'
=== target is u'../../click-app-dir'
=== added directory 'tests/click-root-dir/com.test.good'
=== added symlink 'tests/click-root-dir/com.test.good/1.2.3'
=== target is u'../../click-app-dir'
=== added directory 'tests/click-root-dir/com.test.multiple'
=== added symlink 'tests/click-root-dir/com.test.multiple/1.2.3'
=== target is u'../../click-app-dir'
=== added directory 'tests/click-root-dir/com.test.no-app'
=== added symlink 'tests/click-root-dir/com.test.no-app/1.2.3'
=== target is u'../../click-app-dir'
=== added directory 'tests/click-root-dir/com.test.no-hooks'
=== added symlink 'tests/click-root-dir/com.test.no-hooks/1.2.3'
=== target is u'../../click-app-dir'
=== added directory 'tests/click-root-dir/com.test.no-json'
=== added symlink 'tests/click-root-dir/com.test.no-json/1.2.3'
=== target is u'../../click-app-dir'
=== added directory 'tests/click-root-dir/com.test.no-object'
=== added symlink 'tests/click-root-dir/com.test.no-object/1.2.3'
=== target is u'../../click-app-dir'
=== added directory 'tests/click-root-dir/com.test.no-version'
=== added symlink 'tests/click-root-dir/com.test.no-version/1.2.3'
=== target is u'../../click-app-dir'
=== modified file 'tests/exec-util-test.cc'
--- tests/exec-util-test.cc 2013-12-17 20:29:43 +0000
+++ tests/exec-util-test.cc 2014-03-07 14:19:16 +0000
@@ -90,6 +90,8 @@
90 DbusTestDbusMockObject * obj = dbus_test_dbus_mock_get_object(mock, "/com/ubuntu/Upstart", "com.ubuntu.Upstart0_6", NULL);90 DbusTestDbusMockObject * obj = dbus_test_dbus_mock_get_object(mock, "/com/ubuntu/Upstart", "com.ubuntu.Upstart0_6", NULL);
9191
92 g_setenv("APP_ID", "com.test.good_application_1.2.3", TRUE);92 g_setenv("APP_ID", "com.test.good_application_1.2.3", TRUE);
93 g_setenv("TEST_CLICK_DB", "click-db-dir", TRUE);
94 g_setenv("TEST_CLICK_USER", "test-user", TRUE);
9395
94 g_spawn_command_line_sync(CLICK_EXEC_TOOL, NULL, NULL, NULL, NULL);96 g_spawn_command_line_sync(CLICK_EXEC_TOOL, NULL, NULL, NULL, NULL);
9597
@@ -114,6 +116,8 @@
114 bool got_app_desktop = false;116 bool got_app_desktop = false;
115 bool got_app_desktop_path = false;117 bool got_app_desktop_path = false;
116118
119#define APP_DIR CMAKE_SOURCE_DIR "/click-root-dir/.click/users/test-user/com.test.good"
120
117 for (i = 0; i < len; i++) {121 for (i = 0; i < len; i++) {
118 EXPECT_STREQ("SetEnv", calls[i].name);122 EXPECT_STREQ("SetEnv", calls[i].name);
119123
@@ -139,7 +143,7 @@
139 } else if (g_strcmp0(var, "XDG_RUNTIME_DIR") == 0) {143 } else if (g_strcmp0(var, "XDG_RUNTIME_DIR") == 0) {
140 got_runtime_dir = true;144 got_runtime_dir = true;
141 } else if (g_strcmp0(var, "XDG_DATA_DIRS") == 0) {145 } else if (g_strcmp0(var, "XDG_DATA_DIRS") == 0) {
142 EXPECT_TRUE(g_str_has_prefix(value, CMAKE_SOURCE_DIR "/click-app-dir:"));146 EXPECT_TRUE(g_str_has_prefix(value, APP_DIR ":"));
143 got_data_dirs = true;147 got_data_dirs = true;
144 } else if (g_strcmp0(var, "TMPDIR") == 0) {148 } else if (g_strcmp0(var, "TMPDIR") == 0) {
145 EXPECT_TRUE(g_str_has_suffix(value, "com.test.good"));149 EXPECT_TRUE(g_str_has_suffix(value, "com.test.good"));
@@ -148,7 +152,7 @@
148 EXPECT_TRUE(g_str_has_suffix(value, "com.test.good"));152 EXPECT_TRUE(g_str_has_suffix(value, "com.test.good"));
149 got_shader_dir = true;153 got_shader_dir = true;
150 } else if (g_strcmp0(var, "APP_DIR") == 0) {154 } else if (g_strcmp0(var, "APP_DIR") == 0) {
151 EXPECT_STREQ(CMAKE_SOURCE_DIR "/click-app-dir", value);155 EXPECT_STREQ(APP_DIR, value);
152 got_app_dir = true;156 got_app_dir = true;
153 } else if (g_strcmp0(var, "APP_EXEC") == 0) {157 } else if (g_strcmp0(var, "APP_EXEC") == 0) {
154 EXPECT_STREQ("foo", value);158 EXPECT_STREQ("foo", value);
@@ -156,7 +160,7 @@
156 } else if (g_strcmp0(var, "APP_DESKTOP_FILE") == 0) {160 } else if (g_strcmp0(var, "APP_DESKTOP_FILE") == 0) {
157 got_app_desktop = true;161 got_app_desktop = true;
158 } else if (g_strcmp0(var, "APP_DESKTOP_FILE_PATH") == 0) {162 } else if (g_strcmp0(var, "APP_DESKTOP_FILE_PATH") == 0) {
159 EXPECT_STREQ(CMAKE_SOURCE_DIR "/click-app-dir/application.desktop", value);163 EXPECT_STREQ(APP_DIR "/application.desktop", value);
160 got_app_desktop_path = true;164 got_app_desktop_path = true;
161 } else {165 } else {
162 g_warning("Unknown variable! %s", var);166 g_warning("Unknown variable! %s", var);
@@ -166,6 +170,8 @@
166 g_free(var);170 g_free(var);
167 }171 }
168172
173#undef APP_DIR
174
169 EXPECT_TRUE(got_app_isolation);175 EXPECT_TRUE(got_app_isolation);
170 EXPECT_TRUE(got_cache_home);176 EXPECT_TRUE(got_cache_home);
171 EXPECT_TRUE(got_config_home);177 EXPECT_TRUE(got_config_home);

Subscribers

People subscribed via source and target branches