1 files changed, 47 insertions, 35 deletions

diff --git a/services/incremental/IncrementalService.cpp b/services/incremental/IncrementalService.cpp
index 0da167303ccd..d36eae89c1ff 100644
--- a/services/incremental/IncrementalService.cpp
+++ b/services/incremental/IncrementalService.cpp
@@ -17,7 +17,6 @@
 #define LOG_TAG "IncrementalService"
 
 #include "IncrementalService.h"
-#include "IncrementalServiceValidation.h"
 
 #include <android-base/file.h>
 #include <android-base/logging.h>
@@ -582,25 +581,29 @@ int IncrementalService::setStorageParams(StorageId storageId, bool enableReadLog
         return -EINVAL;
     }
 
-    ifs->dataLoaderFilesystemParams.readLogsEnabled = enableReadLogs;
     if (enableReadLogs) {
-        // We never unregister the callbacks, but given a restricted number of data loaders and even fewer asking for read log access, should be ok.
-        registerAppOpsCallback(ifs->dataLoaderParams.packageName);
+        if (auto status =
+                    mAppOpsManager->checkPermission(kDataUsageStats, kOpUsage,
+                                                    ifs->dataLoaderParams.packageName.c_str());
+            !status.isOk()) {
+            LOG(ERROR) << "checkPermission failed: " << status.toString8();
+            return fromBinderStatus(status);
+        }
     }
 
-    return applyStorageParams(*ifs);
-}
+    if (auto status = applyStorageParams(*ifs, enableReadLogs); !status.isOk()) {
+        LOG(ERROR) << "applyStorageParams failed: " << status.toString8();
+        return fromBinderStatus(status);
+    }
 
-int IncrementalService::applyStorageParams(IncFsMount& ifs) {
-    const bool enableReadLogs = ifs.dataLoaderFilesystemParams.readLogsEnabled;
     if (enableReadLogs) {
-        if (auto status = CheckPermissionForDataDelivery(kDataUsageStats, kOpUsage);
-            !status.isOk()) {
-            LOG(ERROR) << "CheckPermissionForDataDelivery failed: " << status.toString8();
-            return fromBinderStatus(status);
-        }
+        registerAppOpsCallback(ifs->dataLoaderParams.packageName);
     }
 
+    return 0;
+}
+
+binder::Status IncrementalService::applyStorageParams(IncFsMount& ifs, bool enableReadLogs) {
     using unique_fd = ::android::base::unique_fd;
     ::android::os::incremental::IncrementalFileSystemControlParcel control;
     control.cmd.reset(unique_fd(dup(ifs.control.cmd())));
@@ -611,13 +614,7 @@ int IncrementalService::applyStorageParams(IncFsMount& ifs) {
     }
 
     std::lock_guard l(mMountOperationLock);
-    const auto status = mVold->setIncFsMountOptions(control, enableReadLogs);
-    if (!status.isOk()) {
-        LOG(ERROR) << "Calling Vold::setIncFsMountOptions() failed: " << status.toString8();
-        return fromBinderStatus(status);
-    }
-
-    return 0;
+    return mVold->setIncFsMountOptions(control, enableReadLogs);
 }
 
 void IncrementalService::deleteStorage(StorageId storageId) {
@@ -1280,39 +1277,54 @@ bool IncrementalService::configureNativeBinaries(StorageId storage, std::string_
 }
 
 void IncrementalService::registerAppOpsCallback(const std::string& packageName) {
-    if (packageName.empty()) {
-        return;
-    }
-
+    sp<IAppOpsCallback> listener;
     {
         std::unique_lock lock{mCallbacksLock};
-        if (!mCallbackRegistered.insert(packageName).second) {
+        auto& cb = mCallbackRegistered[packageName];
+        if (cb) {
             return;
         }
+        cb = new AppOpsListener(*this, packageName);
+        listener = cb;
     }
 
-    /* TODO(b/152633648): restore callback after it's not crashing Binder anymore.
-    sp<AppOpsListener> listener = new AppOpsListener(*this, packageName);
     mAppOpsManager->startWatchingMode(AppOpsManager::OP_GET_USAGE_STATS, String16(packageName.c_str()), listener);
-    */
 }
 
-void IncrementalService::onAppOppChanged(const std::string& packageName) {
+bool IncrementalService::unregisterAppOpsCallback(const std::string& packageName) {
+    sp<IAppOpsCallback> listener;
+    {
+        std::unique_lock lock{mCallbacksLock};
+        auto found = mCallbackRegistered.find(packageName);
+        if (found == mCallbackRegistered.end()) {
+            return false;
+        }
+        listener = found->second;
+        mCallbackRegistered.erase(found);
+    }
+
+    mAppOpsManager->stopWatchingMode(listener);
+    return true;
+}
+
+void IncrementalService::onAppOpChanged(const std::string& packageName) {
+    if (!unregisterAppOpsCallback(packageName)) {
+        return;
+    }
+
     std::vector<IfsMountPtr> affected;
     {
         std::lock_guard l(mLock);
         affected.reserve(mMounts.size());
         for (auto&& [id, ifs] : mMounts) {
-            if (ifs->dataLoaderFilesystemParams.readLogsEnabled && ifs->dataLoaderParams.packageName == packageName) {
+            if (ifs->mountId == id && ifs->dataLoaderParams.packageName == packageName) {
                 affected.push_back(ifs);
             }
         }
     }
-    /* TODO(b/152633648): restore callback after it's not crashing Kernel anymore.
     for (auto&& ifs : affected) {
-        applyStorageParams(*ifs);
+        applyStorageParams(*ifs, false);
     }
-    */
 }
 
 binder::Status IncrementalService::IncrementalDataLoaderListener::onStatusChanged(MountId mountId,
@@ -1378,8 +1390,8 @@ binder::Status IncrementalService::IncrementalDataLoaderListener::onStatusChange
     return binder::Status::ok();
 }
 
-void IncrementalService::AppOpsListener::opChanged(int32_t op, const String16&) {
-    incrementalService.onAppOppChanged(packageName);
+void IncrementalService::AppOpsListener::opChanged(int32_t, const String16&) {
+    incrementalService.onAppOpChanged(packageName);
 }
 
 } // namespace android::incremental