Service logic moved to kernel#554
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds a kernel-level C service API for manifests, instances, manager control, and path resolution, plus a user-data path helper. The Tactility C++ service layer is updated to use those kernel APIs for state, registration, and path lookup. Bluetooth paired-device storage now uses the computed user-data path. New doctest coverage exercises the path helpers and service lifecycle flows. 🚥 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
| // Register before on_start() so a service can find itself while starting. | ||
| instance_ledger.instances[id] = instance; | ||
| mutex_unlock(&instance_ledger.mutex); | ||
|
|
||
| service_instance_set_state(instance, SERVICE_STATE_STARTING); | ||
|
|
||
| LOG_I(TAG, "start %s", id); | ||
| error = (instance->on_start != nullptr) ? instance->on_start(instance) : ERROR_NONE; | ||
|
|
||
| if (error == ERROR_NONE) { | ||
| service_instance_set_state(instance, SERVICE_STATE_STARTED); | ||
| return ERROR_NONE; | ||
| } | ||
|
|
||
| LOG_E(TAG, "Failed to start %s: %s", id, error_to_string(error)); | ||
| service_instance_set_state(instance, SERVICE_STATE_STOPPED); | ||
|
|
||
| mutex_lock(&instance_ledger.mutex); | ||
| instance_ledger.instances.erase(id); | ||
| mutex_unlock(&instance_ledger.mutex); | ||
|
|
||
| service_instance_destruct(instance); | ||
| delete instance; | ||
|
|
||
| return ERROR_RESOURCE; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP '\bservice_manager_(start|stop)\b' -C3
# Look for on_start callbacks that may re-enter the manager
rg -nP '\bon_start\b\s*=' -C3Repository: TactilityProject/Tactility
Length of output: 164
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the service manager implementation and related interfaces.
git ls-files 'TactilityKernel/source/service/*' 'TactilityKernel/include/service/*' 'TactilityKernel/source/*service*' | sed -n '1,200p'
printf '\n--- service_manager.cpp outline ---\n'
ast-grep outline TactilityKernel/source/service/service_manager.cpp --view expanded || true
printf '\n--- search for service_manager_start/stop and on_start/on_stop implementations ---\n'
rg -n --hidden --glob '!**/build/**' --glob '!**/dist/**' '\bservice_manager_(start|stop)\b|\bon_start\b|\bon_stop\b' TactilityKernel -C 2
printf '\n--- search for use_count/reference-counting helpers ---\n'
rg -n --hidden --glob '!**/build/**' --glob '!**/dist/**' '\buse_count\b|reference count|refcount|retain|release' TactilityKernel -C 2Repository: TactilityProject/Tactility
Length of output: 15218
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the relevant portion of service_manager.cpp with line numbers.
wc -l TactilityKernel/source/service/service_manager.cpp
sed -n '1,260p' TactilityKernel/source/service/service_manager.cpp | cat -n
# Inspect the service instance definition and lifecycle helpers.
rg -n --hidden --glob '!**/build/**' '\bstruct service_instance\b|\bclass service_instance\b|\bservice_instance_(set_state|destruct|construct|add_use_count|remove_use_count|get_use_count|find_context)\b' TactilityKernel -C 3Repository: TactilityProject/Tactility
Length of output: 16111
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Focus on the start/stop implementations and callback wiring.
sed -n '1,260p' TactilityKernel/source/service/service_manager.cpp | cat -n
printf '\n---\n'
sed -n '1,260p' TactilityKernel/source/service/service.cpp | cat -n 2>/dev/null || true
printf '\n---\n'
rg -n --hidden --glob '!**/build/**' '\bon_start\b|\bon_stop\b' TactilityKernel/source TactilityKernel/include -C 4Repository: TactilityProject/Tactility
Length of output: 18329
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Identify the exact service manager and service callback definitions.
files=$(git ls-files 'TactilityKernel/**/*.cpp' 'TactilityKernel/**/*.h' 'TactilityKernel/**/*.hpp')
printf '%s\n' "$files" | rg 'service_manager|service|kernel' || true
printf '\n--- service_manager.cpp ---\n'
sed -n '1,220p' TactilityKernel/source/service/service_manager.cpp | cat -n
printf '\n--- declarations of service_instance and lifecycle helpers ---\n'
rg -n --hidden --glob '!**/build/**' '\bservice_instance\b|service_instance_set_state|service_instance_destruct|service_instance_construct|service_manager_start|service_manager_stop|find_context' TactilityKernel -C 3
printf '\n--- any callbacks that call stop/start or service_manager directly ---\n'
rg -n --hidden --glob '!**/build/**' '\bservice_manager_(start|stop)\s*\(|\bstart_service\s*\(|\bstop_service\s*\(' TactilityKernel -C 3Repository: TactilityProject/Tactility
Length of output: 26977
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the service manager implementation around the relevant lifecycle code.
sed -n '1,240p' TactilityKernel/source/service/service_manager.cpp | cat -n
printf '\n--- related symbols in the service subsystem ---\n'
rg -n --hidden --glob '!**/build/**' 'instance_ledger|use_count|service_instance_set_state|service_instance_destruct|service_manager_(start|stop)|on_start|on_stop' TactilityKernel/source TactilityKernel/include -C 3Repository: TactilityProject/Tactility
Length of output: 32498
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the service-instance lifecycle helpers to see whether a STARTING instance
# can be destructed or whether the refcount protects it.
sed -n '1,140p' TactilityKernel/source/service/service_instance.cpp | cat -nRepository: TactilityProject/Tactility
Length of output: 3986
Use-after-free if stop() runs while on_start() is still executing. The instance is published to instance_ledger before on_start() and then used again after the callback returns, but service_manager_stop() can erase and delete the same instance in that window. service_instance_try_get() only protects SERVICE_STATE_STARTED, so it does not pin the object during startup. Hold a lifetime reference across on_start() or otherwise defer destruction until startup completes.
1 participant
Summary by CodeRabbit