The reason behing this change is to increase the adb push/pull speed
with reduceing the number of packets sent between the host and the
device because the communication is heavily bound by packet latency.
The change maintains two way compatibility in the communication
protocol with negotiating a packet size between the target and the
host with the CONNECT packets.
After this change the push/pull speeds improved significantly
(measured from Linux-x86_64 with 100MB of data):
| Old push | Old pull || New push | New pull |
-----------------------------------------------------------
Hammerhead | 4.6 MB/s | 3.9 MB/s || 13.1 MB/s | 16.5 MB/s |
-----------------------------------------------------------
Volantis | 6.0 MB/s | 6.2 MS/s || 25.9 MB/s | 29.0 MB/s |
-----------------------------------------------------------
Fugu | 6.0 MB/s | 5.1 MB/s || 27.9 MB/s | 33.2 MB/s |
-----------------------------------------------------------
Change-Id: Id9625de31266e43394289e325c7e7e473379c5d8
I think this fixes a scary bug that could be on all host platforms.
When running 'adb unroot' with an emulator, the connection to the
emulator is dropped (as expected). I noticed that the adb.log showed:
_fh_from_int: 1168: 5280 | _fh_from_int: invalid fd 106 passed to adb_close
Background: Every transport has a socketpair (two bidirectional sockets
connected to each other to form one 'pipe') that are used as follows:
* When adb wants to write to a transport, it writes to
t->transport_socket (half of the socketpair). An input thread reads from
t->fd (the other half of the socketpair) and writes the data to the
underlying transport (TCP, USB).
* An output thread reads from the underlying transport (TCP, USB) and
writes the data to t->fd. The main thread runs fdevent_loop() which
reads from t->transport_socket and processes the packets (that really
came from the underlying transport).
So t->fd and t->transport_socket are just an intermediate pipe between
transport agnostic code in adb and the underlying transport (TCP, USB).
Here's what I think is going on:
1. When the TCP transport is closed (such as when running adb unroot),
adb server's output thread notices this (adb_read() returns zero), and
it writes a special packet to t->fd.
2. The main thread processes the special packet by writing the special
packet to the input thread.
3. input_thread() sees the special packet, so it breaks out of a read
loop and calls transport_unref() which calls transport_unref_locked().
4. transport_unref_locked() calls t->close() which is a function pointer
that points to transport_local.cpp: remote_close() which calls
adb_close(t->fd). <----- ****THIS IS THE BUG****
I think this is a (very old) typo and it should instead be
adb_close(t->sfd) (the transport’s actual TCP socket) because it does
not make sense for the particular transport mechanism (TCP, USB) to be
messing with a socket of the socketpair of the transport agnostic code
(t->fd).
5. transport_unref_locked() calls remove_transport() which writes an
action to another special socketpair.
6. The action is read and eventually transport_registration_func() is
called and it calls adb_close(t->fd). But t->fd was already
(erroneously) closed in #4 above!! Anyway, this causes the adb.log
output.
The fix is to fix the typo changing t->fd to t->sfd and adding some
resiliency around whether the socket has already been closed (probably
by remote_kick()).
I tested this by putting a new adbd on an emulator, a new adb on Linux
and Windows and running the adb unroot scenario and checking adb.log. I
also ran test_adb.py (which doesn't totally work without problems with
an emulator, but I'll leave that to another day.)
Change-Id: I188b6c74917a3d721c150fd17ed0f2b63a2178c3
Signed-off-by: Spencer Low <CompareAndSwap@gmail.com>
* sysdeps.h should always be included first.
* TRACE_TAG needs to be defined before anything is included.
* Some files were missing copyright headers.
* Save precious bytes on my SSD by removing useless whitespace.
Change-Id: I88980e6e00b5be1093806cf286740d9e4a033b94
I keep trying to clean things up and needing std::strings. Might as
well just do this now.
usb_linux_client.c is going to stay as C because GCC isn't smart
enough to deal with the designated initializers it uses (though for
some reason it is in C mode).
The Darwin files are staying as C because I don't have a way to test
that they build.
The Windows files are staying as C because while I can actually build
for them, it's slow and painful.
Change-Id: I75367d29205a9049d34460032b3bb36384f43941
Elliott Hughes
Tamas Berghammer
Spencer Low
Dan Albert