On Mon, Aug 08, 2022 at 02:58:06PM -0400, Robbie Harwood wrote:
> Daniel Kiper <dki...@net-space.pl> writes:
>
> > On Tue, Jul 19, 2022 at 04:39:34PM -0400, Robbie Harwood wrote:
> >
> >> +static grub_err_t
> >> +grub_linux_boot (void)
> >> +{
> >> + grub_err_t rc = GRUB_ERR_NONE;
> >> + char *initrd_param;
> >> + const char *kexec[] = { "kexec", "-l", kernel_path, boot_cmdline, NULL,
> >> NULL };
> >> + const char *systemctl[] = { "systemctl", "kexec", NULL };
> >
> > I would prefer if we do not hardcode these commands. E.g. kexec
> > command has many options which can be useful for debugging. If we
> > hardcode the command here we cannot use these options.
>
> Can you clarify what you would like to see instead? I'm not sure what
> the alternative would be.
E.g. grub.cfg could contain:
set kexec_load_cmd=/sbin/kexec
set kexec_load_opts='-l'
Then GRUB should replace "linux" command with combined kexec_load_cmd,
kexec_load_opts, boot_cmdline, etc. Of course we can assume some reasonable
defaults if kexec_load_cmd and/or kexec_load_opts variables are not present.
> >> + rc = grub_util_exec (systemctl);
> >> +
> >> + if (rc == GRUB_ERR_NONE)
> >> + return rc;
> >> +
> >> + grub_error (rc, N_("Error trying to perform 'systemctl kexec'"));
> >> +
> >> + /* need to check read-only root before resetting hard!? */
> >> + grub_dprintf ("linux", "Performing 'kexec -e -x'");
> >
> > I would really do not fall back to 'kexec -e' by default. It is too
> > dangerous. And again I would not hardcode this command too.
>
> Same question as above regarding the alternative... also, can you
set kexec_exec_cmd=/bin/systemctl
set kexec_exec_opts=kexec
set kexec_exec_force_cmd=/sbin/kexec
set kexec_exec_force_opts='-e -x'
Etc...
Maybe names are not perfect but I hope you know what I mean...
> elaborate on the danger you see here?
The "kexec -e" executes the new kernel immediately without unmounting/remounting
filesystems, etc. So, ... I think this feature should be disabled by default.
If somebody knows what is doing then they should be able to enable it, e.g.,
by setting a GRUB env variable.
> >> + grub_fatal (N_("Use '"PACKAGE"-emu --kexec' to force a system
> >> restart."));
> >> +
> >> + grub_dprintf ("linux", "Performing 'systemctl kexec' (%s) ",
> >> + (kexecute==1) ? "do-or-die" : "just-in-case");
> >
> > s/kexecute==1/kexecute/
> >
> > Please be more consistent how you check kexecute.
>
> None of this is my code yet - I just rebased the existing patch - but
> I will make these and other requested changes :) Thanks for the review;
> I'll cut another version once we resolve the conversations above.
Cool! Thank you!
Please do not forget about docs... :-)
Daniel
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
