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

3 files changed, 218 insertions, 37 deletions

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(