diff --git a/automotive/vehicle/aidl/impl/vhal/include/ParcelableUtils.h b/automotive/vehicle/aidl/impl/vhal/include/ParcelableUtils.h index dcb15b9c63..4b7c2f3221 100644 --- a/automotive/vehicle/aidl/impl/vhal/include/ParcelableUtils.h +++ b/automotive/vehicle/aidl/impl/vhal/include/ParcelableUtils.h @@ -31,19 +31,18 @@ namespace vehicle { template ::ndk::ScopedAStatus vectorToStableLargeParcelable(std::vector&& values, T2* output) { + output->payloads = std::move(values); auto result = ::android::automotive::car_binder_lib::LargeParcelableBase:: - parcelableVectorToStableLargeParcelable(values); + parcelableToStableLargeParcelable(*output); if (!result.ok()) { return toScopedAStatus( result, ::aidl::android::hardware::automotive::vehicle::StatusCode::INTERNAL_ERROR); } auto& fd = result.value(); - if (fd == nullptr) { - // If we no longer needs values, move it inside the payloads to avoid copying. - output->payloads = std::move(values); - } else { + if (fd != nullptr) { // Move the returned ScopedFileDescriptor pointer to ScopedFileDescriptor value in // 'sharedMemoryFd' field. + output->payloads.clear(); output->sharedMemoryFd = std::move(*fd); } return ::ndk::ScopedAStatus::ok(); @@ -57,12 +56,13 @@ template return vectorToStableLargeParcelable(std::move(valuesCopy), output); } -template -::android::base::expected, ::ndk::ScopedAStatus> stableLargeParcelableToVector( - const T2& largeParcelable) { - ::android::base::Result>> result = - ::android::automotive::car_binder_lib::LargeParcelableBase:: - stableLargeParcelableToParcelableVector(largeParcelable.sharedMemoryFd); +template +::android::base::expected< + ::android::automotive::car_binder_lib::LargeParcelableBase::BorrowedOwnedObject, + ::ndk::ScopedAStatus> +fromStableLargeParcelable(const T& largeParcelable) { + auto result = ::android::automotive::car_binder_lib::LargeParcelableBase:: + stableLargeParcelableToParcelable(largeParcelable); if (!result.ok()) { return ::android::base::unexpected(toScopedAStatus( @@ -70,15 +70,7 @@ template "failed to parse large parcelable")); } - if (!result.value().has_value()) { - return ::android::base::unexpected( - ::ndk::ScopedAStatus::fromServiceSpecificErrorWithMessage( - toInt(::aidl::android::hardware::automotive::vehicle::StatusCode:: - INVALID_ARG), - "empty request")); - } - - return std::move(result.value().value()); + return std::move(result.value()); } } // namespace vehicle diff --git a/automotive/vehicle/aidl/impl/vhal/src/DefaultVehicleHal.cpp b/automotive/vehicle/aidl/impl/vhal/src/DefaultVehicleHal.cpp index e98f02112e..7f09a59fbb 100644 --- a/automotive/vehicle/aidl/impl/vhal/src/DefaultVehicleHal.cpp +++ b/automotive/vehicle/aidl/impl/vhal/src/DefaultVehicleHal.cpp @@ -57,7 +57,9 @@ DefaultVehicleHal::DefaultVehicleHal(std::unique_ptr hardware) for (auto& config : configs) { mConfigsByPropId[config.prop] = config; } - auto result = LargeParcelableBase::parcelableVectorToStableLargeParcelable(configs); + VehiclePropConfigs vehiclePropConfigs; + vehiclePropConfigs.payloads = std::move(configs); + auto result = LargeParcelableBase::parcelableToStableLargeParcelable(vehiclePropConfigs); if (!result.ok()) { ALOGE("failed to convert configs to shared memory file, error: %s, code: %d", getErrorMsg(result).c_str(), getIntErrorCode(result)); @@ -71,6 +73,7 @@ DefaultVehicleHal::DefaultVehicleHal(std::unique_ptr hardware) ScopedAStatus DefaultVehicleHal::getAllPropConfigs(VehiclePropConfigs* output) { if (mConfigFile != nullptr) { + output->payloads.clear(); output->sharedMemoryFd.set(dup(mConfigFile->get())); return ScopedAStatus::ok(); } @@ -131,20 +134,14 @@ ScopedAStatus DefaultVehicleHal::getValues(const CallbackType& callback, const GetValueRequests& requests) { // TODO(b/203713317): check for duplicate properties and duplicate request IDs. - const std::vector* getValueRequests; - // Define deserializedResults here because we need it to have the same lifetime as - // getValueRequests. - expected, ScopedAStatus> deserializedResults; - if (!requests.payloads.empty()) { - getValueRequests = &requests.payloads; - } else { - deserializedResults = stableLargeParcelableToVector(requests); - if (!deserializedResults.ok()) { - ALOGE("failed to parse getValues requests"); - return std::move(deserializedResults.error()); - } - getValueRequests = &deserializedResults.value(); + expected, ScopedAStatus> + deserializedResults = fromStableLargeParcelable(requests); + if (!deserializedResults.ok()) { + ALOGE("getValues: failed to parse getValues requests"); + return std::move(deserializedResults.error()); } + const std::vector& getValueRequests = + deserializedResults.value().getObject()->payloads; std::shared_ptr client; { @@ -153,7 +150,7 @@ ScopedAStatus DefaultVehicleHal::getValues(const CallbackType& callback, } if (StatusCode status = - mVehicleHardware->getValues(client->getResultCallback(), *getValueRequests); + mVehicleHardware->getValues(client->getResultCallback(), getValueRequests); status != StatusCode::OK) { return ScopedAStatus::fromServiceSpecificErrorWithMessage( toInt(status), "failed to get value from VehicleHardware"); @@ -166,27 +163,21 @@ ScopedAStatus DefaultVehicleHal::setValues(const CallbackType& callback, const SetValueRequests& requests) { // TODO(b/203713317): check for duplicate properties and duplicate request IDs. - const std::vector* setValueRequests; - // Define deserializedResults here because we need it to have the same lifetime as - // setValueRequests. - expected, ScopedAStatus> deserializedResults; - if (!requests.payloads.empty()) { - setValueRequests = &requests.payloads; - } else { - deserializedResults = stableLargeParcelableToVector(requests); - if (!deserializedResults.ok()) { - ALOGE("failed to parse setValues requests"); - return std::move(deserializedResults.error()); - } - setValueRequests = &deserializedResults.value(); + expected, ScopedAStatus> + deserializedResults = fromStableLargeParcelable(requests); + if (!deserializedResults.ok()) { + ALOGE("setValues: failed to parse setValues requests"); + return std::move(deserializedResults.error()); } + const std::vector& setValueRequests = + deserializedResults.value().getObject()->payloads; // A list of failed result we already know before sending to hardware. std::vector failedResults; // The list of requests that we would send to hardware. std::vector hardwareRequests; - for (auto& request : *setValueRequests) { + for (auto& request : setValueRequests) { int64_t requestId = request.requestId; if (auto result = checkProperty(request.value); !result.ok()) { ALOGW("property not valid: %s", result.error().message().c_str()); diff --git a/automotive/vehicle/aidl/impl/vhal/test/DefaultVehicleHalTest.cpp b/automotive/vehicle/aidl/impl/vhal/test/DefaultVehicleHalTest.cpp index 8934a7b012..ffc08a71b8 100644 --- a/automotive/vehicle/aidl/impl/vhal/test/DefaultVehicleHalTest.cpp +++ b/automotive/vehicle/aidl/impl/vhal/test/DefaultVehicleHalTest.cpp @@ -389,15 +389,14 @@ class DefaultVehicleHalTest : public ::testing::Test { }); } - auto result = LargeParcelableBase::parcelableVectorToStableLargeParcelable( - expectedHardwareRequests); + requests.payloads = expectedHardwareRequests; + auto result = LargeParcelableBase::parcelableToStableLargeParcelable(requests); if (!result.ok()) { return result.error(); } - if (result.value() == nullptr) { - requests.payloads = expectedHardwareRequests; - } else { + if (result.value() != nullptr) { requests.sharedMemoryFd = std::move(*result.value()); + requests.payloads.clear(); } return {}; } @@ -423,14 +422,13 @@ class DefaultVehicleHalTest : public ::testing::Test { }); } - auto result = LargeParcelableBase::parcelableVectorToStableLargeParcelable( - expectedHardwareRequests); + requests.payloads = expectedHardwareRequests; + auto result = LargeParcelableBase::parcelableToStableLargeParcelable(requests); if (!result.ok()) { return result.error(); } - if (result.value() == nullptr) { - requests.payloads = expectedHardwareRequests; - } else { + if (result.value() != nullptr) { + requests.payloads.clear(); requests.sharedMemoryFd = std::move(*result.value()); } return {}; @@ -490,13 +488,10 @@ TEST_F(DefaultVehicleHalTest, testGetAllPropConfigsLarge) { ASSERT_TRUE(status.isOk()) << "getAllPropConfigs failed: " << status.getMessage(); ASSERT_TRUE(output.payloads.empty()); - Result>> result = - LargeParcelableBase::stableLargeParcelableToParcelableVector( - output.sharedMemoryFd); + auto result = LargeParcelableBase::stableLargeParcelableToParcelable(output); ASSERT_TRUE(result.ok()) << "failed to parse result shared memory file: " << result.error().message(); - ASSERT_TRUE(result.value().has_value()) << "empty parsed value"; - ASSERT_EQ(result.value().value(), testConfigs); + ASSERT_EQ(result.value().getObject()->payloads, testConfigs); } TEST_F(DefaultVehicleHalTest, testGetValuesSmall) { @@ -544,11 +539,9 @@ TEST_F(DefaultVehicleHalTest, testGetValuesLarge) { ASSERT_TRUE(getValueResults.payloads.empty()) << "payload should be empty, shared memory file should be used"; - auto result = LargeParcelableBase::stableLargeParcelableToParcelableVector( - getValueResults.sharedMemoryFd); + auto result = LargeParcelableBase::stableLargeParcelableToParcelable(getValueResults); ASSERT_TRUE(result.ok()) << "failed to parse shared memory file"; - ASSERT_TRUE(result.value().has_value()) << "no parsed value"; - ASSERT_EQ(result.value().value(), expectedResults) << "results mismatch"; + ASSERT_EQ(result.value().getObject()->payloads, expectedResults) << "results mismatch"; EXPECT_EQ(countClients(), static_cast(1)); } @@ -621,11 +614,9 @@ TEST_F(DefaultVehicleHalTest, testSetValuesLarge) { ASSERT_TRUE(setValueResults.payloads.empty()) << "payload should be empty, shared memory file should be used"; - auto result = LargeParcelableBase::stableLargeParcelableToParcelableVector( - setValueResults.sharedMemoryFd); + auto result = LargeParcelableBase::stableLargeParcelableToParcelable(setValueResults); ASSERT_TRUE(result.ok()) << "failed to parse shared memory file"; - ASSERT_TRUE(result.value().has_value()) << "no parsed value"; - ASSERT_EQ(result.value().value(), expectedResults) << "results mismatch"; + ASSERT_EQ(result.value().getObject()->payloads, expectedResults) << "results mismatch"; EXPECT_EQ(countClients(), static_cast(1)); }