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 2021/06/01 10:37:54 UTC
[GitHub] [incubator-nuttx-apps] RoCorbera opened a new pull request #743: add SIGTERM handling in leds example
RoCorbera opened a new pull request #743:
URL: https://github.com/apache/incubator-nuttx-apps/pull/743
## Summary
Based on PR#742 for having a kill \<pid\> that will send SIGTERM default signal to the task,
this example of a "daemon leds" will handle SIGTERM in order to gracefully terminate itself.
## Impact
It allows to restart this daemon after correctly terminating it. No need to reset the board to reestablish a good state.
## Testing
Tested in flat mode with CONFIG_SIG_DEFAULT=y as well as undefined.
--
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-apps] RoCorbera commented on a change in pull request #743: add SIGTERM handling in leds example
Posted by GitBox <gi...@apache.org>.
RoCorbera commented on a change in pull request #743:
URL: https://github.com/apache/incubator-nuttx-apps/pull/743#discussion_r643129907
##########
File path: examples/leds/leds_main.c
##########
@@ -59,6 +45,26 @@ static bool g_led_daemon_started;
* Private Functions
****************************************************************************/
+/****************************************************************************
+ * Name: sigterm_action
+ ****************************************************************************/
+
+void sigterm_action(int signo, siginfo_t *siginfo, void *arg)
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-apps] acassis commented on a change in pull request #743: add SIGTERM handling in leds example
Posted by GitBox <gi...@apache.org>.
acassis commented on a change in pull request #743:
URL: https://github.com/apache/incubator-nuttx-apps/pull/743#discussion_r643068475
##########
File path: examples/leds/leds_main.c
##########
@@ -170,16 +197,25 @@ static int led_daemon(int argc, char *argv[])
goto errout_with_fd;
}
- usleep(500*1000L);
+ usleep(500 * 1000L);
+ }
+
+ /* treats signal termination of the task */
+
+ if (g_led_daemon_started == false)
+ {
+ /* task terminated by a SIGTERM */
+
+ exit(EXIT_SUCCESS);
Review comment:
I think you don't need to include this block, when the execution exits the while() the g_led_daemon_started will be false for sure. Just let the default flow to flow, closing the fd "manually" etc
##########
File path: examples/leds/leds_main.c
##########
@@ -170,16 +197,25 @@ static int led_daemon(int argc, char *argv[])
goto errout_with_fd;
}
- usleep(500*1000L);
+ usleep(500 * 1000L);
+ }
+
+ /* treats signal termination of the task */
+
+ if (g_led_daemon_started == false)
+ {
+ /* task terminated by a SIGTERM */
+
+ exit(EXIT_SUCCESS);
}
-errout_with_fd:
- close(fd);
+ errout_with_fd:
+ close(fd);
-errout:
- g_led_daemon_started = false;
+ errout:
+ g_led_daemon_started = false;
Review comment:
These lines:
errout:
g_led_daemon_started = false;
now are redundant, you can remove it
--
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-apps] xiaoxiang781216 commented on a change in pull request #743: add SIGTERM handling in leds example
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #743:
URL: https://github.com/apache/incubator-nuttx-apps/pull/743#discussion_r643070778
##########
File path: examples/leds/leds_main.c
##########
@@ -59,6 +45,26 @@ static bool g_led_daemon_started;
* Private Functions
****************************************************************************/
+/****************************************************************************
+ * Name: sigterm_action
+ ****************************************************************************/
+
+void sigterm_action(int signo, siginfo_t *siginfo, void *arg)
Review comment:
Please merge the two patch into one, thanks.
--
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-apps] saramonteiro commented on pull request #743: add SIGTERM handling in leds example
Posted by GitBox <gi...@apache.org>.
saramonteiro commented on pull request #743:
URL: https://github.com/apache/incubator-nuttx-apps/pull/743#issuecomment-852076911
LGTM
--
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-apps] RoCorbera commented on a change in pull request #743: add SIGTERM handling in leds example
Posted by GitBox <gi...@apache.org>.
RoCorbera commented on a change in pull request #743:
URL: https://github.com/apache/incubator-nuttx-apps/pull/743#discussion_r643061980
##########
File path: examples/leds/leds_main.c
##########
@@ -59,6 +45,26 @@ static bool g_led_daemon_started;
* Private Functions
****************************************************************************/
+/****************************************************************************
+ * Name: sigterm_action
+ ****************************************************************************/
+
+void sigterm_action(int signo, siginfo_t *siginfo, void *arg)
Review comment:
Yes, it will better. I'll change it and commit again.
--
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-apps] RoCorbera commented on a change in pull request #743: add SIGTERM handling in leds example
Posted by GitBox <gi...@apache.org>.
RoCorbera commented on a change in pull request #743:
URL: https://github.com/apache/incubator-nuttx-apps/pull/743#discussion_r643094095
##########
File path: examples/leds/leds_main.c
##########
@@ -170,16 +197,25 @@ static int led_daemon(int argc, char *argv[])
goto errout_with_fd;
}
- usleep(500*1000L);
+ usleep(500 * 1000L);
+ }
+
+ /* treats signal termination of the task */
+
+ if (g_led_daemon_started == false)
+ {
+ /* task terminated by a SIGTERM */
+
+ exit(EXIT_SUCCESS);
}
-errout_with_fd:
- close(fd);
+ errout_with_fd:
+ close(fd);
-errout:
- g_led_daemon_started = false;
+ errout:
+ g_led_daemon_started = false;
Review comment:
> These lines:
> errout:
> g_led_daemon_started = false;
>
> now are redundant, you can remove it
Not sure because there is a "goto errout;" in the code. The lines of code before that are executed when the while() loop exits due to change on g_led_daemon_started value change, in which case the function exits. Thus the errout: code will only be executed when an error occurs.
--
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-apps] RoCorbera commented on a change in pull request #743: add SIGTERM handling in leds example
Posted by GitBox <gi...@apache.org>.
RoCorbera commented on a change in pull request #743:
URL: https://github.com/apache/incubator-nuttx-apps/pull/743#discussion_r643061980
##########
File path: examples/leds/leds_main.c
##########
@@ -59,6 +45,26 @@ static bool g_led_daemon_started;
* Private Functions
****************************************************************************/
+/****************************************************************************
+ * Name: sigterm_action
+ ****************************************************************************/
+
+void sigterm_action(int signo, siginfo_t *siginfo, void *arg)
Review comment:
Yes, it will better, specially when replicating this functionality to other examples.
I'll change it and commit again.
--
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-apps] RoCorbera commented on a change in pull request #743: add SIGTERM handling in leds example
Posted by GitBox <gi...@apache.org>.
RoCorbera commented on a change in pull request #743:
URL: https://github.com/apache/incubator-nuttx-apps/pull/743#discussion_r643100473
##########
File path: examples/leds/leds_main.c
##########
@@ -170,16 +197,25 @@ static int led_daemon(int argc, char *argv[])
goto errout_with_fd;
}
- usleep(500*1000L);
+ usleep(500 * 1000L);
+ }
+
+ /* treats signal termination of the task */
+
+ if (g_led_daemon_started == false)
+ {
+ /* task terminated by a SIGTERM */
+
+ exit(EXIT_SUCCESS);
Review comment:
The idea was to force the execution of a exit(EXIT_SUCCESS) instead of exiting with error code (last line if normal flow).
POSIX exit() terminates the task, closes all fds, etc. I think it is safer.
I can remove the if(...) { ... }, but the not the exit(EXIT_SUCCESS) given it was correctly terminated by a SIGTERM at that 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] acassis commented on a change in pull request #743: add SIGTERM handling in leds example
Posted by GitBox <gi...@apache.org>.
acassis commented on a change in pull request #743:
URL: https://github.com/apache/incubator-nuttx-apps/pull/743#discussion_r643117445
##########
File path: examples/leds/leds_main.c
##########
@@ -170,16 +197,25 @@ static int led_daemon(int argc, char *argv[])
goto errout_with_fd;
}
- usleep(500*1000L);
+ usleep(500 * 1000L);
+ }
+
+ /* treats signal termination of the task */
+
+ if (g_led_daemon_started == false)
+ {
+ /* task terminated by a SIGTERM */
+
+ exit(EXIT_SUCCESS);
}
-errout_with_fd:
- close(fd);
+ errout_with_fd:
+ close(fd);
-errout:
- g_led_daemon_started = false;
+ errout:
+ g_led_daemon_started = false;
Review comment:
right, you could just keep the "errout:", although this redundant code is not an issue.
--
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-apps] acassis commented on a change in pull request #743: add SIGTERM handling in leds example
Posted by GitBox <gi...@apache.org>.
acassis commented on a change in pull request #743:
URL: https://github.com/apache/incubator-nuttx-apps/pull/743#discussion_r643117445
##########
File path: examples/leds/leds_main.c
##########
@@ -170,16 +197,25 @@ static int led_daemon(int argc, char *argv[])
goto errout_with_fd;
}
- usleep(500*1000L);
+ usleep(500 * 1000L);
+ }
+
+ /* treats signal termination of the task */
+
+ if (g_led_daemon_started == false)
+ {
+ /* task terminated by a SIGTERM */
+
+ exit(EXIT_SUCCESS);
}
-errout_with_fd:
- close(fd);
+ errout_with_fd:
+ close(fd);
-errout:
- g_led_daemon_started = false;
+ errout:
+ g_led_daemon_started = false;
Review comment:
right, you could just see the "errout:", although this redundant code is not an issue.
--
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-apps] RoCorbera commented on a change in pull request #743: add SIGTERM handling in leds example
Posted by GitBox <gi...@apache.org>.
RoCorbera commented on a change in pull request #743:
URL: https://github.com/apache/incubator-nuttx-apps/pull/743#discussion_r643181403
##########
File path: examples/leds/leds_main.c
##########
@@ -170,16 +197,25 @@ static int led_daemon(int argc, char *argv[])
goto errout_with_fd;
}
- usleep(500*1000L);
+ usleep(500 * 1000L);
+ }
+
+ /* treats signal termination of the task */
+
+ if (g_led_daemon_started == false)
+ {
+ /* task terminated by a SIGTERM */
+
+ exit(EXIT_SUCCESS);
}
-errout_with_fd:
- close(fd);
+ errout_with_fd:
+ close(fd);
-errout:
- g_led_daemon_started = false;
+ errout:
+ g_led_daemon_started = false;
Review comment:
OK. I'll remove the "if (g_led ...)"
I put it there actually to make it explicit for others when reading the code, thus indeed redundant.
Most probably the compiler would optimize it anyway. I'll leave just the commentary for clearness.
--
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-apps] acassis commented on pull request #743: add SIGTERM handling in leds example
Posted by GitBox <gi...@apache.org>.
acassis commented on pull request #743:
URL: https://github.com/apache/incubator-nuttx-apps/pull/743#issuecomment-852367668
Very good! Kudos!!!
--
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-apps] acassis commented on a change in pull request #743: add SIGTERM handling in leds example
Posted by GitBox <gi...@apache.org>.
acassis commented on a change in pull request #743:
URL: https://github.com/apache/incubator-nuttx-apps/pull/743#discussion_r643198193
##########
File path: examples/leds/leds_main.c
##########
@@ -170,16 +197,25 @@ static int led_daemon(int argc, char *argv[])
goto errout_with_fd;
}
- usleep(500*1000L);
+ usleep(500 * 1000L);
+ }
+
+ /* treats signal termination of the task */
+
+ if (g_led_daemon_started == false)
+ {
+ /* task terminated by a SIGTERM */
+
+ exit(EXIT_SUCCESS);
Review comment:
Ok, just remove the if() will be fine
--
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-apps] acassis merged pull request #743: add SIGTERM handling in leds example
Posted by GitBox <gi...@apache.org>.
acassis merged pull request #743:
URL: https://github.com/apache/incubator-nuttx-apps/pull/743
--
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-apps] acassis commented on a change in pull request #743: add SIGTERM handling in leds example
Posted by GitBox <gi...@apache.org>.
acassis commented on a change in pull request #743:
URL: https://github.com/apache/incubator-nuttx-apps/pull/743#discussion_r643151072
##########
File path: examples/leds/leds_main.c
##########
@@ -170,16 +197,25 @@ static int led_daemon(int argc, char *argv[])
goto errout_with_fd;
}
- usleep(500*1000L);
+ usleep(500 * 1000L);
+ }
+
+ /* treats signal termination of the task */
+
+ if (g_led_daemon_started == false)
+ {
+ /* task terminated by a SIGTERM */
+
+ exit(EXIT_SUCCESS);
}
-errout_with_fd:
- close(fd);
+ errout_with_fd:
+ close(fd);
-errout:
- g_led_daemon_started = false;
+ errout:
+ g_led_daemon_started = false;
Review comment:
When exiting with SIGTERM it will be redundant, because g_led_daemon_started is already false (assuming you remove the "if (g_led_daemon_started == false)" block that I suggested.
--
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-apps] acassis commented on a change in pull request #743: add SIGTERM handling in leds example
Posted by GitBox <gi...@apache.org>.
acassis commented on a change in pull request #743:
URL: https://github.com/apache/incubator-nuttx-apps/pull/743#discussion_r643140403
##########
File path: examples/leds/leds_main.c
##########
@@ -170,16 +197,25 @@ static int led_daemon(int argc, char *argv[])
goto errout_with_fd;
}
- usleep(500*1000L);
+ usleep(500 * 1000L);
+ }
+
+ /* treats signal termination of the task */
+
+ if (g_led_daemon_started == false)
+ {
+ /* task terminated by a SIGTERM */
+
+ exit(EXIT_SUCCESS);
}
-errout_with_fd:
- close(fd);
+ errout_with_fd:
+ close(fd);
-errout:
- g_led_daemon_started = false;
+ errout:
+ g_led_daemon_started = false;
Review comment:
I see! You are right! It needs to kept in case of failure during the start process!
--
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-apps] RoCorbera commented on a change in pull request #743: add SIGTERM handling in leds example
Posted by GitBox <gi...@apache.org>.
RoCorbera commented on a change in pull request #743:
URL: https://github.com/apache/incubator-nuttx-apps/pull/743#discussion_r643058661
##########
File path: examples/leds/leds_main.c
##########
@@ -170,16 +197,25 @@ static int led_daemon(int argc, char *argv[])
goto errout_with_fd;
}
- usleep(500*1000L);
+ usleep(500 * 1000L);
+ }
+
+ /* treats signal termination of the task */
+
+ if (g_led_daemon_started == false)
+ {
+ /* task terminated by a SIGTERM */
+
+ exit(EXIT_SUCCESS);
}
-errout_with_fd:
- close(fd);
+ errout_with_fd:
+ close(fd);
-errout:
- g_led_daemon_started = false;
+ errout:
+ g_led_daemon_started = false;
Review comment:
Actually there is no change except for indentation.
The change is due to tools/checkpatch.sh message.
--
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-apps] xiaoxiang781216 commented on a change in pull request #743: add SIGTERM handling in leds example
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #743:
URL: https://github.com/apache/incubator-nuttx-apps/pull/743#discussion_r643053675
##########
File path: examples/leds/leds_main.c
##########
@@ -59,6 +45,26 @@ static bool g_led_daemon_started;
* Private Functions
****************************************************************************/
+/****************************************************************************
+ * Name: sigterm_action
+ ****************************************************************************/
+
+void sigterm_action(int signo, siginfo_t *siginfo, void *arg)
Review comment:
add static
##########
File path: examples/leds/leds_main.c
##########
@@ -170,16 +197,25 @@ static int led_daemon(int argc, char *argv[])
goto errout_with_fd;
}
- usleep(500*1000L);
+ usleep(500 * 1000L);
+ }
+
+ /* treats signal termination of the task */
+
+ if (g_led_daemon_started == false)
+ {
+ /* task terminated by a SIGTERM */
+
+ exit(EXIT_SUCCESS);
}
-errout_with_fd:
- close(fd);
+ errout_with_fd:
+ close(fd);
-errout:
- g_led_daemon_started = false;
+ errout:
+ g_led_daemon_started = false;
Review comment:
why change from line 176-182
--
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-apps] RoCorbera commented on a change in pull request #743: add SIGTERM handling in leds example
Posted by GitBox <gi...@apache.org>.
RoCorbera commented on a change in pull request #743:
URL: https://github.com/apache/incubator-nuttx-apps/pull/743#discussion_r643089050
##########
File path: examples/leds/leds_main.c
##########
@@ -170,16 +197,25 @@ static int led_daemon(int argc, char *argv[])
goto errout_with_fd;
}
- usleep(500*1000L);
+ usleep(500 * 1000L);
+ }
+
+ /* treats signal termination of the task */
+
+ if (g_led_daemon_started == false)
+ {
+ /* task terminated by a SIGTERM */
+
+ exit(EXIT_SUCCESS);
}
-errout_with_fd:
- close(fd);
+ errout_with_fd:
+ close(fd);
-errout:
- g_led_daemon_started = false;
+ errout:
+ g_led_daemon_started = false;
Review comment:
In fact there were two odd error messages from checkpatch.sh.
One that related to the indentation of the very last "return" on lines 176-182.
The other saying that the path in line 2 (original source code) had to be related to "/" as it was not a nuttx path (or something like that). The point is that this error was related to a commentary block...
In order to fix I had to change the commentaries to the APACHE message... even with a path, but after that the error was gone.
--
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-apps] RoCorbera commented on a change in pull request #743: add SIGTERM handling in leds example
Posted by GitBox <gi...@apache.org>.
RoCorbera commented on a change in pull request #743:
URL: https://github.com/apache/incubator-nuttx-apps/pull/743#discussion_r643132697
##########
File path: examples/leds/leds_main.c
##########
@@ -170,16 +197,25 @@ static int led_daemon(int argc, char *argv[])
goto errout_with_fd;
}
- usleep(500*1000L);
+ usleep(500 * 1000L);
+ }
+
+ /* treats signal termination of the task */
+
+ if (g_led_daemon_started == false)
+ {
+ /* task terminated by a SIGTERM */
+
+ exit(EXIT_SUCCESS);
}
-errout_with_fd:
- close(fd);
+ errout_with_fd:
+ close(fd);
-errout:
- g_led_daemon_started = false;
+ errout:
+ g_led_daemon_started = false;
Review comment:
Acctually it is related to exiting successfully after a normal termination with SIGTERM, or exiting with error code (goto errout: and other labels).
--
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-apps] xiaoxiang781216 commented on a change in pull request #743: add SIGTERM handling in leds example
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #743:
URL: https://github.com/apache/incubator-nuttx-apps/pull/743#discussion_r643069382
##########
File path: examples/leds/leds_main.c
##########
@@ -170,16 +197,25 @@ static int led_daemon(int argc, char *argv[])
goto errout_with_fd;
}
- usleep(500*1000L);
+ usleep(500 * 1000L);
+ }
+
+ /* treats signal termination of the task */
+
+ if (g_led_daemon_started == false)
+ {
+ /* task terminated by a SIGTERM */
+
+ exit(EXIT_SUCCESS);
}
-errout_with_fd:
- close(fd);
+ errout_with_fd:
+ close(fd);
-errout:
- g_led_daemon_started = false;
+ errout:
+ g_led_daemon_started = false;
Review comment:
But the old code looks good, what warning do you see?
--
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