Different behavior between "swupdate-progress -r" and "swupdate -p reboot"

[Stefan Herbrechtsmeier]

Hi Stefano,

why does the swupdate-progress restarts the system after SUCCESS and not
after DONE?

> case SUCCESS:
> case FAILURE:

[snip]

> 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");
> break;

The function postupdate in the source file installer.c sends a DONE
before the post update command:

>
> int postupdate(struct swupdate_cfg *swcfg, const char *info)
> {
> swupdate_progress_done(info);
>
> if (swcfg) {
> if (swcfg->parms.dry_run) {
> DEBUG("Dry run, skipping Post-update command");
> } else {
> DEBUG("Running Post-update command");
> return run_system_cmd(swcfg->postupdatecmd);
> }
>
> }
>
> return 0;
> }

This leads to a different behavior between "swupdate-progress -r" and
"swupdate -p reboot" if somebody like the web-app triggers a reboot via
ipc_postupdate.

Regards
Stefan

[Stefano Babic]

Hi Stefan,

On 29.04.21 20:14, Stefan Herbrechtsmeier wrote:
> Hi Stefano,
>
> why does the swupdate-progress restarts the system after SUCCESS and not
> after DONE?
>
>>         case SUCCESS:
>>         case FAILURE:
>
> [snip]
>
>>             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");
>>             break;
>
> The function postupdate in the source file installer.c sends a DONE
> before the post update command

I just think it was not checked in code that the behavior is exactly the
same, and there is no special reason to do in different way. I looked in
code all occurrences of postupdate. The switch to perform an action
remains the result of an update should check SUCCESS or FAILURE. For the
Webserver:

226 if (msg.status == SUCCESS && msg.source ==
SOURCE_WEBSERVER && run_postupdate) {
227 ipc_message ipc = {};
228
229 ipc_postupdate(&ipc);
230 }

So at least swupdate-progress and Webserver seem in sync (or do you mean
they are not ?).

Another actor is swupdate-client, but it looks to me consistent, too:

