From 14d77e9cc4a5203665a512a6485d9a416bfd43de Mon Sep 17 00:00:00 2001 From: Simon Fels Date: Fri, 12 May 2017 18:46:25 +0200 Subject: [PATCH 1/4] User proper constants in our adb message processor --- src/anbox/qemu/adb_message_processor.cpp | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/anbox/qemu/adb_message_processor.cpp b/src/anbox/qemu/adb_message_processor.cpp index 9fe9575..350c549 100644 --- a/src/anbox/qemu/adb_message_processor.cpp +++ b/src/anbox/qemu/adb_message_processor.cpp @@ -27,10 +27,14 @@ namespace { const unsigned short default_adb_client_port{5037}; const unsigned short default_host_listen_port{6664}; +constexpr const char *loopback_address{"127.0.0.1"}; const std::string accept_command{"accept"}; const std::string ok_command{"ok"}; const std::string ko_command{"ko"}; const std::string start_command{"start"}; +// This timeount should be too high to not cause a too long wait time for the +// user until we connect to the adb host instance after it appeared and not +// too short to not put unnecessary burden on the CPU. const boost::posix_time::seconds default_adb_wait_time{1}; static std::mutex active_instance; } @@ -102,7 +106,7 @@ void AdbMessageProcessor::advance_state() { void AdbMessageProcessor::wait_for_host_connection() { if (!host_connector_) { host_connector_ = std::make_shared( - boost::asio::ip::address_v4::from_string("127.0.0.1"), + boost::asio::ip::address_v4::from_string(loopback_address), default_host_listen_port, runtime_, std::make_shared< network::DelegateConnectionCreator>( @@ -113,12 +117,9 @@ void AdbMessageProcessor::wait_for_host_connection() { // Notify the adb host instance so that it knows on which port our // proxy is waiting for incoming connections. auto messenger = std::make_shared( - boost::asio::ip::address_v4::from_string("127.0.0.1"), - default_adb_client_port, runtime_); - auto message = - utils::string_format("host:emulator:%d", default_host_listen_port); - auto handshake = - utils::string_format("%04x%s", message.size(), message.c_str()); + boost::asio::ip::address_v4::from_string(loopback_address), default_adb_client_port, runtime_); + auto message = utils::string_format("host:emulator:%d", default_host_listen_port); + auto handshake = utils::string_format("%04x%s", message.size(), message.c_str()); messenger->send(handshake.data(), handshake.size()); } catch (std::exception &) { // Try again later when the host adb service is maybe available From 183c02d2d0f7e5d61be551996ff20b2e2a6f7483 Mon Sep 17 00:00:00 2001 From: Simon Fels Date: Fri, 12 May 2017 18:46:49 +0200 Subject: [PATCH 2/4] Make adb message processor more robust in shutdown scenarios --- src/anbox/qemu/adb_message_processor.cpp | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/src/anbox/qemu/adb_message_processor.cpp b/src/anbox/qemu/adb_message_processor.cpp index 350c549..ef56773 100644 --- a/src/anbox/qemu/adb_message_processor.cpp +++ b/src/anbox/qemu/adb_message_processor.cpp @@ -16,10 +16,10 @@ */ #include "anbox/qemu/adb_message_processor.h" -#include "anbox/logger.h" #include "anbox/network/delegate_connection_creator.h" #include "anbox/network/delegate_message_processor.h" #include "anbox/network/tcp_socket_messenger.h" +#include "anbox/utils.h" #include #include @@ -54,7 +54,11 @@ AdbMessageProcessor::AdbMessageProcessor( AdbMessageProcessor::~AdbMessageProcessor() { state_ = closed_by_host; + + host_notify_timer_.cancel(); host_connector_.reset(); + + // Unlock our lock to bring down any waiting instance active_instance.unlock(); } @@ -68,6 +72,7 @@ void AdbMessageProcessor::advance_state() { active_instance.lock(); if (state_ == closed_by_host) { + host_notify_timer_.cancel(); host_connector_.reset(); return; } @@ -104,6 +109,9 @@ void AdbMessageProcessor::advance_state() { } void AdbMessageProcessor::wait_for_host_connection() { + if (state_ == closed_by_host || state_ == closed_by_container) + return; + if (!host_connector_) { host_connector_ = std::make_shared( boost::asio::ip::address_v4::from_string(loopback_address), @@ -121,11 +129,11 @@ void AdbMessageProcessor::wait_for_host_connection() { auto message = utils::string_format("host:emulator:%d", default_host_listen_port); auto handshake = utils::string_format("%04x%s", message.size(), message.c_str()); messenger->send(handshake.data(), handshake.size()); - } catch (std::exception &) { + } catch (...) { // Try again later when the host adb service is maybe available + host_notify_timer_.cancel(); host_notify_timer_.expires_from_now(default_adb_wait_time); - host_notify_timer_.async_wait( - [&](const boost::system::error_code &) { wait_for_host_connection(); }); + host_notify_timer_.async_wait([&](const boost::system::error_code &) { wait_for_host_connection(); }); } } @@ -153,11 +161,9 @@ void AdbMessageProcessor::read_next_host_message() { boost::asio::buffer(host_buffer_)); } -void AdbMessageProcessor::on_host_read_size( - const boost::system::error_code &error, std::size_t bytes_read) { +void AdbMessageProcessor::on_host_read_size(const boost::system::error_code &error, std::size_t bytes_read) { if (error) { state_ = closed_by_host; - messenger_->close(); return; } From 62ce199dbc54d0dca70839f15c3215b191248f61 Mon Sep 17 00:00:00 2001 From: Simon Fels Date: Sun, 14 May 2017 12:53:48 +0200 Subject: [PATCH 3/4] Use std::unique_lock for mutex operations --- src/anbox/qemu/adb_message_processor.cpp | 11 +++++------ src/anbox/qemu/adb_message_processor.h | 5 +++++ 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/src/anbox/qemu/adb_message_processor.cpp b/src/anbox/qemu/adb_message_processor.cpp index ef56773..ad2e792 100644 --- a/src/anbox/qemu/adb_message_processor.cpp +++ b/src/anbox/qemu/adb_message_processor.cpp @@ -36,13 +36,14 @@ const std::string start_command{"start"}; // user until we connect to the adb host instance after it appeared and not // too short to not put unnecessary burden on the CPU. const boost::posix_time::seconds default_adb_wait_time{1}; -static std::mutex active_instance; } using namespace std::placeholders; namespace anbox { namespace qemu { +std::mutex AdbMessageProcessor::active_instance_{}; + AdbMessageProcessor::AdbMessageProcessor( const std::shared_ptr &rt, const std::shared_ptr &messenger) @@ -50,16 +51,14 @@ AdbMessageProcessor::AdbMessageProcessor( state_(waiting_for_guest_accept_command), expected_command_(accept_command), messenger_(messenger), - host_notify_timer_(rt->service()) {} + host_notify_timer_(rt->service()), + lock_(active_instance_, std::defer_lock) {} AdbMessageProcessor::~AdbMessageProcessor() { state_ = closed_by_host; host_notify_timer_.cancel(); host_connector_.reset(); - - // Unlock our lock to bring down any waiting instance - active_instance.unlock(); } void AdbMessageProcessor::advance_state() { @@ -69,7 +68,7 @@ void AdbMessageProcessor::advance_state() { // running we don't have to do anything here until that one is done. // The container directly starts a second connection once the first // one is established but will not use it until the active one is closed. - active_instance.lock(); + lock_.lock(); if (state_ == closed_by_host) { host_notify_timer_.cancel(); diff --git a/src/anbox/qemu/adb_message_processor.h b/src/anbox/qemu/adb_message_processor.h index b98fa7a..a58925e 100644 --- a/src/anbox/qemu/adb_message_processor.h +++ b/src/anbox/qemu/adb_message_processor.h @@ -27,6 +27,8 @@ #include +#include + namespace anbox { namespace qemu { class AdbMessageProcessor : public network::MessageProcessor { @@ -66,6 +68,9 @@ class AdbMessageProcessor : public network::MessageProcessor { std::shared_ptr host_messenger_; std::array host_buffer_; boost::asio::deadline_timer host_notify_timer_; + std::unique_lock lock_; + + static std::mutex active_instance_; }; } // namespace graphics } // namespace anbox From 56ed59334ff6f5c474e52967d2cbdf03dbb9b160 Mon Sep 17 00:00:00 2001 From: Simon Fels Date: Sun, 14 May 2017 12:59:08 +0200 Subject: [PATCH 4/4] Don't try to connect to host adb when timer is aborted --- src/anbox/qemu/adb_message_processor.cpp | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/anbox/qemu/adb_message_processor.cpp b/src/anbox/qemu/adb_message_processor.cpp index ad2e792..97135cb 100644 --- a/src/anbox/qemu/adb_message_processor.cpp +++ b/src/anbox/qemu/adb_message_processor.cpp @@ -132,7 +132,11 @@ void AdbMessageProcessor::wait_for_host_connection() { // Try again later when the host adb service is maybe available host_notify_timer_.cancel(); host_notify_timer_.expires_from_now(default_adb_wait_time); - host_notify_timer_.async_wait([&](const boost::system::error_code &) { wait_for_host_connection(); }); + host_notify_timer_.async_wait([this](const boost::system::error_code &err) { + if (err) + return; + wait_for_host_connection(); + }); } } @@ -156,8 +160,7 @@ void AdbMessageProcessor::on_host_connection(std::shared_ptrasync_receive_msg(callback, - boost::asio::buffer(host_buffer_)); + host_messenger_->async_receive_msg(callback, boost::asio::buffer(host_buffer_)); } void AdbMessageProcessor::on_host_read_size(const boost::system::error_code &error, std::size_t bytes_read) {