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

[Mate Kukri]

I believe this to be accidental, as the patch was developed in two
steps with one hunk added later. I believe it shouldn't make a
difference in functionality, but it is unnecessary and I will send
patch v4 tomorrow with the hunks in the original order.

On Thu, Nov 23, 2023 at 6:36 PM Daniel Kiper <dkiper@net-space.pl> wrote:
>
> 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