From 93f699a7d6a751cbcf5d263857450679b4d6f8bd Mon Sep 17 00:00:00 2001
From: Dan Goodliffe <dan@randomdan.homeip.net>
Date: Fri, 11 Aug 2017 21:05:17 +0100
Subject: Fix FileHandle move constructor and bolster test coverage

---
 libadhocutil/fileUtils.cpp               | 10 +++++++-
 libadhocutil/fileUtils.h                 |  2 +-
 libadhocutil/unittests/testFileUtils.cpp | 44 +++++++++++++++++++++++++++++++-
 3 files changed, 53 insertions(+), 3 deletions(-)

diff --git a/libadhocutil/fileUtils.cpp b/libadhocutil/fileUtils.cpp
index 5273836..6cfce68 100644
--- a/libadhocutil/fileUtils.cpp
+++ b/libadhocutil/fileUtils.cpp
@@ -20,6 +20,12 @@ namespace AdHoc {
 		{
 		}
 
+		FileHandle::FileHandle(FileHandle && o) :
+			fh(o.fh)
+		{
+			const_cast<int &>(o.fh) = -1;
+		}
+
 		FileHandle::FileHandle(const boost::filesystem::path & path, int flags) :
 			fh(open(path.c_str(), flags))
 		{
@@ -38,7 +44,9 @@ namespace AdHoc {
 
 		FileHandle::~FileHandle()
 		{
-			close(fh);
+			if (fh >= 0) {
+				BOOST_VERIFY(close(fh) == 0);
+			}
 		}
 
 		FileHandle::operator int() const
diff --git a/libadhocutil/fileUtils.h b/libadhocutil/fileUtils.h
index 708652f..4224643 100644
--- a/libadhocutil/fileUtils.h
+++ b/libadhocutil/fileUtils.h
@@ -24,7 +24,7 @@ namespace AdHoc {
 				/**
 				 * Move constructor.
 				 */
-				FileHandle(FileHandle &&) = default;
+				FileHandle(FileHandle &&);
 
 				/**
 				 * Construct from an existing file descriptor.
diff --git a/libadhocutil/unittests/testFileUtils.cpp b/libadhocutil/unittests/testFileUtils.cpp
index 648281c..0729c1f 100644
--- a/libadhocutil/unittests/testFileUtils.cpp
+++ b/libadhocutil/unittests/testFileUtils.cpp
@@ -5,6 +5,12 @@
 #include <definedDirs.h>
 #include <sys.h>
 
+#define REQUIRE_INVALID_FH(fh) \
+	BOOST_REQUIRE_EQUAL(fcntl(fh, F_GETFD), -1)
+
+#define REQUIRE_VALID_FH(fh) \
+	BOOST_REQUIRE(fcntl(fh, F_GETFD) != -1)
+
 template <typename T>
 void testRaw()
 {
@@ -32,14 +38,42 @@ template <typename T, typename ... P>
 T moveTest(P ... p)
 {
 	T fh = openfh<T>(p...);
+	REQUIRE_VALID_FH(fh);
 	char out;
 	BOOST_REQUIRE_EQUAL(1, read(fh, &out, 1));
 	return fh;
 }
 
+class X {
+	public:
+		X(AdHoc::FileUtils::FileHandle h) : fh(std::move(h)) { }
+		AdHoc::FileUtils::FileHandle fh;
+};
+
+class Y : public X {
+	public:
+		Y(AdHoc::FileUtils::FileHandle h) : X(std::move(h)) { }
+};
+
+BOOST_AUTO_TEST_CASE( movePassThrough )
+{
+	auto f = openfh<AdHoc::FileUtils::FileHandle>();
+	int ffd = f.fh;
+	REQUIRE_VALID_FH(f.fh);
+	auto y = std::make_unique<Y>(std::move(f));
+	BOOST_REQUIRE_EQUAL(y->fh, ffd);
+	REQUIRE_VALID_FH(y->fh);
+	BOOST_REQUIRE_EQUAL(f.fh, -1);
+	REQUIRE_INVALID_FH(f.fh);
+	y.reset();
+	REQUIRE_INVALID_FH(ffd);
+}
+
+
 BOOST_AUTO_TEST_CASE( moveFileHandle )
 {
-	moveTest<AdHoc::FileUtils::FileHandle>();
+	auto f = moveTest<AdHoc::FileUtils::FileHandle>();
+	REQUIRE_VALID_FH(f.fh);
 	moveTest<AdHoc::FileUtils::FileHandle>(O_RDONLY);
 	moveTest<AdHoc::FileUtils::FileHandle>(O_RDONLY, O_NONBLOCK);
 }
@@ -47,6 +81,7 @@ BOOST_AUTO_TEST_CASE( moveFileHandle )
 BOOST_AUTO_TEST_CASE( moveFileHandleStat )
 {
 	auto f = moveTest<AdHoc::FileUtils::FileHandleStat>();
+	REQUIRE_VALID_FH(f.fh);
 	BOOST_REQUIRE_EQUAL(0100644, f.getStat().st_mode);
 	moveTest<AdHoc::FileUtils::FileHandleStat>(O_RDONLY);
 	moveTest<AdHoc::FileUtils::FileHandleStat>(O_RDONLY, O_NONBLOCK);
@@ -55,6 +90,7 @@ BOOST_AUTO_TEST_CASE( moveFileHandleStat )
 BOOST_AUTO_TEST_CASE( moveMemMap )
 {
 	auto f = moveTest<AdHoc::FileUtils::MemMap>();
+	REQUIRE_VALID_FH(f.fh);
 	BOOST_REQUIRE_EQUAL(0100644, f.getStat().st_mode);
 	BOOST_REQUIRE_EQUAL(0, memcmp(f.data, "#define BOOST_TEST_MODULE FileUtils", 35));
 	moveTest<AdHoc::FileUtils::MemMap>(O_RDONLY);
@@ -91,6 +127,12 @@ BOOST_AUTO_TEST_CASE( mapfail )
 	BOOST_REQUIRE_THROW({
 		AdHoc::FileUtils::MemMap f("/dev/null");
 	}, AdHoc::SystemException);
+	auto fd = open("/dev/null", O_RDWR);
+	REQUIRE_VALID_FH(fd);
+	BOOST_REQUIRE_THROW({
+		AdHoc::FileUtils::MemMap f(fd, O_RDWR);
+	}, AdHoc::SystemException);
+	REQUIRE_INVALID_FH(fd);
 }
 
 BOOST_AUTO_TEST_CASE( msg )
-- 
cgit v1.2.3