diff mbox series

bootloader: make transaction marker configurable

Message ID 20190326104555.4710-1-christian.storm@siemens.com
State Changes Requested
Headers show
Series bootloader: make transaction marker configurable | expand

Commit Message

Storm, Christian March 26, 2019, 10:45 a.m. UTC
Currently, SWUpdate sets the bootloader environment
variable "recovery_status" to "in_progress" prior to
an operation and either unsets or sets it to "failed"
after the operation so that SWUpdate-external tools
may use this information to take according measures.

Make setting this bootloader environment variable
configurable, defaulting to "yes".

Signed-off-by: Christian Storm <christian.storm@siemens.com>
---
 Kconfig                    | 11 +++++++++++
 core/swupdate.c            |  6 ++++++
 corelib/stream_interface.c |  8 ++++++++
 3 files changed, 25 insertions(+)

Comments

Stefano Babic March 26, 2019, 11:50 a.m. UTC | #1
Hi Christian,

On 26/03/19 11:45, Christian Storm wrote:
> Currently, SWUpdate sets the bootloader environment
> variable "recovery_status" to "in_progress" prior to
> an operation and either unsets or sets it to "failed"
> after the operation so that SWUpdate-external tools
> may use this information to take according measures.
> 
> Make setting this bootloader environment variable
> configurable, defaulting to "yes".

In fact, I have already thought that this mechanismus has something
missing.  IMHO the information that a new software was installed and it
is available is missing. This is covered by the "ustate" variable, but
just in case suricatta is running. I had added to set recovery_status to
"installed" after a successful update, but this breaks a lot of projects
where the u-boot script just check for the existence of the variable. So
I guess the best way is to stick with the couple recovery_status +
ustate for compatibility reason.

Apart of it, which are the benefits of this patch ? Even if
bootloader_env_set() is not successful, the update is not blocked (it is
wanted that error is ignored). A transaction marker belongs to the
features in SWUpdate, it should be always be done.

In a dual-copy approach, this variable can be ignored in the boot
scripts and I do not see any problems if it is set. In a single-copy, it
must always be checked to avoid to run a broken system.

Best regards,
Stefano

