Re: [PATCH v6] grub-install: Add backup and restore

[Daniel Kiper]

On Tue, Jun 01, 2021 at 11:35:36AM +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 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 <xnox@ubuntu.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com> except...

[...]

> +#ifdef HAVE_ATEXIT
> +static size_t backup_dirs_size = 0;
> +static char **backup_dirs = NULL;
> +static pid_t backup_process = 0;
> +static int grub_install_backup_ponr = 0;
> +
> +void grub_set_install_backup_ponr(void)
> +{
> +  grub_install_backup_ponr = 1;
> +}
> +#else
> +void grub_set_install_backup_ponr(void)
> +{
> +}

This stub should be defined in header file as I proposed. Then compiler
is able to optimize it out. I will fix this before committing...

Thanks for preparing updated patch,

Daniel