Ugh... I have just realized that this will not build properly if HAVE_ATEXIT is not defined. Please look below...
On Mon, May 24, 2021 at 10:59:34AM +0100, Dimitri John Ledkov wrote: > From: Dimitri John Ledkov <x...@ubuntu.com> > > Refactor clean_grub_dir to create a backup of all the files, instead > of just irrevocably removing them as the first action. If available, > register atexit handle to restore the backup if errors occur before > point of no return, or remove the backup if everything was > successful. If atexit is not available, the backup remains on disk for > manual recovery. > > Some platforms defined a point of no return, i.e. after modules & core > images were updated. Failures from any commands after that stage are > ignored, and backup is cleanedup. For example, on EFI platforms update > is not reverted when efibootmgr fails. > > Extra care is taken to ensure atexit handler is only invoked by the > parent process and not any children forks. Some older grub codebases > can invoke parent atexit hooks from forks, which can mess up the > backup. > > This allows safer upgrades of MBR & modules, such that > modules/images/fonts/translations are consistent with MBR in case of > errors. For example accidental grub-install /dev/non-existent-disk > currently clobbers and upgrades modules in /boot/grub, despite not > actually updating any MBR. > > This patch only handles backup and restore of files copied to > /boot/grub. This patch does not perform backup (or restoration) of MBR > itself or blocklists. Thus when installing i386-pc platform, > corruption may still occur with MBR and blocklists which will not be > attempted to be automatically recovered. > > Also add modinfo.sh and *.efi to the cleanup/backup/restore codepath, > to ensure it is also cleaned / backed up / restored. > > Signed-off-by: Dimitri John Ledkov <x...@ubuntu.com> > --- > > changes since v4: > + change ponr from size_t to int (there is no bool in grub yet) > + declare loop variable at the start of the function > + format one more comment correctly > + add comment about significance to set ponr > > configure.ac | 2 +- > include/grub/util/install.h | 13 ++++ > util/grub-install-common.c | 143 ++++++++++++++++++++++++++++++++---- > util/grub-install.c | 40 ++++++++-- > util/grub-mknetdir.c | 3 + > util/grub-mkrescue.c | 3 + > util/grub-mkstandalone.c | 2 + > 7 files changed, 182 insertions(+), 24 deletions(-) > > diff --git a/configure.ac b/configure.ac > index 74719416c4..a5e94f360e 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -419,7 +419,7 @@ else > fi > > # Check for functions and headers. > -AC_CHECK_FUNCS(posix_memalign memalign getextmntent) > +AC_CHECK_FUNCS(posix_memalign memalign getextmntent atexit) > AC_CHECK_HEADERS(sys/param.h sys/mount.h sys/mnttab.h limits.h) > > # glibc 2.25 still includes sys/sysmacros.h in sys/types.h but emits > deprecation > diff --git a/include/grub/util/install.h b/include/grub/util/install.h > index b93c73caac..b2a0175374 100644 > --- a/include/grub/util/install.h > +++ b/include/grub/util/install.h > @@ -275,4 +275,17 @@ extern char *grub_install_copy_buffer; > int > grub_install_is_short_mbrgap_supported (void); > > +/* > + * grub-install-common tries to make backups of modules & auxilary > + * files, and restore the backup upon failure to install core.img. There > + * are platforms with additional actions after modules & core got > + * installed in place. It is a point of no return, as core.img cannot be > + * reverted from this point onwards, and new modules should be kept > + * installed. Before performing these additional actions raise > + * grub_install_backup_ponr flag, this way failure to perform additional > + * actions will not result in reverting new modules to the old > + * ones. (i.e. in case efivars updates fails) > + */ > +extern int grub_install_backup_ponr; I would use a function which sets this variable and then #ifdef HAVE_ATEXIT extern void grub_set_install_backup_ponr (void); #else static inline void grub_set_install_backup_ponr (void) { } #endif > + > #endif > diff --git a/util/grub-install-common.c b/util/grub-install-common.c > index b4f28840f1..d493e4a119 100644 > --- a/util/grub-install-common.c > +++ b/util/grub-install-common.c > @@ -185,38 +185,151 @@ grub_install_mkdir_p (const char *dst) > free (t); > } > > +static int > +strcmp_ext (const char *a, const char *b, const char *ext) > +{ > + char *bsuffix = grub_util_path_concat_ext (1, b, ext); > + int r = strcmp (a, bsuffix); > + > + free (bsuffix); > + return r; > +} > + > +enum clean_grub_dir_mode > +{ > + CLEAN_NEW, > + CLEAN_BACKUP, > + CREATE_BACKUP, > + RESTORE_BACKUP > +}; > + #ifdef HAVE_ATEXIT > +static size_t backup_dirs_size = 0; > +static char **backup_dirs = NULL; > +static pid_t backup_process = 0; > +int grub_install_backup_ponr = 0; #endif [...] #ifdef HAVE_ATEXIT void grub_set_install_backup_ponr (void) { grub_install_backup_ponr = 1; } > +static void > +restore_backup_atexit (void) > +{ > + size_t i; > + /* > + * some child inherited atexit handler, did not clear it, and called > + * it; thus skip clean or restore logic. > + */ > + if (backup_process != getpid ()) > + return; > + > + for (i = 0; i < backup_dirs_size; i++) > + { > + /* > + * if past point of no return simply clean the backups. > + * Otherwise cleanup newly installed files, and restore the > + * backups > + */ > + if (grub_install_backup_ponr) > + clean_grub_dir_real (backup_dirs[i], CLEAN_BACKUP); > + else > + { > + clean_grub_dir_real (backup_dirs[i], CLEAN_NEW); > + clean_grub_dir_real (backup_dirs[i], RESTORE_BACKUP); > + } > + free (backup_dirs[i]); > + } > + > + backup_dirs_size = 0; > + > + free (backup_dirs); > +} > + > +static void > +append_to_backup_dirs (const char *dir) > +{ > + backup_dirs = xrealloc (backup_dirs, sizeof (char *) * (backup_dirs_size + > 1)); > + backup_dirs[backup_dirs_size] = strdup (dir); > + backup_dirs_size++; > + if (!backup_process) > + { > + atexit (restore_backup_atexit); > + backup_process = getpid (); > + } > +} #else static void append_to_backup_dirs (const char *dir __attribute__ ((unused))) { } #endif > +static void > +clean_grub_dir (const char *di) > +{ > + clean_grub_dir_real (di, CLEAN_BACKUP); > + clean_grub_dir_real (di, CREATE_BACKUP); > +#if defined(HAVE_ATEXIT) Please drop this #if... > + append_to_backup_dirs (di); > +#endif > +} I think we need at least these changes. Otherwise if HAVE_ATEXIT is not defined we will have unused code which will compiler complain about... Please try to build the GRUB with and without HAVE_ATEXIT defined after fixing this issue. Daniel _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel