diff --git a/include/sdbus-c++/Types.h b/include/sdbus-c++/Types.h index 340bc6f..7027170 100644 --- a/include/sdbus-c++/Types.h +++ b/include/sdbus-c++/Types.h @@ -34,6 +34,7 @@ #include #include #include +#include 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; }; } diff --git a/src/Message.cpp b/src/Message.cpp index 45c9f0e..d482c73 100644 --- a/src/Message.cpp +++ b/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; } diff --git a/tests/integrationtests/AdaptorAndProxy_test.cpp b/tests/integrationtests/AdaptorAndProxy_test.cpp index 8f72530..6d7927e 100644 --- a/tests/integrationtests/AdaptorAndProxy_test.cpp +++ b/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) diff --git a/tests/integrationtests/TestingAdaptor.h b/tests/integrationtests/TestingAdaptor.h index 23791ae..19caa2b 100644 --- a/tests/integrationtests/TestingAdaptor.h +++ b/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 diff --git a/tests/unittests/Message_test.cpp b/tests/unittests/Message_test.cpp index c1bd214..755a4e9 100644 --- a/tests/unittests/Message_test.cpp +++ b/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) diff --git a/tests/unittests/Types_test.cpp b/tests/unittests/Types_test.cpp index c869ef6..deac3f7 100644 --- a/tests/unittests/Types_test.cpp +++ b/tests/unittests/Types_test.cpp @@ -29,8 +29,10 @@ #include #include #include +#include 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)); }