From ff87855e10423a1ad6f764f378b8182f86c6f738 Mon Sep 17 00:00:00 2001 From: bohu Date: Tue, 28 Feb 2017 14:58:36 -0800 Subject: [PATCH 1/3] Qemu-pipe: refactor qemu_pipe.h into libqemu_pipe Traditionally, qemu_pipe has both the declaration and implentation of each function in one header file--qemu_pipe.h, and it is getting incovenient to maintain. This CL separates the implementation of functions from the header file, and makes qemu_pipe a static library for other modules to link to. Note that the interface and implementation of qemu_pipe are kept unchanged, and future CLs will enhance the implementation to make it more reliable and more compatible with old and new API levels. Following projects are affected by this refactoring, and they are modified accordingly: hardware/ril/reference-ril Change-Id: I541ecbf0cc7eadeef9d4e37ffd9ca7bfcc5c94c0 (cherry picked from aosp 294d44be33bf6ad6d7d53189d38202a4911f2bd7) --- adb/Android.mk | 3 +- adb/transport_local.cpp | 2 +- qemu_pipe/Android.mk | 19 ++++++ qemu_pipe/include/qemu_pipe.h | 62 +++++++++++++++++++ .../qemu_pipe.h => qemu_pipe/qemu_pipe.cpp | 16 +++-- 5 files changed, 91 insertions(+), 11 deletions(-) create mode 100644 qemu_pipe/Android.mk create mode 100644 qemu_pipe/include/qemu_pipe.h rename include/system/qemu_pipe.h => qemu_pipe/qemu_pipe.cpp (91%) diff --git a/adb/Android.mk b/adb/Android.mk index e84120542..a05bb551e 100644 --- a/adb/Android.mk +++ b/adb/Android.mk @@ -128,7 +128,7 @@ LOCAL_SANITIZE := $(adb_target_sanitize) # Even though we're building a static library (and thus there's no link step for # this to take effect), this adds the includes to our path. -LOCAL_STATIC_LIBRARIES := libcrypto_utils libcrypto libbase +LOCAL_STATIC_LIBRARIES := libcrypto_utils libcrypto libqemu_pipe libbase LOCAL_WHOLE_STATIC_LIBRARIES := libadbd_usb @@ -361,6 +361,7 @@ LOCAL_STRIP_MODULE := keep_symbols LOCAL_STATIC_LIBRARIES := \ libadbd \ libbase \ + libqemu_pipe \ libbootloader_message \ libfs_mgr \ libfec \ diff --git a/adb/transport_local.cpp b/adb/transport_local.cpp index 4198a5247..12b98ba3b 100644 --- a/adb/transport_local.cpp +++ b/adb/transport_local.cpp @@ -289,7 +289,7 @@ static void server_socket_thread(void* arg) { #define open adb_open #define read adb_read #define write adb_write -#include +#include #undef open #undef read #undef write diff --git a/qemu_pipe/Android.mk b/qemu_pipe/Android.mk new file mode 100644 index 000000000..6e0144ce1 --- /dev/null +++ b/qemu_pipe/Android.mk @@ -0,0 +1,19 @@ +# Copyright 2011 The Android Open Source Project + +LOCAL_PATH:= $(call my-dir) + +common_static_libraries := \ + libbase +include $(CLEAR_VARS) +LOCAL_CLANG := true +LOCAL_SANITIZE := integer +LOCAL_SRC_FILES:= \ + qemu_pipe.cpp +LOCAL_C_INCLUDES := \ + $(LOCAL_PATH)/include \ + system/base/include +LOCAL_MODULE:= libqemu_pipe +LOCAL_STATIC_LIBRARIES := $(common_static_libraries) +LOCAL_EXPORT_C_INCLUDE_DIRS := $(LOCAL_PATH)/include +LOCAL_CFLAGS := -Werror +include $(BUILD_STATIC_LIBRARY) diff --git a/qemu_pipe/include/qemu_pipe.h b/qemu_pipe/include/qemu_pipe.h new file mode 100644 index 000000000..16486c087 --- /dev/null +++ b/qemu_pipe/include/qemu_pipe.h @@ -0,0 +1,62 @@ +/* + * Copyright (C) 2011 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. + */ +#ifndef ANDROID_CORE_INCLUDE_QEMU_PIPE_H +#define ANDROID_CORE_INCLUDE_QEMU_PIPE_H + +#include + +#ifdef __cplusplus +extern "C" { +#endif +// Try to open a new Qemu fast-pipe. This function returns a file descriptor +// that can be used to communicate with a named service managed by the +// emulator. +// +// This file descriptor can be used as a standard pipe/socket descriptor. +// +// 'pipeName' is the name of the emulator service you want to connect to, +// and must begin with 'pipe:' (e.g. 'pipe:camera' or 'pipe:opengles'). +// +// On success, return a valid file descriptor, or -1/errno on failure. E.g.: +// +// EINVAL -> unknown/unsupported pipeName +// ENOSYS -> fast pipes not available in this system. +// +// ENOSYS should never happen, except if you're trying to run within a +// misconfigured emulator. +// +// You should be able to open several pipes to the same pipe service, +// except for a few special cases (e.g. GSM modem), where EBUSY will be +// returned if more than one client tries to connect to it. +int qemu_pipe_open(const char* pipeName); + +// Send a framed message |buff| of |len| bytes through the |fd| descriptor. +// This really adds a 4-hexchar prefix describing the payload size. +// Returns 0 on success, and -1 on error. +int qemu_pipe_frame_send(int fd, const void* buff, size_t len); + +// Read a frame message from |fd|, and store it into |buff| of |len| bytes. +// If the framed message is larger than |len|, then this returns -1 and the +// content is lost. Otherwise, this returns the size of the message. NOTE: +// empty messages are possible in a framed wire protocol and do not mean +// end-of-stream. +int qemu_pipe_frame_recv(int fd, void* buff, size_t len); + +#ifdef __cplusplus +} +#endif + +#endif /* ANDROID_CORE_INCLUDE_QEMU_PIPE_H */ diff --git a/include/system/qemu_pipe.h b/qemu_pipe/qemu_pipe.cpp similarity index 91% rename from include/system/qemu_pipe.h rename to qemu_pipe/qemu_pipe.cpp index af2507997..a4529deb8 100644 --- a/include/system/qemu_pipe.h +++ b/qemu_pipe/qemu_pipe.cpp @@ -13,13 +13,15 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -#ifndef ANDROID_INCLUDE_SYSTEM_QEMU_PIPE_H -#define ANDROID_INCLUDE_SYSTEM_QEMU_PIPE_H + +#include "qemu_pipe.h" #include #include #include #include +#include + // Define QEMU_PIPE_DEBUG if you want to print error messages when an error // occurs during pipe operations. The macro should simply take a printf-style @@ -48,7 +50,7 @@ // You should be able to open several pipes to the same pipe service, // except for a few special cases (e.g. GSM modem), where EBUSY will be // returned if more than one client tries to connect to it. -static __inline__ int qemu_pipe_open(const char* pipeName) { +int qemu_pipe_open(const char* pipeName) { // Sanity check. if (!pipeName || memcmp(pipeName, "pipe:", 5) != 0) { errno = EINVAL; @@ -81,9 +83,7 @@ static __inline__ int qemu_pipe_open(const char* pipeName) { // Send a framed message |buff| of |len| bytes through the |fd| descriptor. // This really adds a 4-hexchar prefix describing the payload size. // Returns 0 on success, and -1 on error. -static int __inline__ qemu_pipe_frame_send(int fd, - const void* buff, - size_t len) { +int qemu_pipe_frame_send(int fd, const void* buff, size_t len) { char header[5]; snprintf(header, sizeof(header), "%04zx", len); ssize_t ret = TEMP_FAILURE_RETRY(write(fd, header, 4)); @@ -104,7 +104,7 @@ static int __inline__ qemu_pipe_frame_send(int fd, // content is lost. Otherwise, this returns the size of the message. NOTE: // empty messages are possible in a framed wire protocol and do not mean // end-of-stream. -static int __inline__ qemu_pipe_frame_recv(int fd, void* buff, size_t len) { +int qemu_pipe_frame_recv(int fd, void* buff, size_t len) { char header[5]; ssize_t ret = TEMP_FAILURE_RETRY(read(fd, header, 4)); if (ret != 4) { @@ -130,5 +130,3 @@ static int __inline__ qemu_pipe_frame_recv(int fd, void* buff, size_t len) { } return size; } - -#endif /* ANDROID_INCLUDE_HARDWARE_QEMUD_PIPE_H */ From 7b60bd95dfa07e86325b432465fb0043648f6c97 Mon Sep 17 00:00:00 2001 From: bohu Date: Wed, 1 Mar 2017 23:26:02 -0800 Subject: [PATCH 2/3] Emulator: Enhance qemu_pipe.h to handle partial rw Partial read and write happen and it is better to try again unless there is some hard error. This is meant to fix some flaky behavior of emulator pipe services, hopefully. BUG: 35207286 manually tested this on emulator image. (cherry picked from aosp f099dce4a622f2ece313abe71a422489704ee692) Change-Id: I26a4560fa34e979939edcc882fcc5190202fe9f6 --- qemu_pipe/qemu_pipe.cpp | 25 ++++++++++--------------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/qemu_pipe/qemu_pipe.cpp b/qemu_pipe/qemu_pipe.cpp index a4529deb8..ca3b79578 100644 --- a/qemu_pipe/qemu_pipe.cpp +++ b/qemu_pipe/qemu_pipe.cpp @@ -22,6 +22,10 @@ #include #include +#include + +using android::base::ReadFully; +using android::base::WriteFully; // Define QEMU_PIPE_DEBUG if you want to print error messages when an error // occurs during pipe operations. The macro should simply take a printf-style @@ -66,15 +70,10 @@ int qemu_pipe_open(const char* pipeName) { // Write the pipe name, *including* the trailing zero which is necessary. size_t pipeNameLen = strlen(pipeName); - ssize_t ret = TEMP_FAILURE_RETRY(write(fd, pipeName, pipeNameLen + 1U)); - if (ret != (ssize_t)pipeNameLen + 1) { + if (!WriteFully(fd, pipeName, pipeNameLen + 1U)) { QEMU_PIPE_DEBUG("%s: Could not connect to %s pipe service: %s", __FUNCTION__, pipeName, strerror(errno)); - if (ret == 0) { - errno = ECONNRESET; - } else if (ret > 0) { - errno = EINVAL; - } + close(fd); return -1; } return fd; @@ -86,13 +85,11 @@ int qemu_pipe_open(const char* pipeName) { int qemu_pipe_frame_send(int fd, const void* buff, size_t len) { char header[5]; snprintf(header, sizeof(header), "%04zx", len); - ssize_t ret = TEMP_FAILURE_RETRY(write(fd, header, 4)); - if (ret != 4) { + if (!WriteFully(fd, header, 4)) { QEMU_PIPE_DEBUG("Can't write qemud frame header: %s", strerror(errno)); return -1; } - ret = TEMP_FAILURE_RETRY(write(fd, buff, len)); - if (ret != (ssize_t)len) { + if (!WriteFully(fd, buff, len)) { QEMU_PIPE_DEBUG("Can't write qemud frame payload: %s", strerror(errno)); return -1; } @@ -106,8 +103,7 @@ int qemu_pipe_frame_send(int fd, const void* buff, size_t len) { // end-of-stream. int qemu_pipe_frame_recv(int fd, void* buff, size_t len) { char header[5]; - ssize_t ret = TEMP_FAILURE_RETRY(read(fd, header, 4)); - if (ret != 4) { + if (!ReadFully(fd, header, 4)) { QEMU_PIPE_DEBUG("Can't read qemud frame header: %s", strerror(errno)); return -1; } @@ -122,8 +118,7 @@ int qemu_pipe_frame_recv(int fd, void* buff, size_t len) { len); return -1; } - ret = TEMP_FAILURE_RETRY(read(fd, buff, size)); - if (ret != (ssize_t)size) { + if (!ReadFully(fd, buff, size)) { QEMU_PIPE_DEBUG("Could not read qemud frame payload: %s", strerror(errno)); return -1; From a19abf17697863c2458d7d085a225ff4f3c75f75 Mon Sep 17 00:00:00 2001 From: bohu Date: Wed, 1 Mar 2017 23:31:14 -0800 Subject: [PATCH 3/3] Qemu: make the qemu_pipe_open back compatible Commit c7b098ceb528afc62b1545377201e45f5d37f974 has changed the qemu_pipe_open interface to require the "pipe:" prefix in the service name. However in APIs 24 and before, the "pipe:" prefix is not required This causes quite some confusion and bugs since it is very common to forget the difference when working across differnet APIs. This CL is meant to make qemu_pipe_open work in both cases by doing the following: 1. try the service name as is; 2. if it fails, add 'pipe:' prefix and try the service name again. Change-Id: If9782396c03780fad1aadeb8374eb308517dc963 (cherry picked from aosp f7d64fd8e1703c54ff01c2e53b0af850977777a0) --- qemu_pipe/include/qemu_pipe.h | 6 +++-- qemu_pipe/qemu_pipe.cpp | 51 +++++++++++------------------------ 2 files changed, 20 insertions(+), 37 deletions(-) diff --git a/qemu_pipe/include/qemu_pipe.h b/qemu_pipe/include/qemu_pipe.h index 16486c087..098749899 100644 --- a/qemu_pipe/include/qemu_pipe.h +++ b/qemu_pipe/include/qemu_pipe.h @@ -28,8 +28,10 @@ extern "C" { // This file descriptor can be used as a standard pipe/socket descriptor. // // 'pipeName' is the name of the emulator service you want to connect to, -// and must begin with 'pipe:' (e.g. 'pipe:camera' or 'pipe:opengles'). -// +// and should begin with 'pipe:' (e.g. 'pipe:camera' or 'pipe:opengles'). +// For backward compatibility, the 'pipe:' prefix can be omitted, and in +// that case, qemu_pipe_open will add it for you. + // On success, return a valid file descriptor, or -1/errno on failure. E.g.: // // EINVAL -> unknown/unsupported pipeName diff --git a/qemu_pipe/qemu_pipe.cpp b/qemu_pipe/qemu_pipe.cpp index ca3b79578..beeccb07f 100644 --- a/qemu_pipe/qemu_pipe.cpp +++ b/qemu_pipe/qemu_pipe.cpp @@ -34,29 +34,9 @@ using android::base::WriteFully; # define QEMU_PIPE_DEBUG(...) (void)0 #endif -// Try to open a new Qemu fast-pipe. This function returns a file descriptor -// that can be used to communicate with a named service managed by the -// emulator. -// -// This file descriptor can be used as a standard pipe/socket descriptor. -// -// 'pipeName' is the name of the emulator service you want to connect to, -// and must begin with 'pipe:' (e.g. 'pipe:camera' or 'pipe:opengles'). -// -// On success, return a valid file descriptor, or -1/errno on failure. E.g.: -// -// EINVAL -> unknown/unsupported pipeName -// ENOSYS -> fast pipes not available in this system. -// -// ENOSYS should never happen, except if you're trying to run within a -// misconfigured emulator. -// -// You should be able to open several pipes to the same pipe service, -// except for a few special cases (e.g. GSM modem), where EBUSY will be -// returned if more than one client tries to connect to it. int qemu_pipe_open(const char* pipeName) { // Sanity check. - if (!pipeName || memcmp(pipeName, "pipe:", 5) != 0) { + if (!pipeName) { errno = EINVAL; return -1; } @@ -70,18 +50,24 @@ int qemu_pipe_open(const char* pipeName) { // Write the pipe name, *including* the trailing zero which is necessary. size_t pipeNameLen = strlen(pipeName); - if (!WriteFully(fd, pipeName, pipeNameLen + 1U)) { - QEMU_PIPE_DEBUG("%s: Could not connect to %s pipe service: %s", - __FUNCTION__, pipeName, strerror(errno)); - close(fd); - return -1; + if (WriteFully(fd, pipeName, pipeNameLen + 1U)) { + return fd; } - return fd; + + // now, add 'pipe:' prefix and try again + // Note: host side will wait for the trailing '\0' to start + // service lookup. + const char pipe_prefix[] = "pipe:"; + if (WriteFully(fd, pipe_prefix, strlen(pipe_prefix)) && + WriteFully(fd, pipeName, pipeNameLen + 1U)) { + return fd; + } + QEMU_PIPE_DEBUG("%s: Could not write to %s pipe service: %s", + __FUNCTION__, pipeName, strerror(errno)); + close(fd); + return -1; } -// Send a framed message |buff| of |len| bytes through the |fd| descriptor. -// This really adds a 4-hexchar prefix describing the payload size. -// Returns 0 on success, and -1 on error. int qemu_pipe_frame_send(int fd, const void* buff, size_t len) { char header[5]; snprintf(header, sizeof(header), "%04zx", len); @@ -96,11 +82,6 @@ int qemu_pipe_frame_send(int fd, const void* buff, size_t len) { return 0; } -// Read a frame message from |fd|, and store it into |buff| of |len| bytes. -// If the framed message is larger than |len|, then this returns -1 and the -// content is lost. Otherwise, this returns the size of the message. NOTE: -// empty messages are possible in a framed wire protocol and do not mean -// end-of-stream. int qemu_pipe_frame_recv(int fd, void* buff, size_t len) { char header[5]; if (!ReadFully(fd, header, 4)) {