Merge lp:~jamesodhunt/upstart/check-for-overlayfs into lp:upstart

Proposed by James Hunt on 2013-10-16
Status: Merged
Merged at revision: 1560
Proposed branch: lp:~jamesodhunt/upstart/check-for-overlayfs
Merge into: lp:upstart
Diff against target: 191 lines (+156/-3)
4 files modified
ChangeLog (+13/-1)
test/Makefile.am (+14/-1)
test/test_util_common.c (+1/-1)
test/tests/test_util_check_env.c (+128/-0)
To merge this branch: bzr merge lp:~jamesodhunt/upstart/check-for-overlayfs
Reviewer Review Type Date Requested Status
Steve Langasek 2013-10-16 Needs Fixing on 2013-10-16
Review via email: mp+191393@code.launchpad.net

Description of the change

* test/tests/test_util_check_env.c: New test to look for
  overlayfs filesystems which could cause tests to fail.
* test/Makefile.am: Added test_util_check_env meta-test.
* test/test_util_common.c: Formatting.

This branch introduces a new meta-test that currently checks for overlayfs. It will produce a warning that tests will probably fail (due to bug LP:#882147) but does not actually abort the test run since:

- the invoker may know better whether a particular FS could cause problems for the tests.
- overlayfs may one day get fixed so hard-failing would be incorrect behaviour.

Note that the new test will be run before any other test. Further, note that since the overlayfs check is a separate test, if an individual test is run in an overlayfs environment, the invoker may be unaware of the strange resultant test failures. However, having a separate test does mean that we avoid linking every test against libnih-dbus et al.

To post a comment you must log in.
Steve Langasek (vorlon) wrote :

does not merge cleanly, please sync with trunk and resubmit.

review: Needs Fixing
Steve Langasek (vorlon) wrote :

> * Returns: TRUE if any mount point is using overlayfs, else FALSE.

Why warn about *any* overlayfs? Shouldn't we only care about whether the path to our temp directory is affected by overlayfs?

James Hunt (jamesodhunt) wrote :

Ok branch updated.

Steve Langasek (vorlon) wrote :

Since we're checking the type of a single mountpoint, we should be able to use statfs instead of walking around in /proc/self/mounts. E.g.:

        struct statfs statbuf;

        /* Create a file in the temporary work area */
        TEST_FILENAME (path);
        fclose (fopen (path, "w"));

        assert0 (statfs (path, &statbuf));

        if (statbuf.f_type == OVERLAYFS_SUPER_MAGIC) {
                nih_warn ("Mountpoint for '%s' (needed by the Upstart tests) is an overlayfs "
                                        "filesystem, which does not support inotify.",
                                        path);
                found = TRUE;
        }

        assert0 (unlink (path));

        return found;

It seems that the linux-kernel-headers package does not currently export a define for OVERLAYFS_SUPER_MAGIC, which is an oversight. We can copy this define from the kernel source for now, since it shouldn't change, and ask the kernel team to have it properly exported.

#define OVERLAYFS_SUPER_MAGIC 0x794c764f

What do you think of this approach?

James Hunt (jamesodhunt) wrote :

Hi Steve,

Yes, that would clearly be more efficient, but I'm not wholly convinced we need to do this since the current approach is simple, it works and we only call this test-only function once anyway. Additionally, the current approach avoids the need to retain the '#ifndef OVERLAYFS_SUPER_MAGIC' forever to handle non-Ubuntu systems.

I've raised bug 1247769 on the kernel headers for the missing define though.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'ChangeLog'
2--- ChangeLog 2013-10-04 21:34:25 +0000
3+++ ChangeLog 2013-10-17 08:19:01 +0000
4@@ -1,4 +1,16 @@
5-2013-01-04 Steve Langasek <steve.langasek@ubuntu.com
6+2013-10-17 James Hunt <james.hunt@ubuntu.com>
7+
8+ * test/tests/test_util_check_env.c:
9+ - check_for_overlayfs(): Only consider temporary work area.
10+
11+2013-10-16 James Hunt <james.hunt@ubuntu.com>
12+
13+ * test/tests/test_util_check_env.c: New test to look for
14+ overlayfs filesystems which could cause tests to fail.
15+ * test/Makefile.am: Added test_util_check_env meta-test.
16+ * test/test_util_common.c: Formatting.
17+
18+2013-10-04 Steve Langasek <steve.langasek@ubuntu.com
19
20 * extra/upstart-local-bridge.c: use SOCKET_PATH in our event
21 environment, instead of clobbering PATH. (LP: #1235480)
22
23=== modified file 'test/Makefile.am'
24--- test/Makefile.am 2013-06-25 09:19:05 +0000
25+++ test/Makefile.am 2013-10-17 08:19:01 +0000
26@@ -18,4 +18,17 @@
27 -I$(top_srcdir)/intl
28
29 check_LIBRARIES = libtest_util_common.a
30-libtest_util_common_a_SOURCES = test_util_common.c test_util_common.h
31+libtest_util_common_a_SOURCES = \
32+ test_util_common.c \
33+ test_util_common.h
34+
35+TESTS = test_util_check_env
36+check_PROGRAMS = $(TESTS)
37+
38+.PHONY: tests
39+tests: $(check_PROGRAMS)
40+
41+test_util_check_env_SOURCES = tests/test_util_check_env.c
42+test_util_check_env_LDADD = \
43+ libtest_util_common.a \
44+ $(NIH_LIBS)
45
46=== modified file 'test/test_util_common.c'
47--- test/test_util_common.c 2013-10-02 08:59:20 +0000
48+++ test/test_util_common.c 2013-10-17 08:19:01 +0000
49@@ -162,7 +162,7 @@
50 * within a reasonable period of time.
51 */
52 for (i = 0; i < loops; i++) {
53- sleep (1);
54+ sleep (1);
55
56 RUN_COMMAND (NULL, cmd, &output, &lines);
57
58
59=== added directory 'test/tests'
60=== added file 'test/tests/test_util_check_env.c'
61--- test/tests/test_util_check_env.c 1970-01-01 00:00:00 +0000
62+++ test/tests/test_util_check_env.c 2013-10-17 08:19:01 +0000
63@@ -0,0 +1,128 @@
64+/* upstart
65+ *
66+ * test_util_check_env.c - meta test to ensure environment sane for
67+ * running tests.
68+ *
69+ * Copyright © 2013 Canonical Ltd.
70+ * Author: James Hunt <james.hunt@canonical.com>
71+ *
72+ * This program is free software; you can redistribute it and/or modify
73+ * it under the terms of the GNU General Public License version 2, as
74+ * published by the Free Software Foundation.
75+ *
76+ * This program is distributed in the hope that it will be useful,
77+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
78+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
79+ * GNU General Public License for more details.
80+ *
81+ * You should have received a copy of the GNU General Public License along
82+ * with this program; if not, write to the Free Software Foundation, Inc.,
83+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
84+ */
85+
86+#include <stdio.h>
87+#include <stdlib.h>
88+#include <sys/stat.h>
89+#include <limits.h>
90+#include <unistd.h>
91+#include <mntent.h>
92+
93+#include <nih/string.h>
94+#include <nih/test.h>
95+#include <nih/logging.h>
96+
97+#include "test_util_common.h"
98+
99+/**
100+ * check_for_overlayfs:
101+ *
102+ * Determine if the mount point used by the tests for creating temporary
103+ * files is using overlayfs.
104+ *
105+ * Returns: TRUE if temporary work area is on overlayfs, else FALSE.
106+ **/
107+int
108+check_for_overlayfs (void)
109+{
110+ struct stat statbuf;
111+ char path[PATH_MAX];
112+ const char *fs = "overlayfs";
113+ unsigned int maj;
114+ unsigned int min;
115+ const char *mounts = "/proc/self/mounts";
116+ FILE *mtab;
117+ struct mntent *mnt;
118+ int found = FALSE;
119+
120+ /* Create a file in the temporary work area */
121+ TEST_FILENAME (path);
122+ fclose (fopen (path, "w"));
123+
124+ /* Check it exits */
125+ assert0 (stat (path, &statbuf));
126+
127+ /* Extract device details */
128+ maj = major (statbuf.st_dev);
129+ min = minor (statbuf.st_dev);
130+
131+ mtab = fopen (mounts, "r");
132+ TEST_NE_P (mtab, NULL);
133+
134+ /* Look through mount table */
135+ while ((mnt = getmntent (mtab))) {
136+ unsigned int mount_maj;
137+ unsigned int mount_min;
138+
139+ assert0 (stat (mnt->mnt_dir, &statbuf));
140+ mount_maj = major (statbuf.st_dev);
141+ mount_min = minor (statbuf.st_dev);
142+
143+ if (! strcmp (mnt->mnt_type, fs) && mount_maj == maj && mount_min == min) {
144+ found = TRUE;
145+ nih_warn ("Mountpoint '%s' (needed by the Upstart tests) is an '%s' "
146+ "filesystem which does not support inotify.",
147+ mnt->mnt_dir,
148+ fs);
149+ goto out;
150+ }
151+ }
152+
153+out:
154+ fclose (mtab);
155+ assert0 (unlink (path));
156+
157+ return found;
158+}
159+
160+/**
161+ * test_checks:
162+ *
163+ * Perform any checks necessary before real tests are run.
164+ **/
165+void
166+test_checks (void)
167+{
168+ TEST_GROUP ("test environment");
169+
170+ /*
171+ * Warn (*) if overlayfs detected.
172+ *
173+ * (*) - Don't fail in the hope that one day someone might fix
174+ * overlayfs.
175+ */
176+ TEST_FEATURE ("checking for overlayfs");
177+ if (check_for_overlayfs ()) {
178+ nih_warn ("Found overlayfs mounts");
179+ nih_warn ("This environment will probably cause tests to fail mysteriously!!");
180+ nih_warn ("See bug LP:#882147 for further details.");
181+ }
182+}
183+
184+int
185+main (int argc,
186+ char *argv[])
187+{
188+ test_checks ();
189+
190+ return 0;
191+}

Subscribers

People subscribed via source and target branches