diff options
| -rw-r--r-- | current.txt | 6 | ||||
| -rw-r--r-- | dumpstate/1.1/IDumpstateDevice.hal | 24 | ||||
| -rw-r--r-- | dumpstate/1.1/types.hal | 31 | ||||
| -rw-r--r-- | dumpstate/1.1/vts/functional/VtsHalDumpstateV1_1TargetTest.cpp | 200 |
4 files changed, 221 insertions, 40 deletions
diff --git a/current.txt b/current.txt index 0d77db9ec1..1d62a8faf0 100644 --- a/current.txt +++ b/current.txt @@ -612,8 +612,8 @@ dd377f404a8e71f6191d295e10067db629b0f0c28e594af906f2bea5d87fe2cc android.hardwar 40ab2c6866c18d32baf6e49e3053949e79601f56963a791e93e68b9ee18f718d android.hardware.bluetooth@1.1::IBluetoothHciCallbacks 07d0a252b2d8fa35887908a996ba395cf392968395fc30afab791f46e0c22a52 android.hardware.boot@1.1::IBootControl 74049a402be913963edfdd80828a53736570e9d8124a1bf18166b6ed46a6b0ab android.hardware.boot@1.1::types -881aa8720fb1d69aa9843bfab69d810ab7654a61d2f5ab5e2626cbf240f24eaf android.hardware.dumpstate@1.1::types -13b33f623521ded51a6c0f7ea5b77e97066d0aa1e38a83c2873f08ad67294f89 android.hardware.dumpstate@1.1::IDumpstateDevice +446287268831f4ddfac4a51bb1c32ae1e48e47bccd535fccc2c4546d0e7c4013 android.hardware.dumpstate@1.1::types +f284ffde7cadf5a1364b75ab313baf22401eeca289bdde2a2dc7a27ea4ab98d7 android.hardware.dumpstate@1.1::IDumpstateDevice ce8dbe76eb9ee94b46ef98f725be992e760a5751073d4f4912484026541371f3 android.hardware.health@2.1::IHealth 26f04510a0b57aba5167c5c0a7c2f077c2acbb98b81902a072517829fd9fd67f android.hardware.health@2.1::IHealthInfoCallback db47f4ceceb1f06c656f39caa70c557b0f8471ef59fd58611bea667ffca20101 android.hardware.health@2.1::types @@ -644,4 +644,4 @@ e96ae1c3a9c0689002ec2318e9c587f4f607c16a75a3cd38788b77eb91072021 android.hardwar dcc8872337f0135e81970e1d8d5fd7139160dc80e9be76f0ae05290fa7e472b8 android.hardware.radio.config@1.3::types a2977755bc5f1ef47f04b7f2400632efda6218e1515dba847da487145cfabc4f android.hardware.radio.config@1.3::IRadioConfig 742360c775313438b0f82256eac62fb5bbc76a6ae6f388573f3aa142fb2c1eea android.hardware.radio.config@1.3::IRadioConfigIndication -0006ab8e8b0910cbd3bbb08d5f17d5fac7d65a2bdad5f2334e4851db9d1e6fa8 android.hardware.radio.config@1.3::IRadioConfigResponse
\ No newline at end of file +0006ab8e8b0910cbd3bbb08d5f17d5fac7d65a2bdad5f2334e4851db9d1e6fa8 android.hardware.radio.config@1.3::IRadioConfigResponse diff --git a/dumpstate/1.1/IDumpstateDevice.hal b/dumpstate/1.1/IDumpstateDevice.hal index 24831b37b2..502c460998 100644 --- a/dumpstate/1.1/IDumpstateDevice.hal +++ b/dumpstate/1.1/IDumpstateDevice.hal @@ -13,6 +13,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ + package android.hardware.dumpstate@1.1; import @1.0::IDumpstateDevice; @@ -28,14 +29,19 @@ interface IDumpstateDevice extends @1.0::IDumpstateDevice { * The 1.0 version of #dumpstateBoard(handle) should just delegate to this new method and pass * DumpstateMode::DEFAULT and a timeout of 30,000ms (30 seconds). * + * This method may still be called by the dumpstate routine even if getDeviceLoggingEnabled + * returns false. In this case, it must return DumpstateStatus::DEVICE_LOGGING_NOT_ENABLED. + * * @param h A native handle with one or two valid file descriptors. The first FD is for text * output, the second (if present) is for binary output. * @param mode A mode value to restrict dumped content. * @param timeoutMillis An approximate "budget" for how much time this call has been allotted. * If execution runs longer than this, the IDumpstateDevice service may be killed and only * partial information will be included in the report. + * @return status A DumpstateStatus value indicating the final result. */ - dumpstateBoard_1_1(handle h, DumpstateMode mode, uint64_t timeoutMillis); + dumpstateBoard_1_1(handle h, DumpstateMode mode, uint64_t timeoutMillis) + generates (DumpstateStatus status); /** * Turns device vendor logging on or off. @@ -46,8 +52,20 @@ interface IDumpstateDevice extends @1.0::IDumpstateDevice { * memory/storage/battery impacts, calling this method on a user build should only be done after * user consent has been obtained, e.g. from a toggle in developer settings. * + * Even if device logging has been disabled, dumpstateBoard may still be called by the dumpstate + * routine. In this case, it must return DumpstateStatus::DEVICE_LOGGING_NOT_ENABLED. + * * @param enable Whether to enable or disable device vendor logging. - * @return success Whether or not the change took effect. */ - setDeviceLoggingEnabled(bool enable) generates (bool success); + setDeviceLoggingEnabled(bool enable); + + /** + * Queries the current state of device logging. Primarily for UI and informative purposes. + * + * Even if device logging has been disabled, dumpstateBoard may still be called by the dumpstate + * routine. In this case, it must return DumpstateStatus::DEVICE_LOGGING_NOT_ENABLED. + * + * @return enabled Whether or not vendor logging is currently enabled. + */ + getDeviceLoggingEnabled() generates (bool enabled); }; diff --git a/dumpstate/1.1/types.hal b/dumpstate/1.1/types.hal index a6f391aded..f5cbade151 100644 --- a/dumpstate/1.1/types.hal +++ b/dumpstate/1.1/types.hal @@ -13,6 +13,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ + package android.hardware.dumpstate@1.1; /** @@ -23,22 +24,18 @@ enum DumpstateMode : uint32_t { * Takes a bug report without user interference. */ FULL = 0, - /** * Interactive bug report, i.e. triggered by the user. */ INTERACTIVE = 1, - /** * Remote bug report triggered by DevicePolicyManager, for example. */ REMOTE = 2, - /** * Bug report triggered on a wear device. */ WEAR = 3, - /** * Bug report limited to only connectivity info (cellular, wifi, and networking). Sometimes * called "telephony" in legacy contexts. @@ -49,14 +46,34 @@ enum DumpstateMode : uint32_t { * user application traffic. */ CONNECTIVITY = 4, - /** * Bug report limited to only wifi info. */ WIFI = 5, + /** + * Default mode, essentially analogous to calling @1.0::IDumpstateDevice.dumpstateBoard(handle). + * This mode MUST be supported if the dumpstate HAL is implemented. + */ + DEFAULT = 6, +}; +/** + * A simple return enum for use with dumpstateBoard_1_1. + */ +enum DumpstateStatus : uint32_t { + OK = 0, + /** + * Returned for cases where the device doesn't support the given DumpstateMode (e.g. a phone + * trying to use DumpstateMode::WEAR). + */ + UNSUPPORTED_MODE = 1, + /** + * Returned for cases where an IllegalArgumentException is typically appropriate, e.g. missing + * file descriptors. + */ + ILLEGAL_ARGUMENT = 2, /** - * Default mode. + * Returned when device logging is not enabled. */ - DEFAULT = 6 + DEVICE_LOGGING_NOT_ENABLED = 3, }; diff --git a/dumpstate/1.1/vts/functional/VtsHalDumpstateV1_1TargetTest.cpp b/dumpstate/1.1/vts/functional/VtsHalDumpstateV1_1TargetTest.cpp index 583efbf722..089b039c2c 100644 --- a/dumpstate/1.1/vts/functional/VtsHalDumpstateV1_1TargetTest.cpp +++ b/dumpstate/1.1/vts/functional/VtsHalDumpstateV1_1TargetTest.cpp @@ -19,6 +19,7 @@ #include <fcntl.h> #include <unistd.h> +#include <functional> #include <vector> #include <android/hardware/dumpstate/1.1/IDumpstateDevice.h> @@ -34,21 +35,56 @@ namespace { using ::android::sp; using ::android::hardware::Return; using ::android::hardware::dumpstate::V1_1::DumpstateMode; +using ::android::hardware::dumpstate::V1_1::DumpstateStatus; using ::android::hardware::dumpstate::V1_1::IDumpstateDevice; +using ::android::hardware::dumpstate::V1_1::toString; class DumpstateHidl1_1Test : public ::testing::TestWithParam<std::string> { - public: - virtual void SetUp() override { + protected: + virtual void SetUp() override { GetService(); } + + void GetService() { dumpstate = IDumpstateDevice::getService(GetParam()); ASSERT_NE(dumpstate, nullptr) << "Could not get HIDL instance"; } + void ToggleDeviceLogging(bool enable) { + Return<void> status = dumpstate->setDeviceLoggingEnabled(enable); + ASSERT_TRUE(status.isOk()) << "Status should be ok: " << status.description(); + + if (!dumpstate->ping().isOk()) { + ALOGW("IDumpstateDevice service appears to have exited lazily, attempting to get " + "again"); + GetService(); + } + + Return<bool> logging_enabled = dumpstate->getDeviceLoggingEnabled(); + ASSERT_TRUE(logging_enabled.isOk()) + << "Status should be ok: " << logging_enabled.description(); + ASSERT_EQ(logging_enabled, enable) + << "Device logging should now be " << (enable ? "enabled" : "disabled"); + + if (!dumpstate->ping().isOk()) { + ALOGW("IDumpstateDevice service appears to have exited lazily, attempting to get " + "again"); + GetService(); + } + } + + void EnableDeviceLogging() { ToggleDeviceLogging(true); } + + void DisableDeviceLogging() { ToggleDeviceLogging(false); } + sp<IDumpstateDevice> dumpstate; }; #define TEST_FOR_DUMPSTATE_MODE(name, body, mode) \ TEST_P(DumpstateHidl1_1Test, name##_##mode) { body(DumpstateMode::mode); } +// We use a macro to define individual test cases instead of hidl_enum_range<> because some HAL +// implementations are lazy and may call exit() at the end of dumpstateBoard(), which would cause +// DEAD_OBJECT errors after the first iteration. Separate cases re-get the service each time as part +// of SetUp(), and also provide better separation of concerns when specific modes are problematic. #define TEST_FOR_ALL_DUMPSTATE_MODES(name, body) \ TEST_FOR_DUMPSTATE_MODE(name, body, FULL); \ TEST_FOR_DUMPSTATE_MODE(name, body, INTERACTIVE); \ @@ -60,21 +96,51 @@ class DumpstateHidl1_1Test : public ::testing::TestWithParam<std::string> { constexpr uint64_t kDefaultTimeoutMillis = 30 * 1000; // 30 seconds +// Will only execute additional_assertions when status == expected. +void AssertStatusForMode(const DumpstateMode mode, const Return<DumpstateStatus>& status, + const DumpstateStatus expected, + std::function<void()> additional_assertions = nullptr) { + ASSERT_TRUE(status.isOk()) << "Status should be ok and return a more specific DumpstateStatus: " + << status.description(); + if (mode == DumpstateMode::DEFAULT) { + ASSERT_EQ(expected, status) << "Required mode (DumpstateMode::" << toString(mode) + << "): status should be DumpstateStatus::" << toString(expected) + << ", but got DumpstateStatus::" << toString(status); + } else { + // The rest of the modes are optional to support, but they MUST return either the expected + // value or UNSUPPORTED_MODE. + ASSERT_TRUE(status == expected || status == DumpstateStatus::UNSUPPORTED_MODE) + << "Optional mode (DumpstateMode::" << toString(mode) + << "): status should be DumpstateStatus::" << toString(expected) + << " or DumpstateStatus::UNSUPPORTED_MODE, but got DumpstateStatus::" + << toString(status); + } + if (status == expected && additional_assertions != nullptr) { + additional_assertions(); + } +} + // Negative test: make sure dumpstateBoard() doesn't crash when passed a null pointer. TEST_FOR_ALL_DUMPSTATE_MODES(TestNullHandle, [this](DumpstateMode mode) { - Return<void> status = dumpstate->dumpstateBoard_1_1(nullptr, mode, kDefaultTimeoutMillis); + EnableDeviceLogging(); - ASSERT_TRUE(status.isOk()) << "Status should be ok: " << status.description(); + Return<DumpstateStatus> status = + dumpstate->dumpstateBoard_1_1(nullptr, mode, kDefaultTimeoutMillis); + + AssertStatusForMode(mode, status, DumpstateStatus::ILLEGAL_ARGUMENT); }); // Negative test: make sure dumpstateBoard() ignores a handle with no FD. TEST_FOR_ALL_DUMPSTATE_MODES(TestHandleWithNoFd, [this](DumpstateMode mode) { + EnableDeviceLogging(); + native_handle_t* handle = native_handle_create(0, 0); ASSERT_NE(handle, nullptr) << "Could not create native_handle"; - Return<void> status = dumpstate->dumpstateBoard_1_1(handle, mode, kDefaultTimeoutMillis); + Return<DumpstateStatus> status = + dumpstate->dumpstateBoard_1_1(handle, mode, kDefaultTimeoutMillis); - ASSERT_TRUE(status.isOk()) << "Status should be ok: " << status.description(); + AssertStatusForMode(mode, status, DumpstateStatus::ILLEGAL_ARGUMENT); native_handle_close(handle); native_handle_delete(handle); @@ -82,6 +148,8 @@ TEST_FOR_ALL_DUMPSTATE_MODES(TestHandleWithNoFd, [this](DumpstateMode mode) { // Positive test: make sure dumpstateBoard() writes something to the FD. TEST_FOR_ALL_DUMPSTATE_MODES(TestOk, [this](DumpstateMode mode) { + EnableDeviceLogging(); + // Index 0 corresponds to the read end of the pipe; 1 to the write end. int fds[2]; ASSERT_EQ(0, pipe2(fds, O_NONBLOCK)) << errno; @@ -90,12 +158,14 @@ TEST_FOR_ALL_DUMPSTATE_MODES(TestOk, [this](DumpstateMode mode) { ASSERT_NE(handle, nullptr) << "Could not create native_handle"; handle->data[0] = fds[1]; - Return<void> status = dumpstate->dumpstateBoard_1_1(handle, mode, kDefaultTimeoutMillis); - ASSERT_TRUE(status.isOk()) << "Status should be ok: " << status.description(); + Return<DumpstateStatus> status = + dumpstate->dumpstateBoard_1_1(handle, mode, kDefaultTimeoutMillis); - // Check that at least one byte was written - char buff; - ASSERT_EQ(1, read(fds[0], &buff, 1)) << "dumped nothing"; + AssertStatusForMode(mode, status, DumpstateStatus::OK, [&fds]() { + // Check that at least one byte was written. + char buff; + ASSERT_EQ(1, read(fds[0], &buff, 1)) << "Dumped nothing"; + }); native_handle_close(handle); native_handle_delete(handle); @@ -103,6 +173,8 @@ TEST_FOR_ALL_DUMPSTATE_MODES(TestOk, [this](DumpstateMode mode) { // Positive test: make sure dumpstateBoard() doesn't crash with two FDs. TEST_FOR_ALL_DUMPSTATE_MODES(TestHandleWithTwoFds, [this](DumpstateMode mode) { + EnableDeviceLogging(); + int fds1[2]; int fds2[2]; ASSERT_EQ(0, pipe2(fds1, O_NONBLOCK)) << errno; @@ -113,8 +185,17 @@ TEST_FOR_ALL_DUMPSTATE_MODES(TestHandleWithTwoFds, [this](DumpstateMode mode) { handle->data[0] = fds1[1]; handle->data[1] = fds2[1]; - Return<void> status = dumpstate->dumpstateBoard_1_1(handle, mode, kDefaultTimeoutMillis); - ASSERT_TRUE(status.isOk()) << "Status should be ok: " << status.description(); + Return<DumpstateStatus> status = + dumpstate->dumpstateBoard_1_1(handle, mode, kDefaultTimeoutMillis); + + AssertStatusForMode(mode, status, DumpstateStatus::OK, [&fds1, &fds2]() { + // Check that at least one byte was written to one of the FDs. + char buff; + size_t read1 = read(fds1[0], &buff, 1); + size_t read2 = read(fds2[0], &buff, 1); + // Sometimes read returns -1, so we can't just add them together and expect >= 1. + ASSERT_TRUE(read1 == 1 || read2 == 1) << "Dumped nothing"; + }); native_handle_close(handle); native_handle_delete(handle); @@ -122,6 +203,8 @@ TEST_FOR_ALL_DUMPSTATE_MODES(TestHandleWithTwoFds, [this](DumpstateMode mode) { // Make sure dumpstateBoard_1_1 actually validates its arguments. TEST_P(DumpstateHidl1_1Test, TestInvalidModeArgument_Negative) { + EnableDeviceLogging(); + int fds[2]; ASSERT_EQ(0, pipe2(fds, O_NONBLOCK)) << errno; @@ -129,16 +212,21 @@ TEST_P(DumpstateHidl1_1Test, TestInvalidModeArgument_Negative) { ASSERT_NE(handle, nullptr) << "Could not create native_handle"; handle->data[0] = fds[1]; - Return<void> status = dumpstate->dumpstateBoard_1_1(handle, static_cast<DumpstateMode>(-100), - kDefaultTimeoutMillis); - ASSERT_FALSE(status.isOk()) << "Status should not be ok with invalid mode param: " - << status.description(); + Return<DumpstateStatus> status = dumpstate->dumpstateBoard_1_1( + handle, static_cast<DumpstateMode>(-100), kDefaultTimeoutMillis); + + ASSERT_TRUE(status.isOk()) << "Status should be ok and return a more specific DumpstateStatus: " + << status.description(); + ASSERT_EQ(status, DumpstateStatus::ILLEGAL_ARGUMENT) + << "Should return DumpstateStatus::ILLEGAL_ARGUMENT for invalid mode param"; native_handle_close(handle); native_handle_delete(handle); } TEST_P(DumpstateHidl1_1Test, TestInvalidModeArgument_Undefined) { + EnableDeviceLogging(); + int fds[2]; ASSERT_EQ(0, pipe2(fds, O_NONBLOCK)) << errno; @@ -146,26 +234,84 @@ TEST_P(DumpstateHidl1_1Test, TestInvalidModeArgument_Undefined) { ASSERT_NE(handle, nullptr) << "Could not create native_handle"; handle->data[0] = fds[1]; - Return<void> status = dumpstate->dumpstateBoard_1_1(handle, static_cast<DumpstateMode>(9001), - kDefaultTimeoutMillis); - ASSERT_FALSE(status.isOk()) << "Status should not be ok with invalid mode param: " - << status.description(); + Return<DumpstateStatus> status = dumpstate->dumpstateBoard_1_1( + handle, static_cast<DumpstateMode>(9001), kDefaultTimeoutMillis); + + ASSERT_TRUE(status.isOk()) << "Status should be ok and return a more specific DumpstateStatus: " + << status.description(); + ASSERT_EQ(status, DumpstateStatus::ILLEGAL_ARGUMENT) + << "Should return DumpstateStatus::ILLEGAL_ARGUMENT for invalid mode param"; native_handle_close(handle); native_handle_delete(handle); } -// Make sure toggling device logging doesn't crash. -TEST_P(DumpstateHidl1_1Test, TestEnableDeviceLogging) { - Return<bool> status = dumpstate->setDeviceLoggingEnabled(true); +// Positive test: make sure dumpstateBoard() from 1.0 doesn't fail. +TEST_P(DumpstateHidl1_1Test, Test1_0MethodOk) { + EnableDeviceLogging(); + + int fds[2]; + ASSERT_EQ(0, pipe2(fds, O_NONBLOCK)) << errno; + + native_handle_t* handle = native_handle_create(1, 0); + ASSERT_NE(handle, nullptr) << "Could not create native_handle"; + handle->data[0] = fds[1]; + + Return<void> status = dumpstate->dumpstateBoard(handle); ASSERT_TRUE(status.isOk()) << "Status should be ok: " << status.description(); + + // Check that at least one byte was written. + char buff; + ASSERT_EQ(1, read(fds[0], &buff, 1)) << "Dumped nothing"; + + native_handle_close(handle); + native_handle_delete(handle); } -TEST_P(DumpstateHidl1_1Test, TestDisableDeviceLogging) { - Return<bool> status = dumpstate->setDeviceLoggingEnabled(false); +// Make sure disabling device logging behaves correctly. +TEST_FOR_ALL_DUMPSTATE_MODES(TestDeviceLoggingDisabled, [this](DumpstateMode mode) { + DisableDeviceLogging(); - ASSERT_TRUE(status.isOk()) << "Status should be ok: " << status.description(); + // Index 0 corresponds to the read end of the pipe; 1 to the write end. + int fds[2]; + ASSERT_EQ(0, pipe2(fds, O_NONBLOCK)) << errno; + + native_handle_t* handle = native_handle_create(1, 0); + ASSERT_NE(handle, nullptr) << "Could not create native_handle"; + handle->data[0] = fds[1]; + + Return<DumpstateStatus> status = + dumpstate->dumpstateBoard_1_1(handle, mode, kDefaultTimeoutMillis); + + AssertStatusForMode(mode, status, DumpstateStatus::DEVICE_LOGGING_NOT_ENABLED, [&fds]() { + // Check that nothing was written. Could return 0 or -1. + char buff; + ASSERT_NE(1, read(fds[0], &buff, 1)) << "Dumped something when device logging is disabled"; + }); + + native_handle_close(handle); + native_handle_delete(handle); +}); + +// Double-enable is perfectly valid, but the second call shouldn't do anything. +TEST_P(DumpstateHidl1_1Test, TestRepeatedEnable) { + EnableDeviceLogging(); + EnableDeviceLogging(); +} + +// Double-disable is perfectly valid, but the second call shouldn't do anything. +TEST_P(DumpstateHidl1_1Test, TestRepeatedDisable) { + DisableDeviceLogging(); + DisableDeviceLogging(); +} + +// Toggling in short order is perfectly valid. +TEST_P(DumpstateHidl1_1Test, TestRepeatedToggle) { + EnableDeviceLogging(); + DisableDeviceLogging(); + EnableDeviceLogging(); + DisableDeviceLogging(); } INSTANTIATE_TEST_SUITE_P( |