Untangle listeners mess in IncrementalService
Listeners and some binder call parameters were using several
different styles when passed around - copy, move, pointer,
pointer to pointer. This CL tries to 'normalize' that.
Bug: 183067554
Test: atest IncrementalServiceTest
Change-Id: Ia28089aa9e4491b0f28e3e747489199cfccb5a1b
diff --git a/services/incremental/BinderIncrementalService.cpp b/services/incremental/BinderIncrementalService.cpp
index 052b4dd..63488f9 100644
--- a/services/incremental/BinderIncrementalService.cpp
+++ b/services/incremental/BinderIncrementalService.cpp
@@ -118,9 +118,10 @@
binder::Status BinderIncrementalService::createStorage(
const ::std::string& path, const ::android::content::pm::DataLoaderParamsParcel& params,
int32_t createMode, int32_t* _aidl_return) {
- *_aidl_return = mImpl.createStorage(path, params,
- android::incremental::IncrementalService::CreateOptions(
- createMode));
+ *_aidl_return =
+ mImpl.createStorage(path, const_cast<content::pm::DataLoaderParamsParcel&&>(params),
+ android::incremental::IncrementalService::CreateOptions(
+ createMode));
return ok();
}
@@ -144,9 +145,8 @@
bool* _aidl_return) {
*_aidl_return =
mImpl.startLoading(storageId, const_cast<content::pm::DataLoaderParamsParcel&&>(params),
- statusListener,
- const_cast<StorageHealthCheckParams&&>(healthCheckParams),
- healthListener, perUidReadTimeouts);
+ statusListener, healthCheckParams, healthListener,
+ perUidReadTimeouts);
return ok();
}
@@ -334,10 +334,8 @@
int32_t storageId,
const ::android::os::incremental::StorageHealthCheckParams& healthCheckParams,
const ::android::sp<IStorageHealthListener>& healthListener, bool* _aidl_return) {
- *_aidl_return = mImpl.registerStorageHealthListener(storageId,
- const_cast<StorageHealthCheckParams&&>(
- healthCheckParams),
- healthListener);
+ *_aidl_return =
+ mImpl.registerStorageHealthListener(storageId, healthCheckParams, healthListener);
return ok();
}
diff --git a/services/incremental/IncrementalService.cpp b/services/incremental/IncrementalService.cpp
index 958eb15..a88f2b4 100644
--- a/services/incremental/IncrementalService.cpp
+++ b/services/incremental/IncrementalService.cpp
@@ -474,9 +474,9 @@
}
}
-StorageId IncrementalService::createStorage(
- std::string_view mountPoint, const content::pm::DataLoaderParamsParcel& dataLoaderParams,
- CreateOptions options) {
+StorageId IncrementalService::createStorage(std::string_view mountPoint,
+ content::pm::DataLoaderParamsParcel dataLoaderParams,
+ CreateOptions options) {
LOG(INFO) << "createStorage: " << mountPoint << " | " << int(options);
if (!path::isAbsolute(mountPoint)) {
LOG(ERROR) << "path is not absolute: " << mountPoint;
@@ -584,9 +584,9 @@
metadata::Mount m;
m.mutable_storage()->set_id(ifs->mountId);
m.mutable_loader()->set_type((int)dataLoaderParams.type);
- m.mutable_loader()->set_package_name(dataLoaderParams.packageName);
- m.mutable_loader()->set_class_name(dataLoaderParams.className);
- m.mutable_loader()->set_arguments(dataLoaderParams.arguments);
+ m.mutable_loader()->set_package_name(std::move(dataLoaderParams.packageName));
+ m.mutable_loader()->set_class_name(std::move(dataLoaderParams.className));
+ m.mutable_loader()->set_arguments(std::move(dataLoaderParams.arguments));
const auto metadata = m.SerializeAsString();
if (auto err =
mIncFs->makeFile(ifs->control,
@@ -661,14 +661,14 @@
}
bool IncrementalService::startLoading(StorageId storageId,
- content::pm::DataLoaderParamsParcel&& dataLoaderParams,
- const DataLoaderStatusListener& statusListener,
- StorageHealthCheckParams&& healthCheckParams,
- const StorageHealthListener& healthListener,
- const std::vector<PerUidReadTimeouts>& perUidReadTimeouts) {
+ content::pm::DataLoaderParamsParcel dataLoaderParams,
+ DataLoaderStatusListener statusListener,
+ const StorageHealthCheckParams& healthCheckParams,
+ StorageHealthListener healthListener,
+ std::vector<PerUidReadTimeouts> perUidReadTimeouts) {
// Per Uid timeouts.
if (!perUidReadTimeouts.empty()) {
- setUidReadTimeouts(storageId, perUidReadTimeouts);
+ setUidReadTimeouts(storageId, std::move(perUidReadTimeouts));
}
// Re-initialize DataLoader.
@@ -684,8 +684,9 @@
l.unlock();
// DataLoader.
- auto dataLoaderStub = prepareDataLoader(*ifs, std::move(dataLoaderParams), &statusListener,
- std::move(healthCheckParams), &healthListener);
+ auto dataLoaderStub =
+ prepareDataLoader(*ifs, std::move(dataLoaderParams), std::move(statusListener),
+ healthCheckParams, std::move(healthListener));
CHECK(dataLoaderStub);
if (dataLoaderStub->isSystemDataLoader()) {
@@ -1196,8 +1197,8 @@
return mIncFs->getMetadata(ifs->control, node);
}
-void IncrementalService::setUidReadTimeouts(
- StorageId storage, const std::vector<PerUidReadTimeouts>& perUidReadTimeouts) {
+void IncrementalService::setUidReadTimeouts(StorageId storage,
+ std::vector<PerUidReadTimeouts>&& perUidReadTimeouts) {
using microseconds = std::chrono::microseconds;
using milliseconds = std::chrono::milliseconds;
@@ -1615,19 +1616,18 @@
}
IncrementalService::DataLoaderStubPtr IncrementalService::prepareDataLoader(
- IncFsMount& ifs, DataLoaderParamsParcel&& params,
- const DataLoaderStatusListener* statusListener,
- StorageHealthCheckParams&& healthCheckParams, const StorageHealthListener* healthListener) {
+ IncFsMount& ifs, DataLoaderParamsParcel&& params, DataLoaderStatusListener&& statusListener,
+ const StorageHealthCheckParams& healthCheckParams, StorageHealthListener&& healthListener) {
std::unique_lock l(ifs.lock);
- prepareDataLoaderLocked(ifs, std::move(params), statusListener, std::move(healthCheckParams),
- healthListener);
+ prepareDataLoaderLocked(ifs, std::move(params), std::move(statusListener), healthCheckParams,
+ std::move(healthListener));
return ifs.dataLoaderStub;
}
void IncrementalService::prepareDataLoaderLocked(IncFsMount& ifs, DataLoaderParamsParcel&& params,
- const DataLoaderStatusListener* statusListener,
- StorageHealthCheckParams&& healthCheckParams,
- const StorageHealthListener* healthListener) {
+ DataLoaderStatusListener&& statusListener,
+ const StorageHealthCheckParams& healthCheckParams,
+ StorageHealthListener&& healthListener) {
if (ifs.dataLoaderStub) {
LOG(INFO) << "Skipped data loader preparation because it already exists";
return;
@@ -1645,8 +1645,8 @@
ifs.dataLoaderStub =
new DataLoaderStub(*this, ifs.mountId, std::move(params), std::move(fsControlParcel),
- statusListener, std::move(healthCheckParams), healthListener,
- path::join(ifs.root, constants().mount));
+ std::move(statusListener), healthCheckParams,
+ std::move(healthListener), path::join(ifs.root, constants().mount));
}
template <class Duration>
@@ -2036,8 +2036,8 @@
return error ? LoadingProgress{error, error} : LoadingProgress{filledBlocks, totalBlocks};
}
-bool IncrementalService::updateLoadingProgress(
- StorageId storage, const StorageLoadingProgressListener& progressListener) {
+bool IncrementalService::updateLoadingProgress(StorageId storage,
+ StorageLoadingProgressListener&& progressListener) {
const auto progress = getLoadingProgress(storage);
if (progress.isError()) {
// Failed to get progress from incfs, abort.
@@ -2050,15 +2050,15 @@
}
addTimedJob(*mProgressUpdateJobQueue, storage,
Constants::progressUpdateInterval /* repeat after 1s */,
- [storage, progressListener, this]() {
- updateLoadingProgress(storage, progressListener);
+ [storage, progressListener = std::move(progressListener), this]() mutable {
+ updateLoadingProgress(storage, std::move(progressListener));
});
return true;
}
bool IncrementalService::registerLoadingProgressListener(
- StorageId storage, const StorageLoadingProgressListener& progressListener) {
- return updateLoadingProgress(storage, progressListener);
+ StorageId storage, StorageLoadingProgressListener progressListener) {
+ return updateLoadingProgress(storage, std::move(progressListener));
}
bool IncrementalService::unregisterLoadingProgressListener(StorageId storage) {
@@ -2066,8 +2066,8 @@
}
bool IncrementalService::registerStorageHealthListener(
- StorageId storage, StorageHealthCheckParams&& healthCheckParams,
- const StorageHealthListener& healthListener) {
+ StorageId storage, const StorageHealthCheckParams& healthCheckParams,
+ StorageHealthListener healthListener) {
DataLoaderStubPtr dataLoaderStub;
{
std::unique_lock l(mLock);
@@ -2080,14 +2080,12 @@
return false;
}
}
- dataLoaderStub->setHealthListener(std::move(healthCheckParams), &healthListener);
+ dataLoaderStub->setHealthListener(healthCheckParams, std::move(healthListener));
return true;
}
void IncrementalService::unregisterStorageHealthListener(StorageId storage) {
- StorageHealthCheckParams invalidCheckParams;
- invalidCheckParams.blockedTimeoutMs = -1;
- registerStorageHealthListener(storage, std::move(invalidCheckParams), {});
+ registerStorageHealthListener(storage, {}, {});
}
bool IncrementalService::perfLoggingEnabled() {
@@ -2212,26 +2210,23 @@
return ifs->dataLoaderStub->elapsedMsSinceOldestPendingRead();
}
-IncrementalService::DataLoaderStub::DataLoaderStub(IncrementalService& service, MountId id,
- DataLoaderParamsParcel&& params,
- FileSystemControlParcel&& control,
- const DataLoaderStatusListener* statusListener,
- StorageHealthCheckParams&& healthCheckParams,
- const StorageHealthListener* healthListener,
- std::string&& healthPath)
+IncrementalService::DataLoaderStub::DataLoaderStub(
+ IncrementalService& service, MountId id, DataLoaderParamsParcel&& params,
+ FileSystemControlParcel&& control, DataLoaderStatusListener&& statusListener,
+ const StorageHealthCheckParams& healthCheckParams, StorageHealthListener&& healthListener,
+ std::string&& healthPath)
: mService(service),
mId(id),
mParams(std::move(params)),
mControl(std::move(control)),
- mStatusListener(statusListener ? *statusListener : DataLoaderStatusListener()),
- mHealthListener(healthListener ? *healthListener : StorageHealthListener()),
+ mStatusListener(std::move(statusListener)),
+ mHealthListener(std::move(healthListener)),
mHealthPath(std::move(healthPath)),
- mHealthCheckParams(std::move(healthCheckParams)) {
- if (mHealthListener) {
- if (!isHealthParamsValid()) {
- mHealthListener = {};
- }
- } else {
+ mHealthCheckParams(healthCheckParams) {
+ if (mHealthListener && !isHealthParamsValid()) {
+ mHealthListener = {};
+ }
+ if (!mHealthListener) {
// Disable advanced health check statuses.
mHealthCheckParams.blockedTimeoutMs = -1;
}
@@ -2800,14 +2795,12 @@
}
void IncrementalService::DataLoaderStub::setHealthListener(
- StorageHealthCheckParams&& healthCheckParams, const StorageHealthListener* healthListener) {
+ const StorageHealthCheckParams& healthCheckParams, StorageHealthListener&& healthListener) {
std::lock_guard lock(mMutex);
- mHealthCheckParams = std::move(healthCheckParams);
- if (healthListener == nullptr) {
- // reset listener and params
- mHealthListener = {};
- } else {
- mHealthListener = *healthListener;
+ mHealthCheckParams = healthCheckParams;
+ mHealthListener = std::move(healthListener);
+ if (!mHealthListener) {
+ mHealthCheckParams.blockedTimeoutMs = -1;
}
}
diff --git a/services/incremental/IncrementalService.h b/services/incremental/IncrementalService.h
index c7c682f2..4a5db06 100644
--- a/services/incremental/IncrementalService.h
+++ b/services/incremental/IncrementalService.h
@@ -136,17 +136,17 @@
void onSystemReady();
StorageId createStorage(std::string_view mountPoint,
- const content::pm::DataLoaderParamsParcel& dataLoaderParams,
+ content::pm::DataLoaderParamsParcel dataLoaderParams,
CreateOptions options);
StorageId createLinkedStorage(std::string_view mountPoint, StorageId linkedStorage,
CreateOptions options = CreateOptions::Default);
StorageId openStorage(std::string_view path);
- bool startLoading(StorageId storage, content::pm::DataLoaderParamsParcel&& dataLoaderParams,
- const DataLoaderStatusListener& statusListener,
- StorageHealthCheckParams&& healthCheckParams,
- const StorageHealthListener& healthListener,
- const std::vector<PerUidReadTimeouts>& perUidReadTimeouts);
+ bool startLoading(StorageId storage, content::pm::DataLoaderParamsParcel dataLoaderParams,
+ DataLoaderStatusListener statusListener,
+ const StorageHealthCheckParams& healthCheckParams,
+ StorageHealthListener healthListener,
+ std::vector<PerUidReadTimeouts> perUidReadTimeouts);
int bind(StorageId storage, std::string_view source, std::string_view target, BindKind kind);
int unbind(StorageId storage, std::string_view target);
@@ -170,11 +170,11 @@
LoadingProgress getLoadingProgress(StorageId storage) const;
bool registerLoadingProgressListener(StorageId storage,
- const StorageLoadingProgressListener& progressListener);
+ StorageLoadingProgressListener progressListener);
bool unregisterLoadingProgressListener(StorageId storage);
bool registerStorageHealthListener(StorageId storage,
- StorageHealthCheckParams&& healthCheckParams,
- const StorageHealthListener& healthListener);
+ const StorageHealthCheckParams& healthCheckParams,
+ StorageHealthListener healthListener);
void unregisterStorageHealthListener(StorageId storage);
RawMetadata getMetadata(StorageId storage, std::string_view path) const;
RawMetadata getMetadata(StorageId storage, FileId node) const;
@@ -216,9 +216,9 @@
DataLoaderStub(IncrementalService& service, MountId id,
content::pm::DataLoaderParamsParcel&& params,
content::pm::FileSystemControlParcel&& control,
- const DataLoaderStatusListener* statusListener,
- StorageHealthCheckParams&& healthCheckParams,
- const StorageHealthListener* healthListener, std::string&& healthPath);
+ DataLoaderStatusListener&& statusListener,
+ const StorageHealthCheckParams& healthCheckParams,
+ StorageHealthListener&& healthListener, std::string&& healthPath);
~DataLoaderStub();
// Cleans up the internal state and invalidates DataLoaderStub. Any subsequent calls will
// result in an error.
@@ -233,8 +233,8 @@
MountId id() const { return mId.load(std::memory_order_relaxed); }
const content::pm::DataLoaderParamsParcel& params() const { return mParams; }
bool isSystemDataLoader() const;
- void setHealthListener(StorageHealthCheckParams&& healthCheckParams,
- const StorageHealthListener* healthListener);
+ void setHealthListener(const StorageHealthCheckParams& healthCheckParams,
+ StorageHealthListener&& healthListener);
long elapsedMsSinceOldestPendingRead();
private:
@@ -364,7 +364,7 @@
static bool perfLoggingEnabled();
void setUidReadTimeouts(StorageId storage,
- const std::vector<PerUidReadTimeouts>& perUidReadTimeouts);
+ std::vector<PerUidReadTimeouts>&& perUidReadTimeouts);
void clearUidReadTimeouts(StorageId storage);
void updateUidReadTimeouts(StorageId storage, Clock::time_point timeLimit);
@@ -389,13 +389,13 @@
DataLoaderStubPtr prepareDataLoader(IncFsMount& ifs,
content::pm::DataLoaderParamsParcel&& params,
- const DataLoaderStatusListener* statusListener = nullptr,
- StorageHealthCheckParams&& healthCheckParams = {},
- const StorageHealthListener* healthListener = nullptr);
+ DataLoaderStatusListener&& statusListener = {},
+ const StorageHealthCheckParams& healthCheckParams = {},
+ StorageHealthListener&& healthListener = {});
void prepareDataLoaderLocked(IncFsMount& ifs, content::pm::DataLoaderParamsParcel&& params,
- const DataLoaderStatusListener* statusListener = nullptr,
- StorageHealthCheckParams&& healthCheckParams = {},
- const StorageHealthListener* healthListener = nullptr);
+ DataLoaderStatusListener&& statusListener = {},
+ const StorageHealthCheckParams& healthCheckParams = {},
+ StorageHealthListener&& healthListener = {});
BindPathMap::const_iterator findStorageLocked(std::string_view path) const;
StorageId findStorageId(std::string_view path) const;
@@ -432,7 +432,7 @@
bool addTimedJob(TimedQueueWrapper& timedQueue, MountId id, Milliseconds after, Job what);
bool removeTimedJobs(TimedQueueWrapper& timedQueue, MountId id);
bool updateLoadingProgress(int32_t storageId,
- const StorageLoadingProgressListener& progressListener);
+ StorageLoadingProgressListener&& progressListener);
long getMillsSinceOldestPendingRead(StorageId storage);
private:
@@ -454,7 +454,7 @@
BindPathMap mBindsByPath;
std::mutex mCallbacksLock;
- std::map<std::string, sp<AppOpsListener>> mCallbackRegistered;
+ std::unordered_map<std::string, sp<AppOpsListener>> mCallbackRegistered;
std::atomic_bool mSystemReady = false;
StorageId mNextId = 0;