Service logic moved to kernel#554
Conversation
📝 WalkthroughWalkthroughThis PR replaces the C++-based service instance/context implementation with a new C-language kernel service API (service_instance, service_manager, service_manifest, service_paths, paths). ServiceContext becomes a thin non-owning wrapper over a kernel ServiceInstance pointer, ServiceRegistration delegates to service_manager_* functions via extern "C" trampolines, ServicePaths inlines path lookups through the new C API, and Bluetooth device storage now derives paths from the user-data root. New kernel tests cover paths and service lifecycle behavior, and kernel symbols are extended accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant service_manager
participant ServiceInstance
participant ServiceManifest
Caller->>service_manager: service_manager_start(id)
service_manager->>ServiceInstance: service_instance_construct(manifest)
ServiceInstance->>ServiceManifest: create_service(manifest)
service_manager->>ServiceManifest: on_start(instance, data)
service_manager->>ServiceInstance: service_instance_set_state(STARTED)
Caller->>service_manager: service_manager_stop(id)
service_manager->>ServiceManifest: on_stop(instance, data)
service_manager->>ServiceInstance: service_instance_destruct(instance)
Related PRs: None identified. Suggested labels: kernel, service-api, breaking-change Suggested reviewers: TactilityProject maintainers familiar with kernel service subsystem 🐰 A kernel hop, a service spun, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
Tactility/Source/bluetooth/BluetoothPairedDevice.cpp (1)
108-117: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRedundant recomputation of the settings path.
getSettingsFilePath()builds a new string on each call, and here it's invoked twice within the same function (Lines 110 and 113). Minor, but worth caching into a local variable to avoid duplicate string concatenation.Suggested tweak
std::vector<PairedDevice> loadAll() { std::vector<dirent> entries; - if (!file::isDirectory(getSettingsFilePath())) { + const auto settings_path = getSettingsFilePath(); + if (!file::isDirectory(settings_path)) { return {}; } - file::scandir(getSettingsFilePath(), entries, [](const dirent* entry) -> int { + file::scandir(settings_path, entries, [](const dirent* entry) -> int {Tests/TactilityKernel/Source/ServiceTest.cpp (2)
4-5: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFragile ad-hoc
extern "C"declaration for internal test hook.
service_instance_set_stateis declared here by hand rather than via a shared internal/testing header. If the real signature inservice_instance.cppchanges, this won't fail to compile with a clear diagnostic tied to the source of truth — it can silently mismatch at link time. Consider exposing this via a small internal testing header included by both the implementation and this test.
166-182: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider asserting cleanup on start failure.
This test verifies state/context after a failed start, but doesn't assert
destroy_calledat the end (afterservice_manager_remove), leaving the destroy path on a failed-start instance unverified.TactilityKernel/source/paths.cpp (1)
18-21: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPrefer bounded copy over
strcpy, even though manually guarded.Both call sites check length before
strcpy, so they aren't currently exploitable, but static analysis flags the rawstrcpypattern (CWE-120), and the rest of the file already usessnprintffor bounded writes. Switching tosnprintffor consistency removes the lint noise and guards against future edits that might remove the preceding length check.Suggested fix using snprintf
- if (std::strlen(mount_point) + 1 > out_path_size) { - return ERROR_BUFFER_OVERFLOW; - } - std::strcpy(out_path, mount_point); - return ERROR_NONE; + int written = std::snprintf(out_path, out_path_size, "%s", mount_point); + if (written < 0 || (size_t)written >= out_path_size) { + return ERROR_BUFFER_OVERFLOW; + } + return ERROR_NONE;(apply the same change to the second
strcpycall site)Also applies to: 58-61
Source: Linters/SAST tools
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6abd1ca9-84eb-4b18-8748-65caabaf9b69
📒 Files selected for processing (20)
Tactility/Include/Tactility/service/Service.hTactility/Include/Tactility/service/ServicePaths.hTactility/Private/Tactility/service/ServiceInstance.hTactility/Source/bluetooth/BluetoothPairedDevice.cppTactility/Source/lvgl/Lvgl.cppTactility/Source/service/ServiceInstance.cppTactility/Source/service/ServicePaths.cppTactility/Source/service/ServiceRegistration.cppTactilityKernel/include/tactility/paths.hTactilityKernel/include/tactility/service/service_context.hTactilityKernel/include/tactility/service/service_instance.hTactilityKernel/include/tactility/service/service_manager.hTactilityKernel/include/tactility/service/service_manifest.hTactilityKernel/include/tactility/service/service_paths.hTactilityKernel/source/paths.cppTactilityKernel/source/service/service_instance.cppTactilityKernel/source/service/service_manager.cppTactilityKernel/source/service/service_paths.cppTests/TactilityKernel/Source/ServicePathsTest.cppTests/TactilityKernel/Source/ServiceTest.cpp
💤 Files with no reviewable changes (2)
- Tactility/Source/service/ServicePaths.cpp
- Tactility/Source/service/ServiceInstance.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
TactilityKernel/source/service/service_manager.cpp (2)
67-83: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftTOCTOU race between
remove()'s state check and the manifest erase.
service_manager_get_state(id)locks/unlocksinstance_ledger.mutexinternally, thenmanifest_ledger.mutexis acquired separately for the erase. A concurrentservice_manager_start(id)can create and register a running instance in that gap, soremove()can erase the manifest for a service that is now actively running.🔒 Proposed fix: hold `manifest_ledger.mutex` across the whole check-then-erase
error_t service_manager_remove(const char* id) { - if (service_manager_get_state(id) != SERVICE_STATE_STOPPED) { - return ERROR_INVALID_STATE; - } - mutex_lock(&manifest_ledger.mutex); + mutex_lock(&instance_ledger.mutex); + const bool running = instance_ledger.instances.contains(id); + mutex_unlock(&instance_ledger.mutex); + if (running) { + mutex_unlock(&manifest_ledger.mutex); + return ERROR_INVALID_STATE; + } + const auto iterator = manifest_ledger.manifests.find(id); if (iterator == manifest_ledger.manifests.end()) { mutex_unlock(&manifest_ledger.mutex); return ERROR_NOT_FOUND; } manifest_ledger.manifests.erase(iterator); mutex_unlock(&manifest_ledger.mutex);Since
service_manager_start()also locksmanifest_ledgerfirst (and never nests it insideinstance_ledger), this preserves lock ordering without introducing deadlock risk.
95-116: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftGlobal
instance_ledgermutex is held across an external callback (manifest->create_service).
instance_ledger.mutex— a single mutex shared by every service in the system — stays locked from line 95 through line 116, which spans theservice_instance_construct()call at line 107. That call invokesmanifest->create_service(manifest)directly. Any service whosecreate_servicedoes non-trivial work, blocks, or (worst case) calls back into anyservice_manager_*function will stall or deadlock every other service's start/stop/state lookup in the system.Consider reserving the id under the lock (e.g., inserting a placeholder before construction), releasing the lock before invoking
service_instance_construct()/create_service(), and re-acquiring it only to finalize or roll back the reservation. This does requireget_state/find_instanceto tolerate the reserved-but-not-yet-constructed state.
♻️ Duplicate comments (1)
TactilityKernel/source/service/service_manager.cpp (1)
114-139: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftUse-after-free:
start()still usesinstanceafteron_start()returns, with no pin against a concurrentstop().This was already flagged in a previous review pass on this PR ("Use-after-free if
stop()runs whileon_start()is still executing"). The instance is inserted intoinstance_ledgerand its state set toSTARTING(lines 115-118) beforemanifest->on_start()runs unlocked (line 121). Onceon_start()returns,start()dereferences the sameinstanceagain (lines 123-136) to set its final state, and on failure to erase/destruct/delete it. In the interim,instance_ledgeralready exposes this instance to a concurrentservice_manager_stop(id), which can itself erase,destruct(), anddelete instanceonce state reachesSTOPPING/STOPPED. There is no refcount/pin left in the new API (previous suggestion to gate onuse_countno longer applies since that mechanism has been removed entirely), so this race is unresolved and now has zero mitigation.A fix likely needs a lifetime-pinning mechanism (e.g., re-introduce a use-count/generation check, or hold instance ownership exclusively in the starting thread until
on_start()fully completes and only then make it visible/stoppable).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2793ccab-8e3e-4343-815b-f01accb9dda5
📒 Files selected for processing (15)
Tactility/Include/Tactility/service/Service.hTactility/Include/Tactility/service/ServiceContext.hTactility/Include/Tactility/service/ServiceManifest.hTactility/Include/Tactility/service/ServicePaths.hTactility/Include/Tactility/service/ServiceRegistration.hTactility/Private/Tactility/service/ServiceInstance.hTactility/Source/service/ServiceContext.cppTactility/Source/service/ServiceRegistration.cppTactilityKernel/include/tactility/service/service_instance.hTactilityKernel/include/tactility/service/service_manager.hTactilityKernel/include/tactility/service/service_manifest.hTactilityKernel/source/kernel_symbols.cTactilityKernel/source/service/service_instance.cppTactilityKernel/source/service/service_manager.cppTests/TactilityKernel/Source/ServiceTest.cpp
💤 Files with no reviewable changes (1)
- Tactility/Private/Tactility/service/ServiceInstance.h
🚧 Files skipped from review as they are similar to previous changes (2)
- Tests/TactilityKernel/Source/ServiceTest.cpp
- Tactility/Source/service/ServiceRegistration.cpp
1 participant
Summary by CodeRabbit
New Features
Bug Fixes