Use read-write lock to guard property config.
Property config now may change dynamically due to late-init. We might
also support dynamic config later. This CL uses read write lock
to properly guard access to config map.
Flag: EXEMPT HAL change
Test: atest DefaultVehicleHalTest
Bug: 342470570
(cherry picked from https://googleplex-android-review.googlesource.com/q/commit:35401eb1ba251507ed68748b19b0c4a2fcc56c40)
Merged-In: I0df79040c363e66baab48631f457752532d2967d
Change-Id: I0df79040c363e66baab48631f457752532d2967d
diff --git a/automotive/vehicle/aidl/impl/vhal/include/DefaultVehicleHal.h b/automotive/vehicle/aidl/impl/vhal/include/DefaultVehicleHal.h
index fa2a310..b58d0f5 100644
--- a/automotive/vehicle/aidl/impl/vhal/include/DefaultVehicleHal.h
+++ b/automotive/vehicle/aidl/impl/vhal/include/DefaultVehicleHal.h
@@ -31,6 +31,7 @@
#include <android-base/thread_annotations.h>
#include <android/binder_auto_utils.h>
+#include <functional>
#include <memory>
#include <mutex>
#include <shared_mutex>
@@ -138,12 +139,11 @@
// Only used for testing.
int32_t mTestInterfaceVersion = 0;
- // mConfigsByPropId and mConfigFile is lazy initialized.
- mutable std::mutex mConfigInitLock;
- mutable bool mConfigInit GUARDED_BY(mConfigInitLock) = false;
+ mutable std::atomic<bool> mConfigInit = false;
+ mutable std::shared_timed_mutex mConfigLock;
mutable std::unordered_map<int32_t, aidlvhal::VehiclePropConfig> mConfigsByPropId
- GUARDED_BY(mConfigInitLock);
- mutable std::unique_ptr<ndk::ScopedFileDescriptor> mConfigFile GUARDED_BY(mConfigInitLock);
+ GUARDED_BY(mConfigLock);
+ mutable std::unique_ptr<ndk::ScopedFileDescriptor> mConfigFile GUARDED_BY(mConfigLock);
std::mutex mLock;
std::unordered_map<const AIBinder*, std::unique_ptr<OnBinderDiedContext>> mOnBinderDiedContexts
@@ -175,7 +175,10 @@
android::base::Result<std::vector<int64_t>> checkDuplicateRequests(
const std::vector<aidlvhal::SetValueRequest>& requests);
- VhalResult<void> checkSubscribeOptions(const std::vector<aidlvhal::SubscribeOptions>& options);
+ VhalResult<void> checkSubscribeOptions(
+ const std::vector<aidlvhal::SubscribeOptions>& options,
+ const std::unordered_map<int32_t, aidlvhal::VehiclePropConfig>& configsByPropId)
+ REQUIRES_SHARED(mConfigLock);
VhalResult<void> checkPermissionHelper(const aidlvhal::VehiclePropValue& value,
aidlvhal::VehiclePropertyAccess accessToTest) const;
@@ -184,7 +187,7 @@
VhalResult<void> checkWritePermission(const aidlvhal::VehiclePropValue& value) const;
- android::base::Result<const aidlvhal::VehiclePropConfig*> getConfig(int32_t propId) const;
+ android::base::Result<aidlvhal::VehiclePropConfig> getConfig(int32_t propId) const;
void onBinderDiedWithContext(const AIBinder* clientId);
@@ -196,7 +199,7 @@
bool checkDumpPermission();
- bool getAllPropConfigsFromHardwareLocked() const REQUIRES(mConfigInitLock);
+ bool getAllPropConfigsFromHardwareLocked() const EXCLUDES(mConfigLock);
// The looping handler function to process all onBinderDied or onBinderUnlinked events in
// mBinderEvents.
@@ -209,10 +212,12 @@
int32_t getVhalInterfaceVersion() const;
- // Gets mConfigsByPropId, lazy init it if necessary.
- const std::unordered_map<int32_t, aidlvhal::VehiclePropConfig>& getConfigsByPropId() const;
- // Gets mConfigFile, lazy init it if necessary.
- const ndk::ScopedFileDescriptor* getConfigFile() const;
+ // Gets mConfigsByPropId, lazy init it if necessary. Note that the reference is only valid in
+ // the scope of the callback and it is guaranteed that read lock is obtained during the
+ // callback.
+ void getConfigsByPropId(
+ std::function<void(const std::unordered_map<int32_t, aidlvhal::VehiclePropConfig>&)>
+ callback) const EXCLUDES(mConfigLock);
// Puts the property change events into a queue so that they can handled in batch.
static void batchPropertyChangeEvent(
@@ -239,6 +244,12 @@
static void onBinderUnlinked(void* cookie);
+ static void parseSubscribeOptions(
+ const std::vector<aidlvhal::SubscribeOptions>& options,
+ const std::unordered_map<int32_t, aidlvhal::VehiclePropConfig>& configsByPropId,
+ std::vector<aidlvhal::SubscribeOptions>& onChangeSubscriptions,
+ std::vector<aidlvhal::SubscribeOptions>& continuousSubscriptions);
+
// Test-only
// Set the default timeout for pending requests.
void setTimeout(int64_t timeoutInNano);
diff --git a/automotive/vehicle/aidl/impl/vhal/src/DefaultVehicleHal.cpp b/automotive/vehicle/aidl/impl/vhal/src/DefaultVehicleHal.cpp
index 9dc039d..e062a28 100644
--- a/automotive/vehicle/aidl/impl/vhal/src/DefaultVehicleHal.cpp
+++ b/automotive/vehicle/aidl/impl/vhal/src/DefaultVehicleHal.cpp
@@ -95,6 +95,18 @@
return sampleRateHz;
}
+class SCOPED_CAPABILITY SharedScopedLockAssertion {
+ public:
+ SharedScopedLockAssertion(std::shared_timed_mutex& mutex) ACQUIRE_SHARED(mutex) {}
+ ~SharedScopedLockAssertion() RELEASE() {}
+};
+
+class SCOPED_CAPABILITY UniqueScopedLockAssertion {
+ public:
+ UniqueScopedLockAssertion(std::shared_timed_mutex& mutex) ACQUIRE(mutex) {}
+ ~UniqueScopedLockAssertion() RELEASE() {}
+};
+
} // namespace
DefaultVehicleHal::DefaultVehicleHal(std::unique_ptr<IVehicleHardware> vehicleHardware)
@@ -355,68 +367,82 @@
}
filteredConfigs.push_back(std::move(config));
}
- for (auto& config : filteredConfigs) {
- mConfigsByPropId[config.prop] = config;
- }
- VehiclePropConfigs vehiclePropConfigs;
- vehiclePropConfigs.payloads = std::move(filteredConfigs);
- auto result = LargeParcelableBase::parcelableToStableLargeParcelable(vehiclePropConfigs);
- if (!result.ok()) {
- ALOGE("failed to convert configs to shared memory file, error: %s, code: %d",
- result.error().message().c_str(), static_cast<int>(result.error().code()));
- mConfigFile = nullptr;
- return false;
+
+ {
+ std::unique_lock<std::shared_timed_mutex> configWriteLock(mConfigLock);
+ UniqueScopedLockAssertion lockAssertion(mConfigLock);
+
+ for (auto& config : filteredConfigs) {
+ mConfigsByPropId[config.prop] = config;
+ }
+ VehiclePropConfigs vehiclePropConfigs;
+ vehiclePropConfigs.payloads = std::move(filteredConfigs);
+ auto result = LargeParcelableBase::parcelableToStableLargeParcelable(vehiclePropConfigs);
+ if (!result.ok()) {
+ ALOGE("failed to convert configs to shared memory file, error: %s, code: %d",
+ result.error().message().c_str(), static_cast<int>(result.error().code()));
+ mConfigFile = nullptr;
+ return false;
+ }
+
+ if (result.value() != nullptr) {
+ mConfigFile = std::move(result.value());
+ }
}
- if (result.value() != nullptr) {
- mConfigFile = std::move(result.value());
- }
+ mConfigInit = true;
return true;
}
-const ScopedFileDescriptor* DefaultVehicleHal::getConfigFile() const {
- std::scoped_lock lockGuard(mConfigInitLock);
+void DefaultVehicleHal::getConfigsByPropId(
+ std::function<void(const std::unordered_map<int32_t, VehiclePropConfig>&)> callback) const {
if (!mConfigInit) {
CHECK(getAllPropConfigsFromHardwareLocked())
<< "Failed to get property configs from hardware";
- mConfigInit = true;
}
- return mConfigFile.get();
-}
-const std::unordered_map<int32_t, VehiclePropConfig>& DefaultVehicleHal::getConfigsByPropId()
- const {
- std::scoped_lock lockGuard(mConfigInitLock);
- if (!mConfigInit) {
- CHECK(getAllPropConfigsFromHardwareLocked())
- << "Failed to get property configs from hardware";
- mConfigInit = true;
- }
- return mConfigsByPropId;
+ std::shared_lock<std::shared_timed_mutex> configReadLock(mConfigLock);
+ SharedScopedLockAssertion lockAssertion(mConfigLock);
+
+ callback(mConfigsByPropId);
}
ScopedAStatus DefaultVehicleHal::getAllPropConfigs(VehiclePropConfigs* output) {
- const ScopedFileDescriptor* configFile = getConfigFile();
- const auto& configsByPropId = getConfigsByPropId();
- if (configFile != nullptr) {
+ if (!mConfigInit) {
+ CHECK(getAllPropConfigsFromHardwareLocked())
+ << "Failed to get property configs from hardware";
+ }
+
+ std::shared_lock<std::shared_timed_mutex> configReadLock(mConfigLock);
+ SharedScopedLockAssertion lockAssertion(mConfigLock);
+
+ if (mConfigFile != nullptr) {
output->payloads.clear();
- output->sharedMemoryFd.set(dup(configFile->get()));
+ output->sharedMemoryFd.set(dup(mConfigFile->get()));
return ScopedAStatus::ok();
}
- output->payloads.reserve(configsByPropId.size());
- for (const auto& [_, config] : configsByPropId) {
+
+ output->payloads.reserve(mConfigsByPropId.size());
+ for (const auto& [_, config] : mConfigsByPropId) {
output->payloads.push_back(config);
}
return ScopedAStatus::ok();
}
-Result<const VehiclePropConfig*> DefaultVehicleHal::getConfig(int32_t propId) const {
- const auto& configsByPropId = getConfigsByPropId();
- auto it = configsByPropId.find(propId);
- if (it == configsByPropId.end()) {
- return Error() << "no config for property, ID: " << propId;
- }
- return &(it->second);
+Result<VehiclePropConfig> DefaultVehicleHal::getConfig(int32_t propId) const {
+ Result<VehiclePropConfig> result;
+ getConfigsByPropId([this, &result, propId](const auto& configsByPropId) {
+ SharedScopedLockAssertion lockAssertion(mConfigLock);
+
+ auto it = configsByPropId.find(propId);
+ if (it == configsByPropId.end()) {
+ result = Error() << "no config for property, ID: " << propId;
+ return;
+ }
+ // Copy the VehiclePropConfig
+ result = it->second;
+ });
+ return result;
}
Result<void> DefaultVehicleHal::checkProperty(const VehiclePropValue& propValue) {
@@ -425,15 +451,15 @@
if (!result.ok()) {
return result.error();
}
- const VehiclePropConfig* config = result.value();
- const VehicleAreaConfig* areaConfig = getAreaConfig(propValue, *config);
+ const VehiclePropConfig& config = result.value();
+ const VehicleAreaConfig* areaConfig = getAreaConfig(propValue, config);
if (!isGlobalProp(propId) && areaConfig == nullptr) {
// Ignore areaId for global property. For non global property, check whether areaId is
// allowed. areaId must appear in areaConfig.
return Error() << "invalid area ID: " << propValue.areaId << " for prop ID: " << propId
<< ", not listed in config";
}
- if (auto result = checkPropValue(propValue, config); !result.ok()) {
+ if (auto result = checkPropValue(propValue, &config); !result.ok()) {
return Error() << "invalid property value: " << propValue.toString()
<< ", error: " << getErrorMsg(result);
}
@@ -659,17 +685,27 @@
ScopedAStatus DefaultVehicleHal::getPropConfigs(const std::vector<int32_t>& props,
VehiclePropConfigs* output) {
std::vector<VehiclePropConfig> configs;
- const auto& configsByPropId = getConfigsByPropId();
- for (int32_t prop : props) {
- auto it = configsByPropId.find(prop);
- if (it != configsByPropId.end()) {
- configs.push_back(it->second);
- } else {
- return ScopedAStatus::fromServiceSpecificErrorWithMessage(
- toInt(StatusCode::INVALID_ARG),
- StringPrintf("no config for property, ID: %" PRId32, prop).c_str());
+ ScopedAStatus status = ScopedAStatus::ok();
+ getConfigsByPropId([this, &configs, &status, &props](const auto& configsByPropId) {
+ SharedScopedLockAssertion lockAssertion(mConfigLock);
+
+ for (int32_t prop : props) {
+ auto it = configsByPropId.find(prop);
+ if (it != configsByPropId.end()) {
+ configs.push_back(it->second);
+ } else {
+ status = ScopedAStatus::fromServiceSpecificErrorWithMessage(
+ toInt(StatusCode::INVALID_ARG),
+ StringPrintf("no config for property, ID: %" PRId32, prop).c_str());
+ return;
+ }
}
+ });
+
+ if (!status.isOk()) {
+ return status;
}
+
return vectorToStableLargeParcelable(std::move(configs), output);
}
@@ -691,8 +727,8 @@
}
VhalResult<void> DefaultVehicleHal::checkSubscribeOptions(
- const std::vector<SubscribeOptions>& options) {
- const auto& configsByPropId = getConfigsByPropId();
+ const std::vector<SubscribeOptions>& options,
+ const std::unordered_map<int32_t, VehiclePropConfig>& configsByPropId) {
for (const auto& option : options) {
int32_t propId = option.propId;
auto it = configsByPropId.find(propId);
@@ -757,23 +793,15 @@
}
}
}
+
return {};
}
-ScopedAStatus DefaultVehicleHal::subscribe(const CallbackType& callback,
- const std::vector<SubscribeOptions>& options,
- [[maybe_unused]] int32_t maxSharedMemoryFileCount) {
- // TODO(b/205189110): Use shared memory file count.
- if (callback == nullptr) {
- return ScopedAStatus::fromExceptionCode(EX_NULL_POINTER);
- }
- if (auto result = checkSubscribeOptions(options); !result.ok()) {
- ALOGE("subscribe: invalid subscribe options: %s", getErrorMsg(result).c_str());
- return toScopedAStatus(result);
- }
- std::vector<SubscribeOptions> onChangeSubscriptions;
- std::vector<SubscribeOptions> continuousSubscriptions;
- const auto& configsByPropId = getConfigsByPropId();
+void DefaultVehicleHal::parseSubscribeOptions(
+ const std::vector<SubscribeOptions>& options,
+ const std::unordered_map<int32_t, VehiclePropConfig>& configsByPropId,
+ std::vector<SubscribeOptions>& onChangeSubscriptions,
+ std::vector<SubscribeOptions>& continuousSubscriptions) {
for (const auto& option : options) {
int32_t propId = option.propId;
// We have already validate config exists.
@@ -831,6 +859,34 @@
onChangeSubscriptions.push_back(std::move(optionCopy));
}
}
+}
+
+ScopedAStatus DefaultVehicleHal::subscribe(const CallbackType& callback,
+ const std::vector<SubscribeOptions>& options,
+ [[maybe_unused]] int32_t maxSharedMemoryFileCount) {
+ // TODO(b/205189110): Use shared memory file count.
+ if (callback == nullptr) {
+ return ScopedAStatus::fromExceptionCode(EX_NULL_POINTER);
+ }
+ std::vector<SubscribeOptions> onChangeSubscriptions;
+ std::vector<SubscribeOptions> continuousSubscriptions;
+ ScopedAStatus returnStatus = ScopedAStatus::ok();
+ getConfigsByPropId([this, &returnStatus, &options, &onChangeSubscriptions,
+ &continuousSubscriptions](const auto& configsByPropId) {
+ SharedScopedLockAssertion lockAssertion(mConfigLock);
+
+ if (auto result = checkSubscribeOptions(options, configsByPropId); !result.ok()) {
+ ALOGE("subscribe: invalid subscribe options: %s", getErrorMsg(result).c_str());
+ returnStatus = toScopedAStatus(result);
+ return;
+ }
+ parseSubscribeOptions(options, configsByPropId, onChangeSubscriptions,
+ continuousSubscriptions);
+ });
+
+ if (!returnStatus.isOk()) {
+ return returnStatus;
+ }
{
// Lock to make sure onBinderDied would not be called concurrently.
@@ -891,13 +947,13 @@
return StatusError(StatusCode::INVALID_ARG) << getErrorMsg(result);
}
- const VehiclePropConfig* config = result.value();
- const VehicleAreaConfig* areaConfig = getAreaConfig(value, *config);
+ const VehiclePropConfig& config = result.value();
+ const VehicleAreaConfig* areaConfig = getAreaConfig(value, config);
if (areaConfig == nullptr && !isGlobalProp(propId)) {
return StatusError(StatusCode::INVALID_ARG) << "no config for area ID: " << value.areaId;
}
- if (!hasRequiredAccess(config->access, accessToTest) &&
+ if (!hasRequiredAccess(config.access, accessToTest) &&
(areaConfig == nullptr || !hasRequiredAccess(areaConfig->access, accessToTest))) {
return StatusError(StatusCode::ACCESS_DENIED)
<< StringPrintf("Property %" PRId32 " does not have the following access: %" PRId32,
@@ -966,7 +1022,6 @@
}
DumpResult result = mVehicleHardware->dump(options);
if (result.refreshPropertyConfigs) {
- std::scoped_lock lockGuard(mConfigInitLock);
getAllPropConfigsFromHardwareLocked();
}
dprintf(fd, "%s", (result.buffer + "\n").c_str());
@@ -974,11 +1029,16 @@
return STATUS_OK;
}
dprintf(fd, "Vehicle HAL State: \n");
- const auto& configsByPropId = getConfigsByPropId();
+ std::unordered_map<int32_t, VehiclePropConfig> configsByPropIdCopy;
+ getConfigsByPropId([this, &configsByPropIdCopy](const auto& configsByPropId) {
+ SharedScopedLockAssertion lockAssertion(mConfigLock);
+
+ configsByPropIdCopy = configsByPropId;
+ });
{
std::scoped_lock<std::mutex> lockGuard(mLock);
dprintf(fd, "Interface version: %" PRId32 "\n", getVhalInterfaceVersion());
- dprintf(fd, "Containing %zu property configs\n", configsByPropId.size());
+ dprintf(fd, "Containing %zu property configs\n", configsByPropIdCopy.size());
dprintf(fd, "Currently have %zu getValues clients\n", mGetValuesClients.size());
dprintf(fd, "Currently have %zu setValues clients\n", mSetValuesClients.size());
dprintf(fd, "Currently have %zu subscribe clients\n", countSubscribeClients());