> 
> Signed-off-by: Christian Storm <christian.storm@siemens.com>
> ---
>  Kconfig                    | 11 +++++++++++
>  core/swupdate.c            |  6 ++++++
>  corelib/stream_interface.c |  8 ++++++++
>  3 files changed, 25 insertions(+)
> 
> diff --git a/Kconfig b/Kconfig
> index 302166e..c1404c5 100644
> --- a/Kconfig
> +++ b/Kconfig
> @@ -102,6 +102,17 @@ config SCRIPTS
>  	  in the image. For security reason, this option
>  	  can be switched off.
>  
> +config BOOTLOADER_TRANSACTION_MARKER
> +	bool "enable bootloader transaction marker"
> +	default y
> +	help
> +	  Enabling this option sets the bootloader environment
> +	  variable "recovery_status" to "in_progress" prior to
> +	  an operation and either unsets or sets it to "failed"
> +	  after the operation.
> +	  SWUpdate-external tools (e.g., bootloader scripts)
> +	  may use this information to take according measures.
> +
>  config HW_COMPATIBILITY
>  	bool "check for hardware / software compatibility"
>  	default n
> diff --git a/core/swupdate.c b/core/swupdate.c
> index 476358b..e9a211a 100644
> --- a/core/swupdate.c
> +++ b/core/swupdate.c
> @@ -369,7 +369,9 @@ static int install_from_file(char *fname, int check)
>  	/*
>  	 * Set "recovery_status" as begin of the transaction"
>  	 */
> +#ifdef CONFIG_BOOTLOADER_TRANSACTION_MARKER
>  	bootloader_env_set(BOOTVAR_TRANSACTION, "in_progress");
> +#endif
>  
>  	ret = install_images(&swcfg, fdsw, 1);
>  
> @@ -382,7 +384,9 @@ static int install_from_file(char *fname, int check)
>  		return EXIT_FAILURE;
>  	}
>  
> +#ifdef CONFIG_BOOTLOADER_TRANSACTION_MARKER
>  	bootloader_env_unset(BOOTVAR_TRANSACTION);
> +#endif
>  	fprintf(stdout, "Software updated successfully\n");
>  	fprintf(stdout, "Please reboot the device to start the new software\n");
>  
> @@ -969,7 +973,9 @@ int main(int argc, char **argv)
>  		result = install_from_file(fname, opt_c);
>  		switch (result) {
>  		case EXIT_FAILURE:
> +#ifdef CONFIG_BOOTLOADER_TRANSACTION_MARKER
>  			bootloader_env_set(BOOTVAR_TRANSACTION, "failed");
> +#endif
>  			break;
>  		case EXIT_SUCCESS:
>  			notify(SUCCESS, 0, INFOLEVEL, NULL);
> diff --git a/corelib/stream_interface.c b/corelib/stream_interface.c
> index 1261958..d7774bb 100644
> --- a/corelib/stream_interface.c
> +++ b/corelib/stream_interface.c
> @@ -237,7 +237,9 @@ static int extract_files(int fd, struct swupdate_cfg *software)
>  				 * just once
>  				 */
>  				if (!installed_directly) {
> +#ifdef CONFIG_BOOTLOADER_TRANSACTION_MARKER
>  					bootloader_env_set(BOOTVAR_TRANSACTION, "in_progress");
> +#endif
>  					installed_directly = true;
>  				}
>  
> @@ -393,12 +395,16 @@ void *network_initializer(void *data)
>  			 * must be successful. Set we have
>  			 * initiated an update
>  			 */
> +#ifdef CONFIG_BOOTLOADER_TRANSACTION_MARKER
>  			bootloader_env_set(BOOTVAR_TRANSACTION, "in_progress");
> +#endif
>  
>  			notify(RUN, RECOVERY_NO_ERROR, INFOLEVEL, "Installation in progress");
>  			ret = install_images(software, 0, 0);
>  			if (ret != 0) {
> +#ifdef CONFIG_BOOTLOADER_TRANSACTION_MARKER
>  				bootloader_env_set(BOOTVAR_TRANSACTION, "failed");
> +#endif
>  				notify(FAILURE, RECOVERY_ERROR, ERRORLEVEL, "Installation failed !");
>  				inst.last_install = FAILURE;
>  
> @@ -407,7 +413,9 @@ void *network_initializer(void *data)
>  				 * Clear the recovery variable to indicate to bootloader
>  				 * that it is not required to start recovery again
>  				 */
> +#ifdef CONFIG_BOOTLOADER_TRANSACTION_MARKER
>  				bootloader_env_unset(BOOTVAR_TRANSACTION);
> +#endif
>  				notify(SUCCESS, RECOVERY_NO_ERROR, INFOLEVEL, "SWUPDATE successful !");
>  				inst.last_install = SUCCESS;
>  			}
>
Storm, Christian March 26, 2019, 12:42 p.m. UTC | #2
Hi Stefano,

> > Currently, SWUpdate sets the bootloader environment
> > variable "recovery_status" to "in_progress" prior to
> > an operation and either unsets or sets it to "failed"
> > after the operation so that SWUpdate-external tools
> > may use this information to take according measures.
> > 
> > Make setting this bootloader environment variable
> > configurable, defaulting to "yes".
> 
> In fact, I have already thought that this mechanismus has something
> missing.  IMHO the information that a new software was installed and it
> is available is missing. This is covered by the "ustate" variable, but
> just in case suricatta is running. I had added to set recovery_status to
> "installed" after a successful update, but this breaks a lot of projects
> where the u-boot script just check for the existence of the variable. So
> I guess the best way is to stick with the couple recovery_status +
> ustate for compatibility reason.

Yes, I do agree here. This however doesn't rule out that we deprecate it
and come up with a new bootloader environment variable that combines
these two meanings in a uniform way into one variable that's being used
for transaction marking purposes as well as for status transfer to
external programs (or for whatever future use case may emerge).
Modeling it after include/state.h's update_state_t extended with
transaction marking entries would make a good start I think.
Then, we need to have a compat implementation that maps this new to the
current bootloader environment variables for some time...


> Apart of it, which are the benefits of this patch ? Even if
> bootloader_env_set() is not successful, the update is not blocked (it is
> wanted that error is ignored). A transaction marker belongs to the
> features in SWUpdate, it should be always be done.

Again, agreed, except for when it shouldn't be done :)
Considering how versatile SWUpdate has become, one can use it as the
only channel to deliver software (in the most general meaning) down to
the device, e.g., container images, packages then to be updated via
a postinstall callout to the package manager, ....
These use cases needn't be guarded by a transaction marker, I think.
More so, writing the bootloader environment just for a package upgrade
seems to be a bit too much...
That said, if having one SWUpdate running for all of these and more
types of update artifacts, then one may like to control per artifact
whether a marker should be set or not. This could, e.g., be done by
extending sw-description with a respective attribute that defaults to
"true" and can be set to "false" for update types sketched above.


> In a dual-copy approach, this variable can be ignored in the boot
> scripts and I do not see any problems if it is set. In a single-copy, it
> must always be checked to avoid to run a broken system.

Yes, that's true. For an A/B system using suricatta I would disable
BOOTLOADER_TRANSACTION_MARKER for specific deployments where SWUpdate
handles not only firmware whereas for a single-copy system it should be
enabled. That's why I left it defaulting to "yes", and to not break
existing deployments, of course.



Kind regards,
   Christian
Stefano Babic March 26, 2019, 1:41 p.m. UTC | #3
Hi Christian,

On 26/03/19 13:42, Christian Storm wrote:
> Hi Stefano,
> 
>>> Currently, SWUpdate sets the bootloader environment
>>> variable "recovery_status" to "in_progress" prior to
>>> an operation and either unsets or sets it to "failed"
>>> after the operation so that SWUpdate-external tools
>>> may use this information to take according measures.
>>>
>>> Make setting this bootloader environment variable
>>> configurable, defaulting to "yes".
>>
>> In fact, I have already thought that this mechanismus has something
>> missing.  IMHO the information that a new software was installed and it
>> is available is missing. This is covered by the "ustate" variable, but
>> just in case suricatta is running. I had added to set recovery_status to
>> "installed" after a successful update, but this breaks a lot of projects
>> where the u-boot script just check for the existence of the variable. So
>> I guess the best way is to stick with the couple recovery_status +
>> ustate for compatibility reason.
> 
> Yes, I do agree here. This however doesn't rule out that we deprecate it
> and come up with a new bootloader environment variable that combines
> these two meanings in a uniform way into one variable that's being used
> for transaction marking purposes as well as for status transfer to
> external programs (or for whatever future use case may emerge).

Right, I agree with you. The compatibility can be also realized in the
way the new SWU containing an updated SWUpdate changes the bootscript to
avoid to check "recovery_status". This is possible, it is difficult to
spread.


> Modeling it after include/state.h's update_state_t extended with
> transaction marking entries would make a good start I think.
> Then, we need to have a compat implementation that maps this new to the
> current bootloader environment variables for some time...

Right, and maybe for a longer time, if the bootscripts are not updated
(well, this is a SWUpdate's feature). I guess this could be an issue for
some projects working with just the default environment without storing
the environment somewhere.

> 
> 
>> Apart of it, which are the benefits of this patch ? Even if
>> bootloader_env_set() is not successful, the update is not blocked (it is
>> wanted that error is ignored). A transaction marker belongs to the
>> features in SWUpdate, it should be always be done.
> 
> Again, agreed, except for when it shouldn't be done :)
> Considering how versatile SWUpdate has become, one can use it as the
> only channel to deliver software (in the most general meaning) down to
> the device, e.g., container images, packages then to be updated via
> a postinstall callout to the package manager, ....

How can SWUpdate know about the different use cases ? If you upgrade the
OS, you want to set the marker. But what about a failed update of a
single package, followed by a power-cut ?

> These use cases needn't be guarded by a transaction marker, I think.
> More so, writing the bootloader environment just for a package upgrade
> seems to be a bit too much...

Another way could be to make the variable name configurable, that is
SWUpdate writes different variables.

The proposed patch selects this at build time - this looks to me a high
restriction. I would see this patch as run-time condition (with a new
command line parameter, maybe).

> That said, if having one SWUpdate running for all of these and more
> types of update artifacts, then one may like to control per artifact
> whether a marker should be set or not. This could, e.g., be done by
> extending sw-description with a respective attribute that defaults to
> "true" and can be set to "false" for update types sketched above.

Generally, we have several artifacts in a single SWU. The marker is
global to ensure atomicity. The marker is set before installing
anything. A higher granularity is against the atomicity.

> 
> 
>> In a dual-copy approach, this variable can be ignored in the boot
>> scripts and I do not see any problems if it is set. In a single-copy, it
>> must always be checked to avoid to run a broken system.
> 
> Yes, that's true. For an A/B system using suricatta I would disable
> BOOTLOADER_TRANSACTION_MARKER for specific deployments where SWUpdate
> handles not only firmware whereas for a single-copy system it should be
> enabled.

Right, this is the general use case.

> That's why I left it defaulting to "yes", and to not break
> existing deployments, of course.

I would prefer to rule this with a command line parameter instead of at
build time.

Best regards,
Stefano
Storm, Christian March 26, 2019, 4:32 p.m. UTC | #4
Hi Stefano,

> >>> Currently, SWUpdate sets the bootloader environment
> >>> variable "recovery_status" to "in_progress" prior to
> >>> an operation and either unsets or sets it to "failed"
> >>> after the operation so that SWUpdate-external tools
> >>> may use this information to take according measures.
> >>>
> >>> Make setting this bootloader environment variable
> >>> configurable, defaulting to "yes".
> >>
> >> In fact, I have already thought that this mechanismus has something
> >> missing.  IMHO the information that a new software was installed and it
> >> is available is missing. This is covered by the "ustate" variable, but
> >> just in case suricatta is running. I had added to set recovery_status to
> >> "installed" after a successful update, but this breaks a lot of projects
> >> where the u-boot script just check for the existence of the variable. So
> >> I guess the best way is to stick with the couple recovery_status +
> >> ustate for compatibility reason.
> > 
> > Yes, I do agree here. This however doesn't rule out that we deprecate it
> > and come up with a new bootloader environment variable that combines
> > these two meanings in a uniform way into one variable that's being used
> > for transaction marking purposes as well as for status transfer to
> > external programs (or for whatever future use case may emerge).
> 
> Right, I agree with you. The compatibility can be also realized in the
> way the new SWU containing an updated SWUpdate changes the bootscript to
> avoid to check "recovery_status". This is possible, it is difficult to
> spread.

True. This would be something worth to be implemented in the example
Yocto layers to show it off to people.

> > Modeling it after include/state.h's update_state_t extended with
> > transaction marking entries would make a good start I think.
> > Then, we need to have a compat implementation that maps this new to the
> > current bootloader environment variables for some time...
> 
> Right, and maybe for a longer time, if the bootscripts are not updated
> (well, this is a SWUpdate's feature). I guess this could be an issue for
> some projects working with just the default environment without storing
> the environment somewhere.

Yes, right. Depending on how (in)elegant the compat layer becomes,
longer may be shortened :) But let's see...

> >> Apart of it, which are the benefits of this patch ? Even if
> >> bootloader_env_set() is not successful, the update is not blocked (it is
> >> wanted that error is ignored). A transaction marker belongs to the
> >> features in SWUpdate, it should be always be done.
> > 
> > Again, agreed, except for when it shouldn't be done :)
> > Considering how versatile SWUpdate has become, one can use it as the
> > only channel to deliver software (in the most general meaning) down to
> > the device, e.g., container images, packages then to be updated via
> > a postinstall callout to the package manager, ....
> 
> How can SWUpdate know about the different use cases ?

One example could be having implemented (Lua) handlers for it, each
handling a different use case and them chain-calling other handlers.

> If you upgrade the OS, you want to set the marker.

Definitely.

> But what about a failed update of a single package, followed by a power-cut ?

The same question applies to a power-cut when your application writes
files, consider, e.g., a data collector application. I think this is not
the concern of SWUpdate anymore but that of the filesystem's power-cut
safety and package manager's transaction handling. But granted, one
should take care of what packages to update this way and how to check if
an update has succeeded. This is something one can do without a marker
in the bootloader's environment.

> > These use cases needn't be guarded by a transaction marker, I think.
> > More so, writing the bootloader environment just for a package upgrade
> > seems to be a bit too much...
> 
> Another way could be to make the variable name configurable, that is
> SWUpdate writes different variables.

Hm, then you'll end up having a zoo of bootloader environment
variables... I'm not sure I like this..

> The proposed patch selects this at build time - this looks to me a high
> restriction. I would see this patch as run-time condition (with a new
> command line parameter, maybe).

Well, a command-line parameter sets this for the entire run-time of
SWUpdate. So, say I install a number of application updates and then one
firmware via suricatta, the application updates needlessly also modify
the bootloader environment. Hence, it's for this use case the same as
doing it at compile-time. But granted, a more fine-grained selection is
certainly due.

> > That said, if having one SWUpdate running for all of these and more
> > types of update artifacts, then one may like to control per artifact
> > whether a marker should be set or not. This could, e.g., be done by
> > extending sw-description with a respective attribute that defaults to
> > "true" and can be set to "false" for update types sketched above.
> 
> Generally, we have several artifacts in a single SWU. The marker is
> global to ensure atomicity. The marker is set before installing
> anything. A higher granularity is against the atomicity.

Yes, I think it would be OK if it's global for one sw-description. Then,
one could package firmware with setting this attribute in one .swu and
package application updates in another .swu not setting this attribute.
I think that more fine-grain control may not be needed as this can be
exercised by the way artifacts are packaged in .swu files.

> >> In a dual-copy approach, this variable can be ignored in the boot
> >> scripts and I do not see any problems if it is set. In a single-copy, it
> >> must always be checked to avoid to run a broken system.
> > 
> > Yes, that's true. For an A/B system using suricatta I would disable
> > BOOTLOADER_TRANSACTION_MARKER for specific deployments where SWUpdate
> > handles not only firmware whereas for a single-copy system it should be
> > enabled.
> 
> Right, this is the general use case.
> 
> > That's why I left it defaulting to "yes", and to not break
> > existing deployments, of course.
> 
> I would prefer to rule this with a command line parameter instead of at
> build time.

Hm, how about the sw-description attribute, i.e., a global attribute for
one .swu/sw-description defaulting to "yes"? This is at run-time and
more flexible than a command line parameter...


Kind regards,
   Christian
Stefano Babic March 26, 2019, 9:50 p.m. UTC | #5
Hi Christian,

On 26/03/19 17:32, Christian Storm wrote:
> Hi Stefano,
> 
>>>>> Currently, SWUpdate sets the bootloader environment
>>>>> variable "recovery_status" to "in_progress" prior to
>>>>> an operation and either unsets or sets it to "failed"
>>>>> after the operation so that SWUpdate-external tools
>>>>> may use this information to take according measures.
>>>>>
>>>>> Make setting this bootloader environment variable
>>>>> configurable, defaulting to "yes".
>>>>
>>>> In fact, I have already thought that this mechanismus has something
>>>> missing.  IMHO the information that a new software was installed and it
>>>> is available is missing. This is covered by the "ustate" variable, but
>>>> just in case suricatta is running. I had added to set recovery_status to
>>>> "installed" after a successful update, but this breaks a lot of projects
>>>> where the u-boot script just check for the existence of the variable. So
>>>> I guess the best way is to stick with the couple recovery_status +
>>>> ustate for compatibility reason.
>>>
>>> Yes, I do agree here. This however doesn't rule out that we deprecate it
>>> and come up with a new bootloader environment variable that combines
>>> these two meanings in a uniform way into one variable that's being used
>>> for transaction marking purposes as well as for status transfer to
>>> external programs (or for whatever future use case may emerge).
>>
>> Right, I agree with you. The compatibility can be also realized in the
>> way the new SWU containing an updated SWUpdate changes the bootscript to
>> avoid to check "recovery_status". This is possible, it is difficult to
>> spread.
> 
> True. This would be something worth to be implemented in the example
> Yocto layers to show it off to people.

Right.

> 
>>> Modeling it after include/state.h's update_state_t extended with
>>> transaction marking entries would make a good start I think.
>>> Then, we need to have a compat implementation that maps this new to the
>>> current bootloader environment variables for some time...
>>
>> Right, and maybe for a longer time, if the bootscripts are not updated
>> (well, this is a SWUpdate's feature). I guess this could be an issue for
>> some projects working with just the default environment without storing
>> the environment somewhere.
> 
> Yes, right. Depending on how (in)elegant the compat layer becomes,
> longer may be shortened :) But let's see...

ok

> 
>>>> Apart of it, which are the benefits of this patch ? Even if
>>>> bootloader_env_set() is not successful, the update is not blocked (it is
>>>> wanted that error is ignored). A transaction marker belongs to the
>>>> features in SWUpdate, it should be always be done.
>>>
>>> Again, agreed, except for when it shouldn't be done :)
>>> Considering how versatile SWUpdate has become, one can use it as the
>>> only channel to deliver software (in the most general meaning) down to
>>> the device, e.g., container images, packages then to be updated via
>>> a postinstall callout to the package manager, ....
>>
>> How can SWUpdate know about the different use cases ?
> 
> One example could be having implemented (Lua) handlers for it, each
> handling a different use case and them chain-calling other handlers.
> 
>> If you upgrade the OS, you want to set the marker.
> 
> Definitely.
> 
>> But what about a failed update of a single package, followed by a power-cut ?
> 
> The same question applies to a power-cut when your application writes
> files, consider, e.g., a data collector application.

That's true - and this is important in case of databases, too. A
mechanism must be implemented that is power-cut safe. However, this is
not anymore a SWUpdate's problem...

> I think this is not
> the concern of SWUpdate anymore

Exactly, then I do not care - this should be solved at application level.

> but that of the filesystem's power-cut
> safety and package manager's transaction handling. But granted, one
> should take care of what packages to update this way and how to check if
> an update has succeeded. This is something one can do without a marker
> in the bootloader's environment.
> 
>>> These use cases needn't be guarded by a transaction marker, I think.
>>> More so, writing the bootloader environment just for a package upgrade
>>> seems to be a bit too much...
>>
>> Another way could be to make the variable name configurable, that is
>> SWUpdate writes different variables.
> 
> Hm, then you'll end up having a zoo of bootloader environment
> variables... I'm not sure I like this..

I was already thinking to merge recovery_status and ustate in a single
variable..

> 
>> The proposed patch selects this at build time - this looks to me a high
>> restriction. I would see this patch as run-time condition (with a new
>> command line parameter, maybe).
> 
> Well, a command-line parameter sets this for the entire run-time of
> SWUpdate.

But your patch is at compile time, that is even more restrictive.

> So, say I install a number of application updates and then one
> firmware via suricatta, the application updates needlessly also modify
> the bootloader environment. Hence, it's for this use case the same as
> doing it at compile-time.

Yes, it is.

> But granted, a more fine-grained selection is
> certainly due.
> 
>>> That said, if having one SWUpdate running for all of these and more
>>> types of update artifacts, then one may like to control per artifact
>>> whether a marker should be set or not. This could, e.g., be done by
>>> extending sw-description with a respective attribute that defaults to
>>> "true" and can be set to "false" for update types sketched above.
>>
>> Generally, we have several artifacts in a single SWU. The marker is
>> global to ensure atomicity. The marker is set before installing
>> anything. A higher granularity is against the atomicity.
> 
> Yes, I think it would be OK if it's global for one sw-description.

This is welcome - we can add a global attribute, like we have already
"version". This attribute could be board specific in case of
multi-device update, but inside a single device is global.

> Then,
> one could package firmware with setting this attribute in one .swu and
> package application updates in another .swu not setting this attribute.

This is ok, right.

> I think that more fine-grain control may not be needed as this can be
> exercised by the way artifacts are packaged in .swu files.

Agree.

> 
>>>> In a dual-copy approach, this variable can be ignored in the boot
>>>> scripts and I do not see any problems if it is set. In a single-copy, it
>>>> must always be checked to avoid to run a broken system.
>>>
>>> Yes, that's true. For an A/B system using suricatta I would disable
>>> BOOTLOADER_TRANSACTION_MARKER for specific deployments where SWUpdate
>>> handles not only firmware whereas for a single-copy system it should be
>>> enabled.
>>
>> Right, this is the general use case.
>>
>>> That's why I left it defaulting to "yes", and to not break
>>> existing deployments, of course.
>>
>> I would prefer to rule this with a command line parameter instead of at
>> build time.
> 
> Hm, how about the sw-description attribute, i.e., a global attribute for
> one .swu/sw-description defaulting to "yes"? This is at run-time and
> more flexible than a command line parameter...

Fully agree.

Regards,
Stefano
Storm, Christian April 1, 2019, 7:38 a.m. UTC | #6
Hi Stefano,

> > [...]
> > Hm, how about the sw-description attribute, i.e., a global attribute for
> > one .swu/sw-description defaulting to "yes"? This is at run-time and
> > more flexible than a command line parameter...
> 
> Fully agree.

OK, I'll come back with a new patch.


Kind regards,
   Christian
diff mbox series

Patch

diff --git a/Kconfig b/Kconfig
index 302166e..c1404c5 100644
--- a/Kconfig
+++ b/Kconfig
@@ -102,6 +102,17 @@  config SCRIPTS
 	  in the image. For security reason, this option
 	  can be switched off.
 
+config BOOTLOADER_TRANSACTION_MARKER
+	bool "enable bootloader transaction marker"
+	default y
+	help
+	  Enabling this option sets the bootloader environment
+	  variable "recovery_status" to "in_progress" prior to
+	  an operation and either unsets or sets it to "failed"
+	  after the operation.
+	  SWUpdate-external tools (e.g., bootloader scripts)
+	  may use this information to take according measures.
+
 config HW_COMPATIBILITY
 	bool "check for hardware / software compatibility"
 	default n
diff --git a/core/swupdate.c b/core/swupdate.c
index 476358b..e9a211a 100644
--- a/core/swupdate.c
+++ b/core/swupdate.c
@@ -369,7 +369,9 @@  static int install_from_file(char *fname, int check)
 	/*
 	 * Set "recovery_status" as begin of the transaction"
 	 */
+#ifdef CONFIG_BOOTLOADER_TRANSACTION_MARKER
 	bootloader_env_set(BOOTVAR_TRANSACTION, "in_progress");
+#endif
 
 	ret = install_images(&swcfg, fdsw, 1);
 
@@ -382,7 +384,9 @@  static int install_from_file(char *fname, int check)
 		return EXIT_FAILURE;
 	}
 
+#ifdef CONFIG_BOOTLOADER_TRANSACTION_MARKER
 	bootloader_env_unset(BOOTVAR_TRANSACTION);
+#endif
 	fprintf(stdout, "Software updated successfully\n");
 	fprintf(stdout, "Please reboot the device to start the new software\n");
 
@@ -969,7 +973,9 @@  int main(int argc, char **argv)
 		result = install_from_file(fname, opt_c);
 		switch (result) {
 		case EXIT_FAILURE:
+#ifdef CONFIG_BOOTLOADER_TRANSACTION_MARKER
 			bootloader_env_set(BOOTVAR_TRANSACTION, "failed");
+#endif
 			break;
 		case EXIT_SUCCESS:
 			notify(SUCCESS, 0, INFOLEVEL, NULL);
diff --git a/corelib/stream_interface.c b/corelib/stream_interface.c
index 1261958..d7774bb 100644
--- a/corelib/stream_interface.c
+++ b/corelib/stream_interface.c
@@ -237,7 +237,9 @@  static int extract_files(int fd, struct swupdate_cfg *software)
 				 * just once
 				 */
 				if (!installed_directly) {
+#ifdef CONFIG_BOOTLOADER_TRANSACTION_MARKER
 					bootloader_env_set(BOOTVAR_TRANSACTION, "in_progress");
+#endif
 					installed_directly = true;
 				}
 
@@ -393,12 +395,16 @@  void *network_initializer(void *data)
 			 * must be successful. Set we have
 			 * initiated an update
 			 */
+#ifdef CONFIG_BOOTLOADER_TRANSACTION_MARKER
 			bootloader_env_set(BOOTVAR_TRANSACTION, "in_progress");
+#endif
 
 			notify(RUN, RECOVERY_NO_ERROR, INFOLEVEL, "Installation in progress");
 			ret = install_images(software, 0, 0);
 			if (ret != 0) {
+#ifdef CONFIG_BOOTLOADER_TRANSACTION_MARKER
 				bootloader_env_set(BOOTVAR_TRANSACTION, "failed");
+#endif
 				notify(FAILURE, RECOVERY_ERROR, ERRORLEVEL, "Installation failed !");
 				inst.last_install = FAILURE;
 
@@ -407,7 +413,9 @@  void *network_initializer(void *data)
 				 * Clear the recovery variable to indicate to bootloader
 				 * that it is not required to start recovery again
 				 */
+#ifdef CONFIG_BOOTLOADER_TRANSACTION_MARKER
 				bootloader_env_unset(BOOTVAR_TRANSACTION);
+#endif
 				notify(SUCCESS, RECOVERY_NO_ERROR, INFOLEVEL, "SWUPDATE successful !");
 				inst.last_install = SUCCESS;
 			}