From 46e7cc14ad7678ef627b103e7fa107992353204f Mon Sep 17 00:00:00 2001 From: Siarhei Vishniakou Date: Mon, 9 Dec 2019 16:15:44 -0800 Subject: [PATCH 1/3] Use unique_fd for uhid device It is safer to use unique_fd because we don't have to worry about closing it. Bug: none Test: atest NintendoSwitchProTest Change-Id: I7bb0fb23ede65cf503f59a6e48cd708f0e99bba8 --- cmds/hid/jni/Android.bp | 1 + .../jni/com_android_commands_hid_Device.cpp | 33 +++++++++---------- .../hid/jni/com_android_commands_hid_Device.h | 11 ++++--- 3 files changed, 23 insertions(+), 22 deletions(-) diff --git a/cmds/hid/jni/Android.bp b/cmds/hid/jni/Android.bp index 095cfc6ceb53b..2c07de04b6a7e 100644 --- a/cmds/hid/jni/Android.bp +++ b/cmds/hid/jni/Android.bp @@ -5,6 +5,7 @@ cc_library_shared { shared_libs: [ "libandroid", + "libbase", "liblog", "libnativehelper", ], diff --git a/cmds/hid/jni/com_android_commands_hid_Device.cpp b/cmds/hid/jni/com_android_commands_hid_Device.cpp index d4fdf85491d32..fa75ef4350511 100644 --- a/cmds/hid/jni/com_android_commands_hid_Device.cpp +++ b/cmds/hid/jni/com_android_commands_hid_Device.cpp @@ -96,17 +96,17 @@ JNIEnv* DeviceCallback::getJNIEnv() { return env; } -Device* Device::open(int32_t id, const char* name, int32_t vid, int32_t pid, - std::vector descriptor, std::unique_ptr callback) { - +std::unique_ptr Device::open(int32_t id, const char* name, int32_t vid, int32_t pid, + const std::vector& descriptor, + std::unique_ptr callback) { size_t size = descriptor.size(); if (size > HID_MAX_DESCRIPTOR_SIZE) { LOGE("Received invalid hid report with descriptor size %zu, skipping", size); return nullptr; } - int fd = ::open(UHID_PATH, O_RDWR | O_CLOEXEC); - if (fd < 0) { + android::base::unique_fd fd(::open(UHID_PATH, O_RDWR | O_CLOEXEC)); + if (!fd.ok()) { LOGE("Failed to open uhid: %s", strerror(errno)); return nullptr; } @@ -114,8 +114,7 @@ Device* Device::open(int32_t id, const char* name, int32_t vid, int32_t pid, struct uhid_event ev = {}; ev.type = UHID_CREATE2; strlcpy(reinterpret_cast(ev.u.create2.name), name, sizeof(ev.u.create2.name)); - memcpy(&ev.u.create2.rd_data, descriptor.data(), - size * sizeof(ev.u.create2.rd_data[0])); + memcpy(&ev.u.create2.rd_data, descriptor.data(), size * sizeof(ev.u.create2.rd_data[0])); ev.u.create2.rd_size = size; ev.u.create2.bus = BUS_BLUETOOTH; ev.u.create2.vendor = vid; @@ -126,7 +125,6 @@ Device* Device::open(int32_t id, const char* name, int32_t vid, int32_t pid, errno = 0; ssize_t ret = TEMP_FAILURE_RETRY(::write(fd, &ev, sizeof(ev))); if (ret < 0 || ret != sizeof(ev)) { - ::close(fd); LOGE("Failed to create uhid node: %s", strerror(errno)); return nullptr; } @@ -134,21 +132,21 @@ Device* Device::open(int32_t id, const char* name, int32_t vid, int32_t pid, // Wait for the device to actually be created. ret = TEMP_FAILURE_RETRY(::read(fd, &ev, sizeof(ev))); if (ret < 0 || ev.type != UHID_START) { - ::close(fd); LOGE("uhid node failed to start: %s", strerror(errno)); return nullptr; } - return new Device(id, fd, std::move(callback)); + // using 'new' to access non-public constructor + return std::unique_ptr(new Device(id, std::move(fd), std::move(callback))); } -Device::Device(int32_t id, int fd, std::unique_ptr callback) : - mId(id), mFd(fd), mDeviceCallback(std::move(callback)) { +Device::Device(int32_t id, android::base::unique_fd fd, std::unique_ptr callback) + : mId(id), mFd(std::move(fd)), mDeviceCallback(std::move(callback)) { ALooper* aLooper = ALooper_forThread(); if (aLooper == NULL) { LOGE("Could not get ALooper, ALooper_forThread returned NULL"); aLooper = ALooper_prepare(ALOOPER_PREPARE_ALLOW_NON_CALLBACKS); } - ALooper_addFd(aLooper, fd, 0, ALOOPER_EVENT_INPUT, handleLooperEvents, + ALooper_addFd(aLooper, mFd, 0, ALOOPER_EVENT_INPUT, handleLooperEvents, reinterpret_cast(this)); } @@ -162,8 +160,6 @@ Device::~Device() { struct uhid_event ev = {}; ev.type = UHID_DESTROY; TEMP_FAILURE_RETRY(::write(mFd, &ev, sizeof(ev))); - ::close(mFd); - mFd = -1; } void Device::sendReport(const std::vector& report) const { @@ -250,9 +246,10 @@ static jlong openDevice(JNIEnv* env, jclass /* clazz */, jstring rawName, jint i std::unique_ptr cb(new uhid::DeviceCallback(env, callback)); - uhid::Device* d = uhid::Device::open( - id, reinterpret_cast(name.c_str()), vid, pid, desc, std::move(cb)); - return reinterpret_cast(d); + std::unique_ptr d = + uhid::Device::open(id, reinterpret_cast(name.c_str()), vid, pid, desc, + std::move(cb)); + return reinterpret_cast(d.release()); } static void sendReport(JNIEnv* env, jclass /* clazz */, jlong ptr, jbyteArray rawReport) { diff --git a/cmds/hid/jni/com_android_commands_hid_Device.h b/cmds/hid/jni/com_android_commands_hid_Device.h index 892c7cd12953b..b0471eda44c6f 100644 --- a/cmds/hid/jni/com_android_commands_hid_Device.h +++ b/cmds/hid/jni/com_android_commands_hid_Device.h @@ -19,6 +19,8 @@ #include +#include + namespace android { namespace uhid { @@ -39,10 +41,10 @@ private: class Device { public: - static Device* open(int32_t id, const char* name, int32_t vid, int32_t pid, - std::vector descriptor, std::unique_ptr callback); + static std::unique_ptr open(int32_t id, const char* name, int32_t vid, int32_t pid, + const std::vector& descriptor, + std::unique_ptr callback); - Device(int32_t id, int fd, std::unique_ptr callback); ~Device(); void sendReport(const std::vector& report) const; @@ -52,8 +54,9 @@ public: int handleEvents(int events); private: + Device(int32_t id, android::base::unique_fd fd, std::unique_ptr callback); int32_t mId; - int mFd; + android::base::unique_fd mFd; std::unique_ptr mDeviceCallback; }; From 9a02feacd280ac57ae3d1b4e9df5ba4190b0d0d1 Mon Sep 17 00:00:00 2001 From: Siarhei Vishniakou Date: Mon, 9 Dec 2019 16:46:23 -0800 Subject: [PATCH 2/3] Use a separate writeEvent function This function will also be used for handling UHID_OUTPUT. It also moves all error checking into a single place. Bug: none Test: NintendoSwitchProTest Change-Id: I72faa4bbfbe41842bbb651730dbd614c5bcbbaea --- .../jni/com_android_commands_hid_Device.cpp | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/cmds/hid/jni/com_android_commands_hid_Device.cpp b/cmds/hid/jni/com_android_commands_hid_Device.cpp index fa75ef4350511..d7bdda34bc54c 100644 --- a/cmds/hid/jni/com_android_commands_hid_Device.cpp +++ b/cmds/hid/jni/com_android_commands_hid_Device.cpp @@ -162,6 +162,14 @@ Device::~Device() { TEMP_FAILURE_RETRY(::write(mFd, &ev, sizeof(ev))); } +// Send event over the fd. +static void writeEvent(int fd, struct uhid_event& ev, const char* messageType) { + ssize_t ret = TEMP_FAILURE_RETRY(::write(fd, &ev, sizeof(ev))); + if (ret < 0 || ret != sizeof(ev)) { + LOGE("Failed to send uhid_event %s: %s", messageType, strerror(errno)); + } +} + void Device::sendReport(const std::vector& report) const { if (report.size() > UHID_DATA_MAX) { LOGE("Received invalid report of size %zu, skipping", report.size()); @@ -172,10 +180,7 @@ void Device::sendReport(const std::vector& report) const { ev.type = UHID_INPUT2; ev.u.input2.size = report.size(); memcpy(&ev.u.input2.data, report.data(), report.size() * sizeof(ev.u.input2.data[0])); - ssize_t ret = TEMP_FAILURE_RETRY(::write(mFd, &ev, sizeof(ev))); - if (ret < 0 || ret != sizeof(ev)) { - LOGE("Failed to send hid event: %s", strerror(errno)); - } + writeEvent(mFd, ev, "UHID_INPUT2"); } void Device::sendGetFeatureReportReply(uint32_t id, const std::vector& report) const { @@ -186,10 +191,7 @@ void Device::sendGetFeatureReportReply(uint32_t id, const std::vector& ev.u.get_report_reply.size = report.size(); memcpy(&ev.u.get_report_reply.data, report.data(), report.size() * sizeof(ev.u.get_report_reply.data[0])); - ssize_t ret = TEMP_FAILURE_RETRY(::write(mFd, &ev, sizeof(ev))); - if (ret < 0 || ret != sizeof(ev)) { - LOGE("Failed to send hid event (UHID_GET_REPORT_REPLY): %s", strerror(errno)); - } + writeEvent(mFd, ev, "UHID_GET_REPORT_REPLY"); } int Device::handleEvents(int events) { From c490e7acdeaf682275cb687684e82ac984ab4a61 Mon Sep 17 00:00:00 2001 From: Siarhei Vishniakou Date: Mon, 9 Dec 2019 16:58:29 -0800 Subject: [PATCH 3/3] Use switch statement for report types We are adding new report types, and it is easier to use switch statements to ensure that the default case is handled. Also, expand the diagnostic messaging for SET_REPORT type, because it is useful to see the actual data. Bug: none Test: atest NintendoSwitchProTest Change-Id: I330e3b1464ae8e35a57d448e460cdb55ebbf6260 --- .../jni/com_android_commands_hid_Device.cpp | 45 +++++++++++++++---- 1 file changed, 37 insertions(+), 8 deletions(-) diff --git a/cmds/hid/jni/com_android_commands_hid_Device.cpp b/cmds/hid/jni/com_android_commands_hid_Device.cpp index d7bdda34bc54c..f3871d74320b8 100644 --- a/cmds/hid/jni/com_android_commands_hid_Device.cpp +++ b/cmds/hid/jni/com_android_commands_hid_Device.cpp @@ -21,10 +21,11 @@ #include #include +#include +#include #include #include #include -#include #include #include @@ -33,6 +34,8 @@ #include #include +#include + #define LOGE(...) __android_log_print(ANDROID_LOG_ERROR,LOG_TAG,__VA_ARGS__) #define LOGW(...) __android_log_print(ANDROID_LOG_WARN,LOG_TAG,__VA_ARGS__) #define LOGD(...) __android_log_print(ANDROID_LOG_DEBUG,LOG_TAG,__VA_ARGS__) @@ -61,6 +64,14 @@ static void checkAndClearException(JNIEnv* env, const char* methodName) { } } +static std::string toString(const std::vector& data) { + std::string s = ""; + for (uint8_t b : data) { + s += android::base::StringPrintf("%x ", b); + } + return s; +} + DeviceCallback::DeviceCallback(JNIEnv* env, jobject callback) : mCallbackObject(env->NewGlobalRef(callback)) { env->GetJavaVM(&mJavaVM); @@ -208,13 +219,31 @@ int Device::handleEvents(int events) { return 0; } - if (ev.type == UHID_OPEN) { - mDeviceCallback->onDeviceOpen(); - } else if (ev.type == UHID_GET_REPORT) { - mDeviceCallback->onDeviceGetReport(ev.u.get_report.id, ev.u.get_report.rnum); - } else if (ev.type == UHID_SET_REPORT) { - LOGE("UHID_SET_REPORT is currently not supported"); - return 0; + switch (ev.type) { + case UHID_OPEN: { + mDeviceCallback->onDeviceOpen(); + break; + } + case UHID_GET_REPORT: { + mDeviceCallback->onDeviceGetReport(ev.u.get_report.id, ev.u.get_report.rnum); + break; + } + case UHID_SET_REPORT: { + const struct uhid_set_report_req& set_report = ev.u.set_report; + if (set_report.size > UHID_DATA_MAX) { + LOGE("SET_REPORT contains too much data: size = %" PRIu16, set_report.size); + return 0; + } + + std::vector data(set_report.data, set_report.data + set_report.size); + LOGI("Received SET_REPORT: id=%" PRIu32 " rnum=%" PRIu8 " data=%s", set_report.id, + set_report.rnum, toString(data).c_str()); + break; + } + default: { + LOGI("Unhandled event type: %" PRIu32, ev.type); + break; + } } return 1;