| Message ID | 20220728194058.9622-1-ayoub.zaki@embexus.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [V3] replace system("reboot") calls by native glibc call reboot() | expand |
On Thu, Jul 28, 2022 at 4:41 PM Ayoub Zaki <ayoub.zaki@embexus.com> wrote: > > Signed-off-by: Ayoub Zaki <ayoub.zaki@embexus.com> Please add a commit log with details explaining the reason for this patch.
It's self explaining ! it doesn't work on systems on which reboot binary is not installed or restricted to execute ( case of selinux). On 28.07.22 21:43, Fabio Estevam wrote: > On Thu, Jul 28, 2022 at 4:41 PM Ayoub Zaki <ayoub.zaki@embexus.com> wrote: >> Signed-off-by: Ayoub Zaki <ayoub.zaki@embexus.com> > Please add a commit log with details explaining the reason for this patch. Mit freundlichen Grüßen / Kind regards
On Thu, Jul 28, 2022 at 4:46 PM Ayoub Zaki <ayoub.zaki@embexus.com> wrote: > > It's self explaining ! No, it is not. It is not good practice to submit patches without commit logs. > it doesn't work on systems on which reboot binary is not installed or > restricted to execute ( case of selinux). Then just write this information into the commit log.
Hi Ayoub, On 28.07.22 22:10, Fabio Estevam wrote: > On Thu, Jul 28, 2022 at 4:46 PM Ayoub Zaki <ayoub.zaki@embexus.com> wrote: >> >> It's self explaining ! > > No, it is not. It is not, calling system is already bad and a reason enough to change, but the real reason was hidden. > > It is not good practice to submit patches without commit logs. > >> it doesn't work on systems on which reboot binary is not installed or >> restricted to execute ( case of selinux). > > Then just write this information into the commit log. > Indeed - I have added myself this comment to the commit log before merging. Best regards, Stefano
Digging an old thread... Sorry I saw the patch at the time and didn't quite realize the implication. Ayoub Zaki wrote on Thu, Jul 28, 2022 at 09:40:58PM +0200: > - if (system("reboot") < 0) { /* It should never happen */ > + sync(); > + if (reboot(LINUX_REBOOT_CMD_RESTART) < 0) { /* It should never happen */ The two are not quite equivalent, though. reboot() will immediately reboot, e.g. is equivalent to `reboot -f`, and doesn't stop any service. This means that if you have any user-facing applications that require a smooth shutdown (saving state to disk, sending some message on network) then any system shutdown script (or ExecStop commands for systemd) will not be used. It turns out that in my case I'm not rebooting through these two calls so that ended up not to matter for me, but I feel that, to reply to the comment on v2 quoted below, it is much more reliable to use the shutdown scripts than a mere sleep(5) here. What if some high load process is hogging up the CPU and 5 seconds weren't enough for whatever application graceful shutdown? Stefano Babic wrote on Thu, Jul 28, 2022 at 08:50:44AM +0200: > > - sleep(5); > > + sync(); > > There are some reasons to delay a restart. One of them is that the reboot > itself is demanded by one of these tools, but the application (running with > lower rights) was also informed (via progress interface or whatever) and > have time to gracefully shutdown. That means it is not just a delay to allow > filesystem sync. Unfortunately, the proper way to shutdown without the reboot command depends on which init system is used as there is nothing portable (busybox init takes some signal to pid 1, systemd might handle some signal as a fallback but probably expects some dbus message, s6 init has its own IPC...), so for me system() was the correct solution... But as said above I'm not using this so just reporting for awareness if anyone has problems due to this.
Hi Dominique, On 05.04.23 06:12, Dominique MARTINET wrote: > Digging an old thread... Sorry I saw the patch at the time and didn't > quite realize the implication. > No problem ! > Ayoub Zaki wrote on Thu, Jul 28, 2022 at 09:40:58PM +0200: >> - if (system("reboot") < 0) { /* It should never happen */ >> + sync(); >> + if (reboot(LINUX_REBOOT_CMD_RESTART) < 0) { /* It should never happen */ > > The two are not quite equivalent, though. > They are not, and it is not thought they are the same. > reboot() will immediately reboot, e.g. is equivalent to `reboot -f`, and > doesn't stop any service. Correct > This means that if you have any user-facing applications that require a > smooth shutdown (saving state to disk, sending some message on network) > then any system shutdown script (or ExecStop commands for systemd) will > not be used. Anyway, a power-cut can happen at any moment on (most) embedded devices, and the effect is like a glibc reboot. Embedded Devices must be prepared for that, and they should be aware that a "reboot" (due intentionally or not) can happen at any time. If, after an update, system can be restarted under some circunstances, this should be well synchronized. Just calling system("reboot") helps in some cases, in most other cases not. Just in these days I discussed about a use case with a Wallbox for EV charging. Update can run in background, but a restart must be synchronized and happen if no car is charging. That means, project should find the own way. SWUpdate makes available the "bricks" to build the update, the integrator / developer should then add the required glue logic for the specific project. By the way, you can still have the old behavior if you use swupdate-progress as wrapper: you run it with "-e <script>", where of course in the script you can put own logic and call /usr/sbin/reboot at the end (or shutdown, or whatever). > > It turns out that in my case I'm not rebooting through these two calls > so that ended up not to matter for me, Because probably you have also a specific case, and you did your homework to well integrate SWUpdate into your project ;-) > but I feel that, to reply to the > comment on v2 quoted below, it is much more reliable to use the shutdown > scripts than a mere sleep(5) here. What if some high load process is > hogging up the CPU and 5 seconds weren't enough for whatever application > graceful shutdown? I think there is not a generic solution. It depends on the project. In that case, a restart process must be individuated and maybe implemented. More complicate cases, for example where safety is important, require a more strict synchronization. I always considered swupdate-progress an example that works in most cases, but it is duty of the developers to check if it fits on the own project and maybe to write something else - your example is exactly one of these cases. > > Stefano Babic wrote on Thu, Jul 28, 2022 at 08:50:44AM +0200: >>> - sleep(5); >>> + sync(); >> >> There are some reasons to delay a restart. One of them is that the reboot >> itself is demanded by one of these tools, but the application (running with >> lower rights) was also informed (via progress interface or whatever) and >> have time to gracefully shutdown. That means it is not just a delay to allow >> filesystem sync. > > > Unfortunately, the proper way to shutdown without the reboot command > depends on which init system is used as there is nothing portable Right, I see the same. > (busybox init takes some signal to pid 1, systemd might handle some > signal as a fallback but probably expects some dbus message, s6 init has > its own IPC...), Exactly. > so for me system() was the correct solution... But as > said above I'm not using this so just reporting for awareness if anyone > has problems due to this. > Thanks for reporting - I will take into account if more people are coming into discussion and ask to go back. Best regards, Stefano
diff --git a/tools/swupdate-ipc.c b/tools/swupdate-ipc.c index ce8ec10..aa0afe5 100644 --- a/tools/swupdate-ipc.c +++ b/tools/swupdate-ipc.c @@ -23,6 +23,8 @@ #include <sys/stat.h> #include <sys/un.h> #include <sys/select.h> +#include <sys/reboot.h> +#include <linux/reboot.h> #include <arpa/inet.h> #include <netinet/in.h> #include <ifaddrs.h> @@ -571,8 +573,8 @@ static int sysrestart(cmd_t __attribute__((__unused__)) *cmd, int argc, char *a fprintf(stdout, "Ready to reboot !\n"); restart_system(ndevs); sleep(5); - - if (system("reboot") < 0) { /* It should never happen */ + sync(); + if (reboot(LINUX_REBOOT_CMD_RESTART) < 0) { /* It should never happen */ fprintf(stdout, "Please reset the board.\n"); } break; diff --git a/tools/swupdate-progress.c b/tools/swupdate-progress.c index 8910b5a..82a7881 100644 --- a/tools/swupdate-progress.c +++ b/tools/swupdate-progress.c @@ -18,6 +18,8 @@ #include <sys/stat.h> #include <sys/un.h> #include <sys/select.h> +#include <sys/reboot.h> +#include <linux/reboot.h> #include <arpa/inet.h> #include <netinet/in.h> #include <pthread.h> @@ -380,7 +382,8 @@ int main(int argc, char **argv) psplash_ok = 0; if ((msg.status == SUCCESS) && (msg.cur_step > 0) && opt_r) { sleep(5); - if (system("reboot") < 0) { /* It should never happen */ + sync(); + if (reboot(LINUX_REBOOT_CMD_RESTART) < 0) { /* It should never happen */ fprintf(stdout, "Please reset the board.\n"); } }
Signed-off-by: Ayoub Zaki <ayoub.zaki@embexus.com> --- tools/swupdate-ipc.c | 6 ++++-- tools/swupdate-progress.c | 5 ++++- 2 files changed, 8 insertions(+), 3 deletions(-)