You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by "wangchdo (via GitHub)" <gi...@apache.org> on 2025/11/21 07:53:56 UTC
[PR] sched/signal: Fix nxsig_ismember() return value behavior [nuttx]
wangchdo opened a new pull request, #17359:
URL: https://github.com/apache/nuttx/pull/17359
## Summary
nxsig_ismember() has a return type of int, but the current implementation returns a boolean value, which is incorrect.
All callers should determine membership by checking whether the return value is 1 or 0, which is also consistent with the POSIX sigismember() API.
## Impact
Fix issue of nxsig_ismember implementation, no impact to any other Nuttx functions
## Testing
**ostest passed on board fvp-armv8r-aarch32**
```
NuttShell (NSH)
nsh> [ 0] Idle_Task: nx_start: CPU0: Beginning Idle Loop
nsh> uname -a
NuttX 0.0.0 1827748fbb Nov 21 2025 15:51:41 arm fvp-armv8r-aarch32
nsh>
nsh> ostest
(...)
End of test memory usage:
VARIABLE BEFORE AFTER
======== ======== ========
arena 7ff9d3c 7ff9d3c
ordblks 7 5
mxordblk 7fe1ee0 7febf08
uordblks d724 b63c
fordblks 7fec618 7fee700
user_main: vfork() test
[ 4] ostest: arm_fork: fork context [0x2000f6b8]:
[ 4] ostest: arm_fork: r4:20003860 r5:00000000 r6:00000000 r7:00000000
[ 4] ostest: arm_fork: r8:00000000 r9:00000000 r10:00000000
[ 4] ostest: arm_fork: r11:00000000 sp:2000f6e0 lr:00055c04
[ 4] ostest: nxtask_setup_fork: Child priority=100 start=0x55c04
[ 4] ostest: nxtask_setup_fork: parent=0x2000d358, returning child=0x2000f790
[ 4] ostest: arm_fork: TCBs: Parent=0x2000d358 Child=0x2000f790
[ 4] ostest: arm_fork: Parent: stackutil:152
[ 4] ostest: arm_fork: Old stack top:2000f778 SP:2000f6e0 FP:00000000
[ 4] ostest: arm_fork: New stack top:20013a80 SP:200139e8 FP:00000000
[ 4] ostest: nxtask_start_fork: Starting Child TCB=0x2000f790
[ 4] ostest: nxtask_activate: ostest pid=85,TCB=0x2000f790
[85] ostest: nxtask_exit: ostest pid=85,TCB=0x2000f790
vfork_test: Child 85 ran successfully
Final memory usage:
VARIABLE BEFORE AFTER
======== ======== ========
arena 7ff9d3c 7ff9d3c
ordblks 1 5
mxordblk 7ff0880 7febf08
uordblks 94bc b55c
fordblks 7ff0880 7fee7e0
user_main: Exiting
[ 4] ostest: nxtask_exit: ostest pid=4,TCB=0x2000d358
ostest_main: Exiting with status 0
stdio_test: Standard I/O Check: fprintf to stderr
[ 3] ostest: nxtask_exit: ostest pid=3,TCB=0x2000af38
nsh>
```
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sched/signal: Fix nxsig_ismember() return value behavior [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #17359:
URL: https://github.com/apache/nuttx/pull/17359#discussion_r2549184925
##########
fs/vfs/fs_signalfd.c:
##########
@@ -149,7 +149,7 @@ static int signalfd_file_close(FAR struct file *filep)
for (signo = MIN_SIGNO; signo <= MAX_SIGNO; signo++)
{
- if (nxsig_ismember(&dev->sigmask, signo))
+ if (nxsig_ismember(&dev->sigmask, signo) == 1)
Review Comment:
> I think `nxsig_ismember()` is intended to implement the POSIX API` sigismember()`.
>
> According to` sigismember()`, a return value of 1 means the specified signal is a member of the set, and 0 means it is not.
>
yes, it's correct to ensure sigismember return 1 if the signal number is part of siganl set to compliant with POSIX spec.
> In addition, `group_signal_handler(`) also checks for `== 1`.
>
> I changed here because I think these usages should be consistent.
>
> Maybe I should not change here and go to update `group_signal_handler() `to not use `==1`?
>
Yes, since it's more robust and consiste with other check point.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sched/signal: Fix nxsig_ismember() return value behavior [nuttx]
Posted by "wangchdo (via GitHub)" <gi...@apache.org>.
wangchdo commented on code in PR #17359:
URL: https://github.com/apache/nuttx/pull/17359#discussion_r2551429657
##########
fs/vfs/fs_signalfd.c:
##########
@@ -149,7 +149,7 @@ static int signalfd_file_close(FAR struct file *filep)
for (signo = MIN_SIGNO; signo <= MAX_SIGNO; signo++)
{
- if (nxsig_ismember(&dev->sigmask, signo))
+ if (nxsig_ismember(&dev->sigmask, signo) == 1)
Review Comment:
I think about this again, and I think check ==1 is more robust, because this function may return value -EINVAL if the para is invalid, if we keep the previous implementation, this will be treated as success, this is incorrect
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sched/signal: Fix nxsig_ismember() return value behavior [nuttx]
Posted by "anchao (via GitHub)" <gi...@apache.org>.
anchao commented on code in PR #17359:
URL: https://github.com/apache/nuttx/pull/17359#discussion_r2548948288
##########
libs/libc/signal/sig_ismember.c:
##########
@@ -68,7 +68,7 @@ int nxsig_ismember(FAR const sigset_t *set, int signo)
{
/* Check if the signal is in the set */
- return ((set->_elem[_SIGSET_NDX(signo)] & _SIGNO2SET(signo)) != 0);
+ return (set->_elem[_SIGSET_NDX(signo)] & _SIGNO2SET(signo)) ? 1 : 0;
Review Comment:
```suggestion
return !!(set->_elem[_SIGSET_NDX(signo)] & _SIGNO2SET(signo));
```
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sched/signal: Fix nxsig_ismember() return value behavior [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #17359:
URL: https://github.com/apache/nuttx/pull/17359#discussion_r2553153668
##########
fs/vfs/fs_signalfd.c:
##########
@@ -149,7 +149,7 @@ static int signalfd_file_close(FAR struct file *filep)
for (signo = MIN_SIGNO; signo <= MAX_SIGNO; signo++)
{
- if (nxsig_ismember(&dev->sigmask, signo))
+ if (nxsig_ismember(&dev->sigmask, signo) == 1)
Review Comment:
if so, it's better to `>= 1`
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sched/signal: Fix nxsig_ismember() return value behavior [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 merged PR #17359:
URL: https://github.com/apache/nuttx/pull/17359
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sched/signal: Fix nxsig_ismember() return value behavior [nuttx]
Posted by "wangchdo (via GitHub)" <gi...@apache.org>.
wangchdo commented on code in PR #17359:
URL: https://github.com/apache/nuttx/pull/17359#discussion_r2553715053
##########
fs/vfs/fs_signalfd.c:
##########
@@ -149,7 +149,7 @@ static int signalfd_file_close(FAR struct file *filep)
for (signo = MIN_SIGNO; signo <= MAX_SIGNO; signo++)
{
- if (nxsig_ismember(&dev->sigmask, signo))
+ if (nxsig_ismember(&dev->sigmask, signo) == 1)
Review Comment:
Could you help explain why this approach is considered better?
According to the definition of this API, the return value follows a clear convention:
- 1 means the signal is a member of the set,
- 0 means it is not a member,
- -EINVAL indicates that the signo argument is invalid.
Given this behavior, I believe the return value should be handled explicitly as shown below:
```
int ret = nxsig_ismember(&set, signo);
if (ret == 1)
{
/* signo is a member */
}
else if (ret == 0)
{
/* signo is not a member */
}
else
{
/* ret == -EINVAL, signo is invalid */
}
```
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sched/signal: Fix nxsig_ismember() return value behavior [nuttx]
Posted by "wangchdo (via GitHub)" <gi...@apache.org>.
wangchdo commented on code in PR #17359:
URL: https://github.com/apache/nuttx/pull/17359#discussion_r2549131668
##########
fs/vfs/fs_signalfd.c:
##########
@@ -149,7 +149,7 @@ static int signalfd_file_close(FAR struct file *filep)
for (signo = MIN_SIGNO; signo <= MAX_SIGNO; signo++)
{
- if (nxsig_ismember(&dev->sigmask, signo))
+ if (nxsig_ismember(&dev->sigmask, signo) == 1)
Review Comment:
I think `nxsig_ismember()` is intended to implement the POSIX API` sigismember()`.
According to` sigismember()`, a return value of 1 means the specified signal is a member of the set, and 0 means it is not.
In addition, `group_signal_handler(`) also checks for `== 1`.
I changed here because I think these usages should be consistent.
Maybe I should not change here and go to update `group_signal_handler() `to not use `==1`?
<img width="810" height="816" alt="image" src="https://github.com/user-attachments/assets/6851538c-647f-4fbc-866f-4e54d4516edb" />
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sched/signal: Fix nxsig_ismember() return value behavior [nuttx]
Posted by "wangchdo (via GitHub)" <gi...@apache.org>.
wangchdo commented on code in PR #17359:
URL: https://github.com/apache/nuttx/pull/17359#discussion_r2549131668
##########
fs/vfs/fs_signalfd.c:
##########
@@ -149,7 +149,7 @@ static int signalfd_file_close(FAR struct file *filep)
for (signo = MIN_SIGNO; signo <= MAX_SIGNO; signo++)
{
- if (nxsig_ismember(&dev->sigmask, signo))
+ if (nxsig_ismember(&dev->sigmask, signo) == 1)
Review Comment:
I think `nxsig_ismember()` is intended to implement the POSIX API` sigismember()`.
According to` sigismember()`, a return value of 1 means the specified signal is a member of the set, and 0 means it is not.
In addition, `group_signal_handler(`) also checks for `== 1`. I think these usages should be consistent. Maybe we need to update `group_signal_handler() `to not use `==1`?
<img width="810" height="816" alt="image" src="https://github.com/user-attachments/assets/6851538c-647f-4fbc-866f-4e54d4516edb" />
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sched/signal: Fix nxsig_ismember() return value behavior [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #17359:
URL: https://github.com/apache/nuttx/pull/17359#discussion_r2549089845
##########
fs/vfs/fs_signalfd.c:
##########
@@ -149,7 +149,7 @@ static int signalfd_file_close(FAR struct file *filep)
for (signo = MIN_SIGNO; signo <= MAX_SIGNO; signo++)
{
- if (nxsig_ismember(&dev->sigmask, signo))
+ if (nxsig_ismember(&dev->sigmask, signo) == 1)
Review Comment:
why change here? it's better to keep as before since it is more robust.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sched/signal: Fix nxsig_ismember() return value behavior [nuttx]
Posted by "wangchdo (via GitHub)" <gi...@apache.org>.
wangchdo commented on code in PR #17359:
URL: https://github.com/apache/nuttx/pull/17359#discussion_r2549145998
##########
libs/libc/signal/sig_ismember.c:
##########
@@ -68,7 +68,7 @@ int nxsig_ismember(FAR const sigset_t *set, int signo)
{
/* Check if the signal is in the set */
- return ((set->_elem[_SIGSET_NDX(signo)] & _SIGNO2SET(signo)) != 0);
+ return (set->_elem[_SIGSET_NDX(signo)] & _SIGNO2SET(signo)) ? 1 : 0;
Review Comment:
I couldn't decide which one is better, so I asked ChatGPT. It said the two implementations are functionally equivalent, but suggested using `? 1 : 0` because it is clearer to read.
How do you think?
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sched/signal: Fix nxsig_ismember() return value behavior [nuttx]
Posted by "wangchdo (via GitHub)" <gi...@apache.org>.
wangchdo commented on code in PR #17359:
URL: https://github.com/apache/nuttx/pull/17359#discussion_r2553715053
##########
fs/vfs/fs_signalfd.c:
##########
@@ -149,7 +149,7 @@ static int signalfd_file_close(FAR struct file *filep)
for (signo = MIN_SIGNO; signo <= MAX_SIGNO; signo++)
{
- if (nxsig_ismember(&dev->sigmask, signo))
+ if (nxsig_ismember(&dev->sigmask, signo) == 1)
Review Comment:
Could you help explain why this approach is considered better?
According to the definition of this API, the return value follows a clear convention:
- 1 means the signal is a member of the set,
- 0 means it is not a member,
- -EINVAL indicates that the signo argument is invalid.
Given this behavior, I think the return value should be handled explicitly as shown below:
```
int ret = nxsig_ismember(&sigset, signo);
if (ret == 1)
{
/* signo is a member */
}
else if (ret == 0)
{
/* signo is not a member */
}
else
{
/* ret == -EINVAL, signo is invalid */
}
```
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sched/signal: Fix nxsig_ismember() return value behavior [nuttx]
Posted by "wangchdo (via GitHub)" <gi...@apache.org>.
wangchdo commented on code in PR #17359:
URL: https://github.com/apache/nuttx/pull/17359#discussion_r2553715053
##########
fs/vfs/fs_signalfd.c:
##########
@@ -149,7 +149,7 @@ static int signalfd_file_close(FAR struct file *filep)
for (signo = MIN_SIGNO; signo <= MAX_SIGNO; signo++)
{
- if (nxsig_ismember(&dev->sigmask, signo))
+ if (nxsig_ismember(&dev->sigmask, signo) == 1)
Review Comment:
Could you help explain why this approach is considered better?
According to the definition of this API, the return value follows a clear convention:
- 1 means the signal is a member of the set,
- 0 means it is not a member,
- -EINVAL indicates that the signo argument is invalid.
Given this behavior, I believe the return value should be handled explicitly as shown below:
```
int ret = nxsig_ismember(&sigset, signo);
if (ret == 1)
{
/* signo is a member */
}
else if (ret == 0)
{
/* signo is not a member */
}
else
{
/* ret == -EINVAL, signo is invalid */
}
```
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sched/signal: Fix nxsig_ismember() return value behavior [nuttx]
Posted by "wangchdo (via GitHub)" <gi...@apache.org>.
wangchdo commented on code in PR #17359:
URL: https://github.com/apache/nuttx/pull/17359#discussion_r2549145998
##########
libs/libc/signal/sig_ismember.c:
##########
@@ -68,7 +68,7 @@ int nxsig_ismember(FAR const sigset_t *set, int signo)
{
/* Check if the signal is in the set */
- return ((set->_elem[_SIGSET_NDX(signo)] & _SIGNO2SET(signo)) != 0);
+ return (set->_elem[_SIGSET_NDX(signo)] & _SIGNO2SET(signo)) ? 1 : 0;
Review Comment:
I couldn't decide which one is better, so I asked ChatGPT. It said the two implementations are functionally equivalent, but suggested using ? 1 : 0 because it is clearer to read.
How do you think?
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sched/signal: Fix nxsig_ismember() return value behavior [nuttx]
Posted by "wangchdo (via GitHub)" <gi...@apache.org>.
wangchdo commented on code in PR #17359:
URL: https://github.com/apache/nuttx/pull/17359#discussion_r2551429657
##########
fs/vfs/fs_signalfd.c:
##########
@@ -149,7 +149,7 @@ static int signalfd_file_close(FAR struct file *filep)
for (signo = MIN_SIGNO; signo <= MAX_SIGNO; signo++)
{
- if (nxsig_ismember(&dev->sigmask, signo))
+ if (nxsig_ismember(&dev->sigmask, signo) == 1)
Review Comment:
Upon reconsideration, I believe check == 1 is more robust. Since this function may return -EINVAL for invalid parameters, the previous implementation would treat this as success in the condition check, which is incorrect.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sched/signal: Fix nxsig_ismember() return value behavior [nuttx]
Posted by "wangchdo (via GitHub)" <gi...@apache.org>.
wangchdo commented on code in PR #17359:
URL: https://github.com/apache/nuttx/pull/17359#discussion_r2553715053
##########
fs/vfs/fs_signalfd.c:
##########
@@ -149,7 +149,7 @@ static int signalfd_file_close(FAR struct file *filep)
for (signo = MIN_SIGNO; signo <= MAX_SIGNO; signo++)
{
- if (nxsig_ismember(&dev->sigmask, signo))
+ if (nxsig_ismember(&dev->sigmask, signo) == 1)
Review Comment:
Could you help explain why this approach is considered better?
According to the definition of this API, the return value follows a clear convention:
- 1 means the signal is a member of the set,
- 0 means it is not a member,
- -EINVAL indicates that the signo argument is invalid.
Given this behavior, I believe the return value should be handled explicitly as shown below:
```
int ret = nxsig_ismember(&sigset, signo);
if (ret == 1)
{
/* signo is a member */
}
else if (ret == 0)
{
/* signo is not a member */
}
else
{
/* ret == -EINVAL, signo is invalid */
}
```
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sched/signal: Fix nxsig_ismember() return value behavior [nuttx]
Posted by "wangchdo (via GitHub)" <gi...@apache.org>.
wangchdo commented on code in PR #17359:
URL: https://github.com/apache/nuttx/pull/17359#discussion_r2551429657
##########
fs/vfs/fs_signalfd.c:
##########
@@ -149,7 +149,7 @@ static int signalfd_file_close(FAR struct file *filep)
for (signo = MIN_SIGNO; signo <= MAX_SIGNO; signo++)
{
- if (nxsig_ismember(&dev->sigmask, signo))
+ if (nxsig_ismember(&dev->sigmask, signo) == 1)
Review Comment:
I think about this again, and I think check ==1 is more robust, because this function may return value -EINVAL if the para is invalid, if we keep the previous implementation, this will be treated as success in the condition check, this is incorrect
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sched/signal: Fix nxsig_ismember() return value behavior [nuttx]
Posted by "wangchdo (via GitHub)" <gi...@apache.org>.
wangchdo commented on code in PR #17359:
URL: https://github.com/apache/nuttx/pull/17359#discussion_r2553715053
##########
fs/vfs/fs_signalfd.c:
##########
@@ -149,7 +149,7 @@ static int signalfd_file_close(FAR struct file *filep)
for (signo = MIN_SIGNO; signo <= MAX_SIGNO; signo++)
{
- if (nxsig_ismember(&dev->sigmask, signo))
+ if (nxsig_ismember(&dev->sigmask, signo) == 1)
Review Comment:
Could you please help explain why this approach is considered better?
According to the definition of this API, the return value follows a clear convention:
- 1 means the signal is a member of the set,
- 0 means it is not a member,
- -EINVAL indicates that the signo argument is invalid.
Given this behavior, I think the return value should be handled explicitly as shown below:
```
int ret = nxsig_ismember(&sigset, signo);
if (ret == 1)
{
/* signo is a member */
}
else if (ret == 0)
{
/* signo is not a member */
}
else
{
/* ret == -EINVAL, signo is invalid */
}
```
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org