* [PATCH] grub-install: Add backup and restore @ 2020-08-25 23:38 Dimitri John Ledkov 2020-12-07 12:37 ` [PATCHv2] " Dimitri John Ledkov 0 siblings, 1 reply; 8+ messages in thread From: Dimitri John Ledkov @ 2020-08-25 23:38 UTC (permalink / raw) To: grub-devel; +Cc: juliank, cjwatson, steve.langasek, Dimitri John Ledkov 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 on_exit handle to restore the backup if any errors occur, or remove the backup if everything was successful. If on_exit is not available, the backup remains on disk for manual recovery. 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. Also add modinfo.sh to the cleanup/backup/restore codepath, to ensure it is also cleaned / backed up / restored. Signed-off-by: Dimitri John Ledkov <xnox@ubuntu.com> --- configure.ac | 2 +- util/grub-install-common.c | 105 +++++++++++++++++++++++++++++++------ 2 files changed, 91 insertions(+), 16 deletions(-) diff --git a/configure.ac b/configure.ac index 7c10a4db7..71cd414c3 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 on_exit) 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/util/grub-install-common.c b/util/grub-install-common.c index 0295d40f5..e5f069a13 100644 --- a/util/grub-install-common.c +++ b/util/grub-install-common.c @@ -185,38 +185,113 @@ 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 = 0, + CLEAN_BACKUP = 1, + CREATE_BACKUP = 2, + RESTORE_BACKUP = 3, +}; + 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] = ""; + + if ((mode == CLEAN_BACKUP) || (mode == RESTORE_BACKUP)) + { + strcpy (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, ".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_ext (2, di, de->d_name); + dstf[strlen (dstf) - 1] = 0; + 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_on_exit (int status, void *arg) +{ + if (status == 0) + { + clean_grub_dir_real ((char *) arg, CLEAN_BACKUP); + } + else + { + clean_grub_dir_real ((char *) arg, CLEAN); + clean_grub_dir_real ((char *) arg, RESTORE_BACKUP); + } + free (arg); + arg = NULL; +} + +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_ON_EXIT) + on_exit (restore_backup_on_exit, strdup (di)); +#endif +} + struct install_list { int is_default; -- 2.27.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCHv2] grub-install: Add backup and restore 2020-08-25 23:38 [PATCH] grub-install: Add backup and restore Dimitri John Ledkov @ 2020-12-07 12:37 ` Dimitri John Ledkov 2020-12-07 14:19 ` Colin Watson ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: Dimitri John Ledkov @ 2020-12-07 12:37 UTC (permalink / raw) To: grub-devel; +Cc: Dimitri John Ledkov 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 on_exit handle to restore the backup if any errors occur, or remove the backup if everything was successful. If on_exit is not available, the backup remains on disk for manual recovery. 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. Also add modinfo.sh to the cleanup/backup/restore codepath, to ensure it is also cleaned / backed up / restored. Signed-off-by: Dimitri John Ledkov <xnox@ubuntu.com> --- Changes since v1: as reported on linux-ext4 mailing list and debugged by Colin Watson, the grub_util_path_concat_ext call was incorrect in the restore backup case as there was no extention needed. Instead the call is corrected to just grub_util_path_concat. This patch is now shipped in Ubuntu & Debian in multiple series. It would be nice to have this merged upstream, as it greatly improves grub upgrades and prevents missmatches of MBR & modules. configure.ac | 2 +- util/grub-install-common.c | 105 +++++++++++++++++++++++++++++++------ 2 files changed, 91 insertions(+), 16 deletions(-) diff --git a/configure.ac b/configure.ac index 7c10a4db7..71cd414c3 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 on_exit) 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/util/grub-install-common.c b/util/grub-install-common.c index 277eaf4e2..74584b92b 100644 --- a/util/grub-install-common.c +++ b/util/grub-install-common.c @@ -185,38 +185,113 @@ 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 = 0, + CLEAN_BACKUP = 1, + CREATE_BACKUP = 2, + RESTORE_BACKUP = 3, +}; + 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] = ""; + + if ((mode == CLEAN_BACKUP) || (mode == RESTORE_BACKUP)) + { + strcpy (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, ".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; + 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_on_exit (int status, void *arg) +{ + if (status == 0) + { + clean_grub_dir_real ((char *) arg, CLEAN_BACKUP); + } + else + { + clean_grub_dir_real ((char *) arg, CLEAN); + clean_grub_dir_real ((char *) arg, RESTORE_BACKUP); + } + free (arg); + arg = NULL; +} + +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_ON_EXIT) + on_exit (restore_backup_on_exit, strdup (di)); +#endif +} + struct install_list { int is_default; -- 2.27.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCHv2] grub-install: Add backup and restore 2020-12-07 12:37 ` [PATCHv2] " Dimitri John Ledkov @ 2020-12-07 14:19 ` Colin Watson 2020-12-08 3:17 ` Michael Chang 2021-04-21 14:34 ` Daniel Kiper 2 siblings, 0 replies; 8+ messages in thread From: Colin Watson @ 2020-12-07 14:19 UTC (permalink / raw) To: grub-devel On Mon, Dec 07, 2020 at 12:37:28PM +0000, 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 on_exit handle to restore the backup if any errors occur, or > remove the backup if everything was successful. If on_exit is not > available, the backup remains on disk for manual recovery. Reviewed-by: Colin Watson <cjwatson@debian.org> -- Colin Watson (he/him) [cjwatson@debian.org] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCHv2] grub-install: Add backup and restore 2020-12-07 12:37 ` [PATCHv2] " Dimitri John Ledkov 2020-12-07 14:19 ` Colin Watson @ 2020-12-08 3:17 ` Michael Chang 2020-12-08 5:58 ` Dimitri John Ledkov 2021-04-21 14:34 ` Daniel Kiper 2 siblings, 1 reply; 8+ messages in thread From: Michael Chang @ 2020-12-08 3:17 UTC (permalink / raw) To: The development of GNU GRUB; +Cc: Dimitri John Ledkov On Mon, Dec 07, 2020 at 12:37:28PM +0000, 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 on_exit handle to restore the backup if any errors occur, or > remove the backup if everything was successful. If on_exit is not > available, the backup remains on disk for manual recovery. > > 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'd love to have the described problem fixed too, as it helps a lot in the update path to survive grub-install error which can be contributed by many different reasons, even though grub has to stay with old version but still much better than unbootable system. But here I have a concern, as to what will happen if the error comes after image installation ? For example, the efibootmgr may fail to register boot entry after copying the images to efi partition. It looks to me that the restoring backup would be triggerd incorrectly for the new image to load restored old backups. Would you please help to clarify that ? Thanks, Michael > > Also add modinfo.sh to the cleanup/backup/restore codepath, to ensure > it is also cleaned / backed up / restored. > > Signed-off-by: Dimitri John Ledkov <xnox@ubuntu.com> > --- > > Changes since v1: as reported on linux-ext4 mailing list and debugged > by Colin Watson, the grub_util_path_concat_ext call was incorrect in > the restore backup case as there was no extention needed. Instead the > call is corrected to just grub_util_path_concat. > > This patch is now shipped in Ubuntu & Debian in multiple series. It > would be nice to have this merged upstream, as it greatly improves > grub upgrades and prevents missmatches of MBR & modules. > > configure.ac | 2 +- > util/grub-install-common.c | 105 +++++++++++++++++++++++++++++++------ > 2 files changed, 91 insertions(+), 16 deletions(-) > > diff --git a/configure.ac b/configure.ac > index 7c10a4db7..71cd414c3 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 on_exit) > 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/util/grub-install-common.c b/util/grub-install-common.c > index 277eaf4e2..74584b92b 100644 > --- a/util/grub-install-common.c > +++ b/util/grub-install-common.c > @@ -185,38 +185,113 @@ 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 = 0, > + CLEAN_BACKUP = 1, > + CREATE_BACKUP = 2, > + RESTORE_BACKUP = 3, > +}; > + > 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] = ""; > + > + if ((mode == CLEAN_BACKUP) || (mode == RESTORE_BACKUP)) > + { > + strcpy (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, ".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; > + 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_on_exit (int status, void *arg) > +{ > + if (status == 0) > + { > + clean_grub_dir_real ((char *) arg, CLEAN_BACKUP); > + } > + else > + { > + clean_grub_dir_real ((char *) arg, CLEAN); > + clean_grub_dir_real ((char *) arg, RESTORE_BACKUP); > + } > + free (arg); > + arg = NULL; > +} > + > +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_ON_EXIT) > + on_exit (restore_backup_on_exit, strdup (di)); > +#endif > +} > + > struct install_list > { > int is_default; > -- > 2.27.0 > > > _______________________________________________ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCHv2] grub-install: Add backup and restore 2020-12-08 3:17 ` Michael Chang @ 2020-12-08 5:58 ` Dimitri John Ledkov 2020-12-09 5:15 ` Michael Chang 0 siblings, 1 reply; 8+ messages in thread From: Dimitri John Ledkov @ 2020-12-08 5:58 UTC (permalink / raw) To: Michael Chang; +Cc: The development of GNU GRUB [-- Attachment #1: Type: text/plain, Size: 9324 bytes --] On Tue, 8 Dec 2020, 03:17 Michael Chang, <mchang@suse.com> wrote: > On Mon, Dec 07, 2020 at 12:37:28PM +0000, 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 on_exit handle to restore the backup if any errors occur, or > > remove the backup if everything was successful. If on_exit is not > > available, the backup remains on disk for manual recovery. > > > > 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'd love to have the described problem fixed too, as it helps a lot in > the update path to survive grub-install error which can be contributed > by many different reasons, even though grub has to stay with old version > but still much better than unbootable system. > > But here I have a concern, as to what will happen if the error comes > after image installation ? For example, the efibootmgr may fail to > register boot entry after copying the images to efi partition. It looks > to me that the restoring backup would be triggerd incorrectly for the > new image to load restored old backups. > > Would you please help to clarify that ? > On Ubuntu we install secureboot signed prebuilt EFI images only which prohibit module loading. Hence usually it is irrelevant if the EFI grub modules are old or new. And we do not call efibootmgr at all, as it does excessive writes to efivars. Instead we don't even try to update efivars if there is no need. It was submitted to this mailing list but I am not sure on the acceptance status https://lists.gnu.org/archive/html/grub-devel/2019-03/msg00123.html Above two things make EFI updates less fatal, as it is harder for a prebuilt signed grub, which does not use modules, to fail booting. Unlike i386-pc case. And to answer your immediate question - the new EFI image will be used for boot without rolling back. Also, I am not sure how everyone installs signed grubs. Thus adding support for ESP backup and restore might be futile upstream. As far as I can tell it is safe to call grub-install on Ubuntu, whereas it might not be on other distros. As Ubuntu preserves / reinstalls signed grub EFI images, instead of generating a new random one and putting it on ESP thus causing failure to boot with secureboot due to effectively replacing signed grub with an unsigned one. And for resilient boot we have support for maintaining and synchronising multiple ESP for Linux raid. I kind of wish we'd prebuilt more core images at package build time and simply copy them around. Not just the ones that have signing. Instead of invoking mkimage on every installed system. Nonetheless, it should be possible to hook up more files and directories to perform backup, cleanup and restore on. Is there is a desire. The function calls simply need to be added in the appropriate places. > > > > Also add modinfo.sh to the cleanup/backup/restore codepath, to ensure > > it is also cleaned / backed up / restored. > > > > Signed-off-by: Dimitri John Ledkov <xnox@ubuntu.com> > > --- > > > > Changes since v1: as reported on linux-ext4 mailing list and debugged > > by Colin Watson, the grub_util_path_concat_ext call was incorrect in > > the restore backup case as there was no extention needed. Instead the > > call is corrected to just grub_util_path_concat. > > > > This patch is now shipped in Ubuntu & Debian in multiple series. It > > would be nice to have this merged upstream, as it greatly improves > > grub upgrades and prevents missmatches of MBR & modules. > > > > configure.ac | 2 +- > > util/grub-install-common.c | 105 +++++++++++++++++++++++++++++++------ > > 2 files changed, 91 insertions(+), 16 deletions(-) > > > > diff --git a/configure.ac b/configure.ac > > index 7c10a4db7..71cd414c3 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 on_exit) > > 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/util/grub-install-common.c b/util/grub-install-common.c > > index 277eaf4e2..74584b92b 100644 > > --- a/util/grub-install-common.c > > +++ b/util/grub-install-common.c > > @@ -185,38 +185,113 @@ 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 = 0, > > + CLEAN_BACKUP = 1, > > + CREATE_BACKUP = 2, > > + RESTORE_BACKUP = 3, > > +}; > > + > > 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] = ""; > > + > > + if ((mode == CLEAN_BACKUP) || (mode == RESTORE_BACKUP)) > > + { > > + strcpy (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, ".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; > > + 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_on_exit (int status, void *arg) > > +{ > > + if (status == 0) > > + { > > + clean_grub_dir_real ((char *) arg, CLEAN_BACKUP); > > + } > > + else > > + { > > + clean_grub_dir_real ((char *) arg, CLEAN); > > + clean_grub_dir_real ((char *) arg, RESTORE_BACKUP); > > + } > > + free (arg); > > + arg = NULL; > > +} > > + > > +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_ON_EXIT) > > + on_exit (restore_backup_on_exit, strdup (di)); > > +#endif > > +} > > + > > struct install_list > > { > > int is_default; > > -- > > 2.27.0 > > > > > > _______________________________________________ > > Grub-devel mailing list > > Grub-devel@gnu.org > > https://lists.gnu.org/mailman/listinfo/grub-devel > > [-- Attachment #2: Type: text/html, Size: 12847 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCHv2] grub-install: Add backup and restore 2020-12-08 5:58 ` Dimitri John Ledkov @ 2020-12-09 5:15 ` Michael Chang 2020-12-09 10:39 ` Dimitri John Ledkov 0 siblings, 1 reply; 8+ messages in thread From: Michael Chang @ 2020-12-09 5:15 UTC (permalink / raw) To: Dimitri John Ledkov; +Cc: The development of GNU GRUB On Tue, Dec 08, 2020 at 05:58:40AM +0000, Dimitri John Ledkov wrote: > On Tue, 8 Dec 2020, 03:17 Michael Chang, <mchang@suse.com> wrote: > > > On Mon, Dec 07, 2020 at 12:37:28PM +0000, 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 on_exit handle to restore the backup if any errors occur, or > > > remove the backup if everything was successful. If on_exit is not > > > available, the backup remains on disk for manual recovery. > > > > > > 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'd love to have the described problem fixed too, as it helps a lot in > > the update path to survive grub-install error which can be contributed > > by many different reasons, even though grub has to stay with old version > > but still much better than unbootable system. > > > > But here I have a concern, as to what will happen if the error comes > > after image installation ? For example, the efibootmgr may fail to > > register boot entry after copying the images to efi partition. It looks > > to me that the restoring backup would be triggerd incorrectly for the > > new image to load restored old backups. > > > > Would you please help to clarify that ? > > > > > On Ubuntu we install secureboot signed prebuilt EFI images only which > prohibit module loading. Hence usually it is irrelevant if the EFI grub > modules are old or new. > > And we do not call efibootmgr at all, as it does excessive writes to > efivars. Instead we don't even try to update efivars if there is no need. > It was submitted to this mailing list but I am not sure on the acceptance > status https://lists.gnu.org/archive/html/grub-devel/2019-03/msg00123.html > > Above two things make EFI updates less fatal, as it is harder for a > prebuilt signed grub, which does not use modules, to fail booting. Unlike > i386-pc case. My concern is exactly that these are not upstreamed yet so accepting the patch would be premature. Either it has to be adapted to satisfy the expected behavior of existing codebase or wait until those dependent patches gets merged. Besides, other architecture like openfirmware would update nvram in a similar fashion to uefi but cannot be covered by the aforementioned efivars enhacement patch. > > And to answer your immediate question - the new EFI image will be used for > boot without rolling back. > > Also, I am not sure how everyone installs signed grubs. Thus adding support > for ESP backup and restore might be futile upstream. As far as I can tell > it is safe to call grub-install on Ubuntu, whereas it might not be on other > distros. As Ubuntu preserves / reinstalls signed grub EFI images, instead > of generating a new random one and putting it on ESP thus causing failure > to boot with secureboot due to effectively replacing signed grub with an > unsigned one. And for resilient boot we have support for maintaining and > synchronising multiple ESP for Linux raid. Installing signed grub can happen in parallel with any unsigned one installed by grub-install as long as they didn't overlap in the position on EFI System partition. For that matters the efibootmgr is used to manage them. The signed grub is so far not controlled by grub-install, so should be disregared at this moment. The entire backup/recovery should be done only for those made aware by grub-install. > > I kind of wish we'd prebuilt more core images at package build time and > simply copy them around. Not just the ones that have signing. Instead of > invoking mkimage on every installed system. > > Nonetheless, it should be possible to hook up more files and directories to > perform backup, cleanup and restore on. Is there is a desire. The function > calls simply need to be added in the appropriate places. IMHO the hook should be unregistered right after the state is considered complete for new image and modules, or it would be triggered inadvertently if any error comes after that and hence render the system incompete again by restoring old backup. Thanks, Michael > > > > > > > > Also add modinfo.sh to the cleanup/backup/restore codepath, to ensure > > > it is also cleaned / backed up / restored. > > > > > > Signed-off-by: Dimitri John Ledkov <xnox@ubuntu.com> > > > --- > > > > > > Changes since v1: as reported on linux-ext4 mailing list and debugged > > > by Colin Watson, the grub_util_path_concat_ext call was incorrect in > > > the restore backup case as there was no extention needed. Instead the > > > call is corrected to just grub_util_path_concat. > > > > > > This patch is now shipped in Ubuntu & Debian in multiple series. It > > > would be nice to have this merged upstream, as it greatly improves > > > grub upgrades and prevents missmatches of MBR & modules. > > > > > > configure.ac | 2 +- > > > util/grub-install-common.c | 105 +++++++++++++++++++++++++++++++------ > > > 2 files changed, 91 insertions(+), 16 deletions(-) > > > > > > diff --git a/configure.ac b/configure.ac > > > index 7c10a4db7..71cd414c3 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 on_exit) > > > 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/util/grub-install-common.c b/util/grub-install-common.c > > > index 277eaf4e2..74584b92b 100644 > > > --- a/util/grub-install-common.c > > > +++ b/util/grub-install-common.c > > > @@ -185,38 +185,113 @@ 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 = 0, > > > + CLEAN_BACKUP = 1, > > > + CREATE_BACKUP = 2, > > > + RESTORE_BACKUP = 3, > > > +}; > > > + > > > 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] = ""; > > > + > > > + if ((mode == CLEAN_BACKUP) || (mode == RESTORE_BACKUP)) > > > + { > > > + strcpy (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, ".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; > > > + 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_on_exit (int status, void *arg) > > > +{ > > > + if (status == 0) > > > + { > > > + clean_grub_dir_real ((char *) arg, CLEAN_BACKUP); > > > + } > > > + else > > > + { > > > + clean_grub_dir_real ((char *) arg, CLEAN); > > > + clean_grub_dir_real ((char *) arg, RESTORE_BACKUP); > > > + } > > > + free (arg); > > > + arg = NULL; > > > +} > > > + > > > +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_ON_EXIT) > > > + on_exit (restore_backup_on_exit, strdup (di)); > > > +#endif > > > +} > > > + > > > struct install_list > > > { > > > int is_default; > > > -- > > > 2.27.0 > > > > > > > > > _______________________________________________ > > > Grub-devel mailing list > > > Grub-devel@gnu.org > > > https://lists.gnu.org/mailman/listinfo/grub-devel > > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCHv2] grub-install: Add backup and restore 2020-12-09 5:15 ` Michael Chang @ 2020-12-09 10:39 ` Dimitri John Ledkov 0 siblings, 0 replies; 8+ messages in thread From: Dimitri John Ledkov @ 2020-12-09 10:39 UTC (permalink / raw) To: Michael Chang; +Cc: The development of GNU GRUB Hi, On Wed, 9 Dec 2020 at 05:15, Michael Chang <mchang@suse.com> wrote: > > On Tue, Dec 08, 2020 at 05:58:40AM +0000, Dimitri John Ledkov wrote: > > On Tue, 8 Dec 2020, 03:17 Michael Chang, <mchang@suse.com> wrote: > > > > > On Mon, Dec 07, 2020 at 12:37:28PM +0000, 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 on_exit handle to restore the backup if any errors occur, or > > > > remove the backup if everything was successful. If on_exit is not > > > > available, the backup remains on disk for manual recovery. > > > > > > > > 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'd love to have the described problem fixed too, as it helps a lot in > > > the update path to survive grub-install error which can be contributed > > > by many different reasons, even though grub has to stay with old version > > > but still much better than unbootable system. > > > > > > But here I have a concern, as to what will happen if the error comes > > > after image installation ? For example, the efibootmgr may fail to > > > register boot entry after copying the images to efi partition. It looks > > > to me that the restoring backup would be triggerd incorrectly for the > > > new image to load restored old backups. > > > > > > Would you please help to clarify that ? > > > > > > > > > On Ubuntu we install secureboot signed prebuilt EFI images only which > > prohibit module loading. Hence usually it is irrelevant if the EFI grub > > modules are old or new. > > > > And we do not call efibootmgr at all, as it does excessive writes to > > efivars. Instead we don't even try to update efivars if there is no need. > > It was submitted to this mailing list but I am not sure on the acceptance > > status https://lists.gnu.org/archive/html/grub-devel/2019-03/msg00123.html > > > > Above two things make EFI updates less fatal, as it is harder for a > > prebuilt signed grub, which does not use modules, to fail booting. Unlike > > i386-pc case. > > My concern is exactly that these are not upstreamed yet so accepting the > patch would be premature. Either it has to be adapted to satisfy the > expected behavior of existing codebase or wait until those dependent > patches gets merged. > this patch does not depend on efivars patch behaviour, and is useful with either efibootmgr or efivars codepath. If either way to update EFI vars fails, rollback is performed. > Besides, other architecture like openfirmware would update nvram in a > similar fashion to uefi but cannot be covered by the aforementioned > efivars enhacement patch. > If nvram update of openfirmware fails, modules which are copied over first must too be rolled backed. In case of failures there is no way to know for sure which state to take, but it is safer to revert. > > > > And to answer your immediate question - the new EFI image will be used for > > boot without rolling back. > > > > Also, I am not sure how everyone installs signed grubs. Thus adding support > > for ESP backup and restore might be futile upstream. As far as I can tell > > it is safe to call grub-install on Ubuntu, whereas it might not be on other > > distros. As Ubuntu preserves / reinstalls signed grub EFI images, instead > > of generating a new random one and putting it on ESP thus causing failure > > to boot with secureboot due to effectively replacing signed grub with an > > unsigned one. And for resilient boot we have support for maintaining and > > synchronising multiple ESP for Linux raid. > > Installing signed grub can happen in parallel with any unsigned one > installed by grub-install as long as they didn't overlap in the position > on EFI System partition. For that matters the efibootmgr is used to > manage them. > > The signed grub is so far not controlled by grub-install, so should be > disregared at this moment. The entire backup/recovery should be done > only for those made aware by grub-install. > This patch so far only manages things that are done grub-install, precisely as you desire. > > > > I kind of wish we'd prebuilt more core images at package build time and > > simply copy them around. Not just the ones that have signing. Instead of > > invoking mkimage on every installed system. > > > > Nonetheless, it should be possible to hook up more files and directories to > > perform backup, cleanup and restore on. Is there is a desire. The function > > calls simply need to be added in the appropriate places. > > IMHO the hook should be unregistered right after the state is considered > complete for new image and modules, or it would be triggered > inadvertently if any error comes after that and hence render the system > incompete again by restoring old backup. > At the moment grub-install roughly updates /boot/grub things first, then writes out things to MBR, nvram, then completes things like efibootmgr. It is explicit that any failure of any step is treated as an overall failure. Even things like removing, and re-adding identical efibootmgr variable entry. Which was explicitly changed to ensure that efibootmgr are treated as failure of grub-install. We must never ignore failures. And at every stage that it fails it leaves things as they are. For fresh installs it doesn't quite matter, but for upgrades it leaves things in an arbitrary inconsistent state. Typically with modules updated first and aborting upon failure to update MBR/nvram and the like. If everything is successful, this patch does not change the end state after grub-install completes. In case of errors, with this patch, an attempt is made to ensure that state is reverted back to what it was before the unsuccessful attempt was started. As much as possible. In the same way one would rollback an SQL transaction. Overall this is an improvement, and whilst not perfect/complete it is already useful at preventing unbootable machines especially when APIs between core & modules have changed. > Thanks, > Michael > > > > > > > > > > > > > Also add modinfo.sh to the cleanup/backup/restore codepath, to ensure > > > > it is also cleaned / backed up / restored. > > > > > > > > Signed-off-by: Dimitri John Ledkov <xnox@ubuntu.com> > > > > --- > > > > > > > > Changes since v1: as reported on linux-ext4 mailing list and debugged > > > > by Colin Watson, the grub_util_path_concat_ext call was incorrect in > > > > the restore backup case as there was no extention needed. Instead the > > > > call is corrected to just grub_util_path_concat. > > > > > > > > This patch is now shipped in Ubuntu & Debian in multiple series. It > > > > would be nice to have this merged upstream, as it greatly improves > > > > grub upgrades and prevents missmatches of MBR & modules. > > > > > > > > configure.ac | 2 +- > > > > util/grub-install-common.c | 105 +++++++++++++++++++++++++++++++------ > > > > 2 files changed, 91 insertions(+), 16 deletions(-) > > > > > > > > diff --git a/configure.ac b/configure.ac > > > > index 7c10a4db7..71cd414c3 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 on_exit) > > > > 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/util/grub-install-common.c b/util/grub-install-common.c > > > > index 277eaf4e2..74584b92b 100644 > > > > --- a/util/grub-install-common.c > > > > +++ b/util/grub-install-common.c > > > > @@ -185,38 +185,113 @@ 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 = 0, > > > > + CLEAN_BACKUP = 1, > > > > + CREATE_BACKUP = 2, > > > > + RESTORE_BACKUP = 3, > > > > +}; > > > > + > > > > 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] = ""; > > > > + > > > > + if ((mode == CLEAN_BACKUP) || (mode == RESTORE_BACKUP)) > > > > + { > > > > + strcpy (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, ".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; > > > > + 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_on_exit (int status, void *arg) > > > > +{ > > > > + if (status == 0) > > > > + { > > > > + clean_grub_dir_real ((char *) arg, CLEAN_BACKUP); > > > > + } > > > > + else > > > > + { > > > > + clean_grub_dir_real ((char *) arg, CLEAN); > > > > + clean_grub_dir_real ((char *) arg, RESTORE_BACKUP); > > > > + } > > > > + free (arg); > > > > + arg = NULL; > > > > +} > > > > + > > > > +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_ON_EXIT) > > > > + on_exit (restore_backup_on_exit, strdup (di)); > > > > +#endif > > > > +} > > > > + > > > > struct install_list > > > > { > > > > int is_default; > > > > -- > > > > 2.27.0 > > > > > > > > > > > > _______________________________________________ > > > > Grub-devel mailing list > > > > Grub-devel@gnu.org > > > > https://lists.gnu.org/mailman/listinfo/grub-devel > > > > > > > -- Regards, Dimitri. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCHv2] grub-install: Add backup and restore 2020-12-07 12:37 ` [PATCHv2] " Dimitri John Ledkov 2020-12-07 14:19 ` Colin Watson 2020-12-08 3:17 ` Michael Chang @ 2021-04-21 14:34 ` Daniel Kiper 2 siblings, 0 replies; 8+ messages in thread From: Daniel Kiper @ 2021-04-21 14:34 UTC (permalink / raw) To: Dimitri John Ledkov; +Cc: grub-devel Hi, On Michael request I am considering this as 2.06 release material... On Mon, Dec 07, 2020 at 12:37:28PM +0000, 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 on_exit handle to restore the backup if any errors occur, or > remove the backup if everything was successful. If on_exit is not > available, the backup remains on disk for manual recovery. > > 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. > > Also add modinfo.sh to the cleanup/backup/restore codepath, to ensure > it is also cleaned / backed up / restored. > > Signed-off-by: Dimitri John Ledkov <xnox@ubuntu.com> > --- > > Changes since v1: as reported on linux-ext4 mailing list and debugged > by Colin Watson, the grub_util_path_concat_ext call was incorrect in > the restore backup case as there was no extention needed. Instead the > call is corrected to just grub_util_path_concat. > > This patch is now shipped in Ubuntu & Debian in multiple series. It > would be nice to have this merged upstream, as it greatly improves > grub upgrades and prevents missmatches of MBR & modules. > > configure.ac | 2 +- > util/grub-install-common.c | 105 +++++++++++++++++++++++++++++++------ > 2 files changed, 91 insertions(+), 16 deletions(-) > > diff --git a/configure.ac b/configure.ac > index 7c10a4db7..71cd414c3 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 on_exit) I think we should use atexit() instead of on_exit(). The mans say former is more portable... > 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/util/grub-install-common.c b/util/grub-install-common.c > index 277eaf4e2..74584b92b 100644 > --- a/util/grub-install-common.c > +++ b/util/grub-install-common.c > @@ -185,38 +185,113 @@ 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); Please add empty line here. > + free (bsuffix); > + return r; > +} > + > +enum clean_grub_dir_mode > +{ > + CLEAN = 0, > + CLEAN_BACKUP = 1, > + CREATE_BACKUP = 2, > + RESTORE_BACKUP = 3, Do we need these assignments? I do not think so... > +}; > + > 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] = ""; > + > + if ((mode == CLEAN_BACKUP) || (mode == RESTORE_BACKUP)) > + { > + strcpy (suffix, "-"); It seems to me "~" is more common as backup marker. I would use it instead of "-"... > + } Please drop these curly brackets... > 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, '.'); Please add empty line here... > - 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, ".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, "-"); s/"-"/"~"/? ...and please add empty line after variable definitions... > + 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); Ditto... > + dstf[strlen (dstf) - 1] = 0; > + 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_on_exit (int status, void *arg) > +{ > + if (status == 0) > + { > + clean_grub_dir_real ((char *) arg, CLEAN_BACKUP); Do we need this (char *) cast? > + } Please drop this curly brackets. > + else > + { > + clean_grub_dir_real ((char *) arg, CLEAN); > + clean_grub_dir_real ((char *) arg, RESTORE_BACKUP); > + } > + free (arg); > + arg = NULL; I think you can drop this NULL assignment. Daniel ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-04-21 14:34 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-08-25 23:38 [PATCH] grub-install: Add backup and restore Dimitri John Ledkov 2020-12-07 12:37 ` [PATCHv2] " Dimitri John Ledkov 2020-12-07 14:19 ` Colin Watson 2020-12-08 3:17 ` Michael Chang 2020-12-08 5:58 ` Dimitri John Ledkov 2020-12-09 5:15 ` Michael Chang 2020-12-09 10:39 ` Dimitri John Ledkov 2021-04-21 14:34 ` Daniel Kiper
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.