Re: [PATCH v2 1/1] Add support for grub-emu to kexec Linux menu entries

[Robbie Harwood <rharwood@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1730 bytes --]

Daniel Kiper <dkiper@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.

>> +  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
elaborate on the danger you see here?

>> +    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.

Be well,
--Robbie

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]