* [PATCH v3 0/1] Add support for grub-emu to kexec Linux menu entries
@ 2022-08-23 21:15 Robbie Harwood
2022-08-23 21:15 ` [PATCH v3 1/1] " Robbie Harwood
0 siblings, 1 reply; 4+ messages in thread
From: Robbie Harwood @ 2022-08-23 21:15 UTC (permalink / raw)
To: grub-devel; +Cc: Robbie Harwood, dkiper, rw
This version syncs with the openSUSE version of the patch. Interdiff
attached.
Be well,
--Robbie
Raymund Will (1):
Add support for grub-emu to kexec Linux menu entries
grub-core/Makefile.am | 1 +
grub-core/Makefile.core.def | 2 +-
grub-core/kern/emu/main.c | 4 +
grub-core/kern/emu/misc.c | 18 +++-
grub-core/loader/emu/linux.c | 180 +++++++++++++++++++++++++++++++++++
include/grub/emu/exec.h | 4 +-
include/grub/emu/hostfile.h | 3 +-
include/grub/emu/misc.h | 3 +
8 files changed, 211 insertions(+), 4 deletions(-)
create mode 100644 grub-core/loader/emu/linux.c
Interdiff against v2:
diff --git a/grub-core/loader/emu/linux.c b/grub-core/loader/emu/linux.c
index 020cf56cd1..b5ea48a677 100644
--- a/grub-core/loader/emu/linux.c
+++ b/grub-core/loader/emu/linux.c
@@ -38,7 +38,7 @@ 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 *kexec[] = { "kexec", "-la", kernel_path, boot_cmdline, NULL, NULL };
const char *systemctl[] = { "systemctl", "kexec", NULL };
int kexecute = grub_util_get_kexecute ();
@@ -53,7 +53,7 @@ grub_linux_boot (void)
initrd_param = grub_xasprintf ("%s", "");
}
- grub_dprintf ("linux", "%serforming 'kexec -l %s %s %s'\n",
+ grub_dprintf ("linux", "%serforming 'kexec -la %s %s %s'\n",
(kexecute) ? "P" : "Not p",
kernel_path, initrd_param, boot_cmdline);
@@ -76,10 +76,8 @@ grub_linux_boot (void)
(kexecute==1) ? "do-or-die" : "just-in-case");
rc = grub_util_exec (systemctl);
- if (rc == GRUB_ERR_NONE)
- return rc;
-
- grub_error (rc, N_("Error trying to perform 'systemctl kexec'"));
+ if (kexecute == 1)
+ 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'");
--
2.35.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH v3 1/1] Add support for grub-emu to kexec Linux menu entries
2022-08-23 21:15 [PATCH v3 0/1] Add support for grub-emu to kexec Linux menu entries Robbie Harwood
@ 2022-08-23 21:15 ` Robbie Harwood
2022-08-24 10:35 ` Raymund Will
0 siblings, 1 reply; 4+ messages in thread
From: Robbie Harwood @ 2022-08-23 21:15 UTC (permalink / raw)
To: grub-devel
Cc: Raymund Will, dkiper, rw, John Jolly, Javier Martinez Canillas,
Robbie Harwood
From: Raymund Will <rw@suse.com>
The GRUB emulator is used as a debugging utility but it could also be used
as a user-space bootloader if there is support to boot an operating system.
The Linux kernel is already able to (re)boot another kernel via the kexec
boot mechanism. So the grub-emu tool could rely on this feature and have
linux and initrd commands that are used to pass a kernel, initramfs image
and command line parameters to kexec for booting a selected menu entry.
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.
Signed-off-by: Raymund Will <rw@suse.com>
Signed-off-by: John Jolly <jjolly@suse.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Signed-off-by: Robbie Harwood <rharwood@redhat.com>
---
grub-core/Makefile.am | 1 +
grub-core/Makefile.core.def | 2 +-
grub-core/kern/emu/main.c | 4 +
grub-core/kern/emu/misc.c | 18 +++-
grub-core/loader/emu/linux.c | 180 +++++++++++++++++++++++++++++++++++
include/grub/emu/exec.h | 4 +-
include/grub/emu/hostfile.h | 3 +-
include/grub/emu/misc.h | 3 +
8 files changed, 211 insertions(+), 4 deletions(-)
create mode 100644 grub-core/loader/emu/linux.c
diff --git a/grub-core/Makefile.am b/grub-core/Makefile.am
index ee88e44e97..80e7a83edf 100644
--- a/grub-core/Makefile.am
+++ b/grub-core/Makefile.am
@@ -307,6 +307,7 @@ KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/emu/net.h
KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/emu/hostdisk.h
KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/emu/hostfile.h
KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/extcmd.h
+KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/emu/exec.h
if COND_GRUB_EMU_SDL
KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/sdl.h
endif
diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index 7159948721..5350408601 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -1816,9 +1816,9 @@ module = {
arm64 = loader/arm64/linux.c;
riscv32 = loader/riscv/linux.c;
riscv64 = loader/riscv/linux.c;
+ emu = loader/emu/linux.c;
common = loader/linux.c;
common = lib/cmdline.c;
- enable = noemu;
};
module = {
diff --git a/grub-core/kern/emu/main.c b/grub-core/kern/emu/main.c
index 44e087e988..b4da9d21cf 100644
--- a/grub-core/kern/emu/main.c
+++ b/grub-core/kern/emu/main.c
@@ -107,6 +107,7 @@ static struct argp_option options[] = {
N_("use GRUB files in the directory DIR [default=%s]"), 0},
{"verbose", 'v', 0, 0, N_("print verbose messages."), 0},
{"hold", 'H', N_("SECS"), OPTION_ARG_OPTIONAL, N_("wait until a debugger will attach"), 0},
+ {"kexec", 'X', 0, 0, N_("use kexec to boot Linux kernels."), 0},
{ 0, 0, 0, 0, 0, 0 }
};
@@ -164,6 +165,9 @@ argp_parser (int key, char *arg, struct argp_state *state)
case 'v':
verbosity++;
break;
+ case 'X':
+ grub_util_set_kexecute ();
+ break;
case ARGP_KEY_ARG:
{
diff --git a/grub-core/kern/emu/misc.c b/grub-core/kern/emu/misc.c
index d0e7a107e7..521220b49d 100644
--- a/grub-core/kern/emu/misc.c
+++ b/grub-core/kern/emu/misc.c
@@ -39,6 +39,7 @@
#include <grub/emu/misc.h>
int verbosity;
+int kexecute;
void
grub_util_warn (const char *fmt, ...)
@@ -82,7 +83,7 @@ grub_util_error (const char *fmt, ...)
vfprintf (stderr, fmt, ap);
va_end (ap);
fprintf (stderr, ".\n");
- exit (1);
+ grub_exit ();
}
void *
@@ -153,6 +154,9 @@ xasprintf (const char *fmt, ...)
void
grub_exit (void)
{
+#if defined (GRUB_KERNEL)
+ grub_reboot ();
+#endif
exit (1);
}
#endif
@@ -214,3 +218,15 @@ grub_util_load_image (const char *path, char *buf)
fclose (fp);
}
+
+void
+grub_util_set_kexecute (void)
+{
+ kexecute++;
+}
+
+int
+grub_util_get_kexecute (void)
+{
+ return kexecute;
+}
diff --git a/grub-core/loader/emu/linux.c b/grub-core/loader/emu/linux.c
new file mode 100644
index 0000000000..b5ea48a677
--- /dev/null
+++ b/grub-core/loader/emu/linux.c
@@ -0,0 +1,180 @@
+/*
+ * GRUB -- GRand Unified Bootloader
+ * Copyright (C) 2006,2007,2008,2009,2010 Free Software Foundation, Inc.
+ *
+ * GRUB is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * GRUB is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with GRUB. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <grub/loader.h>
+#include <grub/dl.h>
+#include <grub/command.h>
+#include <grub/time.h>
+
+#include <grub/emu/exec.h>
+#include <grub/emu/hostfile.h>
+#include <grub/emu/misc.h>
+
+GRUB_MOD_LICENSE ("GPLv3+");
+
+static grub_dl_t my_mod;
+
+static char *kernel_path;
+static char *initrd_path;
+static char *boot_cmdline;
+
+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 };
+ 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);
+
+ 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'"));
+
+ /* need to check read-only root before resetting hard!? */
+ grub_dprintf ("linux", "Performing 'kexec -e -x'");
+ kexec[1] = "-e";
+ kexec[2] = "-x";
+ kexec[3] = NULL;
+ rc = grub_util_exec (kexec);
+ if ( rc != GRUB_ERR_NONE )
+ grub_fatal (N_("Error trying to directly perform 'kexec -e'."));
+
+ return rc;
+}
+
+static grub_err_t
+grub_linux_unload (void)
+{
+ grub_dl_unref (my_mod);
+ if ( boot_cmdline != NULL )
+ grub_free (boot_cmdline);
+ boot_cmdline = NULL;
+ return GRUB_ERR_NONE;
+}
+
+static grub_err_t
+grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)), int argc, char *argv[])
+{
+ int i;
+ char *tempstr;
+
+ grub_dl_ref (my_mod);
+
+ if (argc == 0)
+ return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
+
+ if ( !grub_util_is_regular (argv[0]) )
+ return grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("Cannot find kernel file %s"), argv[0]);
+
+ if ( kernel_path != NULL )
+ grub_free (kernel_path);
+
+ kernel_path = grub_xasprintf ("%s", argv[0]);
+
+ if ( boot_cmdline != NULL )
+ {
+ grub_free(boot_cmdline);
+ boot_cmdline = NULL;
+ }
+
+ if ( argc > 1 )
+ {
+ boot_cmdline = grub_xasprintf("--command-line=%s", argv[1]);
+ for ( i = 2; i < argc; i++ )
+ {
+ tempstr = grub_xasprintf("%s %s", boot_cmdline, argv[i]);
+ grub_free(boot_cmdline);
+ boot_cmdline = tempstr;
+ }
+ }
+
+ grub_loader_set (grub_linux_boot, grub_linux_unload, 0);
+
+ return GRUB_ERR_NONE;
+}
+
+static grub_err_t
+grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)), int argc, char *argv[])
+{
+ if (argc == 0)
+ return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
+
+ if ( !grub_util_is_regular (argv[0]) )
+ return grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("Cannot find initrd file %s"), argv[0]);
+
+ if ( initrd_path != NULL )
+ grub_free (initrd_path);
+
+ initrd_path = grub_xasprintf("%s", argv[0]);
+
+ grub_dl_unref (my_mod);
+
+ return GRUB_ERR_NONE;
+}
+
+static grub_command_t cmd_linux, cmd_initrd;
+
+GRUB_MOD_INIT (linux)
+{
+ cmd_linux = grub_register_command ("linux", grub_cmd_linux, 0, N_("Load Linux."));
+ cmd_initrd = grub_register_command ("initrd", grub_cmd_initrd, 0, N_("Load initrd."));
+ my_mod = mod;
+ kernel_path = NULL;
+ initrd_path = NULL;
+ boot_cmdline = NULL;
+}
+
+GRUB_MOD_FINI (linux)
+{
+ grub_unregister_command (cmd_linux);
+ grub_unregister_command (cmd_initrd);
+}
diff --git a/include/grub/emu/exec.h b/include/grub/emu/exec.h
index d1073ef86a..1b61b4a2e5 100644
--- a/include/grub/emu/exec.h
+++ b/include/grub/emu/exec.h
@@ -23,6 +23,8 @@
#include <stdarg.h>
#include <sys/types.h>
+#include <grub/symbol.h>
+
pid_t
grub_util_exec_pipe (const char *const *argv, int *fd);
pid_t
@@ -32,7 +34,7 @@ int
grub_util_exec_redirect_all (const char *const *argv, const char *stdin_file,
const char *stdout_file, const char *stderr_file);
int
-grub_util_exec (const char *const *argv);
+EXPORT_FUNC(grub_util_exec) (const char *const *argv);
int
grub_util_exec_redirect (const char *const *argv, const char *stdin_file,
const char *stdout_file);
diff --git a/include/grub/emu/hostfile.h b/include/grub/emu/hostfile.h
index cfb1e2b566..a61568e36e 100644
--- a/include/grub/emu/hostfile.h
+++ b/include/grub/emu/hostfile.h
@@ -22,6 +22,7 @@
#include <grub/disk.h>
#include <grub/partition.h>
#include <sys/types.h>
+#include <grub/symbol.h>
#include <grub/osdep/hostfile.h>
int
@@ -29,7 +30,7 @@ grub_util_is_directory (const char *path);
int
grub_util_is_special_file (const char *path);
int
-grub_util_is_regular (const char *path);
+EXPORT_FUNC(grub_util_is_regular) (const char *path);
char *
grub_util_path_concat (size_t n, ...);
diff --git a/include/grub/emu/misc.h b/include/grub/emu/misc.h
index ff9c48a649..01056954b9 100644
--- a/include/grub/emu/misc.h
+++ b/include/grub/emu/misc.h
@@ -57,6 +57,9 @@ void EXPORT_FUNC(grub_util_warn) (const char *fmt, ...) __attribute__ ((format (
void EXPORT_FUNC(grub_util_info) (const char *fmt, ...) __attribute__ ((format (GNU_PRINTF, 1, 2)));
void EXPORT_FUNC(grub_util_error) (const char *fmt, ...) __attribute__ ((format (GNU_PRINTF, 1, 2), noreturn));
+void EXPORT_FUNC(grub_util_set_kexecute) (void);
+int EXPORT_FUNC(grub_util_get_kexecute) (void) WARN_UNUSED_RESULT;
+
grub_uint64_t EXPORT_FUNC (grub_util_get_cpu_time_ms) (void);
#ifdef HAVE_DEVICE_MAPPER
--
2.35.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v3 1/1] Add support for grub-emu to kexec Linux menu entries
2022-08-23 21:15 ` [PATCH v3 1/1] " Robbie Harwood
@ 2022-08-24 10:35 ` Raymund Will
2022-08-24 14:29 ` Robbie Harwood
0 siblings, 1 reply; 4+ messages in thread
From: Raymund Will @ 2022-08-24 10:35 UTC (permalink / raw)
To: Robbie Harwood; +Cc: grub-devel, dkiper, Javier Martinez Canillas
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)
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3 1/1] Add support for grub-emu to kexec Linux menu entries
2022-08-24 10:35 ` Raymund Will
@ 2022-08-24 14:29 ` Robbie Harwood
0 siblings, 0 replies; 4+ messages in thread
From: Robbie Harwood @ 2022-08-24 14:29 UTC (permalink / raw)
To: Raymund Will; +Cc: grub-devel, dkiper, Javier Martinez Canillas
[-- Attachment #1: Type: text/plain, Size: 4185 bytes --]
Raymund Will <rw@suse.de> writes:
> 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.
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-08-24 14:29 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-23 21:15 [PATCH v3 0/1] Add support for grub-emu to kexec Linux menu entries Robbie Harwood
2022-08-23 21:15 ` [PATCH v3 1/1] " Robbie Harwood
2022-08-24 10:35 ` Raymund Will
2022-08-24 14:29 ` Robbie Harwood
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.