From 6e278a379841d2aca907300ee8ed60034eac730e Mon Sep 17 00:00:00 2001 From: Hunter Knepshield Date: Mon, 3 Feb 2020 16:25:57 -0800 Subject: [PATCH] IDumpstateDevice@1.1 polish - Return a DumpstateStatus from dumpstateBoard_1_1 - Better toggle API surface: set/getDeviceLoggingEnabled - Improved testing to allow for unsupported DumpstateMode values Bug: 143183758 Bug: 143184495 Test: atest VtsHalDumpstateV1_1TargetTest Change-Id: I505c2a790dc28ddce9b6f5b674394ef65b31c80c --- current.txt | 4 +- dumpstate/1.1/IDumpstateDevice.hal | 24 ++- dumpstate/1.1/types.hal | 33 ++- .../VtsHalDumpstateV1_1TargetTest.cpp | 200 +++++++++++++++--- 4 files changed, 221 insertions(+), 40 deletions(-) diff --git a/current.txt b/current.txt index 8d068d0e89..c88af5c1d8 100644 --- a/current.txt +++ b/current.txt @@ -645,8 +645,8 @@ f18695dd36ee205640b8326a17453858a7b4596653aaa6ef0016b0aef1bd4dac android.hardwar 4d85e814f94949dae4dc6cb82bbd7d6bb24ffafda6ddb2eac928d2a4fc2e21ce android.hardware.cas@1.2::types 66931c2506fbb5af61f20138cb05e0a09e7bf67d6964c231d27c648933bb33ec android.hardware.drm@1.3::ICryptoFactory 994d08ab27d613022c258a9ec48cece7adf2a305e92df5d76ef923e2c6665f64 android.hardware.drm@1.3::IDrmFactory -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 769d346927a94fd40ee80a5a976d8d15cf022ef99c5900738f4a82f26c0ed229 android.hardware.gnss@2.1::types 88371e0edf69a1f72bfc45ecb2335e9b145e87339d3eecc92664a1fb200213ba android.hardware.gnss@2.1::IGnss ba62e1e8993bfb9f27fa04816fa0f2241ae2d01edfa3d0c04182e2e5de80045c android.hardware.gnss@2.1::IGnssCallback 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. + * Default mode, essentially analogous to calling @1.0::IDumpstateDevice.dumpstateBoard(handle). + * This mode MUST be supported if the dumpstate HAL is implemented. */ - DEFAULT = 6 + 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, + /** + * Returned when device logging is not enabled. + */ + 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 #include +#include #include #include @@ -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 { - 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 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 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 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 { constexpr uint64_t kDefaultTimeoutMillis = 30 * 1000; // 30 seconds +// Will only execute additional_assertions when status == expected. +void AssertStatusForMode(const DumpstateMode mode, const Return& status, + const DumpstateStatus expected, + std::function 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 status = dumpstate->dumpstateBoard_1_1(nullptr, mode, kDefaultTimeoutMillis); + EnableDeviceLogging(); - ASSERT_TRUE(status.isOk()) << "Status should be ok: " << status.description(); + Return 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 status = dumpstate->dumpstateBoard_1_1(handle, mode, kDefaultTimeoutMillis); + Return 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 status = dumpstate->dumpstateBoard_1_1(handle, mode, kDefaultTimeoutMillis); - ASSERT_TRUE(status.isOk()) << "Status should be ok: " << status.description(); + Return 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 status = dumpstate->dumpstateBoard_1_1(handle, mode, kDefaultTimeoutMillis); - ASSERT_TRUE(status.isOk()) << "Status should be ok: " << status.description(); + Return 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 status = dumpstate->dumpstateBoard_1_1(handle, static_cast(-100), - kDefaultTimeoutMillis); - ASSERT_FALSE(status.isOk()) << "Status should not be ok with invalid mode param: " - << status.description(); + Return status = dumpstate->dumpstateBoard_1_1( + handle, static_cast(-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 status = dumpstate->dumpstateBoard_1_1(handle, static_cast(9001), - kDefaultTimeoutMillis); - ASSERT_FALSE(status.isOk()) << "Status should not be ok with invalid mode param: " - << status.description(); + Return status = dumpstate->dumpstateBoard_1_1( + handle, static_cast(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 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 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 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 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(