Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 6e278a37 authored by Hunter Knepshield's avatar Hunter Knepshield
Browse files

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
parent 2ca858a4

current.txt

+2 −2
Original line number Diff line number Diff line
@@ -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

dumpstate/1.1/IDumpstateDevice.hal

+21 −3
Original line number Diff line number Diff line
@@ -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);

};

dumpstate/1.1/types.hal

+24 −7
Original line number Diff line number Diff line
@@ -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,

};



/**

     * Default mode.

 * 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.

     */

    DEFAULT = 6

    DEVICE_LOGGING_NOT_ENABLED = 3,

};

dumpstate/1.1/vts/functional/VtsHalDumpstateV1_1TargetTest.cpp

+173 −27
Original line number Diff line number Diff line
@@ -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

    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";

        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: "

    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: "

    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(