Re: [PATCHv5] grub-install: Add backup and restore

[Daniel Kiper]

On Mon, May 24, 2021 at 10:59:34AM +0100, Dimitri John Ledkov wrote:
> From: Dimitri John Ledkov <xnox@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 <xnox@ubuntu.com>

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

But...

> +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);

s/strdup/xstrdup/

I will fix it before committing...

> +  backup_dirs_size++;
> +  if (!backup_process)
> +    {
> +      atexit (restore_backup_atexit);
> +      backup_process = getpid ();
> +    }
> +}

Thank you for doing the work!

Daniel