tequilaOS/platform_hardware_interfaces
2
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity

IDumpstateDevice@1.1 polish

Browse source 
- 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
Merged-In: I505c2a790dc28ddce9b6f5b674394ef65b31c80c
(cherry picked from commit 6e278a3798)

Change-Id: Ibbd15f65674cfa6b9f5c1d6a633277a419499d9f
This commit is contained in:
Hunter Knepshield 2020-02-03 16:25:57 -08:00
parent 456d54c936
commit 256f77a588
4 changed files with 222 additions and 41 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

6
current.txt
View file

@ -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
0006ab8e8b0910cbd3bbb08d5f17d5fac7d65a2bdad5f2334e4851db9d1e6fa8 android.hardware.radio.config@1.3::IRadioConfigResponse

24
dumpstate/1.1/IDumpstateDevice.hal
View file

@ -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);
};

33
dumpstate/1.1/types.hal
View file

@ -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,
};

200
dumpstate/1.1/vts/functional/VtsHalDumpstateV1_1TargetTest.cpp
View file

@ -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(