| Message ID | 20241103084851.118639-1-oss@braunwarth.dev |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | tools: use systemd to schedule reboot | expand |
Hi Daniel, On 03.11.24 09:48, Daniel Braunwarth wrote: > On systems which are using systemd we should not use the reboot() > syscall to reboot the system. This is bypassing systemd. > The reason for this is there are use cases where it is wanted to bypass any user space tool and ask the kernel to reboot. Instead of executing systemctl, user space should then run shutdown, that it unaware about init manager, and for systemd based devices it is just a link to systemctl. The use case you want is supported with : - swupdate-progress is started without -r - swudpate is started with -e (exec script), and this can run systemctl or better shutdown. If this appears hidden. we could add a command line parm to configure the reboot command by distinguish between system call or external command. Best regards, Stefano > Signed-off-by: Daniel Braunwarth <oss@braunwarth.dev> > --- > tools/swupdate-ipc.c | 28 +++++++++++++++++++++++----- > tools/swupdate-progress.c | 15 ++++++++++++++- > 2 files changed, 37 insertions(+), 6 deletions(-) > > diff --git a/tools/swupdate-ipc.c b/tools/swupdate-ipc.c > index 0dfaeac..3b17d23 100644 > --- a/tools/swupdate-ipc.c > +++ b/tools/swupdate-ipc.c > @@ -535,6 +535,28 @@ static void restart_system(unsigned int ndevs) > } > } > > +static void reboot_device(void) > +{ > + int ret; > + > +#ifdef CONFIG_SYSTEMD > + ret = system("systemctl reboot"); > + if (ret == 0) { > + /* systemctl reboot is non-blocking and returns after the reboot > + is initiated */ > + while(true); > + } > +#else > + sleep(5); > + sync(); > + ret = reboot(RB_AUTOBOOT); > +#endif > + > + if (ret < 0) { /* Should never happen. */ > + fprintf(stdout, "Please reset the board.\n"); > + } > +} > + > static int sysrestart(cmd_t __attribute__((__unused__)) *cmd, int argc, char *argv[]) { > int connfd; > struct progress_msg msg; > @@ -651,11 +673,7 @@ static int sysrestart(cmd_t __attribute__((__unused__)) *cmd, int argc, char *a > case SUCCESS: > fprintf(stdout, "Ready to reboot !\n"); > restart_system(ndevs); > - sleep(5); > - sync(); > - if (reboot(RB_AUTOBOOT) < 0) { /* It should never happen */ > - fprintf(stdout, "Please reset the board.\n"); > - } > + reboot_device(); > break; > case FAILURE: > ndevs = 0; > diff --git a/tools/swupdate-progress.c b/tools/swupdate-progress.c > index 5e37162..6df3a29 100644 > --- a/tools/swupdate-progress.c > +++ b/tools/swupdate-progress.c > @@ -191,9 +191,22 @@ static void fill_progress_bar(char *bar, size_t size, unsigned int percent) > > static void reboot_device(void) > { > + int ret; > + > +#ifdef CONFIG_SYSTEMD > + ret = system("systemctl reboot"); > + if (ret == 0) { > + /* systemctl reboot is non-blocking and returns after the reboot > + is initiated */ > + while(true); > + } > +#else > sleep(5); > sync(); > - if (reboot(RB_AUTOBOOT) < 0) { /* Should never happen. */ > + ret = reboot(RB_AUTOBOOT); > +#endif > + > + if (ret < 0) { /* Should never happen. */ > fprintf(stdout, "Please reset the board.\n"); > } > }
Hi Stefano On 03.11.24 12:14, Stefano Babic wrote: > Hi Daniel, > > On 03.11.24 09:48, Daniel Braunwarth wrote: >> On systems which are using systemd we should not use the reboot() >> syscall to reboot the system. This is bypassing systemd. >> > > The reason for this is there are use cases where it is wanted to bypass > any user space tool and ask the kernel to reboot. Instead of executing > systemctl, user space should then run shutdown, that it unaware about > init manager, and for systemd based devices it is just a link to systemctl. I can hardly imagine a valid use case to bypass systemd here. Do you have any example? From my point of view it is a high risk bypassing systemd here. This will sooner or later cause problems. > > The use case you want is supported with : > > - swupdate-progress is started without -r > - swudpate is started with -e (exec script), and this can run systemctl > or better shutdown. What flag to you mean by "-e"? I'm only aware of the following: -e, --select <software>,<mode> : Select software images set and source How would I implement a use case were updates with and without the "no-reboot" can be applied to the target? Until now we are using a postupdate command (something like "systemd-timer ... systemctl reboot"), but this prevents the use case where no reboot is required. > > If this appears hidden. we could add a command line parm to configure > the reboot command by distinguish between system call or external command. That's something I could add additionally. Regards Daniel
Hi Daniel, On 03.11.24 13:15, Daniel Braunwarth wrote: > Hi Stefano > > On 03.11.24 12:14, Stefano Babic wrote: >> Hi Daniel, >> >> On 03.11.24 09:48, Daniel Braunwarth wrote: >>> On systems which are using systemd we should not use the reboot() >>> syscall to reboot the system. This is bypassing systemd. >>> >> >> The reason for this is there are use cases where it is wanted to bypass >> any user space tool and ask the kernel to reboot. Instead of executing >> systemctl, user space should then run shutdown, that it unaware about >> init manager, and for systemd based devices it is just a link to >> systemctl. > > I can hardly imagine a valid use case to bypass systemd here. Do you > have any example? > > From my point of view it is a high risk bypassing systemd here. This > will sooner or later cause problems. By many systems, a power cut can happens at any time and the way to restart the system is just a way to power cut, and a reboot via system call is the most close to it. Complex systems will need a shutdown and they are backed up in case of power cut. I see the opposite, if a device will require a systemd shutdown, it is not safe enough against power cut and could not boot again when it is powered again. In such cases, devices should have enough time to shut down before power is away (like servers / pcs). There are use cases, too, where a reboot is initiated by stopping to trigger the watchdog. This is wanted to increase tests and be sure that everything is fine if a power cut happens. > >> >> The use case you want is supported with : >> >> - swupdate-progress is started without -r >> - swudpate is started with -e (exec script), and this can run systemctl >> or better shutdown. > > What flag to you mean by "-e"? I'm only aware of the following: > -e, --select <software>,<mode> : Select software images set and source Usage ./tools/swupdate-progress [OPTION] -c, --color : Use colors to show results -e, --exec <script> : call the script with the result of update > > > How would I implement a use case were updates with and without the "no- > reboot" can be applied to the target? mmhhh...you're right. This case is not supported, it is not passed to the script. > > Until now we are using a postupdate command (something like "systemd- > timer ... systemctl reboot"), but this prevents the use case where no > reboot is required. Understood. In fact, to get this information, the progress interface must be implemented, like swupdate-progress is doing. But I will still maintain a way to bypass systemd for cases where this is forced. I would also avoid using CONFIG_ like CONFIG_SYSTEMD in tools, as the same tools are sometimes installed on devices with systemd (production software) and without (the recovery system). It cna be checked if systemctl is available instead. > >> >> If this appears hidden. we could add a command line parm to configure >> the reboot command by distinguish between system call or external >> command. > > That's something I could add additionally. Ok Regards, Stefano
diff --git a/tools/swupdate-ipc.c b/tools/swupdate-ipc.c index 0dfaeac..3b17d23 100644 --- a/tools/swupdate-ipc.c +++ b/tools/swupdate-ipc.c @@ -535,6 +535,28 @@ static void restart_system(unsigned int ndevs) } } +static void reboot_device(void) +{ + int ret; + +#ifdef CONFIG_SYSTEMD + ret = system("systemctl reboot"); + if (ret == 0) { + /* systemctl reboot is non-blocking and returns after the reboot + is initiated */ + while(true); + } +#else + sleep(5); + sync(); + ret = reboot(RB_AUTOBOOT); +#endif + + if (ret < 0) { /* Should never happen. */ + fprintf(stdout, "Please reset the board.\n"); + } +} + static int sysrestart(cmd_t __attribute__((__unused__)) *cmd, int argc, char *argv[]) { int connfd; struct progress_msg msg; @@ -651,11 +673,7 @@ static int sysrestart(cmd_t __attribute__((__unused__)) *cmd, int argc, char *a case SUCCESS: fprintf(stdout, "Ready to reboot !\n"); restart_system(ndevs); - sleep(5); - sync(); - if (reboot(RB_AUTOBOOT) < 0) { /* It should never happen */ - fprintf(stdout, "Please reset the board.\n"); - } + reboot_device(); break; case FAILURE: ndevs = 0; diff --git a/tools/swupdate-progress.c b/tools/swupdate-progress.c index 5e37162..6df3a29 100644 --- a/tools/swupdate-progress.c +++ b/tools/swupdate-progress.c @@ -191,9 +191,22 @@ static void fill_progress_bar(char *bar, size_t size, unsigned int percent) static void reboot_device(void) { + int ret; + +#ifdef CONFIG_SYSTEMD + ret = system("systemctl reboot"); + if (ret == 0) { + /* systemctl reboot is non-blocking and returns after the reboot + is initiated */ + while(true); + } +#else sleep(5); sync(); - if (reboot(RB_AUTOBOOT) < 0) { /* Should never happen. */ + ret = reboot(RB_AUTOBOOT); +#endif + + if (ret < 0) { /* Should never happen. */ fprintf(stdout, "Please reset the board.\n"); } }
On systems which are using systemd we should not use the reboot() syscall to reboot the system. This is bypassing systemd. Signed-off-by: Daniel Braunwarth <oss@braunwarth.dev> --- tools/swupdate-ipc.c | 28 +++++++++++++++++++++++----- tools/swupdate-progress.c | 15 ++++++++++++++- 2 files changed, 37 insertions(+), 6 deletions(-)