diff --git a/libc/src/__support/File/file.h b/libc/src/__support/File/file.h --- a/libc/src/__support/File/file.h +++ b/libc/src/__support/File/file.h @@ -9,6 +9,7 @@ #ifndef LLVM_LIBC_SRC_SUPPORT_OSUTIL_FILE_H #define LLVM_LIBC_SRC_SUPPORT_OSUTIL_FILE_H +#include "src/__support/CPP/new.h" #include "src/__support/error_or.h" #include "src/__support/threads/mutex.h" @@ -46,6 +47,16 @@ using SeekFunc = ErrorOr(File *, long, int); using CloseFunc = int(File *); using FlushFunc = int(File *); + // CleanupFunc is a function which does the equivalent of this: + // + // void my_file_cleanup(File *f) { + // MyFile *file = reinterpret_cast(f); + // delete file; + // } + // + // Essentially, it a function which calls the delete operator on the + // platform file object to cleanup resources held by it. + using CleanupFunc = void(File *); using ModeFlags = uint32_t; @@ -83,6 +94,7 @@ SeekFunc *platform_seek; CloseFunc *platform_close; FlushFunc *platform_flush; + CleanupFunc *platform_cleanup; Mutex mutex; @@ -130,6 +142,26 @@ FileLock(FileLock &&) = delete; }; + // This is private function and is not to be called by the users of + // File and its derived classes. The correct way to close a file is + // to call the File::cleanup function. + int close() { + { + FileLock lock(this); + if (prev_op == FileOp::WRITE && pos > 0) { + auto buf_result = platform_write(this, buf, pos); + if (buf_result.has_error() || buf_result.value < pos) { + err = true; + return buf_result.error; + } + } + int result = platform_close(this); + if (result != 0) + return result; + } + return 0; + } + protected: constexpr bool write_allowed() const { return mode & (static_cast(OpenMode::WRITE) | @@ -142,6 +174,11 @@ static_cast(OpenMode::PLUS)); } + ~File() { + if (own_buf) + delete buf; + } + public: // We want this constructor to be constexpr so that global file objects // like stdout do not require invocation of the constructor which can @@ -151,16 +188,29 @@ // is zero. This way, we will not have to employ the semantics of // the set_buffer method and allocate a buffer. constexpr File(WriteFunc *wf, ReadFunc *rf, SeekFunc *sf, CloseFunc *cf, - FlushFunc *ff, uint8_t *buffer, size_t buffer_size, - int buffer_mode, bool owned, ModeFlags modeflags) + FlushFunc *ff, CleanupFunc *clf, uint8_t *buffer, + size_t buffer_size, int buffer_mode, bool owned, + ModeFlags modeflags) : platform_write(wf), platform_read(rf), platform_seek(sf), - platform_close(cf), platform_flush(ff), mutex(false, false, false), - ungetc_buf(0), buf(buffer), bufsize(buffer_size), bufmode(buffer_mode), - own_buf(owned), mode(modeflags), pos(0), prev_op(FileOp::NONE), - read_limit(0), eof(false), err(false) { + platform_close(cf), platform_flush(ff), platform_cleanup(clf), + mutex(false, false, false), ungetc_buf(0), buf(buffer), + bufsize(buffer_size), bufmode(buffer_mode), own_buf(owned), + mode(modeflags), pos(0), prev_op(FileOp::NONE), read_limit(0), + eof(false), err(false) { adjust_buf(); } + // Close |f| and cleanup resources held by it. + // Returns the non-zero error value if an error occurs when closing the + // file. + static constexpr int cleanup(File *f) { + int close_result = f->close(); + if (close_result != 0) + return close_result; + f->platform_cleanup(f); + return 0; + } + // Buffered write of |len| bytes from |data| without the file lock. FileIOResult write_unlocked(const void *data, size_t len); @@ -214,9 +264,6 @@ // is ENOMEM. int set_buffer(void *buffer, size_t size, int buffer_mode); - // Closes the file stream and frees up all resources owned by it. - int close(); - void lock() { mutex.lock(); } void unlock() { mutex.unlock(); } @@ -273,6 +320,15 @@ } }; +// Platform specific file implementations can simply pass a pointer to a +// a specialization of this function as the CleanupFunc argument to the +// File constructor. The template type argument FileType should replaced +// with the type of the platform specific file implementation. +template void cleanup_file(File *f) { + auto *file = reinterpret_cast(f); + delete file; +} + // The implementaiton of this function is provided by the platfrom_file // library. ErrorOr openfile(const char *path, const char *mode); diff --git a/libc/src/__support/File/file.cpp b/libc/src/__support/File/file.cpp --- a/libc/src/__support/File/file.cpp +++ b/libc/src/__support/File/file.cpp @@ -338,26 +338,6 @@ return 0; } -int File::close() { - { - FileLock lock(this); - if (prev_op == FileOp::WRITE && pos > 0) { - auto buf_result = platform_write(this, buf, pos); - if (buf_result.has_error() || buf_result.value < pos) { - err = true; - return buf_result.error; - } - } - int result = platform_close(this); - if (result != 0) - return result; - if (own_buf) - free(buf); - } - free(this); - return 0; -} - int File::set_buffer(void *buffer, size_t size, int buffer_mode) { // We do not need to lock the file as this method should be called before // other operations are performed on the file. diff --git a/libc/src/__support/File/linux_file.cpp b/libc/src/__support/File/linux_file.cpp --- a/libc/src/__support/File/linux_file.cpp +++ b/libc/src/__support/File/linux_file.cpp @@ -35,7 +35,8 @@ constexpr LinuxFile(int file_descriptor, uint8_t *buffer, size_t buffer_size, int buffer_mode, bool owned, File::ModeFlags modeflags) : File(&write_func, &read_func, &seek_func, &close_func, flush_func, - buffer, buffer_size, buffer_mode, owned, modeflags), + &cleanup_file, buffer, buffer_size, buffer_mode, owned, + modeflags), fd(file_descriptor) {} int get_fd() const { return fd; } diff --git a/libc/src/stdio/fclose.cpp b/libc/src/stdio/fclose.cpp --- a/libc/src/stdio/fclose.cpp +++ b/libc/src/stdio/fclose.cpp @@ -15,7 +15,8 @@ namespace __llvm_libc { LLVM_LIBC_FUNCTION(int, fclose, (::FILE * stream)) { - int result = reinterpret_cast<__llvm_libc::File *>(stream)->close(); + auto *file = reinterpret_cast<__llvm_libc::File *>(stream); + int result = File::cleanup(file); if (result != 0) { errno = result; return EOF; diff --git a/libc/src/stdio/fopencookie.cpp b/libc/src/stdio/fopencookie.cpp --- a/libc/src/stdio/fopencookie.cpp +++ b/libc/src/stdio/fopencookie.cpp @@ -32,8 +32,9 @@ CookieFile(void *c, cookie_io_functions_t cops, uint8_t *buffer, size_t bufsize, File::ModeFlags mode) : File(&cookie_write, &cookie_read, &CookieFile::cookie_seek, - &cookie_close, &cookie_flush, buffer, bufsize, - 0 /* default buffering mode */, true /* File owns buffer */, mode), + &cookie_close, &cookie_flush, &cleanup_file, buffer, + bufsize, 0 /* default buffering mode */, + true /* File owns buffer */, mode), cookie(c), ops(cops) {} }; diff --git a/libc/test/src/__support/File/file_test.cpp b/libc/test/src/__support/File/file_test.cpp --- a/libc/test/src/__support/File/file_test.cpp +++ b/libc/test/src/__support/File/file_test.cpp @@ -18,9 +18,10 @@ using ModeFlags = __llvm_libc::File::ModeFlags; using MemoryView = __llvm_libc::memory::testing::MemoryView; using __llvm_libc::ErrorOr; +using __llvm_libc::File; using __llvm_libc::FileIOResult; -class StringFile : public __llvm_libc::File { +class StringFile : public File { static constexpr size_t SIZE = 512; size_t pos; char str[SIZE] = {0}; @@ -38,8 +39,9 @@ explicit StringFile(char *buffer, size_t buflen, int bufmode, bool owned, ModeFlags modeflags) : __llvm_libc::File(&str_write, &str_read, &str_seek, &str_close, - &str_flush, reinterpret_cast(buffer), - buflen, bufmode, owned, modeflags), + &str_flush, &__llvm_libc::cleanup_file, + reinterpret_cast(buffer), buflen, bufmode, + owned, modeflags), pos(0), eof_marker(0), write_append(false) { if (modeflags & static_cast(__llvm_libc::File::OpenMode::APPEND)) write_append = true; @@ -144,7 +146,7 @@ EXPECT_TRUE(result.has_error()); } - ASSERT_EQ(f->close(), 0); + ASSERT_EQ(File::cleanup(f), 0); } TEST(LlvmLibcFileTest, WriteLineBuffered) { @@ -203,8 +205,8 @@ EXPECT_MEM_EQ(src3, dst_line_final); EXPECT_MEM_EQ(src3, dst_full_final); - ASSERT_EQ(f_line->close(), 0); - ASSERT_EQ(f_full->close(), 0); + ASSERT_EQ(File::cleanup(f_line), 0); + ASSERT_EQ(File::cleanup(f_full), 0); } TEST(LlvmLibcFileTest, WriteUnbuffered) { @@ -219,7 +221,7 @@ sizeof(data)); // no buffering means this is written immediately. EXPECT_STREQ(f->get_str(), data); - ASSERT_EQ(f->close(), 0); + ASSERT_EQ(File::cleanup(f), 0); } TEST(LlvmLibcFileTest, ReadOnly) { @@ -272,7 +274,7 @@ EXPECT_TRUE(result.has_error()); } - ASSERT_EQ(f->close(), 0); + ASSERT_EQ(File::cleanup(f), 0); } TEST(LlvmLibcFileTest, ReadSeekCurAndRead) { @@ -294,7 +296,7 @@ ASSERT_EQ(f->seek(-5, SEEK_CUR).value(), 0); ASSERT_EQ(f->read(data, READ_SIZE - 1).value, READ_SIZE - 1); ASSERT_STREQ(data, "9098"); - ASSERT_EQ(f->close(), 0); + ASSERT_EQ(File::cleanup(f), 0); } TEST(LlvmLibcFileTest, AppendOnly) { @@ -324,7 +326,7 @@ EXPECT_EQ(f->flush(), int(0)); EXPECT_EQ(f->get_pos(), sizeof(write_data) + sizeof(initial_content)); - ASSERT_EQ(f->close(), 0); + ASSERT_EQ(File::cleanup(f), 0); } TEST(LlvmLibcFileTest, WriteUpdate) { @@ -344,7 +346,7 @@ ASSERT_EQ(f->read(read_data, sizeof(data)).value, sizeof(data)); EXPECT_STREQ(read_data, data); - ASSERT_EQ(f->close(), 0); + ASSERT_EQ(File::cleanup(f), 0); } TEST(LlvmLibcFileTest, ReadUpdate) { @@ -377,7 +379,7 @@ src2(write_data, sizeof(write_data)); EXPECT_MEM_EQ(src2, dst2); - ASSERT_EQ(f->close(), 0); + ASSERT_EQ(File::cleanup(f), 0); } TEST(LlvmLibcFileTest, AppendUpdate) { @@ -419,7 +421,7 @@ MemoryView src4(initial_content, READ_SIZE), dst4(read_data, READ_SIZE); EXPECT_MEM_EQ(src4, dst4); - ASSERT_EQ(f->close(), 0); + ASSERT_EQ(File::cleanup(f), 0); } TEST(LlvmLibcFileTest, SmallBuffer) { @@ -436,7 +438,7 @@ EXPECT_EQ(f->get_pos(), sizeof(WRITE_DATA)); ASSERT_STREQ(f->get_str(), WRITE_DATA); - ASSERT_EQ(f->close(), 0); + ASSERT_EQ(File::cleanup(f), 0); } TEST(LlvmLibcFileTest, ZeroLengthBuffer) { @@ -458,9 +460,9 @@ ASSERT_STREQ(f_lbf->get_str(), WRITE_DATA); ASSERT_STREQ(f_nbf->get_str(), WRITE_DATA); - ASSERT_EQ(f_fbf->close(), 0); - ASSERT_EQ(f_lbf->close(), 0); - ASSERT_EQ(f_nbf->close(), 0); + ASSERT_EQ(File::cleanup(f_fbf), 0); + ASSERT_EQ(File::cleanup(f_lbf), 0); + ASSERT_EQ(File::cleanup(f_nbf), 0); } TEST(LlvmLibcFileTest, WriteNothing) { @@ -485,7 +487,7 @@ ASSERT_FALSE(f_lbf->error_unlocked()); ASSERT_FALSE(f_nbf->error_unlocked()); - ASSERT_EQ(f_fbf->close(), 0); - ASSERT_EQ(f_lbf->close(), 0); - ASSERT_EQ(f_nbf->close(), 0); + ASSERT_EQ(File::cleanup(f_fbf), 0); + ASSERT_EQ(File::cleanup(f_lbf), 0); + ASSERT_EQ(File::cleanup(f_nbf), 0); } diff --git a/libc/test/src/__support/File/platform_file_test.cpp b/libc/test/src/__support/File/platform_file_test.cpp --- a/libc/test/src/__support/File/platform_file_test.cpp +++ b/libc/test/src/__support/File/platform_file_test.cpp @@ -20,7 +20,7 @@ File *file = __llvm_libc::openfile(FILENAME, "w"); ASSERT_FALSE(file == nullptr); ASSERT_EQ(file->write(TEXT, TEXT_SIZE).value, TEXT_SIZE); - ASSERT_EQ(file->close(), 0); + ASSERT_EQ(File::cleanup(file), 0); file = __llvm_libc::openfile(FILENAME, "r"); ASSERT_FALSE(file == nullptr); @@ -33,7 +33,7 @@ ASSERT_EQ(file->read(data, TEXT_SIZE).value, size_t(0)); ASSERT_TRUE(file->iseof()); - ASSERT_EQ(file->close(), 0); + ASSERT_EQ(File::cleanup(file), 0); } TEST(LlvmLibcPlatformFileTest, CreateWriteSeekAndReadBack) { @@ -53,7 +53,7 @@ ASSERT_EQ(file->read(data, TEXT_SIZE).value, size_t(0)); ASSERT_TRUE(file->iseof()); - ASSERT_EQ(file->close(), 0); + ASSERT_EQ(File::cleanup(file), 0); } TEST(LlvmLibcPlatformFileTest, CreateAppendCloseAndReadBack) { @@ -61,14 +61,14 @@ File *file = __llvm_libc::openfile(FILENAME, "w"); ASSERT_FALSE(file == nullptr); ASSERT_EQ(file->write(TEXT, TEXT_SIZE).value, TEXT_SIZE); - ASSERT_EQ(file->close(), 0); + ASSERT_EQ(File::cleanup(file), 0); file = __llvm_libc::openfile(FILENAME, "a"); ASSERT_FALSE(file == nullptr); constexpr char APPEND_TEXT[] = " Append Text"; constexpr size_t APPEND_TEXT_SIZE = sizeof(APPEND_TEXT) - 1; ASSERT_EQ(file->write(APPEND_TEXT, APPEND_TEXT_SIZE).value, APPEND_TEXT_SIZE); - ASSERT_EQ(file->close(), 0); + ASSERT_EQ(File::cleanup(file), 0); file = __llvm_libc::openfile(FILENAME, "r"); ASSERT_FALSE(file == nullptr); @@ -82,7 +82,7 @@ ASSERT_EQ(file->read(data, READ_SIZE).value, size_t(0)); ASSERT_TRUE(file->iseof()); - ASSERT_EQ(file->close(), 0); + ASSERT_EQ(File::cleanup(file), 0); } TEST(LlvmLibcPlatformFileTest, CreateAppendSeekAndReadBack) { @@ -90,7 +90,7 @@ File *file = __llvm_libc::openfile(FILENAME, "w"); ASSERT_FALSE(file == nullptr); ASSERT_EQ(file->write(TEXT, TEXT_SIZE).value, TEXT_SIZE); - ASSERT_EQ(file->close(), 0); + ASSERT_EQ(File::cleanup(file), 0); file = __llvm_libc::openfile(FILENAME, "a+"); ASSERT_FALSE(file == nullptr); @@ -108,7 +108,7 @@ ASSERT_EQ(file->read(data, APPEND_TEXT_SIZE).value, size_t(0)); ASSERT_TRUE(file->iseof()); - ASSERT_EQ(file->close(), 0); + ASSERT_EQ(File::cleanup(file), 0); } TEST(LlvmLibcPlatformFileTest, LargeFile) { @@ -126,7 +126,7 @@ for (int i = 0; i < REPEAT; ++i) { ASSERT_EQ(file->write(write_data, DATA_SIZE).value, DATA_SIZE); } - ASSERT_EQ(file->close(), 0); + ASSERT_EQ(File::cleanup(file), 0); file = __llvm_libc::openfile(FILENAME, "r"); ASSERT_FALSE(file == nullptr); @@ -141,7 +141,7 @@ ASSERT_EQ(file->read(data, 1).value, size_t(0)); ASSERT_TRUE(file->iseof()); - ASSERT_EQ(file->close(), 0); + ASSERT_EQ(File::cleanup(file), 0); } TEST(LlvmLibcPlatformFileTest, ReadSeekCurAndRead) { @@ -151,7 +151,7 @@ constexpr char CONTENT[] = "1234567890987654321"; ASSERT_EQ(sizeof(CONTENT) - 1, file->write(CONTENT, sizeof(CONTENT) - 1).value); - ASSERT_EQ(0, file->close()); + ASSERT_EQ(0, File::cleanup(file)); file = __llvm_libc::openfile(FILENAME, "r"); ASSERT_FALSE(file == nullptr); @@ -168,7 +168,7 @@ ASSERT_EQ(file->read(data, READ_SIZE - 1).value, READ_SIZE - 1); ASSERT_STREQ(data, "9098"); - ASSERT_EQ(file->close(), 0); + ASSERT_EQ(File::cleanup(file), 0); } TEST(LlvmLibcPlatformFileTest, IncorrectOperation) { @@ -180,21 +180,21 @@ ASSERT_EQ(file->read(data, 1).value, size_t(0)); // Cannot read ASSERT_FALSE(file->iseof()); ASSERT_TRUE(file->error()); - ASSERT_EQ(file->close(), 0); + ASSERT_EQ(File::cleanup(file), 0); file = __llvm_libc::openfile(FILENAME, "r"); ASSERT_FALSE(file == nullptr); ASSERT_EQ(file->write(data, 1).value, size_t(0)); // Cannot write ASSERT_FALSE(file->iseof()); ASSERT_TRUE(file->error()); - ASSERT_EQ(file->close(), 0); + ASSERT_EQ(File::cleanup(file), 0); file = __llvm_libc::openfile(FILENAME, "a"); ASSERT_FALSE(file == nullptr); ASSERT_EQ(file->read(data, 1).value, size_t(0)); // Cannot read ASSERT_FALSE(file->iseof()); ASSERT_TRUE(file->error()); - ASSERT_EQ(file->close(), 0); + ASSERT_EQ(File::cleanup(file), 0); } TEST(LlvmLibcPlatformFileTest, StdOutStdErrSmokeTest) {