From 8ae09e81e0617168d79501acb2698b3c5dc55a6e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alfonso=20S=C3=A1nchez-Beato?= Date: Wed, 23 Aug 2017 11:06:34 +0200 Subject: [PATCH 1/5] Undef Status symbol to address conflict between X11 and protobuf --- .../android-emugl/host/libs/Translator/EGL/EglOsApi_glx.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/external/android-emugl/host/libs/Translator/EGL/EglOsApi_glx.cpp b/external/android-emugl/host/libs/Translator/EGL/EglOsApi_glx.cpp index fdcf451..c567efd 100644 --- a/external/android-emugl/host/libs/Translator/EGL/EglOsApi_glx.cpp +++ b/external/android-emugl/host/libs/Translator/EGL/EglOsApi_glx.cpp @@ -25,6 +25,10 @@ #include #include #include +#if defined(Status) +// undef to address conflict between X11 symbol and protobuf +#undef Status +#endif namespace { From 5c7ac4f8854ab27cb5756a19746bf3ad59f65c8c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alfonso=20S=C3=A1nchez-Beato?= Date: Tue, 22 Aug 2017 11:53:42 +0200 Subject: [PATCH 2/5] Release resources on disconnection --- src/anbox/qemu/adb_message_processor.cpp | 31 +++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/src/anbox/qemu/adb_message_processor.cpp b/src/anbox/qemu/adb_message_processor.cpp index 97135cb..263d315 100644 --- a/src/anbox/qemu/adb_message_processor.cpp +++ b/src/anbox/qemu/adb_message_processor.cpp @@ -20,13 +20,21 @@ #include "anbox/network/delegate_message_processor.h" #include "anbox/network/tcp_socket_messenger.h" #include "anbox/utils.h" +#include "anbox/logger.h" #include #include +#ifndef _GNU_SOURCE +#define _GNU_SOURCE +#endif +#include +#include +#include + namespace { const unsigned short default_adb_client_port{5037}; -const unsigned short default_host_listen_port{6664}; +const unsigned short default_host_listen_port{5559}; constexpr const char *loopback_address{"127.0.0.1"}; const std::string accept_command{"accept"}; const std::string ok_command{"ok"}; @@ -52,9 +60,12 @@ AdbMessageProcessor::AdbMessageProcessor( expected_command_(accept_command), messenger_(messenger), host_notify_timer_(rt->service()), - lock_(active_instance_, std::defer_lock) {} + lock_(active_instance_, std::defer_lock) { + INFO("%p constructor", this); +} AdbMessageProcessor::~AdbMessageProcessor() { + INFO("%p destructor", this); state_ = closed_by_host; host_notify_timer_.cancel(); @@ -62,6 +73,7 @@ AdbMessageProcessor::~AdbMessageProcessor() { } void AdbMessageProcessor::advance_state() { + INFO("%p [%d] state %d", this, syscall(SYS_gettid), state_); switch (state_) { case waiting_for_guest_accept_command: // Try to get a lock here as if we already have another processor @@ -108,7 +120,9 @@ void AdbMessageProcessor::advance_state() { } void AdbMessageProcessor::wait_for_host_connection() { - if (state_ == closed_by_host || state_ == closed_by_container) + INFO("%p state %d", this, state_); + + if (state_ != waiting_for_guest_accept_command) return; if (!host_connector_) { @@ -141,6 +155,8 @@ void AdbMessageProcessor::wait_for_host_connection() { } void AdbMessageProcessor::on_host_connection(std::shared_ptr> const &socket) { + INFO("%p state %d", this, state_); + host_messenger_ = std::make_shared(socket); // set_no_delay() reduces the latency of sending data, at the cost @@ -159,13 +175,19 @@ void AdbMessageProcessor::on_host_connection(std::shared_ptrasync_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) { if (error) { + INFO("%p [%d] closed error %d", this, syscall(SYS_gettid), error.value()); state_ = closed_by_host; + host_connector_.reset(); + //messenger_.reset(); + lock_.unlock(); return; } @@ -174,6 +196,8 @@ void AdbMessageProcessor::on_host_read_size(const boost::system::error_code &err } bool AdbMessageProcessor::process_data(const std::vector &data) { + INFO("%p state %d bytes rx %d", this, state_, int(data.size())); + if (state_ == proxying_data) { host_messenger_->send(reinterpret_cast(data.data()), data.size()); @@ -186,6 +210,7 @@ bool AdbMessageProcessor::process_data(const std::vector &data) { buffer_.size() >= expected_command_.size()) { if (::memcmp(buffer_.data(), expected_command_.data(), data.size()) != 0) { // We got not the command we expected and will terminate here + INFO("%p not the expected command", this); return false; } From bc13125585abf5f81b8aabca10ebaafba690ee31 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alfonso=20S=C3=A1nchez-Beato?= Date: Wed, 23 Aug 2017 09:29:27 +0200 Subject: [PATCH 3/5] Avoid crashes on AdbMessageProcessor destruction --- src/anbox/qemu/adb_message_processor.cpp | 28 +++++++++++++++--------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/src/anbox/qemu/adb_message_processor.cpp b/src/anbox/qemu/adb_message_processor.cpp index 263d315..76f83c9 100644 --- a/src/anbox/qemu/adb_message_processor.cpp +++ b/src/anbox/qemu/adb_message_processor.cpp @@ -34,6 +34,9 @@ namespace { const unsigned short default_adb_client_port{5037}; +// For the listening port we have to use an odd port in the 5555-5585 range so +// the host can find us on start. See +// https://developer.android.com/studio/command-line/adb.html. const unsigned short default_host_listen_port{5559}; constexpr const char *loopback_address{"127.0.0.1"}; const std::string accept_command{"accept"}; @@ -120,8 +123,6 @@ void AdbMessageProcessor::advance_state() { } void AdbMessageProcessor::wait_for_host_connection() { - INFO("%p state %d", this, state_); - if (state_ != waiting_for_guest_accept_command) return; @@ -155,8 +156,6 @@ void AdbMessageProcessor::wait_for_host_connection() { } void AdbMessageProcessor::on_host_connection(std::shared_ptr> const &socket) { - INFO("%p state %d", this, state_); - host_messenger_ = std::make_shared(socket); // set_no_delay() reduces the latency of sending data, at the cost @@ -183,11 +182,22 @@ void AdbMessageProcessor::read_next_host_message() { void AdbMessageProcessor::on_host_read_size(const boost::system::error_code &error, std::size_t bytes_read) { if (error) { - INFO("%p [%d] closed error %d", this, syscall(SYS_gettid), error.value()); + // When AdbMessageProcessor is destroyed on program termination, the sockets + // are closed and the standing operations are canceled. But, the callback is + // still called even in that case, and the object has already been + // deleted. We detect that condition by looking at the error code and avoid + // touching *this in that case. + if (error == boost::system::errc::operation_canceled) + return; + + // For other errors, we assume the connection with the host is dropped. We + // close the connection to the container's adbd, which will trigger the + // deletion of this AdbMessageProcessor instance and free resources (most + // importantly, default_host_listen_port and the lock). The standing + // connection that adbd opened can then proceed and wait for the host to be + // up again. state_ = closed_by_host; - host_connector_.reset(); - //messenger_.reset(); - lock_.unlock(); + messenger_->close(); return; } @@ -196,8 +206,6 @@ void AdbMessageProcessor::on_host_read_size(const boost::system::error_code &err } bool AdbMessageProcessor::process_data(const std::vector &data) { - INFO("%p state %d bytes rx %d", this, state_, int(data.size())); - if (state_ == proxying_data) { host_messenger_->send(reinterpret_cast(data.data()), data.size()); From 427354d691b498f2eff7ade73d0393a48cb7084f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alfonso=20S=C3=A1nchez-Beato?= Date: Wed, 23 Aug 2017 09:44:55 +0200 Subject: [PATCH 4/5] Remove now unneeded timer in AdbMessageProcessor --- src/anbox/qemu/adb_message_processor.cpp | 29 ++++-------------------- src/anbox/qemu/adb_message_processor.h | 1 - 2 files changed, 5 insertions(+), 25 deletions(-) diff --git a/src/anbox/qemu/adb_message_processor.cpp b/src/anbox/qemu/adb_message_processor.cpp index 76f83c9..c901862 100644 --- a/src/anbox/qemu/adb_message_processor.cpp +++ b/src/anbox/qemu/adb_message_processor.cpp @@ -25,13 +25,6 @@ #include #include -#ifndef _GNU_SOURCE -#define _GNU_SOURCE -#endif -#include -#include -#include - namespace { const unsigned short default_adb_client_port{5037}; // For the listening port we have to use an odd port in the 5555-5585 range so @@ -62,21 +55,19 @@ AdbMessageProcessor::AdbMessageProcessor( state_(waiting_for_guest_accept_command), expected_command_(accept_command), messenger_(messenger), - host_notify_timer_(rt->service()), lock_(active_instance_, std::defer_lock) { - INFO("%p constructor", this); + DEBUG("%p constructor", this); } AdbMessageProcessor::~AdbMessageProcessor() { - INFO("%p destructor", this); + DEBUG("%p destructor", this); state_ = closed_by_host; - host_notify_timer_.cancel(); host_connector_.reset(); } void AdbMessageProcessor::advance_state() { - INFO("%p [%d] state %d", this, syscall(SYS_gettid), state_); + DEBUG("%p state %d", this, state_); switch (state_) { case waiting_for_guest_accept_command: // Try to get a lock here as if we already have another processor @@ -86,7 +77,6 @@ void AdbMessageProcessor::advance_state() { lock_.lock(); if (state_ == closed_by_host) { - host_notify_timer_.cancel(); host_connector_.reset(); return; } @@ -144,14 +134,7 @@ void AdbMessageProcessor::wait_for_host_connection() { auto handshake = utils::string_format("%04x%s", message.size(), message.c_str()); messenger->send(handshake.data(), handshake.size()); } 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([this](const boost::system::error_code &err) { - if (err) - return; - wait_for_host_connection(); - }); + DEBUG("%p adb host is not up yet", this); } } @@ -174,8 +157,6 @@ void AdbMessageProcessor::on_host_connection(std::shared_ptrasync_receive_msg(callback, boost::asio::buffer(host_buffer_)); } @@ -218,7 +199,7 @@ bool AdbMessageProcessor::process_data(const std::vector &data) { buffer_.size() >= expected_command_.size()) { if (::memcmp(buffer_.data(), expected_command_.data(), data.size()) != 0) { // We got not the command we expected and will terminate here - INFO("%p not the expected command", this); + WARNING("%p not the expected command", this); return false; } diff --git a/src/anbox/qemu/adb_message_processor.h b/src/anbox/qemu/adb_message_processor.h index a58925e..5b41d5b 100644 --- a/src/anbox/qemu/adb_message_processor.h +++ b/src/anbox/qemu/adb_message_processor.h @@ -67,7 +67,6 @@ class AdbMessageProcessor : public network::MessageProcessor { std::shared_ptr host_connector_; 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_; From 1e884a12f7b64be390ab2276c21d2a37f26e4036 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alfonso=20S=C3=A1nchez-Beato?= Date: Wed, 23 Aug 2017 11:10:30 +0200 Subject: [PATCH 5/5] Remove traces in AdbMessageProcessor --- src/anbox/qemu/adb_message_processor.cpp | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/anbox/qemu/adb_message_processor.cpp b/src/anbox/qemu/adb_message_processor.cpp index c901862..24d3bb7 100644 --- a/src/anbox/qemu/adb_message_processor.cpp +++ b/src/anbox/qemu/adb_message_processor.cpp @@ -20,7 +20,6 @@ #include "anbox/network/delegate_message_processor.h" #include "anbox/network/tcp_socket_messenger.h" #include "anbox/utils.h" -#include "anbox/logger.h" #include #include @@ -56,18 +55,15 @@ AdbMessageProcessor::AdbMessageProcessor( expected_command_(accept_command), messenger_(messenger), lock_(active_instance_, std::defer_lock) { - DEBUG("%p constructor", this); } AdbMessageProcessor::~AdbMessageProcessor() { - DEBUG("%p destructor", this); state_ = closed_by_host; host_connector_.reset(); } void AdbMessageProcessor::advance_state() { - DEBUG("%p state %d", this, state_); switch (state_) { case waiting_for_guest_accept_command: // Try to get a lock here as if we already have another processor @@ -134,7 +130,7 @@ void AdbMessageProcessor::wait_for_host_connection() { auto handshake = utils::string_format("%04x%s", message.size(), message.c_str()); messenger->send(handshake.data(), handshake.size()); } catch (...) { - DEBUG("%p adb host is not up yet", this); + // Server not up. No problem, it will contact us when started. } } @@ -199,7 +195,6 @@ bool AdbMessageProcessor::process_data(const std::vector &data) { buffer_.size() >= expected_command_.size()) { if (::memcmp(buffer_.data(), expected_command_.data(), data.size()) != 0) { // We got not the command we expected and will terminate here - WARNING("%p not the expected command", this); return false; }