[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 1/1] Add support for grub-emu to kexec Linux menu entries
From: |
Raymund Will |
Subject: |
Re: [PATCH v3 1/1] Add support for grub-emu to kexec Linux menu entries |
Date: |
Wed, 24 Aug 2022 12:35:22 +0200 |
User-agent: |
Mutt/1.10.1 (2018-07-13) |
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)