diff mbox series

[2/2] Run preinstall scripts before installing an image directly

Message ID YfAhSZ+8Kl9GS2dk@decadent.org.uk
State Changes Requested
Headers show
Series [1/2] installer: Rename extract_scripts() and update comments | expand

Commit Message

Ben Hutchings Jan. 25, 2022, 4:11 p.m. UTC
Currently preinstall scripts are run *after* updating any images that
have the installed-directly flag set.  This is a regression from
SWUpdate 2020.11, at least when updating from a local file.

* Split out the move and execution of preinstall scripts from
  install_images() into new function ensure_preinstall_scripts_done().
* Add a flag in swupdate_cfg and use it to make the function
  idempotent.
* Call it from extract_files() before installing the first image
  directly.
* Document the requirement on ordering of files in the cpio archive.

Signed-off-by: Ben Hutchings <ben.hutchings_ext@softathome.com>
---
 core/installer.c              | 46 +++++++++++++++++++++++++----------
 core/stream_interface.c       |  5 +++-
 doc/source/sw-description.rst |  5 +++-
 include/installer.h           |  1 +
 include/swupdate.h            |  1 +
 5 files changed, 43 insertions(+), 15 deletions(-)

Comments

Stefano Babic Jan. 26, 2022, 2:59 p.m. UTC | #1
Hi Ben,

On 25.01.22 17:11, Ben Hutchings wrote:
> Currently preinstall scripts are run *after* updating any images that
> have the installed-directly flag set.

True, but this was debated several times on ML and there is another 
solution for it.

>  This is a regression from
> SWUpdate 2020.11, at least when updating from a local file.

That it worked with previous releases was a side efeect due to different 
and special paths when the source is a local file. This led to 
duplication of many function (with duplication of bugs, of course) and 
was solved later. That means there is no difference anymore between OTA 
and local update, and using installed-direct for streaming is required 
after 2020.11 for local update, too.

During OTA update and when stream is activated, SWUpdate must install 
what is coming and it cannot wait for scripts that are maybe embedded 
later in the CPIO archive. This means that preinstall scripts cannot run 
before any artifact is installed with stream (installed-directly) 
activated. But they are still called before installing artifacts when 
streaming is off.

To solve this, "embedded-script" in sw-description was implemented. This 
alloes to have Lua scripts directly into sw-description, and they are 
called at parsing time before installing anything, solving the issue 
with pre-install scripts.  What you add here cannot work if streaming is 
on and if the script is packed later in the SWU.

Best regards,
Stefano Babic

