diff mbox series

Set ustate when an update is started

Message ID 20220406100448.2711964-1-sbabic@denx.de
State Accepted
Headers show
Series Set ustate when an update is started | expand

Commit Message

Stefano Babic April 6, 2022, 10:04 a.m. UTC 
ustate is just set after an update with the result, but it was foreseen
to set it to IN_PROGRESS, too, when an update runs. This information is
really srored in the marker (recovery_status bootloader variable).
However, the two variables are not set in an atomic way because they are
used in different use cases (recovery_status in single-copy strategy).

Align ustate to recovery_status, and set it when an update is started. A
power-cut during an update can be detected by examining the value of the
variable and this can be useful in some use cases (reporting a failed
update due to a power-cut).

Factorize code to set the variables in a separate function to make the
code more readabla.

Signed-off-by: Stefano Babic <sbabic@denx.de>
---
 core/stream_interface.c | 37 ++++++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 17 deletions(-)

Comments

Fabio Estevam April 6, 2022, 9:55 p.m. UTC | #1 
Hi Stefano,

Some minor nits:

On Wed, Apr 6, 2022 at 7:04 AM Stefano Babic <sbabic@denx.de> wrote:
>
> ustate is just set after an update with the result, but it was foreseen
> to set it to IN_PROGRESS, too, when an update runs. This information is
> really srored in the marker (recovery_status bootloader variable).

s/srored/stored

> However, the two variables are not set in an atomic way because they are
> used in different use cases (recovery_status in single-copy strategy).
>
> Align ustate to recovery_status, and set it when an update is started. A
> power-cut during an update can be detected by examining the value of the
> variable and this can be useful in some use cases (reporting a failed
> update due to a power-cut).
>
> Factorize code to set the variables in a separate function to make the
> code more readabla.

s/readabla/readable
Stefano Babic April 7, 2022, 5:53 a.m. UTC | #2 
Hi Fabio,

On 06.04.22 23:55, Fabio Estevam wrote:
> Hi Stefano,
> 
> Some minor nits:
> 
> On Wed, Apr 6, 2022 at 7:04 AM Stefano Babic <sbabic@denx.de> wrote:
>>
>> ustate is just set after an update with the result, but it was foreseen
>> to set it to IN_PROGRESS, too, when an update runs. This information is
>> really srored in the marker (recovery_status bootloader variable).
> 
> s/srored/stored
> 
>> However, the two variables are not set in an atomic way because they are
>> used in different use cases (recovery_status in single-copy strategy).
>>
>> Align ustate to recovery_status, and set it when an update is started. A
>> power-cut during an update can be detected by examining the value of the
>> variable and this can be useful in some use cases (reporting a failed
>> update due to a power-cut).
>>
>> Factorize code to set the variables in a separate function to make the
>> code more readabla.
> 
> s/readabla/readable

Thanks, I send V2.

Regards,
Stefano

>
diff mbox series

Patch

diff --git a/core/stream_interface.c b/core/stream_interface.c
index 1c9853d..57a3192 100644
--- a/core/stream_interface.c
+++ b/core/stream_interface.c
@@ -495,6 +495,23 @@  no_copy_output:
 	return ret;
 }
 
+static bool update_transaction_state(struct swupdate_cfg *software, update_state_t newstate)
+{
+	if (!software->parms.dry_run && software->bootloader_transaction_marker) {
+		if (newstate == STATE_INSTALLED)
+			bootloader_env_unset(BOOTVAR_TRANSACTION);
+		else
+			bootloader_env_set(BOOTVAR_TRANSACTION, get_state_string(newstate));
+	}
+	if (!software->parms.dry_run
+	    && software->bootloader_state_marker
+	    && save_state(newstate) != SERVER_OK) {
+		WARN("Cannot persistently store %s update state.", get_state_string(newstate));
+		return false;
+	}
+	return true;
+}
+
 void *network_initializer(void *data)
 {
 	int ret;
@@ -605,34 +622,20 @@  void *network_initializer(void *data)
 			 * must be successful. Set we have
 			 * initiated an update
 			 */
-			if (!software->parms.dry_run && software->bootloader_transaction_marker) {
-				bootloader_env_set(BOOTVAR_TRANSACTION, get_state_string(STATE_IN_PROGRESS));
-			}
+			update_transaction_state(software, STATE_IN_PROGRESS);
 
 			notify(RUN, RECOVERY_NO_ERROR, INFOLEVEL, "Installation in progress");
 			ret = install_images(software);
 			if (ret != 0) {
-				if (!software->parms.dry_run && software->bootloader_transaction_marker) {
-					bootloader_env_set(BOOTVAR_TRANSACTION, get_state_string(STATE_FAILED));
-				}
+				update_transaction_state(software, STATE_FAILED);
 				notify(FAILURE, RECOVERY_ERROR, ERRORLEVEL, "Installation failed !");
 				inst.last_install = FAILURE;
-				if (!software->parms.dry_run
-				    && software->bootloader_state_marker
-				    && save_state(STATE_FAILED) != SERVER_OK) {
-					WARN("Cannot persistently store FAILED update state.");
-				}
 			} else {
 				/*
 				 * Clear the recovery variable to indicate to bootloader
 				 * that it is not required to start recovery again
 				 */
-				if (!software->parms.dry_run && software->bootloader_transaction_marker) {
-					bootloader_env_unset(BOOTVAR_TRANSACTION);
-				}
-				if (!software->parms.dry_run
-				    && software->bootloader_state_marker
-				    && save_state(STATE_INSTALLED) != SERVER_OK) {
+				if (!update_transaction_state(software, STATE_INSTALLED)) {
 					ERROR("Cannot persistently store INSTALLED update state.");
 					notify(FAILURE, RECOVERY_ERROR, ERRORLEVEL, "Installation failed !");
 					inst.last_install = FAILURE;