Injectable monitors #496
Injectable monitors #496
Conversation
There was a problem hiding this comment.
Moved the base monitor system to RecordingCore. Any other module can now inject monitors here with the desired functionality. Since we cannot use bit options in this case, I switched to using a C-string name to distinguish between monitors. Additionally, I introduced monitor properties because previously the base monitor system used monitor types (essentially names) to determine its behavior during exception handling. Now, any monitor can utilize these properties, for instance, by marking itself as a fatal monitor to indicate that if it is triggered, the process can no longer continue to operate.
| void kscm_activateMonitors(void); | ||
|
|
||
| void kscm_disableAllMonitors(void); | ||
|
|
||
| /** Get the currently active monitors. | ||
| */ | ||
| KSCrashMonitorType kscm_getActiveMonitors(void); | ||
| //KSCrashMonitorType kscm_getActiveMonitors(void); | ||
|
|
||
| void kscm_addMonitor(KSCrashMonitorAPI* api); |
There was a problem hiding this comment.
We're transitioning to a new installation process. Upon reviewing the code, I noticed that KSCrash internally only activated monitors during installation and used kscm_setActiveMonitors with 0 in other places (effectively disabling them). With the new injection mechanism, I've changed to a two-stage process: first adding monitors, and then activating all of them. I also introduced a "killswitch," kscm_disableAllMonitors, to disable everything during exception handling. To streamline things, I replaced the KSCrashMonitorType defined in the public API with raw KSCrashMonitorAPI pointers and implemented this mapping in the Recording module. Because of this change, I commented out kscm_getActiveMonitors, as returning an array of raw pointers seemed like poor design and I wanted to avoid any mappings to keep things simple. This function is no longer needed internally, but we might come up with a better design and reintroduce it later.
There was a problem hiding this comment.
Maybe instead we should introduce a kscm_isMonitorActive(KSCrashMonitorAPI* api). But I agree, it's not a requirement now.
There was a problem hiding this comment.
Actually, you can get it just from the KSCrashMonitorAPI* you pass.
kscm_isMonitorEnabled(api);
| #pragma mark - Globals - | ||
| // ============================================================================ | ||
|
|
||
| static MonitorList g_monitors = {}; |
There was a problem hiding this comment.
I am not sure if we need to synchronise here, as we could potentially get a deadlock.
There was a problem hiding this comment.
@naftaly, any ideas here? Should I use atomics instead?
There was a problem hiding this comment.
Yes, but in case of a crash in the synchronized code here, we can get a deadlock, as this code should also handle SINGAL or Mach crash of KSCrash. So I'm thinking of atomic operations here, but it gets complicated with structs
There was a problem hiding this comment.
One of the options is to skip the crash if something updates monitors. E.g. if it's a lock we can add "if locked – exit". This kinda makes sense as if something crashes while KSCrash is being setup it's reasonable to ignore such crash.
| } | ||
| else | ||
| { | ||
| KSLOG_ERROR("Unknown crash monitor type: %s", crash->monitorName); |
There was a problem hiding this comment.
(not for this PR, but) I'm wondering if this logic can be also put into monitors. As if I want to create my own type of crashes, I might need to change logic here.
Sources/KSCrashRecording/Monitors/KSCrashMonitor_CPPException.cpp
Outdated
Show resolved
Hide resolved
GLinnik21
force-pushed
the
injectable-monitors
branch
from
3cbc0d4
to
0a3cb21
Compare
for optimisation
| void kscm_activateMonitors(void); | ||
|
|
||
| void kscm_disableAllMonitors(void); | ||
|
|
||
| /** Get the currently active monitors. | ||
| */ | ||
| KSCrashMonitorType kscm_getActiveMonitors(void); | ||
| //KSCrashMonitorType kscm_getActiveMonitors(void); | ||
|
|
||
| void kscm_addMonitor(KSCrashMonitorAPI* api); |
There was a problem hiding this comment.
Maybe instead we should introduce a kscm_isMonitorActive(KSCrashMonitorAPI* api). But I agree, it's not a requirement now.
No description provided.