From 92ee52cc38ca1a06b7b0851643314b0830f0ece9 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Fri, 25 Jan 2019 16:22:41 -0800 Subject: [PATCH 1/2] base: don't overwrite errno in unique_fd::~unique_fd. unique_fd's destructor potentially mangling errno makes it difficult to use correctly in code that sets errno (or, in reality, it makes it so that errno values get randomly stomped upon if close actually sets errno, because no one accounts for this case). Preserve errno ourselves to avoid this. Test: treehugger Change-Id: Ib06e6f65866d86fff4032b2311021eaf9226a1af --- base/include/android-base/unique_fd.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/base/include/android-base/unique_fd.h b/base/include/android-base/unique_fd.h index 2c890b42d..83213e9c4 100644 --- a/base/include/android-base/unique_fd.h +++ b/base/include/android-base/unique_fd.h @@ -17,10 +17,10 @@ #pragma once #include +#include #include #if !defined(_WIN32) -#include #include #endif @@ -114,6 +114,8 @@ class unique_fd_impl final { private: void reset(int new_value, void* previous_tag) { + int previous_errno = errno; + if (fd_ != -1) { close(fd_, this); } @@ -122,6 +124,8 @@ class unique_fd_impl final { if (new_value != -1) { tag(new_value, previous_tag, this); } + + errno = previous_errno; } int fd_ = -1; From c162c713efe09d9772926e6658aa67e2656cdec3 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Fri, 25 Jan 2019 15:30:25 -0800 Subject: [PATCH 2/2] adb: add fdevent callback that passes the fdevent. This is useful for when we don't want to actually store the fdevent into a separate struct to be able to destroy it, but instead want to destroy it immediately from the callback. Test: adb_test Change-Id: Ief3dbf8ea6a6bd72dc7e73f9ab9b7429e48fc181 --- adb/fdevent.cpp | 35 +++++++++--- adb/fdevent.h | 17 +++--- adb/fdevent_test.cpp | 125 +++++++++++++++++++++++++++---------------- 3 files changed, 114 insertions(+), 63 deletions(-) diff --git a/adb/fdevent.cpp b/adb/fdevent.cpp index e09656033..fa3738d16 100644 --- a/adb/fdevent.cpp +++ b/adb/fdevent.cpp @@ -33,6 +33,8 @@ #include #include #include +#include +#include #include #include @@ -121,13 +123,8 @@ static std::string dump_fde(const fdevent* fde) { state.c_str()); } -void fdevent_install(fdevent* fde, int fd, fd_func func, void* arg) { - check_main_thread(); - CHECK_GE(fd, 0); - memset(fde, 0, sizeof(fdevent)); -} - -fdevent* fdevent_create(int fd, fd_func func, void* arg) { +template +static fdevent* fdevent_create_impl(int fd, F func, void* arg) { check_main_thread(); CHECK_GE(fd, 0); @@ -150,6 +147,14 @@ fdevent* fdevent_create(int fd, fd_func func, void* arg) { return fde; } +fdevent* fdevent_create(int fd, fd_func func, void* arg) { + return fdevent_create_impl(fd, func, arg); +} + +fdevent* fdevent_create(int fd, fd_func2 func, void* arg) { + return fdevent_create_impl(fd, func, arg); +} + unique_fd fdevent_release(fdevent* fde) { check_main_thread(); if (!fde) { @@ -290,13 +295,27 @@ static void fdevent_process() { } } +template +struct always_false : std::false_type {}; + static void fdevent_call_fdfunc(fdevent* fde) { unsigned events = fde->events; fde->events = 0; CHECK(fde->state & FDE_PENDING); fde->state &= (~FDE_PENDING); D("fdevent_call_fdfunc %s", dump_fde(fde).c_str()); - fde->func(fde->fd.get(), events, fde->arg); + std::visit( + [&](auto&& f) { + using F = std::decay_t; + if constexpr (std::is_same_v) { + f(fde->fd.get(), events, fde->arg); + } else if constexpr (std::is_same_v) { + f(fde, events, fde->arg); + } else { + static_assert(always_false::value, "non-exhaustive visitor"); + } + }, + fde->func); } static void fdevent_run_flush() EXCLUDES(run_queue_mutex) { diff --git a/adb/fdevent.h b/adb/fdevent.h index df2339a97..70e0a96e0 100644 --- a/adb/fdevent.h +++ b/adb/fdevent.h @@ -21,6 +21,7 @@ #include /* for int64_t */ #include +#include #include "adb_unique_fd.h" @@ -30,6 +31,7 @@ #define FDE_ERROR 0x0004 typedef void (*fd_func)(int fd, unsigned events, void *userdata); +typedef void (*fd_func2)(struct fdevent* fde, unsigned events, void* userdata); struct fdevent { uint64_t id; @@ -40,15 +42,14 @@ struct fdevent { uint16_t state = 0; uint16_t events = 0; - fd_func func = nullptr; + std::variant func; void* arg = nullptr; }; -/* Allocate and initialize a new fdevent object - * Note: use FD_TIMER as 'fd' to create a fd-less object - * (used to implement timers). -*/ +// Allocate and initialize a new fdevent object +// TODO: Switch these to unique_fd. fdevent *fdevent_create(int fd, fd_func func, void *arg); +fdevent* fdevent_create(int fd, fd_func2 func, void* arg); // Deallocate an fdevent object that was created by fdevent_create. void fdevent_destroy(fdevent *fde); @@ -56,16 +57,14 @@ void fdevent_destroy(fdevent *fde); // fdevent_destroy, except releasing the file descriptor previously owned by the fdevent. unique_fd fdevent_release(fdevent* fde); -/* Change which events should cause notifications -*/ +// Change which events should cause notifications void fdevent_set(fdevent *fde, unsigned events); void fdevent_add(fdevent *fde, unsigned events); void fdevent_del(fdevent *fde, unsigned events); void fdevent_set_timeout(fdevent *fde, int64_t timeout_ms); -/* loop forever, handling events. -*/ +// Loop forever, handling events. void fdevent_loop(); void check_main_thread(); diff --git a/adb/fdevent_test.cpp b/adb/fdevent_test.cpp index 816134f8f..a9746bbc8 100644 --- a/adb/fdevent_test.cpp +++ b/adb/fdevent_test.cpp @@ -30,10 +30,16 @@ class FdHandler { public: - FdHandler(int read_fd, int write_fd) : read_fd_(read_fd), write_fd_(write_fd) { - read_fde_ = fdevent_create(read_fd_, FdEventCallback, this); + FdHandler(int read_fd, int write_fd, bool use_new_callback) + : read_fd_(read_fd), write_fd_(write_fd) { + if (use_new_callback) { + read_fde_ = fdevent_create(read_fd_, FdEventNewCallback, this); + write_fde_ = fdevent_create(write_fd_, FdEventNewCallback, this); + } else { + read_fde_ = fdevent_create(read_fd_, FdEventCallback, this); + write_fde_ = fdevent_create(write_fd_, FdEventCallback, this); + } fdevent_add(read_fde_, FDE_READ); - write_fde_ = fdevent_create(write_fd_, FdEventCallback, this); } ~FdHandler() { @@ -64,6 +70,29 @@ class FdHandler { } } + static void FdEventNewCallback(fdevent* fde, unsigned events, void* userdata) { + int fd = fde->fd.get(); + FdHandler* handler = reinterpret_cast(userdata); + ASSERT_EQ(0u, (events & ~(FDE_READ | FDE_WRITE))) << "unexpected events: " << events; + if (events & FDE_READ) { + ASSERT_EQ(fd, handler->read_fd_); + char c; + ASSERT_EQ(1, adb_read(fd, &c, 1)); + handler->queue_.push(c); + fdevent_add(handler->write_fde_, FDE_WRITE); + } + if (events & FDE_WRITE) { + ASSERT_EQ(fd, handler->write_fd_); + ASSERT_FALSE(handler->queue_.empty()); + char c = handler->queue_.front(); + handler->queue_.pop(); + ASSERT_EQ(1, adb_write(fd, &c, 1)); + if (handler->queue_.empty()) { + fdevent_del(handler->write_fde_, FDE_WRITE); + } + } + } + private: const int read_fd_; const int write_fd_; @@ -84,56 +113,60 @@ TEST_F(FdeventTest, fdevent_terminate) { } TEST_F(FdeventTest, smoke) { - const size_t PIPE_COUNT = 10; - const size_t MESSAGE_LOOP_COUNT = 100; - const std::string MESSAGE = "fdevent_test"; - int fd_pair1[2]; - int fd_pair2[2]; - ASSERT_EQ(0, adb_socketpair(fd_pair1)); - ASSERT_EQ(0, adb_socketpair(fd_pair2)); - ThreadArg thread_arg; - thread_arg.first_read_fd = fd_pair1[0]; - thread_arg.last_write_fd = fd_pair2[1]; - thread_arg.middle_pipe_count = PIPE_COUNT; - int writer = fd_pair1[1]; - int reader = fd_pair2[0]; + for (bool use_new_callback : {true, false}) { + fdevent_reset(); + const size_t PIPE_COUNT = 10; + const size_t MESSAGE_LOOP_COUNT = 100; + const std::string MESSAGE = "fdevent_test"; + int fd_pair1[2]; + int fd_pair2[2]; + ASSERT_EQ(0, adb_socketpair(fd_pair1)); + ASSERT_EQ(0, adb_socketpair(fd_pair2)); + ThreadArg thread_arg; + thread_arg.first_read_fd = fd_pair1[0]; + thread_arg.last_write_fd = fd_pair2[1]; + thread_arg.middle_pipe_count = PIPE_COUNT; + int writer = fd_pair1[1]; + int reader = fd_pair2[0]; - PrepareThread(); + PrepareThread(); - std::vector> fd_handlers; - fdevent_run_on_main_thread([&thread_arg, &fd_handlers]() { - std::vector read_fds; - std::vector write_fds; + std::vector> fd_handlers; + fdevent_run_on_main_thread([&thread_arg, &fd_handlers, use_new_callback]() { + std::vector read_fds; + std::vector write_fds; - read_fds.push_back(thread_arg.first_read_fd); - for (size_t i = 0; i < thread_arg.middle_pipe_count; ++i) { - int fds[2]; - ASSERT_EQ(0, adb_socketpair(fds)); - read_fds.push_back(fds[0]); - write_fds.push_back(fds[1]); + read_fds.push_back(thread_arg.first_read_fd); + for (size_t i = 0; i < thread_arg.middle_pipe_count; ++i) { + int fds[2]; + ASSERT_EQ(0, adb_socketpair(fds)); + read_fds.push_back(fds[0]); + write_fds.push_back(fds[1]); + } + write_fds.push_back(thread_arg.last_write_fd); + + for (size_t i = 0; i < read_fds.size(); ++i) { + fd_handlers.push_back( + std::make_unique(read_fds[i], write_fds[i], use_new_callback)); + } + }); + WaitForFdeventLoop(); + + for (size_t i = 0; i < MESSAGE_LOOP_COUNT; ++i) { + std::string read_buffer = MESSAGE; + std::string write_buffer(MESSAGE.size(), 'a'); + ASSERT_TRUE(WriteFdExactly(writer, read_buffer.c_str(), read_buffer.size())); + ASSERT_TRUE(ReadFdExactly(reader, &write_buffer[0], write_buffer.size())); + ASSERT_EQ(read_buffer, write_buffer); } - write_fds.push_back(thread_arg.last_write_fd); - for (size_t i = 0; i < read_fds.size(); ++i) { - fd_handlers.push_back(std::make_unique(read_fds[i], write_fds[i])); - } - }); - WaitForFdeventLoop(); + fdevent_run_on_main_thread([&fd_handlers]() { fd_handlers.clear(); }); + WaitForFdeventLoop(); - for (size_t i = 0; i < MESSAGE_LOOP_COUNT; ++i) { - std::string read_buffer = MESSAGE; - std::string write_buffer(MESSAGE.size(), 'a'); - ASSERT_TRUE(WriteFdExactly(writer, read_buffer.c_str(), read_buffer.size())); - ASSERT_TRUE(ReadFdExactly(reader, &write_buffer[0], write_buffer.size())); - ASSERT_EQ(read_buffer, write_buffer); + TerminateThread(); + ASSERT_EQ(0, adb_close(writer)); + ASSERT_EQ(0, adb_close(reader)); } - - fdevent_run_on_main_thread([&fd_handlers]() { fd_handlers.clear(); }); - WaitForFdeventLoop(); - - TerminateThread(); - ASSERT_EQ(0, adb_close(writer)); - ASSERT_EQ(0, adb_close(reader)); } struct InvalidFdArg {