MTP add/remove storage instead of disabling MTP
Implement a pipe between TWRP and MTP to allow TWRP to tell MTP
to remove storage partitions as they become unavailable (e.g.
during a wipe, unmount, etc) instead of disabling MTP completely.
This includes some fixes and improvements in destructors to
properly remove / delete various items. This also means that we
will not be toggling adb off and on quite as often.
I do not like that we had to add another thread, but we were
unable to use select() on the mtp_usb character device because
this device does not support polling. Select always returned
indicating that the mtp file descriptor was ready to be read and
the resulting read would block. The read block prevented us from
being able to include reading of the pipe between TWRP and MTP in
the main MTP thread.
We might want to add a return pipe letting TWRP know if the
removal of the storage device was successful, but I am not sure
how we want to implement this. It would invovle timeouts in both
TWRP and MTP to ensure that we returned a failure indicator in a
timely manner to TWRP and prevent deleting the storage device in
the case of a failure. Right now we make no attempt to ensure that
an MTP operation is underway like a large file transfer, but we
were not doing anything like this in the past. In some respects we
have limited control over what happens. If the user installs a
zip that unmounts a storage partition, we will not know about the
change in storage status anyway. Regular Android does not have
these troubles because partitions rarely get unmounted like in
recovery. At some point, we have to hold the user accountable for
performing actions that may remove a storage partition while they
are using MTP anyway.
Ideally we do not want to toggle the USB IDs and thus toggle adb
off and on during early boot, but I am not sure what the best way
to handle that at this time.
Change-Id: I9343e5396bf6023d3b994de1bf01ed91d129bc14
diff --git a/mtp/MtpStorage.cpp b/mtp/MtpStorage.cpp
index 319be09..ab4f8e0 100755
--- a/mtp/MtpStorage.cpp
+++ b/mtp/MtpStorage.cpp
@@ -36,6 +36,7 @@
#include <signal.h>
#include <sys/inotify.h>
#include <fcntl.h>
+#include "tw_sys_atomics.h"
#define WATCH_FLAGS ( IN_CREATE | IN_DELETE | IN_MOVE | IN_MODIFY )
@@ -54,6 +55,8 @@
MTPI("MtpStorage id: %d path: %s\n", id, filePath);
inotify_thread = 0;
inotify_fd = -1;
+ // Threading has not started yet so we should be safe to set these directly instead of using atomics
+ inotify_thread_kill = 0;
sendEvents = false;
handleCurrentlySending = 0;
use_mutex = true;
@@ -63,25 +66,32 @@
}
if (pthread_mutex_init(&inMutex, NULL) != 0) {
MTPE("Failed to init inMutex\n");
+ pthread_mutex_destroy(&mtpMutex);
use_mutex = false;
}
-
}
MtpStorage::~MtpStorage() {
if (inotify_thread) {
- // TODO: what does this do? manpage says it does not kill the thread
- pthread_kill(inotify_thread, 0);
+ __tw_atomic_cmpxchg(0, 1, &inotify_thread_kill);
+ //inotify_thread_kill = 1;
+ MTPD("joining inotify_thread after sending the kill notification.\n");
+ pthread_join(inotify_thread, NULL); // There's not much we can do if there's an error here
+ inotify_thread = 0;
+ MTPD("~MtpStorage removing inotify watches and closing inotify_fd\n");
for (std::map<int, Tree*>::iterator i = inotifymap.begin(); i != inotifymap.end(); i++) {
inotify_rm_watch(inotify_fd, i->first);
}
close(inotify_fd);
+ inotifymap.clear();
}
- for (iter i = mtpmap.begin(); i != mtpmap.end(); i++) {
- delete i->second;
- }
+ // Deleting the root tree causes a cascade in btree.cpp that ends up
+ // deleting all of the trees and nodes.
+ delete mtpmap[0];
+ mtpmap.clear();
if (use_mutex) {
use_mutex = false;
+ MTPD("~MtpStorage destroying mutexes\n");
pthread_mutex_destroy(&mtpMutex);
pthread_mutex_destroy(&inMutex);
}
@@ -566,9 +576,22 @@
pthread_t MtpStorage::inotify(void) {
pthread_t thread;
+ pthread_attr_t tattr;
+
+ if (pthread_attr_init(&tattr)) {
+ MTPE("Unable to pthread_attr_init\n");
+ return 0;
+ }
+ if (pthread_attr_setdetachstate(&tattr, PTHREAD_CREATE_JOINABLE)) {
+ MTPE("Error setting pthread_attr_setdetachstate\n");
+ return 0;
+ }
ThreadPtr inotifyptr = &MtpStorage::inotify_t;
PThreadPtr p = *(PThreadPtr*)&inotifyptr;
- pthread_create(&thread, NULL, p, this);
+ pthread_create(&thread, &tattr, p, this);
+ if (pthread_attr_destroy(&tattr)) {
+ MTPE("Failed to pthread_attr_destroy\n");
+ }
return thread;
}
@@ -669,10 +692,20 @@
#define EVENT_SIZE ( sizeof(struct inotify_event) )
#define EVENT_BUF_LEN ( 1024 * ( EVENT_SIZE + 16) )
char buf[EVENT_BUF_LEN];
+ fd_set fdset;
+ struct timeval seltmout;
+ int sel_ret;
MTPD("inotify thread starting.\n");
- while (true) {
+ while (__tw_atomic_cmpxchg(0, inotify_thread_kill, &inotify_thread_kill) == 0) {
+ FD_ZERO(&fdset);
+ FD_SET(inotify_fd, &fdset);
+ seltmout.tv_sec = 0;
+ seltmout.tv_usec = 25000;
+ sel_ret = select(inotify_fd + 1, &fdset, NULL, NULL, &seltmout);
+ if (sel_ret == 0)
+ continue;
int i = 0;
int len = read(inotify_fd, buf, EVENT_BUF_LEN);
@@ -682,7 +715,7 @@
MTPE("inotify_t Can't read inotify events\n");
}
- while (i < len) {
+ while (i < len && __tw_atomic_cmpxchg(0, inotify_thread_kill, &inotify_thread_kill) == 0) {
struct inotify_event *event = (struct inotify_event *) &buf[i];
if (event->len) {
MTPD("inotify event: wd: %i, mask: %x, name: %s\n", event->wd, event->mask, event->name);
@@ -693,11 +726,12 @@
i += EVENT_SIZE + event->len;
}
}
-
- for (std::map<int, Tree*>::iterator i = inotifymap.begin(); i != inotifymap.end(); i++) {
+ MTPD("inotify_thread_kill received!\n");
+ // This cleanup is handled in the destructor.
+ /*for (std::map<int, Tree*>::iterator i = inotifymap.begin(); i != inotifymap.end(); i++) {
inotify_rm_watch(inotify_fd, i->first);
}
- close(inotify_fd);
+ close(inotify_fd);*/
return 0;
}