Mir

Merge lp:~afrantzis/mir/drm-device-fd-close-on-exec into lp:mir

Proposed by Alexandros Frantzis
Status: Merged
Approved by: Daniel van Vugt
Approved revision: no longer in the source branch.
Merged at revision: 1431
Proposed branch: lp:~afrantzis/mir/drm-device-fd-close-on-exec
Merge into: lp:mir
Diff against target: 221 lines (+89/-10)
7 files modified
include/test/mir_test_doubles/mock_drm.h (+1/-0)
src/platform/graphics/mesa/display_helpers.cpp (+8/-1)
tests/mir_test_doubles/mock_drm.cpp (+7/-4)
tests/unit-tests/graphics/mesa/CMakeLists.txt (+1/-0)
tests/unit-tests/graphics/mesa/test_display.cpp (+1/-1)
tests/unit-tests/graphics/mesa/test_drm_helper.cpp (+65/-0)
tests/unit-tests/graphics/mesa/test_platform.cpp (+6/-4)
To merge this branch: bzr merge lp:~afrantzis/mir/drm-device-fd-close-on-exec
Reviewer Review Type Date Requested Status
Daniel van Vugt Approve
PS Jenkins bot (community) continuous-integration Approve
Alan Griffiths Approve
Review via email: mp+208349@code.launchpad.net

Commit message

mesa: Ensure we close drm device file descriptors on exec

Description of the change

mesa: Ensure we close drm device file descriptors on exec

This is needed for https://bugs.launchpad.net/mir/+bug/1284081 , but it's not a complete fix, because Mesa internally dup()s our drm fd without setting the close-on-exec flag.

To post a comment you must log in.
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

Looks sensible

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

This looks like a potential mismatch:

3 parameters mocked:
8 + MOCK_METHOD3(open, int(char const* path, int flags, mode_t mode));

2 parameters used:
35 + tmp_fd = open(device.devnode(), O_RDWR | O_CLOEXEC);

Maybe it mocks correctly still? I guess the expectations would fail otherwise. The real function is:
/usr/include/fcntl.h:extern int open (const char *__file, int __oflag, ...) __nonnull ((1));

