refactor: minor UnixFd cleanups (#376)

* chore: Use std::exchange in UnixFd

This was suggested in code review for #376 .

* fix: Protect against UnixFd self-assignment

While self-assignment is rare, it is expected to be safe.  Add a check
to prevent putting the object in an invalid state.

* fix: Improve hygiene around dup system call

- Don't try to call dup on a negative value.
- Check dup return code and throw if it fails, rather than returning an
  empty UnixFd object.

* chore: Move UnixFd::close to Types.cpp

Minor convenience for applications: unistd.h doesn't have to be included
in the public header.

---------

Co-authored-by: David Reiss <dreiss@meta.com>

include/sdbus-c++/Types.h

@@ -34,7 +34,7 @@
 #include <typeinfo>
 #include <memory>
 #include <tuple>
-#include <unistd.h>
+#include <utility>
 
 namespace sdbus {
 
@@ -209,7 +209,7 @@ namespace sdbus {
         UnixFd() = default;
 
         explicit UnixFd(int fd)
-            : fd_(::dup(fd))
+            : fd_(checkedDup(fd))
         {
         }
 
@@ -225,8 +225,12 @@ namespace sdbus {
 
         UnixFd& operator=(const UnixFd& other)
         {
+            if (this == &other)
+            {
+                return *this;
+            }
             close();
-            fd_ = ::dup(other.fd_);
+            fd_ = checkedDup(other.fd_);
             return *this;
         }
 
@@ -237,9 +241,12 @@ namespace sdbus {
 
         UnixFd& operator=(UnixFd&& other)
         {
+            if (this == &other)
+            {
+                return *this;
+            }
             close();
-            fd_ = other.fd_;
+            fd_ = std::exchange(other.fd_, -1);
-            other.fd_ = -1;
             return *this;
         }
 
@@ -265,9 +272,7 @@ namespace sdbus {
 
         int release()
         {
-            auto fd = fd_;
+            return std::exchange(fd_, -1);
-            fd_ = -1;
-            return fd;
         }
 
         bool isValid() const
@@ -276,11 +281,12 @@ namespace sdbus {
         }
 
     private:
-        void close()
+        /// Closes file descriptor, but does not set it to -1.
-        {
+        void close();
-            if (fd_ >= 0)
+
-                ::close(fd_);
+        /// Returns negative argument unchanged.
-        }
+        /// Otherwise, call ::dup and throw on failure.
+        static int checkedDup(int fd);
 
         int fd_ = -1;
     };

src/Types.cpp

@@ -29,6 +29,9 @@
 #include "MessageUtils.h"
 #include SDBUS_HEADER
 #include <cassert>
+#include <cerrno>
+#include <system_error>
+#include <unistd.h>
 
 namespace sdbus {
 
@@ -64,4 +67,27 @@ bool Variant::isEmpty() const
     return msg_.isEmpty();
 }
 
+void UnixFd::close()
+{
+    if (fd_ >= 0)
+    {
+        ::close(fd_);
+    }
+}
+
+int UnixFd::checkedDup(int fd)
+{
+    if (fd < 0)
+    {
+        return fd;
+    }
+
+    int ret = ::dup(fd);
+    if (ret < 0)
+    {
+        throw std::system_error(errno, std::generic_category(), "dup failed");
+    }
+    return ret;
+}
+
 }