[PATCH] swupdate-progress: reboot the system via post update

[Stefan Herbrechtsmeier]

From: Stefan Herbrechtsmeier <stefan.herb...@weidmueller.com>

The update source runs the post update after a successful update. Use
the post update status message to trigger a reboot.

Signed-off-by: Stefan Herbrechtsmeier <stefan.herb...@weidmueller.com>
---
tools/swupdate-progress.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/swupdate-progress.c b/tools/swupdate-progress.c
index e3f010b..13d172b 100644
--- a/tools/swupdate-progress.c
+++ b/tools/swupdate-progress.c
@@ -376,16 +376,16 @@ int main(int argc, char **argv)
if (psplash_ok)
psplash_progress(psplash_pipe_path, &msg);
psplash_ok = 0;
- if ((msg.status == SUCCESS) && (msg.cur_step > 0) && opt_r) {
- sleep(5);
- if (system("reboot") < 0) { /* It should never happen */
- fprintf(stdout, "Please reset the board.\n");
- }
- }
wait_update = true;
break;
case DONE:
fprintf(stdout, "\nDONE.\n\n");
+
+ if (opt_r) {
+ if (system("reboot") < 0) { /* It should never happen */
+ fprintf(stdout, "Please reset the board.\n");
+ }
+ }
break;
default:
break;
--
2.30.0.windows.2

[Herbrechtsmeier Dr.-Ing. , Stefan]

Hi Stefano,

> -----Ursprüngliche Nachricht-----
> Von: Herbrechtsmeier, Stefan (OSS) <stefan.herbrechtsmeier-
> o...@weidmueller.com>
> Gesendet: Freitag, 21. Mai 2021 14:00
> An: swup...@googlegroups.com
> Cc: Herbrechtsmeier Dr.-Ing. , Stefan
> <Stefan.Herb...@weidmueller.com>
> Betreff: [PATCH] swupdate-progress: reboot the system via post update

What is the status of this patch?

Regards
Stefan

________________________________
Kommanditgesellschaft - Sitz: Detmold - Amtsgericht Lemgo HRA 2790 -
Komplementärin: Weidmüller Interface Führungsgesellschaft mbH -
Sitz: Detmold - Amtsgericht Lemgo HRB 3924;
Geschäftsführer: Dr. Timo Berger, Volker Bibelhausen, André Sombecki;
USt-ID-Nr. DE124599660

[Stefano Babic]

Sorry, I send review.

Regards,
Stefano

>
> Regards
> Stefan
>
> ________________________________
> Kommanditgesellschaft - Sitz: Detmold - Amtsgericht Lemgo HRA 2790 -
> Komplementärin: Weidmüller Interface Führungsgesellschaft mbH -
> Sitz: Detmold - Amtsgericht Lemgo HRB 3924;
> Geschäftsführer: Dr. Timo Berger, Volker Bibelhausen, André Sombecki;
> USt-ID-Nr. DE124599660
>

--
=====================================================================
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sba...@denx.de
=====================================================================

[Stefano Babic]

Hi Stefan,

This does not only move the code into the DONE case, but change behavior
for some not common use case. The following line:

if ((msg.status == SUCCESS) && (msg.cur_step > 0) && opt_r) {

this is for cases where a SWU contains multiple software for different
devices, and some devices have installed something, something not. Or
even, one of the device is already running the actual SW, and all
artifacts were set as "skipped". That means, the update was successful
but nothing was installed, resulting that the device should not reboot.

I will also avoid to drop the delay, just because I know about users who
won't set up in the application the progress interface (as it should
be), but they rely on the fact that reboot will happen some time later.
As the delay is not a performance hit for an updater, I won't remove it.

Best regards,
Stefano

> break;
> default:
> break;

[Herbrechtsmeier Dr.-Ing. , Stefan]

Hi Stefano,

> -----Ursprüngliche Nachricht-----
> Von: Stefano Babic <sba...@denx.de>
> Gesendet: Montag, 7. Juni 2021 12:46
> An: Herbrechtsmeier, Stefan (OSS) <stefan.herbrechtsmeier-
> o...@weidmueller.com>; swup...@googlegroups.com

> Cc: Herbrechtsmeier Dr.-Ing. , Stefan
> <Stefan.Herb...@weidmueller.com>

> Betreff: Re: [swupdate] [PATCH] swupdate-progress: reboot the system via
> post update
>

How we distinguish between a valid use case and a misuse? The current behavior isn't deterministic and the arbitrary sleep could lead to random update failures if you need more than 5 seconds sometimes.

