You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2020/04/29 18:17:08 UTC
[GitHub] [incubator-nuttx] Ouss4 opened a new pull request #911: Use NuttX's signal set functions inside the OS.
Ouss4 opened a new pull request #911:
URL: https://github.com/apache/incubator-nuttx/pull/911
## Summary
Inside the OS use NuttX's signal functions. These function should not set errno and thus won't interfere with application logic.
## Impact
## Testing
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #911: Use NuttX's signal set functions inside the OS.
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #911:
URL: https://github.com/apache/incubator-nuttx/pull/911#discussion_r417615086
##########
File path: sched/signal/sig_dispatch.c
##########
@@ -319,7 +320,7 @@ int nxsig_tcbdispatch(FAR struct tcb_s *stcb, siginfo_t *info)
/************************** MASKED SIGNAL ACTIONS *************************/
- masked = (bool)sigismember(&stcb->sigprocmask, info->si_signo);
+ masked = (bool)nxsig_ismember(&stcb->sigprocmask, info->si_signo);
Review comment:
I changed it to make the cast correct.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #911: Use NuttX's signal set functions inside the OS.
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #911:
URL: https://github.com/apache/incubator-nuttx/pull/911#discussion_r417610981
##########
File path: include/nuttx/signal.h
##########
@@ -112,6 +112,68 @@ struct sigwork_s
struct timespec; /* Forward reference */
+/****************************************************************************
+ * Name: nxsig_ismember
+ *
+ * Description:
+ * This function tests whether the signal specified by signo is a member
+ * of the set specified by set.
+ *
+ * Input Parameters:
+ * set - Signal set to test
+ * signo - Signal to test for
+ *
+ * Returned Value:
+ * 1 (true), if the specified signal is a member of the set,
+ * 0 (OK or FALSE), if it is not, or
+ * -1 (ERROR) if the signal number is invalid.
Review comment:
Done.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #911: Use NuttX's signal set functions inside the OS.
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #911:
URL: https://github.com/apache/incubator-nuttx/pull/911#discussion_r417537347
##########
File path: sched/group/group_signal.c
##########
@@ -109,7 +111,8 @@ static int group_signal_handler(pid_t pid, FAR void *arg)
* probably blocked).
*/
- if (sigismember(&tcb->sigwaitmask, info->siginfo->si_signo) && !info->atcb)
+ if (nxsig_ismember(&tcb->sigwaitmask, info->siginfo->si_signo) &&
Review comment:
```suggestion
ret = nxsig_ismember(&tcb->sigwaitmask, info->siginfo->si_signo);
if (ret == 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #911: Use NuttX's signal set functions inside the OS.
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #911:
URL: https://github.com/apache/incubator-nuttx/pull/911#discussion_r417570346
##########
File path: sched/signal/sig_dispatch.c
##########
@@ -319,7 +320,7 @@ int nxsig_tcbdispatch(FAR struct tcb_s *stcb, siginfo_t *info)
/************************** MASKED SIGNAL ACTIONS *************************/
- masked = (bool)sigismember(&stcb->sigprocmask, info->si_signo);
+ masked = (bool)nxsig_ismember(&stcb->sigprocmask, info->si_signo);
Review comment:
In this case (and the following on), the signal number should be okay. Probably a DEBUGASSERT(GOOD_SIGNO( info->si_signo)) is all we need.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #911: Use NuttX's signal set functions inside the OS.
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #911:
URL: https://github.com/apache/incubator-nuttx/pull/911#discussion_r417616954
##########
File path: sched/group/group_signal.c
##########
@@ -109,7 +111,8 @@ static int group_signal_handler(pid_t pid, FAR void *arg)
* probably blocked).
*/
- if (sigismember(&tcb->sigwaitmask, info->siginfo->si_signo) && !info->atcb)
+ if (nxsig_ismember(&tcb->sigwaitmask, info->siginfo->si_signo) &&
Review comment:
I used your suggested code rather than DEBUGASSERT.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #911: Use NuttX's signal set functions inside the OS.
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #911:
URL: https://github.com/apache/incubator-nuttx/pull/911#discussion_r417569284
##########
File path: sched/signal/sig_lowest.c
##########
@@ -61,7 +63,7 @@ int nxsig_lowest(sigset_t *set)
for (signo = MIN_SIGNO; signo <= MAX_SIGNO; signo++)
{
- if (sigismember(set, signo))
+ if (nxsig_ismember(set, signo))
{
return signo;
Review comment:
But it should not fail because signo valid: MIN_SIGNO <= signo <= MAX_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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #911: Use NuttX's signal set functions inside the OS.
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #911:
URL: https://github.com/apache/incubator-nuttx/pull/911#discussion_r417521103
##########
File path: libs/libc/signal/sig_delset.c
##########
@@ -62,13 +62,12 @@
*
****************************************************************************/
-int sigdelset(FAR sigset_t *set, int signo)
+int nxsig_delset(FAR sigset_t *set, int signo)
{
/* Verify the signal */
if (!GOOD_SIGNO(signo))
{
- set_errno(EINVAL);
return ERROR;
Review comment:
Similar to the above. ERROR should never be returned inside the OS. It should return -EINVAL if the signal number is invalid. There are probably updates need to the Return Value comments above too.
##########
File path: sched/signal/sig_dispatch.c
##########
@@ -319,7 +320,7 @@ int nxsig_tcbdispatch(FAR struct tcb_s *stcb, siginfo_t *info)
/************************** MASKED SIGNAL ACTIONS *************************/
- masked = (bool)sigismember(&stcb->sigprocmask, info->si_signo);
+ masked = (bool)nxsig_ismember(&stcb->sigprocmask, info->si_signo);
Review comment:
Does this conversion to bool make sense? I think it does not account for error return values.
##########
File path: libs/libc/signal/sig_addset.c
##########
@@ -62,13 +62,12 @@
*
****************************************************************************/
-int sigaddset(FAR sigset_t *set, int signo)
+int nxsig_addset(FAR sigset_t *set, int signo)
{
/* Verify the signal */
if (!GOOD_SIGNO(signo))
{
- set_errno(EINVAL);
return ERROR;
Review comment:
```suggestion
return -EINVAL;
```
There are probably Return Value comments above that need to match.
##########
File path: libs/libc/signal/sig_addset.c
##########
@@ -62,13 +62,12 @@
*
****************************************************************************/
-int sigaddset(FAR sigset_t *set, int signo)
+int nxsig_addset(FAR sigset_t *set, int signo)
{
/* Verify the signal */
if (!GOOD_SIGNO(signo))
{
- set_errno(EINVAL);
return ERROR;
Review comment:
Similar to the above. ERROR should never be returned inside the OS. It should return -EINVAL if the signal number is invalid. there are probably changes need in the Return Value comments too.
##########
File path: sched/signal/sig_lowest.c
##########
@@ -61,7 +63,7 @@ int nxsig_lowest(sigset_t *set)
for (signo = MIN_SIGNO; signo <= MAX_SIGNO; signo++)
{
- if (sigismember(set, signo))
+ if (nxsig_ismember(set, signo))
{
return signo;
Review comment:
It will return a success if nxsig_ismember() fails.
##########
File path: include/nuttx/signal.h
##########
@@ -112,6 +112,68 @@ struct sigwork_s
struct timespec; /* Forward reference */
+/****************************************************************************
+ * Name: nxsig_ismember
+ *
+ * Description:
+ * This function tests whether the signal specified by signo is a member
+ * of the set specified by set.
+ *
+ * Input Parameters:
+ * set - Signal set to test
+ * signo - Signal to test for
+ *
+ * Returned Value:
+ * 1 (true), if the specified signal is a member of the set,
+ * 0 (OK or FALSE), if it is not, or
+ * -1 (ERROR) if the signal number is invalid.
Review comment:
```suggestion
* -EINVAL if the signal number is invalid
```
##########
File path: libs/libc/signal/sig_ismember.c
##########
@@ -63,17 +66,53 @@
*
****************************************************************************/
-int sigismember(FAR const sigset_t *set, int signo)
+int nxsig_ismember(FAR const sigset_t *set, int signo)
{
- int ret = ERROR;
-
/* Verify the signal */
- if (GOOD_SIGNO(signo))
+ if (!GOOD_SIGNO(signo))
+ {
+ return ERROR;
+ }
+ else
{
/* Check if the signal is in the set */
- ret = ((*set & SIGNO2SET(signo)) != 0);
+ return ((*set & SIGNO2SET(signo)) != 0);
+ }
+}
+
+/****************************************************************************
+ * Name: sigismember
+ *
+ * Description:
+ * This function tests whether the signal specified by signo is a member
+ * of the set specified by set.
+ *
+ * Input Parameters:
+ * set - Signal set to test
+ * signo - Signal to test for
+ *
+ * Returned Value:
+ * 1 (true), if the specified signal is a member of the set,
+ * 0 (OK or FALSE), if it is not, or
+ * -1 (ERROR) if the signal number is invalid.
+ *
+ * Assumptions:
+ *
+ ****************************************************************************/
+
+int sigismember(FAR const sigset_t *set, int signo)
+{
+ int ret;
+
+ /* Let nxsig_ismember do all of the work */
+
+ ret = nxsig_ismember(set, signo);
+ if (ret < 0)
+ {
+ set_errno(EINVAL);
Review comment:
In the general case, it should set_errno(-ret); In this case, -EINVAL should be the only value returned by nxsig_ismember() so it really doesn't matter
##########
File path: libs/libc/signal/sig_ismember.c
##########
@@ -63,17 +66,53 @@
*
****************************************************************************/
-int sigismember(FAR const sigset_t *set, int signo)
+int nxsig_ismember(FAR const sigset_t *set, int signo)
{
- int ret = ERROR;
-
/* Verify the signal */
- if (GOOD_SIGNO(signo))
+ if (!GOOD_SIGNO(signo))
+ {
+ return ERROR;
Review comment:
Similar to the above. ERROR should never be returned inside the OS. It should return -EINVAL if the signal number is invalid.
##########
File path: libs/libc/signal/sig_delset.c
##########
@@ -79,3 +78,37 @@ int sigdelset(FAR sigset_t *set, int signo)
return OK;
}
}
+
+/****************************************************************************
+ * Name: sigdelset
+ *
+ * Description:
+ * This function deletes the signal specified by signo from the signal
+ * set specified by the 'set' argument.
+ *
+ * Input Parameters:
+ * set - Signal set to delete the signal from
+ * signo - Signal to delete
+ *
+ * Returned Value:
+ * 0 (OK), or -1 (ERROR) if the signal number is invalid.
+ *
+ * Assumptions:
+ *
+ ****************************************************************************/
+
+int sigdelset(FAR sigset_t *set, int signo)
+{
+ int ret;
+
+ /* Let nxseg_delset do all the work. */
+
+ ret = nxsig_delset(set, signo);
+ if (ret < 0)
+ {
+ set_errno(EINVAL);
Review comment:
In the general case, it should set_errno(-ret); In this case, -EINVAL should be the only value returned by nxsig_delset() so it does not really matter.
##########
File path: libs/libc/signal/sig_delset.c
##########
@@ -62,13 +62,12 @@
*
****************************************************************************/
-int sigdelset(FAR sigset_t *set, int signo)
+int nxsig_delset(FAR sigset_t *set, int signo)
{
/* Verify the signal */
if (!GOOD_SIGNO(signo))
{
- set_errno(EINVAL);
return ERROR;
Review comment:
```suggestion
return -EINVAL;
```
There probably Return Value comments that need to be made to match too.
##########
File path: sched/signal/sig_dispatch.c
##########
@@ -362,7 +363,7 @@ int nxsig_tcbdispatch(FAR struct tcb_s *stcb, siginfo_t *info)
flags = enter_critical_section();
if (stcb->task_state == TSTATE_WAIT_SIG &&
- sigismember(&stcb->sigwaitmask, info->si_signo))
+ nxsig_ismember(&stcb->sigwaitmask, info->si_signo))
Review comment:
There are lots checks here like this that do not account for failure return values. They will all be treated as TRUE, that the signal is a member of the set.
##########
File path: libs/libc/signal/sig_ismember.c
##########
@@ -63,17 +66,53 @@
*
****************************************************************************/
-int sigismember(FAR const sigset_t *set, int signo)
+int nxsig_ismember(FAR const sigset_t *set, int signo)
{
- int ret = ERROR;
-
/* Verify the signal */
- if (GOOD_SIGNO(signo))
+ if (!GOOD_SIGNO(signo))
+ {
+ return ERROR;
Review comment:
```suggestion
return -EINVAL;
```
There are probably Return Value comment above that need to be made to match too.
##########
File path: include/nuttx/signal.h
##########
@@ -112,6 +112,68 @@ struct sigwork_s
struct timespec; /* Forward reference */
+/****************************************************************************
+ * Name: nxsig_ismember
+ *
+ * Description:
+ * This function tests whether the signal specified by signo is a member
+ * of the set specified by set.
+ *
+ * Input Parameters:
+ * set - Signal set to test
+ * signo - Signal to test for
+ *
+ * Returned Value:
+ * 1 (true), if the specified signal is a member of the set,
+ * 0 (OK or FALSE), if it is not, or
+ * -1 (ERROR) if the signal number is invalid.
+ *
+ * Assumptions:
+ *
+ ****************************************************************************/
+
+int nxsig_ismember(FAR const sigset_t *set, int signo);
+
+/****************************************************************************
+ * Name: nxsig_addset
+ *
+ * Description:
+ * This function adds the signal specified by signo to the signal set
+ * specified by set.
+ *
+ * Input Parameters:
+ * set - Signal set to add signal to
+ * signo - Signal to add
+ *
+ * Returned Value:
+ * 0 (OK), or -1 (ERROR) if the signal number is invalid.
+ *
+ * Assumptions:
+ *
+ ****************************************************************************/
+
+int nxsig_addset(FAR sigset_t *set, int signo);
+
+/****************************************************************************
+ * Name: nxsig_delset
+ *
+ * Description:
+ * This function deletes the signal specified by signo from the signal
+ * set specified by the 'set' argument.
+ *
+ * Input Parameters:
+ * set - Signal set to delete the signal from
+ * signo - Signal to delete
+ *
+ * Returned Value:
+ * 0 (OK), or -1 (ERROR) if the signal number is invalid.
Review comment:
```suggestion
* 0 (OK), or-EINVAL if the signal number is invalid.
```
##########
File path: sched/signal/sig_timedwait.c
##########
@@ -406,7 +406,7 @@ int nxsig_timedwait(FAR const sigset_t *set, FAR struct siginfo *info,
* that we were waiting for?
*/
- if (sigismember(set, rtcb->sigunbinfo.si_signo))
+ if (nxsig_ismember(set, rtcb->sigunbinfo.si_signo))
Review comment:
Another case that does not account for failure return values. They will all be treated as TRUE, that the signal is a member of the set.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #911: Use NuttX's signal set functions inside the OS.
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #911:
URL: https://github.com/apache/incubator-nuttx/pull/911#discussion_r417571317
##########
File path: sched/group/group_signal.c
##########
@@ -136,7 +139,7 @@ static int group_signal_handler(pid_t pid, FAR void *arg)
/* Is this signal unblocked on this thread? */
- if (!sigismember(&tcb->sigprocmask, info->siginfo->si_signo) &&
+ if (!nxsig_ismember(&tcb->sigprocmask, info->siginfo->si_signo) &&
Review comment:
Hmmm... In this case the signal number should be okay.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #911: Use NuttX's signal set functions inside the OS.
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #911:
URL: https://github.com/apache/incubator-nuttx/pull/911#discussion_r417564056
##########
File path: sched/signal/sig_timedwait.c
##########
@@ -406,7 +406,7 @@ int nxsig_timedwait(FAR const sigset_t *set, FAR struct siginfo *info,
* that we were waiting for?
*/
- if (sigismember(set, rtcb->sigunbinfo.si_signo))
+ if (nxsig_ismember(set, rtcb->sigunbinfo.si_signo))
Review comment:
```suggestion
ret = nxsig_ismember(set, rtcb->sigunbinfo.si_signo);
if (ret == 1)
```
Plus handling for the case where ret < 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #911: Use NuttX's signal set functions inside the OS.
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #911:
URL: https://github.com/apache/incubator-nuttx/pull/911#discussion_r417611858
##########
File path: include/nuttx/signal.h
##########
@@ -112,6 +112,68 @@ struct sigwork_s
struct timespec; /* Forward reference */
+/****************************************************************************
+ * Name: nxsig_ismember
+ *
+ * Description:
+ * This function tests whether the signal specified by signo is a member
+ * of the set specified by set.
+ *
+ * Input Parameters:
+ * set - Signal set to test
+ * signo - Signal to test for
+ *
+ * Returned Value:
+ * 1 (true), if the specified signal is a member of the set,
+ * 0 (OK or FALSE), if it is not, or
+ * -1 (ERROR) if the signal number is invalid.
+ *
+ * Assumptions:
+ *
+ ****************************************************************************/
+
+int nxsig_ismember(FAR const sigset_t *set, int signo);
+
+/****************************************************************************
+ * Name: nxsig_addset
+ *
+ * Description:
+ * This function adds the signal specified by signo to the signal set
+ * specified by set.
+ *
+ * Input Parameters:
+ * set - Signal set to add signal to
+ * signo - Signal to add
+ *
+ * Returned Value:
+ * 0 (OK), or -1 (ERROR) if the signal number is invalid.
Review comment:
Done.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #911: Use NuttX's signal set functions inside the OS.
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #911:
URL: https://github.com/apache/incubator-nuttx/pull/911#discussion_r417615597
##########
File path: sched/signal/sig_dispatch.c
##########
@@ -362,7 +363,7 @@ int nxsig_tcbdispatch(FAR struct tcb_s *stcb, siginfo_t *info)
flags = enter_critical_section();
if (stcb->task_state == TSTATE_WAIT_SIG &&
- sigismember(&stcb->sigwaitmask, info->si_signo))
+ nxsig_ismember(&stcb->sigwaitmask, info->si_signo))
Review comment:
In this case signo was already tested by the first call to nxsig_ismember. If we are executing this code, signo shall be valid.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #911: Use NuttX's signal set functions inside the OS.
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #911:
URL: https://github.com/apache/incubator-nuttx/pull/911#discussion_r417542052
##########
File path: sched/group/group_signal.c
##########
@@ -136,7 +139,7 @@ static int group_signal_handler(pid_t pid, FAR void *arg)
/* Is this signal unblocked on this thread? */
- if (!sigismember(&tcb->sigprocmask, info->siginfo->si_signo) &&
+ if (!nxsig_ismember(&tcb->sigprocmask, info->siginfo->si_signo) &&
Review comment:
```suggestion
ret = nxsig_ismember(&tcb->sigprocmask, info->siginfo->si_signo)
if (ret == 0 &&
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #911: Use NuttX's signal set functions inside the OS.
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #911:
URL: https://github.com/apache/incubator-nuttx/pull/911#discussion_r417566237
##########
File path: sched/signal/sig_timedwait.c
##########
@@ -406,7 +406,7 @@ int nxsig_timedwait(FAR const sigset_t *set, FAR struct siginfo *info,
* that we were waiting for?
*/
- if (sigismember(set, rtcb->sigunbinfo.si_signo))
+ if (nxsig_ismember(set, rtcb->sigunbinfo.si_signo))
Review comment:
Nevermind here. Look at the code, you can see that there is a good signal number check:
403 if (GOOD_SIGNO(rtcb->sigunbinfo.si_signo))
404 {
409 if (sigismember(set, rtcb->sigunbinfo.si_signo))
This might be the case with others as well.
Hmmm... it would reasonable to have a DEBUGASSERT that return is non-negative, but not essential.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #911: Use NuttX's signal set functions inside the OS.
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #911:
URL: https://github.com/apache/incubator-nuttx/pull/911#discussion_r417611195
##########
File path: libs/libc/signal/sig_addset.c
##########
@@ -62,13 +62,12 @@
*
****************************************************************************/
-int sigaddset(FAR sigset_t *set, int signo)
+int nxsig_addset(FAR sigset_t *set, int signo)
{
/* Verify the signal */
if (!GOOD_SIGNO(signo))
{
- set_errno(EINVAL);
return ERROR;
Review comment:
Done.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #911: Use NuttX's signal set functions inside the OS.
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #911:
URL: https://github.com/apache/incubator-nuttx/pull/911#discussion_r417611358
##########
File path: libs/libc/signal/sig_addset.c
##########
@@ -79,3 +78,37 @@ int sigaddset(FAR sigset_t *set, int signo)
return OK;
}
}
+
+/****************************************************************************
+ * Name: sigaddset
+ *
+ * Description:
+ * This function adds the signal specified by signo to the signal set
+ * specified by set.
+ *
+ * Input Parameters:
+ * set - Signal set to add signal to
+ * signo - Signal to add
+ *
+ * Returned Value:
+ * 0 (OK), or -1 (ERROR) if the signal number is invalid.
+ *
+ * Assumptions:
+ *
+ ****************************************************************************/
+
+int sigaddset(FAR sigset_t *set, int signo)
+{
+ int ret;
+
+ /* Let nxsig_addset do all the work. */
+
+ ret = nxsig_addset(set, signo);
+ if (ret < 0)
+ {
+ set_errno(EINVAL);
Review comment:
I kept EINVAL as it is the only valid errno value here.
##########
File path: libs/libc/signal/sig_delset.c
##########
@@ -62,13 +62,12 @@
*
****************************************************************************/
-int sigdelset(FAR sigset_t *set, int signo)
+int nxsig_delset(FAR sigset_t *set, int signo)
{
/* Verify the signal */
if (!GOOD_SIGNO(signo))
{
- set_errno(EINVAL);
return ERROR;
Review comment:
Done.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #911: Use NuttX's signal set functions inside the OS.
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #911:
URL: https://github.com/apache/incubator-nuttx/pull/911#discussion_r417518969
##########
File path: include/nuttx/signal.h
##########
@@ -112,6 +112,68 @@ struct sigwork_s
struct timespec; /* Forward reference */
+/****************************************************************************
+ * Name: nxsig_ismember
+ *
+ * Description:
+ * This function tests whether the signal specified by signo is a member
+ * of the set specified by set.
+ *
+ * Input Parameters:
+ * set - Signal set to test
+ * signo - Signal to test for
+ *
+ * Returned Value:
+ * 1 (true), if the specified signal is a member of the set,
+ * 0 (OK or FALSE), if it is not, or
+ * -1 (ERROR) if the signal number is invalid.
Review comment:
Internal OS functions should never return -1 (ERROR), then should return a negated errno value. In this case it should return -EINVAL:
[EINVAL]
The signo argument is not a valid signal number, or is an unsupported signal number.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #911: Use NuttX's signal set functions inside the OS.
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #911:
URL: https://github.com/apache/incubator-nuttx/pull/911#discussion_r417611696
##########
File path: libs/libc/signal/sig_ismember.c
##########
@@ -63,17 +66,53 @@
*
****************************************************************************/
-int sigismember(FAR const sigset_t *set, int signo)
+int nxsig_ismember(FAR const sigset_t *set, int signo)
{
- int ret = ERROR;
-
/* Verify the signal */
- if (GOOD_SIGNO(signo))
+ if (!GOOD_SIGNO(signo))
+ {
+ return ERROR;
Review comment:
Done.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #911: Use NuttX's signal set functions inside the OS.
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #911:
URL: https://github.com/apache/incubator-nuttx/pull/911#discussion_r417521036
##########
File path: libs/libc/signal/sig_addset.c
##########
@@ -79,3 +78,37 @@ int sigaddset(FAR sigset_t *set, int signo)
return OK;
}
}
+
+/****************************************************************************
+ * Name: sigaddset
+ *
+ * Description:
+ * This function adds the signal specified by signo to the signal set
+ * specified by set.
+ *
+ * Input Parameters:
+ * set - Signal set to add signal to
+ * signo - Signal to add
+ *
+ * Returned Value:
+ * 0 (OK), or -1 (ERROR) if the signal number is invalid.
+ *
+ * Assumptions:
+ *
+ ****************************************************************************/
+
+int sigaddset(FAR sigset_t *set, int signo)
+{
+ int ret;
+
+ /* Let nxsig_addset do all the work. */
+
+ ret = nxsig_addset(set, signo);
+ if (ret < 0)
+ {
+ set_errno(EINVAL);
Review comment:
In the general case, it should set_errno(-ret); In this case, -EINVAL should be the only value returned by nxsig_addset() so it does not matter.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #911: Use NuttX's signal set functions inside the OS.
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #911:
URL: https://github.com/apache/incubator-nuttx/pull/911#discussion_r417566237
##########
File path: sched/signal/sig_timedwait.c
##########
@@ -406,7 +406,7 @@ int nxsig_timedwait(FAR const sigset_t *set, FAR struct siginfo *info,
* that we were waiting for?
*/
- if (sigismember(set, rtcb->sigunbinfo.si_signo))
+ if (nxsig_ismember(set, rtcb->sigunbinfo.si_signo))
Review comment:
Nevermind here. Look at the code, you can see that there is a good signal number check:
403 if (GOOD_SIGNO(rtcb->sigunbinfo.si_signo))
404 {
409 if (sigismember(set, rtcb->sigunbinfo.si_signo))
This might be the case with others as well.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #911: Use NuttX's signal set functions inside the OS.
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #911:
URL: https://github.com/apache/incubator-nuttx/pull/911#discussion_r417571317
##########
File path: sched/group/group_signal.c
##########
@@ -136,7 +139,7 @@ static int group_signal_handler(pid_t pid, FAR void *arg)
/* Is this signal unblocked on this thread? */
- if (!sigismember(&tcb->sigprocmask, info->siginfo->si_signo) &&
+ if (!nxsig_ismember(&tcb->sigprocmask, info->siginfo->si_signo) &&
Review comment:
Hmmm... In this case the signal number should be okay. Probably a DEBUGASSERT(GOOD_SIGNO( info->siginfo->si_signo)) would give us sufficient protection.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #911: Use NuttX's signal set functions inside the OS.
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #911:
URL: https://github.com/apache/incubator-nuttx/pull/911#discussion_r417612005
##########
File path: libs/libc/signal/sig_addset.c
##########
@@ -62,13 +62,12 @@
*
****************************************************************************/
-int sigaddset(FAR sigset_t *set, int signo)
+int nxsig_addset(FAR sigset_t *set, int signo)
{
/* Verify the signal */
if (!GOOD_SIGNO(signo))
{
- set_errno(EINVAL);
return ERROR;
Review comment:
Done.
##########
File path: libs/libc/signal/sig_ismember.c
##########
@@ -63,17 +66,53 @@
*
****************************************************************************/
-int sigismember(FAR const sigset_t *set, int signo)
+int nxsig_ismember(FAR const sigset_t *set, int signo)
{
- int ret = ERROR;
-
/* Verify the signal */
- if (GOOD_SIGNO(signo))
+ if (!GOOD_SIGNO(signo))
+ {
+ return ERROR;
Review comment:
Done.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #911: Use NuttX's signal set functions inside the OS.
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #911:
URL: https://github.com/apache/incubator-nuttx/pull/911#discussion_r417571814
##########
File path: sched/group/group_signal.c
##########
@@ -109,7 +111,8 @@ static int group_signal_handler(pid_t pid, FAR void *arg)
* probably blocked).
*/
- if (sigismember(&tcb->sigwaitmask, info->siginfo->si_signo) && !info->atcb)
+ if (nxsig_ismember(&tcb->sigwaitmask, info->siginfo->si_signo) &&
Review comment:
Hmmm... In this case the signal number should be okay. Probably a DEBUGASSERT(GOOD_SIGNO( info->siginfo->si_signo)) would give us sufficient protection.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #911: Use NuttX's signal set functions inside the OS.
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #911:
URL: https://github.com/apache/incubator-nuttx/pull/911#discussion_r417621249
##########
File path: sched/signal/sig_dispatch.c
##########
@@ -319,7 +320,8 @@ int nxsig_tcbdispatch(FAR struct tcb_s *stcb, siginfo_t *info)
/************************** MASKED SIGNAL ACTIONS *************************/
- masked = (bool)sigismember(&stcb->sigprocmask, info->si_signo);
+ masked = (nxsig_ismember(&stcb->sigprocmask, info->si_signo) <= 0) ?
+ false : true;
Review comment:
There is really no simple solution to make this "perfect" without adding more error handling logic. If an error occurs, we will say that the signal is not masked.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #911: Use NuttX's signal set functions inside the OS.
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #911:
URL: https://github.com/apache/incubator-nuttx/pull/911#discussion_r417566237
##########
File path: sched/signal/sig_timedwait.c
##########
@@ -406,7 +406,7 @@ int nxsig_timedwait(FAR const sigset_t *set, FAR struct siginfo *info,
* that we were waiting for?
*/
- if (sigismember(set, rtcb->sigunbinfo.si_signo))
+ if (nxsig_ismember(set, rtcb->sigunbinfo.si_signo))
Review comment:
Nevermind here. Lookat the code, you can see that there is a good signal number check:
403 if (GOOD_SIGNO(rtcb->sigunbinfo.si_signo))
404 {
409 if (sigismember(set, rtcb->sigunbinfo.si_signo))
This might be the case with others as well.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org