Merge #374

374: Use mir::Fd in more places. r=AlanGriffiths a=RAOF

There are a couple of places in the IPC layer and mesa-kms where we were storing
fds as raw integers with TODOs to fix it up.

Fix up those TODOs ☺.

Simplify this by not using drmClose() and its thread-safe wrapper;
this has never actually been necessary for us, as that's a DRM v1 API.

Co-authored-by: Christopher James Halse Rogers <christopher.halse.rogers@canonical.com>

include/platform/mir/graphics/platform_operation_message.h

@@ -21,6 +21,7 @@
 
 #include <vector>
 #include <cinttypes>
+#include "mir/fd.h"
 
 namespace mir
 {
@@ -30,7 +31,7 @@ namespace graphics
 struct PlatformOperationMessage
 {
     std::vector<uint8_t> data;
-    std::vector<int32_t> fds;
+    std::vector<mir::Fd> fds;
 };
 
 }

src/platforms/mesa/server/CMakeLists.txt

@@ -16,7 +16,6 @@ add_library(
 
   buffer_allocator.cpp
   display_helpers.cpp
-  drm_close_threadsafe.cpp
   gbm_buffer.cpp
   ipc_operations.cpp
   software_buffer.cpp

src/platforms/mesa/server/display_helpers.cpp

@@ -17,7 +17,6 @@
  */
 
 #include "display_helpers.h"
-#include "drm_close_threadsafe.h"
 
 #include "kms-utils/drm_mode_resources.h"
 #include "mir/graphics/gl_config.h"
@@ -49,7 +48,6 @@ namespace mgmh = mir::graphics::mesa::helpers;
 std::vector<std::shared_ptr<mgmh::DRMHelper>>
 mgmh::DRMHelper::open_all_devices(std::shared_ptr<mir::udev::Context> const& udev)
 {
-    int tmp_fd = -1;
     int error = ENODEV; //Default error is "there are no DRM devices"
 
     mir::udev::Enumerator devices(udev);
@@ -63,7 +61,7 @@ mgmh::DRMHelper::open_all_devices(std::shared_ptr<mir::udev::Context> const& ude
     for(auto& device : devices)
     {
         // If directly opening the DRM device is good enough for X it's good enough for us!
-        tmp_fd = open(device.devnode(), O_RDWR | O_CLOEXEC);
+        auto tmp_fd = mir::Fd{open(device.devnode(), O_RDWR | O_CLOEXEC)};
         if (tmp_fd < 0)
         {
             error = errno;
@@ -84,20 +82,17 @@ mgmh::DRMHelper::open_all_devices(std::shared_ptr<mir::udev::Context> const& ude
 
         if ((error = -drmSetInterfaceVersion(tmp_fd, &sv)))
         {
-            close(tmp_fd);
             mir::log_warning(
                 "Failed to set DRM interface version on device %s: %i (%s)",
                 device.devnode(),
                 error,
                 strerror(error));
-            tmp_fd = -1;
             continue;
         }
 
         // Can't use make_shared with the private constructor.
-        opened_devices.push_back(std::shared_ptr<DRMHelper>{new DRMHelper{tmp_fd}});
+        opened_devices.push_back(std::shared_ptr<DRMHelper>{new DRMHelper{std::move(tmp_fd)}});
         mir::log_info("Using DRM device %s", device.devnode());
-        tmp_fd = -1;
     }
 
     if (opened_devices.size() == 0)
@@ -126,7 +121,7 @@ mir::Fd mgmh::DRMHelper::authenticated_fd()
                 "Tried to get authenticated DRM fd before setting up the DRM master"));
 
     if (node_to_use == DRMNodeToUse::render)
-        return mir::Fd{IntOwnedFd{dup(fd)}};
+        return fd;
 
     char* busid = drmGetBusid(fd);
     if (!busid)
@@ -165,8 +160,7 @@ mir::Fd mgmh::DRMHelper::authenticated_fd()
                 std::runtime_error("Failed to authenticate DRM device magic cookie")) << boost::errinfo_errno(-ret));
     }
 
-    //TODO: remove IntOwnedFd, its how the code works now though
-    return mir::Fd{IntOwnedFd{auth_fd}};
+    return mir::Fd{auth_fd};
 }
 
 void mgmh::DRMHelper::auth_magic(drm_magic_t magic)
