Hey, In general much better but...
On Thu, Apr 29, 2021 at 12:36:37PM +0100, Dimitri John Ledkov wrote: > 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 increases peak disk-usage slightly, by > requiring temporarily twice the disk space to complete grub-install. I think this last sentence does not parse with the rest of paragraph. Additionally, may I ask you to add a blurb here about the blocklist? I think about most important things from the email exchange between Micheal and you. > 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 v2: > - switch from on_exit, to atexit > - introduce point of no return flag, as atexit doesn't know about > exit status and at times it is desired to declare point of no > return earlier and ignore some error. > - address review feedback wrt styling > - improve reliablity, verify that only parent process calls atexit > hook > > configure.ac | 2 +- > include/grub/util/install.h | 11 +++ > util/grub-install-common.c | 142 ++++++++++++++++++++++++++++++++---- > util/grub-install.c | 33 +++++++-- > util/grub-mknetdir.c | 3 + > util/grub-mkrescue.c | 3 + > util/grub-mkstandalone.c | 2 + > 7 files changed, 172 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..d729060ce7 100644 > --- a/include/grub/util/install.h > +++ b/include/grub/util/install.h > @@ -275,4 +275,15 @@ 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) */ Could you format comments like it is documented here: https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html#Comments > +extern size_t grub_install_backup_ponr; > + > #endif > diff --git a/util/grub-install-common.c b/util/grub-install-common.c > index b4f28840f1..db7feae7d3 100644 > --- a/util/grub-install-common.c > +++ b/util/grub-install-common.c > @@ -185,38 +185,150 @@ 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, CLEAN what? s/CLEAN/CLEAN_NEW/? > + CLEAN_BACKUP, > + CREATE_BACKUP, > + RESTORE_BACKUP, Please drop this redundant "," at the end. > +}; > + > +static size_t backup_dirs_len = 0; s/backup_dirs_len/backup_dirs_nr/? or s/backup_dirs_len/backup_dirs_size/? > +static char **backup_dirs; > +static pid_t backup_process = 0; > +size_t grub_install_backup_ponr = 0; > + > static void > -clean_grub_dir (const char *di) > +clean_grub_dir_real (const char *di, enum clean_grub_dir_mode mode) > { > grub_util_fd_dir_t d; > grub_util_fd_dirent_t de; > + char suffix[2] = ""; const char *suffix = ""; > + if ((mode == CLEAN_BACKUP) || (mode == RESTORE_BACKUP)) > + strcpy (suffix, "~"); suffix = "~"; > d = grub_util_fd_opendir (di); > if (!d) > - grub_util_error (_("cannot open directory `%s': %s"), > - di, grub_util_fd_strerror ()); > + { > + if (mode == CLEAN_BACKUP) > + return; > + grub_util_error (_("cannot open directory `%s': %s"), > + di, grub_util_fd_strerror ()); > + } > > while ((de = grub_util_fd_readdir (d))) > { > const char *ext = strrchr (de->d_name, '.'); > - if ((ext && (strcmp (ext, ".mod") == 0 > - || strcmp (ext, ".lst") == 0 > - || strcmp (ext, ".img") == 0 > - || strcmp (ext, ".mo") == 0) > - && strcmp (de->d_name, "menu.lst") != 0) > - || strcmp (de->d_name, "efiemu32.o") == 0 > - || strcmp (de->d_name, "efiemu64.o") == 0) > + > + if ((ext && (strcmp_ext (ext, ".mod", suffix) == 0 > + || strcmp_ext (ext, ".lst", suffix) == 0 > + || strcmp_ext (ext, ".img", suffix) == 0 > + || strcmp_ext (ext, ".efi", suffix) == 0 > + || strcmp_ext (ext, ".mo", suffix) == 0) > + && strcmp_ext (de->d_name, "menu.lst", suffix) != 0) > + || strcmp_ext (de->d_name, "modinfo.sh", suffix) == 0 > + || strcmp_ext (de->d_name, "efiemu32.o", suffix) == 0 > + || strcmp_ext (de->d_name, "efiemu64.o", suffix) == 0) > { > - char *x = grub_util_path_concat (2, di, de->d_name); > - if (grub_util_unlink (x) < 0) > - grub_util_error (_("cannot delete `%s': %s"), x, > - grub_util_fd_strerror ()); > - free (x); > + char *srcf = grub_util_path_concat (2, di, de->d_name); > + > + if (mode == CREATE_BACKUP) > + { > + char *dstf = grub_util_path_concat_ext (2, di, de->d_name, "~"); > + > + if (grub_util_rename (srcf, dstf) < 0) > + grub_util_error (_("cannot backup `%s': %s"), srcf, > + grub_util_fd_strerror ()); > + free (dstf); > + } > + else if (mode == RESTORE_BACKUP) > + { > + char *dstf = grub_util_path_concat (2, di, de->d_name); > + > + dstf[strlen (dstf) - 1] = 0; s/0/'\0'/ The final result is the same but it is a bit clearer what you mean... > + if (grub_util_rename (srcf, dstf) < 0) > + grub_util_error (_("cannot restore `%s': %s"), dstf, > + grub_util_fd_strerror ()); > + free (dstf); > + } > + else > + { > + if (grub_util_unlink (srcf) < 0) > + grub_util_error (_("cannot delete `%s': %s"), srcf, > + grub_util_fd_strerror ()); > + } > + free (srcf); > } > } > grub_util_fd_closedir (d); > } > > +static void > +restore_backup_atexit (void) > +{ > + /* some child inherited atexit handler, did not clear it, and > + * called it, skip clean or restore logic */ Please fix this comment as I mentioned above... > + if (backup_process != getpid ()) > + return; > + > + for (size_t i = 0; i < backup_dirs_len; 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 == 1) I would drop "== 1" here. > + clean_grub_dir_real (backup_dirs[i], CLEAN_BACKUP); > + else > + { > + clean_grub_dir_real (backup_dirs[i], CLEAN); > + clean_grub_dir_real (backup_dirs[i], RESTORE_BACKUP); > + } > + free (backup_dirs[i]); > + } > + > + backup_dirs_len = 0; > + > + free (backup_dirs); > +} > + > +static void > +append_to_backup_dirs (const char *dir) > +{ > + if (backup_dirs_len == 0) > + backup_dirs = xmalloc (sizeof (char *) * (backup_dirs_len + 1)); > + else > + backup_dirs = > + xrealloc (backup_dirs, sizeof (char *) * (backup_dirs_len + 1)); backup_dirs = xrealloc (backup_dirs, sizeof (char *) * (backup_dirs_len + 1)); You do not need "if (backup_dirs_len == 0)" because xrealloc() is able to serve you properly in both cases you care about. And even more... :-) > + backup_dirs[backup_dirs_len] = strdup (dir); > + backup_dirs_len++; > + if (backup_process == 0) if (!backup_process) > + { > + atexit (restore_backup_atexit); > + backup_process = getpid (); > + } > +} > + > +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) > + append_to_backup_dirs (di); > +#endif > +} > + > struct install_list > { > int is_default; > diff --git a/util/grub-install.c b/util/grub-install.c > index a0babe3eff..f85cf473ff 100644 > --- a/util/grub-install.c > +++ b/util/grub-install.c > @@ -1719,10 +1719,14 @@ main (int argc, char *argv[]) > > /* Now perform the installation. */ > if (install_bootsector) > - grub_util_bios_setup (platdir, "boot.img", "core.img", > - install_drive, force, > - fs_probe, allow_floppy, add_rs_codes, > - !grub_install_is_short_mbrgap_supported ()); > + { > + grub_util_bios_setup (platdir, "boot.img", "core.img", > + install_drive, force, > + fs_probe, allow_floppy, add_rs_codes, > + !grub_install_is_short_mbrgap_supported ()); > + > + grub_install_backup_ponr = 1; > + } > break; > } > case GRUB_INSTALL_PLATFORM_SPARC64_IEEE1275: > @@ -1746,10 +1750,14 @@ main (int argc, char *argv[]) > > /* Now perform the installation. */ > if (install_bootsector) > - grub_util_sparc_setup (platdir, "boot.img", "core.img", > - install_drive, force, > - fs_probe, allow_floppy, > - 0 /* unused */, 0 /* unused */ ); > + { > + grub_util_sparc_setup (platdir, "boot.img", "core.img", > + install_drive, force, > + fs_probe, allow_floppy, > + 0 /* unused */, 0 /* unused */ ); > + > + grub_install_backup_ponr = 1; > + } > break; > } > > @@ -1776,6 +1784,8 @@ main (int argc, char *argv[]) > grub_elf = grub_util_path_concat (2, core_services, "grub.elf"); > grub_install_copy_file (imgfile, grub_elf, 1); > > + grub_install_backup_ponr = 1; > + > f = grub_util_fopen (mach_kernel, "a+"); > if (!f) > grub_util_error (_("Can't create file: %s"), strerror (errno)); > @@ -1878,6 +1888,8 @@ main (int argc, char *argv[]) > boot_efi = grub_util_path_concat (2, core_services, "boot.efi"); > grub_install_copy_file (imgfile, boot_efi, 1); > > + grub_install_backup_ponr = 1; > + > f = grub_util_fopen (mach_kernel, "r+"); > if (!f) > grub_util_error (_("Can't create file: %s"), strerror (errno)); > @@ -1916,6 +1928,9 @@ main (int argc, char *argv[]) > { > char *dst = grub_util_path_concat (2, efidir, efi_file); > grub_install_copy_file (imgfile, dst, 1); > + > + grub_install_backup_ponr = 1; > + > free (dst); > } > if (!removable && update_nvram) > @@ -1967,6 +1982,8 @@ main (int argc, char *argv[]) > break; > } > > + grub_install_backup_ponr = 1; > + Do we really need this one? > fprintf (stderr, "%s\n", _("Installation finished. No error reported.")); > > /* Free resources. */ Daniel _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
