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