Re: [PATCH v2] grub-install: Delay copying files to grubdir after install_device was validated.

From: Julian Andres Klode <julian.klode@canonical.com>
To: The development of GNU GRUB <grub-devel@gnu.org>
Cc: Mate Kukri <mate.kukri@canonical.com>
Subject: Re: [PATCH v2] grub-install: Delay copying files to grubdir after install_device was validated.
Date: Wed, 22 Nov 2023 11:33:02 +0100	[thread overview]
Message-ID: <20231122103302.jyian6vi4kqlbpvg@jak-t14-g3> (raw)
In-Reply-To: <20231122100627.26688-1-mate.kukri@canonical.com>

On Wed, Nov 22, 2023 at 10:06:27AM +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.
> ---
>  util/grub-install.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/util/grub-install.c b/util/grub-install.c
> index 1ad04db36..fd799a75f 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.  */
> @@ -1419,6 +1412,13 @@ 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);
> +
>    if (!have_abstractions)
>      {
>        if ((disk_module && grub_strcmp (disk_module, "biosdisk") != 0)
> -- 
> 2.39.2

This looks reasonable to me, there is some more code there writing to
the grub dir before the disk has been identified:

grub_util_unlink (load_cfg);

and following lines; it may make sense to move them later too, I think
it would overall make more sense to have them where we generate an empty
grubenv.

But aside from that happy to add my

Reviewed-by: Julian Andres Klode <julian.klode@canonical.com>

-- 
debian developer - deb.li/jak | jak-linux.org - free software dev
ubuntu core developer                              i speak de, en

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel