Switch from plain UnixFd to owning UnixFd (#69)

include/sdbus-c++/Types.h

@@ -34,6 +34,7 @@
 #include <typeinfo>
 #include <memory>
 #include <tuple>
+#include <unistd.h>
 
 namespace sdbus {
 
@@ -176,25 +177,104 @@ namespace sdbus {
         using std::string::operator=;
     };
 
+    struct adopt_fd_t { explicit adopt_fd_t() = default; };
+#ifdef __cpp_inline_variables
+    inline constexpr adopt_fd_t adopt_fd{};
+#else
+    constexpr adopt_fd_t adopt_fd{};
+#endif
+
     /********************************************//**
      * @struct UnixFd
      *
-     * Representation of Unix file descriptor D-Bus type
+     * UnixFd is a representation of file descriptor D-Bus type that owns
+     * the underlying fd, provides access to it, and closes the fd when
+     * the UnixFd goes out of scope.
+     *
+     * UnixFd can be default constructed (owning invalid fd), or constructed from
+     * an explicitly provided fd by either duplicating or adopting that fd as-is.
      *
      ***********************************************/
-    struct UnixFd
+    class UnixFd
     {
-        int fd_ = -1;
-
+    public:
         UnixFd() = default;
-        UnixFd(int fd)
-            : fd_(fd)
-        {}
 
-        operator int() const
+        explicit UnixFd(int fd)
+            : fd_(::dup(fd))
+        {
+        }
+
+        UnixFd(int fd, adopt_fd_t)
+            : fd_(fd)
+        {
+        }
+
+        UnixFd(const UnixFd& other)
+        {
+            *this = other;
+        }
+
+        UnixFd& operator=(const UnixFd& other)
+        {
+            close();
+            fd_ = ::dup(other.fd_);
+            return *this;
+        }
+
+        UnixFd(UnixFd&& other)
+        {
+            *this = std::move(other);
+        }
+
+        UnixFd& operator=(UnixFd&& other)
+        {
+            close();
+            fd_ = other.fd_;
+            other.fd_ = -1;
+            return *this;
+        }
+
+        ~UnixFd()
+        {
+            close();
+        }
+
+        int get() const
         {
             return fd_;
         }
+
+        void reset(int fd = -1)
+        {
+            *this = UnixFd{fd};
+        }
+
+        void reset(int fd, adopt_fd_t)
+        {
+            *this = UnixFd{fd, adopt_fd};
+        }
+
+        int release()
+        {
+            auto fd = fd_;
+            fd_ = -1;
+            return fd;
+        }
+
+        bool isValid() const
+        {
+            return fd_ >= 0;
+        }
+
+    private:
+        void close()
+        {
+            if (fd_ >= 0)
+                ::close(fd_);
+        }
+
+        int fd_ = -1;
     };
 
 }

src/Message.cpp

@@ -218,7 +218,8 @@ Message& Message::operator<<(const Signature &item)
 
 Message& Message::operator<<(const UnixFd &item)
 {
-    auto r = sd_bus_message_append_basic((sd_bus_message*)msg_, SD_BUS_TYPE_UNIX_FD, &item.fd_);
+    auto fd = item.get();
+    auto r = sd_bus_message_append_basic((sd_bus_message*)msg_, SD_BUS_TYPE_UNIX_FD, &fd);
     SDBUS_THROW_ERROR_IF(r < 0, "Failed to serialize a UnixFd value", -r);
 
     return *this;
@@ -394,12 +395,15 @@ Message& Message::operator>>(Signature &item)
 
 Message& Message::operator>>(UnixFd &item)
 {
-    auto r = sd_bus_message_read_basic((sd_bus_message*)msg_, SD_BUS_TYPE_UNIX_FD, &item.fd_);
+    int fd = -1;
+    auto r = sd_bus_message_read_basic((sd_bus_message*)msg_, SD_BUS_TYPE_UNIX_FD, &fd);
     if (r == 0)
         ok_ = false;
 
     SDBUS_THROW_ERROR_IF(r < 0, "Failed to deserialize a UnixFd value", -r);
 
+    item.reset(fd);
+
     return *this;
 }
 

tests/integrationtests/AdaptorAndProxy_test.cpp

@@ -213,7 +213,7 @@ TEST_F(SdbusTestObject, CallsMethodWithObjectPathSuccesfully)
 TEST_F(SdbusTestObject, CallsMethodWithUnixFdSuccesfully)
 {
     auto resUnixFd = m_proxy->getUnixFd();
-    ASSERT_THAT(resUnixFd, Gt(UNIX_FD_VALUE));
+    ASSERT_THAT(resUnixFd.get(), Gt(UNIX_FD_VALUE));
 }
 
 TEST_F(SdbusTestObject, CallsMethodWithComplexTypeSuccesfully)

tests/integrationtests/TestingAdaptor.h

@@ -160,7 +160,7 @@ protected:
     }
     sdbus::UnixFd getUnixFd() const
     {
-        return UNIX_FD_VALUE;
+        return sdbus::UnixFd{UNIX_FD_VALUE};
     }
 
     ComplexType getComplex() const

tests/unittests/Message_test.cpp

@@ -131,7 +131,7 @@ TEST(AMessage, CanCarryAUnixFd)
 {
     auto msg = sdbus::createPlainMessage();
 
-    sdbus::UnixFd dataWritten = 0;
+    sdbus::UnixFd dataWritten{0};
     msg << dataWritten;
 
     msg.seal();
@@ -139,7 +139,7 @@ TEST(AMessage, CanCarryAUnixFd)
     sdbus::UnixFd dataRead;
     msg >> dataRead;
 
-    ASSERT_THAT(dataRead, Gt(dataWritten));
+    ASSERT_THAT(dataRead.get(), Gt(dataWritten.get()));
 }
 
 TEST(AMessage, CanCarryAVariant)

tests/unittests/Types_test.cpp

@@ -29,8 +29,10 @@
 #include <gtest/gtest.h>
 #include <gmock/gmock.h>
 #include <cstdint>
+#include <sys/eventfd.h>
 
 using ::testing::Eq;
+using ::testing::Gt;
 using namespace std::string_literals;
 
 namespace
@@ -242,9 +244,103 @@ TEST(ASignature, CanBeConstructedFromStdString)
     ASSERT_THAT(sdbus::Signature{aSignature}, Eq(aSignature));
 }
 
