xiaoxiang781216 commented on code in PR #17644:
URL: https://github.com/apache/nuttx/pull/17644#discussion_r2674933360
##########
sched/task/task_restart.c:
##########
@@ -119,7 +119,9 @@ static void nxtask_reset_task(FAR struct tcb_s *tcb, bool
remove)
/* Deallocate anything left in the TCB's signal queues */
+#ifdef CONFIG_ENABLE_ALL_SIGNALS
Review Comment:
can we add a dummy macro
##########
arch/risc-v/src/common/riscv_signal_dispatch.c:
##########
@@ -75,4 +75,4 @@ void up_signal_dispatch(_sa_sigaction_t sighand, int signo,
(uintptr_t)info, (uintptr_t)ucontext);
}
-#endif /* !CONFIG_BUILD_FLAT && __KERNEL__ */
Review Comment:
keep `__KERNEL__`
##########
sched/signal/signal.h:
##########
@@ -214,8 +216,6 @@ void nxsig_release_pendingsignal(FAR
sigpendq_t *sigpend);
FAR sigpendq_t *nxsig_remove_pendingsignal(FAR struct tcb_s *stcb,
int signo);
-#ifdef CONFIG_ENABLE_ALL_SIGNALS
Review Comment:
why change
##########
sched/task/task_exithook.c:
##########
@@ -454,8 +454,9 @@ void nxtask_exithook(FAR struct tcb_s *tcb, int status)
group_leave(tcb);
/* Deallocate anything left in the TCB's queues */
-
Review Comment:
keep blank line
##########
sched/task/task_exithook.c:
##########
@@ -454,8 +454,9 @@ void nxtask_exithook(FAR struct tcb_s *tcb, int status)
group_leave(tcb);
/* Deallocate anything left in the TCB's queues */
-
+#ifdef CONFIG_ENABLE_ALL_SIGNALS
Review Comment:
ditto
##########
include/nuttx/sched.h:
##########
@@ -514,13 +514,17 @@ struct task_group_s
/* POSIX Signal Control Fields ********************************************/
+#ifndef CONFIG_DISABLE_ALL_SIGNALS
Review Comment:
why need
##########
include/sys/syscall_lookup.h:
##########
@@ -154,6 +154,7 @@ SYSCALL_LOOKUP(nxsem_wait_slow, 1)
SYSCALL_LOOKUP(kill, 2)
SYSCALL_LOOKUP(tgkill, 3)
+#ifndef CONFIG_DISABLE_ALL_SIGNALS
Review Comment:
move after line 161
##########
fs/vfs/Kconfig:
##########
@@ -51,6 +51,7 @@ endif # TIMER_FD
config SIGNAL_FD
bool "SignalFD"
+ depends on ENABLE_ALL_SIGNALS
Review Comment:
`!DISABLE_ALL_SIGNALS` and move to next patch
##########
include/nuttx/sched.h:
##########
@@ -675,6 +681,7 @@ struct tcb_s
sq_queue_t sigpostedq; /* List of posted signals */
#endif /* CONFIG_ENABLE_ALL_SIGNALS */
siginfo_t *sigunbinfo; /* Signal info when task unblocked */
Review Comment:
move line 683 and 684 after line 672 to avoid the nest `#ifdef`
##########
sched/Kconfig:
##########
@@ -1547,6 +1547,53 @@ endmenu # RTOS hooks
menu "Signal Configuration"
+choice
+ prompt "Signal support level"
+ default ENABLE_ALL_SIGNALS if !DEFAULT_SMALL
+ default ENABLE_PARTIAL_SIGNALS if DEFAULT_SMALL
+
+config ENABLE_ALL_SIGNALS
+ bool "Enable full signal support"
+ ---help---
+ Enable full POSIX signal support, including signal handling,
+ thread cancellation, timers, and process-related notifications.
+
+ This option provides the most complete signal functionality,
+ but increases code size and resource usage.
+
+ Typical use cases:
+ - Applications relying on POSIX signals or timers
+ - Systems where memory and CPU resources are sufficient
+
+config ENABLE_PARTIAL_SIGNALS
+ bool "Enable partial signal support"
+ ---help---
+ Disable process-related signal functionality while keeping
+ basic signal handling support.
+
+ This option provides a balance between reduced resource usage
+ and limited signal functionality.
+
+ Typical use cases:
+ - Applications using basic signals without fork/exec
+ - Resource-constrained real-time systems
+
+config DISABLE_ALL_SIGNALS
Review Comment:
move to next patch
##########
sched/task/task_spawnparms.c:
##########
@@ -165,6 +166,7 @@ int spawn_execattrs(pid_t pid, FAR const posix_spawnattr_t
*attr)
tcb->sigprocmask = attr->sigmask;
}
}
+#endif
Review Comment:
need guard syscall function with if(NOT CONFIG_DISABLE_ALL_SIGNALS)
##########
sched/signal/sig_dispatch.c:
##########
@@ -697,13 +708,15 @@ int nxsig_tcbdispatch(FAR struct tcb_s *stcb, siginfo_t
*info,
leave_critical_section(flags);
+#ifdef CONFIG_ENABLE_ALL_SIGNALS
/* Dispatch kernel action, if needed, in case a pending signal was added */
if (sigpend != NULL)
{
nxsig_dispatch_kernel_action(stcb, &sigpend->info);
}
Review Comment:
move blank line after line 719
##########
sched/signal/Make.defs:
##########
@@ -39,3 +39,4 @@ endif
DEPPATH += --dep-path signal
VPATH += :signal
+endif
Review Comment:
move before line 27
##########
sched/task/task_restart.c:
##########
@@ -119,10 +119,12 @@ static void nxtask_reset_task(FAR struct tcb_s *tcb, bool
remove)
/* Deallocate anything left in the TCB's signal queues */
-#ifdef CONFIG_ENABLE_ALL_SIGNALS
Review Comment:
don't need change
##########
libs/libc/signal/Make.defs:
##########
@@ -36,3 +37,4 @@ endif
DEPPATH += --dep-path signal
VPATH += :signal
+endif
Review Comment:
move before line 30
##########
sched/signal/CMakeLists.txt:
##########
@@ -20,43 +20,45 @@
#
#
##############################################################################
-set(SRCS
- sig_procmask.c
- sig_suspend.c
- sig_kill.c
- sig_tgkill.c
- sig_queue.c
- sig_waitinfo.c
- sig_timedwait.c
- sig_lowest.c
- sig_notification.c
- sig_dispatch.c
- sig_pause.c
- sig_nanosleep.c
- sig_usleep.c
- sig_sleep.c
- sig_ppoll.c
- sig_pselect.c)
+if(NOT CONFIG_DISABLE_ALL_SIGNALS)
+ set(SRCS
+ sig_procmask.c
+ sig_suspend.c
+ sig_kill.c
+ sig_tgkill.c
+ sig_queue.c
+ sig_waitinfo.c
+ sig_timedwait.c
+ sig_lowest.c
+ sig_notification.c
+ sig_dispatch.c
+ sig_pause.c
+ sig_nanosleep.c
+ sig_usleep.c
+ sig_sleep.c
+ sig_ppoll.c
+ sig_pselect.c)
-if(CONFIG_ENABLE_ALL_SIGNALS)
- list(
- APPEND
- SRCS
- sig_action.c
- sig_allocpendingsigaction.c
- sig_cleanup.c
- sig_deliver.c
- sig_findaction.c
- sig_initialize.c
- sig_pending.c
- sig_releasependingsigaction.c
- sig_releasependingsignal.c
- sig_removependingsignal.c
- sig_unmaskpendingsignal.c)
-endif()
+ if(CONFIG_ENABLE_ALL_SIGNALS)
Review Comment:
move out of `if(NOT CONFIG_DISABLE_ALL_SIGNALS)`
##########
sched/task/task_setup.c:
##########
@@ -460,8 +460,9 @@ static int nxthread_setup_scheduler(FAR struct tcb_s *tcb,
int priority,
/* exec(), pthread_create(), task_create(), and vfork() all
* inherit the signal mask of the parent thread.
*/
-
Review Comment:
keep blank line
##########
libs/libc/signal/CMakeLists.txt:
##########
@@ -20,28 +20,30 @@
#
#
##############################################################################
-set(SRCS
- sig_addset.c
- sig_delset.c
- sig_emptyset.c
- sig_fillset.c
- sig_nandset.c
- sig_andset.c
- sig_orset.c
- sig_xorset.c
- sig_isemptyset.c
- sig_killpg.c
- sig_altstack.c
- sig_hold.c
- sig_ismember.c
- sig_pause.c
- sig_psignal.c
- sig_raise.c
- sig_relse.c
- sig_wait.c)
+if(NOT CONFIG_DISABLE_ALL_SIGNALS)
+ set(SRCS
+ sig_addset.c
+ sig_delset.c
+ sig_emptyset.c
+ sig_fillset.c
+ sig_nandset.c
+ sig_andset.c
+ sig_orset.c
+ sig_xorset.c
+ sig_isemptyset.c
+ sig_killpg.c
+ sig_altstack.c
+ sig_hold.c
+ sig_ismember.c
+ sig_pause.c
+ sig_psignal.c
+ sig_raise.c
+ sig_relse.c
+ sig_wait.c)
-if(CONFIG_ENABLE_ALL_SIGNALS)
- list(APPEND SRCS sig_ignore.c sig_interrupt.c sig_set.c sig_signal.c)
-endif()
+ if(CONFIG_ENABLE_ALL_SIGNALS)
Review Comment:
move out `if(NOT CONFIG_DISABLE_ALL_SIGNALS)`
--
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]