Seems to work.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'include/test/mir_test_doubles/mock_drm.h'
2--- include/test/mir_test_doubles/mock_drm.h 2013-08-28 03:41:48 +0000
3+++ include/test/mir_test_doubles/mock_drm.h 2014-02-26 12:30:46 +0000
4@@ -88,6 +88,7 @@
5 MockDRM();
6 ~MockDRM() noexcept;
7
8+ MOCK_METHOD3(open, int(char const* path, int flags, mode_t mode));
9 MOCK_METHOD2(drmOpen, int(const char *name, const char *busid));
10 MOCK_METHOD1(drmClose, int(int fd));
11 MOCK_METHOD3(drmIoctl, int(int fd, unsigned long request, void *arg));
12
13=== modified file 'src/platform/graphics/mesa/display_helpers.cpp'
14--- src/platform/graphics/mesa/display_helpers.cpp 2014-01-22 08:32:55 +0000
15+++ src/platform/graphics/mesa/display_helpers.cpp 2014-02-26 12:30:46 +0000
16@@ -65,6 +65,13 @@
17 BOOST_THROW_EXCEPTION(
18 std::runtime_error("Failed to open DRM device for authenticated fd"));
19
20+ if (fcntl(auth_fd, F_SETFD, fcntl(auth_fd, F_GETFD) | FD_CLOEXEC) == -1)
21+ {
22+ BOOST_THROW_EXCEPTION(
23+ boost::enable_error_info(
24+ std::runtime_error("Failed to set FD_CLOEXEC for authenticated drm fd")));
25+ }
26+
27 drm_magic_t magic;
28 int ret = -1;
29 if ((ret = drmGetMagic(auth_fd, &magic)) < 0)
30@@ -198,7 +205,7 @@
31 continue;
32
33 // If directly opening the DRM device is good enough for X it's good enough for us!
34- tmp_fd = open(device.devnode(), O_RDWR, O_CLOEXEC);
35+ tmp_fd = open(device.devnode(), O_RDWR | O_CLOEXEC);
36 if (tmp_fd < 0)
37 {
38 error = errno;
39
40=== modified file 'tests/mir_test_doubles/mock_drm.cpp'
41--- tests/mir_test_doubles/mock_drm.cpp 2014-01-13 06:12:33 +0000
42+++ tests/mir_test_doubles/mock_drm.cpp 2014-02-26 12:30:46 +0000
43@@ -229,6 +229,9 @@
44
45 global_mock = this;
46
47+ ON_CALL(*this, open(_,_,_))
48+ .WillByDefault(Return(fake_drm.fd()));
49+
50 ON_CALL(*this, drmOpen(_,_))
51 .WillByDefault(Return(fake_drm.fd()));
52
53@@ -405,7 +408,7 @@
54 {
55 char const* drm_prefix = "/dev/dri/";
56 if (!strncmp(path, drm_prefix, strlen(drm_prefix)))
57- return global_mock->drmOpen("i915", NULL);
58+ return global_mock->open(path, flags, mode);
59
60 int (*real_open)(char const *path, int flags, mode_t mode);
61 *(void **)(&real_open) = dlsym(RTLD_NEXT, "open");
62@@ -417,7 +420,7 @@
63 {
64 char const* drm_prefix = "/dev/dri/";
65 if (!strncmp(path, drm_prefix, strlen(drm_prefix)))
66- return global_mock->drmOpen("i915", NULL);
67+ return global_mock->open(path, flags, mode);
68
69 int (*real_open64)(char const *path, int flags, mode_t mode);
70 *(void **)(&real_open64) = dlsym(RTLD_NEXT, "open64");
71@@ -429,7 +432,7 @@
72 {
73 char const* drm_prefix = "/dev/dri/";
74 if (!strncmp(path, drm_prefix, strlen(drm_prefix)))
75- return global_mock->drmOpen("i915", NULL);
76+ return global_mock->open(path, flags, mode);
77
78 int (*real_open)(char const *path, int flags, mode_t mode);
79 *(void **)(&real_open) = dlsym(RTLD_NEXT, "__open");
80@@ -441,7 +444,7 @@
81 {
82 char const* drm_prefix = "/dev/dri/";
83 if (!strncmp(path, drm_prefix, strlen(drm_prefix)))
84- return global_mock->drmOpen("i915", NULL);
85+ return global_mock->open(path, flags, mode);
86
87 int (*real_open64)(char const *path, int flags, mode_t mode);
88 *(void **)(&real_open64) = dlsym(RTLD_NEXT, "__open64");
89
90=== modified file 'tests/unit-tests/graphics/mesa/CMakeLists.txt'
91--- tests/unit-tests/graphics/mesa/CMakeLists.txt 2014-01-22 08:32:55 +0000
92+++ tests/unit-tests/graphics/mesa/CMakeLists.txt 2014-02-26 12:30:46 +0000
93@@ -17,6 +17,7 @@
94 ${CMAKE_CURRENT_SOURCE_DIR}/test_native_platform.cpp
95 ${CMAKE_CURRENT_SOURCE_DIR}/test_anonymous_shm_file.cpp
96 ${CMAKE_CURRENT_SOURCE_DIR}/test_shm_buffer.cpp
97+ ${CMAKE_CURRENT_SOURCE_DIR}/test_drm_helper.cpp
98 )
99
100 set(UNIT_TEST_SOURCES ${UNIT_TEST_SOURCES} PARENT_SCOPE)
101
102=== modified file 'tests/unit-tests/graphics/mesa/test_display.cpp'
103--- tests/unit-tests/graphics/mesa/test_display.cpp 2014-02-21 15:32:30 +0000
104+++ tests/unit-tests/graphics/mesa/test_display.cpp 2014-02-26 12:30:46 +0000
105@@ -332,7 +332,7 @@
106 {
107 using namespace testing;
108
109- EXPECT_CALL(mock_drm, drmOpen(_,_))
110+ EXPECT_CALL(mock_drm, open(_,_,_))
111 .Times(AtLeast(1))
112 .WillRepeatedly(Return(-1));
113
114
115=== added file 'tests/unit-tests/graphics/mesa/test_drm_helper.cpp'
116--- tests/unit-tests/graphics/mesa/test_drm_helper.cpp 1970-01-01 00:00:00 +0000
117+++ tests/unit-tests/graphics/mesa/test_drm_helper.cpp 2014-02-26 12:30:46 +0000
118@@ -0,0 +1,65 @@
119+/*
120+ * Copyright © 2014 Canonical Ltd.
121+ *
122+ * This program is free software: you can redistribute it and/or modify
123+ * it under the terms of the GNU General Public License version 3 as
124+ * published by the Free Software Foundation.
125+ *
126+ * This program is distributed in the hope that it will be useful,
127+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
128+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
129+ * GNU General Public License for more details.
130+ *
131+ * You should have received a copy of the GNU General Public License
132+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
133+ *
134+ * Authored by: Alexandros Frantzis <alexandros.frantzis@canonical.com>
135+ */
136+
137+#include "src/platform/graphics/mesa/display_helpers.h"
138+#include "mir/udev/wrapper.h"
139+
140+#include "mir_test_framework/udev_environment.h"
141+#include "mir_test_doubles/mock_drm.h"
142+
143+#include <fcntl.h>
144+
145+#include <gtest/gtest.h>
146+#include <gmock/gmock.h>
147+
148+namespace mgm = mir::graphics::mesa;
149+namespace mtf = mir::mir_test_framework;
150+namespace mtd = mir::test::doubles;
151+
152+namespace
153+{
154+
155+MATCHER_P(FlagSet, flag, "")
156+{
157+ return (arg & flag);
158+}
159+
160+class DRMHelperTest : public ::testing::Test
161+{
162+public:
163+ DRMHelperTest()
164+ {
165+ fake_devices.add_standard_device("standard-drm-devices");
166+ }
167+
168+protected:
169+ ::testing::NiceMock<mtd::MockDRM> mock_drm;
170+ mtf::UdevEnvironment fake_devices;
171+};
172+
173+}
174+
175+TEST_F(DRMHelperTest, closes_drm_fd_on_exec)
176+{
177+ using namespace testing;
178+
179+ EXPECT_CALL(mock_drm, open(_, FlagSet(O_CLOEXEC), _));
180+
181+ mgm::helpers::DRMHelper drm_helper;
182+ drm_helper.setup(std::make_shared<mir::udev::Context>());
183+}
184
185=== modified file 'tests/unit-tests/graphics/mesa/test_platform.cpp'
186--- tests/unit-tests/graphics/mesa/test_platform.cpp 2014-02-19 14:01:37 +0000
187+++ tests/unit-tests/graphics/mesa/test_platform.cpp 2014-02-26 12:30:46 +0000
188@@ -29,6 +29,7 @@
189 #include <gtest/gtest.h>
190
191 #include "mir_test_framework/udev_environment.h"
192+#include "mir_test/pipe.h"
193
194 #include "mir_test_doubles/mock_drm.h"
195 #include "mir_test_doubles/mock_gbm.h"
196@@ -76,12 +77,13 @@
197 TEST_F(MesaGraphicsPlatform, get_ipc_package)
198 {
199 using namespace testing;
200- const int auth_fd{66};
201+ mir::test::Pipe auth_pipe;
202+ int const auth_fd{auth_pipe.read_fd()};
203
204 /* First time for master DRM fd, second for authenticated fd */
205+ EXPECT_CALL(mock_drm, open(_,_,_))
206+ .WillOnce(Return(mock_drm.fake_drm.fd()));
207 EXPECT_CALL(mock_drm, drmOpen(_,_))
208- .Times(2)
209- .WillOnce(Return(mock_drm.fake_drm.fd()))
210 .WillOnce(Return(auth_fd));
211
212 /* Expect proper authorization */
213@@ -107,7 +109,7 @@
214 {
215 using namespace ::testing;
216
217- EXPECT_CALL(mock_drm, drmOpen(_,_))
218+ EXPECT_CALL(mock_drm, open(_,_,_))
219 .WillRepeatedly(Return(-1));
220
221 try

Subscribers

People subscribed via source and target branches