-TEST(AUnixFd, CanBeConstructedFromInt)
+TEST(AUnixFd, DuplicatesAndOwnsFdUponStandardConstruction)
 {
-    int fd{2};
+    auto fd = ::eventfd(0, EFD_SEMAPHORE | EFD_NONBLOCK);
 
-    ASSERT_THAT((int)sdbus::UnixFd{fd}, Eq(fd));
+    EXPECT_THAT(sdbus::UnixFd{fd}.get(), Gt(fd));
+    EXPECT_THAT(::close(fd), Eq(0));
+}
+
+TEST(AUnixFd, AdoptsAndOwnsFdAsIsUponAdoptionConstruction)
+{
+    auto fd = ::eventfd(0, EFD_SEMAPHORE | EFD_NONBLOCK);
+
+    EXPECT_THAT(sdbus::UnixFd(fd, sdbus::adopt_fd).get(), Eq(fd));
+    EXPECT_THAT(::close(fd), Eq(-1));
+}
+
+TEST(AUnixFd, DuplicatesFdUponCopyConstruction)
+{
+    sdbus::UnixFd unixFd(::eventfd(0, EFD_SEMAPHORE | EFD_NONBLOCK));
+
+    sdbus::UnixFd unixFdCopy{unixFd};
+
+    EXPECT_THAT(unixFdCopy.get(), Gt(unixFd.get()));
+}
+
+TEST(AUnixFd, TakesOverFdUponMoveConstruction)
+{
+    auto fd = ::eventfd(0, EFD_SEMAPHORE | EFD_NONBLOCK);
+    sdbus::UnixFd unixFd(fd, sdbus::adopt_fd);
+
+    sdbus::UnixFd unixFdNew{std::move(unixFd)};
+
+    EXPECT_FALSE(unixFd.isValid());
+    EXPECT_THAT(unixFdNew.get(), Eq(fd));
+}
+
+TEST(AUnixFd, ClosesFdProperlyUponDestruction)
+{
+    int fd, fdCopy;
+    {
+        fd = ::eventfd(0, EFD_SEMAPHORE | EFD_NONBLOCK);
+        sdbus::UnixFd unixFd(fd, sdbus::adopt_fd);
+        auto unixFdNew = std::move(unixFd);
+        auto unixFdCopy = unixFdNew;
+        fdCopy = unixFdCopy.get();
+    }
+
+    EXPECT_THAT(::close(fd), Eq(-1));
+    EXPECT_THAT(::close(fdCopy), Eq(-1));
+}
+
+TEST(AUnixFd, DoesNotCloseReleasedFd)
+{
+    auto fd = ::eventfd(0, EFD_SEMAPHORE | EFD_NONBLOCK);
+    int fdReleased;
+    {
+        sdbus::UnixFd unixFd(fd, sdbus::adopt_fd);
+        fdReleased = unixFd.release();
+        EXPECT_FALSE(unixFd.isValid());
+    }
+
+    EXPECT_THAT(fd, Eq(fdReleased));
+    EXPECT_THAT(::close(fd), Eq(0));
+}
+
+TEST(AUnixFd, ClosesFdOnReset)
+{
+    auto fd = ::eventfd(0, EFD_SEMAPHORE | EFD_NONBLOCK);
+    sdbus::UnixFd unixFd(fd, sdbus::adopt_fd);
+
+    unixFd.reset();
+
+    EXPECT_FALSE(unixFd.isValid());
+    EXPECT_THAT(::close(fd), Eq(-1));
+}
+
+TEST(AUnixFd, DuplicatesNewFdAndClosesOriginalFdOnReset)
+{
+    auto fd = ::eventfd(0, EFD_SEMAPHORE | EFD_NONBLOCK);
+    sdbus::UnixFd unixFd(fd, sdbus::adopt_fd);
+    auto newFd = ::eventfd(0, EFD_SEMAPHORE | EFD_NONBLOCK);
+
+    unixFd.reset(newFd);
+
+    EXPECT_THAT(unixFd.get(), Gt(newFd));
+    EXPECT_THAT(::close(fd), Eq(-1));
+    EXPECT_THAT(::close(newFd), Eq(0));
+}
+
+TEST(AUnixFd, TakesOverNewFdAndClosesOriginalFdOnAdoptingReset)
+{
+    auto fd = ::eventfd(0, EFD_SEMAPHORE | EFD_NONBLOCK);
+    sdbus::UnixFd unixFd(fd, sdbus::adopt_fd);
+    auto newFd = ::eventfd(0, EFD_SEMAPHORE | EFD_NONBLOCK);
+
+    unixFd.reset(newFd, sdbus::adopt_fd);
+
+    EXPECT_THAT(unixFd.get(), Eq(newFd));
+    EXPECT_THAT(::close(fd), Eq(-1));
 }