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;
}
diff --git a/services/incremental/IncrementalService.h b/services/incremental/IncrementalService.h
index db14a79..27d40f1 100644
--- a/services/incremental/IncrementalService.h
+++ b/services/incremental/IncrementalService.h
@@ -60,7 +60,8 @@
using TimePoint = std::chrono::time_point<Clock>;
using Seconds = std::chrono::seconds;
-using DataLoaderStatusListener = ::android::sp<::android::content::pm::IDataLoaderStatusListener>;
+using IDataLoaderStatusListener = ::android::content::pm::IDataLoaderStatusListener;
+using DataLoaderStatusListener = ::android::sp<IDataLoaderStatusListener>;
class IncrementalService final {
public:
@@ -95,7 +96,7 @@
void onDump(int fd);
- std::optional<std::future<void>> onSystemReady();
+ void onSystemReady();
StorageId createStorage(std::string_view mountPoint, DataLoaderParamsParcel&& dataLoaderParams,
const DataLoaderStatusListener& dataLoaderStatusListener,
@@ -134,19 +135,6 @@
bool configureNativeBinaries(StorageId storage, std::string_view apkFullPath,
std::string_view libDirRelativePath, std::string_view abi);
- class IncrementalDataLoaderListener : public android::content::pm::BnDataLoaderStatusListener {
- public:
- IncrementalDataLoaderListener(IncrementalService& incrementalService,
- DataLoaderStatusListener externalListener)
- : incrementalService(incrementalService), externalListener(externalListener) {}
- // Callbacks interface
- binder::Status onStatusChanged(MountId mount, int newStatus) final;
-
- private:
- IncrementalService& incrementalService;
- DataLoaderStatusListener externalListener;
- };
-
class AppOpsListener : public android::BnAppOpsCallback {
public:
AppOpsListener(IncrementalService& incrementalService, std::string packageName) : incrementalService(incrementalService), packageName(std::move(packageName)) {}
@@ -171,6 +159,45 @@
private:
static const bool sEnablePerfLogging;
+ struct IncFsMount;
+
+ class DataLoaderStub : public android::content::pm::BnDataLoaderStatusListener {
+ public:
+ DataLoaderStub(IncrementalService& service, MountId id, DataLoaderParamsParcel&& params,
+ FileSystemControlParcel&& control,
+ const DataLoaderStatusListener* externalListener)
+ : mService(service),
+ mId(id),
+ mParams(std::move(params)),
+ mControl(std::move(control)),
+ mListener(externalListener ? *externalListener : DataLoaderStatusListener()) {}
+ ~DataLoaderStub();
+
+ bool create();
+ bool start();
+ void destroy();
+
+ // accessors
+ MountId id() const { return mId; }
+ const DataLoaderParamsParcel& params() const { return mParams; }
+ int status() const { return mStatus.load(); }
+ bool startRequested() const { return mStartRequested; }
+
+ private:
+ binder::Status onStatusChanged(MountId mount, int newStatus) final;
+
+ IncrementalService& mService;
+ MountId const mId;
+ DataLoaderParamsParcel const mParams;
+ FileSystemControlParcel const mControl;
+ DataLoaderStatusListener const mListener;
+
+ std::atomic<int> mStatus = -1;
+ bool mStartRequested = false;
+ bool mDestroyRequested = false;
+ };
+ using DataLoaderStubPtr = sp<DataLoaderStub>;
+
struct IncFsMount {
struct Bind {
StorageId storage;
@@ -194,10 +221,8 @@
/*const*/ MountId mountId;
StorageMap storages;
BindMap bindPoints;
- DataLoaderParamsParcel dataLoaderParams;
+ DataLoaderStubPtr dataLoaderStub;
std::atomic<int> nextStorageDirNo{0};
- std::atomic<int> dataLoaderStatus = -1;
- bool dataLoaderStartRequested = false;
const IncrementalService& incrementalService;
IncFsMount(std::string root, MountId mountId, Control control,
@@ -232,8 +257,8 @@
std::string&& source, std::string&& target, BindKind kind,
std::unique_lock<std::mutex>& mainLock);
- bool prepareDataLoader(IncFsMount& ifs, const DataLoaderStatusListener* externalListener = nullptr);
- bool startDataLoader(MountId mountId) const;
+ DataLoaderStubPtr prepareDataLoader(IncFsMount& ifs, DataLoaderParamsParcel&& params,
+ const DataLoaderStatusListener* externalListener = nullptr);
BindPathMap::const_iterator findStorageLocked(std::string_view path) const;
StorageId findStorageId(std::string_view path) const;
@@ -269,7 +294,6 @@
std::atomic_bool mSystemReady = false;
StorageId mNextId = 0;
- std::promise<void> mPrepareDataLoaders;
};
} // namespace android::incremental
diff --git a/services/incremental/test/IncrementalServiceTest.cpp b/services/incremental/test/IncrementalServiceTest.cpp
index 18ae4b5..9911319 100644
--- a/services/incremental/test/IncrementalServiceTest.cpp
+++ b/services/incremental/test/IncrementalServiceTest.cpp
@@ -131,6 +131,23 @@
binder::Status(int32_t mountId, sp<IDataLoader>* _aidl_return));
MOCK_CONST_METHOD1(destroyDataLoader, binder::Status(int32_t mountId));
+ void initializeDataLoaderSuccess() {
+ ON_CALL(*this, initializeDataLoader(_, _, _, _, _))
+ .WillByDefault(Invoke(this, &MockDataLoaderManager::initializeDataLoaderOk));
+ }
+ void initializeDataLoaderFails() {
+ ON_CALL(*this, initializeDataLoader(_, _, _, _, _))
+ .WillByDefault(Return(
+ (binder::Status::fromExceptionCode(1, String8("failed to prepare")))));
+ }
+ void getDataLoaderSuccess() {
+ ON_CALL(*this, getDataLoader(_, _))
+ .WillByDefault(Invoke(this, &MockDataLoaderManager::getDataLoaderOk));
+ }
+ void destroyDataLoaderOk() {
+ ON_CALL(*this, destroyDataLoader(_))
+ .WillByDefault(Invoke(this, &MockDataLoaderManager::setDataLoaderStatusDestroyed));
+ }
binder::Status initializeDataLoaderOk(int32_t mountId, const DataLoaderParamsParcel& params,
const FileSystemControlParcel& control,
const sp<IDataLoaderStatusListener>& listener,
@@ -141,32 +158,22 @@
*_aidl_return = true;
return binder::Status::ok();
}
-
binder::Status getDataLoaderOk(int32_t mountId, sp<IDataLoader>* _aidl_return) {
*_aidl_return = mDataLoader;
return binder::Status::ok();
}
-
- void initializeDataLoaderFails() {
- ON_CALL(*this, initializeDataLoader(_, _, _, _, _))
- .WillByDefault(Return(
- (binder::Status::fromExceptionCode(1, String8("failed to prepare")))));
- }
- void initializeDataLoaderSuccess() {
- ON_CALL(*this, initializeDataLoader(_, _, _, _, _))
- .WillByDefault(Invoke(this, &MockDataLoaderManager::initializeDataLoaderOk));
- }
- void getDataLoaderSuccess() {
- ON_CALL(*this, getDataLoader(_, _))
- .WillByDefault(Invoke(this, &MockDataLoaderManager::getDataLoaderOk));
- }
void setDataLoaderStatusNotReady() {
mListener->onStatusChanged(mId, IDataLoaderStatusListener::DATA_LOADER_DESTROYED);
}
void setDataLoaderStatusReady() {
mListener->onStatusChanged(mId, IDataLoaderStatusListener::DATA_LOADER_CREATED);
}
-
+ binder::Status setDataLoaderStatusDestroyed(int32_t id) {
+ if (mListener) {
+ mListener->onStatusChanged(id, IDataLoaderStatusListener::DATA_LOADER_DESTROYED);
+ }
+ return binder::Status::ok();
+ }
int32_t setStorageParams(bool enableReadLogs) {
int32_t result = -1;
EXPECT_NE(mServiceConnector.get(), nullptr);
@@ -299,6 +306,7 @@
mRootDir.path);
mDataLoaderParcel.packageName = "com.test";
mDataLoaderParcel.arguments = "uri";
+ mDataLoaderManager->destroyDataLoaderOk();
mIncrementalService->onSystemReady();
}
@@ -346,6 +354,7 @@
TEST_F(IncrementalServiceTest, testCreateStorageMountIncFsInvalidControlParcel) {
mVold->mountIncFsInvalidControlParcel();
EXPECT_CALL(*mDataLoaderManager, initializeDataLoader(_, _, _, _, _)).Times(0);
+ EXPECT_CALL(*mDataLoaderManager, destroyDataLoader(_)).Times(0);
TemporaryDir tempDir;
int storageId =
mIncrementalService->createStorage(tempDir.path, std::move(mDataLoaderParcel), {},
@@ -357,7 +366,7 @@
mVold->mountIncFsSuccess();
mIncFs->makeFileFails();
EXPECT_CALL(*mDataLoaderManager, initializeDataLoader(_, _, _, _, _)).Times(0);
- EXPECT_CALL(*mDataLoaderManager, destroyDataLoader(_));
+ EXPECT_CALL(*mDataLoaderManager, destroyDataLoader(_)).Times(0);
EXPECT_CALL(*mVold, unmountIncFs(_));
TemporaryDir tempDir;
int storageId =
@@ -371,7 +380,7 @@
mIncFs->makeFileSuccess();
mVold->bindMountFails();
EXPECT_CALL(*mDataLoaderManager, initializeDataLoader(_, _, _, _, _)).Times(0);
- EXPECT_CALL(*mDataLoaderManager, destroyDataLoader(_));
+ EXPECT_CALL(*mDataLoaderManager, destroyDataLoader(_)).Times(0);
EXPECT_CALL(*mVold, unmountIncFs(_));
TemporaryDir tempDir;
int storageId =
@@ -385,7 +394,7 @@
mIncFs->makeFileSuccess();
mVold->bindMountSuccess();
mDataLoaderManager->initializeDataLoaderFails();
- EXPECT_CALL(*mDataLoaderManager, destroyDataLoader(_));
+ EXPECT_CALL(*mDataLoaderManager, destroyDataLoader(_)).Times(1);
EXPECT_CALL(*mVold, unmountIncFs(_)).Times(2);
TemporaryDir tempDir;
int storageId =
@@ -399,7 +408,7 @@
mIncFs->makeFileSuccess();
mVold->bindMountSuccess();
mDataLoaderManager->initializeDataLoaderSuccess();
- EXPECT_CALL(*mDataLoaderManager, destroyDataLoader(_));
+ EXPECT_CALL(*mDataLoaderManager, destroyDataLoader(_)).Times(1);
EXPECT_CALL(*mVold, unmountIncFs(_)).Times(2);
TemporaryDir tempDir;
int storageId =