Refactor: move dataLoader details to a separate class.
Bug: b/151241369
Test: atest PackageManagerShellCommandTest PackageManagerShellCommandIncrementalTest IncrementalServiceTest
Change-Id: If4e7d3bb9f0951dcaee4315f4be525883b7fc333
diff --git a/services/incremental/IncrementalService.cpp b/services/incremental/IncrementalService.cpp
index 92366e5..30c2cfd 100644
--- a/services/incremental/IncrementalService.cpp
+++ b/services/incremental/IncrementalService.cpp
@@ -163,7 +163,9 @@
android::base::GetBoolProperty("incremental.perflogging", false);
IncrementalService::IncFsMount::~IncFsMount() {
- incrementalService.mDataLoaderManager->destroyDataLoader(mountId);
+ if (dataLoaderStub) {
+ dataLoaderStub->destroy();
+ }
LOG(INFO) << "Unmounting and cleaning up mount " << mountId << " with root '" << root << '\'';
for (auto&& [target, _] : bindPoints) {
LOG(INFO) << "\tbind: " << target;
@@ -288,9 +290,12 @@
dprintf(fd, "\t\tmountId: %d\n", mnt.mountId);
dprintf(fd, "\t\troot: %s\n", mnt.root.c_str());
dprintf(fd, "\t\tnextStorageDirNo: %d\n", mnt.nextStorageDirNo.load());
- dprintf(fd, "\t\tdataLoaderStatus: %d\n", mnt.dataLoaderStatus.load());
- {
- const auto& params = mnt.dataLoaderParams;
+ if (mnt.dataLoaderStub) {
+ const auto& dataLoaderStub = *mnt.dataLoaderStub;
+ dprintf(fd, "\t\tdataLoaderStatus: %d\n", dataLoaderStub.status());
+ dprintf(fd, "\t\tdataLoaderStartRequested: %s\n",
+ dataLoaderStub.startRequested() ? "true" : "false");
+ const auto& params = dataLoaderStub.params();
dprintf(fd, "\t\tdataLoaderParams:\n");
dprintf(fd, "\t\t\ttype: %s\n", toString(params.type).c_str());
dprintf(fd, "\t\t\tpackageName: %s\n", params.packageName.c_str());
@@ -321,10 +326,9 @@
}
}
-std::optional<std::future<void>> IncrementalService::onSystemReady() {
- std::promise<void> threadFinished;
+void IncrementalService::onSystemReady() {
if (mSystemReady.exchange(true)) {
- return {};
+ return;
}
std::vector<IfsMountPtr> mounts;
@@ -338,8 +342,8 @@
}
}
+ /* TODO(b/151241369): restore data loaders on reboot.
std::thread([this, mounts = std::move(mounts)]() {
- /* TODO(b/151241369): restore data loaders on reboot.
for (auto&& ifs : mounts) {
if (prepareDataLoader(*ifs)) {
LOG(INFO) << "Successfully started data loader for mount " << ifs->mountId;
@@ -348,10 +352,8 @@
LOG(WARNING) << "Failed to start data loader for mount " << ifs->mountId;
}
}
- */
- mPrepareDataLoaders.set_value_at_thread_exit();
}).detach();
- return mPrepareDataLoaders.get_future();
+ */
}
auto IncrementalService::getStorageSlotLocked() -> MountMap::iterator {
@@ -468,15 +470,13 @@
return kInvalidStorageId;
}
- ifs->dataLoaderParams = std::move(dataLoaderParams);
-
{
metadata::Mount m;
m.mutable_storage()->set_id(ifs->mountId);
- m.mutable_loader()->set_type((int)ifs->dataLoaderParams.type);
- m.mutable_loader()->set_package_name(ifs->dataLoaderParams.packageName);
- m.mutable_loader()->set_class_name(ifs->dataLoaderParams.className);
- m.mutable_loader()->set_arguments(ifs->dataLoaderParams.arguments);
+ 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);
const auto metadata = m.SerializeAsString();
m.mutable_loader()->release_arguments();
m.mutable_loader()->release_class_name();
@@ -504,14 +504,20 @@
// Done here as well, all data structures are in good state.
secondCleanupOnFailure.release();
- if (!prepareDataLoader(*ifs, &dataLoaderStatusListener)) {
- LOG(ERROR) << "prepareDataLoader() failed";
- deleteStorageLocked(*ifs, std::move(l));
- return kInvalidStorageId;
- }
+ auto dataLoaderStub =
+ prepareDataLoader(*ifs, std::move(dataLoaderParams), &dataLoaderStatusListener);
+ CHECK(dataLoaderStub);
mountIt->second = std::move(ifs);
l.unlock();
+
+ if (mSystemReady.load(std::memory_order_relaxed) && !dataLoaderStub->create()) {
+ // failed to create data loader
+ LOG(ERROR) << "initializeDataLoader() failed";
+ deleteStorage(dataLoaderStub->id());
+ return kInvalidStorageId;
+ }
+
LOG(INFO) << "created storage " << mountId;
return mountId;
}
@@ -585,10 +591,10 @@
return -EINVAL;
}
+ const auto& params = ifs->dataLoaderStub->params();
if (enableReadLogs) {
- if (auto status =
- mAppOpsManager->checkPermission(kDataUsageStats, kOpUsage,
- ifs->dataLoaderParams.packageName.c_str());
+ if (auto status = mAppOpsManager->checkPermission(kDataUsageStats, kOpUsage,
+ params.packageName.c_str());
!status.isOk()) {
LOG(ERROR) << "checkPermission failed: " << status.toString8();
return fromBinderStatus(status);
@@ -601,7 +607,7 @@
}
if (enableReadLogs) {
- registerAppOpsCallback(ifs->dataLoaderParams.packageName);
+ registerAppOpsCallback(params.packageName);
}
return 0;
@@ -984,34 +990,19 @@
}
bool IncrementalService::startLoading(StorageId storage) const {
+ DataLoaderStubPtr dataLoaderStub;
{
std::unique_lock l(mLock);
const auto& ifs = getIfsLocked(storage);
if (!ifs) {
return false;
}
- if (ifs->dataLoaderStatus != IDataLoaderStatusListener::DATA_LOADER_CREATED) {
- ifs->dataLoaderStartRequested = true;
- return true;
+ dataLoaderStub = ifs->dataLoaderStub;
+ if (!dataLoaderStub) {
+ return false;
}
}
- return startDataLoader(storage);
-}
-
-bool IncrementalService::startDataLoader(MountId mountId) const {
- sp<IDataLoader> dataloader;
- auto status = mDataLoaderManager->getDataLoader(mountId, &dataloader);
- if (!status.isOk()) {
- return false;
- }
- if (!dataloader) {
- return false;
- }
- status = dataloader->start(mountId);
- if (!status.isOk()) {
- return false;
- }
- return true;
+ return dataLoaderStub->start();
}
void IncrementalService::mountExistingImages() {
@@ -1057,13 +1048,13 @@
mNextId = std::max(mNextId, ifs->mountId + 1);
// DataLoader params
+ DataLoaderParamsParcel dataLoaderParams;
{
- auto& dlp = ifs->dataLoaderParams;
const auto& loader = mount.loader();
- dlp.type = (android::content::pm::DataLoaderType)loader.type();
- dlp.packageName = loader.package_name();
- dlp.className = loader.class_name();
- dlp.arguments = loader.arguments();
+ dataLoaderParams.type = (android::content::pm::DataLoaderType)loader.type();
+ dataLoaderParams.packageName = loader.package_name();
+ dataLoaderParams.className = loader.class_name();
+ dataLoaderParams.arguments = loader.arguments();
}
std::vector<std::pair<std::string, metadata::BindPoint>> bindPoints;
@@ -1135,17 +1126,13 @@
return true;
}
-bool IncrementalService::prepareDataLoader(IncrementalService::IncFsMount& ifs,
- const DataLoaderStatusListener* externalListener) {
- if (!mSystemReady.load(std::memory_order_relaxed)) {
- std::unique_lock l(ifs.lock);
- return true; // eventually...
- }
-
+IncrementalService::DataLoaderStubPtr IncrementalService::prepareDataLoader(
+ IncrementalService::IncFsMount& ifs, DataLoaderParamsParcel&& params,
+ const DataLoaderStatusListener* externalListener) {
std::unique_lock l(ifs.lock);
- if (ifs.dataLoaderStatus != -1) {
+ if (ifs.dataLoaderStub) {
LOG(INFO) << "Skipped data loader preparation because it already exists";
- return true;
+ return ifs.dataLoaderStub;
}
FileSystemControlParcel fsControlParcel;
@@ -1155,17 +1142,10 @@
base::unique_fd(::dup(ifs.control.pendingReads())));
fsControlParcel.incremental->log.reset(base::unique_fd(::dup(ifs.control.logs())));
fsControlParcel.service = new IncrementalServiceConnector(*this, ifs.mountId);
- sp<IncrementalDataLoaderListener> listener =
- new IncrementalDataLoaderListener(*this,
- externalListener ? *externalListener
- : DataLoaderStatusListener());
- bool created = false;
- auto status = mDataLoaderManager->initializeDataLoader(ifs.mountId, ifs.dataLoaderParams, fsControlParcel, listener, &created);
- if (!status.isOk() || !created) {
- LOG(ERROR) << "Failed to create a data loader for mount " << ifs.mountId;
- return false;
- }
- return true;
+
+ ifs.dataLoaderStub = new DataLoaderStub(*this, ifs.mountId, std::move(params),
+ std::move(fsControlParcel), externalListener);
+ return ifs.dataLoaderStub;
}
template <class Duration>
@@ -1376,7 +1356,7 @@
std::lock_guard l(mLock);
affected.reserve(mMounts.size());
for (auto&& [id, ifs] : mMounts) {
- if (ifs->mountId == id && ifs->dataLoaderParams.packageName == packageName) {
+ if (ifs->mountId == id && ifs->dataLoaderStub->params().packageName == packageName) {
affected.push_back(ifs);
}
}
@@ -1386,37 +1366,79 @@
}
}
-binder::Status IncrementalService::IncrementalDataLoaderListener::onStatusChanged(MountId mountId,
- int newStatus) {
- if (externalListener) {
- // Give an external listener a chance to act before we destroy something.
- externalListener->onStatusChanged(mountId, newStatus);
+IncrementalService::DataLoaderStub::~DataLoaderStub() {
+ CHECK(mStatus == -1 || mStatus == IDataLoaderStatusListener::DATA_LOADER_DESTROYED)
+ << "Dataloader has to be destroyed prior to destructor: " << mId
+ << ", status: " << mStatus;
+}
+
+bool IncrementalService::DataLoaderStub::create() {
+ bool created = false;
+ auto status = mService.mDataLoaderManager->initializeDataLoader(mId, mParams, mControl, this,
+ &created);
+ if (!status.isOk() || !created) {
+ LOG(ERROR) << "Failed to create a data loader for mount " << mId;
+ return false;
+ }
+ return true;
+}
+
+bool IncrementalService::DataLoaderStub::start() {
+ if (mStatus != IDataLoaderStatusListener::DATA_LOADER_CREATED) {
+ mStartRequested = true;
+ return true;
}
- bool startRequested = false;
+ sp<IDataLoader> dataloader;
+ auto status = mService.mDataLoaderManager->getDataLoader(mId, &dataloader);
+ if (!status.isOk()) {
+ return false;
+ }
+ if (!dataloader) {
+ return false;
+ }
+ status = dataloader->start(mId);
+ if (!status.isOk()) {
+ return false;
+ }
+ return true;
+}
+
+void IncrementalService::DataLoaderStub::destroy() {
+ mDestroyRequested = true;
+ mService.mDataLoaderManager->destroyDataLoader(mId);
+}
+
+binder::Status IncrementalService::DataLoaderStub::onStatusChanged(MountId mountId, int newStatus) {
+ if (mStatus == newStatus) {
+ return binder::Status::ok();
+ }
+
+ if (mListener) {
+ // Give an external listener a chance to act before we destroy something.
+ mListener->onStatusChanged(mountId, newStatus);
+ }
+
{
- std::unique_lock l(incrementalService.mLock);
- const auto& ifs = incrementalService.getIfsLocked(mountId);
+ std::unique_lock l(mService.mLock);
+ const auto& ifs = mService.getIfsLocked(mountId);
if (!ifs) {
LOG(WARNING) << "Received data loader status " << int(newStatus)
<< " for unknown mount " << mountId;
return binder::Status::ok();
}
- ifs->dataLoaderStatus = newStatus;
+ mStatus = newStatus;
- if (newStatus == IDataLoaderStatusListener::DATA_LOADER_DESTROYED) {
- ifs->dataLoaderStatus = IDataLoaderStatusListener::DATA_LOADER_STOPPED;
- incrementalService.deleteStorageLocked(*ifs, std::move(l));
+ if (!mDestroyRequested && newStatus == IDataLoaderStatusListener::DATA_LOADER_DESTROYED) {
+ mService.deleteStorageLocked(*ifs, std::move(l));
return binder::Status::ok();
}
-
- startRequested = ifs->dataLoaderStartRequested;
}
switch (newStatus) {
case IDataLoaderStatusListener::DATA_LOADER_CREATED: {
- if (startRequested) {
- incrementalService.startDataLoader(mountId);
+ if (mStartRequested) {
+ start();
}
break;
}