> 
> * Split out the move and execution of preinstall scripts from
>    install_images() into new function ensure_preinstall_scripts_done().
> * Add a flag in swupdate_cfg and use it to make the function
>    idempotent.
> * Call it from extract_files() before installing the first image
>    directly.
> * Document the requirement on ordering of files in the cpio archive.
> 
> Signed-off-by: Ben Hutchings <ben.hutchings_ext@softathome.com>
> ---
>   core/installer.c              | 46 +++++++++++++++++++++++++----------
>   core/stream_interface.c       |  5 +++-
>   doc/source/sw-description.rst |  5 +++-
>   include/installer.h           |  1 +
>   include/swupdate.h            |  1 +
>   5 files changed, 43 insertions(+), 15 deletions(-)
> 
> diff --git a/core/installer.c b/core/installer.c
> index 089f638..2b3d1ab 100644
> --- a/core/installer.c
> +++ b/core/installer.c
> @@ -241,21 +241,13 @@ int install_single_image(struct img_type *img, bool dry_run)
>   	return ret;
>   }
>   
> -/*
> - * streamfd: file descriptor if it is required to extract
> - *           images from the stream (update from file)
> - * extract : boolean, true to enable extraction
> - */
> -
> -int install_images(struct swupdate_cfg *sw)
> +int ensure_preinstall_scripts_done(struct swupdate_cfg *sw)
>   {
>   	int ret;
> -	struct img_type *img, *tmp;
> -	char *filename;
> -	struct stat buf;
> -	const char* TMPDIR = get_tmpdir();
>   	bool dry_run = sw->parms.dry_run;
> -	bool dropimg;
> +
> +	if (sw->preinstall_scripts_done)
> +		return 0;
>   
>   	/* Move all scripts, preinstall scripts must be run now */
>   	const char* tmpdir_scripts = get_tmpdirscripts();
> @@ -275,6 +267,34 @@ int install_images(struct swupdate_cfg *sw)
>   		}
>   	}
>   
> +	sw->preinstall_scripts_done = true;
> +	return 0;
> +}
> +
> +/*
> + * streamfd: file descriptor if it is required to extract
> + *           images from the stream (update from file)
> + * extract : boolean, true to enable extraction
> + */
> +
> +int install_images(struct swupdate_cfg *sw)
> +{
> +	int ret;
> +	struct img_type *img, *tmp;
> +	char *filename;
> +	struct stat buf;
> +	const char* TMPDIR = get_tmpdir();
> +	bool dry_run = sw->parms.dry_run;
> +	bool dropimg;
> +
> +	/*
> +	 * Run preinstall scripts if they weren't already triggered a
> +	 * directly installed image.
> +	 */
> +	ret = ensure_preinstall_scripts_done(sw);
> +	if (ret)
> +		return ret;
> +
>   	LIST_FOREACH_SAFE(img, &sw->images, next, tmp) {
>   
>   		dropimg = false;
> diff --git a/core/stream_interface.c b/core/stream_interface.c
> index 1c9853d..b721509 100644
> --- a/core/stream_interface.c
> +++ b/core/stream_interface.c
> @@ -251,12 +251,15 @@ static int extract_files(int fd, struct swupdate_cfg *software)
>   				/*
>   				 * If this is the first image to be directly installed, set transaction flag
>   				 * to on to be checked if a power-off happens. Be sure to set the flag
> -				 * just once
> +				 * just once. Also run preinstall scripts, if there are any.
>   				 */
>   				if (!installed_directly) {
>   					if (!software->parms.dry_run && software->bootloader_transaction_marker) {
>   						bootloader_env_set(BOOTVAR_TRANSACTION, get_state_string(STATE_IN_PROGRESS));
>   					}
> +					if (ensure_preinstall_scripts_done(software)) {
> +						return -1;
> +					}
>   					installed_directly = true;
>   				}
>   
> diff --git a/doc/source/sw-description.rst b/doc/source/sw-description.rst
> index 3de3636..b8f72b8 100644
> --- a/doc/source/sw-description.rst
> +++ b/doc/source/sw-description.rst
> @@ -1334,7 +1334,10 @@ There are 4 main sections inside sw-description:
>      | installed-\ | bool     | images     | flag to indicate that image is        |
>      | directly    |          |            | streamed into the target without any  |
>      |             |          |            | temporary copy. Not all handlers      |
> -   |             |          |            | support streaming.                    |
> +   |             |          |            | support streaming. If this flag is    |
> +   |             |          |            | set, all preinstall scripts used with |
> +   |             |          |            | the image must be placed before it in |
> +   |             |          |            | the cpio archive.                     |
>      +-------------+----------+------------+---------------------------------------+
>      | name        | string   | bootenv    | name of the bootloader variable to be |
>      |             |          |            | set.                                  |
> diff --git a/include/installer.h b/include/installer.h
> index 80aa442..7d415b9 100644
> --- a/include/installer.h
> +++ b/include/installer.h
> @@ -17,6 +17,7 @@
>   swupdate_file_t check_if_required(struct imglist *list, struct filehdr *pfdh,
>   				const char *destdir,
>   				struct img_type **pimg);
> +int ensure_preinstall_scripts_done(struct swupdate_cfg *sw);
>   int install_images(struct swupdate_cfg *sw);
>   int install_single_image(struct img_type *img, bool dry_run);
>   int install_from_file(const char *filename, bool check);
> diff --git a/include/swupdate.h b/include/swupdate.h
> index 4cce892..9d350ca 100644
> --- a/include/swupdate.h
> +++ b/include/swupdate.h
> @@ -150,6 +150,7 @@ struct swupdate_cfg {
>   	bool no_transaction_marker;
>   	bool no_state_marker;
>   	bool check_max_version;
> +	bool preinstall_scripts_done;
>   	int verbose;
>   	int loglevel;
>   	int cert_purpose;
Ben Hutchings Jan. 26, 2022, 3:30 p.m. UTC | #2
Stefano Babic wrote:
> Hi Ben,
>
> On 25.01.22 17:11, Ben Hutchings wrote:
> > Currently preinstall scripts are run *after* updating any images that
> > have the installed-directly flag set.
>
> True, but this was debated several times on ML and there is another
> solution for it.
>
> >  This is a regression from
> > SWUpdate 2020.11, at least when updating from a local file.
>
> That it worked with previous releases was a side efeect due to different
> and special paths when the source is a local file.

I understand that. But it is reasonable to expect that "preinstall" scripts
run before installation, and that no longer happens in cases where it
used to.

What's really bad is that SWUpdate does not report an error for this
unsupported case, but just does things in the wrong order.

> During OTA update and when stream is activated, SWUpdate must install
> what is coming and it cannot wait for scripts that are maybe embedded
> later in the CPIO archive.

Of course, and this patch documents the ordering requirement.

> This means that preinstall scripts cannot run
> before any artifact is installed with stream (installed-directly)
> activated. But they are still called before installing artifacts when
> streaming is off.

Not all devices have enough free space to be able to install updates
without this. So it is important that installed-directly can work
together with preinstall scripts.

The particular use case I have is that I'm using a custom script
handler to update boot metadata for an unusual boot loader. I don't
expect this to be suitable for SWUpdate upstream so it's internal for
now. I'm not even using a script file in the update image, just
properties. But it is still being called too late when using SWUpdate
2021.11.

> To solve this, "embedded-script" in sw-description was implemented.

Thank you, I will look at this.

> This
> alloes to have Lua scripts directly into sw-description, and they are
> called at parsing time before installing anything, solving the issue
> with pre-install scripts.  What you add here cannot work if streaming is
> on and if the script is packed later in the SWU.

And if the files are ordered correctly, then what's the problem?

Ben.
Stefano Babic Jan. 26, 2022, 3:46 p.m. UTC | #3
Hi Ben,

On 26.01.22 16:30, Ben HUTCHINGS (EXT) wrote:
> Stefano Babic wrote:
>> Hi Ben,
>>
>> On 25.01.22 17:11, Ben Hutchings wrote:
>>> Currently preinstall scripts are run *after* updating any images that
>>> have the installed-directly flag set.
>>
>> True, but this was debated several times on ML and there is another
>> solution for it.
>>
>>>   This is a regression from
>>> SWUpdate 2020.11, at least when updating from a local file.
>>
>> That it worked with previous releases was a side efeect due to different
>> and special paths when the source is a local file.
> 
> I understand that. But it is reasonable to expect that "preinstall" scripts
> run before installation, and that no longer happens in cases where it
> used to.
> 

The attempt is to document this - see doc/source/swupdate.rst:

"- Support for preinstall scripts. They run after streamed handlers have
    handled their data, and before regular handlers."

and

" - iterates through all `scripts` and call the corresponding
    handler for pre-install scripts.
    Please note: if artifacts are streamed, they will be extracted
    before this runs. If earlier execution is required, please use
    the "embedded-script" or hooks features to ensure code is run
    before installation takes place.
"

> What's really bad is that SWUpdate does not report an error for this
> unsupported case, but just does things in the wrong order.

Because this is another use case that can happen, that is a pre-install 
script called after streamed artifacts were already installed and before 
a not-streamed artifact must be installed. It cannot know if the missing 
script is thpought to be run later, but before another artifact.

> 
>> During OTA update and when stream is activated, SWUpdate must install
>> what is coming and it cannot wait for scripts that are maybe embedded
>> later in the CPIO archive.
> 
> Of course, and this patch documents the ordering requirement.

Point is that strict order in SWU was already discussed and rejected.

> 
>> This means that preinstall scripts cannot run
>> before any artifact is installed with stream (installed-directly)
>> activated. But they are still called before installing artifacts when
>> streaming is off.
> 
> Not all devices have enough free space to be able to install updates
> without this. So it is important that installed-directly can work
> together with preinstall scripts.
> 
> The particular use case I have is that I'm using a custom script
> handler to update boot metadata for an unusual boot loader. I don't
> expect this to be suitable for SWUpdate upstream so it's internal for
> now. I'm not even using a script file in the update image, just
> properties. But it is still being called too late when using SWUpdate
> 2021.11.
> 
>> To solve this, "embedded-script" in sw-description was implemented.
> 
> Thank you, I will look at this.
> 

Fine.

>> This
>> alloes to have Lua scripts directly into sw-description, and they are
>> called at parsing time before installing anything, solving the issue
>> with pre-install scripts.  What you add here cannot work if streaming is
>> on and if the script is packed later in the SWU.
> 
> And if the files are ordered correctly, then what's the problem?

For design, a SWU can be packed in any order without any strict 
requirement, with just sw-description and sw-description.sig (for signed 
images) to be the first files in CPIO. This lets to unpack and repack 
the SWU from external tools, for example due to re-encrypt or re-sign an 
older version without having to know details about the software (for 
example, which artifacts are scripts, and if they are preinstall 
scripts, and so on). Your patch requires this reorder (of course), but 
to avoid this, the solution with embedded-script (even if more complex) 
was developed. I am sure you can use it in your project, too.

Best regards,
Stefano Babic
Dominique Martinet Jan. 27, 2022, 1:35 a.m. UTC | #4
Ben HUTCHINGS (EXT) wrote on Wed, Jan 26, 2022 at 03:30:40PM +0000:
> > To solve this, "embedded-script" in sw-description was implemented.
> 
> Thank you, I will look at this.

For what it's worth, I wasn't confident in my ability to do error
handling in lua so I've taken a different approach -- we're using
another installed-directly handler to run some scripts first thing in
the update.

Note that it means care must be taken to have said script first in the
cpio archive when generating the swu, but as long as that is respected
the script is the first thing to run in the image.


Unfortunatelly the shell script handler doesn't seem to work with
installed-directly, but that's probably an easy fix if there is interest
in that?
I don't know how others generate images so I don't know if ordering
files within the cpio archive is a realistic constraint for most...


(We're doing this with our own "exec handler" that stores a file to tmp
a runs a command with that file as argument, which is a lesser version
of the pipe handler that had been discussed here briefly last year.
I'd like to upstream something at some point but I'm not really
convinced myself so this has been left in the closet for now...
If anyone cares the code is public though:
https://github.com/atmark-techno/swupdate
https://github.com/atmark-techno/swupdate-mkimage
)
diff mbox series

Patch

diff --git a/core/installer.c b/core/installer.c
index 089f638..2b3d1ab 100644
--- a/core/installer.c
+++ b/core/installer.c
@@ -241,21 +241,13 @@  int install_single_image(struct img_type *img, bool dry_run)
 	return ret;
 }
 
-/*
- * streamfd: file descriptor if it is required to extract
- *           images from the stream (update from file)
- * extract : boolean, true to enable extraction
- */
-
-int install_images(struct swupdate_cfg *sw)
+int ensure_preinstall_scripts_done(struct swupdate_cfg *sw)
 {
 	int ret;
-	struct img_type *img, *tmp;
-	char *filename;
-	struct stat buf;
-	const char* TMPDIR = get_tmpdir();
 	bool dry_run = sw->parms.dry_run;
-	bool dropimg;
+
+	if (sw->preinstall_scripts_done)
+		return 0;
 
 	/* Move all scripts, preinstall scripts must be run now */
 	const char* tmpdir_scripts = get_tmpdirscripts();
@@ -275,6 +267,34 @@  int install_images(struct swupdate_cfg *sw)
 		}
 	}
 
+	sw->preinstall_scripts_done = true;
+	return 0;
+}
+
+/*
+ * streamfd: file descriptor if it is required to extract
+ *           images from the stream (update from file)
+ * extract : boolean, true to enable extraction
+ */
+
+int install_images(struct swupdate_cfg *sw)
+{
+	int ret;
+	struct img_type *img, *tmp;
+	char *filename;
+	struct stat buf;
+	const char* TMPDIR = get_tmpdir();
+	bool dry_run = sw->parms.dry_run;
+	bool dropimg;
+
+	/*
+	 * Run preinstall scripts if they weren't already triggered a
+	 * directly installed image.
+	 */
+	ret = ensure_preinstall_scripts_done(sw);
+	if (ret)
+		return ret;
+
 	LIST_FOREACH_SAFE(img, &sw->images, next, tmp) {
 
 		dropimg = false;
diff --git a/core/stream_interface.c b/core/stream_interface.c
index 1c9853d..b721509 100644
--- a/core/stream_interface.c
+++ b/core/stream_interface.c
@@ -251,12 +251,15 @@  static int extract_files(int fd, struct swupdate_cfg *software)
 				/*
 				 * If this is the first image to be directly installed, set transaction flag
 				 * to on to be checked if a power-off happens. Be sure to set the flag
-				 * just once
+				 * just once. Also run preinstall scripts, if there are any.
 				 */
 				if (!installed_directly) {
 					if (!software->parms.dry_run && software->bootloader_transaction_marker) {
 						bootloader_env_set(BOOTVAR_TRANSACTION, get_state_string(STATE_IN_PROGRESS));
 					}
+					if (ensure_preinstall_scripts_done(software)) {
+						return -1;
+					}
 					installed_directly = true;
 				}
 
diff --git a/doc/source/sw-description.rst b/doc/source/sw-description.rst
index 3de3636..b8f72b8 100644
--- a/doc/source/sw-description.rst
+++ b/doc/source/sw-description.rst
@@ -1334,7 +1334,10 @@  There are 4 main sections inside sw-description:
    | installed-\ | bool     | images     | flag to indicate that image is        |
    | directly    |          |            | streamed into the target without any  |
    |             |          |            | temporary copy. Not all handlers      |
-   |             |          |            | support streaming.                    |
+   |             |          |            | support streaming. If this flag is    |
+   |             |          |            | set, all preinstall scripts used with |
+   |             |          |            | the image must be placed before it in |
+   |             |          |            | the cpio archive.                     |
    +-------------+----------+------------+---------------------------------------+
    | name        | string   | bootenv    | name of the bootloader variable to be |
    |             |          |            | set.                                  |
diff --git a/include/installer.h b/include/installer.h
index 80aa442..7d415b9 100644
--- a/include/installer.h
+++ b/include/installer.h
@@ -17,6 +17,7 @@ 
 swupdate_file_t check_if_required(struct imglist *list, struct filehdr *pfdh,
 				const char *destdir,
 				struct img_type **pimg);
+int ensure_preinstall_scripts_done(struct swupdate_cfg *sw);
 int install_images(struct swupdate_cfg *sw);
 int install_single_image(struct img_type *img, bool dry_run);
 int install_from_file(const char *filename, bool check);
diff --git a/include/swupdate.h b/include/swupdate.h
index 4cce892..9d350ca 100644
--- a/include/swupdate.h
+++ b/include/swupdate.h
@@ -150,6 +150,7 @@  struct swupdate_cfg {
 	bool no_transaction_marker;
 	bool no_state_marker;
 	bool check_max_version;
+	bool preinstall_scripts_done;
 	int verbose;
 	int loglevel;
 	int cert_purpose;