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

[Raymund Will <rw@suse.de>]

Robbie Harwood wrote on 2022-08-23T17:15:42 -0400:
> From: Raymund Will <rw@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.

[...]
> --- /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?

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

> +
> +  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!)

> +
> +  /* 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.)

> +  grub_dprintf ("linux", "Performing 'kexec -e -x'");
> +  kexec[1] = "-e";
> +  kexec[2] = "-x";
> +  kexec[3] = NULL;

Provided the kexec-load gets a tunable, this kexec-exec probably deserves
one as well (as this '-ex' initially was only a '-e' (see  ----vv)).

> +  rc = grub_util_exec (kexec);
> +  if ( rc != GRUB_ERR_NONE )
> +    grub_fatal (N_("Error trying to directly perform 'kexec -e'."));
> +
> +  return rc;
> +}
[...]

Thanks Robbie for driving this, as I'm lacking the time...

Best,
-- 
Raymund WILL                                                  rw@SUSE.de
SUSE Software Solutions Germany GmbH, Frankenstr. 146, D-90461 Nuernberg
Geschaeftsfuehrer: Ivo Totev, et al            (HRB 36809, AG Nuernberg)