[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] grub-install: Add backup and restore
From: |
Daniel Kiper |
Subject: |
Re: [PATCH] grub-install: Add backup and restore |
Date: |
Tue, 4 May 2021 19:39:57 +0200 |
User-agent: |
NeoMutt/20170113 (1.7.2) |
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 <xnox@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