Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit f4769e2f authored by Yurii Zubrytskyi's avatar Yurii Zubrytskyi Committed by Songchun Fan
Browse files

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
parent 883a27a3

services/incremental/BinderIncrementalService.cpp

+8 −10
Original line number Diff line number Diff line
@@ -118,7 +118,8 @@ binder::Status BinderIncrementalService::openStorage(const std::string& path, 
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,

    *_aidl_return =

            mImpl.createStorage(path, const_cast<content::pm::DataLoaderParamsParcel&&>(params),

                                android::incremental::IncrementalService::CreateOptions(

                                        createMode));

    return ok();
@@ -144,9 +145,8 @@ binder::Status BinderIncrementalService::startLoading( 
        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 @@ binder::Status BinderIncrementalService::registerStorageHealthListener( 
        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();

}

services/incremental/IncrementalService.cpp

+53 −60
Original line number Diff line number Diff line
@@ -474,8 +474,8 @@ auto IncrementalService::getStorageSlotLocked() -> MountMap::iterator { 
    }

}



StorageId IncrementalService::createStorage(

        std::string_view mountPoint, const content::pm::DataLoaderParamsParcel& dataLoaderParams,

StorageId IncrementalService::createStorage(std::string_view mountPoint,

                                            content::pm::DataLoaderParamsParcel dataLoaderParams,

                                            CreateOptions options) {

    LOG(INFO) << "createStorage: " << mountPoint << " | " << int(options);

    if (!path::isAbsolute(mountPoint)) {
@@ -584,9 +584,9 @@ StorageId IncrementalService::createStorage( 
        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 @@ StorageId IncrementalService::createLinkedStorage(std::string_view mountPoint, 
}



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 @@ bool IncrementalService::startLoading(StorageId storageId, 
    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 @@ RawMetadata IncrementalService::getMetadata(StorageId storage, FileId node) cons 
    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 @@ void IncrementalService::runCmdLooper() { 
}



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 @@ void IncrementalService::prepareDataLoaderLocked(IncFsMount& ifs, DataLoaderPara 


    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 @@ IncrementalService::LoadingProgress IncrementalService::getLoadingProgressFromPa 
    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 @@ bool IncrementalService::updateLoadingProgress( 
    }

    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::unregisterLoadingProgressListener(StorageId storage) { 
}



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 @@ bool IncrementalService::registerStorageHealthListener( 
            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 @@ long IncrementalService::getMillsSinceOldestPendingRead(StorageId storageId) { 
    return ifs->dataLoaderStub->elapsedMsSinceOldestPendingRead();

}



IncrementalService::DataLoaderStub::DataLoaderStub(IncrementalService& service, MountId id,

                                                   DataLoaderParamsParcel&& params,

                                                   FileSystemControlParcel&& control,

                                                   const DataLoaderStatusListener* statusListener,

                                                   StorageHealthCheckParams&& healthCheckParams,

                                                   const StorageHealthListener* healthListener,

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()) {

        mHealthCheckParams(healthCheckParams) {

    if (mHealthListener && !isHealthParamsValid()) {

        mHealthListener = {};

    }

    } else {

    if (!mHealthListener) {

        // Disable advanced health check statuses.

        mHealthCheckParams.blockedTimeoutMs = -1;

    }
@@ -2800,14 +2795,12 @@ void IncrementalService::DataLoaderStub::unregisterFromPendingReads() { 
}



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;

    }

}

services/incremental/IncrementalService.h

+23 −23
Original line number Diff line number Diff line
@@ -136,17 +136,17 @@ public: 
    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 @@ public: 
    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 @@ private: 
        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 @@ private: 
        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 @@ private: 
    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 @@ private: 


    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 @@ private: 
    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 @@ private: 
    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;