From 726a0206326ca4cf22aa7c59e174a83a0da9727f Mon Sep 17 00:00:00 2001 From: Ethan Yonker Date: Tue, 16 Dec 2014 20:01:38 -0600 Subject: 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 --- mtp/MtpDatabase.h | 1 + mtp/MtpMessage.hpp | 33 ++++++++++++++++++++++++ mtp/MtpServer.cpp | 29 +++++++++++++++++++-- mtp/MtpStorage.cpp | 58 +++++++++++++++++++++++++++++++++--------- mtp/MtpStorage.h | 1 + mtp/btree.cpp | 1 + mtp/mtp_MtpDatabase.cpp | 7 ++++++ mtp/mtp_MtpDatabase.hpp | 1 + mtp/mtp_MtpServer.cpp | 52 +++++++++++++++++++++++++++++++++++++- mtp/mtp_MtpServer.hpp | 5 ++++ mtp/tw_sys_atomics.h | 67 +++++++++++++++++++++++++++++++++++++++++++++++++ mtp/twrpMtp.cpp | 7 +++++- mtp/twrpMtp.hpp | 3 ++- 13 files changed, 248 insertions(+), 17 deletions(-) create mode 100644 mtp/MtpMessage.hpp create mode 100644 mtp/tw_sys_atomics.h (limited to 'mtp') diff --git a/mtp/MtpDatabase.h b/mtp/MtpDatabase.h index c25e9b24c..a0ff8dace 100755 --- a/mtp/MtpDatabase.h +++ b/mtp/MtpDatabase.h @@ -61,6 +61,7 @@ public: virtual MtpDevicePropertyList* getSupportedDeviceProperties() = 0; virtual void createDB(MtpStorage* storage, MtpStorageID storageID) = 0; + virtual void destroyDB(MtpStorageID storageID) = 0; virtual MtpResponseCode getObjectPropertyValue(MtpObjectHandle handle, MtpObjectProperty property, diff --git a/mtp/MtpMessage.hpp b/mtp/MtpMessage.hpp new file mode 100644 index 000000000..272da1743 --- /dev/null +++ b/mtp/MtpMessage.hpp @@ -0,0 +1,33 @@ +/* + * Copyright (C) 2010 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. + * + * Copyright (C) 2014 TeamWin - bigbiff and Dees_Troy mtp database conversion to C++ + */ + +#ifndef _MTPMESSAGE_HPP +#define _MTPMESSAGE_HPP + +#define MTP_MESSAGE_ADD_STORAGE 1 +#define MTP_MESSAGE_REMOVE_STORAGE 2 + +struct mtpmsg { + int message_type; // 1 is add, 2 is remove, see above + unsigned int storage_id; + const char* display; + const char* path; + uint64_t maxFileSize; +}; + +#endif //_MTPMESSAGE_HPP diff --git a/mtp/MtpServer.cpp b/mtp/MtpServer.cpp index f4af2b948..f99554b03 100755 --- a/mtp/MtpServer.cpp +++ b/mtp/MtpServer.cpp @@ -116,9 +116,13 @@ MtpServer::~MtpServer() { } void MtpServer::addStorage(MtpStorage* storage) { + android::Mutex::Autolock autoLock(mMutex); MTPD("addStorage(): storage: %x\n", storage); + if (getStorage(storage->getStorageID()) != NULL) { + MTPE("MtpServer::addStorage Storage for storage ID %i already exists.\n", storage->getStorageID()); + return; + } mDatabase->createDB(storage, storage->getStorageID()); - android::Mutex::Autolock autoLock(mMutex); mStorages.push(storage); sendStoreAdded(storage->getStorageID()); } @@ -128,11 +132,31 @@ void MtpServer::removeStorage(MtpStorage* storage) { for (size_t i = 0; i < mStorages.size(); i++) { if (mStorages[i] == storage) { + MTPD("MtpServer::removeStorage calling sendStoreRemoved\n"); + // First lock the mutex so that the inotify thread and main + // thread do not do anything while we remove the storage + // item, and to make sure we don't remove the item while an + // operation is in progress + mDatabase->lockMutex(); + // Grab the storage ID before we delete the item from the + // database + MtpStorageID storageID = storage->getStorageID(); + // Remove the item from the mStorages from the vector. At + // this point the main thread will no longer be able to find + // this storage item anymore. mStorages.removeAt(i); - sendStoreRemoved(storage->getStorageID()); + // Destroy the storage item, free up all the memory, kill + // the inotify thread. + mDatabase->destroyDB(storageID); + // Tell the host OS that the storage item is gone. + sendStoreRemoved(storageID); + // Unlock any remaining mutexes on other storage devices. + // If no storage devices exist anymore this will do nothing. + mDatabase->unlockMutex(); break; } } + MTPD("MtpServer::removeStorage DONE\n"); } MtpStorage* MtpServer::getStorage(MtpStorageID id) { @@ -275,6 +299,7 @@ void MtpServer::sendStoreAdded(MtpStorageID id) { void MtpServer::sendStoreRemoved(MtpStorageID id) { MTPD("sendStoreRemoved %08X\n", id); sendEvent(MTP_EVENT_STORE_REMOVED, id); + MTPD("MtpServer::sendStoreRemoved done\n"); } void MtpServer::sendEvent(MtpEventCode code, uint32_t param1) { diff --git a/mtp/MtpStorage.cpp b/mtp/MtpStorage.cpp index 319be094b..ab4f8e044 100755 --- a/mtp/MtpStorage.cpp +++ b/mtp/MtpStorage.cpp @@ -36,6 +36,7 @@ #include #include #include +#include "tw_sys_atomics.h" #define WATCH_FLAGS ( IN_CREATE | IN_DELETE | IN_MOVE | IN_MODIFY ) @@ -54,6 +55,8 @@ MtpStorage::MtpStorage(MtpStorageID id, const char* filePath, 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 @@ MtpStorage::MtpStorage(MtpStorageID id, const char* filePath, } 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::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 @@ int MtpStorage::getObjectPropertyValue(MtpObjectHandle handle, MtpObjectProperty 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 @@ int MtpStorage::inotify_t(void) { #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 @@ int MtpStorage::inotify_t(void) { 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 @@ int MtpStorage::inotify_t(void) { i += EVENT_SIZE + event->len; } } - - for (std::map::iterator i = inotifymap.begin(); i != inotifymap.end(); i++) { + MTPD("inotify_thread_kill received!\n"); + // This cleanup is handled in the destructor. + /*for (std::map::iterator i = inotifymap.begin(); i != inotifymap.end(); i++) { inotify_rm_watch(inotify_fd, i->first); } - close(inotify_fd); + close(inotify_fd);*/ return 0; } diff --git a/mtp/MtpStorage.h b/mtp/MtpStorage.h index 418e3db8c..cdbb73b50 100755 --- a/mtp/MtpStorage.h +++ b/mtp/MtpStorage.h @@ -113,6 +113,7 @@ private: bool use_mutex; pthread_mutex_t inMutex; // inotify mutex pthread_mutex_t mtpMutex; // main mtp mutex + int inotify_thread_kill; }; #endif // _MTP_STORAGE_H diff --git a/mtp/btree.cpp b/mtp/btree.cpp index 3a5648db0..e53afab98 100755 --- a/mtp/btree.cpp +++ b/mtp/btree.cpp @@ -28,6 +28,7 @@ Tree::Tree(MtpObjectHandle handle, MtpObjectHandle parent, const std::string& na Tree::~Tree() { for (std::map::iterator it = entries.begin(); it != entries.end(); ++it) delete it->second; + entries.clear(); } int Tree::getCount(void) { diff --git a/mtp/mtp_MtpDatabase.cpp b/mtp/mtp_MtpDatabase.cpp index 05bb5d9fb..17053f1de 100755 --- a/mtp/mtp_MtpDatabase.cpp +++ b/mtp/mtp_MtpDatabase.cpp @@ -233,10 +233,17 @@ void MyMtpDatabase::endSendObject(const char* path, MtpObjectHandle handle, } void MyMtpDatabase::createDB(MtpStorage* storage, MtpStorageID storageID) { + MTPD("MyMtpDatabase::createDB called\n"); storagemap[storageID] = storage; storage->createDB(); } +void MyMtpDatabase::destroyDB(MtpStorageID storageID) { + MtpStorage* storage = storagemap[storageID]; + storagemap.erase(storageID); + delete storage; +} + MtpObjectHandleList* MyMtpDatabase::getObjectList(MtpStorageID storageID, MtpObjectFormat format, MtpObjectHandle parent) { diff --git a/mtp/mtp_MtpDatabase.hpp b/mtp/mtp_MtpDatabase.hpp index cc8097be2..49e59135a 100755 --- a/mtp/mtp_MtpDatabase.hpp +++ b/mtp/mtp_MtpDatabase.hpp @@ -64,6 +64,7 @@ public: virtual ~MyMtpDatabase(); void createDB(MtpStorage* storage, MtpStorageID storageID); + void destroyDB(MtpStorageID storageID); virtual MtpObjectHandle beginSendObject(const char* path, MtpObjectFormat format, MtpObjectHandle parent, diff --git a/mtp/mtp_MtpServer.cpp b/mtp/mtp_MtpServer.cpp index 17facdd70..f49270fdf 100755 --- a/mtp/mtp_MtpServer.cpp +++ b/mtp/mtp_MtpServer.cpp @@ -25,11 +25,13 @@ #include #include #include +#include #include "mtp_MtpServer.hpp" #include "MtpServer.h" #include "MtpStorage.h" #include "MtpDebug.h" +#include "MtpMessage.hpp" #include @@ -37,6 +39,11 @@ void twmtp_MtpServer::start() { if (setup() == 0) { add_storage(); + MTPD("Starting add / remove mtppipe monitor thread\n"); + pthread_t thread; + ThreadPtr mtpptr = &twmtp_MtpServer::mtppipe_thread; + PThreadPtr p = *(PThreadPtr*)&mtpptr; + pthread_create(&thread, NULL, p, this); server->run(); } } @@ -140,9 +147,52 @@ void twmtp_MtpServer::remove_storage(int storageId) if (server) { MtpStorage* storage = server->getStorage(storageId); if (storage) { + MTPD("twmtp_MtpServer::remove_storage calling removeStorage\n"); server->removeStorage(storage); - delete storage; } } else MTPD("server is null in remove_storage"); + MTPD("twmtp_MtpServer::remove_storage DONE\n"); +} + +int twmtp_MtpServer::mtppipe_thread(void) +{ + if (mtp_read_pipe == -1) { + MTPD("mtppipe_thread exiting because mtp_read_pipe not set\n"); + return 0; + } + MTPD("Starting twmtp_MtpServer::mtppipe_thread\n"); + int read_count; + struct mtpmsg mtp_message; + while (1) { + read_count = ::read(mtp_read_pipe, &mtp_message, sizeof(mtp_message)); + MTPD("read %i from mtppipe\n", read_count); + if (read_count == sizeof(mtp_message)) { + if (mtp_message.message_type == MTP_MESSAGE_ADD_STORAGE) { + MTPI("mtppipe add storage %i '%s'\n", mtp_message.storage_id, mtp_message.path); + long reserveSpace = 1; + bool removable = false; + MtpStorage* storage = new MtpStorage(mtp_message.storage_id, mtp_message.path, mtp_message.display, reserveSpace, removable, mtp_message.maxFileSize, refserver); + server->addStorage(storage); + MTPD("mtppipe done adding storage\n"); + } else if (mtp_message.message_type == MTP_MESSAGE_REMOVE_STORAGE) { + MTPI("mtppipe remove storage %i\n", mtp_message.storage_id); + remove_storage(mtp_message.storage_id); + MTPD("mtppipe done removing storage\n"); + } else { + MTPE("Unknown mtppipe message value: %i\n", mtp_message.message_type); + } + } else { + MTPE("twmtp_MtpServer::mtppipe_thread unexpected read_count %i\n", read_count); + close(mtp_read_pipe); + break; + } + } + MTPD("twmtp_MtpServer::mtppipe_thread closing\n"); + return 0; +} + +void twmtp_MtpServer::set_read_pipe(int pipe) +{ + mtp_read_pipe = pipe; } diff --git a/mtp/mtp_MtpServer.hpp b/mtp/mtp_MtpServer.hpp index 3153e80bf..622ed6d6f 100755 --- a/mtp/mtp_MtpServer.hpp +++ b/mtp/mtp_MtpServer.hpp @@ -52,11 +52,16 @@ class twmtp_MtpServer { void add_storage(); void remove_storage(int storageId); void set_storages(storages* mtpstorages); + void set_read_pipe(int pipe); storages *stores; private: + typedef int (twmtp_MtpServer::*ThreadPtr)(void); + typedef void* (*PThreadPtr)(void *); + int mtppipe_thread(void); bool usePtp; MtpServer* server; MtpServer* refserver; + int mtp_read_pipe; }; #endif diff --git a/mtp/tw_sys_atomics.h b/mtp/tw_sys_atomics.h new file mode 100644 index 000000000..6349a931d --- /dev/null +++ b/mtp/tw_sys_atomics.h @@ -0,0 +1,67 @@ +/* + * Copyright (C) 2008 The Android Open Source Project + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions + * are met: + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * * Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in + * the documentation and/or other materials provided with the + * distribution. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS + * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE + * COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, + * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, + * BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS + * OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED + * AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, + * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT + * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF + * SUCH DAMAGE. + */ +#ifndef _TW_SYS_ATOMICS_H +#define _TW_SYS_ATOMICS_H + +#include +#include + +__BEGIN_DECLS + +/* Note: atomic operations that were exported by the C library didn't + * provide any memory barriers, which created potential issues on + * multi-core devices. We now define them as inlined calls to + * GCC sync builtins, which always provide a full barrier. + * + * NOTE: The C library still exports atomic functions by the same + * name to ensure ABI stability for existing NDK machine code. + * + * If you are an NDK developer, we encourage you to rebuild your + * unmodified sources against this header as soon as possible. + */ + +/* This was kanged from Android 4.4 bionic/libc/include/sys/atomics.h + * This header was removed in Android 5.0 in favor of stdatomics.h but + * to maintain compatibility across multiple trees, we are including our + * own copy. + */ + +#ifndef __ATOMIC_INLINE__ +#define __ATOMIC_INLINE__ static __inline__ __attribute__((always_inline)) +#endif + +__ATOMIC_INLINE__ int +__tw_atomic_cmpxchg(int old_value, int new_value, volatile int* ptr) +{ + /* We must return 0 on success */ + return __sync_val_compare_and_swap(ptr, old_value, new_value) != old_value; +} + +__END_DECLS + +#endif /* _TW_SYS_ATOMICS_H */ diff --git a/mtp/twrpMtp.cpp b/mtp/twrpMtp.cpp index d9db4246e..d47b8fa0d 100755 --- a/mtp/twrpMtp.cpp +++ b/mtp/twrpMtp.cpp @@ -72,12 +72,14 @@ twrpMtp::twrpMtp(int debug_enabled = 0) { if (debug_enabled) MtpDebug::enableDebug(); mtpstorages = new storages; + mtp_read_pipe = -1; } int twrpMtp::start(void) { MTPI("Starting MTP\n"); twmtp_MtpServer *mtp = new twmtp_MtpServer(); mtp->set_storages(mtpstorages); + mtp->set_read_pipe(mtp_read_pipe); mtp->start(); return 0; } @@ -90,7 +92,7 @@ pthread_t twrpMtp::threadserver(void) { return thread; } -pid_t twrpMtp::forkserver(void) { +pid_t twrpMtp::forkserver(int mtppipe[2]) { pid_t pid; if ((pid = fork()) == -1) { MTPE("MTP fork failed.\n"); @@ -98,8 +100,11 @@ pid_t twrpMtp::forkserver(void) { } if (pid == 0) { // Child process + close(mtppipe[1]); // Child closes write side + mtp_read_pipe = mtppipe[0]; start(); MTPD("MTP child process exited.\n"); + close(mtppipe[0]); _exit(0); } else { return pid; diff --git a/mtp/twrpMtp.hpp b/mtp/twrpMtp.hpp index 3aaa96414..ec7cd4b59 100755 --- a/mtp/twrpMtp.hpp +++ b/mtp/twrpMtp.hpp @@ -35,7 +35,7 @@ class twrpMtp { public: twrpMtp(int debug_enabled /* = 0 */); pthread_t threadserver(void); - pid_t forkserver(void); + pid_t forkserver(int mtppipe[2]); void addStorage(std::string display, std::string path, int mtpid, uint64_t maxFileSize); private: int start(void); @@ -43,5 +43,6 @@ class twrpMtp { typedef void* (*PThreadPtr)(void *); storages *mtpstorages; storage *s; + int mtp_read_pipe; }; #endif -- cgit v1.2.3