Re: [PATCH v3] grub-install: Delay copying files to {grubdir, platdir} after install_device was validated.

[Daniel Kiper]

On Wed, Nov 22, 2023 at 11:53:13AM +0000, Mate Kukri wrote:
> Previously grub-install copied modules to grubdir before doing any validation 
> on
> the install_device.
>
> When grub-install was called with an invalid install_device, modules
> were already copied to /boot before it found out and was forced to rely
> on atexit rollback.
>
> This patch delays copying the modules after at least some install_device
> validation was done, and thus reduces reliance on successful rollback.
>
> Signed-off-by: Mate Kukri <mate.kukri@canonical.com>
> Reviewed-by: Julian Andres Klode <julian.klode@canonical.com>
> ---
>  util/grub-install.c | 38 ++++++++++++++++++++------------------
>  1 file changed, 20 insertions(+), 18 deletions(-)
>
> diff --git a/util/grub-install.c b/util/grub-install.c
> index 1ad04db36..8838fb677 100644
> --- a/util/grub-install.c
> +++ b/util/grub-install.c
> @@ -1291,13 +1291,6 @@ main (int argc, char *argv[])
>       }
>      }
>
> -  grub_install_copy_files (grub_install_source_directory,
> -                        grubdir, platform);
> -
> -  char *envfile = grub_util_path_concat (2, grubdir, "grubenv");
> -  if (!grub_util_is_regular (envfile))
> -    grub_util_create_envblk_file (envfile);
> -
>    size_t ndev = 0;
>
>    /* Write device to a variable so we don't have to traverse /dev every 
> time.  */
> @@ -1384,18 +1377,7 @@ main (int argc, char *argv[])
>                      t);
>      free (t);
>    }
> -  load_cfg = grub_util_path_concat (2, platdir,
> -                               "load.cfg");
> -
> -  grub_util_unlink (load_cfg);
>
> -  if (debug_image && debug_image[0])
> -    {
> -      load_cfg_f = grub_util_fopen (load_cfg, "wb");
> -      have_load_cfg = 1;
> -      fprintf (load_cfg_f, "set debug='%s'\n",
> -           debug_image);
> -    }
>    char *prefix_drive = NULL;
>    char *install_drive = NULL;
>
> @@ -1419,6 +1401,26 @@ main (int argc, char *argv[])
>       }
>      }
>
> +  load_cfg = grub_util_path_concat (2, platdir,
> +                                  "load.cfg");
> +
> +  grub_util_unlink (load_cfg);
> +
> +  if (debug_image && debug_image[0])
> +    {
> +      load_cfg_f = grub_util_fopen (load_cfg, "wb");
> +      have_load_cfg = 1;
> +      fprintf (load_cfg_f, "set debug='%s'\n",
> +              debug_image);
> +    }
> +
> +  grub_install_copy_files (grub_install_source_directory,
> +                           grubdir, platform);
> +
> +  char *envfile = grub_util_path_concat (2, grubdir, "grubenv");
> +  if (!grub_util_is_regular (envfile))
> +    grub_util_create_envblk_file (envfile);
> +

I think changed order of hunks in this patch requires at least short
explanation in the commit message.

Daniel