@@ -229,8 +223,8 @@ void mgmh::DRMHelper::set_master() const
     }
 }
 
-mgmh::DRMHelper::DRMHelper(int fd)
-    : fd{fd},
+mgmh::DRMHelper::DRMHelper(mir::Fd&& fd)
+    : fd{std::move(fd)},
       node_to_use{DRMNodeToUse::card}
 {
 }
@@ -270,9 +264,9 @@ int mgmh::DRMHelper::count_connections(int fd)
     return n_connected;
 }
 
-int mgmh::DRMHelper::open_drm_device(std::shared_ptr<mir::udev::Context> const& udev)
+mir::Fd mgmh::DRMHelper::open_drm_device(std::shared_ptr<mir::udev::Context> const& udev)
 {
-    int tmp_fd = -1;
+    mir::Fd tmp_fd;
     int error = ENODEV; //Default error is "there are no DRM devices"
 
     mir::udev::Enumerator devices(udev);
@@ -290,7 +284,7 @@ int mgmh::DRMHelper::open_drm_device(std::shared_ptr<mir::udev::Context> const&
             continue;
 
         // If directly opening the DRM device is good enough for X it's good enough for us!
-        tmp_fd = open(device.devnode(), O_RDWR | O_CLOEXEC);
+        tmp_fd = mir::Fd{open(device.devnode(), O_RDWR | O_CLOEXEC)};
         if (tmp_fd < 0)
         {
             error = errno;
@@ -308,17 +302,15 @@ int mgmh::DRMHelper::open_drm_device(std::shared_ptr<mir::udev::Context> const&
 
             if ((error = -drmSetInterfaceVersion(tmp_fd, &sv)))
             {
-                close(tmp_fd);
-                tmp_fd = -1;
+                tmp_fd = mir::Fd{};
                 continue;
             }
 
             // Stop if this device has connections to display on
             if (count_connections(tmp_fd) > 0)
                 break;
 
-            close(tmp_fd);
-            tmp_fd = -1;
+            tmp_fd = mir::Fd{};
         }
         else
             break;
@@ -336,8 +328,6 @@ int mgmh::DRMHelper::open_drm_device(std::shared_ptr<mir::udev::Context> const&
 
 mgmh::DRMHelper::~DRMHelper()
 {
-    if (fd >= 0)
-        mgm::drm_close_threadsafe(fd);
 }
 
 /*************

src/platforms/mesa/server/display_helpers.h

@@ -54,7 +54,7 @@ enum class DRMNodeToUse
 class DRMHelper : public DRMAuthentication
 {
 public:
-    DRMHelper(DRMNodeToUse const node_to_use) : fd{-1}, node_to_use{node_to_use} {}
+    DRMHelper(DRMNodeToUse const node_to_use) : node_to_use{node_to_use} {}
     ~DRMHelper();
 
     DRMHelper(const DRMHelper &) = delete;
@@ -70,19 +70,19 @@ class DRMHelper : public DRMAuthentication
     void drop_master() const;
     void set_master() const;
 
-    int fd;
+    mir::Fd fd;
     DRMNodeToUse const node_to_use;
 
 private:
-    DRMHelper(int fd);
+    explicit DRMHelper(mir::Fd&& fd);
 
     // TODO: This herustic is temporary; should be replaced with
     // handling >1 DRM device.
     int is_appropriate_device(std::shared_ptr<mir::udev::Context> const& udev, mir::udev::Device const& dev);
 
     int count_connections(int fd);
 
-    int open_drm_device(std::shared_ptr<mir::udev::Context> const& udev);
+    mir::Fd open_drm_device(std::shared_ptr<mir::udev::Context> const& udev);
 };
 
 class GBMHelper

src/platforms/mesa/server/ipc_operations.cpp

@@ -25,7 +25,6 @@
 #include "mir/libname.h"
 #include "display_helpers.h"
 #include "drm_authentication.h"
-#include "drm_close_threadsafe.h"
 #include "ipc_operations.h"
 #include "mir_toolkit/mesa/platform_operation.h"
 #include "native_buffer.h"
@@ -51,17 +50,15 @@ mir::ModuleProperties const description = {
 
 struct MesaPlatformIPCPackage : public mg::PlatformIPCPackage
 {
-    MesaPlatformIPCPackage(int drm_auth_fd) :
-        mg::PlatformIPCPackage(&description)
+    MesaPlatformIPCPackage(mir::Fd&& drm_auth_fd)
+        : mg::PlatformIPCPackage(&description),
+          authed_fd{std::move(drm_auth_fd)}
     {
-        ipc_fds.push_back(drm_auth_fd);
+        ipc_fds.push_back(authed_fd);
     }
 
-    ~MesaPlatformIPCPackage()
-    {
-        if (ipc_fds.size() > 0 && ipc_fds[0] >= 0)
-            mgm::drm_close_threadsafe(ipc_fds[0]);
-    }
+private:
+    mir::Fd const authed_fd;
 };
 }
 

src/server/frontend/session_mediator.cpp

@@ -1174,8 +1174,10 @@ void mf::SessionMediator::platform_operation(
     unsigned int const opcode = request->opcode();
     platform_request.data.assign(request->data().begin(),
                                  request->data().end());
-    platform_request.fds.assign(request->fd().begin(),
-                                request->fd().end());
+    for (auto const request_fd : request->fd())
+    {
+        platform_request.fds.emplace_back(request_fd);
+    }
 
     auto const& platform_response = ipc_operations->platform_operation(opcode, platform_request);
 

tests/include/mir/test/doubles/mock_drm.h

@@ -92,7 +92,6 @@ class MockDRM
 
     MOCK_METHOD3(open, int(char const* path, int flags, mode_t mode));
     MOCK_METHOD2(drmOpen, int(const char *name, const char *busid));
-    MOCK_METHOD1(drmClose, int(int fd));
     MOCK_METHOD3(drmIoctl, int(int fd, unsigned long request, void *arg));
 
     MOCK_METHOD1(drmModeGetResources, drmModeResPtr(int fd));

tests/mir_test_doubles/mock_drm.cpp

@@ -261,9 +261,6 @@ mtd::MockDRM::MockDRM()
                     return fd;
                 }));
 
-    ON_CALL(*this, drmClose(_))
-        .WillByDefault(Invoke([](int fd){ return close(fd); }));
-
     ON_CALL(*this, drmModeGetResources(_))
         .WillByDefault(
             Invoke(
@@ -445,11 +442,6 @@ int drmOpen(const char *name, const char *busid)
     return global_mock->drmOpen(name, busid);
 }
 
-int drmClose(int fd)
-{
-    return global_mock->drmClose(fd);
-}
-
 int drmIoctl(int fd, unsigned long request, void *arg)
 {
     return global_mock->drmIoctl(fd, request, arg);

tests/mir_test_framework/stubbed_graphics_platform.cpp

@@ -229,7 +229,7 @@ class StubIpcOps : public mg::PlatformIpcOperations
                     std::runtime_error("Failed to write to pipe in 'echo_fd' operation"));
             }
 
-            reply.fds.push_back(dup(pipe.read_fd()));
+            reply.fds.push_back(pipe.read_fd());
         }
         else
         {

tests/unit-tests/platforms/mesa/kms/test_display.cpp

@@ -342,9 +342,6 @@ TEST_F(MesaDisplayTest, create_display_drm_failure)
                 InvokeWithoutArgs([]() { errno = ENODEV; }),
                 Return(-1)));
 
-    EXPECT_CALL(mock_drm, drmClose(_))
-        .Times(Exactly(0));
-
     EXPECT_THROW(
     {
         auto display = create_display(create_platform());
@@ -366,10 +363,6 @@ TEST_F(MesaDisplayTest, create_display_kms_failure)
     EXPECT_CALL(mock_drm, drmModeFreeResources(_))
         .Times(Exactly(0));
 
-    // There are 2 DRM device nodes in our mock environment.
-    EXPECT_CALL(mock_drm, drmClose(_))
-        .Times(Exactly(2));
-
     EXPECT_THROW({
         auto display = create_display(platform);
     }, std::runtime_error) << "Expected that c'tor of mgm::Display throws";
@@ -386,9 +379,6 @@ TEST_F(MesaDisplayTest, create_display_gbm_failure)
     EXPECT_CALL(mock_gbm, gbm_device_destroy(_))
         .Times(Exactly(0));
 
-    EXPECT_CALL(mock_drm, drmClose(_))
-        .Times(Exactly(2));
-
     EXPECT_THROW({
         auto platform = create_platform();
     }, std::runtime_error) << "Expected c'tor of Platform to throw an exception";