[block-cache] fix potential file descriptor leak

Encapsulate file descriptor into an object, to ensure that an fd will be
closed properly while exception raised, e.g., the block_cache throws
exception during the block_manager's construction.

base/file_utils.cc

@@ -39,14 +39,16 @@ namespace {
 
 //----------------------------------------------------------------
 
-int
+file_utils::file_descriptor::file_descriptor(string const &path, int flags) {
-file_utils::open_file(string const &path, int flags) {
+	fd_ = ::open(path.c_str(), OPEN_FLAGS | flags, DEFAULT_MODE);
-	int fd = ::open(path.c_str(), OPEN_FLAGS | flags, DEFAULT_MODE);
+	if (fd_ < 0)
-	if (fd < 0)
 		syscall_failed("open",
 			       "Note: you cannot run this tool with these options on live metadata.");
+}
 
-	return fd;
+file_utils::file_descriptor::~file_descriptor() {
+	close(fd_);
+	fd_ = -1;
 }
 
 bool
@@ -76,7 +78,7 @@ file_utils::check_file_exists(string const &file, bool must_be_regular_file) {
 		throw runtime_error("Not a regular file");
 }
 
-int
+file_utils::file_descriptor
 file_utils::create_block_file(string const &path, off_t file_size) {
 	if (file_exists(path)) {
 		ostringstream out;
@@ -84,16 +86,16 @@ file_utils::create_block_file(string const &path, off_t file_size) {
 		throw runtime_error(out.str());
 	}
 
-	int fd = open_file(path, O_CREAT | O_EXCL | O_RDWR);
+	file_descriptor fd(path, O_CREAT | O_EXCL | O_RDWR);
 
-	int r = ::ftruncate(fd, file_size);
+	int r = ::ftruncate(fd.fd_, file_size);
 	if (r < 0)
 		syscall_failed("ftruncate");
 
 	return fd;
 }
 
-int
+file_utils::file_descriptor
 file_utils::open_block_file(string const &path, off_t min_size, bool writeable, bool excl) {
 	if (!file_exists(path)) {
 		ostringstream out;
@@ -105,7 +107,7 @@ file_utils::open_block_file(string const &path, off_t min_size, bool writeable,
 	if (excl)
 		flags |= O_EXCL;
 
-	return open_file(path, flags);
+	return file_descriptor(path, flags);
 }
 
 uint64_t
@@ -146,17 +148,15 @@ file_utils::zero_superblock(std::string const &path)
 {
 	char *buffer;
 	unsigned const SUPERBLOCK_SIZE = 4096;
-	int fd = open_block_file(path, SUPERBLOCK_SIZE, true, true);
+	file_descriptor fd = open_block_file(path, SUPERBLOCK_SIZE, true, true);
 
 	buffer = reinterpret_cast<char *>(aligned_alloc(SUPERBLOCK_SIZE, SUPERBLOCK_SIZE));
 	if (!buffer)
         	throw runtime_error("out of memory");
 
 	memset(buffer, 0, SUPERBLOCK_SIZE);
-	if (::write(fd, buffer, SUPERBLOCK_SIZE) != SUPERBLOCK_SIZE)
+	if (::write(fd.fd_, buffer, SUPERBLOCK_SIZE) != SUPERBLOCK_SIZE)
 		throw runtime_error("couldn't zero superblock");
-
-	::close(fd);
 }
 
 //----------------------------------------------------------------

base/file_utils.h

@@ -8,11 +8,20 @@
 //----------------------------------------------------------------
 
 namespace file_utils {
-	int open_file(std::string const &path, int flags);
+	struct file_descriptor {
+		// file_descriptor is movable but not copyable
+		file_descriptor(file_descriptor &&) = default;
+		file_descriptor& operator=(file_descriptor &&) = default;
+		file_descriptor(std::string const &path, int flags);
+		virtual ~file_descriptor();
+
+		int fd_;
+	};
+
 	bool file_exists(std::string const &path);
 	void check_file_exists(std::string const &file, bool must_be_regular_file = true);
-	int create_block_file(std::string const &path, off_t file_size);
+	file_descriptor create_block_file(std::string const &path, off_t file_size);
-	int open_block_file(std::string const &path, off_t min_size, bool writeable, bool excl = true);
+	file_descriptor open_block_file(std::string const &path, off_t min_size, bool writeable, bool excl = true);
 	uint64_t get_file_length(std::string const &file);
 	void zero_superblock(std::string const &path);
 }

block-cache/block_cache.cc

@@ -15,6 +15,7 @@
 #include <sstream>
 
 using namespace bcache;
+using namespace file_utils;
 
 //----------------------------------------------------------------
 
@@ -290,7 +291,7 @@ block_cache::setup_control_block(block &b)
 	size_t block_size_bytes = block_size_ << SECTOR_SHIFT;
 
 	memset(cb, 0, sizeof(*cb));
-	cb->aio_fildes = fd_;
+	cb->aio_fildes = fd_.fd_;
 
 	cb->u.c.buf = b.data_;
 	cb->u.c.offset = block_size_bytes * b.index_;
@@ -371,8 +372,9 @@ block_cache::calc_nr_buckets(unsigned nr_blocks)
 	return r;
 }
 
-block_cache::block_cache(int fd, sector_t block_size, uint64_t on_disk_blocks, size_t mem)
+block_cache::block_cache(file_descriptor &fd, sector_t block_size, uint64_t on_disk_blocks, size_t mem)
-	: nr_locked_(0),
+	: fd_(fd),
+	  nr_locked_(0),
 	  nr_dirty_(0),
 	  nr_io_pending_(0),
 	  read_hits_(0),
@@ -386,7 +388,6 @@ block_cache::block_cache(int fd, sector_t block_size, uint64_t on_disk_blocks, s
 	int r;
 	unsigned nr_cache_blocks = calc_nr_cache_blocks(mem, block_size);
 
-	fd_ = fd;
 	block_size_ = block_size;
 	nr_data_blocks_ = on_disk_blocks;
 	nr_cache_blocks_ = nr_cache_blocks;
@@ -418,8 +419,6 @@ block_cache::~block_cache()
 	if (aio_context_)
 		io_destroy(aio_context_);
 
-	::close(fd_);
-
 #if 0
 	std::cerr << "\nblock cache stats\n"
 		  << "=================\n"

block-cache/block_cache.h

@@ -2,6 +2,7 @@
 #define BLOCK_CACHE_H
 
 #include "base/container_of.h"
+#include "base/file_utils.h"
 
 #include <boost/intrusive/list.hpp>
 #include <boost/intrusive/set.hpp>
@@ -185,7 +186,7 @@ namespace bcache {
 
 		//--------------------------------
 
-		block_cache(int fd, sector_t block_size,
+		block_cache(file_utils::file_descriptor &fd, sector_t block_size,
 			    uint64_t max_nr_blocks, size_t mem);
 		~block_cache();
 
@@ -247,7 +248,7 @@ namespace bcache {
 
 		//--------------------------------
 
-		int fd_;
+		file_utils::file_descriptor &fd_;
 		sector_t block_size_;
 		uint64_t nr_data_blocks_;
 		uint64_t nr_cache_blocks_;

persistent-data/block.h

@@ -136,11 +136,12 @@ namespace persistent_data {
 
 	private:
 		uint64_t choose_cache_size(block_address nr_blocks) const;
-		int open_or_create_block_file(std::string const &path, off_t file_size,
+		file_utils::file_descriptor open_or_create_block_file(std::string const &path,
+								      off_t file_size,
 								      mode m, bool excl);
 		void check(block_address b) const;
 
-		int fd_;
+		file_utils::file_descriptor fd_;
 		mutable block_cache bc_;
 		unsigned superblock_ref_count_;
 	};

persistent-data/block.tcc

@@ -154,7 +154,7 @@ namespace persistent_data {
 	}
 
 	template <uint32_t BlockSize>
-	int
+	file_utils::file_descriptor
 	block_manager<BlockSize>::open_or_create_block_file(std::string const &path, off_t file_size, mode m, bool excl)
 	{
 		switch (m) {

thin-provisioning/cache_stream.cc

@@ -18,7 +18,7 @@ cache_stream::cache_stream(string const &path,
 
 	  // hack because cache uses LRU rather than MRU
 	  cache_blocks_((cache_mem / block_size) / 2u),
-	  fd_(file_utils::open_file(path, O_RDONLY | O_EXCL)),
+	  fd_(path, O_RDONLY | O_EXCL),
 	  v_(new bcache::noop_validator()),
 	  cache_(new block_cache(fd_, block_size / 512, nr_blocks_, cache_mem)),
 	  current_index_(0) {

thin-provisioning/cache_stream.h

@@ -38,7 +38,7 @@ namespace thin_provisioning {
 		block_address nr_blocks_;
 		block_address cache_blocks_;
 
-		int fd_;
+		file_utils::file_descriptor fd_;
 		validator::ptr v_;
 		std::unique_ptr<block_cache> cache_;
 

unit-tests/bcache_t.cc

@@ -38,7 +38,7 @@ TEST(BCacheTests, cleaned_on_demand)
 	unsigned const NR_BLOCKS = 16;
 
 	temp_file tmp("bcache_t", 1);
-	int fd = open(tmp.get_path().c_str(), O_RDWR | O_DIRECT, 0666);
+	file_utils::file_descriptor fd(tmp.get_path().c_str(), O_RDWR | O_DIRECT);
 
 	uint64_t bs = 8;
 	block_cache bc(fd, bs, 64, (bs << SECTOR_SHIFT) * NR_BLOCKS);