> The following line:
>
> if ((msg.status == SUCCESS) && (msg.cur_step > 0) && opt_r) {
>
> this is for cases where a SWU contains multiple software for different
> devices, and some devices have installed something, something not. Or
> even, one of the device is already running the actual SW, and all artifacts
> were set as "skipped".

This sounds like a feature for swupdate core. This should be handled by the installer and it should skip the done message.

> That means, the update was successful but nothing
> was installed, resulting that the device should not reboot.

But shouldn't the post update command behavior in the same way?

> I will also avoid to drop the delay, just because I know about users who won't
> set up in the application the progress interface (as it should be), but they rely
> on the fact that reboot will happen some time later.

Isn't this a misuse? What happens if the "sometime later" need to be 6 seconds or more? Why don't they use the script or post update command to reset the system after they are done?

> As the delay is not a performance hit for an updater, I won't remove it.

But it is a indicator for a bad design and could lead to random failures. What happens if 5 seconds is only enough in 99% of the updates?

Regards

Stefan Herbrechtsmeier
Entwickler Software Embedded Systems

Let’s connect.

[Stefano Babic]

Hi Stefan,

It could be, yes - core could avoid to send the DONE.

>
>> That means, the update was successful but nothing
>> was installed, resulting that the device should not reboot.
>
> But shouldn't the post update command behavior in the same way?

It should be

>
>> I will also avoid to drop the delay, just because I know about users who won't
>> set up in the application the progress interface (as it should be), but they rely
>> on the fact that reboot will happen some time later.
>
> Isn't this a misuse?

It is, it is nasty, but is is like..history. We can drop the delay, but
I am expecting general complains because SWUpdate is not "working
anymore". And I am ask myself if it is worth to drop it or simply let it in.

> What happens if the "sometime later" need to be 6 seconds or more?

Well, a mistake should be done once...such changes will be rejected and
pointed to the correct way to to, that means with post update, or
listening to progress interface.

> Why don't they use the script or post update command to reset the system after they are done?

I cannot answer for them...but I am thinking about if it is worth to
create this issue just to drop the delay.

>
>> As the delay is not a performance hit for an updater, I won't remove it.
>
> But it is a indicator for a bad design and could lead to random failures. What happens if 5 seconds is only enough in 99% of the updates?

Well, the good design is, if someone needs such as housekeeping, to not
use swupdate-progress at all and the own application (or the process
needed some tiome before reboot) should implement the progress interface
and handles the messages. This makes the synchronization and it is safe.
How many users are doing this, I am unsure, I have at least some
customers of mine not doing this. I propose to drop the fix delay and
add a parameter ("-d <seconds>") for a smooth transition.

Regards,
Stefano

>
> Regards
>
> Stefan Herbrechtsmeier
> Entwickler Software Embedded Systems
>
> Let’s connect.
> ________________________________
> Kommanditgesellschaft - Sitz: Detmold - Amtsgericht Lemgo HRA 2790 -
> Komplementärin: Weidmüller Interface Führungsgesellschaft mbH -
> Sitz: Detmold - Amtsgericht Lemgo HRB 3924;
> Geschäftsführer: Dr. Timo Berger, Volker Bibelhausen, André Sombecki;
> USt-ID-Nr. DE124599660
>

[Stefano Babic]

Hi Stefano,

> -----Ursprüngliche Nachricht-----
> Von: Stefano Babic <sba...@denx.de>

> Gesendet: Montag, 7. Juni 2021 18:37
> An: Herbrechtsmeier Dr.-Ing. , Stefan
> <Stefan.Herb...@weidmueller.com>; Stefano Babic
> <sba...@denx.de>; Herbrechtsmeier, Stefan (OSS)
> <stefan.herbre...@weidmueller.com>;
> swup...@googlegroups.com
> Betreff: Re: AW: [swupdate] [PATCH] swupdate-progress: reboot the system

> >>> + never

> >> happen */
> >>> + fprintf(stdout, "Please reset

> >>> + the

> >> board.\n");
> >>> + }
> >>> + }
> >>
> >> This does not only move the code into the DONE case, but change
> >> behavior for some not common use case.
> >
> > How we distinguish between a valid use case and a misuse? The current
> behavior isn't deterministic and the arbitrary sleep could lead to random
> update failures if you need more than 5 seconds sometimes.
> >
> >> The following line:
> >>
> >> if ((msg.status == SUCCESS) && (msg.cur_step > 0) && opt_r) {
> >>
> >> this is for cases where a SWU contains multiple software for
> >> different devices, and some devices have installed something,
> >> something not. Or even, one of the device is already running the
> >> actual SW, and all artifacts were set as "skipped".
> >
> > This sounds like a feature for swupdate core. This should be handled by the
> installer and it should skip the done message.
>
> It could be, yes - core could avoid to send the DONE.

This leads to the question what is the intention / meaning of the DONE
message. The name suggest that it is the last step in the update
process. The current implementation bundle it with the post update
command and optimal skip this message.

> >> That means, the update was successful but nothing was installed,
> >> resulting that the device should not reboot.
> >
> > But shouldn't the post update command behavior in the same way?
>
> It should be
>

[snip]

Let's skip the delay discussion for now.

[Stefano Babic]

Hi Stefan,

Right, I see your point now. There was some time ago a discussion on ML
about the topic, this led to commit:

commit 909e6edf53a50d970fa698b5aa65084054ab5743
Author: Stefano Babic <sba...@denx.de>
Date: Tue Nov 10 12:25:34 2020 +0100

Reset progress value in swupdate_progress_end()

(I reread previous thread because I forgot it..)

The "DONE" is currently sent only by postupdate. But postupdate is
optional, and it can also be sent via IPC by an external command. In
many cases, there is no post-update at all - it depends on
configuration. But the internal state machine cannot rely on it, and it
must emit if an update (that is, an "install") successful was or not. So
it is guaranteed that SWUpdate always emits a "SUCCESS" or "FAILURE"
message, but it is not guaranteed that DONE is sent. The postupdate is
thought as command to be sent after a successful update: so the update
was already successful. It is also questionable if DONE should be
emitted, but it could be in some setup where postupdate is called, but
there is not reboot and it is doing some "housekeeping" (but this could
be maybe integrated as script into sw-description, and then SUCCESS will
also mean that housekeeping was done), and I guess this is quite your
use case.

On the other side and in my setup, if I need something more complex
after an update, I register with the progress interface and I wil ldo
everything in a separate process (like an "extended" swupdate-progress).

I can just say that in my projects, I always check for SUCCESS/FAILURE,
while DONE is ignored (as you see in swupdate-progress), and postupdate
is used by me as alternative to swupdate-progress (and then it reboots
automatically).

>
>> >> That means, the update was successful but nothing was installed,
>> >> resulting that the device should not reboot.
>> >
>> > But shouldn't the post update command behavior in the same way?
>>
>> It should be
>>
>
> [snip]
>
> Let's skip the delay discussion for now.
>
> Regards
>

Best regards,
Stefano

[Herbrechtsmeier Dr.-Ing. , Stefan]

Hi Stefano,


> -----Ursprüngliche Nachricht-----
> Von: Stefano Babic <sba...@denx.de>

> Gesendet: Mittwoch, 9. Juni 2021 16:17
> An: 'swup...@googlegroups.com' <swup...@googlegroups.com>

> Cc: Herbrechtsmeier Dr.-Ing. , Stefan
> <Stefan.Herb...@weidmueller.com>

> Betreff: Re: Fwd: AW: AW: [swupdate] [PATCH] swupdate-progress: reboot

> >> >>> +should never

What does this means for the reboot button in the webserver? Should it requires a reboot as postupdate command and shouldn't be used together with the swupdate-progress reboot option?

[Stefano Babic]

Yes !

> and shouldn't be used together with the swupdate-progress reboot option?

Right !

And I set in swupdate.cfg:

webserver :
{
....
run-postupdate = false;
};

To disable an automatic update.

Best regards,
Stefano

>
> Regards
>
> Stefan Herbrechtsmeier
> Entwickler Software Embedded Systems
>
> Let’s connect.
> ________________________________
> Kommanditgesellschaft - Sitz: Detmold - Amtsgericht Lemgo HRA 2790 -
> Komplementärin: Weidmüller Interface Führungsgesellschaft mbH -
> Sitz: Detmold - Amtsgericht Lemgo HRB 3924;
> Geschäftsführer: Dr. Timo Berger, Volker Bibelhausen, André Sombecki;
> USt-ID-Nr. DE124599660
>

[Herbrechtsmeier Dr.-Ing. , Stefan]

Hi Stefano,

> -----Ursprüngliche Nachricht-----
> Von: Stefano Babic <sba...@denx.de>

> Gesendet: Mittwoch, 9. Juni 2021 16:49

> An: Herbrechtsmeier Dr.-Ing. , Stefan
> <Stefan.Herb...@weidmueller.com>; Stefano Babic
> <sba...@denx.de>

> Cc: 'swup...@googlegroups.com' <swup...@googlegroups.com>
> Betreff: Re: AW: Fwd: AW: AW: [swupdate] [PATCH] swupdate-progress:

> >>>>>>> +reset the

Okay, I will follow your solution and you can reject this patch.

Maybe you should mark DONE as deprecated or remove it to avoid confusion.

[Stefano Babic]

Hi Stefan,

Ok

> Maybe you should mark DONE as deprecated or remove it to avoid confusion.
>

Fully agree - I was myself confused...

Regards,
Stefano

> Regards
>
> Stefan Herbrechtsmeier
> Entwickler Software Embedded Systems
>
> Let’s connect.
>
> ________________________________
> Kommanditgesellschaft - Sitz: Detmold - Amtsgericht Lemgo HRA 2790 -
> Komplementärin: Weidmüller Interface Führungsgesellschaft mbH -
> Sitz: Detmold - Amtsgericht Lemgo HRB 3924;
> Geschäftsführer: Dr. Timo Berger, Volker Bibelhausen, André Sombecki;
> USt-ID-Nr. DE124599660
>