Raymund Will <r...@suse.de> writes: > Robbie Harwood wrote on 2022-08-23T17:15:42 -0400: >> From: Raymund Will <r...@suse.com> > [...] >> By default the systemctl kexec option is used so systemd can shutdown >> all of the running services before doing a reboot using kexec. But if >> this is not present, it fallbacks to executing the kexec user-space >> tool directly. > > The last sentence should probably read more like: > > The provision to force a kexec-reboot, in case systemctl kexec > fails, must only be used in controlled environments to avoid > possible file-system corruption and data-loss.
I can append something to this effect, sure.
> [...]
>> --- /dev/null
>> +++ b/grub-core/loader/emu/linux.c
>> @@ -0,0 +1,180 @@
> [...]
>> +static grub_err_t
>> +grub_linux_boot (void)
>> +{
>> + grub_err_t rc = GRUB_ERR_NONE;
>> + char *initrd_param;
>> + const char *kexec[] = { "kexec", "-la", kernel_path, boot_cmdline, NULL,
>> NULL };
>
> You might have noticed the change in the first parameter to kexec,
> which makes a perfect argument for Daniel's request to have that
> configurable! But implementation would be quite expensive, maybe
> unless it's strictly restricted to non-whitespace- bearing parameters.
> Would that be sufficient and viable?
Well, just because configuration changed doesn't mean it should be
configurable... my understanding is that -a causes KEXEC_FILE_LOAD to be
tried first, and that without -a it isn't tried at all, so I don't see
the use case for not having -a.
I mentioned in my other email that restricting to parameters that don't
contain whiespace breaks things like --append and --comand-line. Maybe
that's okay? I'm just not immediately seeing the use case for it being
configurable, but I could be convinced if someone has one.
>> + const char *systemctl[] = { "systemctl", "kexec", NULL };
>> + int kexecute = grub_util_get_kexecute ();
>> +
>> + if (initrd_path)
>> + {
>> + initrd_param = grub_xasprintf ("--initrd=%s", initrd_path);
>> + kexec[3] = initrd_param;
>> + kexec[4] = boot_cmdline;
>> + }
>> + else
>> + {
>> + initrd_param = grub_xasprintf ("%s", "");
>> + }
>> +
>> + grub_dprintf ("linux", "%serforming 'kexec -la %s %s %s'\n",
>> + (kexecute) ? "P" : "Not p",
>> + kernel_path, initrd_param, boot_cmdline);
>
> Well, the original grub_printf() in this case was very helpful to
> "create" a kexec-load command for cut-n-paste. Is it really necessary
> to bury it in a ton of debug messages?
I'll defer to Daniel on this, but feedback we've received in the past
has requested that all printing go through the debug infrastructure.
>> + if (kexecute)
>> + rc = grub_util_exec (kexec);
>> +
>> + grub_free(initrd_param);
>> +
>> + if (rc != GRUB_ERR_NONE)
>> + {
>> + grub_error (rc, N_("Error trying to perform kexec load operation."));
>> + grub_sleep (3);
>> + return rc;
>> + }
>> +
>> + if (kexecute < 1)
>> + 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");
>> + rc = grub_util_exec (systemctl);
>> +
>> + if (kexecute == 1)
>> + grub_error (rc, N_("Error trying to perform 'systemctl kexec'"));
>
> This grub_error() needs to be a grub_fatal() to achieve the intended
> behavior, right?
>
> If kexecute is 1 it should bail out here. Only if it's bigger the
> forced kexec should be tried! (Note, that "< 1" is already covered
> above!)
Correct, will fix. (Peeking behind the curtain, I'm manually merging
your patch with ours, so this kind of checking is appreciated.)
>> + /* need to check read-only root before resetting hard!? */
>
> This comment probably needs to be replaced with a strict one
> (reflected in GRUB's docs) explaining, that the user takes full
> responsiblity in "force" being exclusively used in read-only
> environments, as grub-emu itself simply can't guarantee this. (Any
> check here would hardly scratch the surface.)
Okay. For what it's worth, the openSUSE patch also has the same
comment.
Be well,
--Robbie
signature.asc
Description: PGP signature
_______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
