From 1ce99576f0cf76a9cfe4176c0a4a2cb018568a52 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Wed, 7 Mar 2018 16:52:28 -0800 Subject: [PATCH 1/5] adb: switch apacket payload to a type that doesn't initialize its contents. Switch from using std::string as the type we use to hold our payload in apacket to a custom reimplementation that doesn't zero initialize. This improves bulk transfer throughput in the adb_benchmark microbenchmark on walleye by ~20%. Test: adb shell taskset f0 /data/benchmarktest64/adb_benchmark/adb_benchmark Change-Id: Ibad797701eb1460c9321b0400c5b167b89b2b4d0 --- adb/adb.cpp | 16 ++-- adb/adb.h | 15 +--- adb/client/auth.cpp | 6 +- adb/daemon/jdwp_service.cpp | 11 +-- adb/range.h | 65 -------------- adb/socket.h | 5 +- adb/socket_test.cpp | 2 +- adb/sockets.cpp | 16 ++-- adb/transport.cpp | 4 +- adb/types.h | 163 ++++++++++++++++++++++++++++++++++++ 10 files changed, 196 insertions(+), 107 deletions(-) delete mode 100644 adb/range.h create mode 100644 adb/types.h diff --git a/adb/adb.cpp b/adb/adb.cpp index 65fa2e795..702a5a2fb 100644 --- a/adb/adb.cpp +++ b/adb/adb.cpp @@ -257,7 +257,7 @@ void send_connect(atransport* t) { << connection_str.length() << ")"; } - cp->payload = std::move(connection_str); + cp->payload.assign(connection_str.begin(), connection_str.end()); cp->msg.data_length = cp->payload.size(); send_packet(cp, t); @@ -329,7 +329,8 @@ static void handle_new_connection(atransport* t, apacket* p) { handle_offline(t); t->update_version(p->msg.arg0, p->msg.arg1); - parse_banner(p->payload, t); + std::string banner(p->payload.begin(), p->payload.end()); + parse_banner(banner, t); #if ADB_HOST handle_online(t); @@ -369,8 +370,10 @@ void handle_packet(apacket *p, atransport *t) send_auth_response(p->payload.data(), p->msg.data_length, t); break; #else - case ADB_AUTH_SIGNATURE: - if (adbd_auth_verify(t->token, sizeof(t->token), p->payload)) { + case ADB_AUTH_SIGNATURE: { + // TODO: Switch to string_view. + std::string signature(p->payload.begin(), p->payload.end()); + if (adbd_auth_verify(t->token, sizeof(t->token), signature)) { adbd_auth_verified(t); t->failed_auth_attempts = 0; } else { @@ -378,6 +381,7 @@ void handle_packet(apacket *p, atransport *t) send_auth_request(t); } break; + } case ADB_AUTH_RSAPUBLICKEY: adbd_auth_confirm_key(p->payload.data(), p->msg.data_length, t); @@ -392,7 +396,9 @@ void handle_packet(apacket *p, atransport *t) case A_OPEN: /* OPEN(local-id, 0, "destination") */ if (t->online && p->msg.arg0 != 0 && p->msg.arg1 == 0) { - asocket* s = create_local_service_socket(p->payload.c_str(), t); + // TODO: Switch to string_view. + std::string address(p->payload.begin(), p->payload.end()); + asocket* s = create_local_service_socket(address.c_str(), t); if (s == nullptr) { send_close(0, p->msg.arg0, t); } else { diff --git a/adb/adb.h b/adb/adb.h index c74fa99aa..86da43c35 100644 --- a/adb/adb.h +++ b/adb/adb.h @@ -28,6 +28,7 @@ #include "adb_trace.h" #include "fdevent.h" #include "socket.h" +#include "types.h" #include "usb.h" constexpr size_t MAX_PAYLOAD_V1 = 4 * 1024; @@ -63,20 +64,6 @@ std::string adb_version(); using TransportId = uint64_t; class atransport; -struct amessage { - uint32_t command; /* command identifier constant */ - uint32_t arg0; /* first argument */ - uint32_t arg1; /* second argument */ - uint32_t data_length; /* length of payload (0 is allowed) */ - uint32_t data_check; /* checksum of data payload */ - uint32_t magic; /* command ^ 0xffffffff */ -}; - -struct apacket { - amessage msg; - std::string payload; -}; - uint32_t calculate_apacket_checksum(const apacket* packet); /* the adisconnect structure is used to record a callback that diff --git a/adb/client/auth.cpp b/adb/client/auth.cpp index c3aef16d4..ade2623b8 100644 --- a/adb/client/auth.cpp +++ b/adb/client/auth.cpp @@ -454,10 +454,8 @@ static void send_auth_publickey(atransport* t) { p->msg.command = A_AUTH; p->msg.arg0 = ADB_AUTH_RSAPUBLICKEY; - p->payload = std::move(key); - // adbd expects a null-terminated string. - p->payload.push_back('\0'); + p->payload.assign(key.data(), key.data() + key.size() + 1); p->msg.data_length = p->payload.size(); send_packet(p, t); } @@ -482,7 +480,7 @@ void send_auth_response(const char* token, size_t token_size, atransport* t) { p->msg.command = A_AUTH; p->msg.arg0 = ADB_AUTH_SIGNATURE; - p->payload = std::move(result); + p->payload.assign(result.begin(), result.end()); p->msg.data_length = p->payload.size(); send_packet(p, t); } diff --git a/adb/daemon/jdwp_service.cpp b/adb/daemon/jdwp_service.cpp index 976155884..89577cb7c 100644 --- a/adb/daemon/jdwp_service.cpp +++ b/adb/daemon/jdwp_service.cpp @@ -459,7 +459,7 @@ static void jdwp_socket_close(asocket* s) { delete s; } -static int jdwp_socket_enqueue(asocket* s, std::string) { +static int jdwp_socket_enqueue(asocket* s, apacket::payload_type) { /* you can't write to this asocket */ D("LS(%d): JDWP socket received data?", s->id); s->peer->close(s->peer); @@ -474,7 +474,7 @@ static void jdwp_socket_ready(asocket* s) { * on the second one, close the connection */ if (!jdwp->pass) { - std::string data; + apacket::payload_type data; data.resize(s->get_max_payload()); size_t len = jdwp_process_list(&data[0], data.size()); data.resize(len); @@ -521,7 +521,8 @@ static void jdwp_process_list_updated(void) { for (auto& t : _jdwp_trackers) { if (t->peer) { // The tracker might not have been connected yet. - t->peer->enqueue(t->peer, data); + apacket::payload_type payload(data.begin(), data.end()); + t->peer->enqueue(t->peer, std::move(payload)); } } } @@ -547,7 +548,7 @@ static void jdwp_tracker_ready(asocket* s) { JdwpTracker* t = (JdwpTracker*)s; if (t->need_initial) { - std::string data; + apacket::payload_type data; data.resize(s->get_max_payload()); data.resize(jdwp_process_list_msg(&data[0], data.size())); t->need_initial = false; @@ -555,7 +556,7 @@ static void jdwp_tracker_ready(asocket* s) { } } -static int jdwp_tracker_enqueue(asocket* s, std::string) { +static int jdwp_tracker_enqueue(asocket* s, apacket::payload_type) { /* you can't write to this socket */ D("LS(%d): JDWP tracker received data?", s->id); s->peer->close(s->peer); diff --git a/adb/range.h b/adb/range.h deleted file mode 100644 index 7a0b8221d..000000000 --- a/adb/range.h +++ /dev/null @@ -1,65 +0,0 @@ -#pragma once - -/* - * Copyright (C) 2018 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -#include - -#include - -struct Range { - explicit Range(std::string data) : data_(std::move(data)) {} - - Range(const Range& copy) = delete; - Range& operator=(const Range& copy) = delete; - - Range(Range&& move) = default; - Range& operator=(Range&& move) = default; - - bool empty() const { - return size() == 0; - } - - size_t size() const { - return data_.size() - begin_offset_ - end_offset_; - }; - - void drop_front(size_t n) { - CHECK_GE(size(), n); - begin_offset_ += n; - } - - void drop_end(size_t n) { - CHECK_GE(size(), n); - end_offset_ += n; - } - - char* data() { - return &data_[0] + begin_offset_; - } - - std::string::iterator begin() { - return data_.begin() + begin_offset_; - } - - std::string::iterator end() { - return data_.end() - end_offset_; - } - - std::string data_; - size_t begin_offset_ = 0; - size_t end_offset_ = 0; -}; diff --git a/adb/socket.h b/adb/socket.h index 2f0908050..e8cb58bd7 100644 --- a/adb/socket.h +++ b/adb/socket.h @@ -24,9 +24,8 @@ #include #include "fdevent.h" -#include "range.h" +#include "types.h" -struct apacket; class atransport; /* An asocket represents one half of a connection between a local and @@ -73,7 +72,7 @@ struct asocket { * peer->ready() when we once again are ready to * receive data. */ - int (*enqueue)(asocket* s, std::string data) = nullptr; + int (*enqueue)(asocket* s, apacket::payload_type data) = nullptr; /* ready is called by the peer when it is ready for * us to send data via enqueue again diff --git a/adb/socket_test.cpp b/adb/socket_test.cpp index 6c4a8b2c1..04214a20b 100644 --- a/adb/socket_test.cpp +++ b/adb/socket_test.cpp @@ -118,7 +118,7 @@ static void CreateCloser(CloseWithPacketArg* arg) { // each write to give the underlying implementation time to flush. bool socket_filled = false; for (int i = 0; i < 128; ++i) { - std::string data; + apacket::payload_type data; data.resize(MAX_PAYLOAD); arg->bytes_written += data.size(); int ret = s->enqueue(s, std::move(data)); diff --git a/adb/sockets.cpp b/adb/sockets.cpp index 0887e6f01..f3fce27a1 100644 --- a/adb/sockets.cpp +++ b/adb/sockets.cpp @@ -37,8 +37,8 @@ #include "adb.h" #include "adb_io.h" -#include "range.h" #include "transport.h" +#include "types.h" static std::recursive_mutex& local_socket_list_lock = *new std::recursive_mutex(); static unsigned local_socket_next_id = 1; @@ -147,7 +147,7 @@ static SocketFlushResult local_socket_flush_incoming(asocket* s) { // Returns false if the socket has been closed and destroyed as a side-effect of this function. static bool local_socket_flush_outgoing(asocket* s) { const size_t max_payload = s->get_max_payload(); - std::string data; + apacket::payload_type data; data.resize(max_payload); char* x = &data[0]; size_t avail = max_payload; @@ -214,7 +214,7 @@ static bool local_socket_flush_outgoing(asocket* s) { return true; } -static int local_socket_enqueue(asocket* s, std::string data) { +static int local_socket_enqueue(asocket* s, apacket::payload_type data) { D("LS(%d): enqueue %zu", s->id, data.size()); Range r(std::move(data)); @@ -394,7 +394,7 @@ static asocket* create_host_service_socket(const char* name, const char* serial, } #endif /* ADB_HOST */ -static int remote_socket_enqueue(asocket* s, std::string data) { +static int remote_socket_enqueue(asocket* s, apacket::payload_type data) { D("entered remote_socket_enqueue RS(%d) WRITE fd=%d peer.fd=%d", s->id, s->fd, s->peer->fd); apacket* p = get_apacket(); @@ -476,8 +476,7 @@ void connect_to_remote(asocket* s, const char* destination) { p->msg.arg0 = s->id; // adbd expects a null-terminated string. - p->payload = destination; - p->payload.push_back('\0'); + p->payload.assign(destination, destination + strlen(destination) + 1); p->msg.data_length = p->payload.size(); if (p->msg.data_length > s->get_max_payload()) { @@ -612,7 +611,7 @@ char* skip_host_serial(char* service) { #endif // ADB_HOST -static int smart_socket_enqueue(asocket* s, std::string data) { +static int smart_socket_enqueue(asocket* s, apacket::payload_type data) { #if ADB_HOST char* service = nullptr; char* serial = nullptr; @@ -623,7 +622,8 @@ static int smart_socket_enqueue(asocket* s, std::string data) { D("SS(%d): enqueue %zu", s->id, data.size()); if (s->smart_socket_data.empty()) { - s->smart_socket_data = std::move(data); + // TODO: Make this a BlockChain? + s->smart_socket_data.assign(data.begin(), data.end()); } else { std::copy(data.begin(), data.end(), std::back_inserter(s->smart_socket_data)); } diff --git a/adb/transport.cpp b/adb/transport.cpp index 2867d3837..92c52e23c 100644 --- a/adb/transport.cpp +++ b/adb/transport.cpp @@ -335,7 +335,7 @@ static void device_tracker_close(asocket* socket) { delete tracker; } -static int device_tracker_enqueue(asocket* socket, std::string) { +static int device_tracker_enqueue(asocket* socket, apacket::payload_type) { /* you can't read from a device tracker, close immediately */ device_tracker_close(socket); return -1; @@ -344,7 +344,7 @@ static int device_tracker_enqueue(asocket* socket, std::string) { static int device_tracker_send(device_tracker* tracker, const std::string& string) { asocket* peer = tracker->socket.peer; - std::string data; + apacket::payload_type data; data.resize(4 + string.size()); char buf[5]; snprintf(buf, sizeof(buf), "%04x", static_cast(string.size())); diff --git a/adb/types.h b/adb/types.h new file mode 100644 index 000000000..dd3e06390 --- /dev/null +++ b/adb/types.h @@ -0,0 +1,163 @@ +/* + * Copyright (C) 2018 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#pragma once + +#include +#include + +#include + +#include "sysdeps/memory.h" + +// Essentially std::vector, except without zero initialization or reallocation. +struct Block { + using iterator = char*; + + Block() {} + + explicit Block(size_t size) { allocate(size); } + + template + Block(Iterator begin, Iterator end) : Block(end - begin) { + std::copy(begin, end, data_); + } + + Block(const Block& copy) = delete; + Block(Block&& move) { + std::swap(data_, move.data_); + std::swap(capacity_, move.capacity_); + std::swap(size_, move.size_); + } + + Block& operator=(const Block& copy) = delete; + Block& operator=(Block&& move) { + clear(); + + std::swap(data_, move.data_); + std::swap(capacity_, move.capacity_); + std::swap(size_, move.size_); + + return *this; + } + + ~Block() { clear(); } + + void resize(size_t new_size) { + if (!data_) { + allocate(new_size); + } else { + CHECK_GE(capacity_, new_size); + size_ = new_size; + } + } + + template + void assign(InputIt begin, InputIt end) { + clear(); + allocate(end - begin); + std::copy(begin, end, data_); + } + + void clear() { + free(data_); + capacity_ = 0; + size_ = 0; + } + + size_t capacity() const { return capacity_; } + size_t size() const { return size_; } + bool empty() const { return size() == 0; } + + char* data() { return data_; } + const char* data() const { return data_; } + + char* begin() { return data_; } + const char* begin() const { return data_; } + + char* end() { return data() + size_; } + const char* end() const { return data() + size_; } + + char& operator[](size_t idx) { return data()[idx]; } + const char& operator[](size_t idx) const { return data()[idx]; } + + bool operator==(const Block& rhs) const { + return size() == rhs.size() && memcmp(data(), rhs.data(), size()) == 0; + } + + private: + void allocate(size_t size) { + CHECK(data_ == nullptr); + CHECK_EQ(0ULL, capacity_); + CHECK_EQ(0ULL, size_); + if (size != 0) { + data_ = static_cast(malloc(size)); + capacity_ = size; + size_ = size; + } + } + + char* data_ = nullptr; + size_t capacity_ = 0; + size_t size_ = 0; +}; + +struct amessage { + uint32_t command; /* command identifier constant */ + uint32_t arg0; /* first argument */ + uint32_t arg1; /* second argument */ + uint32_t data_length; /* length of payload (0 is allowed) */ + uint32_t data_check; /* checksum of data payload */ + uint32_t magic; /* command ^ 0xffffffff */ +}; + +struct apacket { + using payload_type = Block; + amessage msg; + payload_type payload; +}; + +struct Range { + explicit Range(apacket::payload_type data) : data_(std::move(data)) {} + + Range(const Range& copy) = delete; + Range& operator=(const Range& copy) = delete; + + Range(Range&& move) = default; + Range& operator=(Range&& move) = default; + + size_t size() const { return data_.size() - begin_offset_ - end_offset_; }; + bool empty() const { return size() == 0; } + + void drop_front(size_t n) { + CHECK_GE(size(), n); + begin_offset_ += n; + } + + void drop_end(size_t n) { + CHECK_GE(size(), n); + end_offset_ += n; + } + + char* data() { return &data_[0] + begin_offset_; } + + apacket::payload_type::iterator begin() { return data_.begin() + begin_offset_; } + apacket::payload_type::iterator end() { return data_.end() - end_offset_; } + + apacket::payload_type data_; + size_t begin_offset_ = 0; + size_t end_offset_ = 0; +}; From 116aa0a4adf3756cb49b2c3090de8ae04eb0824d Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Thu, 5 Apr 2018 17:55:25 -0700 Subject: [PATCH 2/5] adb: implement adb_writev. Change-Id: I55258c155d7b07368ebb45577b2e01ca804cf258 Test: adb_test Test: python test_device.py --- adb/sysdeps/uio.h | 39 +++++++++++ adb/sysdeps_win32.cpp | 155 +++++++++++++++++++++++++++++++----------- 2 files changed, 154 insertions(+), 40 deletions(-) create mode 100644 adb/sysdeps/uio.h diff --git a/adb/sysdeps/uio.h b/adb/sysdeps/uio.h new file mode 100644 index 000000000..d06ef8958 --- /dev/null +++ b/adb/sysdeps/uio.h @@ -0,0 +1,39 @@ +/* + * Copyright (C) 2018 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#pragma once + +#include + +#if defined(_WIN32) + +// Layout of this struct must match struct WSABUF (verified via static assert in sysdeps_win32.cpp) +struct adb_iovec { + size_t iov_len; + void* iov_base; +}; + +ssize_t adb_writev(int fd, const adb_iovec* iov, int iovcnt); + +#else + +#include +using adb_iovec = struct iovec; +#define adb_writev writev + +#endif + +#pragma GCC poison writev diff --git a/adb/sysdeps_win32.cpp b/adb/sysdeps_win32.cpp index 7d35fb67b..433a59d8d 100644 --- a/adb/sysdeps_win32.cpp +++ b/adb/sysdeps_win32.cpp @@ -36,6 +36,7 @@ #include #include +#include #include #include #include @@ -43,6 +44,8 @@ #include "adb.h" #include "adb_utils.h" +#include "sysdeps/uio.h" + extern void fatal(const char *fmt, ...); /* forward declarations */ @@ -57,6 +60,7 @@ typedef struct FHClassRec_ { int (*_fh_lseek)(FH, int, int); int (*_fh_read)(FH, void*, int); int (*_fh_write)(FH, const void*, int); + int (*_fh_writev)(FH, const adb_iovec*, int); } FHClassRec; static void _fh_file_init(FH); @@ -64,6 +68,7 @@ static int _fh_file_close(FH); static int _fh_file_lseek(FH, int, int); static int _fh_file_read(FH, void*, int); static int _fh_file_write(FH, const void*, int); +static int _fh_file_writev(FH, const adb_iovec*, int); static const FHClassRec _fh_file_class = { _fh_file_init, @@ -71,6 +76,7 @@ static const FHClassRec _fh_file_class = { _fh_file_lseek, _fh_file_read, _fh_file_write, + _fh_file_writev, }; static void _fh_socket_init(FH); @@ -78,6 +84,7 @@ static int _fh_socket_close(FH); static int _fh_socket_lseek(FH, int, int); static int _fh_socket_read(FH, void*, int); static int _fh_socket_write(FH, const void*, int); +static int _fh_socket_writev(FH, const adb_iovec*, int); static const FHClassRec _fh_socket_class = { _fh_socket_init, @@ -85,6 +92,7 @@ static const FHClassRec _fh_socket_class = { _fh_socket_lseek, _fh_socket_read, _fh_socket_write, + _fh_socket_writev, }; #define assert(cond) \ @@ -248,57 +256,88 @@ typedef std::unique_ptr unique_fh; /**************************************************************************/ /**************************************************************************/ -static void _fh_file_init( FH f ) { +static void _fh_file_init(FH f) { f->fh_handle = INVALID_HANDLE_VALUE; } -static int _fh_file_close( FH f ) { - CloseHandle( f->fh_handle ); +static int _fh_file_close(FH f) { + CloseHandle(f->fh_handle); f->fh_handle = INVALID_HANDLE_VALUE; return 0; } -static int _fh_file_read( FH f, void* buf, int len ) { - DWORD read_bytes; +static int _fh_file_read(FH f, void* buf, int len) { + DWORD read_bytes; - if ( !ReadFile( f->fh_handle, buf, (DWORD)len, &read_bytes, NULL ) ) { - D( "adb_read: could not read %d bytes from %s", len, f->name ); + if (!ReadFile(f->fh_handle, buf, (DWORD)len, &read_bytes, NULL)) { + D("adb_read: could not read %d bytes from %s", len, f->name); errno = EIO; return -1; } else if (read_bytes < (DWORD)len) { f->eof = 1; } - return (int)read_bytes; + return read_bytes; } -static int _fh_file_write( FH f, const void* buf, int len ) { - DWORD wrote_bytes; +static int _fh_file_write(FH f, const void* buf, int len) { + DWORD wrote_bytes; - if ( !WriteFile( f->fh_handle, buf, (DWORD)len, &wrote_bytes, NULL ) ) { - D( "adb_file_write: could not write %d bytes from %s", len, f->name ); + if (!WriteFile(f->fh_handle, buf, (DWORD)len, &wrote_bytes, NULL)) { + D("adb_file_write: could not write %d bytes from %s", len, f->name); errno = EIO; return -1; } else if (wrote_bytes < (DWORD)len) { f->eof = 1; } - return (int)wrote_bytes; + return wrote_bytes; } -static int _fh_file_lseek( FH f, int pos, int origin ) { - DWORD method; - DWORD result; +static int _fh_file_writev(FH f, const adb_iovec* iov, int iovcnt) { + if (iovcnt <= 0) { + errno = EINVAL; + return -1; + } - switch (origin) - { - case SEEK_SET: method = FILE_BEGIN; break; - case SEEK_CUR: method = FILE_CURRENT; break; - case SEEK_END: method = FILE_END; break; + DWORD wrote_bytes = 0; + + for (int i = 0; i < iovcnt; ++i) { + ssize_t rc = _fh_file_write(f, iov[i].iov_base, iov[i].iov_len); + if (rc == -1) { + return wrote_bytes > 0 ? wrote_bytes : -1; + } else if (rc == 0) { + return wrote_bytes; + } + + wrote_bytes += rc; + + if (static_cast(rc) < iov[i].iov_len) { + return wrote_bytes; + } + } + + return wrote_bytes; +} + +static int _fh_file_lseek(FH f, int pos, int origin) { + DWORD method; + DWORD result; + + switch (origin) { + case SEEK_SET: + method = FILE_BEGIN; + break; + case SEEK_CUR: + method = FILE_CURRENT; + break; + case SEEK_END: + method = FILE_END; + break; default: errno = EINVAL; return -1; } - result = SetFilePointer( f->fh_handle, pos, NULL, method ); + result = SetFilePointer(f->fh_handle, pos, NULL, method); if (result == INVALID_SET_FILE_POINTER) { errno = EIO; return -1; @@ -308,7 +347,6 @@ static int _fh_file_lseek( FH f, int pos, int origin ) { return (int)result; } - /**************************************************************************/ /**************************************************************************/ /***** *****/ @@ -424,22 +462,18 @@ int adb_creat(const char* path, int mode) return _fh_to_int(f); } - -int adb_read(int fd, void* buf, int len) -{ - FH f = _fh_from_int(fd, __func__); +int adb_read(int fd, void* buf, int len) { + FH f = _fh_from_int(fd, __func__); if (f == NULL) { return -1; } - return f->clazz->_fh_read( f, buf, len ); + return f->clazz->_fh_read(f, buf, len); } - -int adb_write(int fd, const void* buf, int len) -{ - FH f = _fh_from_int(fd, __func__); +int adb_write(int fd, const void* buf, int len) { + FH f = _fh_from_int(fd, __func__); if (f == NULL) { return -1; @@ -448,6 +482,16 @@ int adb_write(int fd, const void* buf, int len) return f->clazz->_fh_write(f, buf, len); } +ssize_t adb_writev(int fd, const adb_iovec* iov, int iovcnt) { + FH f = _fh_from_int(fd, __func__); + + if (f == NULL) { + errno = EBADF; + return -1; + } + + return f->clazz->_fh_writev(f, iov, iovcnt); +} int adb_lseek(int fd, int pos, int where) { @@ -582,7 +626,7 @@ static void _fh_socket_init(FH f) { f->fh_socket = INVALID_SOCKET; } -static int _fh_socket_close( FH f ) { +static int _fh_socket_close(FH f) { if (f->fh_socket != INVALID_SOCKET) { /* gently tell any peer that we're closing the socket */ if (shutdown(f->fh_socket, SD_BOTH) == SOCKET_ERROR) { @@ -603,13 +647,13 @@ static int _fh_socket_close( FH f ) { return 0; } -static int _fh_socket_lseek( FH f, int pos, int origin ) { +static int _fh_socket_lseek(FH f, int pos, int origin) { errno = EPIPE; return -1; } static int _fh_socket_read(FH f, void* buf, int len) { - int result = recv(f->fh_socket, reinterpret_cast(buf), len, 0); + int result = recv(f->fh_socket, reinterpret_cast(buf), len, 0); if (result == SOCKET_ERROR) { const DWORD err = WSAGetLastError(); // WSAEWOULDBLOCK is normal with a non-blocking socket, so don't trace @@ -621,11 +665,11 @@ static int _fh_socket_read(FH f, void* buf, int len) { _socket_set_errno(err); result = -1; } - return result; + return result; } static int _fh_socket_write(FH f, const void* buf, int len) { - int result = send(f->fh_socket, reinterpret_cast(buf), len, 0); + int result = send(f->fh_socket, reinterpret_cast(buf), len, 0); if (result == SOCKET_ERROR) { const DWORD err = WSAGetLastError(); // WSAEWOULDBLOCK is normal with a non-blocking socket, so don't trace @@ -639,13 +683,44 @@ static int _fh_socket_write(FH f, const void* buf, int len) { } else { // According to https://code.google.com/p/chromium/issues/detail?id=27870 // Winsock Layered Service Providers may cause this. - CHECK_LE(result, len) << "Tried to write " << len << " bytes to " - << f->name << ", but " << result - << " bytes reportedly written"; + CHECK_LE(result, len) << "Tried to write " << len << " bytes to " << f->name << ", but " + << result << " bytes reportedly written"; } return result; } +// Make sure that adb_iovec is compatible with WSABUF. +static_assert(sizeof(adb_iovec) == sizeof(WSABUF), ""); +static_assert(SIZEOF_MEMBER(adb_iovec, iov_len) == SIZEOF_MEMBER(WSABUF, len), ""); +static_assert(offsetof(adb_iovec, iov_len) == offsetof(WSABUF, len), ""); + +static_assert(SIZEOF_MEMBER(adb_iovec, iov_base) == SIZEOF_MEMBER(WSABUF, buf), ""); +static_assert(offsetof(adb_iovec, iov_base) == offsetof(WSABUF, buf), ""); + +static int _fh_socket_writev(FH f, const adb_iovec* iov, int iovcnt) { + if (iovcnt <= 0) { + errno = EINVAL; + return -1; + } + + WSABUF* wsabuf = reinterpret_cast(const_cast(iov)); + DWORD bytes_written = 0; + int result = WSASend(f->fh_socket, wsabuf, iovcnt, &bytes_written, 0, nullptr, nullptr); + if (result == SOCKET_ERROR) { + const DWORD err = WSAGetLastError(); + // WSAEWOULDBLOCK is normal with a non-blocking socket, so don't trace + // that to reduce spam and confusion. + if (err != WSAEWOULDBLOCK) { + D("send fd %d failed: %s", _fh_to_int(f), + android::base::SystemErrorCodeToString(err).c_str()); + } + _socket_set_errno(err); + result = -1; + } + CHECK_GE(static_cast(std::numeric_limits::max()), bytes_written); + return static_cast(bytes_written); +} + /**************************************************************************/ /**************************************************************************/ /***** *****/ From 64a63acba93d75fb28374a198babae221b21b1c7 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Thu, 5 Apr 2018 18:09:02 -0700 Subject: [PATCH 3/5] adb: partially clang-format sysdeps_win32.cpp. clang-format some functions that I'm about to touch, but do it in a separate commit to avoid making the actual changes unreadable. Test: treehugger Change-Id: I7dc5258d9ecc1c091d42311149d9e18ae483715b --- adb/sysdeps_win32.cpp | 116 +++++++++++++++++++----------------------- 1 file changed, 53 insertions(+), 63 deletions(-) diff --git a/adb/sysdeps_win32.cpp b/adb/sysdeps_win32.cpp index 433a59d8d..b5334b141 100644 --- a/adb/sysdeps_win32.cpp +++ b/adb/sysdeps_win32.cpp @@ -355,12 +355,11 @@ static int _fh_file_lseek(FH f, int pos, int origin) { /**************************************************************************/ /**************************************************************************/ -int adb_open(const char* path, int options) -{ - FH f; +int adb_open(const char* path, int options) { + FH f; - DWORD desiredAccess = 0; - DWORD shareMode = FILE_SHARE_READ | FILE_SHARE_WRITE; + DWORD desiredAccess = 0; + DWORD shareMode = FILE_SHARE_READ | FILE_SHARE_WRITE; switch (options) { case O_RDONLY: @@ -378,8 +377,8 @@ int adb_open(const char* path, int options) return -1; } - f = _fh_alloc( &_fh_file_class ); - if ( !f ) { + f = _fh_alloc(&_fh_file_class); + if (!f) { return -1; } @@ -387,21 +386,21 @@ int adb_open(const char* path, int options) if (!android::base::UTF8ToWide(path, &path_wide)) { return -1; } - f->fh_handle = CreateFileW( path_wide.c_str(), desiredAccess, shareMode, - NULL, OPEN_EXISTING, 0, NULL ); + f->fh_handle = + CreateFileW(path_wide.c_str(), desiredAccess, shareMode, NULL, OPEN_EXISTING, 0, NULL); - if ( f->fh_handle == INVALID_HANDLE_VALUE ) { + if (f->fh_handle == INVALID_HANDLE_VALUE) { const DWORD err = GetLastError(); _fh_close(f); - D( "adb_open: could not open '%s': ", path ); + D("adb_open: could not open '%s': ", path); switch (err) { case ERROR_FILE_NOT_FOUND: - D( "file not found" ); + D("file not found"); errno = ENOENT; return -1; case ERROR_PATH_NOT_FOUND: - D( "path not found" ); + D("path not found"); errno = ENOTDIR; return -1; @@ -412,18 +411,17 @@ int adb_open(const char* path, int options) } } - snprintf( f->name, sizeof(f->name), "%d(%s)", _fh_to_int(f), path ); - D( "adb_open: '%s' => fd %d", path, _fh_to_int(f) ); + snprintf(f->name, sizeof(f->name), "%d(%s)", _fh_to_int(f), path); + D("adb_open: '%s' => fd %d", path, _fh_to_int(f)); return _fh_to_int(f); } /* ignore mode on Win32 */ -int adb_creat(const char* path, int mode) -{ - FH f; +int adb_creat(const char* path, int mode) { + FH f; - f = _fh_alloc( &_fh_file_class ); - if ( !f ) { + f = _fh_alloc(&_fh_file_class); + if (!f) { return -1; } @@ -431,23 +429,21 @@ int adb_creat(const char* path, int mode) if (!android::base::UTF8ToWide(path, &path_wide)) { return -1; } - f->fh_handle = CreateFileW( path_wide.c_str(), GENERIC_WRITE, - FILE_SHARE_READ | FILE_SHARE_WRITE, - NULL, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, - NULL ); + f->fh_handle = CreateFileW(path_wide.c_str(), GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_WRITE, + NULL, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL); - if ( f->fh_handle == INVALID_HANDLE_VALUE ) { + if (f->fh_handle == INVALID_HANDLE_VALUE) { const DWORD err = GetLastError(); _fh_close(f); - D( "adb_creat: could not open '%s': ", path ); + D("adb_creat: could not open '%s': ", path); switch (err) { case ERROR_FILE_NOT_FOUND: - D( "file not found" ); + D("file not found"); errno = ENOENT; return -1; case ERROR_PATH_NOT_FOUND: - D( "path not found" ); + D("path not found"); errno = ENOTDIR; return -1; @@ -457,8 +453,8 @@ int adb_creat(const char* path, int mode) return -1; } } - snprintf( f->name, sizeof(f->name), "%d(%s)", _fh_to_int(f), path ); - D( "adb_creat: '%s' => fd %d", path, _fh_to_int(f) ); + snprintf(f->name, sizeof(f->name), "%d(%s)", _fh_to_int(f), path); + D("adb_creat: '%s' => fd %d", path, _fh_to_int(f)); return _fh_to_int(f); } @@ -493,9 +489,8 @@ ssize_t adb_writev(int fd, const adb_iovec* iov, int iovcnt) { return f->clazz->_fh_writev(f, iov, iovcnt); } -int adb_lseek(int fd, int pos, int where) -{ - FH f = _fh_from_int(fd, __func__); +int adb_lseek(int fd, int pos, int where) { + FH f = _fh_from_int(fd, __func__); if (!f) { return -1; @@ -504,16 +499,14 @@ int adb_lseek(int fd, int pos, int where) return f->clazz->_fh_lseek(f, pos, where); } - -int adb_close(int fd) -{ - FH f = _fh_from_int(fd, __func__); +int adb_close(int fd) { + FH f = _fh_from_int(fd, __func__); if (!f) { return -1; } - D( "adb_close: %s", f->name); + D("adb_close: %s", f->name); _fh_close(f); return 0; } @@ -987,52 +980,49 @@ int network_connect(const std::string& host, int port, int type, int timeout, st return fd; } -int adb_register_socket(SOCKET s) { - FH f = _fh_alloc( &_fh_socket_class ); +int adb_register_socket(SOCKET s) { + FH f = _fh_alloc(&_fh_socket_class); f->fh_socket = s; return _fh_to_int(f); } #undef accept -int adb_socket_accept(int serverfd, struct sockaddr* addr, socklen_t *addrlen) -{ - FH serverfh = _fh_from_int(serverfd, __func__); +int adb_socket_accept(int serverfd, struct sockaddr* addr, socklen_t* addrlen) { + FH serverfh = _fh_from_int(serverfd, __func__); - if ( !serverfh || serverfh->clazz != &_fh_socket_class ) { + if (!serverfh || serverfh->clazz != &_fh_socket_class) { D("adb_socket_accept: invalid fd %d", serverfd); errno = EBADF; return -1; } - unique_fh fh(_fh_alloc( &_fh_socket_class )); + unique_fh fh(_fh_alloc(&_fh_socket_class)); if (!fh) { PLOG(ERROR) << "adb_socket_accept: failed to allocate accepted socket " "descriptor"; return -1; } - fh->fh_socket = accept( serverfh->fh_socket, addr, addrlen ); + fh->fh_socket = accept(serverfh->fh_socket, addr, addrlen); if (fh->fh_socket == INVALID_SOCKET) { const DWORD err = WSAGetLastError(); - LOG(ERROR) << "adb_socket_accept: accept on fd " << serverfd << - " failed: " + android::base::SystemErrorCodeToString(err); - _socket_set_errno( err ); + LOG(ERROR) << "adb_socket_accept: accept on fd " << serverfd + << " failed: " + android::base::SystemErrorCodeToString(err); + _socket_set_errno(err); return -1; } const int fd = _fh_to_int(fh.get()); - snprintf( fh->name, sizeof(fh->name), "%d(accept:%s)", fd, serverfh->name ); - D( "adb_socket_accept on fd %d returns fd %d", serverfd, fd ); + snprintf(fh->name, sizeof(fh->name), "%d(accept:%s)", fd, serverfh->name); + D("adb_socket_accept on fd %d returns fd %d", serverfd, fd); fh.release(); - return fd; + return fd; } +int adb_setsockopt(int fd, int level, int optname, const void* optval, socklen_t optlen) { + FH fh = _fh_from_int(fd, __func__); -int adb_setsockopt( int fd, int level, int optname, const void* optval, socklen_t optlen ) -{ - FH fh = _fh_from_int(fd, __func__); - - if ( !fh || fh->clazz != &_fh_socket_class ) { + if (!fh || fh->clazz != &_fh_socket_class) { D("adb_setsockopt: invalid fd %d", fd); errno = EBADF; return -1; @@ -1042,13 +1032,13 @@ int adb_setsockopt( int fd, int level, int optname, const void* optval, soc // to set SOL_SOCKET, SO_SNDBUF/SO_RCVBUF, ignore it since the OS has // auto-tuning. - int result = setsockopt( fh->fh_socket, level, optname, - reinterpret_cast(optval), optlen ); - if ( result == SOCKET_ERROR ) { + int result = + setsockopt(fh->fh_socket, level, optname, reinterpret_cast(optval), optlen); + if (result == SOCKET_ERROR) { const DWORD err = WSAGetLastError(); - D("adb_setsockopt: setsockopt on fd %d level %d optname %d failed: %s\n", - fd, level, optname, android::base::SystemErrorCodeToString(err).c_str()); - _socket_set_errno( err ); + D("adb_setsockopt: setsockopt on fd %d level %d optname %d failed: %s\n", fd, level, + optname, android::base::SystemErrorCodeToString(err).c_str()); + _socket_set_errno(err); result = -1; } return result; From 011ba4b9bf9f0b1b1f99639f8395a76760f717c0 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Thu, 5 Apr 2018 18:09:39 -0700 Subject: [PATCH 4/5] adb: win32: properly set EBADF in some functions. Test: treehugger Change-Id: If3f29f9ee586e29652e9709b3f594a1376ed4bb3 --- adb/sysdeps_win32.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/adb/sysdeps_win32.cpp b/adb/sysdeps_win32.cpp index b5334b141..caad50bf3 100644 --- a/adb/sysdeps_win32.cpp +++ b/adb/sysdeps_win32.cpp @@ -462,6 +462,7 @@ int adb_read(int fd, void* buf, int len) { FH f = _fh_from_int(fd, __func__); if (f == NULL) { + errno = EBADF; return -1; } @@ -472,6 +473,7 @@ int adb_write(int fd, const void* buf, int len) { FH f = _fh_from_int(fd, __func__); if (f == NULL) { + errno = EBADF; return -1; } @@ -493,6 +495,7 @@ int adb_lseek(int fd, int pos, int where) { FH f = _fh_from_int(fd, __func__); if (!f) { + errno = EBADF; return -1; } @@ -503,6 +506,7 @@ int adb_close(int fd) { FH f = _fh_from_int(fd, __func__); if (!f) { + errno = EBADF; return -1; } From 2e93df2e1446abb250472f2d39b20e00bba97dcc Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Thu, 5 Apr 2018 18:10:03 -0700 Subject: [PATCH 5/5] adb: win32: cleanup winsock initialization. Instead of doing it in 3 arbitrary functions, do it at startup always. Test: wine adb_test.exe Change-Id: Ida272d218aee6c331471250edce64d512d3b628a --- adb/sysdeps_win32.cpp | 29 +++++++++-------------------- 1 file changed, 9 insertions(+), 20 deletions(-) diff --git a/adb/sysdeps_win32.cpp b/adb/sysdeps_win32.cpp index caad50bf3..bfac34208 100644 --- a/adb/sysdeps_win32.cpp +++ b/adb/sysdeps_win32.cpp @@ -726,23 +726,15 @@ static int _fh_socket_writev(FH f, const adb_iovec* iov, int iovcnt) { /**************************************************************************/ /**************************************************************************/ -#include - -static int _winsock_init; - -static void -_init_winsock( void ) -{ - // TODO: Multiple threads calling this may potentially cause multiple calls - // to WSAStartup() which offers no real benefit. - if (!_winsock_init) { - WSADATA wsaData; - int rc = WSAStartup( MAKEWORD(2,2), &wsaData); +static int _init_winsock(void) { + static std::once_flag once; + std::call_once(once, []() { + WSADATA wsaData; + int rc = WSAStartup(MAKEWORD(2, 2), &wsaData); if (rc != 0) { fatal("adb: could not initialize Winsock: %s", android::base::SystemErrorCodeToString(rc).c_str()); } - _winsock_init = 1; // Note that we do not call atexit() to register WSACleanup to be called // at normal process termination because: @@ -757,9 +749,12 @@ _init_winsock( void ) // setupapi.dll which tries to load wintrust.dll which tries to load // crypt32.dll which calls atexit() which tries to acquire the C // Runtime lock that the other thread holds. - } + }); + return 0; } +static int _winsock_init = _init_winsock(); + // Map a socket type to an explicit socket protocol instead of using the socket // protocol of 0. Explicit socket protocols are used by most apps and we should // do the same to reduce the chance of exercising uncommon code-paths that might @@ -787,8 +782,6 @@ int network_loopback_client(int port, int type, std::string* error) { return -1; } - if (!_winsock_init) _init_winsock(); - memset(&addr, 0, sizeof(addr)); addr.sin_family = AF_INET; addr.sin_port = htons(port); @@ -837,8 +830,6 @@ static int _network_server(int port, int type, u_long interface_address, std::st return -1; } - if (!_winsock_init) _init_winsock(); - memset(&addr, 0, sizeof(addr)); addr.sin_family = AF_INET; addr.sin_port = htons(port); @@ -915,8 +906,6 @@ int network_connect(const std::string& host, int port, int type, int timeout, st return -1; } - if (!_winsock_init) _init_winsock(); - struct addrinfo hints; memset(&hints, 0, sizeof(hints)); hints.ai_family = AF_UNSPEC;