wangchdo commented on PR #17357: URL: https://github.com/apache/nuttx/pull/17357#issuecomment-3561292937
> > > > 1. Please refer to [sched/signal: Add support to disable signals #17352](https://github.com/apache/nuttx/pull/17352) as the primary commit; @wangchdo submitted this change first. > > > > 2. cc NuttX PMC and commiterrs, Please do not merge this PR, especially @xiaoxiang781216 > > > > > > > > > The previous submission at #17352 had an overly broad scope when disabling signals, as it also disabled functionalities like slee, kill etc. The current patchset narrows this scope, ensuring that APIs such as sleep and kill remain available while only certain signal functionalities are disabled. > > > Alternatively, we could introduce a concept like "disable levels" to differentiate the extent of signal functionality being disabled. > > > > > > For sleep()/usleep(), compatibility with the CONFIG_DISABLE_SIGNALS scenario should be considered in the implementation. For other APIs that have mandatory dependencies on signals (such as kill()/pkill()), scenario-based optimization can be performed—even the interfaces can be retained with error code returns supported. > > Basically, signal has two reaction: > > 1. Call the signal handler function in the target process/task > 2. Wake up the targe thread just like sem post > > The major memory/code size contribution come from the first item, the second item consume the small code/memory, but could keep many frequent used function(e.g. sleep/usleep/wait etc.). So, the best approach is disable signal by level to make it more useful in the different case. > > > 1. Please refer to [sched/signal: Add support to disable signals #17352](https://github.com/apache/nuttx/pull/17352) as the primary commit; @wangchdo submitted this change first. > > 2. cc NuttX PMC and commiterrs, Please do not merge this PR, especially @xiaoxiang781216 > > The detailed implementation is different, both approach could benefit in the different usage. It's better to introduce the disable level concept, so both pr could be merged. Why do we need two PRs for this? I already have a PR that disables signals. If the level-control approach works, it would be straightforward to integrate it into #17352. Opening a new PR would only consume additional reviewer time. I think we should focus on improving the existing PR instead. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
