FileDescriptor: Add move and close methods
Made the following enhancements to the FileDescriptor class:
* Moved header file from file.hpp to file_descriptor.hpp to match naming
convention used in other files.
* Added move constructor and move assignment operator so FileDescriptor
objects can be put in containers like std::vector.
* Added close() method so users of class can optionally close the
descriptor explicitly and check the return code for errors. If not
explicitly closed, the descriptor will still be closed by the destructor
which ignores errors.
* Added automated tests
* Added doxygen comments
Signed-off-by: Shawn McCarney <shawnmm@us.ibm.com>
Change-Id: I1e05bbb232443ed1a79728768aaaa1efac8707ec
diff --git a/file.hpp b/file.hpp
deleted file mode 100644
index 1c9c887..0000000
--- a/file.hpp
+++ /dev/null
@@ -1,71 +0,0 @@
-#pragma once
-
-#include <unistd.h>
-namespace phosphor
-{
-namespace power
-{
-namespace util
-{
-
-/**
- * @class FileDescriptor
- *
- * Closes the file descriptor on destruction
- */
-class FileDescriptor
-{
- public:
- FileDescriptor() = default;
- FileDescriptor(const FileDescriptor&) = delete;
- FileDescriptor& operator=(const FileDescriptor&) = delete;
- FileDescriptor(FileDescriptor&&) = delete;
- FileDescriptor& operator=(FileDescriptor&&) = delete;
-
- /**
- * Constructor
- *
- * @param[in] fd - File descriptor
- */
- FileDescriptor(int fd) : fd(fd)
- {
- }
-
- ~FileDescriptor()
- {
- if (fd >= 0)
- {
- close(fd);
- }
- }
-
- int operator()()
- {
- return fd;
- }
-
- operator bool() const
- {
- return fd != -1;
- }
-
- void set(int descriptor)
- {
- if (fd >= 0)
- {
- close(fd);
- }
-
- fd = descriptor;
- }
-
- private:
- /**
- * File descriptor
- */
- int fd = -1;
-};
-
-} // namespace util
-} // namespace power
-} // namespace phosphor
diff --git a/file_descriptor.hpp b/file_descriptor.hpp
new file mode 100644
index 0000000..7929aa6
--- /dev/null
+++ b/file_descriptor.hpp
@@ -0,0 +1,136 @@
+#pragma once
+
+#include <unistd.h> // for close()
+
+namespace phosphor::power::util
+{
+
+/**
+ * @class FileDescriptor
+ *
+ * This class manages an open file descriptor.
+ *
+ * The file descriptor can be closed by calling close(). Otherwise it will be
+ * closed by the destructor.
+ *
+ * FileDescriptor objects cannot be copied, but they can be moved. This enables
+ * them to be stored in containers like std::vector.
+ */
+class FileDescriptor
+{
+ public:
+ FileDescriptor() = default;
+ FileDescriptor(const FileDescriptor&) = delete;
+ FileDescriptor& operator=(const FileDescriptor&) = delete;
+
+ /**
+ * Constructor.
+ *
+ * @param[in] fd - File descriptor
+ */
+ FileDescriptor(int fd) : fd(fd)
+ {
+ }
+
+ /**
+ * Move constructor.
+ *
+ * Transfers ownership of a file descriptor.
+ *
+ * @param other - FileDescriptor object being moved
+ */
+ FileDescriptor(FileDescriptor&& other) : fd(other.fd)
+ {
+ other.fd = -1;
+ }
+
+ /**
+ * Move assignment operator.
+ *
+ * Closes the file descriptor owned by this object, if any. Then transfers
+ * ownership of the file descriptor owned by the other object.
+ *
+ * @param other - FileDescriptor object being moved
+ */
+ FileDescriptor& operator=(FileDescriptor&& other)
+ {
+ // Verify not assigning object to itself (a = std::move(a))
+ if (this != &other)
+ {
+ set(other.fd);
+ other.fd = -1;
+ }
+ return *this;
+ }
+
+ /**
+ * Destructor.
+ *
+ * Closes the file descriptor if necessary.
+ */
+ ~FileDescriptor()
+ {
+ close();
+ }
+
+ /**
+ * Returns the file descriptor.
+ *
+ * @return File descriptor. Returns -1 if this object does not contain an
+ * open file descriptor.
+ */
+ int operator()()
+ {
+ return fd;
+ }
+
+ /**
+ * Returns whether this object contains an open file descriptor.
+ *
+ * @return true if object contains an open file descriptor, false otherwise.
+ */
+ operator bool() const
+ {
+ return fd != -1;
+ }
+
+ /**
+ * Closes the file descriptor.
+ *
+ * Does nothing if the file descriptor was not set or was already closed.
+ *
+ * @return 0 if descriptor was successfully closed. Returns -1 if an error
+ * occurred; errno will be set appropriately.
+ */
+ int close()
+ {
+ int rc = 0;
+ if (fd >= 0)
+ {
+ rc = ::close(fd);
+ fd = -1;
+ }
+ return rc;
+ }
+
+ /**
+ * Sets the file descriptor.
+ *
+ * Closes the previous file descriptor if necessary.
+ *
+ * @param[in] descriptor - File descriptor
+ */
+ void set(int descriptor)
+ {
+ close();
+ fd = descriptor;
+ }
+
+ private:
+ /**
+ * File descriptor.
+ */
+ int fd = -1;
+};
+
+} // namespace phosphor::power::util
diff --git a/gpio.hpp b/gpio.hpp
index edb9b77..0969995 100644
--- a/gpio.hpp
+++ b/gpio.hpp
@@ -1,6 +1,6 @@
#pragma once
-#include "file.hpp"
+#include "file_descriptor.hpp"
#include <linux/gpio.h>
diff --git a/test/file_descriptor_tests.cpp b/test/file_descriptor_tests.cpp
new file mode 100644
index 0000000..4273086
--- /dev/null
+++ b/test/file_descriptor_tests.cpp
@@ -0,0 +1,270 @@
+/**
+ * Copyright © 2020 IBM Corporation
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+#include "file_descriptor.hpp"
+
+#include <errno.h> // for errno
+#include <fcntl.h> // for open() and fcntl()
+#include <sys/stat.h> // for open()
+#include <sys/types.h> // for open()
+#include <unistd.h> // for fcntl()
+
+#include <utility>
+
+#include <gtest/gtest.h>
+
+using namespace phosphor::power::util;
+
+/**
+ * Returns whether the specified file descriptor is valid/open.
+ *
+ * @param[in] fd - File descriptor
+ * @return true if descriptor is valid/open, false otherwise
+ */
+bool isValid(int fd)
+{
+ return (fcntl(fd, F_GETFL) != -1) || (errno != EBADF);
+}
+
+/**
+ * Creates an open file descriptor.
+ *
+ * Verifies the file descriptor is valid.
+ *
+ * @return file descriptor
+ */
+int createOpenFileDescriptor()
+{
+ int fd = open("/etc/hosts", O_RDONLY);
+ EXPECT_NE(fd, -1);
+ EXPECT_TRUE(isValid(fd));
+ return fd;
+}
+
+TEST(FileDescriptorTests, DefaultConstructor)
+{
+ FileDescriptor descriptor;
+ EXPECT_EQ(descriptor(), -1);
+ EXPECT_FALSE(descriptor.operator bool());
+}
+
+TEST(FileDescriptorTests, IntConstructor)
+{
+ int fd = createOpenFileDescriptor();
+ FileDescriptor descriptor{fd};
+ EXPECT_EQ(descriptor(), fd);
+ EXPECT_TRUE(descriptor.operator bool());
+ EXPECT_TRUE(isValid(fd));
+}
+
+TEST(FileDescriptorTests, MoveConstructor)
+{
+ // Create first FileDescriptor object with open file descriptor
+ int fd = createOpenFileDescriptor();
+ FileDescriptor descriptor1{fd};
+ EXPECT_EQ(descriptor1(), fd);
+ EXPECT_TRUE(isValid(fd));
+
+ // Create second FileDescriptor object, moving in the contents of the first
+ FileDescriptor descriptor2{std::move(descriptor1)};
+
+ // Verify descriptor has been moved out of first object
+ EXPECT_EQ(descriptor1(), -1);
+
+ // Verify descriptor has been moved into second object
+ EXPECT_EQ(descriptor2(), fd);
+
+ // Verify descriptor is still valid/open
+ EXPECT_TRUE(isValid(fd));
+}
+
+TEST(FileDescriptorTests, MoveAssignmentOperator)
+{
+ // Test where move is valid
+ {
+ // Create first FileDescriptor object with open file descriptor
+ int fd1 = createOpenFileDescriptor();
+ FileDescriptor descriptor1{fd1};
+ EXPECT_EQ(descriptor1(), fd1);
+ EXPECT_TRUE(isValid(fd1));
+
+ // Create second FileDescriptor object with open file descriptor
+ int fd2 = createOpenFileDescriptor();
+ FileDescriptor descriptor2{fd2};
+ EXPECT_EQ(descriptor2(), fd2);
+ EXPECT_TRUE(isValid(fd2));
+
+ // Move second FileDescriptor object into the first
+ descriptor1 = std::move(descriptor2);
+
+ // Verify second file descriptor has been moved into first object
+ EXPECT_EQ(descriptor1(), fd2);
+
+ // Verify second file descriptor has been moved out of second object
+ EXPECT_EQ(descriptor2(), -1);
+
+ // Verify first file descriptor has been closed and is no longer valid
+ EXPECT_FALSE(isValid(fd1));
+
+ // Verify second file descriptor is still valid
+ EXPECT_TRUE(isValid(fd2));
+ }
+
+ // Test where move is invalid: Attempt to move object into itself
+ {
+ // Create FileDescriptor object with open file descriptor
+ int fd = createOpenFileDescriptor();
+ FileDescriptor descriptor{fd};
+ EXPECT_EQ(descriptor(), fd);
+ EXPECT_TRUE(isValid(fd));
+
+ // Try to move object into itself
+ descriptor = std::move(descriptor);
+
+ // Verify object still contains file descriptor
+ EXPECT_EQ(descriptor(), fd);
+ EXPECT_TRUE(isValid(fd));
+ }
+}
+
+TEST(FileDescriptorTests, Destructor)
+{
+ // Test where file descriptor was never set
+ {
+ FileDescriptor descriptor;
+ EXPECT_EQ(descriptor(), -1);
+ }
+
+ // Test where file descriptor was already closed
+ {
+ // Create FileDescriptor object with open file descriptor. Close the
+ // descriptor explicitly.
+ int fd = createOpenFileDescriptor();
+ {
+ FileDescriptor descriptor{fd};
+ EXPECT_EQ(descriptor(), fd);
+ EXPECT_TRUE(isValid(fd));
+
+ EXPECT_EQ(descriptor.close(), 0);
+ EXPECT_EQ(descriptor(), -1);
+ EXPECT_FALSE(isValid(fd));
+ }
+ EXPECT_FALSE(isValid(fd));
+ }
+
+ // Test where file descriptor needs to be closed
+ {
+ // Create FileDescriptor object with open file descriptor. Destructor
+ // will close descriptor.
+ int fd = createOpenFileDescriptor();
+ {
+ FileDescriptor descriptor{fd};
+ EXPECT_EQ(descriptor(), fd);
+ EXPECT_TRUE(isValid(fd));
+ }
+ EXPECT_FALSE(isValid(fd));
+ }
+}
+
+TEST(FileDescriptorTests, FunctionCallOperator)
+{
+ // Test where FileDescriptor object does not contain a valid file descriptor
+ FileDescriptor descriptor{};
+ EXPECT_EQ(descriptor(), -1);
+
+ // Test where FileDescriptor object contains a valid file descriptor
+ int fd = createOpenFileDescriptor();
+ descriptor.set(fd);
+ EXPECT_EQ(descriptor(), fd);
+}
+
+TEST(FileDescriptorTests, OperatorBool)
+{
+ // Test where FileDescriptor object does not contain a valid file descriptor
+ FileDescriptor descriptor{};
+ EXPECT_FALSE(descriptor.operator bool());
+ if (descriptor)
+ {
+ ADD_FAILURE() << "Should not have reached this line.";
+ }
+
+ // Test where FileDescriptor object contains a valid file descriptor
+ int fd = createOpenFileDescriptor();
+ descriptor.set(fd);
+ EXPECT_TRUE(descriptor.operator bool());
+ if (!descriptor)
+ {
+ ADD_FAILURE() << "Should not have reached this line.";
+ }
+
+ // Test where file descriptor has been closed
+ EXPECT_EQ(descriptor.close(), 0);
+ EXPECT_FALSE(descriptor.operator bool());
+ if (descriptor)
+ {
+ ADD_FAILURE() << "Should not have reached this line.";
+ }
+}
+
+TEST(FileDescriptorTests, Close)
+{
+ // Test where object contains an open file descriptor
+ int fd = createOpenFileDescriptor();
+ FileDescriptor descriptor{fd};
+ EXPECT_EQ(descriptor(), fd);
+ EXPECT_TRUE(isValid(fd));
+ EXPECT_EQ(descriptor.close(), 0);
+ EXPECT_EQ(descriptor(), -1);
+ EXPECT_FALSE(isValid(fd));
+
+ // Test where object does not contain an open file descriptor
+ EXPECT_EQ(descriptor(), -1);
+ EXPECT_EQ(descriptor.close(), 0);
+ EXPECT_EQ(descriptor(), -1);
+
+ // Test where close() fails due to invalid file descriptor
+ descriptor.set(999999);
+ EXPECT_EQ(descriptor.close(), -1);
+ EXPECT_EQ(errno, EBADF);
+ EXPECT_EQ(descriptor(), -1);
+}
+
+TEST(FileDescriptorTests, Set)
+{
+ // Test where object does not contain an open file descriptor
+ FileDescriptor descriptor{};
+ EXPECT_EQ(descriptor(), -1);
+ int fd1 = createOpenFileDescriptor();
+ descriptor.set(fd1);
+ EXPECT_EQ(descriptor(), fd1);
+ EXPECT_TRUE(isValid(fd1));
+
+ // Test where object contains an open file descriptor. Should close
+ // previous descriptor.
+ EXPECT_EQ(descriptor(), fd1);
+ EXPECT_TRUE(isValid(fd1));
+ int fd2 = createOpenFileDescriptor();
+ descriptor.set(fd2);
+ EXPECT_EQ(descriptor(), fd2);
+ EXPECT_FALSE(isValid(fd1));
+ EXPECT_TRUE(isValid(fd2));
+
+ // Test where -1 is specified. Should close previous descriptor.
+ EXPECT_EQ(descriptor(), fd2);
+ EXPECT_TRUE(isValid(fd2));
+ descriptor.set(-1);
+ EXPECT_EQ(descriptor(), -1);
+ EXPECT_FALSE(isValid(fd2));
+}
diff --git a/test/meson.build b/test/meson.build
index 48233c5..ca71994 100644
--- a/test/meson.build
+++ b/test/meson.build
@@ -12,3 +12,17 @@
include_directories: '..',
)
)
+
+test(
+ 'file_descriptor_tests',
+ executable(
+ 'file_descriptor_tests', 'file_descriptor_tests.cpp',
+ dependencies: [
+ gtest,
+ ],
+ link_args: dynamic_linker,
+ build_rpath: get_option('oe-sdk').enabled() ? rpath : '',
+ implicit_include_directories: false,
+ include_directories: '..',
+ )
+)