106
107 if (status == SUCCESS && run_postupdate) {
108 fprintf(stdout, "Executing post-update actions.\n");
109 ipc_message msg;
110 if (ipc_postupdate(&msg) != 0)
111 fprintf(stderr, "Running post-update
failed!\n");
112 }

So action when SUCCESS or FAILURE is received.

In core/install_from_file.c, there is another use case:


50 static int endupdate(RECOVERY_STATUS status)
51 {
52 end_status = (status == SUCCESS) ? EXIT_SUCCESS :
EXIT_FAILURE;
53
54 INFO("Swupdate %s\n",
55 status == FAILURE ? "*failed* !" :
56 "was successful !");
57
58 if (status == SUCCESS) {
59 ipc_message msg;
60 msg.data.procmsg.len = 0;
61 if (ipc_postupdate(&msg) != 0 || msg.type != ACK) {

All of them are waiting for SUCCESS or FAILURE. IMHO this is the value
to be parsed, even if the state machine goes further and it will reach
the DONE and then back to IDLE.

Nevertheless, if there is some discrepancy, it is now mitigated by the
delay before executing the command, and drawbacks are maybe hidden. Do
you have seen a concrete problem ? Can you show it ?

>
>>
>> int postupdate(struct swupdate_cfg *swcfg, const char *info)
>> {
>>     swupdate_progress_done(info);
>>
>>     if (swcfg) {
>>         if (swcfg->parms.dry_run) {
>>             DEBUG("Dry run, skipping Post-update command");
>>         } else {
>>             DEBUG("Running Post-update command");
>>             return run_system_cmd(swcfg->postupdatecmd);
>>         }
>>
>>     }
>>
>>     return 0;
>> }
>
> This leads to a different behavior between "swupdate-progress -r" and
> "swupdate -p reboot" if somebody like the web-app triggers a reboot via
> ipc_postupdate.

I am not sure, it looks to me that Webserver is looking for SUCCESS as well.

Best regards,
Stefano

--
=====================================================================
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
=====================================================================

[Stefan Herbrechtsmeier]

Hi Stefano,

Am 30.04.21 um 10:25 schrieb Stefano Babic:

The problem is in swupdate-progress. This checks for SUCCESS and not for
DONE. This means it isn't triggered by the function ipc_postupdate but
by a SUCCESS msg. The web-app use the ipc_postupdate to trigger a reboot
via UI.

> 226                 if (msg.status == SUCCESS && msg.source ==
> SOURCE_WEBSERVER && run_postupdate) {
>  227                         ipc_message ipc = {};
>  228
>  229                         ipc_postupdate(&ipc);
>  230                 }
>
> So at least swupdate-progress and Webserver seem in sync (or do you mean
> they are not ?).

The problem isn't the call of ipc_postupdate but how swupdate-progress
trigger a reboot. It should use the DONE which is triggered by the
ipc_postupdate.

[snip]

> Nevertheless, if there is some discrepancy, it is now mitigated by the
> delay before executing the command, and drawbacks are maybe hidden. Do
> you have seen a concrete problem ? Can you show it ?

You have to start swupdate with web server support and trigger a reboot
via UI. You could reboot the system only if you set postupdate to
reboot. A reboot via swupdate-progress doesn't work and you only see a
"DONE." message.

Regards
Stefan

[Stefano Babic]

Hi Stefan,

Ok - that is important because the behavior inside SWUpdate should be
consistent. swupdate-progress is an external tool, and also thought as
example for users how to integrate and communicate with SWUpdate in the
own application. At least, at the early beginning, but most users are
running swupdate-progress as it is.

> This checks for SUCCESS and not for
> DONE. This means it isn't triggered by the function ipc_postupdate but
> by a SUCCESS msg. The web-app use the ipc_postupdate to trigger a reboot
> via UI.

But as swupdate-progress is thought as independent application, it
decides itself how to reboot - that the reason of the "-r" switch. It
does not ask SWUpdate for it, it decides itself to reboot the board.

>
>
>> 226                 if (msg.status == SUCCESS && msg.source ==
>> SOURCE_WEBSERVER && run_postupdate) {
>>   227                         ipc_message ipc = {};
>>   228
>>   229                         ipc_postupdate(&ipc);
>>   230                 }
>>
>> So at least swupdate-progress and Webserver seem in sync (or do you
>> mean they are not ?).
>
> The problem isn't the call of ipc_postupdate but how swupdate-progress
> trigger a reboot. It should use the DONE which is triggered by the
> ipc_postupdate.
>

It can be moved, but I am not yet able to

> [snip]
>
>
>> Nevertheless, if there is some discrepancy, it is now mitigated by the
>> delay before executing the command, and drawbacks are maybe hidden. Do
>> you have seen a concrete problem ? Can you show it ?
>
> You have to start swupdate with web server support

Ok

> and trigger a reboot
> via UI. You could reboot the system only if you set postupdate to
> reboot.

Right

> A reboot via swupdate-progress doesn't work and you only see a
> "DONE." message.

mmhhh...

swupdate-progress -r reboots unconditionally the board. I guess there
are different use cases and some of them (yours !) do not match with the
implementation. My assumption was:

- customer / operator does not want that system automatically reboots.
The button on the Web is active and the postupdate command (as
/sbin/reboot) is set. When the operator after an update decides that it
can reboot the device, it works. swupdate-progress is not used or "-r"
is disabled.

- customer / operator wants that system automatically reboots after a
successfull update: swupdate-progress is started with "-r", it does not
matter if postupdate is set.

Your use case seems nearby but not exactly the same. Any way, I do not
see issue if we move the reboot in swupdate-progress when DONE is
received as you say.

[Stefan Herbrechtsmeier]

Hi Stefano,

Am 30.04.21 um 16:21 schrieb Stefano Babic:

How should this work? The postupdate command is called after a SUCCESS
during the last step (DONE). You have to leave the postupdate empty if
you don't want a reboot after an update. In this case the reboot on the
UI doesn't work and need to be removed.

> - customer / operator wants that system automatically reboots after a
> successfull update: swupdate-progress is started with "-r", it does not
> matter if postupdate is set.

Really? I would say it reboots during a successful update because it
reboots after a SUCCESS message and not after the DONE message.

> Your use case seems nearby but not exactly the same. Any way, I do not
> see issue if we move the reboot in swupdate-progress when DONE is
> received as you say.

If you need a reboot in the UI but no reboot after an update you need to
call reboot direct in the web server, rephrase the UI and change the
service file of swupdate-progress.

Regards
Stefan

[Stefano Babic]

Hi Stefan,

You are surely right, I have to convince myself because I am sure this
worked before. I will set a test project to do this and I will be back
to this issue.